[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.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.

-Scott