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

[Yao Yuan]

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)) {
                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++;
                }
        }