Re: [PATCH v3 2/2] mtd: rework partitions handling

Re: [PATCH v3 2/2] mtd: rework partitions handling

[Boris Brezillon]

On Thu, 17 Jan 2019 16:29:29 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> @@ -600,6 +601,7 @@ static int mtdchar_blkpg_ioctl(struct mtd_info *mtd,
>  static int mtdchar_write_ioctl(struct mtd_info *mtd,
>  		struct mtd_write_req __user *argp)
>  {
> +	struct mtd_info *master = mtd_get_master(mtd);
>  	struct mtd_write_req req;
>  	struct mtd_oob_ops ops;
>  	const void __user *usr_data, *usr_oob;
> @@ -611,9 +613,8 @@ static int mtdchar_write_ioctl(struct mtd_info *mtd,
>  	usr_data = (const void __user *)(uintptr_t)req.usr_data;
>  	usr_oob = (const void __user *)(uintptr_t)req.usr_oob;
>  
> -	if (!mtd->_write_oob)
> +	if (!master->_write_oob)
>  		return -EOPNOTSUPP;
> -

Keep this blank line.

>  	ops.mode = req.mode;
>  	ops.len = (size_t)req.len;
>  	ops.ooblen = (size_t)req.ooblen;

> @@ -936,20 +942,26 @@ EXPORT_SYMBOL_GPL(get_mtd_device);
>  
>  int __get_mtd_device(struct mtd_info *mtd)
>  {
> +	struct mtd_info *master = mtd_get_master(mtd);
>  	int err;
>  
> -	if (!try_module_get(mtd->owner))
> +	if (!try_module_get(master->owner))
>  		return -ENODEV;
>  
> -	if (mtd->_get_device) {
> -		err = mtd->_get_device(mtd);
> +	if (master->_get_device) {
> +		err = master->_get_device(mtd);

					 ^ master

I think I mentioned that one in my previous review ;-).

>  
>  		if (err) {
> -			module_put(mtd->owner);
> +			module_put(master->owner);
>  			return err;
>  		}
>  	}
> -	mtd->usecount++;
> +
> +	while (mtd->parent) {
> +		mtd->usecount++;
> +		mtd = mtd->parent;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(__get_mtd_device);

...

>  int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  {
> -	if (!mtd->_block_markbad)
> +	struct mtd_info *master = mtd_get_master(mtd);
> +	int ret;
> +
> +	if (!master->_block_markbad)
>  		return -EOPNOTSUPP;
>  	if (ofs < 0 || ofs >= mtd->size)
>  		return -EINVAL;
>  	if (!(mtd->flags & MTD_WRITEABLE))
>  		return -EROFS;
> -	return mtd->_block_markbad(mtd, ofs);
> +
> +	ret = master->_block_markbad(master, mtd_get_master_offset(mtd, ofs));
> +	if (ret)
> +		return ret;
> +
> +	while (mtd->parent) {
> +		mtd->ecc_stats.badblocks++;
> +		mtd = mtd->parent;
> +	}

Not exactly related to this patch, but if we mark a block bad on the
master device this information is not propagated to parts exposing this
block.

> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(mtd_block_markbad);

...

>  
>  /*
>   * This function unregisters and destroy all slave MTD objects which are
> - * attached to the given MTD object.
> + * attached to the given MTD object, recursively.
>   */
> +int del_mtd_partitions_locked(struct mtd_info *mtd)

Should be static as the only users are in mtdpart.c.

> +{
> +	struct mtd_info *child, *next;
> +	int ret, err = 0;
> +
> +	list_for_each_entry_safe(child, next, &mtd->partitions,
> +				 props.part.node) {
> +		if (mtd_has_partitions(child))
> +			del_mtd_partitions_locked(child);
> +
> +		pr_info("Deleting %s MTD partition\n", child->name);
> +		ret = del_mtd_device(child);
> +		if (ret < 0) {
> +			pr_err("Error when deleting partition \"%s\" (%d)\n",
> +			       child->name, ret);
> +			err = ret;
> +			continue;
> +		}
> +
> +		list_del(&child->props.part.node);
> +		free_partition(child);
> +	}
> +
> +	return err;
> +}

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 2/2] mtd: rework partitions handling

[Miquel Raynal]

Hi Boris,

> >  int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs)
> >  {
> > -	if (!mtd->_block_markbad)
> > +	struct mtd_info *master = mtd_get_master(mtd);
> > +	int ret;
> > +
> > +	if (!master->_block_markbad)
> >  		return -EOPNOTSUPP;
> >  	if (ofs < 0 || ofs >= mtd->size)
> >  		return -EINVAL;
> >  	if (!(mtd->flags & MTD_WRITEABLE))
> >  		return -EROFS;
> > -	return mtd->_block_markbad(mtd, ofs);
> > +
> > +	ret = master->_block_markbad(master, mtd_get_master_offset(mtd, ofs));
> > +	if (ret)
> > +		return ret;
> > +
> > +	while (mtd->parent) {
> > +		mtd->ecc_stats.badblocks++;
> > +		mtd = mtd->parent;
> > +	}  
> 
> Not exactly related to this patch, but if we mark a block bad on the
> master device this information is not propagated to parts exposing this
> block.

No but is this important? It is very quick to get up to the master
device so the penalty for doing that is almost non-existent.

Other comments addressed, thanks for the review.
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Boris Brezillon]

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Miquel Raynal]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/