| Message ID | 1324406536-18250-2-git-send-email-mikedunn@newsguy.com |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote: > Ensure driver methods always go through the wrapper functions in the > partitioning code by creating a single "partition" on otherwise unpartitioned > devices. > > Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently > out-of-tree). > > Signed-off-by: Mike Dunn <mikedunn@newsguy.com> Looks good to me in general. Does it change anything from the user-space POW (API/ABI)? One thing to be aware of: some drivers do like this: drivers/mtd/maps/plat-ram.c: /* check to see if there are any available partitions, or wether * to add this device whole */ err = mtd_device_parse_register(info->mtd, pdata->probes, 0, pdata->partitions, pdata->nr_partitions); if (!err) dev_info(&pdev->dev, "registered mtd device\n"); if (pdata->nr_partitions) { /* add the whole device. */ err = mtd_device_register(info->mtd, NULL, 0); if (err) { dev_err(&pdev->dev, "failed to register the entire device\n"); } } Could you please: 1. Try to find drivers like this and kill the redundant mtd_device_register() call. 2. There is no guarantee you will find all. So you could add a check in mtd_device_parse_register() which would print a warning if the device is added for the second time and return. So the code would work and people would be able to notice the warning and fix up the driver. > - add_mtd_device(&slave->mtd); > + ret = add_mtd_device(&slave->mtd); > + if (ret == 1) > + return -ENODEV; Is this an unrelated change? Would you separate it out?
On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote: > + /* > + * For unpartitioned devices, create a single "partition" that > + * spans the entire device, so that driver methods go through > + * partition wrappers in all cases. > + */ > + struct mtd_partition single_part = { > + .name = (char *)mtd->name, > + .offset = 0, > + .size = mtd->size, > + .mask_flags = 0, > + .ecclayout = mtd->ecclayout, > + }; > + err = add_mtd_partitions(mtd, &single_part, 1); > } There is a problem with this approach :-( Look at the 'mtd_blkpg_ioctl()' function which is used to re-partition MTD device from user-space. It is a bit strange, but we use block device for re-partition in order to not invent new ioctls. This function has this code: /* Only master mtd device must be used to add partitions */ if (mtd_is_partition(mtd)) return -EINVAL; and your patch will brake re-partitioning. So I guess we should go back to your first patch where you just change the ->read() / ->read_oob() interface. Or: 1. Rework whole MTD interface from ->read()/->write()/etc (pointers to functions) to mtd_read(mtd, ...)/mtd_write(mtd, ...)/etc. 2. In the new functions you may do the stuff you do at patch 2. For me the second approach is more plausible.
On Thu, 2011-12-22 at 15:05 +0200, Artem Bityutskiy wrote: > 1. Rework whole MTD interface from ->read()/->write()/etc (pointers to > functions) to mtd_read(mtd, ...)/mtd_write(mtd, ...)/etc. I will send a patch-set for this change tomorrow. > 2. In the new functions you may do the stuff you do at patch 2. Then you will be able to do this change easily.
On 12/22/2011 04:34 AM, Artem Bityutskiy wrote: > On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote: >> Ensure driver methods always go through the wrapper functions in the >> partitioning code by creating a single "partition" on otherwise unpartitioned >> devices. >> >> Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently >> out-of-tree). >> >> Signed-off-by: Mike Dunn <mikedunn@newsguy.com> > Looks good to me in general. Does it change anything from the user-space > POW (API/ABI)? No, should be completely transparent (at least that's my intention). The only new behavior is that the partition code puts a string in the log with the partition boundaries and partition name, which in the single partition case is mtd->name, which should avoid any confusion. > One thing to be aware of: some drivers do like this: > > drivers/mtd/maps/plat-ram.c: > > /* check to see if there are any available partitions, or wether > * to add this device whole */ > > err = mtd_device_parse_register(info->mtd, pdata->probes, 0, > pdata->partitions, pdata->nr_partitions); > if (!err) > dev_info(&pdev->dev, "registered mtd device\n"); > > if (pdata->nr_partitions) { > /* add the whole device. */ > err = mtd_device_register(info->mtd, NULL, 0); > if (err) { > dev_err(&pdev->dev, > "failed to register the entire device\n"); > } > } > > Could you please: > 1. Try to find drivers like this and kill the redundant mtd_device_register() > call. > 2. There is no guarantee you will find all. So you could add a check in > mtd_device_parse_register() which would print a warning if the device is > added for the second time and return. So the code would work and people > would be able to notice the warning and fix up the driver. OK. Sorry, missed that. >> - add_mtd_device(&slave->mtd); >> + ret = add_mtd_device(&slave->mtd); >> + if (ret == 1) >> + return -ENODEV; > Is this an unrelated change? Would you separate it out? No, it's related. Currently, mtd_device_parse_register() does the above if no partitions are created, whereas here in add_mtd_partitions() the return code of add_mtd_device() is not checked, so I added this to be consistent with the previous behavior. mtd_device_parse_register() propagates this return code back to the caller, so the result is the same. Thanks, Mike
On 12/22/2011 05:05 AM, Artem Bityutskiy wrote: > On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote: >> + /* >> + * For unpartitioned devices, create a single "partition" that >> + * spans the entire device, so that driver methods go through >> + * partition wrappers in all cases. >> + */ >> + struct mtd_partition single_part = { >> + .name = (char *)mtd->name, >> + .offset = 0, >> + .size = mtd->size, >> + .mask_flags = 0, >> + .ecclayout = mtd->ecclayout, >> + }; >> + err = add_mtd_partitions(mtd, &single_part, 1); >> } > There is a problem with this approach :-( > > Look at the 'mtd_blkpg_ioctl()' function which is used to re-partition > MTD device from user-space. It is a bit strange, but we use block device > for re-partition in order to not invent new ioctls. Sorry, missed that too. So many perils... Thanks, Mike
On 12/22/2011 10:03 AM, Artem Bityutskiy wrote: > On Thu, 2011-12-22 at 15:05 +0200, Artem Bityutskiy wrote: >> 1. Rework whole MTD interface from ->read()/->write()/etc (pointers to >> functions) to mtd_read(mtd, ...)/mtd_write(mtd, ...)/etc. > I will send a patch-set for this change tomorrow. > >> 2. In the new functions you may do the stuff you do at patch 2. > Then you will be able to do this change easily. > OK, thank you Artem. I'll watch for it and follow up with a reworked patch 2. Mike
On Thu, 2011-12-22 at 15:28 -0800, Mike Dunn wrote:
> OK, thank you Artem. I'll watch for it and follow up with a reworked patch 2.
I'm on it, but I think I've under-estimated the amount of work
and it won't be ready today.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index b01993e..4ac1962 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -477,10 +477,21 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char **types, if (err > 0) { err = add_mtd_partitions(mtd, real_parts, err); kfree(real_parts); + } else if (err == 0) { - err = add_mtd_device(mtd); - if (err == 1) - err = -ENODEV; + /* + * For unpartitioned devices, create a single "partition" that + * spans the entire device, so that driver methods go through + * partition wrappers in all cases. + */ + struct mtd_partition single_part = { + .name = (char *)mtd->name, + .offset = 0, + .size = mtd->size, + .mask_flags = 0, + .ecclayout = mtd->ecclayout, + }; + err = add_mtd_partitions(mtd, &single_part, 1); } return err; diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index a0bd2de..81975a5 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -665,9 +665,11 @@ int add_mtd_partitions(struct mtd_info *master, { struct mtd_part *slave; uint64_t cur_offset = 0; - int i; + int i, ret; - printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name); + if (nbparts > 1) + printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", + nbparts, master->name); for (i = 0; i < nbparts; i++) { slave = allocate_partition(master, parts + i, i, cur_offset); @@ -678,7 +680,9 @@ int add_mtd_partitions(struct mtd_info *master, list_add(&slave->list, &mtd_partitions); mutex_unlock(&mtd_partitions_mutex); - add_mtd_device(&slave->mtd); + ret = add_mtd_device(&slave->mtd); + if (ret == 1) + return -ENODEV; cur_offset = slave->offset + slave->mtd.size; }
Ensure driver methods always go through the wrapper functions in the partitioning code by creating a single "partition" on otherwise unpartitioned devices. Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently out-of-tree). Signed-off-by: Mike Dunn <mikedunn@newsguy.com> --- drivers/mtd/mtdcore.c | 17 ++++++++++++++--- drivers/mtd/mtdpart.c | 10 +++++++--- 2 files changed, 21 insertions(+), 6 deletions(-)