Fix Oops with Atmel SPI
in [Kernel]
|
Prev: powerpc/perf_event: Fix oops due to perf_event_do_pending call
Next: Security: Replace dac_mmap_min_addr to mmap_min_addr in cap_file_mmap()
From: Artem Bityutskiy on 27 Apr 2010 09:10 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(a)brightstareng.com> was found here: > http://lists.infradead.org/pipermail/linux-mtd/2007-December/020184.html > > Signed-off-by: Anders Larsen <al(a)alarsen.net> > Cc: Ian McDonnell <ian(a)brightstareng.com> > Cc: David Woodhouse <dwmw2(a)infradead.org> > Cc: Matthias Kaehlcke <matthias(a)kaehlcke.net> > Cc: Artem Bityutskiy <Artem.Bityutskiy(a)nokia.com> > Cc: Nicolas Pitre <nico(a)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 (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Anders Larsen on 19 May 2010 07:10 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, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 21 May 2010 15:10 On Wed, 19 May 2010 13:05:00 +0200 Anders Larsen <al(a)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, > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ian McDonnell on 24 May 2010 11:50 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(a)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, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Haavard Skinnemoen on 28 May 2010 05:40
Andrew Morton <akpm(a)linux-foundation.org> wrote: > On Wed, 19 May 2010 13:05:00 +0200 > Anders Larsen <al(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |