[PATCH] Fix Oops with Atmel SPI

[PATCH] Fix Oops with Atmel SPI

[Anders Larsen]

Tweak MTD's cache allocation to make it work with the atmel DMA'ed SPI.
Substitute kmalloc for vmalloc so the cache buffer is mappable as per
the Atmel SPI driver's requirements, otherwise an Oops would occur.

The original patch by Ian McDonnell <ian@brightstareng.com> was found here:
http://lists.infradead.org/pipermail/linux-mtd/2007-December/020184.html

Signed-off-by: Anders Larsen <al@alarsen.net>
Cc: Ian McDonnell <ian@brightstareng.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Matthias Kaehlcke <matthias@kaehlcke.net>
Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Nicolas Pitre <nico@fluxnic.net>
---
 drivers/mtd/mtdblock.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: b/drivers/mtd/mtdblock.c
===================================================================
--- a/drivers/mtd/mtdblock.c
+++ b/drivers/mtd/mtdblock.c
@@ -253,7 +253,11 @@ static int mtdblock_writesect(struct mtd
 {
 	struct mtdblk_dev *mtdblk = mtdblks[dev->devnum];
 	if (unlikely(!mtdblk->cache_data && mtdblk->cache_size)) {
+#ifdef CONFIG_SPI_ATMEL
+		mtdblk->cache_data = kmalloc(mtdblk->mtd->erasesize, GFP_KERNEL);
+#else
 		mtdblk->cache_data = vmalloc(mtdblk->mtd->erasesize);
+#endif
 		if (!mtdblk->cache_data)
 			return -EINTR;
 		/* -EINTR is not really correct, but it is the best match
@@ -322,7 +326,11 @@ static int mtdblock_release(struct mtd_b
 		mtdblks[dev] = NULL;
 		if (mtdblk->mtd->sync)
 			mtdblk->mtd->sync(mtdblk->mtd);
+#ifdef CONFIG_SPI_ATMEL
+		kfree(mtdblk->cache_data);
+#else
 		vfree(mtdblk->cache_data);
+#endif
 		kfree(mtdblk);
 	}
 

Re: [PATCH] Fix Oops with Atmel SPI

[Iwo Mergler]

Hi Anders,

I wouldn't recommend that. MTD erase blocks are 64K or more. In a typical
embedded system you will not be able to kmalloc that much memory after
a few day's of operation - the page pool gets fragmented.

A possibly better approach is to arrange for that memory to get allocated
at driver start time.

An even better approach would be to change the algorithm to operate on
a list of smaller allocations, e.g. MTD page size.


Best regards,

Iwo


Anders Larsen wrote:
> Tweak MTD's cache allocation to make it work with the atmel DMA'ed SPI.
> Substitute kmalloc for vmalloc so the cache buffer is mappable as per
> the Atmel SPI driver's requirements, otherwise an Oops would occur.
> 
> The original patch by Ian McDonnell <ian@brightstareng.com> was found here:
> http://lists.infradead.org/pipermail/linux-mtd/2007-December/020184.html
> 
> Signed-off-by: Anders Larsen <al@alarsen.net>
> Cc: Ian McDonnell <ian@brightstareng.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Matthias Kaehlcke <matthias@kaehlcke.net>
> Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> ---
>  drivers/mtd/mtdblock.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> Index: b/drivers/mtd/mtdblock.c
> ===================================================================
> --- a/drivers/mtd/mtdblock.c
> +++ b/drivers/mtd/mtdblock.c
> @@ -253,7 +253,11 @@ static int mtdblock_writesect(struct mtd
>  {
>  	struct mtdblk_dev *mtdblk = mtdblks[dev->devnum];
>  	if (unlikely(!mtdblk->cache_data && mtdblk->cache_size)) {
> +#ifdef CONFIG_SPI_ATMEL
> +		mtdblk->cache_data = kmalloc(mtdblk->mtd->erasesize, GFP_KERNEL);
> +#else
>  		mtdblk->cache_data = vmalloc(mtdblk->mtd->erasesize);
> +#endif
>  		if (!mtdblk->cache_data)
>  			return -EINTR;
>  		/* -EINTR is not really correct, but it is the best match
> @@ -322,7 +326,11 @@ static int mtdblock_release(struct mtd_b
>  		mtdblks[dev] = NULL;
>  		if (mtdblk->mtd->sync)
>  			mtdblk->mtd->sync(mtdblk->mtd);
> +#ifdef CONFIG_SPI_ATMEL
> +		kfree(mtdblk->cache_data);
> +#else
>  		vfree(mtdblk->cache_data);
> +#endif
>  		kfree(mtdblk);
>  	}
>  
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 

Re: [PATCH] Fix Oops with Atmel SPI

[Anders Larsen]

Hi Iwo,

On 2010-04-14 09:30:41, Iwo Mergler wrote:
> I wouldn't recommend that. MTD erase blocks are 64K or more. In a typical
> embedded system you will not be able to kmalloc that much memory after
> a few day's of operation - the page pool gets fragmented.

the original problem occurs with SPI flashes, which typically have a much
smaller erase block size (and it only occurs when they are driven by an Atmel
SoC SPI controller, hence the #ifdefs)

> A possibly better approach is to arrange for that memory to get allocated
> at driver start time.

The buffer in question is indeed allocated _once_ (at the first write
operation to the device) and only deallocated when the device is unmounted,
so allocating it at driver load time wouldn't make much difference IMHO.

I realize that my patch also affects e.g. parallel NOR flash on the system,
but unless an MTD device is unmounted/remounted over and over again, I don't
see a problem.

Did I miss something else?

> An even better approach would be to change the algorithm to operate on
> a list of smaller allocations, e.g. MTD page size.

That's unfortunately beyond my abilities, I fear.

Cheers
Anders

> Anders Larsen wrote:
> > Tweak MTD's cache allocation to make it work with the atmel DMA'ed SPI.
> > Substitute kmalloc for vmalloc so the cache buffer is mappable as per
> > the Atmel SPI driver's requirements, otherwise an Oops would occur.
> > 
> > The original patch by Ian McDonnell <ian@brightstareng.com> was found here:
> > http://lists.infradead.org/pipermail/linux-mtd/2007-December/020184.html
> > 
> > Signed-off-by: Anders Larsen <al@alarsen.net>
> > Cc: Ian McDonnell <ian@brightstareng.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Matthias Kaehlcke <matthias@kaehlcke.net>
> > Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > Cc: Nicolas Pitre <nico@fluxnic.net>

Re: [PATCH] Fix Oops with Atmel SPI

[Kevin Cernekee]

On Wed, Apr 14, 2010 at 12:57 AM, Anders Larsen <al@alarsen.net> wrote:
> the original problem occurs with SPI flashes, which typically have a much
> smaller erase block size (and it only occurs when they are driven by an Atmel
> SoC SPI controller, hence the #ifdefs)

FWIW, the 16MiB Spansion SPI NOR flashes I've been seeing on new
designs have 64KiB or 256KiB (!) eraseblocks.  256KiB eraseblocks are
likely to become even more common as the device capacity increases to
32MiB and beyond.

Re: [PATCH] Fix Oops with Atmel SPI

[Iwo Mergler]

Hi Anders,

Anders Larsen wrote:
> Hi Iwo,
> 
> On 2010-04-14 09:30:41, Iwo Mergler wrote:
>> I wouldn't recommend that. MTD erase blocks are 64K or more. In a typical
>> embedded system you will not be able to kmalloc that much memory after
>> a few day's of operation - the page pool gets fragmented.
> 
> the original problem occurs with SPI flashes, which typically have a much
> smaller erase block size (and it only occurs when they are driven by an Atmel
> SoC SPI controller, hence the #ifdefs)
> 
>> A possibly better approach is to arrange for that memory to get allocated
>> at driver start time.
> 
> The buffer in question is indeed allocated _once_ (at the first write
> operation to the device) and only deallocated when the device is unmounted,
> so allocating it at driver load time wouldn't make much difference IMHO.
> 

I'm sorry, I thought you were somewhere else in the MTD source.
The bad block handling code for NAND also has a full erase block
allocation, which happens during runtime. 

You are correct in that the mount time allocation should be
safe, for most systems.

Best regards,

Iwo

Re: [PATCH] Fix Oops with Atmel SPI

[Andrew Morton]

On Wed, 14 Apr 2010 09:57:20 +0200
Anders Larsen <al@alarsen.net> wrote:

> On 2010-04-14 09:30:41, Iwo Mergler wrote:
> > I wouldn't recommend that. MTD erase blocks are 64K or more. In a typical
> > embedded system you will not be able to kmalloc that much memory after
> > a few day's of operation - the page pool gets fragmented.
> 
> the original problem occurs with SPI flashes, which typically have a much
> smaller erase block size (and it only occurs when they are driven by an Atmel
> SoC SPI controller, hence the #ifdefs)
> 
> > A possibly better approach is to arrange for that memory to get allocated
> > at driver start time.
> 
> The buffer in question is indeed allocated _once_ (at the first write
> operation to the device) and only deallocated when the device is unmounted,
> so allocating it at driver load time wouldn't make much difference IMHO.
> 
> I realize that my patch also affects e.g. parallel NOR flash on the system,
> but unless an MTD device is unmounted/remounted over and over again, I don't
> see a problem.

Attempting the allocation at mtdblock_writesect()-time is the least
reliable approach.

It would be much more reliable to perform the allocation at boot-time
or modprobe-time.

It would be 100% reliable to perform the allocation at compile time
too!  If that's possible.  A statically allocated buffer with
appropriate locking around it to prevent it from getting scribbled on. 
Of course, this assumes that the buffer is shared between different
devices and it won't work at all if cache_data is really a "cache".

Ho-hum.  Anyway, please do try to find a way to make this allocation
more reliable.  It'd be pretty bad to have an embedded device go crump
when the user tries to save his data.

Also, the mdtblock code has changed a lot in this very area in the
linux-next tree (mtdblks[] has gone away).  So please redo any patch
against linux-next.


Finally..  Wouldn't it be better to just fix the atmel SPI driver so
that it doesn't barf when handed vmalloc'ed memory?  Who do we ridicule
about that?  <checks, adds cc>

Re: [PATCH] Fix Oops with Atmel SPI

[Anders Larsen]

On 2010-04-22 00:24:10, Andrew Morton wrote:
> Finally..  Wouldn't it be better to just fix the atmel SPI driver so
> that it doesn't barf when handed vmalloc'ed memory?  Who do we ridicule
> about that?  <checks, adds cc>

You mean something like this instead?

diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
index c4e0442..a9ad5e8 100644
--- a/drivers/spi/atmel_spi.c
+++ b/drivers/spi/atmel_spi.c
@@ -352,16 +352,30 @@ atmel_spi_dma_map_xfer(struct atmel_spi *as, struct spi_transfer *xfer)
 
 	xfer->tx_dma = xfer->rx_dma = INVALID_DMA_ADDRESS;
 	if (xfer->tx_buf) {
-		xfer->tx_dma = dma_map_single(dev,
-				(void *) xfer->tx_buf, xfer->len,
-				DMA_TO_DEVICE);
+		if (is_vmalloc_addr(xfer->tx_buf))
+			xfer->tx_dma = dma_map_page(dev,
+					vmalloc_to_page(xfer->tx_buf),
+					(unsigned long)xfer->tx_buf & (PAGE_SIZE-1),
+					xfer->len,
+					DMA_TO_DEVICE);
+		else
+			xfer->tx_dma = dma_map_single(dev,
+					(void *) xfer->tx_buf, xfer->len,
+					DMA_TO_DEVICE);
 		if (dma_mapping_error(dev, xfer->tx_dma))
 			return -ENOMEM;
 	}
 	if (xfer->rx_buf) {
-		xfer->rx_dma = dma_map_single(dev,
-				xfer->rx_buf, xfer->len,
-				DMA_FROM_DEVICE);
+		if (is_vmalloc_addr(xfer->rx_buf))
+			xfer->rx_dma = dma_map_page(dev,
+					vmalloc_to_page(xfer->rx_buf),
+					(unsigned long)xfer->rx_buf & (PAGE_SIZE-1),
+					xfer->len,
+					DMA_FROM_DEVICE);
+		else
+			xfer->rx_dma = dma_map_single(dev,
+					xfer->rx_buf, xfer->len,
+					DMA_FROM_DEVICE);
 		if (dma_mapping_error(dev, xfer->rx_dma)) {
 			if (xfer->tx_buf)
 				dma_unmap_single(dev,

Re: [PATCH] Fix Oops with Atmel SPI

[Andrew Morton]

On Wed, 19 May 2010 13:05:00 +0200
Anders Larsen <al@alarsen.net> wrote:

> On 2010-04-22 00:24:10, Andrew Morton wrote:
> > Finally..  Wouldn't it be better to just fix the atmel SPI driver so
> > that it doesn't barf when handed vmalloc'ed memory?  Who do we ridicule
> > about that?  <checks, adds cc>
> 
> You mean something like this instead?

That looks simple enough.  How do we get it tested, changelogged and
merged up?  Haavard, can you please take a look?


> diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
> index c4e0442..a9ad5e8 100644
> --- a/drivers/spi/atmel_spi.c
> +++ b/drivers/spi/atmel_spi.c
> @@ -352,16 +352,30 @@ atmel_spi_dma_map_xfer(struct atmel_spi *as, struct spi_transfer *xfer)
>  
>  	xfer->tx_dma = xfer->rx_dma = INVALID_DMA_ADDRESS;
>  	if (xfer->tx_buf) {
> -		xfer->tx_dma = dma_map_single(dev,
> -				(void *) xfer->tx_buf, xfer->len,
> -				DMA_TO_DEVICE);
> +		if (is_vmalloc_addr(xfer->tx_buf))
> +			xfer->tx_dma = dma_map_page(dev,
> +					vmalloc_to_page(xfer->tx_buf),
> +					(unsigned long)xfer->tx_buf & (PAGE_SIZE-1),
> +					xfer->len,
> +					DMA_TO_DEVICE);
> +		else
> +			xfer->tx_dma = dma_map_single(dev,
> +					(void *) xfer->tx_buf, xfer->len,
> +					DMA_TO_DEVICE);
>  		if (dma_mapping_error(dev, xfer->tx_dma))
>  			return -ENOMEM;
>  	}
>  	if (xfer->rx_buf) {
> -		xfer->rx_dma = dma_map_single(dev,
> -				xfer->rx_buf, xfer->len,
> -				DMA_FROM_DEVICE);
> +		if (is_vmalloc_addr(xfer->rx_buf))
> +			xfer->rx_dma = dma_map_page(dev,
> +					vmalloc_to_page(xfer->rx_buf),
> +					(unsigned long)xfer->rx_buf & (PAGE_SIZE-1),
> +					xfer->len,
> +					DMA_FROM_DEVICE);
> +		else
> +			xfer->rx_dma = dma_map_single(dev,
> +					xfer->rx_buf, xfer->len,
> +					DMA_FROM_DEVICE);
>  		if (dma_mapping_error(dev, xfer->rx_dma)) {
>  			if (xfer->tx_buf)
>  				dma_unmap_single(dev,
> 

Re: [PATCH] Fix Oops with Atmel SPI

[Ian McDonnell]

Anders,

I just tested one path, the "if (xfer->rx_buf)...", on
2.6.33 plus the at91 patch
http://maxim.org.za/AT91RM9200/2.6/2.6.33-at91.patch.gz
running on AT91SAM9260.

The test case involved doing i/o via the /dev/mtdblock
interface -- but this only exercises the rx_buf/vmalloc path --
MTD reads a block into the cache-buf to merge the write data. Not 
sure that we have any use cases for the tx_buf path using MTD.

-Ian

On Friday 21 May 2010, Andrew Morton wrote:
> On Wed, 19 May 2010 13:05:00 +0200
>
> Anders Larsen <al@alarsen.net> wrote:
> > On 2010-04-22 00:24:10, Andrew Morton wrote:
> > > Finally..  Wouldn't it be better to just fix the atmel SPI
> > > driver so that it doesn't barf when handed vmalloc'ed
> > > memory?  Who do we ridicule about that?  <checks, adds cc>
> >
> > You mean something like this instead?
>
> That looks simple enough.  How do we get it tested,
> changelogged and merged up?  Haavard, can you please take a
> look?
>
> > diff --git a/drivers/spi/atmel_spi.c
> > b/drivers/spi/atmel_spi.c index c4e0442..a9ad5e8 100644
> > --- a/drivers/spi/atmel_spi.c
> > +++ b/drivers/spi/atmel_spi.c
> > @@ -352,16 +352,30 @@ atmel_spi_dma_map_xfer(struct
> > atmel_spi *as, struct spi_transfer *xfer)
> >
> >  	xfer->tx_dma = xfer->rx_dma = INVALID_DMA_ADDRESS;
> >  	if (xfer->tx_buf) {
> > -		xfer->tx_dma = dma_map_single(dev,
> > -				(void *) xfer->tx_buf, xfer->len,
> > -				DMA_TO_DEVICE);
> > +		if (is_vmalloc_addr(xfer->tx_buf))
> > +			xfer->tx_dma = dma_map_page(dev,
> > +					vmalloc_to_page(xfer->tx_buf),
> > +					(unsigned long)xfer->tx_buf & (PAGE_SIZE-1),
> > +					xfer->len,
> > +					DMA_TO_DEVICE);
> > +		else
> > +			xfer->tx_dma = dma_map_single(dev,
> > +					(void *) xfer->tx_buf, xfer->len,
> > +					DMA_TO_DEVICE);
> >  		if (dma_mapping_error(dev, xfer->tx_dma))
> >  			return -ENOMEM;
> >  	}
> >  	if (xfer->rx_buf) {
> > -		xfer->rx_dma = dma_map_single(dev,
> > -				xfer->rx_buf, xfer->len,
> > -				DMA_FROM_DEVICE);
> > +		if (is_vmalloc_addr(xfer->rx_buf))
> > +			xfer->rx_dma = dma_map_page(dev,
> > +					vmalloc_to_page(xfer->rx_buf),
> > +					(unsigned long)xfer->rx_buf & (PAGE_SIZE-1),
> > +					xfer->len,
> > +					DMA_FROM_DEVICE);
> > +		else
> > +			xfer->rx_dma = dma_map_single(dev,
> > +					xfer->rx_buf, xfer->len,
> > +					DMA_FROM_DEVICE);
> >  		if (dma_mapping_error(dev, xfer->rx_dma)) {
> >  			if (xfer->tx_buf)
> >  				dma_unmap_single(dev,

Re: [PATCH] Fix Oops with Atmel SPI

[Haavard Skinnemoen]

Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 19 May 2010 13:05:00 +0200
> Anders Larsen <al@alarsen.net> wrote:
> 
> > On 2010-04-22 00:24:10, Andrew Morton wrote:
> > > Finally..  Wouldn't it be better to just fix the atmel SPI driver so
> > > that it doesn't barf when handed vmalloc'ed memory?  Who do we ridicule
> > > about that?  <checks, adds cc>
> > 
> > You mean something like this instead?
> 
> That looks simple enough.  How do we get it tested, changelogged and
> merged up?  Haavard, can you please take a look?

Sure. Sorry for the late response; I've been traveling for the last two
weeks.

Did anyone check what other drivers do to handle this case? Surely this
isn't the only driver which supports DMA?

> > diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
> > index c4e0442..a9ad5e8 100644
> > --- a/drivers/spi/atmel_spi.c
> > +++ b/drivers/spi/atmel_spi.c
> > @@ -352,16 +352,30 @@ atmel_spi_dma_map_xfer(struct atmel_spi *as, struct spi_transfer *xfer)
> >  
> >  	xfer->tx_dma = xfer->rx_dma = INVALID_DMA_ADDRESS;
> >  	if (xfer->tx_buf) {
> > -		xfer->tx_dma = dma_map_single(dev,
> > -				(void *) xfer->tx_buf, xfer->len,
> > -				DMA_TO_DEVICE);
> > +		if (is_vmalloc_addr(xfer->tx_buf))
> > +			xfer->tx_dma = dma_map_page(dev,
> > +					vmalloc_to_page(xfer->tx_buf),
> > +					(unsigned long)xfer->tx_buf & (PAGE_SIZE-1),
> > +					xfer->len,
> > +					DMA_TO_DEVICE);

Ok, this should be fine for small transfers, but what happens if the
transfer crosses a page boundary? Are there any guarantees that this
will never happen? What callers are passing vmalloc'ed memory in the
first place?

Ditto for the rx path.

Haavard

Re: [PATCH] Fix Oops with Atmel SPI

[Artem Bityutskiy]

On Tue, 2010-04-13 at 13:31 +0200, Anders Larsen wrote:
> Tweak MTD's cache allocation to make it work with the atmel DMA'ed SPI.
> Substitute kmalloc for vmalloc so the cache buffer is mappable as per
> the Atmel SPI driver's requirements, otherwise an Oops would occur.
> 
> The original patch by Ian McDonnell <ian@brightstareng.com> was found here:
> http://lists.infradead.org/pipermail/linux-mtd/2007-December/020184.html
> 
> Signed-off-by: Anders Larsen <al@alarsen.net>
> Cc: Ian McDonnell <ian@brightstareng.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Matthias Kaehlcke <matthias@kaehlcke.net>
> Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> ---
>  drivers/mtd/mtdblock.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> Index: b/drivers/mtd/mtdblock.c
> ===================================================================
> --- a/drivers/mtd/mtdblock.c
> +++ b/drivers/mtd/mtdblock.c
> @@ -253,7 +253,11 @@ static int mtdblock_writesect(struct mtd
>  {
>  	struct mtdblk_dev *mtdblk = mtdblks[dev->devnum];
>  	if (unlikely(!mtdblk->cache_data && mtdblk->cache_size)) {
> +#ifdef CONFIG_SPI_ATMEL
> +		mtdblk->cache_data = kmalloc(mtdblk->mtd->erasesize, GFP_KERNEL);
> +#else
>  		mtdblk->cache_data = vmalloc(mtdblk->mtd->erasesize);
> +#endif
>  		if (!mtdblk->cache_data)
>  			return -EINTR;
>  		/* -EINTR is not really correct, but it is the best match
> @@ -322,7 +326,11 @@ static int mtdblock_release(struct mtd_b
>  		mtdblks[dev] = NULL;
>  		if (mtdblk->mtd->sync)
>  			mtdblk->mtd->sync(mtdblk->mtd);
> +#ifdef CONFIG_SPI_ATMEL
> +		kfree(mtdblk->cache_data);
> +#else
>  		vfree(mtdblk->cache_data);
> +#endif
>  		kfree(mtdblk);
>  	}

This is an old problem. Instead of doing this dirty hack, change the
code and teach it to work with array of 1-4 pages , not with buffers.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)