Patchwork [01/44] mtd: add new API for handling MTD registration

login
register
mail settings
Submitter Dmitry Eremin-Solenikov
Date June 7, 2011, 3:48 p.m.
Message ID <1307461738-16811-1-git-send-email-dbaryshkov@gmail.com>
Download mbox | patch
Permalink /patch/99276/
State New
Headers show

Comments

Dmitry Eremin-Solenikov - June 7, 2011, 3:48 p.m.
Lots (nearly all) mtd drivers contain nearly the similar code that
calls parse_mtd_partitions, provides some platform-default values, if
parsing fails, and registers  mtd device.

This is an aim to provide single implementation of this scenario:
mtd_device_parse_register() which will handle all this parsing and
defaults.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 drivers/mtd/mtdcore.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h |    5 +++++
 2 files changed, 52 insertions(+), 0 deletions(-)
Artem Bityutskiy - June 8, 2011, 8:55 a.m.
I agree we need a good comment, but I find this one difficult to follow.

On Tue, 2011-06-07 at 19:48 +0400, Dmitry Eremin-Solenikov wrote:
>  /**
> + * mtd_device_parse_register - register an MTD device.

Not just register, it also contains parsing functionality.

> + *
> + * @mtd: the MTD device to register
> + * @part_probe_types: the list of MTD partition probes to try

Please, name it 'types' to be consistent with 'parse_mtd_partitions()'.
Also, you do not say that this can be %NULL. With my patch, you can
refer the reader like "see 'parse_mtd_partitions()' for more
information'.

> + * @origin: start address of MTD device. =0 unless you are sure you need this.

Try to avoid using dots. Also, constants should be prefixed with %,
i.e., use %0.

> + * @defparts: default partition information to register. Only valid if
> + *		defnr_parts > 0

Ditto about a dot, prefix variables with @, i.e., in this case:
@defnr_parts > %0.

Also about naming: why "default"? What is default there? This is more
like "alternative, or fall-back" - you first try to detect partition
specified in @part_probe_types, if if you fail - you use "alternative"
partitions.

Please, rename it.

> + * @defnr_parts: the number of partitions in defparts.  If zero then the full
> + *		MTD device is registered if no partition info is found

Ditto.

> + *
> + * Extract partition info and register MTD device (partitions or a whole device)
> + * It calls parse_mtd_partitions(), checks the result. If there were no
> + * partitions found, it uses default info specified (via defparts/defnr_parts)
> + * and then registers either partitions, or (if no partitions were
> + * found/specified) the whow MTD.
> + */

Sorry, I find this unreadable.

I suggest something like this.

This function aggregates MTD partitions parsing (done by
'parse_mtd_partitions()') and MTD device and partitions registering. It
basically follows the most common pattern found in many MTD drivers:

o It first tries to probe partitions on MTD device @mtd using parsers
  specified in @types (if @types is %NULL, then the default list of
  parsers is used, see 'parse_mtd_partitions()' for more information).
o If this succeeds, this function registers the found partitions, as
  well as the whole MTD device @mtd.
o If no partitions were found this function just registers the MTD
  device @mtd and exits.

Returns zero in case of success and a negative error code in case of
failure.

Would you please amend the patch and re-send?
Jamie Iles - June 8, 2011, 10:19 a.m.
On Tue, Jun 07, 2011 at 07:48:58PM +0400, Dmitry Eremin-Solenikov wrote:
> Lots (nearly all) mtd drivers contain nearly the similar code that
> calls parse_mtd_partitions, provides some platform-default values, if
> parsing fails, and registers  mtd device.
> 
> This is an aim to provide single implementation of this scenario:
> mtd_device_parse_register() which will handle all this parsing and
> defaults.
> 
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
>  drivers/mtd/mtdcore.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/mtd.h |    5 +++++
>  2 files changed, 52 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index c510aff..d538e0a 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -452,6 +452,53 @@ int mtd_device_register(struct mtd_info *master,
>  EXPORT_SYMBOL_GPL(mtd_device_register);
>  
>  /**
> + * mtd_device_parse_register - register an MTD device.
> + *
> + * @mtd: the MTD device to register
> + * @part_probe_types: the list of MTD partition probes to try
> + * @origin: start address of MTD device. =0 unless you are sure you need this.
> + * @defparts: default partition information to register. Only valid if
> + *		defnr_parts > 0
> + * @defnr_parts: the number of partitions in defparts.  If zero then the full
> + *		MTD device is registered if no partition info is found
> + *
> + * Extract partition info and register MTD device (partitions or a whole device)
> + * It calls parse_mtd_partitions(), checks the result. If there were no
> + * partitions found, it uses default info specified (via defparts/defnr_parts)
> + * and then registers either partitions, or (if no partitions were
> + * found/specified) the whow MTD.

s/whow/whole ?

> + */
> +int mtd_device_parse_register(struct mtd_info *mtd,
> +			      const char **part_probe_types,
> +			      unsigned long origin,
> +			      const struct mtd_partition *defparts,
> +			      int defnr_parts)
> +{
> +	int err;
> +	struct mtd_partition *parts;
> +
> +	err = parse_mtd_partitions(mtd, part_probe_types, &parts, origin);
> +	if (err <= 0 && defnr_parts) {
> +		parts = kmemdup(defparts, sizeof(*parts) * defnr_parts,
> +				GFP_KERNEL);
> +		if (!parts)
> +			err = -ENOMEM;
> +	}

I see that some of your patches for drivers (e.g. plat_nand) remove the 
kfree() of the partitions but this doesn't appear to be handled anywhere 
else afaict.  I don't think this can be done in mtd_device_unregister() 
as it doesn't have the mtd_partition array so probably needs to remain 
in the driver.

Jamie
Dmitry Eremin-Solenikov - June 8, 2011, 12:22 p.m.
On 6/8/11, Jamie Iles <jamie@jamieiles.com> wrote:
> On Tue, Jun 07, 2011 at 07:48:58PM +0400, Dmitry Eremin-Solenikov wrote:
>> Lots (nearly all) mtd drivers contain nearly the similar code that
>> calls parse_mtd_partitions, provides some platform-default values, if
>> parsing fails, and registers  mtd device.
>>
>> This is an aim to provide single implementation of this scenario:
>> mtd_device_parse_register() which will handle all this parsing and
>> defaults.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
>>  drivers/mtd/mtdcore.c   |   47
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/mtd.h |    5 +++++
>>  2 files changed, 52 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index c510aff..d538e0a 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -452,6 +452,53 @@ int mtd_device_register(struct mtd_info *master,
>>  EXPORT_SYMBOL_GPL(mtd_device_register);
>>
>>  /**
>> + * mtd_device_parse_register - register an MTD device.
>> + *
>> + * @mtd: the MTD device to register
>> + * @part_probe_types: the list of MTD partition probes to try
>> + * @origin: start address of MTD device. =0 unless you are sure you need
>> this.
>> + * @defparts: default partition information to register. Only valid if
>> + *		defnr_parts > 0
>> + * @defnr_parts: the number of partitions in defparts.  If zero then the
>> full
>> + *		MTD device is registered if no partition info is found
>> + *
>> + * Extract partition info and register MTD device (partitions or a whole
>> device)
>> + * It calls parse_mtd_partitions(), checks the result. If there were no
>> + * partitions found, it uses default info specified (via
>> defparts/defnr_parts)
>> + * and then registers either partitions, or (if no partitions were
>> + * found/specified) the whow MTD.
>
> s/whow/whole ?

Yup.

>> + */
>> +int mtd_device_parse_register(struct mtd_info *mtd,
>> +			      const char **part_probe_types,
>> +			      unsigned long origin,
>> +			      const struct mtd_partition *defparts,
>> +			      int defnr_parts)
>> +{
>> +	int err;
>> +	struct mtd_partition *parts;
>> +
>> +	err = parse_mtd_partitions(mtd, part_probe_types, &parts, origin);
>> +	if (err <= 0 && defnr_parts) {
>> +		parts = kmemdup(defparts, sizeof(*parts) * defnr_parts,
>> +				GFP_KERNEL);
>> +		if (!parts)
>> +			err = -ENOMEM;
>> +	}
>
> I see that some of your patches for drivers (e.g. plat_nand) remove the
> kfree() of the partitions but this doesn't appear to be handled anywhere
> else afaict.  I don't think this can be done in mtd_device_unregister()
> as it doesn't have the mtd_partition array so probably needs to remain
> in the driver.

It's handled by kfree right aftert add_mtd_partitions. No mtd partitions
information is exported out of mtd_device_parse_register(). In fact I'll have
to check if I can remove all <mtd/partitions.h> includes from drivers.

Anyway I'll post a V2 later after fixing issues identified by you and Artem.

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index c510aff..d538e0a 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -452,6 +452,53 @@  int mtd_device_register(struct mtd_info *master,
 EXPORT_SYMBOL_GPL(mtd_device_register);
 
 /**
+ * mtd_device_parse_register - register an MTD device.
+ *
+ * @mtd: the MTD device to register
+ * @part_probe_types: the list of MTD partition probes to try
+ * @origin: start address of MTD device. =0 unless you are sure you need this.
+ * @defparts: default partition information to register. Only valid if
+ *		defnr_parts > 0
+ * @defnr_parts: the number of partitions in defparts.  If zero then the full
+ *		MTD device is registered if no partition info is found
+ *
+ * Extract partition info and register MTD device (partitions or a whole device)
+ * It calls parse_mtd_partitions(), checks the result. If there were no
+ * partitions found, it uses default info specified (via defparts/defnr_parts)
+ * and then registers either partitions, or (if no partitions were
+ * found/specified) the whow MTD.
+ */
+int mtd_device_parse_register(struct mtd_info *mtd,
+			      const char **part_probe_types,
+			      unsigned long origin,
+			      const struct mtd_partition *defparts,
+			      int defnr_parts)
+{
+	int err;
+	struct mtd_partition *parts;
+
+	err = parse_mtd_partitions(mtd, part_probe_types, &parts, origin);
+	if (err <= 0 && defnr_parts) {
+		parts = kmemdup(defparts, sizeof(*parts) * defnr_parts,
+				GFP_KERNEL);
+		if (!parts)
+			err = -ENOMEM;
+	}
+
+	if (err > 0) {
+		err = add_mtd_partitions(mtd, parts, err);
+		kfree(parts);
+	} else if (err == 0) {
+		err = add_mtd_device(mtd);
+		if (err == 1)
+			err = -ENODEV;
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(mtd_device_parse_register);
+
+/**
  * mtd_device_unregister - unregister an existing MTD device.
  *
  * @master: the MTD device to unregister.  This will unregister both the master
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 2541fb8..d28a241 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -327,6 +327,11 @@  struct mtd_partition;
 extern int mtd_device_register(struct mtd_info *master,
 			       const struct mtd_partition *parts,
 			       int nr_parts);
+extern int mtd_device_parse_register(struct mtd_info *mtd,
+			      const char **part_probe_types,
+			      unsigned long origin,
+			      const struct mtd_partition *defparts,
+			      int defnr_parts);
 extern int mtd_device_unregister(struct mtd_info *master);
 extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
 extern int __get_mtd_device(struct mtd_info *mtd);