[U-Boot] [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
Qianyu Gong
qianyu.gong at nxp.com
Tue Jan 26 08:16:37 CET 2016
> -----Original Message-----
> From: york sun
> Sent: Tuesday, January 26, 2016 1:02 AM
> To: Scott Wood <scott.wood at nxp.com>; Yao Yuan <yao.yuan at nxp.com>;
> Qianyu Gong <qianyu.gong at nxp.com>
> Cc: B48286 at freescale.com; u-boot at lists.denx.de; Wenbin.Song at freescale.com;
> jteki at openedev.com
> Subject: Re: [U-Boot] [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
>
> 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, ®s->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, ®s->rbsr);
> >>>>>> if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) {
> >>>>>> data = qspi_read32(priv->flags, ®s->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, ®s->mcr);
> >>> qspi_write32(priv->flags, ®s->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, ®s->rbsr);
> >>> if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) {
> >>> data = qspi_read32(priv->flags, ®s->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
>
Great. Thank you all.
I send out a V7 patch set to fix it.
Regards,
Qianyu
More information about the U-Boot
mailing list