Message ID | 20190117152929.28256-1-miquel.raynal@bootlin.com |
---|---|
State | Changes Requested |
Delegated to: | Boris Brezillon |
Headers | show |
Series | [v3,1/2] mtd: partitions: clarify __mtd_del_partition() name | expand |
Hello, Miquel Raynal <miquel.raynal@bootlin.com> wrote on Thu, 17 Jan 2019 16:29:28 +0100: > __mtd_del_partition() is a helper that must be called after the > partition lock has ben taken. As there are already a few similar names > (mtd_del_partitions(), del_mtd_partitions(), __mtd_del_partitions()), > make this clear by renaming the helper mtd_del_partition_locked(). > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- And here is the changelog: Changes since v2: ================= * Added a patch to rename __del_mtd_partition() into del_mtd_partition_locked(). * Fixed bugs in the implementation. * Added a union into the mtd_info structure instead of several new entries, either we are on the master ("root") MTD device and we use the master's properties, or we are a partition and the partition properties will be used. Changes since v1: ================= * Commit name changed "proper partition handling" -> "rework partition tree". * Update mtd_get_device_size() to support recursive partitioning. * Remove part_*() helpers, update the corresponding mtd_*() helpers to handle the partitions themselves. * Drop the global partitions lock, add a lock per mtd_info but only use the root one to protect against partitions updates.
On Thu, 17 Jan 2019 16:29:28 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > __mtd_del_partition() is a helper that must be called after the > partition lock has ben taken. As there are already a few similar names ^been > (mtd_del_partitions(), del_mtd_partitions(), __mtd_del_partitions()), > make this clear by renaming the helper mtd_del_partition_locked(). That's indeed a good step forward, but we still have all those variants using sightly different names (mtd_<action>() and <action>_mtd()). Would be great if we could make the names more explicit for those functions too (or remove those that are unneeded). > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/mtdpart.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index b6af41b04622..32beb9c2641f 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -627,20 +627,20 @@ int mtd_add_partition(struct mtd_info *parent, const char *name, > EXPORT_SYMBOL_GPL(mtd_add_partition); > > /** > - * __mtd_del_partition - delete MTD partition > + * mtd_del_partition_locked - 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) > +static int mtd_del_partition_locked(struct mtd_part *priv) > { > struct mtd_part *child, *next; > int err; > > list_for_each_entry_safe(child, next, &mtd_partitions, list) { > if (child->parent == &priv->mtd) { > - err = __mtd_del_partition(child); > + err = mtd_del_partition_locked(child); > if (err) > return err; > } > @@ -670,7 +670,7 @@ int del_mtd_partitions(struct mtd_info *mtd) > mutex_lock(&mtd_partitions_mutex); > list_for_each_entry_safe(slave, next, &mtd_partitions, list) > if (slave->parent == mtd) { > - ret = __mtd_del_partition(slave); > + ret = mtd_del_partition_locked(slave); > if (ret < 0) > err = ret; > } > @@ -688,7 +688,7 @@ int mtd_del_partition(struct mtd_info *mtd, int partno) > list_for_each_entry_safe(slave, next, &mtd_partitions, list) > if ((slave->parent == mtd) && > (slave->mtd.index == partno)) { > - ret = __mtd_del_partition(slave); > + ret = mtd_del_partition_locked(slave); > break; > } > mutex_unlock(&mtd_partitions_mutex);
Hi Boris, Boris Brezillon <bbrezillon@kernel.org> wrote on Mon, 21 Jan 2019 11:03:24 +0100: > On Thu, 17 Jan 2019 16:29:28 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > __mtd_del_partition() is a helper that must be called after the > > partition lock has ben taken. As there are already a few similar names > > ^been Will correct it. > > > (mtd_del_partitions(), del_mtd_partitions(), __mtd_del_partitions()), > > make this clear by renaming the helper mtd_del_partition_locked(). > > That's indeed a good step forward, but we still have all those variants > using sightly different names (mtd_<action>() and <action>_mtd()). > Would be great if we could make the names more explicit for those > functions too (or remove those that are unneeded). I truly agree. I actually tried but this is much more time consuming than expected so I will let this change aside for now but I would really like these helpers to be merged. Thanks, Miquèl
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index b6af41b04622..32beb9c2641f 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -627,20 +627,20 @@ int mtd_add_partition(struct mtd_info *parent, const char *name, EXPORT_SYMBOL_GPL(mtd_add_partition); /** - * __mtd_del_partition - delete MTD partition + * mtd_del_partition_locked - 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) +static int mtd_del_partition_locked(struct mtd_part *priv) { struct mtd_part *child, *next; int err; list_for_each_entry_safe(child, next, &mtd_partitions, list) { if (child->parent == &priv->mtd) { - err = __mtd_del_partition(child); + err = mtd_del_partition_locked(child); if (err) return err; } @@ -670,7 +670,7 @@ int del_mtd_partitions(struct mtd_info *mtd) mutex_lock(&mtd_partitions_mutex); list_for_each_entry_safe(slave, next, &mtd_partitions, list) if (slave->parent == mtd) { - ret = __mtd_del_partition(slave); + ret = mtd_del_partition_locked(slave); if (ret < 0) err = ret; } @@ -688,7 +688,7 @@ int mtd_del_partition(struct mtd_info *mtd, int partno) list_for_each_entry_safe(slave, next, &mtd_partitions, list) if ((slave->parent == mtd) && (slave->mtd.index == partno)) { - ret = __mtd_del_partition(slave); + ret = mtd_del_partition_locked(slave); break; } mutex_unlock(&mtd_partitions_mutex);
__mtd_del_partition() is a helper that must be called after the partition lock has ben taken. As there are already a few similar names (mtd_del_partitions(), del_mtd_partitions(), __mtd_del_partitions()), make this clear by renaming the helper mtd_del_partition_locked(). Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/mtdpart.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)