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

[york sun]

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.

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

York