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

[Scott Wood]

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;

-Scott