[PATCH v2 0/5] mtd: core: OTP nvmem provider support

[PATCH v2 0/5] mtd: core: OTP nvmem provider support

[Michael Walle]

Some flashes provide one (or more) OTP regions which can be used to
store MAC addresses or serial numbers. Implement a NVMEM provider for
this storage which then can be used by a network card to fetch the
MAC adress for example.

This is an example DT node:

flash@0 {
    otp {
        compatible = "user-otp";
        #address-cells = <1>;
        #size-cells = <1>;

        serial-number@0 {
            reg = <0x0 0x8>;
        };
    };
};

Michael Walle (5):
  nvmem: core: allow specifying of_node
  dt-bindings: mtd: add YAML schema for the generic MTD bindings
  dt-bindings: mtd: add OTP bindings
  dt-bindings: mtd: spi-nor: add otp property
  mtd: core: add OTP nvmem provider support

 .../devicetree/bindings/mtd/common.txt        |  16 +-
 .../bindings/mtd/jedec,spi-nor.yaml           |   6 +
 .../devicetree/bindings/mtd/mtd.yaml          |  89 +++++++++++
 drivers/mtd/mtdcore.c                         | 148 ++++++++++++++++++
 drivers/nvmem/core.c                          |   4 +-
 include/linux/mtd/mtd.h                       |   2 +
 include/linux/nvmem-provider.h                |   2 +
 7 files changed, 251 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtd.yaml

-- 
2.20.1

[PATCH v2 1/5] nvmem: core: allow specifying of_node

[Michael Walle]

Until now, the of_node of the parent device is used. Some devices
provide more than just the nvmem provider. To avoid name space clashes,
add a way to allow specifying the nvmem cells in subnodes. Consider the
following example:

    flash@0 {
        compatible = "jedec,spi-nor";

        partitions {
            compatible = "fixed-partitions";
            #address-cells = <1>;
            #size-cells = <1>;

            partition@0 {
                reg = <0x000000 0x010000>;
            };
        };

        otp {
            compatible = "user-otp";
            #address-cells = <1>;
            #size-cells = <1>;

            serial-number@0 {
                reg = <0x0 0x8>;
            };
        };
    };

There the nvmem provider might be the MTD partition or the OTP region of
the flash.

Add a new config->of_node parameter, which if set, will be used instead
of the parent's of_node.

Signed-off-by: Michael Walle <michael@walle.cc>
Acked-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Changes since v1:
 - none

Changes since RFC:
 - none

 drivers/nvmem/core.c           | 4 +++-
 include/linux/nvmem-provider.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index bca671ff4e54..62d363a399d3 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -789,7 +789,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	nvmem->reg_write = config->reg_write;
 	nvmem->keepout = config->keepout;
 	nvmem->nkeepout = config->nkeepout;
-	if (!config->no_of_node)
+	if (config->of_node)
+		nvmem->dev.of_node = config->of_node;
+	else if (!config->no_of_node)
 		nvmem->dev.of_node = config->dev->of_node;
 
 	switch (config->id) {
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index e162b757b6d5..471cb7b9e896 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -57,6 +57,7 @@ struct nvmem_keepout {
  * @type:	Type of the nvmem storage
  * @read_only:	Device is read-only.
  * @root_only:	Device is accessibly to root only.
+ * @of_node:	If given, this will be used instead of the parent's of_node.
  * @no_of_node:	Device should not use the parent's of_node even if it's !NULL.
  * @reg_read:	Callback to read data.
  * @reg_write:	Callback to write data.
@@ -86,6 +87,7 @@ struct nvmem_config {
 	enum nvmem_type		type;
 	bool			read_only;
 	bool			root_only;
+	struct device_node	*of_node;
 	bool			no_of_node;
 	nvmem_reg_read_t	reg_read;
 	nvmem_reg_write_t	reg_write;
-- 
2.20.1

[PATCH v2 2/5] dt-bindings: mtd: add YAML schema for the generic MTD bindings

[Michael Walle]

Convert MTD's common.txt to mtd.yaml.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes since v1:
 - none

Changes since RFC:
 - use real device compatibles

 .../devicetree/bindings/mtd/common.txt        | 16 +-------
 .../devicetree/bindings/mtd/mtd.yaml          | 39 +++++++++++++++++++
 2 files changed, 40 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtd.yaml

diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
index fc068b923d7a..ae16f9ea8606 100644
--- a/Documentation/devicetree/bindings/mtd/common.txt
+++ b/Documentation/devicetree/bindings/mtd/common.txt
@@ -1,15 +1 @@
-* Common properties of all MTD devices
-
-Optional properties:
-- label: user-defined MTD device name. Can be used to assign user
-  friendly names to MTD devices (instead of the flash model or flash
-  controller based name) in order to ease flash device identification
-  and/or describe what they are used for.
-
-Example:
-
-	flash@0 {
-		label = "System-firmware";
-
-		/* flash type specific properties */
-	};
+This file has been moved to mtd.yaml.
diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml b/Documentation/devicetree/bindings/mtd/mtd.yaml
new file mode 100644
index 000000000000..086b0ecd1604
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mtd/mtd.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MTD (Memory Technology Device) Device Tree Bindings
+
+maintainers:
+  - Miquel Raynal <miquel.raynal@bootlin.com>
+  - Richard Weinberger <richard@nod.at>
+
+properties:
+  $nodename:
+    pattern: "^flash(@.*)?$"
+
+  label:
+    description:
+      User-defined MTD device name. Can be used to assign user friendly
+      names to MTD devices (instead of the flash model or flash controller
+      based name) in order to ease flash device identification and/or
+      describe what they are used for.
+
+additionalProperties: true
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        flash@0 {
+            reg = <0>;
+            compatible = "jedec,spi-nor";
+            label = "System-firmware";
+        };
+    };
+
+...
-- 
2.20.1

[PATCH v2 3/5] dt-bindings: mtd: add OTP bindings

[Michael Walle]

Flash devices can have one-time-programmable regions. Add a nvmem
binding so they can be used as a nvmem provider.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v1:
 - drop "mtd-" prefix for the compatible strings

Changes since RFC:
 - added missing "$"
 - dropped first example
 - use real device compatibles

   Please note, that this will lead to an error without patch 4/5, which
   introduces that property for the jedec,spi-nor. Should I keep it
   seperate or should I squash that patch into this one?

 .../devicetree/bindings/mtd/mtd.yaml          | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml b/Documentation/devicetree/bindings/mtd/mtd.yaml
index 086b0ecd1604..376b679cfc70 100644
--- a/Documentation/devicetree/bindings/mtd/mtd.yaml
+++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
@@ -21,6 +21,25 @@ properties:
       based name) in order to ease flash device identification and/or
       describe what they are used for.
 
+patternProperties:
+  "^otp(-[0-9]+)?$":
+    type: object
+    $ref: ../nvmem/nvmem.yaml#
+
+    description: |
+      An OTP memory region. Some flashes provide a one-time-programmable
+      memory whose content can either be programmed by a user or is already
+      pre-programmed by the factory. Some flashes might provide both.
+
+    properties:
+      compatible:
+        enum:
+          - user-otp
+          - factory-otp
+
+    required:
+      - compatible
+
 additionalProperties: true
 
 examples:
@@ -36,4 +55,35 @@ examples:
         };
     };
 
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        flash@0 {
+            reg = <0>;
+            compatible = "jedec,spi-nor";
+
+            otp-1 {
+                compatible = "factory-otp";
+                #address-cells = <1>;
+                #size-cells = <1>;
+
+                electronic-serial-number@0 {
+                    reg = <0 8>;
+                };
+            };
+
+            otp-2 {
+                compatible = "user-otp";
+                #address-cells = <1>;
+                #size-cells = <1>;
+
+                mac-address@0 {
+                    reg = <0 6>;
+                };
+            };
+        };
+    };
+
 ...
-- 
2.20.1

[PATCH v2 4/5] dt-bindings: mtd: spi-nor: add otp property

[Michael Walle]

SPI-NOR flashes may have OTP regions and have a nvmem binding. This
binding is described in mtd.yaml.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v1:
 - none

Changes since RFC:
 - new patch

 Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
index 5e7e5349f9a1..ed590d7c6e37 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
@@ -9,6 +9,9 @@ title: SPI NOR flash ST M25Pxx (and similar) serial flash chips
 maintainers:
   - Rob Herring <robh@kernel.org>
 
+allOf:
+  - $ref: "mtd.yaml#"
+
 properties:
   compatible:
     oneOf:
@@ -82,6 +85,9 @@ patternProperties:
   '^partition@':
     type: object
 
+  "^otp(-[0-9]+)?$":
+    type: object
+
 additionalProperties: false
 
 examples:
-- 
2.20.1

[PATCH v2 5/5] mtd: core: add OTP nvmem provider support

[Michael Walle]

Flash OTP regions can already be read via user space. Some boards have
their serial number or MAC addresses stored in the OTP regions. Add
support for them being a (read-only) nvmem provider.

The API to read the OTP data is already in place. It distinguishes
between factory and user OTP, thus there are up to two different
providers.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v1:
 - combine name and compatible string in mtd_otp_nvmem_register()

Changes since RFC:
 - none

 drivers/mtd/mtdcore.c   | 148 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h |   2 +
 2 files changed, 150 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 9aaeadd53eb4..72e7000a86fd 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -777,6 +777,146 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
 	mutex_init(&mtd->master.chrdev_lock);
 }
 
+static ssize_t mtd_otp_size(struct mtd_info *mtd, bool is_user)
+{
+	struct otp_info *info = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	ssize_t size = 0;
+	unsigned int i;
+	size_t retlen;
+	int ret;
+
+	if (is_user)
+		ret = mtd_get_user_prot_info(mtd, PAGE_SIZE, &retlen, info);
+	else
+		ret = mtd_get_fact_prot_info(mtd, PAGE_SIZE, &retlen, info);
+	if (ret)
+		goto err;
+
+	for (i = 0; i < retlen / sizeof(*info); i++) {
+		size += info->length;
+		info++;
+	}
+
+	kfree(info);
+	return size;
+
+err:
+	kfree(info);
+	return ret;
+}
+
+static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
+						   const char *compatible,
+						   int size,
+						   nvmem_reg_read_t reg_read)
+{
+	struct nvmem_device *nvmem = NULL;
+	struct nvmem_config config = {};
+	struct device_node *np;
+
+	/* DT binding is optional */
+	np = of_get_compatible_child(mtd->dev.of_node, compatible);
+
+	/* OTP nvmem will be registered on the physical device */
+	config.dev = mtd->dev.parent;
+	/* just reuse the compatible as name */
+	config.name = compatible;
+	config.id = NVMEM_DEVID_NONE;
+	config.owner = THIS_MODULE;
+	config.type = NVMEM_TYPE_OTP;
+	config.root_only = true;
+	config.reg_read = reg_read;
+	config.size = size;
+	config.of_node = np;
+	config.priv = mtd;
+
+	nvmem = nvmem_register(&config);
+	/* Just ignore if there is no NVMEM support in the kernel */
+	if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EOPNOTSUPP)
+		nvmem = NULL;
+
+	of_node_put(np);
+
+	return nvmem;
+}
+
+static int mtd_nvmem_user_otp_reg_read(void *priv, unsigned int offset,
+				       void *val, size_t bytes)
+{
+	struct mtd_info *mtd = priv;
+	size_t retlen;
+	int ret;
+
+	ret = mtd_read_user_prot_reg(mtd, offset, bytes, &retlen, val);
+	if (ret)
+		return ret;
+
+	return retlen == bytes ? 0 : -EIO;
+}
+
+static int mtd_nvmem_fact_otp_reg_read(void *priv, unsigned int offset,
+				       void *val, size_t bytes)
+{
+	struct mtd_info *mtd = priv;
+	size_t retlen;
+	int ret;
+
+	ret = mtd_read_fact_prot_reg(mtd, offset, bytes, &retlen, val);
+	if (ret)
+		return ret;
+
+	return retlen == bytes ? 0 : -EIO;
+}
+
+static int mtd_otp_nvmem_add(struct mtd_info *mtd)
+{
+	struct nvmem_device *nvmem;
+	ssize_t size;
+	int err;
+
+	if (mtd->_get_user_prot_info && mtd->_read_user_prot_reg) {
+		size = mtd_otp_size(mtd, true);
+		if (size < 0)
+			return size;
+
+		if (size > 0) {
+			nvmem = mtd_otp_nvmem_register(mtd, "user-otp", size,
+						       mtd_nvmem_user_otp_reg_read);
+			if (IS_ERR(nvmem)) {
+				dev_err(&mtd->dev, "Failed to register OTP NVMEM device\n");
+				return PTR_ERR(nvmem);
+			}
+			mtd->otp_user_nvmem = nvmem;
+		}
+	}
+
+	if (mtd->_get_fact_prot_info && mtd->_read_fact_prot_reg) {
+		size = mtd_otp_size(mtd, false);
+		if (size < 0) {
+			err = size;
+			goto err;
+		}
+
+		if (size > 0) {
+			nvmem = mtd_otp_nvmem_register(mtd, "factory-otp", size,
+						       mtd_nvmem_fact_otp_reg_read);
+			if (IS_ERR(nvmem)) {
+				dev_err(&mtd->dev, "Failed to register OTP NVMEM device\n");
+				err = PTR_ERR(nvmem);
+				goto err;
+			}
+			mtd->otp_factory_nvmem = nvmem;
+		}
+	}
+
+	return 0;
+
+err:
+	if (mtd->otp_user_nvmem)
+		nvmem_unregister(mtd->otp_user_nvmem);
+	return err;
+}
+
 /**
  * mtd_device_parse_register - parse partitions and register an MTD device.
  *
@@ -852,6 +992,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		register_reboot_notifier(&mtd->reboot_notifier);
 	}
 
+	ret = mtd_otp_nvmem_add(mtd);
+
 out:
 	if (ret && device_is_registered(&mtd->dev))
 		del_mtd_device(mtd);
@@ -873,6 +1015,12 @@ int mtd_device_unregister(struct mtd_info *master)
 	if (master->_reboot)
 		unregister_reboot_notifier(&master->reboot_notifier);
 
+	if (master->otp_user_nvmem)
+		nvmem_unregister(master->otp_user_nvmem);
+
+	if (master->otp_factory_nvmem)
+		nvmem_unregister(master->otp_factory_nvmem);
+
 	err = del_mtd_partitions(master);
 	if (err)
 		return err;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index a89955f3cbc8..88227044fc86 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -380,6 +380,8 @@ struct mtd_info {
 	int usecount;
 	struct mtd_debug_info dbg;
 	struct nvmem_device *nvmem;
+	struct nvmem_device *otp_user_nvmem;
+	struct nvmem_device *otp_factory_nvmem;
 
 	/*
 	 * Parent device from the MTD partition point of view.
-- 
2.20.1

Re: [PATCH v2 3/5] dt-bindings: mtd: add OTP bindings

[Rob Herring]

On Sat, 24 Apr 2021 13:06:06 +0200, Michael Walle wrote:
> Flash devices can have one-time-programmable regions. Add a nvmem
> binding so they can be used as a nvmem provider.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> Changes since v1:
>  - drop "mtd-" prefix for the compatible strings
> 
> Changes since RFC:
>  - added missing "$"
>  - dropped first example
>  - use real device compatibles
> 
>    Please note, that this will lead to an error without patch 4/5, which
>    introduces that property for the jedec,spi-nor. Should I keep it
>    seperate or should I squash that patch into this one?
> 
>  .../devicetree/bindings/mtd/mtd.yaml          | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 4/5] dt-bindings: mtd: spi-nor: add otp property

[Rob Herring]

On Sat, 24 Apr 2021 13:06:07 +0200, Michael Walle wrote:
> SPI-NOR flashes may have OTP regions and have a nvmem binding. This
> binding is described in mtd.yaml.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> Changes since v1:
>  - none
> 
> Changes since RFC:
>  - new patch
> 
>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 5/5] mtd: core: add OTP nvmem provider support

[Miquel Raynal]

On Sat, 2021-04-24 at 11:06:08 UTC, Michael Walle wrote:
> Flash OTP regions can already be read via user space. Some boards have
> their serial number or MAC addresses stored in the OTP regions. Add
> support for them being a (read-only) nvmem provider.
> 
> The API to read the OTP data is already in place. It distinguishes
> between factory and user OTP, thus there are up to two different
> providers.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

Re: [PATCH v2 4/5] dt-bindings: mtd: spi-nor: add otp property

[Miquel Raynal]

On Sat, 2021-04-24 at 11:06:07 UTC, Michael Walle wrote:
> SPI-NOR flashes may have OTP regions and have a nvmem binding. This
> binding is described in mtd.yaml.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Rob Herring <robh@kernel.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

Re: [PATCH v2 3/5] dt-bindings: mtd: add OTP bindings

[Miquel Raynal]

On Sat, 2021-04-24 at 11:06:06 UTC, Michael Walle wrote:
> Flash devices can have one-time-programmable regions. Add a nvmem
> binding so they can be used as a nvmem provider.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Rob Herring <robh@kernel.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

Re: [PATCH v2 2/5] dt-bindings: mtd: add YAML schema for the generic MTD bindings

[Miquel Raynal]

On Sat, 2021-04-24 at 11:06:05 UTC, Michael Walle wrote:
> Convert MTD's common.txt to mtd.yaml.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Rob Herring <robh@kernel.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

Re: [PATCH v2 1/5] nvmem: core: allow specifying of_node

[Miquel Raynal]

On Sat, 2021-04-24 at 11:06:04 UTC, Michael Walle wrote:
> Until now, the of_node of the parent device is used. Some devices
> provide more than just the nvmem provider. To avoid name space clashes,
> add a way to allow specifying the nvmem cells in subnodes. Consider the
> following example:
> 
>     flash@0 {
>         compatible = "jedec,spi-nor";
> 
>         partitions {
>             compatible = "fixed-partitions";
>             #address-cells = <1>;
>             #size-cells = <1>;
> 
>             partition@0 {
>                 reg = <0x000000 0x010000>;
>             };
>         };
> 
>         otp {
>             compatible = "user-otp";
>             #address-cells = <1>;
>             #size-cells = <1>;
> 
>             serial-number@0 {
>                 reg = <0x0 0x8>;
>             };
>         };
>     };
> 
> There the nvmem provider might be the MTD partition or the OTP region of
> the flash.
> 
> Add a new config->of_node parameter, which if set, will be used instead
> of the parent's of_node.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Acked-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

Re: [PATCH v2 2/5] dt-bindings: mtd: add YAML schema for the generic MTD bindings

[Rob Herring]

On Mon, May 10, 2021 at 5:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> On Sat, 2021-04-24 at 11:06:05 UTC, Michael Walle wrote:
> > Convert MTD's common.txt to mtd.yaml.
> >
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > Reviewed-by: Rob Herring <robh@kernel.org>
>
> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

This is causing a warning in linux-next:

/builds/robherring/linux-dt/Documentation/devicetree/bindings/spi/spi-mux.example.dt.yaml:
spi-flash@0: $nodename:0: 'spi-flash@0' does not match '^flash(@.*)?$'
 From schema: /builds/robherring/linux-dt/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml

Rob

Re: [PATCH v2 2/5] dt-bindings: mtd: add YAML schema for the generic MTD bindings

[Michael Walle]

Am 2021-05-17 17:12, schrieb Rob Herring:
> On Mon, May 10, 2021 at 5:44 AM Miquel Raynal 
> <miquel.raynal@bootlin.com> wrote:
>> 
>> On Sat, 2021-04-24 at 11:06:05 UTC, Michael Walle wrote:
>> > Convert MTD's common.txt to mtd.yaml.
>> >
>> > Signed-off-by: Michael Walle <michael@walle.cc>
>> > Reviewed-by: Rob Herring <robh@kernel.org>
>> 
>> Applied to 
>> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git 
>> mtd/next, thanks.
> 
> This is causing a warning in linux-next:
> 
> /builds/robherring/linux-dt/Documentation/devicetree/bindings/spi/spi-mux.example.dt.yaml:
> spi-flash@0: $nodename:0: 'spi-flash@0' does not match '^flash(@.*)?$'
>  From schema:
> /builds/robherring/linux-dt/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml

For reference, the patch which fixes this:
https://lore.kernel.org/linux-devicetree/20210517153946.9502-1-michael@walle.cc/

-michael

[PATCH] mtd: core: Fix freeing of otp_info buffer

[Jon Hunter]

Commit 4b361cfa8624 ("mtd: core: add OTP nvmem provider support") is
causing the following panic ...

 ------------[ cut here ]------------
 kernel BUG at /local/workdir/tegra/linux_next/kernel/mm/slab.c:2730!
 Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
 Modules linked in:
 CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.13.0-rc2-next-20210518 #1
 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
 PC is at ___cache_free+0x3f8/0x51c
 ...
 [<c029bb1c>] (___cache_free) from [<c029c658>] (kfree+0xac/0x1bc)
 [<c029c658>] (kfree) from [<c06da094>] (mtd_otp_size+0xc4/0x108)
 [<c06da094>] (mtd_otp_size) from [<c06dc864>] (mtd_device_parse_register+0xe4/0x2b4)
 [<c06dc864>] (mtd_device_parse_register) from [<c06e3ccc>] (spi_nor_probe+0x210/0x2c0)
 [<c06e3ccc>] (spi_nor_probe) from [<c06e9578>] (spi_probe+0x88/0xac)
 [<c06e9578>] (spi_probe) from [<c066891c>] (really_probe+0x214/0x3a4)
 [<c066891c>] (really_probe) from [<c0668b14>] (driver_probe_device+0x68/0xc0)
 [<c0668b14>] (driver_probe_device) from [<c0666cf8>] (bus_for_each_drv+0x5c/0xbc)
 [<c0666cf8>] (bus_for_each_drv) from [<c0668694>] (__device_attach+0xe4/0x150)
 [<c0668694>] (__device_attach) from [<c06679e0>] (bus_probe_device+0x84/0x8c)
 [<c06679e0>] (bus_probe_device) from [<c06657f8>] (device_add+0x48c/0x868)
 [<c06657f8>] (device_add) from [<c06eb784>] (spi_add_device+0xa0/0x168)
 [<c06eb784>] (spi_add_device) from [<c06ec9a8>] (spi_register_controller+0x8b8/0xb38)
 [<c06ec9a8>] (spi_register_controller) from [<c06ecc3c>] (devm_spi_register_controller+0x14/0x50)
 [<c06ecc3c>] (devm_spi_register_controller) from [<c06f0510>] (tegra_spi_probe+0x33c/0x450)
 [<c06f0510>] (tegra_spi_probe) from [<c066abec>] (platform_probe+0x5c/0xb8)
 [<c066abec>] (platform_probe) from [<c066891c>] (really_probe+0x214/0x3a4)
 [<c066891c>] (really_probe) from [<c0668b14>] (driver_probe_device+0x68/0xc0)
 [<c0668b14>] (driver_probe_device) from [<c0668e30>] (device_driver_attach+0x58/0x60)
 [<c0668e30>] (device_driver_attach) from [<c0668eb8>] (__driver_attach+0x80/0xc8)
 [<c0668eb8>] (__driver_attach) from [<c0666c48>] (bus_for_each_dev+0x78/0xb8)
 [<c0666c48>] (bus_for_each_dev) from [<c0667c44>] (bus_add_driver+0x164/0x1e8)
 [<c0667c44>] (bus_add_driver) from [<c066997c>] (driver_register+0x7c/0x114)
 [<c066997c>] (driver_register) from [<c010223c>] (do_one_initcall+0x50/0x2b0)
 [<c010223c>] (do_one_initcall) from [<c11011f0>] (kernel_init_freeable+0x1a8/0x1fc)
 [<c11011f0>] (kernel_init_freeable) from [<c0c09190>] (kernel_init+0x8/0x118)
 [<c0c09190>] (kernel_init) from [<c01001b0>] (ret_from_fork+0x14/0x24)
 ...
 ---[ end trace 0f652dd222de75d7 ]---

In the function mtd_otp_size() a buffer is allocated by calling
kmalloc() and a pointer to the buffer is stored in a variable 'info'.
The pointer 'info' may then be incremented depending on the length
returned from mtd_get_user/fact_prot_info(). If 'info' is incremented,
when kfree() is called to free the buffer the above panic occurs because
we are no longer passing the original address of the buffer allocated.
Fix this by indexing through the buffer allocated to avoid incrementing
the pointer.

Fixes: 4b361cfa8624 ("mtd: core: add OTP nvmem provider support")
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/mtd/mtdcore.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 3ae261661eea..ffa46ccea0cf 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -792,10 +792,8 @@ static ssize_t mtd_otp_size(struct mtd_info *mtd, bool is_user)
 	if (ret)
 		goto err;
 
-	for (i = 0; i < retlen / sizeof(*info); i++) {
-		size += info->length;
-		info++;
-	}
+	for (i = 0; i < retlen / sizeof(*info); i++)
+		size += info[i].length;
 
 	kfree(info);
 	return size;
-- 
2.7.4

Re: [PATCH] mtd: core: Fix freeing of otp_info buffer

[Michael Walle]

Am 2021-05-18 20:55, schrieb Jon Hunter:
> Commit 4b361cfa8624 ("mtd: core: add OTP nvmem provider support") is
> causing the following panic ...
> 
>  ------------[ cut here ]------------
>  kernel BUG at /local/workdir/tegra/linux_next/kernel/mm/slab.c:2730!
>  Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
>  Modules linked in:
>  CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.13.0-rc2-next-20210518 #1
>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>  PC is at ___cache_free+0x3f8/0x51c
>  ...
>  [<c029bb1c>] (___cache_free) from [<c029c658>] (kfree+0xac/0x1bc)
>  [<c029c658>] (kfree) from [<c06da094>] (mtd_otp_size+0xc4/0x108)
>  [<c06da094>] (mtd_otp_size) from [<c06dc864>]
> (mtd_device_parse_register+0xe4/0x2b4)
>  [<c06dc864>] (mtd_device_parse_register) from [<c06e3ccc>]
> (spi_nor_probe+0x210/0x2c0)
>  [<c06e3ccc>] (spi_nor_probe) from [<c06e9578>] (spi_probe+0x88/0xac)
>  [<c06e9578>] (spi_probe) from [<c066891c>] (really_probe+0x214/0x3a4)
>  [<c066891c>] (really_probe) from [<c0668b14>] 
> (driver_probe_device+0x68/0xc0)
>  [<c0668b14>] (driver_probe_device) from [<c0666cf8>]
> (bus_for_each_drv+0x5c/0xbc)
>  [<c0666cf8>] (bus_for_each_drv) from [<c0668694>] 
> (__device_attach+0xe4/0x150)
>  [<c0668694>] (__device_attach) from [<c06679e0>] 
> (bus_probe_device+0x84/0x8c)
>  [<c06679e0>] (bus_probe_device) from [<c06657f8>] 
> (device_add+0x48c/0x868)
>  [<c06657f8>] (device_add) from [<c06eb784>] 
> (spi_add_device+0xa0/0x168)
>  [<c06eb784>] (spi_add_device) from [<c06ec9a8>]
> (spi_register_controller+0x8b8/0xb38)
>  [<c06ec9a8>] (spi_register_controller) from [<c06ecc3c>]
> (devm_spi_register_controller+0x14/0x50)
>  [<c06ecc3c>] (devm_spi_register_controller) from [<c06f0510>]
> (tegra_spi_probe+0x33c/0x450)
>  [<c06f0510>] (tegra_spi_probe) from [<c066abec>] 
> (platform_probe+0x5c/0xb8)
>  [<c066abec>] (platform_probe) from [<c066891c>] 
> (really_probe+0x214/0x3a4)
>  [<c066891c>] (really_probe) from [<c0668b14>] 
> (driver_probe_device+0x68/0xc0)
>  [<c0668b14>] (driver_probe_device) from [<c0668e30>]
> (device_driver_attach+0x58/0x60)
>  [<c0668e30>] (device_driver_attach) from [<c0668eb8>]
> (__driver_attach+0x80/0xc8)
>  [<c0668eb8>] (__driver_attach) from [<c0666c48>] 
> (bus_for_each_dev+0x78/0xb8)
>  [<c0666c48>] (bus_for_each_dev) from [<c0667c44>] 
> (bus_add_driver+0x164/0x1e8)
>  [<c0667c44>] (bus_add_driver) from [<c066997c>] 
> (driver_register+0x7c/0x114)
>  [<c066997c>] (driver_register) from [<c010223c>] 
> (do_one_initcall+0x50/0x2b0)
>  [<c010223c>] (do_one_initcall) from [<c11011f0>]
> (kernel_init_freeable+0x1a8/0x1fc)
>  [<c11011f0>] (kernel_init_freeable) from [<c0c09190>] 
> (kernel_init+0x8/0x118)
>  [<c0c09190>] (kernel_init) from [<c01001b0>] (ret_from_fork+0x14/0x24)
>  ...
>  ---[ end trace 0f652dd222de75d7 ]---
> 
> In the function mtd_otp_size() a buffer is allocated by calling
> kmalloc() and a pointer to the buffer is stored in a variable 'info'.
> The pointer 'info' may then be incremented depending on the length
> returned from mtd_get_user/fact_prot_info(). If 'info' is incremented,
> when kfree() is called to free the buffer the above panic occurs 
> because
> we are no longer passing the original address of the buffer allocated.
> Fix this by indexing through the buffer allocated to avoid incrementing
> the pointer.
> 
> Fixes: 4b361cfa8624 ("mtd: core: add OTP nvmem provider support")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

uhm.. yes of course. Two fixes for this function. Not my best day :/

I'm wondering why CONFIG_SLUB_DEBUG_ON doesn't catch this, whereas
slub_debug=f (or fzpu) as commandline parameter works as expected.

Reviewed-by: Michael Walle <michael@walle.cc>

Thanks,
-michael

Re: [PATCH] mtd: core: Fix freeing of otp_info buffer

[Miquel Raynal]

On Tue, 2021-05-18 at 18:55:03 UTC, Jon Hunter wrote:
> Commit 4b361cfa8624 ("mtd: core: add OTP nvmem provider support") is
> causing the following panic ...
> 
>  ------------[ cut here ]------------
>  kernel BUG at /local/workdir/tegra/linux_next/kernel/mm/slab.c:2730!
>  Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
>  Modules linked in:
>  CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.13.0-rc2-next-20210518 #1
>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>  PC is at ___cache_free+0x3f8/0x51c
>  ...
>  [<c029bb1c>] (___cache_free) from [<c029c658>] (kfree+0xac/0x1bc)
>  [<c029c658>] (kfree) from [<c06da094>] (mtd_otp_size+0xc4/0x108)
>  [<c06da094>] (mtd_otp_size) from [<c06dc864>] (mtd_device_parse_register+0xe4/0x2b4)
>  [<c06dc864>] (mtd_device_parse_register) from [<c06e3ccc>] (spi_nor_probe+0x210/0x2c0)
>  [<c06e3ccc>] (spi_nor_probe) from [<c06e9578>] (spi_probe+0x88/0xac)
>  [<c06e9578>] (spi_probe) from [<c066891c>] (really_probe+0x214/0x3a4)
>  [<c066891c>] (really_probe) from [<c0668b14>] (driver_probe_device+0x68/0xc0)
>  [<c0668b14>] (driver_probe_device) from [<c0666cf8>] (bus_for_each_drv+0x5c/0xbc)
>  [<c0666cf8>] (bus_for_each_drv) from [<c0668694>] (__device_attach+0xe4/0x150)
>  [<c0668694>] (__device_attach) from [<c06679e0>] (bus_probe_device+0x84/0x8c)
>  [<c06679e0>] (bus_probe_device) from [<c06657f8>] (device_add+0x48c/0x868)
>  [<c06657f8>] (device_add) from [<c06eb784>] (spi_add_device+0xa0/0x168)
>  [<c06eb784>] (spi_add_device) from [<c06ec9a8>] (spi_register_controller+0x8b8/0xb38)
>  [<c06ec9a8>] (spi_register_controller) from [<c06ecc3c>] (devm_spi_register_controller+0x14/0x50)
>  [<c06ecc3c>] (devm_spi_register_controller) from [<c06f0510>] (tegra_spi_probe+0x33c/0x450)
>  [<c06f0510>] (tegra_spi_probe) from [<c066abec>] (platform_probe+0x5c/0xb8)
>  [<c066abec>] (platform_probe) from [<c066891c>] (really_probe+0x214/0x3a4)
>  [<c066891c>] (really_probe) from [<c0668b14>] (driver_probe_device+0x68/0xc0)
>  [<c0668b14>] (driver_probe_device) from [<c0668e30>] (device_driver_attach+0x58/0x60)
>  [<c0668e30>] (device_driver_attach) from [<c0668eb8>] (__driver_attach+0x80/0xc8)
>  [<c0668eb8>] (__driver_attach) from [<c0666c48>] (bus_for_each_dev+0x78/0xb8)
>  [<c0666c48>] (bus_for_each_dev) from [<c0667c44>] (bus_add_driver+0x164/0x1e8)
>  [<c0667c44>] (bus_add_driver) from [<c066997c>] (driver_register+0x7c/0x114)
>  [<c066997c>] (driver_register) from [<c010223c>] (do_one_initcall+0x50/0x2b0)
>  [<c010223c>] (do_one_initcall) from [<c11011f0>] (kernel_init_freeable+0x1a8/0x1fc)
>  [<c11011f0>] (kernel_init_freeable) from [<c0c09190>] (kernel_init+0x8/0x118)
>  [<c0c09190>] (kernel_init) from [<c01001b0>] (ret_from_fork+0x14/0x24)
>  ...
>  ---[ end trace 0f652dd222de75d7 ]---
> 
> In the function mtd_otp_size() a buffer is allocated by calling
> kmalloc() and a pointer to the buffer is stored in a variable 'info'.
> The pointer 'info' may then be incremented depending on the length
> returned from mtd_get_user/fact_prot_info(). If 'info' is incremented,
> when kfree() is called to free the buffer the above panic occurs because
> we are no longer passing the original address of the buffer allocated.
> Fix this by indexing through the buffer allocated to avoid incrementing
> the pointer.
> 
> Fixes: 4b361cfa8624 ("mtd: core: add OTP nvmem provider support")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> Reviewed-by: Michael Walle <michael@walle.cc>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

Re: [PATCH v2 5/5] mtd: core: add OTP nvmem provider support

[Guenter Roeck]

Hi,

On Sat, Apr 24, 2021 at 01:06:08PM +0200, Michael Walle wrote:
> Flash OTP regions can already be read via user space. Some boards have
> their serial number or MAC addresses stored in the OTP regions. Add
> support for them being a (read-only) nvmem provider.
> 
> The API to read the OTP data is already in place. It distinguishes
> between factory and user OTP, thus there are up to two different
> providers.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

This patch causes a boot failure with one of my qemu tests.
With the patch in place, the flash fails to instantiate.

[    1.156578] Creating 3 MTD partitions on "physmap-flash":
[    1.157192] 0x000000000000-0x000000040000 : "U-Boot Bootloader"
[    1.184632] 0x000000040000-0x000000060000 : "U-Boot Environment"
[    1.201767] 0x000000060000-0x000000800000 : "Flash"
[    1.222320] Deleting MTD partitions on "physmap-flash":
[    1.222744] Deleting U-Boot Bootloader MTD partition
[    1.303597] Deleting U-Boot Environment MTD partition
[    1.368751] Deleting Flash MTD partition
[    1.430619] physmap-flash: probe of physmap-flash failed with error -61

-61 is -ENODATA.

Other boot tests with different flash chips can still boot.
Reverting this patch (as well as the follow-up patches) fixes
the problem.

I do not know if this is a problem with qemu or a problem with the
patch, but, as I mentioned, other flash chips do still instantiate.

Do you have an idea what to look for when I try to track down the problem ?

Thanks,
Guenter

> ---
> Changes since v1:
>  - combine name and compatible string in mtd_otp_nvmem_register()
> 
> Changes since RFC:
>  - none
> 
>  drivers/mtd/mtdcore.c   | 148 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/mtd.h |   2 +
>  2 files changed, 150 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 9aaeadd53eb4..72e7000a86fd 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -777,6 +777,146 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
>  	mutex_init(&mtd->master.chrdev_lock);
>  }
>  
> +static ssize_t mtd_otp_size(struct mtd_info *mtd, bool is_user)
> +{
> +	struct otp_info *info = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	ssize_t size = 0;
> +	unsigned int i;
> +	size_t retlen;
> +	int ret;
> +
> +	if (is_user)
> +		ret = mtd_get_user_prot_info(mtd, PAGE_SIZE, &retlen, info);
> +	else
> +		ret = mtd_get_fact_prot_info(mtd, PAGE_SIZE, &retlen, info);
> +	if (ret)
> +		goto err;
> +
> +	for (i = 0; i < retlen / sizeof(*info); i++) {
> +		size += info->length;
> +		info++;
> +	}
> +
> +	kfree(info);
> +	return size;
> +
> +err:
> +	kfree(info);
> +	return ret;
> +}
> +
> +static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
> +						   const char *compatible,
> +						   int size,
> +						   nvmem_reg_read_t reg_read)
> +{
> +	struct nvmem_device *nvmem = NULL;
> +	struct nvmem_config config = {};
> +	struct device_node *np;
> +
> +	/* DT binding is optional */
> +	np = of_get_compatible_child(mtd->dev.of_node, compatible);
> +
> +	/* OTP nvmem will be registered on the physical device */
> +	config.dev = mtd->dev.parent;
> +	/* just reuse the compatible as name */
> +	config.name = compatible;
> +	config.id = NVMEM_DEVID_NONE;
> +	config.owner = THIS_MODULE;
> +	config.type = NVMEM_TYPE_OTP;
> +	config.root_only = true;
> +	config.reg_read = reg_read;
> +	config.size = size;
> +	config.of_node = np;
> +	config.priv = mtd;
> +
> +	nvmem = nvmem_register(&config);
> +	/* Just ignore if there is no NVMEM support in the kernel */
> +	if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EOPNOTSUPP)
> +		nvmem = NULL;
> +
> +	of_node_put(np);
> +
> +	return nvmem;
> +}
> +
> +static int mtd_nvmem_user_otp_reg_read(void *priv, unsigned int offset,
> +				       void *val, size_t bytes)
> +{
> +	struct mtd_info *mtd = priv;
> +	size_t retlen;
> +	int ret;
> +
> +	ret = mtd_read_user_prot_reg(mtd, offset, bytes, &retlen, val);
> +	if (ret)
> +		return ret;
> +
> +	return retlen == bytes ? 0 : -EIO;
> +}
> +
> +static int mtd_nvmem_fact_otp_reg_read(void *priv, unsigned int offset,
> +				       void *val, size_t bytes)
> +{
> +	struct mtd_info *mtd = priv;
> +	size_t retlen;
> +	int ret;
> +
> +	ret = mtd_read_fact_prot_reg(mtd, offset, bytes, &retlen, val);
> +	if (ret)
> +		return ret;
> +
> +	return retlen == bytes ? 0 : -EIO;
> +}
> +
> +static int mtd_otp_nvmem_add(struct mtd_info *mtd)
> +{
> +	struct nvmem_device *nvmem;
> +	ssize_t size;
> +	int err;
> +
> +	if (mtd->_get_user_prot_info && mtd->_read_user_prot_reg) {
> +		size = mtd_otp_size(mtd, true);
> +		if (size < 0)
> +			return size;
> +
> +		if (size > 0) {
> +			nvmem = mtd_otp_nvmem_register(mtd, "user-otp", size,
> +						       mtd_nvmem_user_otp_reg_read);
> +			if (IS_ERR(nvmem)) {
> +				dev_err(&mtd->dev, "Failed to register OTP NVMEM device\n");
> +				return PTR_ERR(nvmem);
> +			}
> +			mtd->otp_user_nvmem = nvmem;
> +		}
> +	}
> +
> +	if (mtd->_get_fact_prot_info && mtd->_read_fact_prot_reg) {
> +		size = mtd_otp_size(mtd, false);
> +		if (size < 0) {
> +			err = size;
> +			goto err;
> +		}
> +
> +		if (size > 0) {
> +			nvmem = mtd_otp_nvmem_register(mtd, "factory-otp", size,
> +						       mtd_nvmem_fact_otp_reg_read);
> +			if (IS_ERR(nvmem)) {
> +				dev_err(&mtd->dev, "Failed to register OTP NVMEM device\n");
> +				err = PTR_ERR(nvmem);
> +				goto err;
> +			}
> +			mtd->otp_factory_nvmem = nvmem;
> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	if (mtd->otp_user_nvmem)
> +		nvmem_unregister(mtd->otp_user_nvmem);
> +	return err;
> +}
> +
>  /**
>   * mtd_device_parse_register - parse partitions and register an MTD device.
>   *
> @@ -852,6 +992,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		register_reboot_notifier(&mtd->reboot_notifier);
>  	}
>  
> +	ret = mtd_otp_nvmem_add(mtd);
> +
>  out:
>  	if (ret && device_is_registered(&mtd->dev))
>  		del_mtd_device(mtd);
> @@ -873,6 +1015,12 @@ int mtd_device_unregister(struct mtd_info *master)
>  	if (master->_reboot)
>  		unregister_reboot_notifier(&master->reboot_notifier);
>  
> +	if (master->otp_user_nvmem)
> +		nvmem_unregister(master->otp_user_nvmem);
> +
> +	if (master->otp_factory_nvmem)
> +		nvmem_unregister(master->otp_factory_nvmem);
> +
>  	err = del_mtd_partitions(master);
>  	if (err)
>  		return err;
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index a89955f3cbc8..88227044fc86 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -380,6 +380,8 @@ struct mtd_info {
>  	int usecount;
>  	struct mtd_debug_info dbg;
>  	struct nvmem_device *nvmem;
> +	struct nvmem_device *otp_user_nvmem;
> +	struct nvmem_device *otp_factory_nvmem;
>  
>  	/*
>  	 * Parent device from the MTD partition point of view.

Re: [PATCH v2 5/5] mtd: core: add OTP nvmem provider support

[Michael Walle]

Hi Guenter,

Am 2021-07-01 23:34, schrieb Guenter Roeck:
> Hi,
> 
> On Sat, Apr 24, 2021 at 01:06:08PM +0200, Michael Walle wrote:
>> Flash OTP regions can already be read via user space. Some boards have
>> their serial number or MAC addresses stored in the OTP regions. Add
>> support for them being a (read-only) nvmem provider.
>> 
>> The API to read the OTP data is already in place. It distinguishes
>> between factory and user OTP, thus there are up to two different
>> providers.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> This patch causes a boot failure with one of my qemu tests.
> With the patch in place, the flash fails to instantiate.
> 
> [    1.156578] Creating 3 MTD partitions on "physmap-flash":
> [    1.157192] 0x000000000000-0x000000040000 : "U-Boot Bootloader"
> [    1.184632] 0x000000040000-0x000000060000 : "U-Boot Environment"
> [    1.201767] 0x000000060000-0x000000800000 : "Flash"
> [    1.222320] Deleting MTD partitions on "physmap-flash":
> [    1.222744] Deleting U-Boot Bootloader MTD partition
> [    1.303597] Deleting U-Boot Environment MTD partition
> [    1.368751] Deleting Flash MTD partition
> [    1.430619] physmap-flash: probe of physmap-flash failed with error 
> -61
> 
> -61 is -ENODATA.
> 
> Other boot tests with different flash chips can still boot.
> Reverting this patch (as well as the follow-up patches) fixes
> the problem.
> 
> I do not know if this is a problem with qemu or a problem with the
> patch, but, as I mentioned, other flash chips do still instantiate.
> 
> Do you have an idea what to look for when I try to track down the 
> problem ?

I'd start by looking at the return code of mtd_otp_size() because that
should be the only function which communicates with the flash at probe
time.

Can you share how to reproduce that problem? Like the qemu commandline
and involved images?

-michael

Re: [PATCH v2 5/5] mtd: core: add OTP nvmem provider support

[Guenter Roeck]

On 7/1/21 3:10 PM, Michael Walle wrote:
> Hi Guenter,
> 
> Am 2021-07-01 23:34, schrieb Guenter Roeck:
>> Hi,
>>
>> On Sat, Apr 24, 2021 at 01:06:08PM +0200, Michael Walle wrote:
>>> Flash OTP regions can already be read via user space. Some boards have
>>> their serial number or MAC addresses stored in the OTP regions. Add
>>> support for them being a (read-only) nvmem provider.
>>>
>>> The API to read the OTP data is already in place. It distinguishes
>>> between factory and user OTP, thus there are up to two different
>>> providers.
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>
>> This patch causes a boot failure with one of my qemu tests.
>> With the patch in place, the flash fails to instantiate.
>>
>> [    1.156578] Creating 3 MTD partitions on "physmap-flash":
>> [    1.157192] 0x000000000000-0x000000040000 : "U-Boot Bootloader"
>> [    1.184632] 0x000000040000-0x000000060000 : "U-Boot Environment"
>> [    1.201767] 0x000000060000-0x000000800000 : "Flash"
>> [    1.222320] Deleting MTD partitions on "physmap-flash":
>> [    1.222744] Deleting U-Boot Bootloader MTD partition
>> [    1.303597] Deleting U-Boot Environment MTD partition
>> [    1.368751] Deleting Flash MTD partition
>> [    1.430619] physmap-flash: probe of physmap-flash failed with error -61
>>
>> -61 is -ENODATA.
>>
>> Other boot tests with different flash chips can still boot.
>> Reverting this patch (as well as the follow-up patches) fixes
>> the problem.
>>
>> I do not know if this is a problem with qemu or a problem with the
>> patch, but, as I mentioned, other flash chips do still instantiate.
>>
>> Do you have an idea what to look for when I try to track down the problem ?
> 
> I'd start by looking at the return code of mtd_otp_size() because that
> should be the only function which communicates with the flash at probe
> time.
> 
> Can you share how to reproduce that problem? Like the qemu commandline
> and involved images?
> 

qemu-system-arm -M z2 -kernel arch/arm/boot/zImage -no-reboot \
     -snapshot -drive file=/tmp/flash,format=raw,if=pflash \
     --append "root=/dev/mtdblock2 console=ttyS0" \
     -nographic -monitor null -serial stdio

This is with qemu v6.0 and pxa_defconfig. The actual flash image doesn't
really matter (an empty file with a size of 1024*1024*8 bytes is sufficient).

Debugging shows that -ENODATA is reported by cfi_intelext_otp_walk(),
thanks to:

[    0.737244] #### FeatureSupport: 0x0 NumProtectionFields: 1

which seems to suggest that there are indeed flash chips which don't support
OTP data. With this in mind, is it indeed appropriate to disable support for
all flash chips which don't support OTP data ?

Thanks,
Guenter

Re: [PATCH v2 5/5] mtd: core: add OTP nvmem provider support

[Michael Walle]

Am 2021-07-02 03:55, schrieb Guenter Roeck:
> On 7/1/21 3:10 PM, Michael Walle wrote:
>> Hi Guenter,
>> 
>> Am 2021-07-01 23:34, schrieb Guenter Roeck:
>>> Hi,
>>> 
>>> On Sat, Apr 24, 2021 at 01:06:08PM +0200, Michael Walle wrote:
>>>> Flash OTP regions can already be read via user space. Some boards 
>>>> have
>>>> their serial number or MAC addresses stored in the OTP regions. Add
>>>> support for them being a (read-only) nvmem provider.
>>>> 
>>>> The API to read the OTP data is already in place. It distinguishes
>>>> between factory and user OTP, thus there are up to two different
>>>> providers.
>>>> 
>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> 
>>> This patch causes a boot failure with one of my qemu tests.
>>> With the patch in place, the flash fails to instantiate.
>>> 
>>> [    1.156578] Creating 3 MTD partitions on "physmap-flash":
>>> [    1.157192] 0x000000000000-0x000000040000 : "U-Boot Bootloader"
>>> [    1.184632] 0x000000040000-0x000000060000 : "U-Boot Environment"
>>> [    1.201767] 0x000000060000-0x000000800000 : "Flash"
>>> [    1.222320] Deleting MTD partitions on "physmap-flash":
>>> [    1.222744] Deleting U-Boot Bootloader MTD partition
>>> [    1.303597] Deleting U-Boot Environment MTD partition
>>> [    1.368751] Deleting Flash MTD partition
>>> [    1.430619] physmap-flash: probe of physmap-flash failed with 
>>> error -61
>>> 
>>> -61 is -ENODATA.
>>> 
>>> Other boot tests with different flash chips can still boot.
>>> Reverting this patch (as well as the follow-up patches) fixes
>>> the problem.
>>> 
>>> I do not know if this is a problem with qemu or a problem with the
>>> patch, but, as I mentioned, other flash chips do still instantiate.
>>> 
>>> Do you have an idea what to look for when I try to track down the 
>>> problem ?
>> 
>> I'd start by looking at the return code of mtd_otp_size() because that
>> should be the only function which communicates with the flash at probe
>> time.
>> 
>> Can you share how to reproduce that problem? Like the qemu commandline
>> and involved images?
>> 
> 
> qemu-system-arm -M z2 -kernel arch/arm/boot/zImage -no-reboot \
>     -snapshot -drive file=/tmp/flash,format=raw,if=pflash \
>     --append "root=/dev/mtdblock2 console=ttyS0" \
>     -nographic -monitor null -serial stdio
> 
> This is with qemu v6.0 and pxa_defconfig. The actual flash image 
> doesn't
> really matter (an empty file with a size of 1024*1024*8 bytes is 
> sufficient).

For completeness: with pxa_defconfig, I guess.

> Debugging shows that -ENODATA is reported by cfi_intelext_otp_walk(),
> thanks to:

Thanks for already looking into this.

> 
> [    0.737244] #### FeatureSupport: 0x0 NumProtectionFields: 1
> 
> which seems to suggest that there are indeed flash chips which don't 
> support
> OTP data. With this in mind, is it indeed appropriate to disable 
> support for
> all flash chips which don't support OTP data ?

Yes of course. The SPI NOR drivers doesn't register the callbacks if
there is no OTP support. The others return ENODATA, which I missed.

I'll send a patch shortly.

-michael