| Message ID | 201009220343.33025.vapier.adi@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2010-09-22 at 03:43 -0400, Mike Frysinger wrote: > On Sunday, September 12, 2010 03:54:03 Artem Bityutskiy wrote: > > On Sun, 2010-09-12 at 00:03 -0400, Mike Frysinger wrote: > > > On Sat, Sep 11, 2010 at 02:32, Artem Bityutskiy wrote: > > > > It will be confusing if the same word is used in MTD for "unmarking" > > > > eraseblocks. How about: 'force erase' or 'bad erase' ? > > > > > > that makes it sound like an option to the existing MEMERASE operation. > > > > > > so i guess what if we just do that -- extend the erase_info_user > > > > > > structure to contain a flags field and add a MEMERASE2 that works with > > > the larger structure ? for now we'd only have one option (FORCE), but > > > it makes it easy to extend in the future. > > > > Ohh, this was so stupid of me to not ask people to add extra fields to > > 'struct erase_info_user64' which was introduced relatively recently... I > > always add extra fields to ioctl data structures... > > > > But yeah, what you say sounds ok to me. > > here's a POC that works for me. with a simple tweak to `flash_eraseall`, i > can now recover my mtd devices with funky OOB layouts. > > ive only extended MEMERASE64 as i believe the non-64 variants are EOL ? or > should i also extend the 32bit interface as well ? We need something consistent. This patch will just erase the bad eraseblock. This will not mark it as good in the BBT (neither in-ram nor on-flash). If the erasure succeeds, the block will still be marked as bad in BBT, but after reboot, if the BBT is not on-flash, it will be treated as good eraseblock, because scanning will not find the bad block marker anymore. If the BBT is on-flash, it'll stay bad. This is inconsistent. > +struct erase_info_user64_flags { > + __u64 start; > + __u64 length; > + __u32 flags; > +}; I think it needs to have som more room for possible future extentions. Also, good tone for ioctls is to make them to be multiple of 64-bit - less pain in mixed 32/64 bit setups. Please, add some u8 padding[12] field and add a comment that this has to be zero, and may be used in future. Then in future we may extend ioctls and add more fields. > +#define MEMERASE64_FLAGS _IOW('M', 23, struct erase_info_user64_flags) I do not like the name. We may add something else, not just flags later. May be MEMERASE64_EXTENDED ?
On Thu, Sep 23, 2010 at 08:28, Artem Bityutskiy wrote: > On Wed, 2010-09-22 at 03:43 -0400, Mike Frysinger wrote: >> here's a POC that works for me. with a simple tweak to `flash_eraseall`, i >> can now recover my mtd devices with funky OOB layouts. >> >> ive only extended MEMERASE64 as i believe the non-64 variants are EOL ? or >> should i also extend the 32bit interface as well ? > > We need something consistent. This patch will just erase the bad > eraseblock. This will not mark it as good in the BBT (neither in-ram nor > on-flash). If the erasure succeeds, the block will still be marked as > bad in BBT, but after reboot, if the BBT is not on-flash, it will be > treated as good eraseblock, because scanning will not find the bad block > marker anymore. If the BBT is on-flash, it'll stay bad. This is > inconsistent. i dont think the current BBT API supports marking bad blocks as good ? if it does, could you highlight it for me ? >> +struct erase_info_user64_flags { >> + __u64 start; >> + __u64 length; >> + __u32 flags; >> +}; > > I think it needs to have som more room for possible future extentions. > Also, good tone for ioctls is to make them to be multiple of 64-bit - > less pain in mixed 32/64 bit setups. > > Please, add some > > u8 padding[12] > > field and add a comment that this has to be zero, and may be used in > future. > > Then in future we may extend ioctls and add more fields. well, the idea i have with flags is that we carve one bit out to mean "extend". so if in the future we wanted to extend the struct, we set that flag and the kernel knows that userspace wants the larger version. that gives you a way of extending things indefinitely at the cost of 1 bit. which also means i should probably carve that bit out now and have the kernel return an error when it is set today. >> +#define MEMERASE64_FLAGS _IOW('M', 23, struct erase_info_user64_flags) > > I do not like the name. We may add something else, not just flags later. i was going to use "MEMERASE2", but that didnt scale well with "MEMERASE264". > May be MEMERASE64_EXTENDED ? OK -mike
On Thu, 2010-09-23 at 15:55 -0400, Mike Frysinger wrote: > On Thu, Sep 23, 2010 at 08:28, Artem Bityutskiy wrote: > > On Wed, 2010-09-22 at 03:43 -0400, Mike Frysinger wrote: > >> here's a POC that works for me. with a simple tweak to `flash_eraseall`, i > >> can now recover my mtd devices with funky OOB layouts. > >> > >> ive only extended MEMERASE64 as i believe the non-64 variants are EOL ? or > >> should i also extend the 32bit interface as well ? > > > > We need something consistent. This patch will just erase the bad > > eraseblock. This will not mark it as good in the BBT (neither in-ram nor > > on-flash). If the erasure succeeds, the block will still be marked as > > bad in BBT, but after reboot, if the BBT is not on-flash, it will be > > treated as good eraseblock, because scanning will not find the bad block > > marker anymore. If the BBT is on-flash, it'll stay bad. This is > > inconsistent. > > i dont think the current BBT API supports marking bad blocks as good ? > if it does, could you highlight it for me ? No, unfortunately, it'd require some work with ancient code - not very easy task, IMO. > >> +struct erase_info_user64_flags { > >> + __u64 start; > >> + __u64 length; > >> + __u32 flags; > >> +}; > > > > I think it needs to have som more room for possible future extentions. > > Also, good tone for ioctls is to make them to be multiple of 64-bit - > > less pain in mixed 32/64 bit setups. > > > > Please, add some > > > > u8 padding[12] > > > > field and add a comment that this has to be zero, and may be used in > > future. > > > > Then in future we may extend ioctls and add more fields. > > well, the idea i have with flags is that we carve one bit out to mean > "extend". so if in the future we wanted to extend the struct, we set > that flag and the kernel knows that userspace wants the larger > version. that gives you a way of extending things indefinitely at the > cost of 1 bit. I think this is not fast-path and it does not hurt to have extra fields just in case someone else needs them in future. Also, make sure size of this structure is multiple of 8, not 4. > which also means i should probably carve that bit out now and have the > kernel return an error when it is set today. I think no, you should ignore them. But you should write a comment that userspace has to zero them, this is kind of part of API. Then when new flags are added, old kernels will not fail, but ignore them. But yes, this will work only if userspace follows the API.
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index 5b081cb..68c2864 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -513,6 +513,7 @@ static int mtd_ioctl(struct inode *inode, struct file *file, case MEMERASE: case MEMERASE64: + case MEMERASE64_FLAGS: { struct erase_info *erase; @@ -538,6 +539,17 @@ static int mtd_ioctl(struct inode *inode, struct file *file, } erase->addr = einfo64.start; erase->len = einfo64.length; + } else if (cmd == MEMERASE64_FLAGS) { + struct erase_info_user64_flags einfo64; + + if (copy_from_user(&einfo64, argp, + sizeof(struct erase_info_user64_flags))) { + kfree(erase); + return -EFAULT; + } + erase->addr = einfo64.start; + erase->len = einfo64.length; + erase->flags = einfo64.flags; } else { struct erase_info_user einfo32; diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 8f2958f..e440d84 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2354,7 +2354,8 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr, /* * heck if we have a bad block, we do not erase bad blocks ! */ - if (nand_block_checkbad(mtd, ((loff_t) page) << + if (!(instr->flags & MTD_ERASE_BADBLOCKS) && + nand_block_checkbad(mtd, ((loff_t) page) << chip->page_shift, 0, allowbbt)) { printk(KERN_WARNING "%s: attempt to erase a bad block " "at page 0x%08x\n", __func__, page); diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 0f32a9b..f1cda73 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -37,6 +37,7 @@ struct erase_info { struct mtd_info *mtd; uint64_t addr; uint64_t len; + uint32_t flags; uint64_t fail_addr; u_long time; u_long retries; diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h index be51ae2..fef14ba 100644 --- a/include/mtd/mtd-abi.h +++ b/include/mtd/mtd-abi.h @@ -17,6 +17,12 @@ struct erase_info_user64 { __u64 length; }; +struct erase_info_user64_flags { + __u64 start; + __u64 length; + __u32 flags; +}; + struct mtd_oob_buf { __u32 start; __u32 length; @@ -61,6 +67,9 @@ struct mtd_oob_buf64 { #define MTD_OTP_FACTORY 1 #define MTD_OTP_USER 2 +/* Erase flags */ +#define MTD_ERASE_BADBLOCKS 0x1 + struct mtd_info_user { __u8 type; __u32 flags; @@ -110,6 +119,7 @@ struct otp_info { #define MEMERASE64 _IOW('M', 20, struct erase_info_user64) #define MEMWRITEOOB64 _IOWR('M', 21, struct mtd_oob_buf64) #define MEMREADOOB64 _IOWR('M', 22, struct mtd_oob_buf64) +#define MEMERASE64_FLAGS _IOW('M', 23, struct erase_info_user64_flags) /* * Obsolete legacy interface. Keep it in order not to break userspace