On 01/24/2016 08:09 PM, Yao Yuan wrote: > On 01/25/2016 04:16 AM, York Sun wrote: >> On 01/22/2016 07:43 AM, Scott Wood wrote: >>> On 01/21/2016 09:35 PM, Qianyu Gong wrote: >>>> >>>>> -----Original Message----- >>>>> From: Scott Wood >>>>> Sent: Friday, January 22, 2016 3:30 AM >>>>> To: Qianyu Gong <qianyu.g...@nxp.com>; u-boot@lists.denx.de; >>>>> r58...@freescale.com >>>>> Cc: mingkai...@freescale.com; jt...@openedev.com; >>>>> b48...@freescale.com; shaohui....@freescale.com; >>>>> wenbin.s...@freescale.com; Scott Wood <o...@buserror.net>; Gong >>>>> Qianyu <qianyu.g...@freescale.com> >>>>> Subject: Re: [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy >>>>> issue >>>>> >>>>> On 01/20/2016 09:43 PM, Gong Qianyu wrote: >>>>>> From: Gong Qianyu <qianyu.g...@freescale.com> >>>>>> >>>>>> In current driver everytime we memcpy 4 bytes to the dest memory >>>>>> regardless of the remaining length. >>>>>> This patch adds checking the remaining length before memcpy. >>>>>> If the length is shorter than 4 bytes, memcpy the actual length of >>>>>> data to the dest memory. >>>>>> >>>>>> Signed-off-by: Gong Qianyu <qianyu.g...@freescale.com> >>>>>> --- >>>>>> V2-V5: >>>>>> - No change. >>>>>> >>>>>> drivers/spi/fsl_qspi.c | 5 ++++- >>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index >>>>>> 38e5900..f178857 100644 >>>>>> --- a/drivers/spi/fsl_qspi.c >>>>>> +++ b/drivers/spi/fsl_qspi.c >>>>>> @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv >>>>>> *priv, u32 >>>>> *rxbuf, u32 len) >>>>>> if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { >>>>>> data = qspi_read32(priv->flags, ®s->rbdr[i]); >>>>>> data = qspi_endian_xchg(data); >>>>>> - memcpy(rxbuf, &data, 4); >>>>>> + if (size < 4) >>>>>> + memcpy(rxbuf, &data, size); >>>>>> + else >>>>>> + memcpy(rxbuf, &data, 4); >>>>> >>>>> memcpy(rxbuf, &data, min(size, 4)); >>>>> >>>>>> rxbuf++; >>>>>> size -= 4; >>>>>> i++; >>>>> >>>>> size -= 4 even if size was < 4? >>>>> >>>>> -Scott >>>> >>>> Yes.. The following is complete code: >>>> >>>> i = 0; >>>> size = len; >>>> while ((RX_BUFFER_SIZE >= size) && (size > 0)) { >>>> rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); >>>> if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { >>>> data = qspi_read32(priv->flags, ®s->rbdr[i]); >>>> data = qspi_endian_xchg(data); >>>> memcpy(rxbuf, &data, min(size, 4)); >>>> rxbuf++; >>>> size -= 4; >>>> i++; >>>> } >>>> } >>> >>> I'm not saying it doesn't work (assuming i is signed, which the >>> "complete code" above doesn't show). I'm saying it looks weird, and >>> it would be better to have a variable that holds min(size, 4) and pass >>> that to both memcpy and the subtraction. >>> >> >> Qianyu, >> >> Previously I said it looked weird for doing this. Please fix. >> >> "size" is declared as "int". >> "len" is declared as u32. That's not "int". If you trace back the functions, >> you >> may see it came from DIV_ROUND_UP(bitlen, 8) where bitlen is "unsigned int". >> So technically the code is safe. But it is _confusing_. We don't want to >> confuse >> ourselves when reading the code later. And the fix is easy, isn't it? >> >> York >> > > How about like this? > > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c > index feec3e8..13bba09 100644 > --- a/drivers/spi/fsl_qspi.c > +++ b/drivers/spi/fsl_qspi.c > @@ -477,8 +477,8 @@ static void qspi_op_rdbank(struct fsl_qspi_priv *priv, u8 > *rxbuf, u32 len) > static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len) > { > struct fsl_qspi_regs *regs = priv->regs; > - u32 mcr_reg, rbsr_reg, data; > - int i, size; > + u32 mcr_reg, rbsr_reg, data, size; > + int i; > > mcr_reg = qspi_read32(priv->flags, ®s->mcr); > qspi_write32(priv->flags, ®s->mcr, > @@ -495,14 +495,14 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, > u32 *rxbuf, u32 len) > > i = 0; > size = len; > - while ((RX_BUFFER_SIZE >= size) && (size > 0)) { > + while ((RX_BUFFER_SIZE >= size) && (size != 0)) {
You can keep using "size > 0". It is still correct. > rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); > if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { > data = qspi_read32(priv->flags, ®s->rbdr[i]); > data = qspi_endian_xchg(data); > - memcpy(rxbuf, &data, 4); > + memcpy(rxbuf, &data, min(size, 4)); > + size = (size < 4) ? 0 : ( size - 4); > rxbuf++; > - size -= 4; > i++; > } > } > The rest looks OK to me. York _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot