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)) { 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++; } } _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot