[PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[Cyrille Pitchen]

spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
When the m25p80 driver is used (pretty common case), nor->read() is then
implemented by the m25p80_read() function, which is likely to initialize a
'struct spi_transfer' from its buf argument before appending this
structure inside the 'struct spi_message' argument of spi_sync().

Besides the SPI sub-system states that both .tx_buf and .rx_buf members of
'struct spi_transfer' must point into dma-safe memory. However, two of the
three calls of spi_nor_read_sfdp() were given pointers to stack allocated
memory as buf argument, hence not in a dma-safe area.
Hopefully, the third and last call of spi_nor_read_sfdp() was already
given a kmalloc'ed buffer argument, hence dma-safe.

So this patch fixes this issue by introducing a
spi_nor_read_sfdp_dma_unsafe() function which simply wraps the existing
spi_nor_read_sfdp() function and uses some kmalloc'ed memory as a bounce
buffer.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
---

Compiled but not tested yet!

 drivers/mtd/spi-nor/spi-nor.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index cf1d4a15e10a..05254dd6a4a0 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1784,7 +1784,7 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
  * @nor:	pointer to a 'struct spi_nor'
  * @addr:	offset in the SFDP area to start reading data from
  * @len:	number of bytes to read
- * @buf:	buffer where the SFDP data are copied into
+ * @buf:	buffer where the SFDP data are copied into (dma-safe memory)
  *
  * Whatever the actual numbers of bytes for address and dummy cycles are
  * for (Fast) Read commands, the Read SFDP (5Ah) instruction is always
@@ -1829,6 +1829,36 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
 	return ret;
 }
 
+/**
+ * spi_nor_read_sfdp_dma_unsafe() - read Serial Flash Discoverable Parameters.
+ * @nor:	pointer to a 'struct spi_nor'
+ * @addr:	offset in the SFDP area to start reading data from
+ * @len:	number of bytes to read
+ * @buf:	buffer where the SFDP data are copied into
+ *
+ * Wrap spi_nor_read_sfdp() using a kmalloc'ed bounce buffer as @buf is now not
+ * guaranteed to be dma-safe.
+ *
+ * Return: -ENOMEM if kmalloc() fails, the return code of spi_nor_read_sfdp()
+ *          otherwise.
+ */
+static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
+					size_t len, void *buf)
+{
+	void *dma_safe_buf;
+	int ret;
+
+	dma_safe_buf = kmalloc(len, GFP_KERNEL);
+	if (!dma_safe_buf)
+		return -ENOMEM;
+
+	ret = spi_nor_read_sfdp(nor, addr, len, dma_safe_buf);
+	memcpy(buf, dma_safe_buf, len);
+	kfree(dma_safe_buf);
+
+	return ret;
+}
+
 struct sfdp_parameter_header {
 	u8		id_lsb;
 	u8		minor;
@@ -2101,7 +2131,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 		    bfpt_header->length * sizeof(u32));
 	addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
 	memset(&bfpt, 0, sizeof(bfpt));
-	err = spi_nor_read_sfdp(nor,  addr, len, &bfpt);
+	err = spi_nor_read_sfdp_dma_unsafe(nor,  addr, len, &bfpt);
 	if (err < 0)
 		return err;
 
@@ -2243,7 +2273,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 	int i, err;
 
 	/* Get the SFDP header. */
-	err = spi_nor_read_sfdp(nor, 0, sizeof(header), &header);
+	err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sizeof(header), &header);
 	if (err < 0)
 		return err;
 
-- 
2.11.0

Re: [PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[Cyrille Pitchen]

Le 06/09/2017 à 23:45, Cyrille Pitchen a écrit :
> spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
> When the m25p80 driver is used (pretty common case), nor->read() is then
> implemented by the m25p80_read() function, which is likely to initialize a
> 'struct spi_transfer' from its buf argument before appending this
> structure inside the 'struct spi_message' argument of spi_sync().
> 
> Besides the SPI sub-system states that both .tx_buf and .rx_buf members of
> 'struct spi_transfer' must point into dma-safe memory. However, two of the
> three calls of spi_nor_read_sfdp() were given pointers to stack allocated
> memory as buf argument, hence not in a dma-safe area.
> Hopefully, the third and last call of spi_nor_read_sfdp() was already
> given a kmalloc'ed buffer argument, hence dma-safe.
> 
> So this patch fixes this issue by introducing a
> spi_nor_read_sfdp_dma_unsafe() function which simply wraps the existing
> spi_nor_read_sfdp() function and uses some kmalloc'ed memory as a bounce
> buffer.
> 
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> ---
> 
> Compiled but not tested yet!

tested on a sama5d2 xplained board with:
- an Adesto at25df321a on spi0 (using the m25p80.c driver)
- a Macronix mx25l25673g on qspi0 (using the atmel-quadspi.c driver)

applied on the spi-nor/next branch of l2-mtd

should be quickly sent as a fix to the MTD pull-request for 4.14

Sorry for that!

> 
>  drivers/mtd/spi-nor/spi-nor.c | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index cf1d4a15e10a..05254dd6a4a0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1784,7 +1784,7 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
>   * @nor:	pointer to a 'struct spi_nor'
>   * @addr:	offset in the SFDP area to start reading data from
>   * @len:	number of bytes to read
> - * @buf:	buffer where the SFDP data are copied into
> + * @buf:	buffer where the SFDP data are copied into (dma-safe memory)
>   *
>   * Whatever the actual numbers of bytes for address and dummy cycles are
>   * for (Fast) Read commands, the Read SFDP (5Ah) instruction is always
> @@ -1829,6 +1829,36 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>  	return ret;
>  }
>  
> +/**
> + * spi_nor_read_sfdp_dma_unsafe() - read Serial Flash Discoverable Parameters.
> + * @nor:	pointer to a 'struct spi_nor'
> + * @addr:	offset in the SFDP area to start reading data from
> + * @len:	number of bytes to read
> + * @buf:	buffer where the SFDP data are copied into
> + *
> + * Wrap spi_nor_read_sfdp() using a kmalloc'ed bounce buffer as @buf is now not
> + * guaranteed to be dma-safe.
> + *
> + * Return: -ENOMEM if kmalloc() fails, the return code of spi_nor_read_sfdp()
> + *          otherwise.
> + */
> +static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
> +					size_t len, void *buf)
> +{
> +	void *dma_safe_buf;
> +	int ret;
> +
> +	dma_safe_buf = kmalloc(len, GFP_KERNEL);
> +	if (!dma_safe_buf)
> +		return -ENOMEM;
> +
> +	ret = spi_nor_read_sfdp(nor, addr, len, dma_safe_buf);
> +	memcpy(buf, dma_safe_buf, len);
> +	kfree(dma_safe_buf);
> +
> +	return ret;
> +}
> +
>  struct sfdp_parameter_header {
>  	u8		id_lsb;
>  	u8		minor;
> @@ -2101,7 +2131,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  		    bfpt_header->length * sizeof(u32));
>  	addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
>  	memset(&bfpt, 0, sizeof(bfpt));
> -	err = spi_nor_read_sfdp(nor,  addr, len, &bfpt);
> +	err = spi_nor_read_sfdp_dma_unsafe(nor,  addr, len, &bfpt);
>  	if (err < 0)
>  		return err;
>  
> @@ -2243,7 +2273,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  	int i, err;
>  
>  	/* Get the SFDP header. */
> -	err = spi_nor_read_sfdp(nor, 0, sizeof(header), &header);
> +	err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sizeof(header), &header);
>  	if (err < 0)
>  		return err;
>  
> 

Re: [PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[Boris Brezillon]

On Wed,  6 Sep 2017 23:45:02 +0200
Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote:

> spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
> When the m25p80 driver is used (pretty common case), nor->read() is then
> implemented by the m25p80_read() function, which is likely to initialize a
> 'struct spi_transfer' from its buf argument before appending this
> structure inside the 'struct spi_message' argument of spi_sync().
> 
> Besides the SPI sub-system states that both .tx_buf and .rx_buf members of
> 'struct spi_transfer' must point into dma-safe memory. However, two of the
> three calls of spi_nor_read_sfdp() were given pointers to stack allocated
> memory as buf argument, hence not in a dma-safe area.
> Hopefully, the third and last call of spi_nor_read_sfdp() was already
> given a kmalloc'ed buffer argument, hence dma-safe.
> 
> So this patch fixes this issue by introducing a
> spi_nor_read_sfdp_dma_unsafe() function which simply wraps the existing
> spi_nor_read_sfdp() function and uses some kmalloc'ed memory as a bounce
> buffer.
> 
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

Missing

Fixes: f384b352cbf0310f ("mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables")

> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> ---
> 
> Compiled but not tested yet!
> 
>  drivers/mtd/spi-nor/spi-nor.c | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index cf1d4a15e10a..05254dd6a4a0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1784,7 +1784,7 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
>   * @nor:	pointer to a 'struct spi_nor'
>   * @addr:	offset in the SFDP area to start reading data from
>   * @len:	number of bytes to read
> - * @buf:	buffer where the SFDP data are copied into
> + * @buf:	buffer where the SFDP data are copied into (dma-safe memory)
>   *
>   * Whatever the actual numbers of bytes for address and dummy cycles are
>   * for (Fast) Read commands, the Read SFDP (5Ah) instruction is always
> @@ -1829,6 +1829,36 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>  	return ret;
>  }
>  
> +/**
> + * spi_nor_read_sfdp_dma_unsafe() - read Serial Flash Discoverable Parameters.
> + * @nor:	pointer to a 'struct spi_nor'
> + * @addr:	offset in the SFDP area to start reading data from
> + * @len:	number of bytes to read
> + * @buf:	buffer where the SFDP data are copied into
> + *
> + * Wrap spi_nor_read_sfdp() using a kmalloc'ed bounce buffer as @buf is now not
> + * guaranteed to be dma-safe.
> + *
> + * Return: -ENOMEM if kmalloc() fails, the return code of spi_nor_read_sfdp()
> + *          otherwise.
> + */
> +static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
> +					size_t len, void *buf)
> +{
> +	void *dma_safe_buf;
> +	int ret;
> +
> +	dma_safe_buf = kmalloc(len, GFP_KERNEL);
> +	if (!dma_safe_buf)
> +		return -ENOMEM;
> +
> +	ret = spi_nor_read_sfdp(nor, addr, len, dma_safe_buf);
> +	memcpy(buf, dma_safe_buf, len);
> +	kfree(dma_safe_buf);
> +
> +	return ret;
> +}

Hm, do we really need to add this function? I would just kmalloc the bfpt
and header objects in spi_nor_parse_bfpt(), which would avoid the extra
heap-to-stack copy and also simplify this patch.

I understand that you want to generically address the problem, but AFAICT
this patch is not doing that since the user has to explicitly call
spi_nor_read_sfdp_dma_unsafe(), and I'm not even sure
spi_nor_read_sfdp_dma_unsafe() can/will be re-used in the generic solution
you envision.

Let's try to keep the fix as simple as possible and think about a better
approach afterwards.

> +
>  struct sfdp_parameter_header {
>  	u8		id_lsb;
>  	u8		minor;
> @@ -2101,7 +2131,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  		    bfpt_header->length * sizeof(u32));
>  	addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
>  	memset(&bfpt, 0, sizeof(bfpt));
> -	err = spi_nor_read_sfdp(nor,  addr, len, &bfpt);
> +	err = spi_nor_read_sfdp_dma_unsafe(nor,  addr, len, &bfpt);
>  	if (err < 0)
>  		return err;
>  
> @@ -2243,7 +2273,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  	int i, err;
>  
>  	/* Get the SFDP header. */
> -	err = spi_nor_read_sfdp(nor, 0, sizeof(header), &header);
> +	err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sizeof(header), &header);
>  	if (err < 0)
>  		return err;
>  

Re: [PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[Boris Brezillon]

On Thu, 7 Sep 2017 00:50:12 +0200
Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote:

> Le 06/09/2017 à 23:45, Cyrille Pitchen a écrit :
> > spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
> > When the m25p80 driver is used (pretty common case), nor->read() is then
> > implemented by the m25p80_read() function, which is likely to initialize a
> > 'struct spi_transfer' from its buf argument before appending this
> > structure inside the 'struct spi_message' argument of spi_sync().
> > 
> > Besides the SPI sub-system states that both .tx_buf and .rx_buf members of
> > 'struct spi_transfer' must point into dma-safe memory. However, two of the
> > three calls of spi_nor_read_sfdp() were given pointers to stack allocated
> > memory as buf argument, hence not in a dma-safe area.
> > Hopefully, the third and last call of spi_nor_read_sfdp() was already
> > given a kmalloc'ed buffer argument, hence dma-safe.
> > 
> > So this patch fixes this issue by introducing a
> > spi_nor_read_sfdp_dma_unsafe() function which simply wraps the existing
> > spi_nor_read_sfdp() function and uses some kmalloc'ed memory as a bounce
> > buffer.
> > 
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> > ---
> > 
> > Compiled but not tested yet!  
> 
> tested on a sama5d2 xplained board with:
> - an Adesto at25df321a on spi0 (using the m25p80.c driver)
> - a Macronix mx25l25673g on qspi0 (using the atmel-quadspi.c driver)

Cool, that was fast.

> 
> applied on the spi-nor/next branch of l2-mtd

Maybe a bit too fast. You should leave at least one day to
reviewers/testers before applying the patch.

BTW, I was planning on taking the patch directly.

> 
> should be quickly sent as a fix to the MTD pull-request for 4.14
> 
> Sorry for that!
> 
> > 
> >  drivers/mtd/spi-nor/spi-nor.c | 36 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 33 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index cf1d4a15e10a..05254dd6a4a0 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -1784,7 +1784,7 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
> >   * @nor:	pointer to a 'struct spi_nor'
> >   * @addr:	offset in the SFDP area to start reading data from
> >   * @len:	number of bytes to read
> > - * @buf:	buffer where the SFDP data are copied into
> > + * @buf:	buffer where the SFDP data are copied into (dma-safe memory)
> >   *
> >   * Whatever the actual numbers of bytes for address and dummy cycles are
> >   * for (Fast) Read commands, the Read SFDP (5Ah) instruction is always
> > @@ -1829,6 +1829,36 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
> >  	return ret;
> >  }
> >  
> > +/**
> > + * spi_nor_read_sfdp_dma_unsafe() - read Serial Flash Discoverable Parameters.
> > + * @nor:	pointer to a 'struct spi_nor'
> > + * @addr:	offset in the SFDP area to start reading data from
> > + * @len:	number of bytes to read
> > + * @buf:	buffer where the SFDP data are copied into
> > + *
> > + * Wrap spi_nor_read_sfdp() using a kmalloc'ed bounce buffer as @buf is now not
> > + * guaranteed to be dma-safe.
> > + *
> > + * Return: -ENOMEM if kmalloc() fails, the return code of spi_nor_read_sfdp()
> > + *          otherwise.
> > + */
> > +static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
> > +					size_t len, void *buf)
> > +{
> > +	void *dma_safe_buf;
> > +	int ret;
> > +
> > +	dma_safe_buf = kmalloc(len, GFP_KERNEL);
> > +	if (!dma_safe_buf)
> > +		return -ENOMEM;
> > +
> > +	ret = spi_nor_read_sfdp(nor, addr, len, dma_safe_buf);
> > +	memcpy(buf, dma_safe_buf, len);
> > +	kfree(dma_safe_buf);
> > +
> > +	return ret;
> > +}
> > +
> >  struct sfdp_parameter_header {
> >  	u8		id_lsb;
> >  	u8		minor;
> > @@ -2101,7 +2131,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> >  		    bfpt_header->length * sizeof(u32));
> >  	addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
> >  	memset(&bfpt, 0, sizeof(bfpt));
> > -	err = spi_nor_read_sfdp(nor,  addr, len, &bfpt);
> > +	err = spi_nor_read_sfdp_dma_unsafe(nor,  addr, len, &bfpt);
> >  	if (err < 0)
> >  		return err;
> >  
> > @@ -2243,7 +2273,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
> >  	int i, err;
> >  
> >  	/* Get the SFDP header. */
> > -	err = spi_nor_read_sfdp(nor, 0, sizeof(header), &header);
> > +	err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sizeof(header), &header);
> >  	if (err < 0)
> >  		return err;
> >  
> >   
> 

Re: [PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[Geert Uytterhoeven]

Hi Cyrille,

On Wed, Sep 6, 2017 at 11:45 PM, Cyrille Pitchen
<cyrille.pitchen@wedev4u.fr> wrote:
> spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
> When the m25p80 driver is used (pretty common case), nor->read() is then
> implemented by the m25p80_read() function, which is likely to initialize a
> 'struct spi_transfer' from its buf argument before appending this
> structure inside the 'struct spi_message' argument of spi_sync().
>
> Besides the SPI sub-system states that both .tx_buf and .rx_buf members of
> 'struct spi_transfer' must point into dma-safe memory. However, two of the
> three calls of spi_nor_read_sfdp() were given pointers to stack allocated
> memory as buf argument, hence not in a dma-safe area.
> Hopefully, the third and last call of spi_nor_read_sfdp() was already
> given a kmalloc'ed buffer argument, hence dma-safe.
>
> So this patch fixes this issue by introducing a
> spi_nor_read_sfdp_dma_unsafe() function which simply wraps the existing
> spi_nor_read_sfdp() function and uses some kmalloc'ed memory as a bounce
> buffer.
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>

While this patch got rid of the warning, it does not fix the SPI FLASH
identification
issue:

    m25p80 spi0.0: s25fl512s (0 Kbytes)
    3 ofpart partitions found on MTD device spi0.0
    Creating 3 MTD partitions on "spi0.0":
    0x000000000000-0x000000040000 : "loader"
    mtd: partition "loader" is out of reach -- disabled
    0x000000040000-0x000000080000 : "system"
    mtd: partition "system" is out of reach -- disabled
    0x000000080000-0x000004000000 : "user"
    mtd: partition "user" is out of reach -- disabled

I noticed there's still one direct call to spi_nor_read_sfdp() left in
spi_nor_parse_sfdp().
I tried changing that to spi_nor_read_sfdp_dma_unsafe(), but that didn't help.

> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1784,7 +1784,7 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
>   * @nor:       pointer to a 'struct spi_nor'
>   * @addr:      offset in the SFDP area to start reading data from
>   * @len:       number of bytes to read
> - * @buf:       buffer where the SFDP data are copied into
> + * @buf:       buffer where the SFDP data are copied into (dma-safe memory)
>   *
>   * Whatever the actual numbers of bytes for address and dummy cycles are
>   * for (Fast) Read commands, the Read SFDP (5Ah) instruction is always
> @@ -1829,6 +1829,36 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>         return ret;
>  }
>
> +/**
> + * spi_nor_read_sfdp_dma_unsafe() - read Serial Flash Discoverable Parameters.
> + * @nor:       pointer to a 'struct spi_nor'
> + * @addr:      offset in the SFDP area to start reading data from
> + * @len:       number of bytes to read
> + * @buf:       buffer where the SFDP data are copied into
> + *
> + * Wrap spi_nor_read_sfdp() using a kmalloc'ed bounce buffer as @buf is now not
> + * guaranteed to be dma-safe.
> + *
> + * Return: -ENOMEM if kmalloc() fails, the return code of spi_nor_read_sfdp()
> + *          otherwise.
> + */
> +static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
> +                                       size_t len, void *buf)
> +{
> +       void *dma_safe_buf;
> +       int ret;
> +
> +       dma_safe_buf = kmalloc(len, GFP_KERNEL);
> +       if (!dma_safe_buf)
> +               return -ENOMEM;
> +
> +       ret = spi_nor_read_sfdp(nor, addr, len, dma_safe_buf);
> +       memcpy(buf, dma_safe_buf, len);
> +       kfree(dma_safe_buf);
> +
> +       return ret;
> +}
> +
>  struct sfdp_parameter_header {
>         u8              id_lsb;
>         u8              minor;
> @@ -2101,7 +2131,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>                     bfpt_header->length * sizeof(u32));
>         addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
>         memset(&bfpt, 0, sizeof(bfpt));
> -       err = spi_nor_read_sfdp(nor,  addr, len, &bfpt);
> +       err = spi_nor_read_sfdp_dma_unsafe(nor,  addr, len, &bfpt);
>         if (err < 0)
>                 return err;
>
> @@ -2243,7 +2273,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>         int i, err;
>
>         /* Get the SFDP header. */
> -       err = spi_nor_read_sfdp(nor, 0, sizeof(header), &header);
> +       err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sizeof(header), &header);
>         if (err < 0)
>                 return err;
>

Instead of having buffers on the stack, passing them around through multiple
call levels, and then kmalloc()ing a buffer, what about using the helpers in
<linux/spi/spi.h> instead, which take care of the issue through the
static bounce
buffer or kmalloc() themselves?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[Boris Brezillon]

On Thu, 7 Sep 2017 10:00:50 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Cyrille,
> 
> On Wed, Sep 6, 2017 at 11:45 PM, Cyrille Pitchen
> <cyrille.pitchen@wedev4u.fr> wrote:
> > spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
> > When the m25p80 driver is used (pretty common case), nor->read() is then
> > implemented by the m25p80_read() function, which is likely to initialize a
> > 'struct spi_transfer' from its buf argument before appending this
> > structure inside the 'struct spi_message' argument of spi_sync().
> >
> > Besides the SPI sub-system states that both .tx_buf and .rx_buf members of
> > 'struct spi_transfer' must point into dma-safe memory. However, two of the
> > three calls of spi_nor_read_sfdp() were given pointers to stack allocated
> > memory as buf argument, hence not in a dma-safe area.
> > Hopefully, the third and last call of spi_nor_read_sfdp() was already
> > given a kmalloc'ed buffer argument, hence dma-safe.
> >
> > So this patch fixes this issue by introducing a
> > spi_nor_read_sfdp_dma_unsafe() function which simply wraps the existing
> > spi_nor_read_sfdp() function and uses some kmalloc'ed memory as a bounce
> > buffer.
> >
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>  
> 
> While this patch got rid of the warning, it does not fix the SPI FLASH
> identification
> issue:
> 
>     m25p80 spi0.0: s25fl512s (0 Kbytes)
>     3 ofpart partitions found on MTD device spi0.0
>     Creating 3 MTD partitions on "spi0.0":
>     0x000000000000-0x000000040000 : "loader"
>     mtd: partition "loader" is out of reach -- disabled
>     0x000000040000-0x000000080000 : "system"
>     mtd: partition "system" is out of reach -- disabled
>     0x000000080000-0x000004000000 : "user"
>     mtd: partition "user" is out of reach -- disabled
> 
> I noticed there's still one direct call to spi_nor_read_sfdp() left in
> spi_nor_parse_sfdp().
> I tried changing that to spi_nor_read_sfdp_dma_unsafe(), but that didn't help.
> 
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -1784,7 +1784,7 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
> >   * @nor:       pointer to a 'struct spi_nor'
> >   * @addr:      offset in the SFDP area to start reading data from
> >   * @len:       number of bytes to read
> > - * @buf:       buffer where the SFDP data are copied into
> > + * @buf:       buffer where the SFDP data are copied into (dma-safe memory)
> >   *
> >   * Whatever the actual numbers of bytes for address and dummy cycles are
> >   * for (Fast) Read commands, the Read SFDP (5Ah) instruction is always
> > @@ -1829,6 +1829,36 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
> >         return ret;
> >  }
> >
> > +/**
> > + * spi_nor_read_sfdp_dma_unsafe() - read Serial Flash Discoverable Parameters.
> > + * @nor:       pointer to a 'struct spi_nor'
> > + * @addr:      offset in the SFDP area to start reading data from
> > + * @len:       number of bytes to read
> > + * @buf:       buffer where the SFDP data are copied into
> > + *
> > + * Wrap spi_nor_read_sfdp() using a kmalloc'ed bounce buffer as @buf is now not
> > + * guaranteed to be dma-safe.
> > + *
> > + * Return: -ENOMEM if kmalloc() fails, the return code of spi_nor_read_sfdp()
> > + *          otherwise.
> > + */
> > +static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
> > +                                       size_t len, void *buf)
> > +{
> > +       void *dma_safe_buf;
> > +       int ret;
> > +
> > +       dma_safe_buf = kmalloc(len, GFP_KERNEL);
> > +       if (!dma_safe_buf)
> > +               return -ENOMEM;
> > +
> > +       ret = spi_nor_read_sfdp(nor, addr, len, dma_safe_buf);
> > +       memcpy(buf, dma_safe_buf, len);
> > +       kfree(dma_safe_buf);
> > +
> > +       return ret;
> > +}
> > +
> >  struct sfdp_parameter_header {
> >         u8              id_lsb;
> >         u8              minor;
> > @@ -2101,7 +2131,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> >                     bfpt_header->length * sizeof(u32));
> >         addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
> >         memset(&bfpt, 0, sizeof(bfpt));
> > -       err = spi_nor_read_sfdp(nor,  addr, len, &bfpt);
> > +       err = spi_nor_read_sfdp_dma_unsafe(nor,  addr, len, &bfpt);
> >         if (err < 0)
> >                 return err;
> >
> > @@ -2243,7 +2273,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
> >         int i, err;
> >
> >         /* Get the SFDP header. */
> > -       err = spi_nor_read_sfdp(nor, 0, sizeof(header), &header);
> > +       err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sizeof(header), &header);
> >         if (err < 0)
> >                 return err;
> >  
> 
> Instead of having buffers on the stack, passing them around through multiple
> call levels, and then kmalloc()ing a buffer, what about using the helpers in
> <linux/spi/spi.h> instead, which take care of the issue through the
> static bounce
> buffer or kmalloc() themselves?

Are you referring to spi_write_then_read()? If this is the case, I'm not
sure we can use this because m25p80_read/write() can have more than 2
transfers.

Re: [PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[Geert Uytterhoeven]

Hi Boris,

On Thu, Sep 7, 2017 at 1:37 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Thu, 7 Sep 2017 10:00:50 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Instead of having buffers on the stack, passing them around through multiple
>> call levels, and then kmalloc()ing a buffer, what about using the helpers in
>> <linux/spi/spi.h> instead, which take care of the issue through the
>> static bounce
>> buffer or kmalloc() themselves?
>
> Are you referring to spi_write_then_read()? If this is the case, I'm not

For example. There are more of them.

> sure we can use this because m25p80_read/write() can have more than 2
> transfers.

OK.More than two transfers may need special handling.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[DEBUG] mtd: spi-nor: dump DWORDs of the Basic Flash Parameter Table

[Cyrille Pitchen]

debug purpose only, should not be merged!

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
---

Hi Geert,

Can you apply this patch on your tree then report me what was printed, please?
I have an idea of the root cause of your issue then a potential work-around
but I first need to validate my assumption to confirm that the work-around
would actually work.

For instance, here is what I get with a Macronix MX25L25673G (same JEDEC ID as
MX25L25635E):

[    0.700000] atmel_qspi f0020000.spi: DWORD1 = 0xfffb20e5
[    0.710000] atmel_qspi f0020000.spi: DWORD2 = 0x0fffffff
[    0.710000] atmel_qspi f0020000.spi: DWORD3 = 0x6b08eb44
[    0.720000] atmel_qspi f0020000.spi: DWORD4 = 0xbb043b08
[    0.720000] atmel_qspi f0020000.spi: DWORD5 = 0xfffffffe
[    0.720000] atmel_qspi f0020000.spi: DWORD6 = 0xff00ffff
[    0.730000] atmel_qspi f0020000.spi: DWORD7 = 0xeb44ffff
[    0.730000] atmel_qspi f0020000.spi: DWORD8 = 0x520f200c
[    0.740000] atmel_qspi f0020000.spi: DWORD9 = 0xff00d810
[    0.740000] atmel_qspi f0020000.spi: DWORD10 = 0x00dd59d6
[    0.740000] atmel_qspi f0020000.spi: DWORD11 = 0xdb039f82
[    0.750000] atmel_qspi f0020000.spi: DWORD12 = 0x38670344
[    0.750000] atmel_qspi f0020000.spi: DWORD13 = 0xb030b030
[    0.760000] atmel_qspi f0020000.spi: DWORD14 = 0x5cd5bdf7
[    0.760000] atmel_qspi f0020000.spi: DWORD15 = 0xff299e4a
[    0.760000] atmel_qspi f0020000.spi: DWORD16 = 0x85f950f0
[    0.770000] atmel_qspi f0020000.spi: BFPT version 1.6 (length = 16)
[    0.770000] atmel_qspi f0020000.spi: mx25l25635e (32768 Kbytes)

Best regards,

Cyrille

 drivers/mtd/spi-nor/spi-nor.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 05254dd6a4a0..5066d99b9f50 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2136,8 +2136,14 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 		return err;
 
 	/* Fix endianness of the BFPT DWORDs. */
-	for (i = 0; i < BFPT_DWORD_MAX; i++)
+	for (i = 0; i < BFPT_DWORD_MAX; i++) {
 		bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
+		dev_info(nor->dev, "DWORD%d = 0x%08x\n", i + 1, bfpt.dwords[i]);
+	}
+	dev_info(nor->dev, "BFPT version %d.%d (length = %u)\n",
+		 bfpt_header->major,
+		 bfpt_header->minor,
+		 bfpt_header->length);
 
 	/* Number of address bytes. */
 	switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
-- 
2.11.0

Re: [DEBUG] mtd: spi-nor: dump DWORDs of the Basic Flash Parameter Table

[Cyrille Pitchen]

Hi again,

Le 07/09/2017 à 20:54, Cyrille Pitchen a écrit :
> debug purpose only, should not be merged!
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> ---
> 
> Hi Geert,
> 
> Can you apply this patch on your tree then report me what was printed, please?
> I have an idea of the root cause of your issue then a potential work-around
> but I first need to validate my assumption to confirm that the work-around
> would actually work.
>

If you could also dump the value of the 'addr' argument of
spi_nor_read_sfdp_dma_unsafe() just before the for () loop below in the
very same function. Actually, I suspect the SFDP tables of your SPI NOR
memory sample to have been programmed with invalid values, neither
compliant with the JEDEC JESD216 specification nor with the Cypress
datasheet for this memory part.

> For instance, here is what I get with a Macronix MX25L25673G (same JEDEC ID as
> MX25L25635E):
> 
> [    0.700000] atmel_qspi f0020000.spi: DWORD1 = 0xfffb20e5
> [    0.710000] atmel_qspi f0020000.spi: DWORD2 = 0x0fffffff
> [    0.710000] atmel_qspi f0020000.spi: DWORD3 = 0x6b08eb44
> [    0.720000] atmel_qspi f0020000.spi: DWORD4 = 0xbb043b08
> [    0.720000] atmel_qspi f0020000.spi: DWORD5 = 0xfffffffe
> [    0.720000] atmel_qspi f0020000.spi: DWORD6 = 0xff00ffff
> [    0.730000] atmel_qspi f0020000.spi: DWORD7 = 0xeb44ffff
> [    0.730000] atmel_qspi f0020000.spi: DWORD8 = 0x520f200c
> [    0.740000] atmel_qspi f0020000.spi: DWORD9 = 0xff00d810
> [    0.740000] atmel_qspi f0020000.spi: DWORD10 = 0x00dd59d6
> [    0.740000] atmel_qspi f0020000.spi: DWORD11 = 0xdb039f82
> [    0.750000] atmel_qspi f0020000.spi: DWORD12 = 0x38670344
> [    0.750000] atmel_qspi f0020000.spi: DWORD13 = 0xb030b030
> [    0.760000] atmel_qspi f0020000.spi: DWORD14 = 0x5cd5bdf7
> [    0.760000] atmel_qspi f0020000.spi: DWORD15 = 0xff299e4a
> [    0.760000] atmel_qspi f0020000.spi: DWORD16 = 0x85f950f0
> [    0.770000] atmel_qspi f0020000.spi: BFPT version 1.6 (length = 16)
> [    0.770000] atmel_qspi f0020000.spi: mx25l25635e (32768 Kbytes)
> 
> Best regards,
> 
> Cyrille
> 
>  drivers/mtd/spi-nor/spi-nor.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 05254dd6a4a0..5066d99b9f50 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2136,8 +2136,14 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  		return err;
>  
>  	/* Fix endianness of the BFPT DWORDs. */
> -	for (i = 0; i < BFPT_DWORD_MAX; i++)
> +	for (i = 0; i < BFPT_DWORD_MAX; i++) {
>  		bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
> +		dev_info(nor->dev, "DWORD%d = 0x%08x\n", i + 1, bfpt.dwords[i]);
> +	}
> +	dev_info(nor->dev, "BFPT version %d.%d (length = %u)\n",
> +		 bfpt_header->major,
> +		 bfpt_header->minor,
> +		 bfpt_header->length);
>  
>  	/* Number of address bytes. */
>  	switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
> 

Re: [PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[Boris Brezillon]

On Thu, 7 Sep 2017 10:00:50 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Cyrille,
> 
> On Wed, Sep 6, 2017 at 11:45 PM, Cyrille Pitchen
> <cyrille.pitchen@wedev4u.fr> wrote:
> > spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
> > When the m25p80 driver is used (pretty common case), nor->read() is then
> > implemented by the m25p80_read() function, which is likely to initialize a
> > 'struct spi_transfer' from its buf argument before appending this
> > structure inside the 'struct spi_message' argument of spi_sync().
> >
> > Besides the SPI sub-system states that both .tx_buf and .rx_buf members of
> > 'struct spi_transfer' must point into dma-safe memory. However, two of the
> > three calls of spi_nor_read_sfdp() were given pointers to stack allocated
> > memory as buf argument, hence not in a dma-safe area.
> > Hopefully, the third and last call of spi_nor_read_sfdp() was already
> > given a kmalloc'ed buffer argument, hence dma-safe.
> >
> > So this patch fixes this issue by introducing a
> > spi_nor_read_sfdp_dma_unsafe() function which simply wraps the existing
> > spi_nor_read_sfdp() function and uses some kmalloc'ed memory as a bounce
> > buffer.
> >
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>  
> 
> While this patch got rid of the warning, it does not fix the SPI FLASH
> identification
> issue:
> 
>     m25p80 spi0.0: s25fl512s (0 Kbytes)
>     3 ofpart partitions found on MTD device spi0.0
>     Creating 3 MTD partitions on "spi0.0":
>     0x000000000000-0x000000040000 : "loader"
>     mtd: partition "loader" is out of reach -- disabled
>     0x000000040000-0x000000080000 : "system"
>     mtd: partition "system" is out of reach -- disabled
>     0x000000080000-0x000004000000 : "user"
>     mtd: partition "user" is out of reach -- disabled
> 
> I noticed there's still one direct call to spi_nor_read_sfdp() left in
> spi_nor_parse_sfdp().

I think the remaining call site is valid because the caller allocates
the buffer it passes to spi_nor_parse_sfdp() with kmalloc().

> I tried changing that to spi_nor_read_sfdp_dma_unsafe(), but that didn't help.

Ok, we're still working on that. Did you have time to test Cyrille's
debug patch?

Cyrille, can we add more consistency checks in the SFDP parser code to
detect devices exposing invalid SFPD pages? For example, a device size
of 0 is impossible and could be easily detected when parsing the SFPD?

Re: [DEBUG] mtd: spi-nor: dump DWORDs of the Basic Flash Parameter Table

[Geert Uytterhoeven]

Hi Cyrille,

On Thu, Sep 7, 2017 at 9:28 PM, Cyrille Pitchen
<cyrille.pitchen@wedev4u.fr> wrote:
>> Can you apply this patch on your tree then report me what was printed, please?
>> I have an idea of the root cause of your issue then a potential work-around
>> but I first need to validate my assumption to confirm that the work-around
>> would actually work.

+m25p80 spi0.0: DWORD1 = 0xffffffff
+m25p80 spi0.0: DWORD2 = 0xffffffff
+m25p80 spi0.0: DWORD3 = 0xffffffff
+m25p80 spi0.0: DWORD4 = 0xffffffff
+m25p80 spi0.0: DWORD5 = 0xffffffff
+m25p80 spi0.0: DWORD6 = 0xffffffff
+m25p80 spi0.0: DWORD7 = 0xffffffff
+m25p80 spi0.0: DWORD8 = 0xffffffff
+m25p80 spi0.0: DWORD9 = 0xffffffff
+m25p80 spi0.0: DWORD10 = 0x00000000
+m25p80 spi0.0: DWORD11 = 0x00000000
+m25p80 spi0.0: DWORD12 = 0x00000000
+m25p80 spi0.0: DWORD13 = 0x00000000
+m25p80 spi0.0: DWORD14 = 0x00000000
+m25p80 spi0.0: DWORD15 = 0x00000000
+m25p80 spi0.0: DWORD16 = 0x00000000
+m25p80 spi0.0: BFPT version 1.0 (length = 9)

> If you could also dump the value of the 'addr' argument of
> spi_nor_read_sfdp_dma_unsafe() just before the for () loop below in the
> very same function. Actually, I suspect the SFDP tables of your SPI NOR

+m25p80 spi0.0: addr = 0x448

> memory sample to have been programmed with invalid values, neither
> compliant with the JEDEC JESD216 specification nor with the Cypress
> datasheet for this memory part.

Sounds plausible.
I get the same values when disabling DMA, so it's not due to bad DMA handling.
All Renesas boards I have local or remote access to have spansion,s25fl512s.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [DEBUG] mtd: spi-nor: dump DWORDs of the Basic Flash Parameter Table

[Boris Brezillon]

Hi Geert,

On Mon, 11 Sep 2017 10:58:36 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Cyrille,
> 
> On Thu, Sep 7, 2017 at 9:28 PM, Cyrille Pitchen
> <cyrille.pitchen@wedev4u.fr> wrote:
> >> Can you apply this patch on your tree then report me what was printed, please?
> >> I have an idea of the root cause of your issue then a potential work-around
> >> but I first need to validate my assumption to confirm that the work-around
> >> would actually work.  
> 
> +m25p80 spi0.0: DWORD1 = 0xffffffff
> +m25p80 spi0.0: DWORD2 = 0xffffffff
> +m25p80 spi0.0: DWORD3 = 0xffffffff
> +m25p80 spi0.0: DWORD4 = 0xffffffff
> +m25p80 spi0.0: DWORD5 = 0xffffffff
> +m25p80 spi0.0: DWORD6 = 0xffffffff
> +m25p80 spi0.0: DWORD7 = 0xffffffff
> +m25p80 spi0.0: DWORD8 = 0xffffffff
> +m25p80 spi0.0: DWORD9 = 0xffffffff
> +m25p80 spi0.0: DWORD10 = 0x00000000
> +m25p80 spi0.0: DWORD11 = 0x00000000
> +m25p80 spi0.0: DWORD12 = 0x00000000
> +m25p80 spi0.0: DWORD13 = 0x00000000
> +m25p80 spi0.0: DWORD14 = 0x00000000
> +m25p80 spi0.0: DWORD15 = 0x00000000
> +m25p80 spi0.0: DWORD16 = 0x00000000
> +m25p80 spi0.0: BFPT version 1.0 (length = 9)
> 
> > If you could also dump the value of the 'addr' argument of
> > spi_nor_read_sfdp_dma_unsafe() just before the for () loop below in the
> > very same function. Actually, I suspect the SFDP tables of your SPI NOR  
> 
> +m25p80 spi0.0: addr = 0x448
> 
> > memory sample to have been programmed with invalid values, neither
> > compliant with the JEDEC JESD216 specification nor with the Cypress
> > datasheet for this memory part.  
> 
> Sounds plausible.
> I get the same values when disabling DMA, so it's not due to bad DMA handling.
> All Renesas boards I have local or remote access to have spansion,s25fl512s.

Can you try with the following patch?

Thanks,

Boris

--->8---
>From 000ff63fdb149d87d755483f5edc0aba010da6b4 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@free-electrons.com>
Date: Tue, 12 Sep 2017 15:10:35 +0200
Subject: [PATCH] mtd: spi-nor: Check consistency of the memory size extracted
 from the SFDP

One field of the flash parameter table contains information about the
flash device size.
Most of the time the data extracted from this field is valid, but
sometimes the BFPT section of the SFDP table is corrupted or invalid and
this field is set to 0xffffffff, thus resulting in an integer overflow
when setting params->size.

Since NOR devices are anayway always smaller than 2^64 bytes, we can
easily stop the BFPT parsing if the size reported in this table is
invalid.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index cf1d4a15e10a..665ccae1d090 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2127,6 +2127,15 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 	params->size = bfpt.dwords[BFPT_DWORD(2)];
 	if (params->size & BIT(31)) {
 		params->size &= ~BIT(31);
+
+		/*
+		 * Prevent overflows on params->size. Anyway, a NOR of 1^64
+		 * bytes is unlikely to exist so this error probably means
+		 * the BFPT we are reading is corrupted/wrong.
+		 */
+		if (params->size > 63)
+			return -EINVAL;
+
 		params->size = 1ULL << params->size;
 	} else {
 		params->size++;

Re: [DEBUG] mtd: spi-nor: dump DWORDs of the Basic Flash Parameter Table

[Cyrille Pitchen]

Le 12/09/2017 à 15:12, Boris Brezillon a écrit :
> Hi Geert,
> 
> On Mon, 11 Sep 2017 10:58:36 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
>> Hi Cyrille,
>>
>> On Thu, Sep 7, 2017 at 9:28 PM, Cyrille Pitchen
>> <cyrille.pitchen@wedev4u.fr> wrote:
>>>> Can you apply this patch on your tree then report me what was printed, please?
>>>> I have an idea of the root cause of your issue then a potential work-around
>>>> but I first need to validate my assumption to confirm that the work-around
>>>> would actually work.  
>>
>> +m25p80 spi0.0: DWORD1 = 0xffffffff
>> +m25p80 spi0.0: DWORD2 = 0xffffffff
>> +m25p80 spi0.0: DWORD3 = 0xffffffff
>> +m25p80 spi0.0: DWORD4 = 0xffffffff
>> +m25p80 spi0.0: DWORD5 = 0xffffffff
>> +m25p80 spi0.0: DWORD6 = 0xffffffff
>> +m25p80 spi0.0: DWORD7 = 0xffffffff
>> +m25p80 spi0.0: DWORD8 = 0xffffffff
>> +m25p80 spi0.0: DWORD9 = 0xffffffff
>> +m25p80 spi0.0: DWORD10 = 0x00000000
>> +m25p80 spi0.0: DWORD11 = 0x00000000
>> +m25p80 spi0.0: DWORD12 = 0x00000000
>> +m25p80 spi0.0: DWORD13 = 0x00000000
>> +m25p80 spi0.0: DWORD14 = 0x00000000
>> +m25p80 spi0.0: DWORD15 = 0x00000000
>> +m25p80 spi0.0: DWORD16 = 0x00000000
>> +m25p80 spi0.0: BFPT version 1.0 (length = 9)
>>
>>> If you could also dump the value of the 'addr' argument of
>>> spi_nor_read_sfdp_dma_unsafe() just before the for () loop below in the
>>> very same function. Actually, I suspect the SFDP tables of your SPI NOR  
>>
>> +m25p80 spi0.0: addr = 0x448
>>
>>> memory sample to have been programmed with invalid values, neither
>>> compliant with the JEDEC JESD216 specification nor with the Cypress
>>> datasheet for this memory part.  
>>
>> Sounds plausible.
>> I get the same values when disabling DMA, so it's not due to bad DMA handling.
>> All Renesas boards I have local or remote access to have spansion,s25fl512s.
> 
> Can you try with the following patch?
> 
> Thanks,
> 
> Boris
> 
> --->8---
> From 000ff63fdb149d87d755483f5edc0aba010da6b4 Mon Sep 17 00:00:00 2001
> From: Boris Brezillon <boris.brezillon@free-electrons.com>
> Date: Tue, 12 Sep 2017 15:10:35 +0200
> Subject: [PATCH] mtd: spi-nor: Check consistency of the memory size extracted
>  from the SFDP
> 
> One field of the flash parameter table contains information about the
> flash device size.
> Most of the time the data extracted from this field is valid, but
> sometimes the BFPT section of the SFDP table is corrupted or invalid and
> this field is set to 0xffffffff, thus resulting in an integer overflow
> when setting params->size.
> 
> Since NOR devices are anayway always smaller than 2^64 bytes, we can
> easily stop the BFPT parsing if the size reported in this table is
> invalid.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.com>

with few comments below:
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index cf1d4a15e10a..665ccae1d090 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2127,6 +2127,15 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  	params->size = bfpt.dwords[BFPT_DWORD(2)];
>  	if (params->size & BIT(31)) {
>  		params->size &= ~BIT(31);
> +
> +		/*
> +		 * Prevent overflows on params->size. Anyway, a NOR of 1^64
typo: should be 2^64
> +		 * bytes is unlikely to exist so this error probably means

Here the size is still expressed in bits, not yet in byte, the
conversion is done right after this chunk.
> +		 * the BFPT we are reading is corrupted/wrong.
> +		 */
> +		if (params->size > 63)
> +			return -EINVAL;
> +
>  		params->size = 1ULL << params->size;
>  	} else {
>  		params->size++;
> 

Re: [DEBUG] mtd: spi-nor: dump DWORDs of the Basic Flash Parameter Table

[Boris Brezillon]

On Thu, 14 Sep 2017 18:44:31 +0200
Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote:

> Le 12/09/2017 à 15:12, Boris Brezillon a écrit :
> > Hi Geert,
> > 
> > On Mon, 11 Sep 2017 10:58:36 +0200
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >   
> >> Hi Cyrille,
> >>
> >> On Thu, Sep 7, 2017 at 9:28 PM, Cyrille Pitchen
> >> <cyrille.pitchen@wedev4u.fr> wrote:  
> >>>> Can you apply this patch on your tree then report me what was printed, please?
> >>>> I have an idea of the root cause of your issue then a potential work-around
> >>>> but I first need to validate my assumption to confirm that the work-around
> >>>> would actually work.    
> >>
> >> +m25p80 spi0.0: DWORD1 = 0xffffffff
> >> +m25p80 spi0.0: DWORD2 = 0xffffffff
> >> +m25p80 spi0.0: DWORD3 = 0xffffffff
> >> +m25p80 spi0.0: DWORD4 = 0xffffffff
> >> +m25p80 spi0.0: DWORD5 = 0xffffffff
> >> +m25p80 spi0.0: DWORD6 = 0xffffffff
> >> +m25p80 spi0.0: DWORD7 = 0xffffffff
> >> +m25p80 spi0.0: DWORD8 = 0xffffffff
> >> +m25p80 spi0.0: DWORD9 = 0xffffffff
> >> +m25p80 spi0.0: DWORD10 = 0x00000000
> >> +m25p80 spi0.0: DWORD11 = 0x00000000
> >> +m25p80 spi0.0: DWORD12 = 0x00000000
> >> +m25p80 spi0.0: DWORD13 = 0x00000000
> >> +m25p80 spi0.0: DWORD14 = 0x00000000
> >> +m25p80 spi0.0: DWORD15 = 0x00000000
> >> +m25p80 spi0.0: DWORD16 = 0x00000000
> >> +m25p80 spi0.0: BFPT version 1.0 (length = 9)
> >>  
> >>> If you could also dump the value of the 'addr' argument of
> >>> spi_nor_read_sfdp_dma_unsafe() just before the for () loop below in the
> >>> very same function. Actually, I suspect the SFDP tables of your SPI NOR    
> >>
> >> +m25p80 spi0.0: addr = 0x448
> >>  
> >>> memory sample to have been programmed with invalid values, neither
> >>> compliant with the JEDEC JESD216 specification nor with the Cypress
> >>> datasheet for this memory part.    
> >>
> >> Sounds plausible.
> >> I get the same values when disabling DMA, so it's not due to bad DMA handling.
> >> All Renesas boards I have local or remote access to have spansion,s25fl512s.  
> > 
> > Can you try with the following patch?
> > 
> > Thanks,
> > 
> > Boris
> >   
> > --->8---  
> > From 000ff63fdb149d87d755483f5edc0aba010da6b4 Mon Sep 17 00:00:00 2001
> > From: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Date: Tue, 12 Sep 2017 15:10:35 +0200
> > Subject: [PATCH] mtd: spi-nor: Check consistency of the memory size extracted
> >  from the SFDP
> > 
> > One field of the flash parameter table contains information about the
> > flash device size.
> > Most of the time the data extracted from this field is valid, but
> > sometimes the BFPT section of the SFDP table is corrupted or invalid and
> > this field is set to 0xffffffff, thus resulting in an integer overflow
> > when setting params->size.
> > 
> > Since NOR devices are anayway always smaller than 2^64 bytes, we can
> > easily stop the BFPT parsing if the size reported in this table is
> > invalid.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>  
> Acked-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.com>

Applied after fixing the things you pointed below.

> 
> with few comments below:
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index cf1d4a15e10a..665ccae1d090 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -2127,6 +2127,15 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> >  	params->size = bfpt.dwords[BFPT_DWORD(2)];
> >  	if (params->size & BIT(31)) {
> >  		params->size &= ~BIT(31);
> > +
> > +		/*
> > +		 * Prevent overflows on params->size. Anyway, a NOR of 1^64  
> typo: should be 2^64
> > +		 * bytes is unlikely to exist so this error probably means  
> 
> Here the size is still expressed in bits, not yet in byte, the
> conversion is done right after this chunk.
> > +		 * the BFPT we are reading is corrupted/wrong.
> > +		 */
> > +		if (params->size > 63)
> > +			return -EINVAL;
> > +
> >  		params->size = 1ULL << params->size;
> >  	} else {
> >  		params->size++;
> >   
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp()

[Boris Brezillon]

On Wed,  6 Sep 2017 23:45:02 +0200
Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote:

> spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
> When the m25p80 driver is used (pretty common case), nor->read() is then
> implemented by the m25p80_read() function, which is likely to initialize a
> 'struct spi_transfer' from its buf argument before appending this
> structure inside the 'struct spi_message' argument of spi_sync().
> 
> Besides the SPI sub-system states that both .tx_buf and .rx_buf members of
> 'struct spi_transfer' must point into dma-safe memory. However, two of the
> three calls of spi_nor_read_sfdp() were given pointers to stack allocated
> memory as buf argument, hence not in a dma-safe area.
> Hopefully, the third and last call of spi_nor_read_sfdp() was already
> given a kmalloc'ed buffer argument, hence dma-safe.
> 
> So this patch fixes this issue by introducing a
> spi_nor_read_sfdp_dma_unsafe() function which simply wraps the existing
> spi_nor_read_sfdp() function and uses some kmalloc'ed memory as a bounce
> buffer.
> 
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>

Applied.

Thanks,

Boris

> ---
> 
> Compiled but not tested yet!
> 
>  drivers/mtd/spi-nor/spi-nor.c | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index cf1d4a15e10a..05254dd6a4a0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1784,7 +1784,7 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
>   * @nor:	pointer to a 'struct spi_nor'
>   * @addr:	offset in the SFDP area to start reading data from
>   * @len:	number of bytes to read
> - * @buf:	buffer where the SFDP data are copied into
> + * @buf:	buffer where the SFDP data are copied into (dma-safe memory)
>   *
>   * Whatever the actual numbers of bytes for address and dummy cycles are
>   * for (Fast) Read commands, the Read SFDP (5Ah) instruction is always
> @@ -1829,6 +1829,36 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>  	return ret;
>  }
>  
> +/**
> + * spi_nor_read_sfdp_dma_unsafe() - read Serial Flash Discoverable Parameters.
> + * @nor:	pointer to a 'struct spi_nor'
> + * @addr:	offset in the SFDP area to start reading data from
> + * @len:	number of bytes to read
> + * @buf:	buffer where the SFDP data are copied into
> + *
> + * Wrap spi_nor_read_sfdp() using a kmalloc'ed bounce buffer as @buf is now not
> + * guaranteed to be dma-safe.
> + *
> + * Return: -ENOMEM if kmalloc() fails, the return code of spi_nor_read_sfdp()
> + *          otherwise.
> + */
> +static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
> +					size_t len, void *buf)
> +{
> +	void *dma_safe_buf;
> +	int ret;
> +
> +	dma_safe_buf = kmalloc(len, GFP_KERNEL);
> +	if (!dma_safe_buf)
> +		return -ENOMEM;
> +
> +	ret = spi_nor_read_sfdp(nor, addr, len, dma_safe_buf);
> +	memcpy(buf, dma_safe_buf, len);
> +	kfree(dma_safe_buf);
> +
> +	return ret;
> +}
> +
>  struct sfdp_parameter_header {
>  	u8		id_lsb;
>  	u8		minor;
> @@ -2101,7 +2131,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  		    bfpt_header->length * sizeof(u32));
>  	addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
>  	memset(&bfpt, 0, sizeof(bfpt));
> -	err = spi_nor_read_sfdp(nor,  addr, len, &bfpt);
> +	err = spi_nor_read_sfdp_dma_unsafe(nor,  addr, len, &bfpt);
>  	if (err < 0)
>  		return err;
>  
> @@ -2243,7 +2273,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  	int i, err;
>  
>  	/* Get the SFDP header. */
> -	err = spi_nor_read_sfdp(nor, 0, sizeof(header), &header);
> +	err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sizeof(header), &header);
>  	if (err < 0)
>  		return err;
>