Skip to contentSkip to conversation

nand_update_bbt fix

PATCH Andrew McKay 3 participants 6 messages Aug 10, 2009 – Aug 13, 2009

Subsystems (2)
Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

Andrew McKay

author
#0 lore raw

Hello,

For NAND parts with 2K pages or larger, kmalloc of one erase block will exceed 128K and fail. A vmalloc is used in nand_scan_bbt to allocate enough memory for one erase block. This should likely be the case for nand_update_bbt as well.

Not sure the proper way to submit a patch, but here's a diff of the change:

drivers/mtd/nand/nand_bbt.c +2 -2
index 55c23e5..43b8d08 100644
@@ -1030,7 +1030,7 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
  /* Allocate a temporary buffer for one eraseblock incl. oob */
  len = (1 << this->bbt_erase_shift);
  len += (len >> this->page_shift) * mtd->oobsize;
- buf = kmalloc(len, GFP_KERNEL);
+ buf = vmalloc(len);
  if (!buf) {
    printk(KERN_ERR "nand_update_bbt: Out of memory\n");
    return -ENOMEM;

@@ -1063,7 +1063,7 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
  }
 
  out:
- kfree(buf);
+ vfree(buf);
  return res;
 }

Andrew McKay
Iders Inc.

#1 lore raw
Quoted 5 lines · Andrew McKay

Hello,

For NAND parts with 2K pages or larger, kmalloc of one erase block will exceed 128K and fail. A vmalloc is used in nand_scan_bbt to allocate enough memory for one erase block. This should likely be the case for nand_update_bbt as well.

I think this is not about parts with 2K pages, but about parts with 256KiB eraseblocks.

Quoted 14 lines
drivers/mtd/nand/nand_bbt.c +1 -1
index 55c23e5..43b8d08 100644
@@ -1030,7 +1030,7 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
   /* Allocate a temporary buffer for one eraseblock incl. oob */
   len = (1 << this->bbt_erase_shift);
   len += (len >> this->page_shift) * mtd->oobsize;
- buf = kmalloc(len, GFP_KERNEL);
+ buf = vmalloc(len);
   if (!buf) {
     printk(KERN_ERR "nand_update_bbt: Out of memory\n");
     return -ENOMEM;

drivers/mtd/nand/nand_bbt.c int nand_update_bbt(struct mtd_info *mtd, loff_t offs) · line 1068

Quoted 7 lines
drivers/mtd/nand/nand_bbt.c +1 -1
@@ -1063,7 +1063,7 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
   }
 
   out:
- kfree(buf);
+ vfree(buf);
   return res;
  }

I would disagree with this patch. Other people are fighting against vmalloc, because they want to do DMA in their drivers, but they cannot. I've CCed Zhang who is doing the opposite to what you do.

I think you should instead split the array on several smaller parts and work with those parts. And since this is a very common task in MTD, it is better to create a helper library.

You may also take a look at what I suggested to Zhang:
http://lists.infradead.org/pipermail/linux-mtd/2009-August/026845.html

Signature · 2 lines
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
author
#2 lore raw
Quoted 7 lines · Artem Bityutskiy
Quoted 3 lines · Andrew McKay

Hello,

For NAND parts with 2K pages or larger, kmalloc of one erase block will exceed 128K and fail. A vmalloc is used in nand_scan_bbt to allocate enough memory for one erase block. This should likely be the case for nand_update_bbt as well.

I think this is not about parts with 2K pages, but about parts with 256KiB eraseblocks.

Yes, you're correct, any part that has an erase block, including OOB, that exceeds 128K bytes.

Quoted 17 lines
Quoted 17 lines
drivers/mtd/nand/nand_bbt.c +1 -1
index 55c23e5..43b8d08 100644
@@ -1030,7 +1030,7 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
   /* Allocate a temporary buffer for one eraseblock incl. oob */
   len = (1 << this->bbt_erase_shift);
   len += (len >> this->page_shift) * mtd->oobsize;
- buf = kmalloc(len, GFP_KERNEL);
+ buf = vmalloc(len);
   if (!buf) {
     printk(KERN_ERR "nand_update_bbt: Out of memory\n");
     return -ENOMEM;

@@ -1063,7 +1063,7 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
   }
Quoted 9 lines

drivers/mtd/nand/nand_bbt.c int nand_update_bbt(struct mtd_info *mtd, loff_t offs) · line 1068

Quoted 5 lines
drivers/mtd/nand/nand_bbt.c +1 -1
   out:
- kfree(buf);
+ vfree(buf);
   return res;
  }

I would disagree with this patch. Other people are fighting against vmalloc, because they want to do DMA in their drivers, but they cannot. I've CCed Zhang who is doing the opposite to what you do.

I understand vmalloc is frowned upon in general, but then why is it already being used in nand_scan_bbt? Any fix that avoids vmalloc for nand_update_bbt should be used in nand_scan_bbt as well.

Quoted 6 lines

I think you should instead split the array on several smaller parts and work with those parts. And since this is a very common task in MTD, it is better to create a helper library.

You may also take a look at what I suggested to Zhang:
http://lists.infradead.org/pipermail/linux-mtd/2009-August/026845.html

Thanks, I will take a look.

Andrew

#3 lore raw
Quoted 6 lines · Andrew McKay
Quoted 6 lines
Quoted 5 lines

out:
- kfree(buf);
+ vfree(buf);
return res;
}

Quoted 7 lines
Quoted 3 lines

I would disagree with this patch. Other people are fighting against vmalloc, because they want to do DMA in their drivers, but they cannot. I've CCed Zhang who is doing the opposite to what you do.

I understand vmalloc is frowned upon in general, but then why is it already being used in nand_scan_bbt? Any fix that avoids vmalloc for nand_update_bbt should be used in nand_scan_bbt as well.

My position is:
* vmalloc is a problem because it prevents DMA
* kmalloc is a problem because large allocations of contiguous memory are impossible

Thus, I think people should invent some nice solution for the whole issue instead of turning vmalloc's into kmallocks and back and forth. I'm CCing David Brownell because AFAIR he was discussing similar things on lkml some time ago.

Signature · 2 lines
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
#4 lore raw
Quoted 7 lines · Artem Bityutskiy

My position is:
   * vmalloc is a problem because it prevents DMA
   * kmalloc is a problem because large allocations of contiguous memory are impossible

Thus, I think people should invent some nice solution for the whole issue instead of turning vmalloc's into kmallocks and back and forth.

BBT is a constrained sub-problem, but not the only one.

(Another BBT issue: I've thought that with MLC chips and their small limits on number-of-erases, the current waste of BBT pages deserves more attention. On a 2 GByte chip with 4KB pages and blocks at 256KB, each block could hold 64 BBT versions, with newer ones after older ones, even at one-per-page. But today's BBT code is dumb: one-per-block. That's a lot of needless and extra erasures for BBT blocks...)

Quoted 3 lines

I'm CCing
David Brownell because AFAIR he was discussing similar things on lkml some time ago.

The MTD stack is DMA-unfriendly today.

The issue I saw was with SPI flash chips, where the underlying SPI master controller often uses DMA ... causing trouble for certain code paths through MTD (or was it just JFFS2?).

I'm not sure it's well understood which things are DMA-unsafe. Using vmalloc is one problem. There's also code making more subtle assumptions, which dma-incoherent caches will break...

There are two issues I see. One is "does it work at all"; that was the SPI issue. Another is performance ... it's easy for DMA overheads to be excessive: DMA setup/teardown can cost more than the PIO code would, even ignoring cache operation costs. Fixing performance will likely require interface changes.

Either issue will need care to fix. Given that NOR is now becoming less prevalent, I'd suggest that "people" doing such work focus on large-page NAND.

- Dave

#5 lore raw
Quoted 11 lines · David Brownell
Quoted 7 lines · Artem Bityutskiy

My position is:
* vmalloc is a problem because it prevents DMA
* kmalloc is a problem because large allocations of contiguous memory are impossible

Thus, I think people should invent some nice solution for the whole issue instead of turning vmalloc's into kmallocks and back and forth.

BBT is a constrained sub-problem, but not the only one.

Quoted 7 lines

(Another BBT issue: I've thought that with MLC chips and their small limits on number-of-erases, the current waste of BBT pages deserves more attention. On a 2 GByte chip with 4KB pages and blocks at 256KB, each block could hold 64 BBT versions, with newer ones after older ones, even at one-per-page. But today's BBT code is dumb: one-per-block. That's a lot of needless and extra erasures for BBT blocks...)

Agree. IMO, today's MTD is fits the "ancient crap" classification, and badly needs some brave knight who would improve it.

Quoted 9 lines
Quoted 3 lines

I'm CCing
David Brownell because AFAIR he was discussing similar things on lkml some time ago.

The MTD stack is DMA-unfriendly today.

The issue I saw was with SPI flash chips, where the underlying SPI master controller often uses DMA ... causing trouble for certain code paths through MTD (or was it just JFFS2?).

UBI / UBIFS also send vmalloc'ed buffers :-)

Signature · 2 lines
Best Regards,
Artem Bityutskiy (Артём Битюцкий)