| Message ID | 20170526131415.27186-2-zajec5@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Brian Norris |
| Headers | show |
On Fri, May 26, 2017 at 03:14:11PM +0200, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > There are two similar functions handling deletion. One handles single > partition and another whole MTD flash device. They share (duplicate) > some code so it makes sense to add a small helper for that part. > > Function del_mtd_partitions has been moved a bit to keep all deleting > stuff together. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > +/** > + * __mtd_del_partition - delete MTD partition > + * > + * @priv: internal MTD struct for partition to be deleted > + * > + * This function must be called with the partitions mutex locked. > + */ > +static int __mtd_del_partition(struct mtd_part *priv) > +{ > + int err; > + > + sysfs_remove_files(&priv->mtd.dev.kobj, mtd_partition_attrs); Notably, the above wasn't called in del_mtd_partitions() previously. Is that intentional? As I read the code, we were actually missing this before; either in error handling, or on device removal. But in either case, the entire device was going to disappear, so the sysfs files would have been cleaned up anyway? Also, is there any chance of double-calling this? I think not, but even if there is, it looks like the low-level routines are reslient to non-existent files. I'm mostly thinking out loud here. I think this is a positive change, even if it wasn't noted explicitly. > + > + err = del_mtd_device(&priv->mtd); > + if (err) > + return err; > + > + list_del(&priv->list); > + free_partition(priv); > + > + return 0; > +} Brian
On 21 June 2017 at 00:50, Brian Norris <computersforpeace@gmail.com> wrote: > On Fri, May 26, 2017 at 03:14:11PM +0200, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> There are two similar functions handling deletion. One handles single >> partition and another whole MTD flash device. They share (duplicate) >> some code so it makes sense to add a small helper for that part. >> >> Function del_mtd_partitions has been moved a bit to keep all deleting >> stuff together. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >> +/** >> + * __mtd_del_partition - delete MTD partition >> + * >> + * @priv: internal MTD struct for partition to be deleted >> + * >> + * This function must be called with the partitions mutex locked. >> + */ >> +static int __mtd_del_partition(struct mtd_part *priv) >> +{ >> + int err; >> + >> + sysfs_remove_files(&priv->mtd.dev.kobj, mtd_partition_attrs); > > Notably, the above wasn't called in del_mtd_partitions() previously. Is > that intentional? > > As I read the code, we were actually missing this before; either in > error handling, or on device removal. But in either case, the entire > device was going to disappear, so the sysfs files would have been > cleaned up anyway? > > Also, is there any chance of double-calling this? I think not, but even > if there is, it looks like the low-level routines are reslient to > non-existent files. > > I'm mostly thinking out loud here. I think this is a positive change, > even if it wasn't noted explicitly. I think that call was missing, however I could make it more clear I'm adding it. I'll try splitting this change out V7 and add a proper description.
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 6960e66eb7a6..92a679062903 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -363,32 +363,6 @@ static inline void free_partition(struct mtd_part *p) kfree(p); } -/* - * This function unregisters and destroy all slave MTD objects which are - * attached to the given master MTD object. - */ - -int del_mtd_partitions(struct mtd_info *master) -{ - struct mtd_part *slave, *next; - int ret, err = 0; - - mutex_lock(&mtd_partitions_mutex); - list_for_each_entry_safe(slave, next, &mtd_partitions, list) - if (slave->master == master) { - ret = del_mtd_device(&slave->mtd); - if (ret < 0) { - err = ret; - continue; - } - list_del(&slave->list); - free_partition(slave); - } - mutex_unlock(&mtd_partitions_mutex); - - return err; -} - static struct mtd_part *allocate_partition(struct mtd_info *master, const struct mtd_partition *part, int partno, uint64_t cur_offset) @@ -667,6 +641,50 @@ int mtd_add_partition(struct mtd_info *master, const char *name, } EXPORT_SYMBOL_GPL(mtd_add_partition); +/** + * __mtd_del_partition - delete MTD partition + * + * @priv: internal MTD struct for partition to be deleted + * + * This function must be called with the partitions mutex locked. + */ +static int __mtd_del_partition(struct mtd_part *priv) +{ + int err; + + sysfs_remove_files(&priv->mtd.dev.kobj, mtd_partition_attrs); + + err = del_mtd_device(&priv->mtd); + if (err) + return err; + + list_del(&priv->list); + free_partition(priv); + + return 0; +} + +/* + * This function unregisters and destroy all slave MTD objects which are + * attached to the given master MTD object. + */ +int del_mtd_partitions(struct mtd_info *master) +{ + struct mtd_part *slave, *next; + int ret, err = 0; + + mutex_lock(&mtd_partitions_mutex); + list_for_each_entry_safe(slave, next, &mtd_partitions, list) + if (slave->master == master) { + ret = __mtd_del_partition(slave); + if (ret < 0) + err = ret; + } + mutex_unlock(&mtd_partitions_mutex); + + return err; +} + int mtd_del_partition(struct mtd_info *master, int partno) { struct mtd_part *slave, *next; @@ -676,14 +694,7 @@ int mtd_del_partition(struct mtd_info *master, int partno) list_for_each_entry_safe(slave, next, &mtd_partitions, list) if ((slave->master == master) && (slave->mtd.index == partno)) { - sysfs_remove_files(&slave->mtd.dev.kobj, - mtd_partition_attrs); - ret = del_mtd_device(&slave->mtd); - if (ret < 0) - break; - - list_del(&slave->list); - free_partition(slave); + ret = __mtd_del_partition(slave); break; } mutex_unlock(&mtd_partitions_mutex);