[PATCH] mtd: spi-nor: Use DMA-safe buffer for JEDEC ID in spi_nor_read_id()

[PATCH] mtd: spi-nor: Use DMA-safe buffer for JEDEC ID in spi_nor_read_id()

[Jarkko Nikula]

After commit 4120f8d158ef ("mtd: spi-nor: Use the spi_mem_xx() API")
there is no allocation for DMA-safe buffer when transmitting data bytes
over SPI bus in m25p80 driver.

JEDEC ID reading in spi_nor_read_id() has the buffer in stack. This is
not safe with the m25p80 driver anymore after commit 4120f8d158ef if
underlying SPI controller is using DMA for transfers.

Therefore allocate a temporary DMA-safe buffer for JEDEC ID reading.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
I'm not an spi-nor expert at all but noticed this "WARNING: CPU: 3 PID: 154
at kernel/dma/debug.c:1191 check_for_stack+0xc2/0x1a0" after
4120f8d158ef since my test setup has DMA debugging enabled and using DMA
for SPI.
I don't know are there other places that may transfer from stack but it
looked potentially better to have the buffer allocated in
spi_nor_read_id() instead of doing allocation & copy all the places
that 4120f8d158ef touches.
---
 drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f028277fb1ce..f2294ed14d8f 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1269,25 +1269,37 @@ static const struct flash_info spi_nor_ids[] = {
 static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
 {
 	int			tmp;
-	u8			id[SPI_NOR_MAX_ID_LEN];
-	const struct flash_info	*info;
+	u8			*id;
+	const struct flash_info	*info, *ret;
+
+	id = kzalloc(SPI_NOR_MAX_ID_LEN, GFP_KERNEL | GFP_DMA);
+	if (!id) {
+		ret = ERR_PTR(ENOMEM);
+		goto out;
+	}
 
 	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN);
 	if (tmp < 0) {
 		dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp);
-		return ERR_PTR(tmp);
+		ret = ERR_PTR(tmp);
+		goto out;
 	}
 
 	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
 		info = &spi_nor_ids[tmp];
 		if (info->id_len) {
-			if (!memcmp(info->id, id, info->id_len))
-				return &spi_nor_ids[tmp];
+			if (!memcmp(info->id, id, info->id_len)) {
+				ret = &spi_nor_ids[tmp];
+				goto out;
+			}
 		}
 	}
 	dev_err(nor->dev, "unrecognized JEDEC id bytes: %02x, %02x, %02x\n",
 		id[0], id[1], id[2]);
-	return ERR_PTR(-ENODEV);
+	ret = ERR_PTR(-ENODEV);
+out:
+	kfree(id);
+	return ret;
 }
 
 static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
-- 
2.18.0

Re: [PATCH] mtd: spi-nor: Use DMA-safe buffer for JEDEC ID in spi_nor_read_id()

[Boris Brezillon]

On Fri, 14 Sep 2018 13:49:01 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> After commit 4120f8d158ef ("mtd: spi-nor: Use the spi_mem_xx() API")
> there is no allocation for DMA-safe buffer when transmitting data bytes
> over SPI bus in m25p80 driver.
> 
> JEDEC ID reading in spi_nor_read_id() has the buffer in stack. This is
> not safe with the m25p80 driver anymore after commit 4120f8d158ef if
> underlying SPI controller is using DMA for transfers.
> 
> Therefore allocate a temporary DMA-safe buffer for JEDEC ID reading.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> I'm not an spi-nor expert at all but noticed this "WARNING: CPU: 3 PID: 154
> at kernel/dma/debug.c:1191 check_for_stack+0xc2/0x1a0" after
> 4120f8d158ef since my test setup has DMA debugging enabled and using DMA
> for SPI.
> I don't know are there other places that may transfer from stack but it
> looked potentially better to have the buffer allocated in
> spi_nor_read_id() instead of doing allocation & copy all the places
> that 4120f8d158ef touches.
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index f028277fb1ce..f2294ed14d8f 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1269,25 +1269,37 @@ static const struct flash_info spi_nor_ids[] = {
>  static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>  {
>  	int			tmp;
> -	u8			id[SPI_NOR_MAX_ID_LEN];
> -	const struct flash_info	*info;
> +	u8			*id;
> +	const struct flash_info	*info, *ret;
> +
> +	id = kzalloc(SPI_NOR_MAX_ID_LEN, GFP_KERNEL | GFP_DMA);

You can drop the GFP_DMA flag here, it's not needed (see the GFP_DMA
description [1] for more details).

the patch looks good otherwise.

> +	if (!id) {
> +		ret = ERR_PTR(ENOMEM);
> +		goto out;
> +	}
>  
>  	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN);
>  	if (tmp < 0) {
>  		dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp);
> -		return ERR_PTR(tmp);
> +		ret = ERR_PTR(tmp);
> +		goto out;
>  	}
>  
>  	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>  		info = &spi_nor_ids[tmp];
>  		if (info->id_len) {
> -			if (!memcmp(info->id, id, info->id_len))
> -				return &spi_nor_ids[tmp];
> +			if (!memcmp(info->id, id, info->id_len)) {
> +				ret = &spi_nor_ids[tmp];
> +				goto out;
> +			}
>  		}
>  	}
>  	dev_err(nor->dev, "unrecognized JEDEC id bytes: %02x, %02x, %02x\n",
>  		id[0], id[1], id[2]);
> -	return ERR_PTR(-ENODEV);
> +	ret = ERR_PTR(-ENODEV);
> +out:
> +	kfree(id);
> +	return ret;
>  }
>  
>  static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,

[1]https://elixir.bootlin.com/linux/v4.19-rc3/source/Documentation/kernel-hacking/hacking.rst#L320