[PATCH] mtd: part: Create the master device node when partitioned

[PATCH] mtd: part: Create the master device node when partitioned

[Dan Ehrenberg]

For many use cases, it helps to have a device node for the entire
MTD device as well as device nodes for the individual partitions.
For example, this allows querying the entire device's properties.
A common idiom is to create an additional partition which spans
over the whole device.

This patch makes a config option, CONFIG_MTD_PARTITIONED_MASTER,
which makes the master partition present even when the device is
partitioned. This isn't turned on by default since it presents
a backwards-incompatible device numbering.

Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
---
 drivers/mtd/Kconfig   |  9 +++++++++
 drivers/mtd/mtdcore.c | 11 ++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 71fea89..92d4bc7 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -309,6 +309,15 @@ config MTD_SWAP
 	  The driver provides wear leveling by storing erase counter into the
 	  OOB.
 
+config MTD_PARTITIONED_MASTER
+	bool "Retain master device when partitioned"
+	default n
+	depends on MTD
+	help
+	  Ordinarily, either a master is present or several partitions
+	  are present. This config option leaves the master in even if
+	  the device is partitioned.
+
 source "drivers/mtd/chips/Kconfig"
 
 source "drivers/mtd/maps/Kconfig"
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 11883bd..c7c1245 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -548,7 +548,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 	}
 
 	if (err > 0) {
-		err = add_mtd_partitions(mtd, real_parts, err);
+		int nbparts = err;
+#ifdef CONFIG_MTD_PARTITIONED_MASTER
+		err = add_mtd_device(mtd);
+		if (err == 1) {
+			err = -ENODEV;
+			goto out;
+		}
+#endif
+		err = add_mtd_partitions(mtd, real_parts, nbparts);
 		kfree(real_parts);
 	} else if (err == 0) {
 		err = add_mtd_device(mtd);
@@ -569,6 +577,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		register_reboot_notifier(&mtd->reboot_notifier);
 	}
 
+out:
 	return err;
 }
 EXPORT_SYMBOL_GPL(mtd_device_parse_register);
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH] mtd: part: Create the master device node when partitioned

[Brian Norris]

On Fri, Mar 06, 2015 at 02:22:02PM -0800, Dan Ehrenberg wrote:
> For many use cases, it helps to have a device node for the entire
> MTD device as well as device nodes for the individual partitions.
> For example, this allows querying the entire device's properties.
> A common idiom is to create an additional partition which spans
> over the whole device.
> 
> This patch makes a config option, CONFIG_MTD_PARTITIONED_MASTER,
> which makes the master partition present even when the device is
> partitioned. This isn't turned on by default since it presents
> a backwards-incompatible device numbering.

I kinda like this strategy. If users were flexible enough, I'd like to
be able to make this the default eventually. But that probably won't
happen, so we may have to live with this Kconfig forever, in that case.

BTW, is there any way to tell the difference between a partition and a
master device, from user space?

Also, would we want to address these comments from mtdpart.c?

 * We don't register the master, or expect the caller to have done so,
 * for reasons of data integrity.


        /* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
         * to have the same data be in two different partitions.
         */

First of all, as I mentioned previously, I don't think the argument is
valid. We can't (and shouldn't have to) protect users from themselves
here.

Secondly, I think maybe this should be changed if we register both
master and partition(s). The partition *should* use the master as its
parent if we register both devices.

Side ntoe: that might provide the means for differentiating master and
partition -- check the device parent in sysfs.

Brian

> Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
> ---
>  drivers/mtd/Kconfig   |  9 +++++++++
>  drivers/mtd/mtdcore.c | 11 ++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 71fea89..92d4bc7 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -309,6 +309,15 @@ config MTD_SWAP
>  	  The driver provides wear leveling by storing erase counter into the
>  	  OOB.
>  
> +config MTD_PARTITIONED_MASTER
> +	bool "Retain master device when partitioned"
> +	default n
> +	depends on MTD
> +	help
> +	  Ordinarily, either a master is present or several partitions
> +	  are present. This config option leaves the master in even if
> +	  the device is partitioned.
> +
>  source "drivers/mtd/chips/Kconfig"
>  
>  source "drivers/mtd/maps/Kconfig"
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 11883bd..c7c1245 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -548,7 +548,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  	}
>  
>  	if (err > 0) {
> -		err = add_mtd_partitions(mtd, real_parts, err);
> +		int nbparts = err;
> +#ifdef CONFIG_MTD_PARTITIONED_MASTER
> +		err = add_mtd_device(mtd);
> +		if (err == 1) {
> +			err = -ENODEV;
> +			goto out;
> +		}
> +#endif
> +		err = add_mtd_partitions(mtd, real_parts, nbparts);
>  		kfree(real_parts);
>  	} else if (err == 0) {
>  		err = add_mtd_device(mtd);
> @@ -569,6 +577,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		register_reboot_notifier(&mtd->reboot_notifier);
>  	}
>  
> +out:
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(mtd_device_parse_register);
> -- 
> 2.2.0.rc0.207.ga3a616c
> 

Re: [PATCH] mtd: part: Create the master device node when partitioned

[Daniel Ehrenberg]

On Mon, Mar 9, 2015 at 6:53 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> I kinda like this strategy. If users were flexible enough, I'd like to
> be able to make this the default eventually. But that probably won't
> happen, so we may have to live with this Kconfig forever, in that case.

:( It'd also be nice if we could name things like mtda, mtda0, mtda1,
etc like SCSI. Alas!
>
> BTW, is there any way to tell the difference between a partition and a
> master device, from user space?

Not that I know of.
>
> Also, would we want to address these comments from mtdpart.c?
>
>  * We don't register the master, or expect the caller to have done so,
>  * for reasons of data integrity.
>
>
>         /* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
>          * to have the same data be in two different partitions.
>          */
>
> First of all, as I mentioned previously, I don't think the argument is
> valid. We can't (and shouldn't have to) protect users from themselves
> here.

Agreed. Anything I should do to address this comment besides deleting it?
>
> Secondly, I think maybe this should be changed if we register both
> master and partition(s). The partition *should* use the master as its
> parent if we register both devices.
>
While I'd like that change, it looks to me like it'd have to be
conditional on the new sysfs variable. The master's sysfs entries
won't be populated otherwise. Or should we build the sysfs node in
either case? What if people are depending on the current sysfs
semantics (not too hard to imagine actually)?

> Side ntoe: that might provide the means for differentiating master and
> partition -- check the device parent in sysfs.
>
> Brian

Yeah, that'd be easy. It'd also be easy to add sysfs properties
describing the partition more.

Thanks,
Dan

Re: [PATCH] mtd: part: Create the master device node when partitioned

[Brian Norris]

On Tue, Mar 10, 2015 at 01:00:35PM -0700, Daniel Ehrenberg wrote:
> On Mon, Mar 9, 2015 at 6:53 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > I kinda like this strategy. If users were flexible enough, I'd like to
> > be able to make this the default eventually. But that probably won't
> > happen, so we may have to live with this Kconfig forever, in that case.
> 
> :( It'd also be nice if we could name things like mtda, mtda0, mtda1,
> etc like SCSI. Alas!

Yeah...

> >
> > BTW, is there any way to tell the difference between a partition and a
> > master device, from user space?
> 
> Not that I know of.
> >
> > Also, would we want to address these comments from mtdpart.c?
> >
> >  * We don't register the master, or expect the caller to have done so,
> >  * for reasons of data integrity.
> >
> >
> >         /* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
> >          * to have the same data be in two different partitions.
> >          */
> >
> > First of all, as I mentioned previously, I don't think the argument is
> > valid. We can't (and shouldn't have to) protect users from themselves
> > here.
> 
> Agreed. Anything I should do to address this comment besides deleting it?

This depends on the answer to the next part.

> >
> > Secondly, I think maybe this should be changed if we register both
> > master and partition(s). The partition *should* use the master as its
> > parent if we register both devices.
> >
> While I'd like that change, it looks to me like it'd have to be
> conditional on the new sysfs variable. The master's sysfs entries
> won't be populated otherwise.

Right.

> Or should we build the sysfs node in
> either case?

No, I don't think we want to change the current behavior.

> What if people are depending on the current sysfs
> semantics (not too hard to imagine actually)?

If we're making these kinds of changes, they should only be for when you
have the new Kconfig entry enabled. I think it's fair to induce a few
sensible, related ABI changes when the kernel is configured to use a new
option. So:
1. register both master and partition(s)
2. use master as the parent of the partition (i.e., tree structure)

We should briefly describe those changes in the Kconfig, and adjust the
code comments to something like, "Historically, we did not register both
the master and partition(s) as devices--and therefore didn't arrange
them as a tree structure--because <fill in bogus reasons>."

> > Side ntoe: that might provide the means for differentiating master and
> > partition -- check the device parent in sysfs.
> >
> > Brian
> 
> Yeah, that'd be easy. It'd also be easy to add sysfs properties
> describing the partition more.

Sure. Off the top of my head: I don't think we have a way to read what
the offset of a partition is within the master. This info is only
printed to the kernel log. (I'm not specifically requesting such
information, but I can imagine it being useful.)

Brian

Re: [PATCH] mtd: part: Create the master device node when partitioned

[Brian Norris]

On Tue, Mar 10, 2015 at 01:23:17PM -0700, Brian Norris wrote:
> On Tue, Mar 10, 2015 at 01:00:35PM -0700, Daniel Ehrenberg wrote:
> > On Mon, Mar 9, 2015 at 6:53 PM, Brian Norris
> > <computersforpeace@gmail.com> wrote:
> > > I kinda like this strategy. If users were flexible enough, I'd like to
> > > be able to make this the default eventually. But that probably won't
> > > happen, so we may have to live with this Kconfig forever, in that case.
> > 
> > :( It'd also be nice if we could name things like mtda, mtda0, mtda1,
> > etc like SCSI. Alas!
> 
> Yeah...

BTW, mtda{,0,1,...} is probably a little ambitious, but we might be able
to get by with phasing in your new Kconfig option as default after a few
releases. Then we could probably gauge whether we can kill the old
behavior completely.

Brian