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

[Scott Wood]

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, &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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot