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

[PATCH v2] 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>
---
v2: Drop GFP_DMA.

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..f186b95c55f9 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);
+	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 v2] mtd: spi-nor: Use DMA-safe buffer for JEDEC ID in spi_nor_read_id()

[Boris Brezillon]

On Mon, 17 Sep 2018 11:10:18 +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>

Oh, I forgot to mention that you should have Cc and Fixes tag here, to
make sure the patch gets backported to 4.18:

Fixes: 4120f8d158ef ("mtd: spi-nor: Use the spi_mem_xx() API")
Cc: <stable@vger.kernel.org>

No need to send a new version, I can add them when applying.

> ---
> v2: Drop GFP_DMA.
> 
> 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..f186b95c55f9 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);
> +	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,

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

[Boris Brezillon]

On Mon, 17 Sep 2018 10:18:51 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Mon, 17 Sep 2018 11:10:18 +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>  
> 
> Oh, I forgot to mention that you should have Cc and Fixes tag here, to
> make sure the patch gets backported to 4.18:
> 
> Fixes: 4120f8d158ef ("mtd: spi-nor: Use the spi_mem_xx() API")
> Cc: <stable@vger.kernel.org>
> 
> No need to send a new version, I can add them when applying.

Hm, I just had a look at all ->read/write_reg() call sites and it seems
we have the same problem in other places. Maybe it's just better to fix
the problem in m25p80 ->read/write_reg() implems for now and see how we
can fix that generically afterwards.

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

[Jarkko Nikula]

On 09/17/2018 11:28 AM, Boris Brezillon wrote:
> 
> Hm, I just had a look at all ->read/write_reg() call sites and it seems
> we have the same problem in other places. Maybe it's just better to fix
> the problem in m25p80 ->read/write_reg() implems for now and see how we
> can fix that generically afterwards.
> 
It's not only m25p80, how about all other cases calling spi_nor_scan()?

-- 
Jarkko

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

[Boris Brezillon]

On Mon, 17 Sep 2018 11:41:31 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> On 09/17/2018 11:28 AM, Boris Brezillon wrote:
> > 
> > Hm, I just had a look at all ->read/write_reg() call sites and it seems
> > we have the same problem in other places. Maybe it's just better to fix
> > the problem in m25p80 ->read/write_reg() implems for now and see how we
> > can fix that generically afterwards.
> >   
> It's not only m25p80, how about all other cases calling spi_nor_scan()?
> 

I agree, the problem is more generic than that, but commit 4120f8d158ef
only breaks the m25p80 driver, and I'd like to keep the fix as simple as
possible.

We can also discuss how to fix that generically, but I'd like the fix
to be merged in 4.19.

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

[Jarkko Nikula]

On 09/17/2018 11:46 AM, Boris Brezillon wrote:
> On Mon, 17 Sep 2018 11:41:31 +0300
> Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> 
>> On 09/17/2018 11:28 AM, Boris Brezillon wrote:
>>>
>>> Hm, I just had a look at all ->read/write_reg() call sites and it seems
>>> we have the same problem in other places. Maybe it's just better to fix
>>> the problem in m25p80 ->read/write_reg() implems for now and see how we
>>> can fix that generically afterwards.
>>>    
>> It's not only m25p80, how about all other cases calling spi_nor_scan()?
>>
> 
> I agree, the problem is more generic than that, but commit 4120f8d158ef
> only breaks the m25p80 driver, and I'd like to keep the fix as simple as
> possible.
> 
You are right, I looked at too quickly spi_nor_scan() users. Those 
others are controller drivers.

-- 
Jarkko

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

[Boris Brezillon]

On Mon, 17 Sep 2018 12:04:43 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> On 09/17/2018 11:46 AM, Boris Brezillon wrote:
> > On Mon, 17 Sep 2018 11:41:31 +0300
> > Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> >   
> >> On 09/17/2018 11:28 AM, Boris Brezillon wrote:  
> >>>
> >>> Hm, I just had a look at all ->read/write_reg() call sites and it seems
> >>> we have the same problem in other places. Maybe it's just better to fix
> >>> the problem in m25p80 ->read/write_reg() implems for now

Do you plan to send a new fix doing that, or should I? I'd really like
to have the fix queued to mtd/master early this week, so that it
spends a bit of time in -next.

Thanks,

Boris

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

[Jarkko Nikula]

On 09/17/2018 03:28 PM, Boris Brezillon wrote:
> On Mon, 17 Sep 2018 12:04:43 +0300
> Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> 
>> On 09/17/2018 11:46 AM, Boris Brezillon wrote:
>>> On Mon, 17 Sep 2018 11:41:31 +0300
>>> Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
>>>    
>>>> On 09/17/2018 11:28 AM, Boris Brezillon wrote:
>>>>>
>>>>> Hm, I just had a look at all ->read/write_reg() call sites and it seems
>>>>> we have the same problem in other places. Maybe it's just better to fix
>>>>> the problem in m25p80 ->read/write_reg() implems for now
> 
> Do you plan to send a new fix doing that, or should I? I'd really like
> to have the fix queued to mtd/master early this week, so that it
> spends a bit of time in -next.
> 
I guess better if you could as I haven't started anything yet. I'm not 
so familiar with the code to see what would be best way to fix, 
reverting m25p80_read_reg/m25p80_write_reg back to spi_write(), the 
whole commit or do some kmemdup() there.

-- 
Jarkko

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

[Boris Brezillon]

On Mon, 17 Sep 2018 15:51:24 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> On 09/17/2018 03:28 PM, Boris Brezillon wrote:
> > On Mon, 17 Sep 2018 12:04:43 +0300
> > Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> >   
> >> On 09/17/2018 11:46 AM, Boris Brezillon wrote:  
> >>> On Mon, 17 Sep 2018 11:41:31 +0300
> >>> Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> >>>      
> >>>> On 09/17/2018 11:28 AM, Boris Brezillon wrote:  
> >>>>>
> >>>>> Hm, I just had a look at all ->read/write_reg() call sites and it seems
> >>>>> we have the same problem in other places. Maybe it's just better to fix
> >>>>> the problem in m25p80 ->read/write_reg() implems for now  
> > 
> > Do you plan to send a new fix doing that, or should I? I'd really like
> > to have the fix queued to mtd/master early this week, so that it
> > spends a bit of time in -next.
> >   
> I guess better if you could as I haven't started anything yet. I'm not 
> so familiar with the code to see what would be best way to fix, 
> reverting m25p80_read_reg/m25p80_write_reg back to spi_write(), the 
> whole commit or do some kmemdup() there.

kmemdup is better (or any safe alternative validated by Kees), we
definitely don't want to go back to spi_write().

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

[Boris Brezillon]

On Mon, 17 Sep 2018 15:51:24 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> On 09/17/2018 03:28 PM, Boris Brezillon wrote:
> > On Mon, 17 Sep 2018 12:04:43 +0300
> > Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> >   
> >> On 09/17/2018 11:46 AM, Boris Brezillon wrote:  
> >>> On Mon, 17 Sep 2018 11:41:31 +0300
> >>> Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> >>>      
> >>>> On 09/17/2018 11:28 AM, Boris Brezillon wrote:  
> >>>>>
> >>>>> Hm, I just had a look at all ->read/write_reg() call sites and it seems
> >>>>> we have the same problem in other places. Maybe it's just better to fix
> >>>>> the problem in m25p80 ->read/write_reg() implems for now  
> > 
> > Do you plan to send a new fix doing that, or should I? I'd really like
> > to have the fix queued to mtd/master early this week, so that it
> > spends a bit of time in -next.
> >   
> I guess better if you could as I haven't started anything yet.

I just sent a fix. Would be great if you could test it (and add your
Tested-by if it works).

Thanks,

Boris