[PATCH 0/3] mtd: Add support for reading MTD devices via the nvmem API

[PATCH 0/3] mtd: Add support for reading MTD devices via the nvmem API

[Alban]

Hi all,

this is the followup to my "device data" patch[1] that add nvmem support
to the MTD subsystem. I took a look at the work done by Moritz[2] but we
had different goals. Moritz needed access to the MTD protection
registers where I need access to the normal data. For my use-case we can
just use the normal nvmem bindings and define the nvmem cells as
children nodes of the MTD partitions. The only thing I added is the
requirement for a `nvmem-provider` property in the partition to allow
the driver to only register the required partitions.

Note that nvmem cells currently can't be defined on the raw MTD devices
as that would clash with the old partitions binding. I think it would
be better to change the nvmem binding to require putting the cells in a
dedicated subnode, like in the new partitions binding.

Otherwise I just skipped Moritz's use-case as I don't know anything
about the MTD protection registers.

Finally I included a bug fix for the case where several nvmem devices
without name are registered. Currently all devices get registered as
`nvmem0` and the second registration fails.

[1] http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1341549.html
[2] https://patchwork.ozlabs.org/patch/626460/

Alban (3):
  doc: bindings: Add bindings documentation for mtd nvmem
  mtd: Add support for reading MTD devices via the nvmem API
  nvmem: core: Allow allocating several anonymous nvmem devices

 .../devicetree/bindings/nvmem/mtd-nvmem.txt        |  29 +++++
 drivers/mtd/Kconfig                                |   9 ++
 drivers/mtd/Makefile                               |   1 +
 drivers/mtd/mtdnvmem.c                             | 121 +++++++++++++++++++++
 drivers/nvmem/core.c                               |   3 +-
 5 files changed, 162 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
 create mode 100644 drivers/mtd/mtdnvmem.c

-- 
2.7.4

[PATCH 1/3] doc: bindings: Add bindings documentation for mtd nvmem

[Alban]

Add the binding to expose MTD partitions as nvmem providers.

Signed-off-by: Alban <albeu@free.fr>
---
 .../devicetree/bindings/nvmem/mtd-nvmem.txt        | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt

diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
new file mode 100644
index 0000000..47602f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
@@ -0,0 +1,29 @@
+= NVMEM in MTD =
+
+Config data for drivers is often stored in MTD devices. This binding
+define how such data can be represented in device tree.
+
+An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
+property to their node. Data cells can then be defined as child nodes
+of the partition as defined in nvmem.txt.
+
+Example:
+
+	flash@0 {
+		...
+
+		partition@2 {
+			label = "art";
+			reg = <0x7F0000 0x010000>;
+			read-only;
+
+			nvmem-provider;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			eeprom@1000 {
+				label = "wmac-eeprom";
+				reg = <0x1000 0x1000>;
+			};
+		};
+	};
-- 
2.7.4

Re: [PATCH 1/3] doc: bindings: Add bindings documentation for mtd nvmem

[Boris Brezillon]

On Thu,  2 Mar 2017 20:50:21 +0100
Alban <albeu@free.fr> wrote:

> Add the binding to expose MTD partitions as nvmem providers.

Looks good. Maybe you should take the case you describe in your
cover-letter into account and add an extra layer: add an nvmem sub-node
containing the nvmem cells, so that you can expose nvmem cells directly
under master MTD devices (and not only partitions). 

> 
> Signed-off-by: Alban <albeu@free.fr>
> ---
>  .../devicetree/bindings/nvmem/mtd-nvmem.txt        | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> new file mode 100644
> index 0000000..47602f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> @@ -0,0 +1,29 @@
> += NVMEM in MTD =
> +
> +Config data for drivers is often stored in MTD devices. This binding
> +define how such data can be represented in device tree.
> +
> +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> +property to their node. Data cells can then be defined as child nodes
> +of the partition as defined in nvmem.txt.
> +
> +Example:
> +
> +	flash@0 {
> +		...
> +
> +		partition@2 {
> +			label = "art";
> +			reg = <0x7F0000 0x010000>;
> +			read-only;
> +
> +			nvmem-provider;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			eeprom@1000 {
> +				label = "wmac-eeprom";
> +				reg = <0x1000 0x1000>;
> +			};
> +		};
> +	};

Re: [PATCH 1/3] doc: bindings: Add bindings documentation for mtd nvmem

[Alban]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

On Thu, 2 Mar 2017 21:22:20 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Thu,  2 Mar 2017 20:50:21 +0100
> Alban <albeu@free.fr> wrote:
> 
> > Add the binding to expose MTD partitions as nvmem providers.  
> 
> Looks good. Maybe you should take the case you describe in your
> cover-letter into account and add an extra layer: add an nvmem sub-node
> containing the nvmem cells, so that you can expose nvmem cells directly
> under master MTD devices (and not only partitions). 

I think that would be the better solution. This can be done
independently, once we agree on a binding we just have to fix
of_nvmem_cell_get(). My suggestion would be to have the new binding
looking like this:

nvmem-device@10 {
	...
	nvmem-provider;
	nvmem-cells {
		compatible = "nvmem-cells";
		#address-cells = <1>;
		#size-cells = <1>;

		nvmem-cell@100 {
			label = "mac-address";
			reg = <0x100 0x200>;
		}

		...
	}
}

I would also suggest making the "nvmem-provider" property mandatory
to indicate that the device provides this capability. Up to now all
nvmem providers only support this API but I think there might be more
multi function devices in the future.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] doc: bindings: Add bindings documentation for mtd nvmem

[Boris Brezillon]

On Fri, 3 Mar 2017 13:17:05 +0100
Alban <albeu@free.fr> wrote:

> On Thu, 2 Mar 2017 21:22:20 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Thu,  2 Mar 2017 20:50:21 +0100
> > Alban <albeu@free.fr> wrote:
> >   
> > > Add the binding to expose MTD partitions as nvmem providers.    
> > 
> > Looks good. Maybe you should take the case you describe in your
> > cover-letter into account and add an extra layer: add an nvmem sub-node
> > containing the nvmem cells, so that you can expose nvmem cells directly
> > under master MTD devices (and not only partitions).   
> 
> I think that would be the better solution. This can be done
> independently, once we agree on a binding we just have to fix
> of_nvmem_cell_get(). My suggestion would be to have the new binding
> looking like this:
> 
> nvmem-device@10 {
> 	...
> 	nvmem-provider;
> 	nvmem-cells {
> 		compatible = "nvmem-cells";
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		nvmem-cell@100 {
> 			label = "mac-address";
> 			reg = <0x100 0x200>;
> 		}
> 
> 		...
> 	}
> }
> 
> I would also suggest making the "nvmem-provider" property mandatory
> to indicate that the device provides this capability. Up to now all
> nvmem providers only support this API but I think there might be more
> multi function devices in the future.

If you enforce the name of the child node (here nvmem-cells), you don't
need this extra nvmem-provider property. Am I missing something?

Re: [PATCH 1/3] doc: bindings: Add bindings documentation for mtd nvmem

[Alban]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Fri, 3 Mar 2017 13:37:44 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Fri, 3 Mar 2017 13:17:05 +0100
> Alban <albeu@free.fr> wrote:
> 
> > On Thu, 2 Mar 2017 21:22:20 +0100
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >   
> > > On Thu,  2 Mar 2017 20:50:21 +0100
> > > Alban <albeu@free.fr> wrote:
> > >     
> > > > Add the binding to expose MTD partitions as nvmem providers.      
> > > 
> > > Looks good. Maybe you should take the case you describe in your
> > > cover-letter into account and add an extra layer: add an nvmem sub-node
> > > containing the nvmem cells, so that you can expose nvmem cells directly
> > > under master MTD devices (and not only partitions).     
> > 
> > I think that would be the better solution. This can be done
> > independently, once we agree on a binding we just have to fix
> > of_nvmem_cell_get(). My suggestion would be to have the new binding
> > looking like this:
> > 
> > nvmem-device@10 {
> > 	...
> > 	nvmem-provider;
> > 	nvmem-cells {
> > 		compatible = "nvmem-cells";
> > 		#address-cells = <1>;
> > 		#size-cells = <1>;
> > 
> > 		nvmem-cell@100 {
> > 			label = "mac-address";
> > 			reg = <0x100 0x200>;
> > 		}
> > 
> > 		...
> > 	}
> > }
> > 
> > I would also suggest making the "nvmem-provider" property mandatory
> > to indicate that the device provides this capability. Up to now all
> > nvmem providers only support this API but I think there might be more
> > multi function devices in the future.  
> 
> If you enforce the name of the child node (here nvmem-cells), you don't
> need this extra nvmem-provider property. Am I missing something?

That property would define the capability to be used as nvmem-provider,
furthermore it would cover the case where no cell is defined. Also in
the case of MTD devices it would avoid an ambiguity when there is no
'partitions' sub node, as then the nvmem-cells node could be interpreted
as a partition following the old binding.

From what I understand most of such "implicit" binding have sooner or
later proved to be too limited, or worth, clashing with another one.
They then had to be deprecated and replaced by explicit ones. The MTD
partitions binding is a good example of such evolution.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] doc: bindings: Add bindings documentation for mtd nvmem

[Srinivas Kandagatla]

On 02/03/17 19:50, Alban wrote:
> Add the binding to expose MTD partitions as nvmem providers.

It would be nice to see more description of this patch, explaining the 
real use case.
>
> Signed-off-by: Alban <albeu@free.fr>
> ---
>  .../devicetree/bindings/nvmem/mtd-nvmem.txt        | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>
> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> new file mode 100644
> index 0000000..47602f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> @@ -0,0 +1,29 @@
> += NVMEM in MTD =
> +
> +Config data for drivers is often stored in MTD devices. This binding
> +define how such data can be represented in device tree.
> +
> +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
We should prefix this property with "mtd" to make it more explicit that 
this is specific to mtd devices.

May be we should put this under "Required Properties" section, marking 
it as mandatory for mtd nvmem providers.

> +property to their node. Data cells can then be defined as child nodes
> +of the partition as defined in nvmem.txt.
> +
> +Example:
> +
> +	flash@0 {
> +		...
> +
> +		partition@2 {
> +			label = "art";
> +			reg = <0x7F0000 0x010000>;
> +			read-only;
> +
> +			nvmem-provider;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			eeprom@1000 {
> +				label = "wmac-eeprom";
> +				reg = <0x1000 0x1000>;
> +			};
> +		};
> +	};
>

Re: [PATCH 1/3] doc: bindings: Add bindings documentation for mtd nvmem

[Boris Brezillon]

On Fri, 3 Mar 2017 11:27:34 +0000
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 02/03/17 19:50, Alban wrote:
> > Add the binding to expose MTD partitions as nvmem providers.  
> 
> It would be nice to see more description of this patch, explaining the 
> real use case.
> >
> > Signed-off-by: Alban <albeu@free.fr>
> > ---
> >  .../devicetree/bindings/nvmem/mtd-nvmem.txt        | 29 ++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > new file mode 100644
> > index 0000000..47602f7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > @@ -0,0 +1,29 @@
> > += NVMEM in MTD =
> > +
> > +Config data for drivers is often stored in MTD devices. This binding
> > +define how such data can be represented in device tree.
> > +
> > +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`  
> We should prefix this property with "mtd" to make it more explicit that 
> this is specific to mtd devices.

MTD is a linux-ism, not sure DT maintainers will like it ;-).

> 
> May be we should put this under "Required Properties" section, marking 
> it as mandatory for mtd nvmem providers.

It's definitely optional. It's really a choice to provide a nvmem cells
under an MTD partition.

> 
> > +property to their node. Data cells can then be defined as child nodes
> > +of the partition as defined in nvmem.txt.
> > +
> > +Example:
> > +
> > +	flash@0 {
> > +		...
> > +
> > +		partition@2 {
> > +			label = "art";
> > +			reg = <0x7F0000 0x010000>;
> > +			read-only;
> > +
> > +			nvmem-provider;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +
> > +			eeprom@1000 {
> > +				label = "wmac-eeprom";
> > +				reg = <0x1000 0x1000>;
> > +			};
> > +		};
> > +	};
> >  

Re: [PATCH 1/3] doc: bindings: Add bindings documentation for mtd nvmem

[Alban]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

On Fri, 3 Mar 2017 11:27:34 +0000
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 02/03/17 19:50, Alban wrote:
> > Add the binding to expose MTD partitions as nvmem providers.  
> 
> It would be nice to see more description of this patch, explaining the 
> real use case.

I'll try, writing good documentation is not my strong point :/

> >
> > Signed-off-by: Alban <albeu@free.fr>
> > ---
> >  .../devicetree/bindings/nvmem/mtd-nvmem.txt        | 29 ++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > new file mode 100644
> > index 0000000..47602f7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > @@ -0,0 +1,29 @@
> > += NVMEM in MTD =
> > +
> > +Config data for drivers is often stored in MTD devices. This binding
> > +define how such data can be represented in device tree.
> > +
> > +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`  
> We should prefix this property with "mtd" to make it more explicit that 
> this is specific to mtd devices.
> 
> May be we should put this under "Required Properties" section, marking 
> it as mandatory for mtd nvmem providers.

I agree it should be required, but I would not make it MTD specific. In
fact I would suggest making it mandatory for all nvmem providers. That
would be inline with 'interrupt-controller' and all the other properties
used to indicate the capabilities of devices.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Alban]

Allow drivers that use the nvmem API to read data stored on MTD devices.
This add a simple mtd user that register itself as a read-only nvmem
device.

Signed-off-by: Alban <albeu@free.fr>
---
 drivers/mtd/Kconfig    |   9 ++++
 drivers/mtd/Makefile   |   1 +
 drivers/mtd/mtdnvmem.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/mtd/mtdnvmem.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index e83a279..9cad86c 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -322,6 +322,15 @@ config MTD_PARTITIONED_MASTER
 	  the parent of the partition device be the master device, rather than
 	  what lies behind the master.
 
+config MTD_NVMEM
+	tristate "Read config data from MTD devices"
+	default y
+	depends on NVMEM
+	help
+	  Provides support for reading config data from MTD devices. This can
+	  be used by drivers to read device specific data such as MAC addresses
+	  or calibration results.
+
 source "drivers/mtd/chips/Kconfig"
 
 source "drivers/mtd/maps/Kconfig"
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 99bb9a1..f62f50b 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_SSFDC)		+= ssfdc.o
 obj-$(CONFIG_SM_FTL)		+= sm_ftl.o
 obj-$(CONFIG_MTD_OOPS)		+= mtdoops.o
 obj-$(CONFIG_MTD_SWAP)		+= mtdswap.o
+obj-$(CONFIG_MTD_NVMEM)		+= mtdnvmem.o
 
 nftl-objs		:= nftlcore.o nftlmount.o
 inftl-objs		:= inftlcore.o inftlmount.o
diff --git a/drivers/mtd/mtdnvmem.c b/drivers/mtd/mtdnvmem.c
new file mode 100644
index 0000000..6eb4216
--- /dev/null
+++ b/drivers/mtd/mtdnvmem.c
@@ -0,0 +1,121 @@
+/*
+ * Copyright (C) 2017 Alban Bedel <albeu@free.fr>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/nvmem-provider.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+struct mtd_nvmem {
+	struct list_head list;
+	struct mtd_info *mtd;
+	struct nvmem_device *nvmem;
+};
+
+static DEFINE_MUTEX(mtd_nvmem_list_lock);
+static LIST_HEAD(mtd_nvmem_list);
+
+static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
+			      void *val, size_t bytes)
+{
+	struct mtd_info *mtd = priv;
+	size_t retlen;
+	int err;
+
+	err = mtd_read(mtd, offset, bytes, &retlen, val);
+	if (err && err != -EUCLEAN)
+		return err;
+
+	return retlen == bytes ? 0 : -EIO;
+}
+
+static void mtd_nvmem_add(struct mtd_info *mtd)
+{
+	struct device *dev = &mtd->dev;
+	struct device_node *np = dev_of_node(dev);
+	struct nvmem_config config = {};
+	struct mtd_nvmem *mtd_nvmem;
+
+	/* OF devices have to provide the nvmem node */
+	if (np && !of_property_read_bool(np, "nvmem-provider"))
+		return;
+
+	config.dev = dev;
+	config.owner = THIS_MODULE;
+	config.reg_read = mtd_nvmem_reg_read;
+	config.size = mtd->size;
+	config.word_size = 1;
+	config.stride = 1;
+	config.read_only = true;
+	config.priv = mtd;
+
+	/* Alloc our struct to keep track of the MTD NVMEM devices */
+	mtd_nvmem = kzalloc(sizeof(*mtd_nvmem), GFP_KERNEL);
+	if (!mtd_nvmem)
+		return;
+
+	mtd_nvmem->mtd = mtd;
+	mtd_nvmem->nvmem = nvmem_register(&config);
+	if (IS_ERR(mtd_nvmem->nvmem)) {
+		dev_err(dev, "Failed to register NVMEM device\n");
+		kfree(mtd_nvmem);
+		return;
+	}
+
+	mutex_lock(&mtd_nvmem_list_lock);
+	list_add_tail(&mtd_nvmem->list, &mtd_nvmem_list);
+	mutex_unlock(&mtd_nvmem_list_lock);
+}
+
+static void mtd_nvmem_remove(struct mtd_info *mtd)
+{
+	struct mtd_nvmem *mtd_nvmem;
+	bool found = false;
+
+	mutex_lock(&mtd_nvmem_list_lock);
+	list_for_each_entry(mtd_nvmem, &mtd_nvmem_list, list) {
+		if (mtd_nvmem->mtd == mtd) {
+			list_del(&mtd_nvmem->list);
+			found = true;
+			break;
+		}
+	}
+	mutex_unlock(&mtd_nvmem_list_lock);
+
+	if (found) {
+		if (nvmem_unregister(mtd_nvmem->nvmem))
+			dev_err(&mtd->dev,
+				"Failed to unregister NVMEM device\n");
+		kfree(mtd_nvmem);
+	}
+}
+
+static struct mtd_notifier mtd_nvmem_notifier = {
+	.add = mtd_nvmem_add,
+	.remove = mtd_nvmem_remove,
+};
+
+static int __init mtd_nvmem_init(void)
+{
+	register_mtd_user(&mtd_nvmem_notifier);
+	return 0;
+}
+module_init(mtd_nvmem_init);
+
+static void __exit mtd_nvmem_exit(void)
+{
+	unregister_mtd_user(&mtd_nvmem_notifier);
+}
+module_exit(mtd_nvmem_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alban Bedel <albeu@free.fr>");
+MODULE_DESCRIPTION("Driver to read config data from MTD devices");
-- 
2.7.4

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Boris Brezillon]

On Thu,  2 Mar 2017 20:50:22 +0100
Alban <albeu@free.fr> wrote:

> Allow drivers that use the nvmem API to read data stored on MTD devices.
> This add a simple mtd user that register itself as a read-only nvmem
> device.
> 
> Signed-off-by: Alban <albeu@free.fr>

Just a few comments, but it looks pretty good already.

> ---
>  drivers/mtd/Kconfig    |   9 ++++
>  drivers/mtd/Makefile   |   1 +
>  drivers/mtd/mtdnvmem.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)
>  create mode 100644 drivers/mtd/mtdnvmem.c
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index e83a279..9cad86c 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -322,6 +322,15 @@ config MTD_PARTITIONED_MASTER
>  	  the parent of the partition device be the master device, rather than
>  	  what lies behind the master.
>  
> +config MTD_NVMEM
> +	tristate "Read config data from MTD devices"
> +	default y
> +	depends on NVMEM
> +	help
> +	  Provides support for reading config data from MTD devices. This can
> +	  be used by drivers to read device specific data such as MAC addresses
> +	  or calibration results.
> +
>  source "drivers/mtd/chips/Kconfig"
>  
>  source "drivers/mtd/maps/Kconfig"
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..f62f50b 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_SSFDC)		+= ssfdc.o
>  obj-$(CONFIG_SM_FTL)		+= sm_ftl.o
>  obj-$(CONFIG_MTD_OOPS)		+= mtdoops.o
>  obj-$(CONFIG_MTD_SWAP)		+= mtdswap.o
> +obj-$(CONFIG_MTD_NVMEM)		+= mtdnvmem.o
>  
>  nftl-objs		:= nftlcore.o nftlmount.o
>  inftl-objs		:= inftlcore.o inftlmount.o
> diff --git a/drivers/mtd/mtdnvmem.c b/drivers/mtd/mtdnvmem.c
> new file mode 100644
> index 0000000..6eb4216
> --- /dev/null
> +++ b/drivers/mtd/mtdnvmem.c
> @@ -0,0 +1,121 @@
> +/*
> + * Copyright (C) 2017 Alban Bedel <albeu@free.fr>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +
> +struct mtd_nvmem {
> +	struct list_head list;
> +	struct mtd_info *mtd;
> +	struct nvmem_device *nvmem;
> +};
> +
> +static DEFINE_MUTEX(mtd_nvmem_list_lock);
> +static LIST_HEAD(mtd_nvmem_list);

I was wondering if we should have the nvmem pointer directly embedded
in the mtd_info struct. Your approach has the benefit of keeping then
nvmem wrapper completely independent, which is a good thing, but you'll
see below that there's a problem with the MTD notifier approach.

> +
> +static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
> +			      void *val, size_t bytes)
> +{
> +	struct mtd_info *mtd = priv;
> +	size_t retlen;
> +	int err;
> +
> +	err = mtd_read(mtd, offset, bytes, &retlen, val);
> +	if (err && err != -EUCLEAN)
> +		return err;
> +
> +	return retlen == bytes ? 0 : -EIO;
> +}
> +
> +static void mtd_nvmem_add(struct mtd_info *mtd)
> +{
> +	struct device *dev = &mtd->dev;
> +	struct device_node *np = dev_of_node(dev);
> +	struct nvmem_config config = {};
> +	struct mtd_nvmem *mtd_nvmem;
> +
> +	/* OF devices have to provide the nvmem node */
> +	if (np && !of_property_read_bool(np, "nvmem-provider"))
> +		return;

Might have to be adapted according to the DT binding if we decide to
add an extra subnode, but then, I'm not sure the nvmem cells creation
will work correctly, because the framework expect nvmem cells to be
direct children of the nvmem device, which will no longer be the case
if you add an intermediate node between the MTD device node and the
nvmem cell nodes.

> +
> +	config.dev = dev;
> +	config.owner = THIS_MODULE;
> +	config.reg_read = mtd_nvmem_reg_read;
> +	config.size = mtd->size;
> +	config.word_size = 1;
> +	config.stride = 1;
> +	config.read_only = true;
> +	config.priv = mtd;
> +
> +	/* Alloc our struct to keep track of the MTD NVMEM devices */
> +	mtd_nvmem = kzalloc(sizeof(*mtd_nvmem), GFP_KERNEL);
> +	if (!mtd_nvmem)
> +		return;
> +
> +	mtd_nvmem->mtd = mtd;
> +	mtd_nvmem->nvmem = nvmem_register(&config);
> +	if (IS_ERR(mtd_nvmem->nvmem)) {
> +		dev_err(dev, "Failed to register NVMEM device\n");
> +		kfree(mtd_nvmem);
> +		return;
> +	}
> +
> +	mutex_lock(&mtd_nvmem_list_lock);
> +	list_add_tail(&mtd_nvmem->list, &mtd_nvmem_list);
> +	mutex_unlock(&mtd_nvmem_list_lock);
> +}
> +
> +static void mtd_nvmem_remove(struct mtd_info *mtd)
> +{
> +	struct mtd_nvmem *mtd_nvmem;
> +	bool found = false;
> +
> +	mutex_lock(&mtd_nvmem_list_lock);
> +	list_for_each_entry(mtd_nvmem, &mtd_nvmem_list, list) {
> +		if (mtd_nvmem->mtd == mtd) {
> +			list_del(&mtd_nvmem->list);
> +			found = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&mtd_nvmem_list_lock);
> +
> +	if (found) {
> +		if (nvmem_unregister(mtd_nvmem->nvmem))
> +			dev_err(&mtd->dev,
> +				"Failed to unregister NVMEM device\n");

Ouch! You failed to unregister the NVMEM device but you have no way to
stop MTD dev removal, which means you have a potential use-after-free
bug. Not sure this can happen in real life, but I don't like that.

Maybe we should let notifiers return an error if they want to cancel
the removal, or maybe this is a good reason to put the nvmem pointer
directly in mtd_info and call mtd_nvmem_add/remove() directly from
add/del_mtd_device() and allow them to return an error.

Not that, if you go for this solution, you'll also get rid of the
global mtd_nvmem_list list and the associated lock.

> +		kfree(mtd_nvmem);
> +	}
> +}
> +
> +static struct mtd_notifier mtd_nvmem_notifier = {
> +	.add = mtd_nvmem_add,
> +	.remove = mtd_nvmem_remove,
> +};
> +
> +static int __init mtd_nvmem_init(void)
> +{
> +	register_mtd_user(&mtd_nvmem_notifier);
> +	return 0;
> +}
> +module_init(mtd_nvmem_init);
> +
> +static void __exit mtd_nvmem_exit(void)
> +{
> +	unregister_mtd_user(&mtd_nvmem_notifier);
> +}
> +module_exit(mtd_nvmem_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Alban Bedel <albeu@free.fr>");
> +MODULE_DESCRIPTION("Driver to read config data from MTD devices");

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Alban]

[-- Attachment #1: Type: text/plain, Size: 3042 bytes --]

On Thu, 2 Mar 2017 22:18:03 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Thu,  2 Mar 2017 20:50:22 +0100
> Alban <albeu@free.fr> wrote:
> 
> [snip]
>
> > +static void mtd_nvmem_add(struct mtd_info *mtd)
> > +{
> > +	struct device *dev = &mtd->dev;
> > +	struct device_node *np = dev_of_node(dev);
> > +	struct nvmem_config config = {};
> > +	struct mtd_nvmem *mtd_nvmem;
> > +
> > +	/* OF devices have to provide the nvmem node */
> > +	if (np && !of_property_read_bool(np, "nvmem-provider"))
> > +		return;  
> 
> Might have to be adapted according to the DT binding if we decide to
> add an extra subnode, but then, I'm not sure the nvmem cells creation
> will work correctly, because the framework expect nvmem cells to be
> direct children of the nvmem device, which will no longer be the case
> if you add an intermediate node between the MTD device node and the
> nvmem cell nodes.

Yes to support such a binding we would have to fix of_nvmem_cell_get(),
but that should be quiet simple to have it support both the new and old
binding.

>
> [snip]
>
> > +static void mtd_nvmem_remove(struct mtd_info *mtd)
> > +{
> > +	struct mtd_nvmem *mtd_nvmem;
> > +	bool found = false;
> > +
> > +	mutex_lock(&mtd_nvmem_list_lock);
> > +	list_for_each_entry(mtd_nvmem, &mtd_nvmem_list, list) {
> > +		if (mtd_nvmem->mtd == mtd) {
> > +			list_del(&mtd_nvmem->list);
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +	mutex_unlock(&mtd_nvmem_list_lock);
> > +
> > +	if (found) {
> > +		if (nvmem_unregister(mtd_nvmem->nvmem))
> > +			dev_err(&mtd->dev,
> > +				"Failed to unregister NVMEM device\n");  
> 
> Ouch! You failed to unregister the NVMEM device but you have no way to
> stop MTD dev removal, which means you have a potential use-after-free
> bug. Not sure this can happen in real life, but I don't like that.

Yes, I'm aware of this problem. Sorry, I forgot to mention this in the
cover letter.

> Maybe we should let notifiers return an error if they want to cancel
> the removal, or maybe this is a good reason to put the nvmem pointer
> directly in mtd_info and call mtd_nvmem_add/remove() directly from
> add/del_mtd_device() and allow them to return an error.
> 
> Not that, if you go for this solution, you'll also get rid of the
> global mtd_nvmem_list list and the associated lock.

IMHO the MTD users framework has to be re-worked to be useful. First
both the add and remove callbacks should have return values. Users where
the add failed shouldn't be removed later and users where the remove
fails should block the removal of the MTD.

Furthermore only passing the MTD device to the add/remove callback
force the users to keep their own list, which is annoying to say the
least. A simple fix would be to have the add callback return a pointer
that would be passed back to the remove callback. Trivial to implement
and the MTD user wouldn't have to keep any list. I will look into this
in the next days.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Boris Brezillon]

On Fri, 3 Mar 2017 13:36:29 +0100
Alban <albeu@free.fr> wrote:

> On Thu, 2 Mar 2017 22:18:03 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Thu,  2 Mar 2017 20:50:22 +0100
> > Alban <albeu@free.fr> wrote:
> > 
> > [snip]
> >  
> > > +static void mtd_nvmem_add(struct mtd_info *mtd)
> > > +{
> > > +	struct device *dev = &mtd->dev;
> > > +	struct device_node *np = dev_of_node(dev);
> > > +	struct nvmem_config config = {};
> > > +	struct mtd_nvmem *mtd_nvmem;
> > > +
> > > +	/* OF devices have to provide the nvmem node */
> > > +	if (np && !of_property_read_bool(np, "nvmem-provider"))
> > > +		return;    
> > 
> > Might have to be adapted according to the DT binding if we decide to
> > add an extra subnode, but then, I'm not sure the nvmem cells creation
> > will work correctly, because the framework expect nvmem cells to be
> > direct children of the nvmem device, which will no longer be the case
> > if you add an intermediate node between the MTD device node and the
> > nvmem cell nodes.  
> 
> Yes to support such a binding we would have to fix of_nvmem_cell_get(),
> but that should be quiet simple to have it support both the new and old
> binding.
> 
> >
> > [snip]
> >  
> > > +static void mtd_nvmem_remove(struct mtd_info *mtd)
> > > +{
> > > +	struct mtd_nvmem *mtd_nvmem;
> > > +	bool found = false;
> > > +
> > > +	mutex_lock(&mtd_nvmem_list_lock);
> > > +	list_for_each_entry(mtd_nvmem, &mtd_nvmem_list, list) {
> > > +		if (mtd_nvmem->mtd == mtd) {
> > > +			list_del(&mtd_nvmem->list);
> > > +			found = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +	mutex_unlock(&mtd_nvmem_list_lock);
> > > +
> > > +	if (found) {
> > > +		if (nvmem_unregister(mtd_nvmem->nvmem))
> > > +			dev_err(&mtd->dev,
> > > +				"Failed to unregister NVMEM device\n");    
> > 
> > Ouch! You failed to unregister the NVMEM device but you have no way to
> > stop MTD dev removal, which means you have a potential use-after-free
> > bug. Not sure this can happen in real life, but I don't like that.  
> 
> Yes, I'm aware of this problem. Sorry, I forgot to mention this in the
> cover letter.

No problem.

> 
> > Maybe we should let notifiers return an error if they want to cancel
> > the removal, or maybe this is a good reason to put the nvmem pointer
> > directly in mtd_info and call mtd_nvmem_add/remove() directly from
> > add/del_mtd_device() and allow them to return an error.
> > 
> > Not that, if you go for this solution, you'll also get rid of the
> > global mtd_nvmem_list list and the associated lock.  
> 
> IMHO the MTD users framework has to be re-worked to be useful. First
> both the add and remove callbacks should have return values. Users where
> the add failed shouldn't be removed later and users where the remove
> fails should block the removal of the MTD.

As said in my previous reply, it's not just about returning an error. I
had a closer look at the code, and it seems that using
__get_mtd_device() properly should prevent the problem we are talking
about (call __get_mtd_device() after your nvmem_register() and call
__put_mtd_device() only if nvmem_unregister() succeed).

> 
> Furthermore only passing the MTD device to the add/remove callback
> force the users to keep their own list, which is annoying to say the
> least. A simple fix would be to have the add callback return a pointer
> that would be passed back to the remove callback. Trivial to implement
> and the MTD user wouldn't have to keep any list. I will look into this
> in the next days.

That's a different problem, and I'm not sure I like the idea of
changing the ->add() prototype into

	void *(*add)(struct mtd_info *);

If we want to do that, I'd rather see an API extension allowing one to
attach/detach/query/update user data to an MTD device.

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Alban]

[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]

On Fri, 3 Mar 2017 14:36:58 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Fri, 3 Mar 2017 13:36:29 +0100
> Alban <albeu@free.fr> wrote:
> 
> > On Thu, 2 Mar 2017 22:18:03 +0100
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >   
> > > On Thu,  2 Mar 2017 20:50:22 +0100
> > > Alban <albeu@free.fr> wrote:
> > > 
> > > [snip]
> > >    
> [snip]
>   
> > > Maybe we should let notifiers return an error if they want to cancel
> > > the removal, or maybe this is a good reason to put the nvmem pointer
> > > directly in mtd_info and call mtd_nvmem_add/remove() directly from
> > > add/del_mtd_device() and allow them to return an error.
> > > 
> > > Not that, if you go for this solution, you'll also get rid of the
> > > global mtd_nvmem_list list and the associated lock.    
> > 
> > IMHO the MTD users framework has to be re-worked to be useful. First
> > both the add and remove callbacks should have return values. Users where
> > the add failed shouldn't be removed later and users where the remove
> > fails should block the removal of the MTD.  
> 
> As said in my previous reply, it's not just about returning an error. I
> had a closer look at the code, and it seems that using
> __get_mtd_device() properly should prevent the problem we are talking
> about (call __get_mtd_device() after your nvmem_register() and call
> __put_mtd_device() only if nvmem_unregister() succeed).

That's not going to work. If the notifier add increase the MTD reference
count it can never be removed again. What could work would be to
propagate the nvmem device ref counting down to the MTD device, but that
sound complex and would require some non-trivial locking to still allow
for an "always succeed" removal.

> > 
> > Furthermore only passing the MTD device to the add/remove callback
> > force the users to keep their own list, which is annoying to say the
> > least. A simple fix would be to have the add callback return a pointer
> > that would be passed back to the remove callback. Trivial to implement
> > and the MTD user wouldn't have to keep any list. I will look into this
> > in the next days.  
> 
> That's a different problem, and I'm not sure I like the idea of
> changing the ->add() prototype into
> 
> 	void *(*add)(struct mtd_info *);
> 
> If we want to do that, I'd rather see an API extension allowing one to
> attach/detach/query/update user data to an MTD device.

Under which condition would these be triggered? That sound more than is
needed. I would just use the above add along with:

 int (*remove)(struct mtd_info *, void *);

And add a list of successfully added notifiers, along with their
data pointer, to the MTD device. That's simple and would also remove
the need for notifier to have a private list of their instances as I
had to do here.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Boris Brezillon]

On Fri, 3 Mar 2017 14:57:26 +0100
Alban <albeu@free.fr> wrote:

> On Fri, 3 Mar 2017 14:36:58 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Fri, 3 Mar 2017 13:36:29 +0100
> > Alban <albeu@free.fr> wrote:
> >   
> > > On Thu, 2 Mar 2017 22:18:03 +0100
> > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > >     
> > > > On Thu,  2 Mar 2017 20:50:22 +0100
> > > > Alban <albeu@free.fr> wrote:
> > > > 
> > > > [snip]
> > > >      
> > [snip]
> >     
> > > > Maybe we should let notifiers return an error if they want to cancel
> > > > the removal, or maybe this is a good reason to put the nvmem pointer
> > > > directly in mtd_info and call mtd_nvmem_add/remove() directly from
> > > > add/del_mtd_device() and allow them to return an error.
> > > > 
> > > > Not that, if you go for this solution, you'll also get rid of the
> > > > global mtd_nvmem_list list and the associated lock.      
> > > 
> > > IMHO the MTD users framework has to be re-worked to be useful. First
> > > both the add and remove callbacks should have return values. Users where
> > > the add failed shouldn't be removed later and users where the remove
> > > fails should block the removal of the MTD.    
> > 
> > As said in my previous reply, it's not just about returning an error. I
> > had a closer look at the code, and it seems that using
> > __get_mtd_device() properly should prevent the problem we are talking
> > about (call __get_mtd_device() after your nvmem_register() and call
> > __put_mtd_device() only if nvmem_unregister() succeed).  
> 
> That's not going to work. If the notifier add increase the MTD reference
> count it can never be removed again.

That's not true, see the del_mtd_device() function [1], it's calling
the ->remove() notifiers before even testing ->usecount, so, if you
call __put_mtd_device() in your ->remove() hook you should be fine.

> What could work would be to
> propagate the nvmem device ref counting down to the MTD device, but that
> sound complex and would require some non-trivial locking to still allow
> for an "always succeed" removal.
> 
> > > 
> > > Furthermore only passing the MTD device to the add/remove callback
> > > force the users to keep their own list, which is annoying to say the
> > > least. A simple fix would be to have the add callback return a pointer
> > > that would be passed back to the remove callback. Trivial to implement
> > > and the MTD user wouldn't have to keep any list. I will look into this
> > > in the next days.    
> > 
> > That's a different problem, and I'm not sure I like the idea of
> > changing the ->add() prototype into
> > 
> > 	void *(*add)(struct mtd_info *);
> > 
> > If we want to do that, I'd rather see an API extension allowing one to
> > attach/detach/query/update user data to an MTD device.  
> 
> Under which condition would these be triggered? That sound more than is
> needed. I would just use the above add along with:
> 
>  int (*remove)(struct mtd_info *, void *);
> 
> And add a list of successfully added notifiers, along with their
> data pointer, to the MTD device. That's simple and would also remove
> the need for notifier to have a private list of their instances as I
> had to do here.

And then you're abusing the notifier concept. As said earlier, a
notifier is not necessarily using the device, and thus, don't
necessarily need private data.
It's not only about what is the simplest solution for your use case,
but also what other users want/need.


[1]http://lxr.free-electrons.com/source/drivers/mtd/mtdcore.c#L592

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Richard Weinberger]

Am 03.03.2017 um 15:11 schrieb Boris Brezillon:
>> And add a list of successfully added notifiers, along with their
>> data pointer, to the MTD device. That's simple and would also remove
>> the need for notifier to have a private list of their instances as I
>> had to do here.
> 
> And then you're abusing the notifier concept. As said earlier, a
> notifier is not necessarily using the device, and thus, don't
> necessarily need private data.
> It's not only about what is the simplest solution for your use case,
> but also what other users want/need.

Yes, please don't use the mtd_notifier.
I strongly vote to embed the nvmem pointer into struct mtd_info.

Thanks,
//richard

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Alban]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

On Fri, 3 Mar 2017 23:21:29 +0100
Richard Weinberger <richard@nod.at> wrote:

> Am 03.03.2017 um 15:11 schrieb Boris Brezillon:
> >> And add a list of successfully added notifiers, along with their
> >> data pointer, to the MTD device. That's simple and would also remove
> >> the need for notifier to have a private list of their instances as I
> >> had to do here.  
> > 
> > And then you're abusing the notifier concept. As said earlier, a
> > notifier is not necessarily using the device, and thus, don't
> > necessarily need private data.
> > It's not only about what is the simplest solution for your use case,
> > but also what other users want/need.  
> 
> Yes, please don't use the mtd_notifier.
> I strongly vote to embed the nvmem pointer into struct mtd_info.

Ok, I'll do that. However it mean it will have to stays in
drivers/mtd as it then become part of the MTD core.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Richard Weinberger]

Am 06.03.2017 um 18:21 schrieb Alban:
> On Fri, 3 Mar 2017 23:21:29 +0100
> Richard Weinberger <richard@nod.at> wrote:
> 
>> Am 03.03.2017 um 15:11 schrieb Boris Brezillon:
>>>> And add a list of successfully added notifiers, along with their
>>>> data pointer, to the MTD device. That's simple and would also remove
>>>> the need for notifier to have a private list of their instances as I
>>>> had to do here.  
>>>
>>> And then you're abusing the notifier concept. As said earlier, a
>>> notifier is not necessarily using the device, and thus, don't
>>> necessarily need private data.
>>> It's not only about what is the simplest solution for your use case,
>>> but also what other users want/need.  
>>
>> Yes, please don't use the mtd_notifier.
>> I strongly vote to embed the nvmem pointer into struct mtd_info.
> 
> Ok, I'll do that. However it mean it will have to stays in
> drivers/mtd as it then become part of the MTD core.

Brian, are you fine with this?
I know, refcounting in MTD is tricky. :(

Thanks,
//richard

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Boris Brezillon]

On Mon, 6 Mar 2017 20:03:28 +0100
Richard Weinberger <richard@nod.at> wrote:

> Am 06.03.2017 um 18:21 schrieb Alban:
> > On Fri, 3 Mar 2017 23:21:29 +0100
> > Richard Weinberger <richard@nod.at> wrote:
> >   
> >> Am 03.03.2017 um 15:11 schrieb Boris Brezillon:  
> >>>> And add a list of successfully added notifiers, along with their
> >>>> data pointer, to the MTD device. That's simple and would also remove
> >>>> the need for notifier to have a private list of their instances as I
> >>>> had to do here.    
> >>>
> >>> And then you're abusing the notifier concept. As said earlier, a
> >>> notifier is not necessarily using the device, and thus, don't
> >>> necessarily need private data.
> >>> It's not only about what is the simplest solution for your use case,
> >>> but also what other users want/need.    
> >>
> >> Yes, please don't use the mtd_notifier.
> >> I strongly vote to embed the nvmem pointer into struct mtd_info.  
> > 
> > Ok, I'll do that. However it mean it will have to stays in
> > drivers/mtd as it then become part of the MTD core.  
> 
> Brian, are you fine with this?

Same question to Srinivas.

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Srinivas Kandagatla]

On 02/03/17 19:50, Alban wrote:
> Allow drivers that use the nvmem API to read data stored on MTD devices.
> This add a simple mtd user that register itself as a read-only nvmem
> device.
>
Good stuff!! and useful for MAC addresses.

Am not going to repeat the same comments as Boris, but I totally agree 
with his comments.

> Signed-off-by: Alban <albeu@free.fr>
> ---
>  drivers/mtd/Kconfig    |   9 ++++
>  drivers/mtd/Makefile   |   1 +
>  drivers/mtd/mtdnvmem.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)
>  create mode 100644 drivers/mtd/mtdnvmem.c

May be we should move this driver to drivers/nvmem/
>
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index e83a279..9cad86c 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -322,6 +322,15 @@ config MTD_PARTITIONED_MASTER
>  	  the parent of the partition device be the master device, rather than
>  	  what lies behind the master.
>
> +config MTD_NVMEM
> +	tristate "Read config data from MTD devices"

May be..

"Read config data from MTD devices via NVMEM API".

Or

"MTD NVMEM Provider"

> +	default y

Do you really want it be ON by default?
> +	depends on NVMEM

Adding COMPILE_TEST would give us good test coverage.

> +	help
> +	  Provides support for reading config data from MTD devices. This can
> +	  be used by drivers to read device specific data such as MAC addresses
> +	  or calibration results.
> +
>  source "drivers/mtd/chips/Kconfig"
>
>  source "drivers/mtd/maps/Kconfig"
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..f62f50b 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_SSFDC)		+= ssfdc.o
>  obj-$(CONFIG_SM_FTL)		+= sm_ftl.o
>  obj-$(CONFIG_MTD_OOPS)		+= mtdoops.o
>  obj-$(CONFIG_MTD_SWAP)		+= mtdswap.o
> +obj-$(CONFIG_MTD_NVMEM)		+= mtdnvmem.o
>
>  nftl-objs		:= nftlcore.o nftlmount.o
>  inftl-objs		:= inftlcore.o inftlmount.o
> diff --git a/drivers/mtd/mtdnvmem.c b/drivers/mtd/mtdnvmem.c
> new file mode 100644
> index 0000000..6eb4216
> --- /dev/null
> +++ b/drivers/mtd/mtdnvmem.c
> @@ -0,0 +1,121 @@
> +/*
> + * Copyright (C) 2017 Alban Bedel <albeu@free.fr>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/nvmem-consumer.h>
??

> +#include <linux/slab.h>
> +#include <linux/of.h>
> +
> +struct mtd_nvmem {
> +	struct list_head list;
> +	struct mtd_info *mtd;
> +	struct nvmem_device *nvmem;
> +};
> +
> +static DEFINE_MUTEX(mtd_nvmem_list_lock);
> +static LIST_HEAD(mtd_nvmem_list);
> +
> +static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
> +			      void *val, size_t bytes)
> +{
> +	struct mtd_info *mtd = priv;
> +	size_t retlen;
> +	int err;
> +
> +	err = mtd_read(mtd, offset, bytes, &retlen, val);
> +	if (err && err != -EUCLEAN)
> +		return err;
> +
> +	return retlen == bytes ? 0 : -EIO;
> +}
> +
> +static void mtd_nvmem_add(struct mtd_info *mtd)
> +{
> +	struct device *dev = &mtd->dev;
> +	struct device_node *np = dev_of_node(dev);
> +	struct nvmem_config config = {};
> +	struct mtd_nvmem *mtd_nvmem;
> +
> +	/* OF devices have to provide the nvmem node */
> +	if (np && !of_property_read_bool(np, "nvmem-provider"))
> +		return;

we should prefix the property with mtd to make to more explicit that 
this is very much specific to MTD.


> +
> +	config.dev = dev;
> +	config.owner = THIS_MODULE;
> +	config.reg_read = mtd_nvmem_reg_read;
> +	config.size = mtd->size;
> +	config.word_size = 1;
> +	config.stride = 1;
> +	config.read_only = true;
> +	config.priv = mtd;
> +
> +	/* Alloc our struct to keep track of the MTD NVMEM devices */
> +	mtd_nvmem = kzalloc(sizeof(*mtd_nvmem), GFP_KERNEL);
> +	if (!mtd_nvmem)
> +		return;
> +
> +	mtd_nvmem->mtd = mtd;
> +	mtd_nvmem->nvmem = nvmem_register(&config);
> +	if (IS_ERR(mtd_nvmem->nvmem)) {
> +		dev_err(dev, "Failed to register NVMEM device\n");
> +		kfree(mtd_nvmem);
> +		return;
> +	}
> +
> +	mutex_lock(&mtd_nvmem_list_lock);
> +	list_add_tail(&mtd_nvmem->list, &mtd_nvmem_list);
> +	mutex_unlock(&mtd_nvmem_list_lock);
> +}
> +
> +static void mtd_nvmem_remove(struct mtd_info *mtd)
> +{
> +	struct mtd_nvmem *mtd_nvmem;
> +	bool found = false;
> +

May be we can use of_nvmem_find() directly here and avoid all this list 
and lock thingy. It should make the driver much simpler.

Am sure we can add exception to make of_nvmem_find() symbol public if 
its helping providers like this.


> +	mutex_lock(&mtd_nvmem_list_lock);
> +	list_for_each_entry(mtd_nvmem, &mtd_nvmem_list, list) {
> +		if (mtd_nvmem->mtd == mtd) {
> +			list_del(&mtd_nvmem->list);
> +			found = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&mtd_nvmem_list_lock);
> +
> +	if (found) {
> +		if (nvmem_unregister(mtd_nvmem->nvmem))
> +			dev_err(&mtd->dev,
> +				"Failed to unregister NVMEM device\n");

I will be nice to feedback error to top layer, as it does not make sense 
to remove providers if there are active consumers using it.

del_mtd_device(), unregister_mtd_user() have return values, I see no 
reason why notifiers  should not return errors.
May be if we should fix the remove() call backs to handle and return errors.


> +		kfree(mtd_nvmem);
> +	}

> +}
> +
> +static struct mtd_notifier mtd_nvmem_notifier = {
> +	.add = mtd_nvmem_add,
> +	.remove = mtd_nvmem_remove,
> +};
> +
> +static int __init mtd_nvmem_init(void)
> +{
> +	register_mtd_user(&mtd_nvmem_notifier);
> +	return 0;
> +}
> +module_init(mtd_nvmem_init);
> +
> +static void __exit mtd_nvmem_exit(void)
> +{
> +	unregister_mtd_user(&mtd_nvmem_notifier);
> +}
> +module_exit(mtd_nvmem_exit);

> +
> +MODULE_LICENSE("GPL");

GPL V2  ??


Thanks,
srini
> +MODULE_AUTHOR("Alban Bedel <albeu@free.fr>");
> +MODULE_DESCRIPTION("Driver to read config data from MTD devices");
>

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Boris Brezillon]

On Fri, 3 Mar 2017 11:23:16 +0000
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:


> 
> > +	mutex_lock(&mtd_nvmem_list_lock);
> > +	list_for_each_entry(mtd_nvmem, &mtd_nvmem_list, list) {
> > +		if (mtd_nvmem->mtd == mtd) {
> > +			list_del(&mtd_nvmem->list);
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +	mutex_unlock(&mtd_nvmem_list_lock);
> > +
> > +	if (found) {
> > +		if (nvmem_unregister(mtd_nvmem->nvmem))
> > +			dev_err(&mtd->dev,
> > +				"Failed to unregister NVMEM device\n");  
> 
> I will be nice to feedback error to top layer, as it does not make sense 
> to remove providers if there are active consumers using it.
> 
> del_mtd_device(), unregister_mtd_user() have return values, I see no 
> reason why notifiers  should not return errors.
> May be if we should fix the remove() call backs to handle and return errors.

It's more complicated than that. What should you do if one of the
->remove() notifier in the middle of the list is returning an error?
Some of them have already taken the remove notification into account.
Should we call ->add() back on those notifiers? Also, I'm not sure they
are all safe against double ->remove() calls, so if we might be in
trouble when the removal is retried.

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Alban]

[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]

On Fri, 3 Mar 2017 13:34:19 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Fri, 3 Mar 2017 11:23:16 +0000
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
> 
> >   
> > > +	mutex_lock(&mtd_nvmem_list_lock);
> > > +	list_for_each_entry(mtd_nvmem, &mtd_nvmem_list, list) {
> > > +		if (mtd_nvmem->mtd == mtd) {
> > > +			list_del(&mtd_nvmem->list);
> > > +			found = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +	mutex_unlock(&mtd_nvmem_list_lock);
> > > +
> > > +	if (found) {
> > > +		if (nvmem_unregister(mtd_nvmem->nvmem))
> > > +			dev_err(&mtd->dev,
> > > +				"Failed to unregister NVMEM device\n");    
> > 
> > I will be nice to feedback error to top layer, as it does not make sense 
> > to remove providers if there are active consumers using it.
> > 
> > del_mtd_device(), unregister_mtd_user() have return values, I see no 
> > reason why notifiers  should not return errors.
> > May be if we should fix the remove() call backs to handle and return errors.  
> 
> It's more complicated than that. What should you do if one of the
> ->remove() notifier in the middle of the list is returning an error?  
> Some of them have already taken the remove notification into account.
> Should we call ->add() back on those notifiers? Also, I'm not sure they
> are all safe against double ->remove() calls, so if we might be in
> trouble when the removal is retried.

Re-adding make no sense as that could also fails. Keep it simple,
remove the notifier from the list when remove() succeed, abort when one
fails. In such a scenario that mean there is a dependency, the sys
admin should then solve this dependency and re-trigger the MTD removal.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API

[Boris Brezillon]

On Fri, 3 Mar 2017 14:30:21 +0100
Alban <albeu@free.fr> wrote:

> On Fri, 3 Mar 2017 13:34:19 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Fri, 3 Mar 2017 11:23:16 +0000
> > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> > 
> >   
> > >     
> > > > +	mutex_lock(&mtd_nvmem_list_lock);
> > > > +	list_for_each_entry(mtd_nvmem, &mtd_nvmem_list, list) {
> > > > +		if (mtd_nvmem->mtd == mtd) {
> > > > +			list_del(&mtd_nvmem->list);
> > > > +			found = true;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +	mutex_unlock(&mtd_nvmem_list_lock);
> > > > +
> > > > +	if (found) {
> > > > +		if (nvmem_unregister(mtd_nvmem->nvmem))
> > > > +			dev_err(&mtd->dev,
> > > > +				"Failed to unregister NVMEM device\n");      
> > > 
> > > I will be nice to feedback error to top layer, as it does not make sense 
> > > to remove providers if there are active consumers using it.
> > > 
> > > del_mtd_device(), unregister_mtd_user() have return values, I see no 
> > > reason why notifiers  should not return errors.
> > > May be if we should fix the remove() call backs to handle and return errors.    
> > 
> > It's more complicated than that. What should you do if one of the  
> > ->remove() notifier in the middle of the list is returning an error?    
> > Some of them have already taken the remove notification into account.
> > Should we call ->add() back on those notifiers? Also, I'm not sure they
> > are all safe against double ->remove() calls, so if we might be in
> > trouble when the removal is retried.  
> 
> Re-adding make no sense as that could also fails.

I agree.

> Keep it simple,
> remove the notifier from the list when remove() succeed, abort when one
> fails. In such a scenario that mean there is a dependency, the sys
> admin should then solve this dependency and re-trigger the MTD removal.

Except notifiers are by definition not attached to a specific MTD
device. I get your point, but I think we should clarify the different
concepts.

An mtd_notifier (which seems to also be called a user in a few places)
is something that should be called each time you have an MTD
creation/removal event (or when you add a notifier to the list). You
could have notifiers that don't do anything special with the MTD
device, hence they don't require private data.

I think we should add the mtd_user concept, which would be a specific
user of an MTD device that can contain private data and is likely to be
attached to the MTD device after the notifier's ->add() method is
called.

struct mtd_user_ops {
	int (*remove)(struct mtd_user *);
};

struct mtd_user {
	struct list_node node;
	const struct mtd_user_ops *ops;
}

int mtd_attach_user(struct mtd_info *mtd, struct mtd_user *user);
int mtd_detach_user(struct mtd_info *mtd, struct mtd_user *user);

and then inside the del_mtd_device() function, before you iterate over
all notifiers, you could iterate over all attached users and call their
->remove() method. If one fails, then you stop the removal procedure.

[PATCH 3/3] nvmem: core: Allow allocating several anonymous nvmem devices

[Alban]

Currently the nvmem core expect the config to provide a name and ID
that are then used to create the device name. When no device name is
given 'nvmem' is used. However if there is several such anonymous
devices they all get named 'nvmem0', which doesn't work.

To fix this problem use the ID from the config only when the config
also provides a name. When no name is provided take the uinque ID of
the nvmem device instead.

Signed-off-by: Alban <albeu@free.fr>
---
 drivers/nvmem/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 408b521..8c830a8 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -468,7 +468,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	np = config->dev->of_node;
 	nvmem->dev.of_node = np;
 	dev_set_name(&nvmem->dev, "%s%d",
-		     config->name ? : "nvmem", config->id);
+		     config->name ? : "nvmem",
+		     config->name ? config->id : nvmem->id);
 
 	nvmem->read_only = of_property_read_bool(np, "read-only") |
 			   config->read_only;
-- 
2.7.4

Re: [PATCH 3/3] nvmem: core: Allow allocating several anonymous nvmem devices

[Boris Brezillon]

On Thu,  2 Mar 2017 20:50:23 +0100
Alban <albeu@free.fr> wrote:

> Currently the nvmem core expect the config to provide a name and ID
> that are then used to create the device name. When no device name is
> given 'nvmem' is used. However if there is several such anonymous
> devices they all get named 'nvmem0', which doesn't work.
> 
> To fix this problem use the ID from the config only when the config
> also provides a name. When no name is provided take the uinque ID of
> the nvmem device instead.
> 
> Signed-off-by: Alban <albeu@free.fr>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/nvmem/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 408b521..8c830a8 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -468,7 +468,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  	np = config->dev->of_node;
>  	nvmem->dev.of_node = np;
>  	dev_set_name(&nvmem->dev, "%s%d",
> -		     config->name ? : "nvmem", config->id);
> +		     config->name ? : "nvmem",
> +		     config->name ? config->id : nvmem->id);
>  
>  	nvmem->read_only = of_property_read_bool(np, "read-only") |
>  			   config->read_only;

Re: [PATCH 3/3] nvmem: core: Allow allocating several anonymous nvmem devices

[Moritz Fischer]

On Thu, Mar 2, 2017 at 12:03 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Thu,  2 Mar 2017 20:50:23 +0100
> Alban <albeu@free.fr> wrote:
>
>> Currently the nvmem core expect the config to provide a name and ID
>> that are then used to create the device name. When no device name is
>> given 'nvmem' is used. However if there is several such anonymous
>> devices they all get named 'nvmem0', which doesn't work.
>>
>> To fix this problem use the ID from the config only when the config
>> also provides a name. When no name is provided take the uinque ID of
>> the nvmem device instead.
>>
>> Signed-off-by: Alban <albeu@free.fr>
>
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Reviewed-by: Moritz Fischer <mdf@kernel.org>

Thanks,
Moritz

Re: [PATCH 3/3] nvmem: core: Allow allocating several anonymous nvmem devices

[Srinivas Kandagatla]

On 02/03/17 19:50, Alban wrote:
> Currently the nvmem core expect the config to provide a name and ID
> that are then used to create the device name. When no device name is
> given 'nvmem' is used. However if there is several such anonymous
> devices they all get named 'nvmem0', which doesn't work.
>
> To fix this problem use the ID from the config only when the config
> also provides a name. When no name is provided take the uinque ID of
> the nvmem device instead.
>
> Signed-off-by: Alban <albeu@free.fr>
> ---

Thanks for the Fix, looks good to me, I will queue this up once rc1 is out.


>  drivers/nvmem/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 408b521..8c830a8 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -468,7 +468,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  	np = config->dev->of_node;
>  	nvmem->dev.of_node = np;
>  	dev_set_name(&nvmem->dev, "%s%d",
> -		     config->name ? : "nvmem", config->id);
> +		     config->name ? : "nvmem",
> +		     config->name ? config->id : nvmem->id);
>
>  	nvmem->read_only = of_property_read_bool(np, "read-only") |
>  			   config->read_only;
>