On 01/25/2016 09:01 AM, Scott Wood wrote: > On 01/25/2016 10:47 AM, york sun wrote: >> 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. > > And more robust. > >> >>> 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. > > It's still awkward compared to: > > int chunk; > > ... > > chunk = min(size, 4); > memcpy(rxbuf, &data, chunk); > size -= chunk; >
Agree. York _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot