2023-08-08 17:45:51

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v3 0/8] mtd: ubi: allow UBI volumes to provide NVMEM

The series is a follow-up and contains all patches of the previous
series "mtd: ubi: behave like a good MTD citizen"[1] which was meant in
preparation for implementing the NVMEM provider.

The goal is to support embedded Linux devices which got NVMEM bits
stored inside a UBI volume. Representing the UBI volume in the Device
Tree, adding a phandle to be referenced by NVMEM consumers allows such
devices to come up with their correct MAC addresses and device-specific
Wi-Fi calibration data loaded.

In order to be available for other drivers, attaching UBI devices has
to be moved from late_initcall (which is too late for other drivers) to
happen earlier. As an alternative to the existing kernel cmdline
parameter the Device Tree property 'compatible = "linux,ubi";' inside
an MTD partition can be used to have that MTD device attached as UBI
device. MTD partitions which serve as UBI devices may have a "volumes"
firmware subnode with volumes which may be compatible with
"nvmem-cells".

In this way, other drivers (think: Ethernet, Wi-Fi) can resolve and
acquire NVMEM bits using the usual device tree phandle, just this time
the NVMEM content is read from a UBI volume.

[1]: https://patchwork.ozlabs.org/project/linux-mtd/list/?series=353177&state=%2A&archive=both

Changes since v2:
* include dt-bindings additions

Changes since v1:
* include patch to fix exiting Kconfig formatting issues
* fix typo and indentation in Kconfig

Daniel Golle (8):
dt-bindings: mtd: add basic bindings for UBI
dt-bindings: mtd: nvmem-cells: allow UBI volumes to provide NVMEM
mtd: ubi: block: don't return on error when removing
mtd: ubi: block: use notifier to create ubiblock from parameter
mtd: ubi: attach MTD partition from device-tree
mtd: ubi: introduce pre-removal notification for UBI volumes
mtd: ubi: populate ubi volume fwnode
mtd: ubi: provide NVMEM layer over UBI volumes

.../bindings/mtd/partitions/linux,ubi.yaml | 66 ++++++
.../bindings/mtd/partitions/nvmem-cells.yaml | 5 +-
.../bindings/mtd/partitions/ubi-volume.yaml | 35 ++++
drivers/mtd/ubi/Kconfig | 12 ++
drivers/mtd/ubi/Makefile | 1 +
drivers/mtd/ubi/block.c | 186 ++++++++++-------
drivers/mtd/ubi/build.c | 160 +++++++++++----
drivers/mtd/ubi/cdev.c | 4 +-
drivers/mtd/ubi/nvmem.c | 189 ++++++++++++++++++
drivers/mtd/ubi/ubi.h | 6 +-
drivers/mtd/ubi/vmt.c | 32 +++
include/linux/mtd/ubi.h | 2 +
12 files changed, 578 insertions(+), 120 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
create mode 100644 Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
create mode 100644 drivers/mtd/ubi/nvmem.c

--
2.41.0


2023-08-08 17:46:39

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v3 1/8] dt-bindings: mtd: add basic bindings for UBI

Add basic bindings for UBI devices and volumes.

Signed-off-by: Daniel Golle <[email protected]>
---
.../bindings/mtd/partitions/linux,ubi.yaml | 65 +++++++++++++++++++
.../bindings/mtd/partitions/ubi-volume.yaml | 35 ++++++++++
2 files changed, 100 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
create mode 100644 Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml

diff --git a/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml b/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
new file mode 100644
index 0000000000000..79cfa0a3eaa7d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mtd/partitions/linux,ubi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Unsorted Block Images
+
+description: |
+ UBI ("Unsorted Block Images") is a volume management system for raw
+ flash devices which manages multiple logical volumes on a single
+ physical flash device and spreads the I/O load (i.e, wear-leveling)
+ across whole flash chip.
+
+maintainers:
+ - Daniel Golle <[email protected]>
+
+allOf:
+ - $ref: partition.yaml#
+
+properties:
+ compatible:
+ const: linux,ubi
+
+ volumes:
+ type: object
+ description: UBI Volumes
+
+ patternProperties:
+ "^ubi-volume-.*$":
+ $ref: "/schemas/mtd/partitions/ubi-volume.yaml"
+
+ unevaluatedProperties: false
+
+required:
+ - compatible
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ partitions {
+ compatible = "fixed-partitions";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ partition@0 {
+ reg = <0x0 0x100000>;
+ label = "bootloader";
+ read-only;
+ };
+
+ partition@100000 {
+ reg = <0x100000 0x1ff00000>;
+ label = "ubi";
+ compatible = "linux,ubi";
+
+ volumes {
+ ubi-volume-caldata {
+ volid = <2>;
+ volname = "rf";
+ };
+ };
+ };
+ };
diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
new file mode 100644
index 0000000000000..c17d0caf07d97
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mtd/partitions/ubi-volume.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: UBI volume
+
+description: |
+ This binding describes a single UBI volume. Volumes can be matches either
+ by their ID or their name, or both.
+
+maintainers:
+ - Daniel Golle <[email protected]>
+
+properties:
+ volid:
+ $ref: "/schemas/types.yaml#/definitions/uint32"
+ description:
+ Match UBI volume ID
+
+ volname:
+ $ref: "/schemas/types.yaml#/definitions/string"
+ description:
+ Match UBI volume ID
+
+anyOf:
+ - required:
+ - volid
+
+ - required:
+ - volname
+
+# This is a generic file other binding inherit from and extend
+additionalProperties: true
--
2.41.0

2023-08-08 20:33:56

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v3 4/8] mtd: ubi: block: use notifier to create ubiblock from parameter

Use UBI_VOLUME_ADDED notification to create ubiblock device specified
on kernel cmdline or module parameter.
This makes thing more simple and has the advantage that ubiblock devices
on volumes which are not present at the time the ubi module is probed
will still be created.

Suggested-by: Zhihao Cheng <[email protected]>
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/mtd/ubi/block.c | 152 ++++++++++++++++++++++------------------
1 file changed, 84 insertions(+), 68 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 69fa6fecb8494..e0618bbde3613 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -33,6 +33,7 @@
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/mutex.h>
+#include <linux/namei.h>
#include <linux/slab.h>
#include <linux/mtd/ubi.h>
#include <linux/blkdev.h>
@@ -65,10 +66,10 @@ struct ubiblock_pdu {
};

/* Numbers of elements set in the @ubiblock_param array */
-static int ubiblock_devs __initdata;
+static int ubiblock_devs;

/* MTD devices specification parameters */
-static struct ubiblock_param ubiblock_param[UBIBLOCK_MAX_DEVICES] __initdata;
+static struct ubiblock_param ubiblock_param[UBIBLOCK_MAX_DEVICES];

struct ubiblock {
struct ubi_volume_desc *desc;
@@ -532,6 +533,85 @@ static int ubiblock_resize(struct ubi_volume_info *vi)
return 0;
}

+static bool
+match_volume_desc(struct ubi_volume_info *vi, const char *name, int ubi_num, int vol_id)
+{
+ int err, len;
+ struct path path;
+ struct kstat stat;
+
+ if (ubi_num == -1) {
+ /* No ubi num, name must be a vol device path */
+ err = kern_path(name, LOOKUP_FOLLOW, &path);
+ if (err)
+ return false;
+
+ err = vfs_getattr(&path, &stat, STATX_TYPE, AT_STATX_SYNC_AS_STAT);
+ path_put(&path);
+ if (err)
+ return false;
+
+ if (!S_ISCHR(stat.mode))
+ return false;
+
+ if (vi->ubi_num != ubi_major2num(MAJOR(stat.rdev)))
+ return false;
+
+ if (vi->vol_id != MINOR(stat.rdev) - 1)
+ return false;
+
+ return true;
+ }
+
+ if (vol_id == -1) {
+ if (vi->ubi_num != ubi_num)
+ return false;
+
+ len = strnlen(name, UBI_VOL_NAME_MAX + 1);
+ if (len < 1 || vi->name_len != len)
+ return false;
+
+ if (strcmp(name, vi->name))
+ return false;
+
+ return true;
+ }
+
+ if (vi->ubi_num != ubi_num)
+ return false;
+
+ if (vi->vol_id != vol_id)
+ return false;
+
+ return true;
+}
+
+static void
+ubiblock_create_from_param(struct ubi_volume_info *vi)
+{
+ int i, ret = 0;
+ struct ubiblock_param *p;
+
+ /*
+ * Iterate over ubiblock cmdline parameters. If a parameter matches the
+ * newly added volume create the ubiblock device for it.
+ */
+ for (i = 0; i < ubiblock_devs; i++) {
+ p = &ubiblock_param[i];
+
+ if (!match_volume_desc(vi, p->name, p->ubi_num, p->vol_id))
+ continue;
+
+ ret = ubiblock_create(vi);
+ if (ret) {
+ pr_err(
+ "UBI: block: can't add '%s' volume on ubi%d_%d, err=%d\n",
+ vi->name, p->ubi_num, p->vol_id, ret);
+ }
+ break;
+ }
+}
+
static int ubiblock_notify(struct notifier_block *nb,
unsigned long notification_type, void *ns_ptr)
{
@@ -539,10 +619,7 @@ static int ubiblock_notify(struct notifier_block *nb,

switch (notification_type) {
case UBI_VOLUME_ADDED:
- /*
- * We want to enforce explicit block device creation for
- * volumes, so when a volume is added we do nothing.
- */
+ ubiblock_create_from_param(&nt->vi);
break;
case UBI_VOLUME_REMOVED:
ubiblock_remove(&nt->vi, true);
@@ -568,56 +645,6 @@ static struct notifier_block ubiblock_notifier = {
.notifier_call = ubiblock_notify,
};

-static struct ubi_volume_desc * __init
-open_volume_desc(const char *name, int ubi_num, int vol_id)
-{
- if (ubi_num == -1)
- /* No ubi num, name must be a vol device path */
- return ubi_open_volume_path(name, UBI_READONLY);
- else if (vol_id == -1)
- /* No vol_id, must be vol_name */
- return ubi_open_volume_nm(ubi_num, name, UBI_READONLY);
- else
- return ubi_open_volume(ubi_num, vol_id, UBI_READONLY);
-}
-
-static void __init ubiblock_create_from_param(void)
-{
- int i, ret = 0;
- struct ubiblock_param *p;
- struct ubi_volume_desc *desc;
- struct ubi_volume_info vi;
-
- /*
- * If there is an error creating one of the ubiblocks, continue on to
- * create the following ubiblocks. This helps in a circumstance where
- * the kernel command-line specifies multiple block devices and some
- * may be broken, but we still want the working ones to come up.
- */
- for (i = 0; i < ubiblock_devs; i++) {
- p = &ubiblock_param[i];
-
- desc = open_volume_desc(p->name, p->ubi_num, p->vol_id);
- if (IS_ERR(desc)) {
- pr_err(
- "UBI: block: can't open volume on ubi%d_%d, err=%ld\n",
- p->ubi_num, p->vol_id, PTR_ERR(desc));
- continue;
- }
-
- ubi_get_volume_info(desc, &vi);
- ubi_close_volume(desc);
-
- ret = ubiblock_create(&vi);
- if (ret) {
- pr_err(
- "UBI: block: can't add '%s' volume on ubi%d_%d, err=%d\n",
- vi.name, p->ubi_num, p->vol_id, ret);
- continue;
- }
- }
-}
-
static void ubiblock_remove_all(void)
{
struct ubiblock *next;
@@ -643,18 +670,7 @@ int __init ubiblock_init(void)
if (ubiblock_major < 0)
return ubiblock_major;

- /*
- * Attach block devices from 'block=' module param.
- * Even if one block device in the param list fails to come up,
- * still allow the module to load and leave any others up.
- */
- ubiblock_create_from_param();
-
- /*
- * Block devices are only created upon user requests, so we ignore
- * existing volumes.
- */
- ret = ubi_register_volume_notifier(&ubiblock_notifier, 1);
+ ret = ubi_register_volume_notifier(&ubiblock_notifier, 0);
if (ret)
goto err_unreg;
return 0;
--
2.41.0

2023-08-08 20:41:46

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v3 1/8] dt-bindings: mtd: add basic bindings for UBI


On Tue, 08 Aug 2023 17:03:19 +0100, Daniel Golle wrote:
> Add basic bindings for UBI devices and volumes.
>
> Signed-off-by: Daniel Golle <[email protected]>
> ---
> .../bindings/mtd/partitions/linux,ubi.yaml | 65 +++++++++++++++++++
> .../bindings/mtd/partitions/ubi-volume.yaml | 35 ++++++++++
> 2 files changed, 100 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
> create mode 100644 Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml:29:5: [warning] wrong indentation: expected 6 but found 4 (indentation)
./Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml:32:5: [warning] wrong indentation: expected 6 but found 4 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/094e68da59bbd9efea1469b122f34f5dcf156f0f.1691510312.git.daniel@makrotopia.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


2023-08-09 23:47:59

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v3 1/8] dt-bindings: mtd: add basic bindings for UBI

On Tue, Aug 08, 2023 at 05:03:19PM +0100, Daniel Golle wrote:
> Add basic bindings for UBI devices and volumes.
>
> Signed-off-by: Daniel Golle <[email protected]>
> ---
> .../bindings/mtd/partitions/linux,ubi.yaml | 65 +++++++++++++++++++
> .../bindings/mtd/partitions/ubi-volume.yaml | 35 ++++++++++
> 2 files changed, 100 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
> create mode 100644 Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml b/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
> new file mode 100644
> index 0000000000000..79cfa0a3eaa7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/partitions/linux,ubi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Unsorted Block Images
> +
> +description: |
> + UBI ("Unsorted Block Images") is a volume management system for raw
> + flash devices which manages multiple logical volumes on a single
> + physical flash device and spreads the I/O load (i.e, wear-leveling)
> + across whole flash chip.
> +
> +maintainers:
> + - Daniel Golle <[email protected]>
> +
> +allOf:
> + - $ref: partition.yaml#
> +
> +properties:
> + compatible:
> + const: linux,ubi
> +
> + volumes:
> + type: object
> + description: UBI Volumes
> +
> + patternProperties:
> + "^ubi-volume-.*$":
> + $ref: "/schemas/mtd/partitions/ubi-volume.yaml"

Don't need quotes.

> +
> + unevaluatedProperties: false
> +
> +required:
> + - compatible
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + partition@0 {
> + reg = <0x0 0x100000>;
> + label = "bootloader";
> + read-only;
> + };
> +
> + partition@100000 {
> + reg = <0x100000 0x1ff00000>;
> + label = "ubi";
> + compatible = "linux,ubi";
> +
> + volumes {
> + ubi-volume-caldata {
> + volid = <2>;
> + volname = "rf";
> + };
> + };
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> new file mode 100644
> index 0000000000000..c17d0caf07d97
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/partitions/ubi-volume.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: UBI volume
> +
> +description: |
> + This binding describes a single UBI volume. Volumes can be matches either
> + by their ID or their name, or both.
> +
> +maintainers:
> + - Daniel Golle <[email protected]>
> +
> +properties:
> + volid:

volume-id or ubi-vol-id.

> + $ref: "/schemas/types.yaml#/definitions/uint32"

Drop quotes.

> + description:
> + Match UBI volume ID
> +
> + volname:
> + $ref: "/schemas/types.yaml#/definitions/string"
> + description:
> + Match UBI volume ID

ID?

> +
> +anyOf:
> + - required:
> + - volid
> +
> + - required:
> + - volname
> +
> +# This is a generic file other binding inherit from and extend
> +additionalProperties: true

Where do we restrict this? You referenced it, but didn't didn't restrict
it there (with unevaluatedProperties).

What other properties would you expect? These nodes ultimately need a
single schema (can be multiple files with $ref's) containing all
possible properties. Otherwise, any random property can be added
unchecked.

Rob