Re: [PATCH 1/2] mtd: Make sure MTD objects always have a valid debugfs dir

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Wed, 4 Oct 2017 15:53:42 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Wed,  4 Oct 2017 15:52:38 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > Master MTD devices are not registered to MTD subsystem if they are
> > exposing MTD partitions unless the CONFIG_MTD_PARTITIONED_MASTER option
> > is enabled.
> > 
> > This lead to a weird situation where some MTD device drivers are trying
> > to add debugfs files to the master MTD device, but this device has no
> > valid debugfs directory.
> > 
> > Rework the core logic to do most of the MTD registration steps
> > (including debugfs dir creation) except the registration to the device
> > model, so that master devices are never exposed to the outside world
> > but are still able to expose debugfs entries.
> > 
> > These devices will be exposed as mtd-hiddenX in the mtd debugfs dir.
> > Note that X is unique within the hidden MTD device pool but can collide
> > with ids of exposed MTD devs.
> > 
> > This commit fixes a bug introduced by commit e8e3edb95ce6 ("mtd: create
> > per-device and module-scope debugfs entries") which is preventing
> > nandsim from loading when CONFIG_DEBUG_FS is enabled.
> > 
> > Fixes: e8e3edb95ce6 ("mtd: create per-device and module-scope debugfs entries")  
> 
> Oops, I forgot
> 
> Reported-by: Richard Weinberger <richard@nod.at>

and

Cc: <stable@vger.kernel.org>

:-/

> 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/mtdcore.c   | 83 ++++++++++++++++++++++++++++++++-----------------
> >  include/linux/mtd/mtd.h |  1 +
> >  2 files changed, 56 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index e7ea842ba3db..323e621f2eba 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -79,6 +79,10 @@ static struct class mtd_class = {
> >  	.pm = MTD_CLS_PM_OPS,
> >  };
> >  
> > +#define MTD_MAX_IDS		0x10000
> > +#define MTD_ID_START		0x0
> > +#define MTD_HIDDEN_ID_START	(MTD_ID_START + MTD_MAX_IDS)
> > +
> >  static DEFINE_IDR(mtd_idr);
> >  
> >  /* These are exported solely for the purpose of mtd_blkdevs.c. You
> > @@ -88,7 +92,13 @@ EXPORT_SYMBOL_GPL(mtd_table_mutex);
> >  
> >  struct mtd_info *__mtd_next_device(int i)
> >  {
> > -	return idr_get_next(&mtd_idr, &i);
> > +	struct mtd_info *mtd;
> > +
> > +	mtd = idr_get_next(&mtd_idr, &i);
> > +	if (!mtd || i >= MTD_MAX_IDS)
> > +		return NULL;
> > +
> > +	return mtd;
> >  }
> >  EXPORT_SYMBOL_GPL(__mtd_next_device);
> >  
> > @@ -505,7 +515,13 @@ int add_mtd_device(struct mtd_info *mtd)
> >  	BUG_ON(mtd->writesize == 0);
> >  	mutex_lock(&mtd_table_mutex);
> >  
> > -	i = idr_alloc(&mtd_idr, mtd, 0, 0, GFP_KERNEL);
> > +	if (!mtd->hidden)
> > +		i = idr_alloc(&mtd_idr, mtd, MTD_ID_START, MTD_MAX_IDS,
> > +			      GFP_KERNEL);
> > +	else
> > +		i = idr_alloc(&mtd_idr, mtd, MTD_HIDDEN_ID_START,
> > +			      MTD_HIDDEN_ID_START + MTD_MAX_IDS,
> > +			      GFP_KERNEL);
> >  	if (i < 0) {
> >  		error = i;
> >  		goto fail_locked;
> > @@ -545,15 +561,20 @@ int add_mtd_device(struct mtd_info *mtd)
> >  	/* Caller should have set dev.parent to match the
> >  	 * physical device, if appropriate.
> >  	 */
> > -	mtd->dev.type = &mtd_devtype;
> > -	mtd->dev.class = &mtd_class;
> > -	mtd->dev.devt = MTD_DEVT(i);
> > -	dev_set_name(&mtd->dev, "mtd%d", i);
> >  	dev_set_drvdata(&mtd->dev, mtd);
> >  	of_node_get(mtd_get_of_node(mtd));
> > -	error = device_register(&mtd->dev);
> > -	if (error)
> > -		goto fail_added;
> > +	if (!mtd->hidden) {
> > +		mtd->dev.type = &mtd_devtype;
> > +		mtd->dev.class = &mtd_class;
> > +		mtd->dev.devt = MTD_DEVT(i);
> > +		dev_set_name(&mtd->dev, "mtd%d", i);
> > +		error = device_register(&mtd->dev);
> > +		if (error)
> > +			goto fail_added;
> > +	} else {
> > +		dev_set_name(&mtd->dev, "mtdmaster%d",
> > +			     i - MTD_HIDDEN_ID_START);
> > +	}
> >  
> >  	if (!IS_ERR_OR_NULL(dfs_dir_mtd)) {
> >  		mtd->dbg.dfs_dir = debugfs_create_dir(dev_name(&mtd->dev), dfs_dir_mtd);
> > @@ -563,14 +584,16 @@ int add_mtd_device(struct mtd_info *mtd)
> >  		}
> >  	}
> >  
> > -	device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
> > -		      "mtd%dro", i);
> > +	if (!mtd->hidden) {
> > +		device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1,
> > +			      NULL, "mtd%dro", i);
> >  
> > -	pr_debug("mtd: Giving out device %d to %s\n", i, mtd->name);
> > -	/* No need to get a refcount on the module containing
> > -	   the notifier, since we hold the mtd_table_mutex */
> > -	list_for_each_entry(not, &mtd_notifiers, list)
> > -		not->add(mtd);
> > +		pr_debug("mtd: Giving out device %d to %s\n", i, mtd->name);
> > +		/* No need to get a refcount on the module containing
> > +		   the notifier, since we hold the mtd_table_mutex */
> > +		list_for_each_entry(not, &mtd_notifiers, list)
> > +			not->add(mtd);
> > +	}
> >  
> >  	mutex_unlock(&mtd_table_mutex);
> >  	/* We _know_ we aren't being removed, because
> > @@ -612,17 +635,20 @@ int del_mtd_device(struct mtd_info *mtd)
> >  		goto out_error;
> >  	}
> >  
> > -	/* No need to get a refcount on the module containing
> > -		the notifier, since we hold the mtd_table_mutex */
> > -	list_for_each_entry(not, &mtd_notifiers, list)
> > -		not->remove(mtd);
> > +	if (!mtd->hidden) {
> > +		/* No need to get a refcount on the module containing
> > +		   the notifier, since we hold the mtd_table_mutex */
> > +		list_for_each_entry(not, &mtd_notifiers, list)
> > +			not->remove(mtd);
> > +	}
> >  
> >  	if (mtd->usecount) {
> >  		printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
> >  		       mtd->index, mtd->name, mtd->usecount);
> >  		ret = -EBUSY;
> >  	} else {
> > -		device_unregister(&mtd->dev);
> > +		if (!mtd->hidden)
> > +			device_unregister(&mtd->dev);
> >  
> >  		idr_remove(&mtd_idr, mtd->index);
> >  		of_node_put(mtd_get_of_node(mtd));
> > @@ -643,15 +669,16 @@ static int mtd_add_device_partitions(struct mtd_info *mtd,
> >  	int nbparts = parts->nr_parts;
> >  	int ret;
> >  
> > -	if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
> > -		ret = add_mtd_device(mtd);
> > -		if (ret)
> > -			return ret;
> > -	}
> > +	if (nbparts && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> > +		mtd->hidden = true;
> > +
> > +	ret = add_mtd_device(mtd);
> > +	if (ret)
> > +		return ret;
> >  
> >  	if (nbparts > 0) {
> >  		ret = add_mtd_partitions(mtd, real_parts, nbparts);
> > -		if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> > +		if (ret)
> >  			del_mtd_device(mtd);
> >  		return ret;
> >  	}
> > @@ -857,7 +884,7 @@ struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num)
> >  				break;
> >  			}
> >  		}
> > -	} else if (num >= 0) {
> > +	} else if (num >= MTD_ID_START && num < MTD_MAX_IDS) {
> >  		ret = idr_find(&mtd_idr, num);
> >  		if (mtd && mtd != ret)
> >  			ret = NULL;
> > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > index 6cd0f6b7658b..382c996b51df 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -268,6 +268,7 @@ struct mtd_info {
> >  	unsigned int bitflip_threshold;
> >  
> >  	// Kernel-only stuff starts here.
> > +	bool hidden;
> >  	const char *name;
> >  	int index;
> >    
>