[U-Boot] [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue

Qianyu Gong qianyu.gong at nxp.com
Tue Jan 26 08:16:37 CET 2016


> -----Original Message-----
> From: york sun
> Sent: Tuesday, January 26, 2016 1:02 AM
> To: Scott Wood <scott.wood at nxp.com>; Yao Yuan <yao.yuan at nxp.com>;
> Qianyu Gong <qianyu.gong at nxp.com>
> Cc: B48286 at freescale.com; u-boot at lists.denx.de; Wenbin.Song at freescale.com;
> jteki at openedev.com
> Subject: Re: [U-Boot] [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
> 
> 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.gong at nxp.com>; u-boot at lists.denx.de;
> >>>>>>> R58495 at freescale.com
> >>>>>>> Cc: Mingkai.Hu at freescale.com; jteki at openedev.com;
> >>>>>>> B48286 at freescale.com; Shaohui.Xie at freescale.com;
> >>>>>>> Wenbin.Song at freescale.com; Scott Wood <oss at buserror.net>; Gong
> >>>>>>> Qianyu <Qianyu.Gong at 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.Gong at 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.Gong at 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, &regs->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, &regs->rbsr);
> >>>>>>                 if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) {
> >>>>>>                         data = qspi_read32(priv->flags, &regs->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, &regs->mcr);
> >>>         qspi_write32(priv->flags, &regs->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, &regs->rbsr);
> >>>                 if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) {
> >>>                         data = qspi_read32(priv->flags, &regs->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
> 

Great. Thank you all. 
I send out a V7 patch set to fix it.


Regards,
Qianyu


More information about the U-Boot mailing list