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

[york sun]

On 01/25/2016 09:01 AM, Scott Wood wrote:
> 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;
> 

Agree.

York