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: Iwo Mergler on 14 Apr 2010 03:40 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(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); > } > > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > > -- 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 14 Apr 2010 04:00 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(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> -- 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: Kevin Cernekee on 14 Apr 2010 14:20 On Wed, Apr 14, 2010 at 12:57 AM, Anders Larsen <al(a)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. -- 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: Iwo Mergler on 15 Apr 2010 03:40 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 -- 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 Apr 2010 18:30
On Wed, 14 Apr 2010 09:57:20 +0200 Anders Larsen <al(a)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> -- 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/ |