diff mbox series

[v3,1/2] mtd: partitions: clarify __mtd_del_partition() name

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

Commit Message

Miquel Raynal Jan. 17, 2019, 3:29 p.m. UTC
__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(-)

Comments

Miquel Raynal Jan. 17, 2019, 5:59 p.m. UTC | #1
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.
Boris Brezillon Jan. 21, 2019, 10:03 a.m. UTC | #2
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);
Miquel Raynal Jan. 22, 2019, 11:01 a.m. UTC | #3
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 mbox series

Patch

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);