diff mbox

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

Message ID 1453347252-1579-2-git-send-email-Qianyu.Gong@nxp.com
State Superseded
Delegated to: York Sun
Headers show

Commit Message

Gong Qianyu Jan. 21, 2016, 3:34 a.m. UTC 
From: Gong Qianyu <Qianyu.Gong@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@freescale.com>
---
V2-V5:
 - No change.
 
 drivers/spi/fsl_qspi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Scott Wood Jan. 21, 2016, 7:30 p.m. UTC | #1 
On 01/20/2016 09:43 PM, Gong Qianyu wrote:
> From: Gong Qianyu <Qianyu.Gong@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@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
Gong Qianyu Jan. 22, 2016, 3:35 a.m. UTC | #2 
> -----Original Message-----
> From: Scott Wood
> Sent: Friday, January 22, 2016 3:30 AM
> To: Qianyu Gong <qianyu.gong@nxp.com>; u-boot@lists.denx.de;
> R58495@freescale.com
> Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com;
> Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood
> <oss@buserror.net>; Gong Qianyu <Qianyu.Gong@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@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@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++;
                }
        }


Regards,
Qianyu
Scott Wood Jan. 22, 2016, 3:43 p.m. UTC | #3 
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@nxp.com>; u-boot@lists.denx.de;
>> R58495@freescale.com
>> Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com;
>> Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood
>> <oss@buserror.net>; Gong Qianyu <Qianyu.Gong@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@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@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.

-Scott
York Sun Jan. 22, 2016, 8:15 p.m. UTC | #4 
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@nxp.com>; u-boot@lists.denx.de;
>>> R58495@freescale.com
>>> Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com;
>>> Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood
>>> <oss@buserror.net>; Gong Qianyu <Qianyu.Gong@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@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@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
diff mbox

Patch

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);
 			rxbuf++;
 			size -= 4;
 			i++;