Similar to how MAC addresses and Wi-Fi calibration data would be
stored inside an MTD partition on devices coming with NOR flash, a UBI
volume is used by some vendors in the same way on devices with NAND
flash.
The goal of this series is to support such embedded Linux devices which
got NVMEM bits stored inside a UBI volume.
Representing the UBI volume in the Device Tree and 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 for NVMEM bits 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" subnode with volumes, and volumes may
have an "nvmem-layout" object which will trigger the creation of an
emulated NVMEM device on top of the UBI volume.
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.
This series is a follow-up and contains most patches of the previous
series "mtd: ubi: behave like a good MTD citizen"[1] which was meant in
preparation for implementing the NVMEM provider.
[1]: https://patchwork.ozlabs.org/project/linux-mtd/list/?series=353177&state=%2A&archive=both
Changes since v6:
* dt-bindings fixes got squashed into the wrong patch, fix that and
newly introduced YAML white space issues
Changes since v5:
* fix whitespace problems in dt-schema additions
Changes since v4:
* split ubi_open_volume_path() breaking out reusable parts for
new match_volume_desc() function as suggested by Richard Weinberger.
Doing the same for ubi_open_volume_nm() doesn't work as we are working
on struct ubi_volume_info in match_volume_desc() while ubi_open_volume_nm()
is working on struct ubi_volume. That reduces the common part to a string
comparision and length check which doesn't seem worth breaking out of the
existing function.
* drop patches and changes not strictly needed for NVMEM use-case:
- don't handle ubi detach on MTD removal notification. It was not done
until now and the locking hell I was facing when trying to implement
that is non trivial.
- don't relocate the call to ubiblock device creation to the
notification handler
- change ubiblock only as far as needed to handle creation from cmdline
parameter when a volume is added.
* improve commit messages and comments
Changes since v3:
* dt-bindings fixes as requested
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 (7):
dt-bindings: mtd: add basic bindings for UBI
dt-bindings: mtd: ubi-volume: allow UBI volumes to provide NVMEM
mtd: ubi: block: use notifier to create ubiblock from parameter
mtd: ubi: attach 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 | 75 +++++++
.../bindings/mtd/partitions/ubi-volume.yaml | 40 ++++
drivers/mtd/ubi/Kconfig | 12 ++
drivers/mtd/ubi/Makefile | 1 +
drivers/mtd/ubi/block.c | 136 ++++++-------
drivers/mtd/ubi/build.c | 154 ++++++++++----
drivers/mtd/ubi/kapi.c | 56 ++++--
drivers/mtd/ubi/nvmem.c | 188 ++++++++++++++++++
drivers/mtd/ubi/ubi.h | 3 +
drivers/mtd/ubi/vmt.c | 44 +++-
include/linux/mtd/ubi.h | 2 +
11 files changed, 579 insertions(+), 132 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.43.0
UBI volumes may be used to contain NVMEM bits, typically device MAC
addresses or wireless radio calibration data.
Signed-off-by: Daniel Golle <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../devicetree/bindings/mtd/partitions/linux,ubi.yaml | 10 ++++++++++
.../devicetree/bindings/mtd/partitions/ubi-volume.yaml | 5 +++++
2 files changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml b/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
index 7084a1945b310..27e1ac1f252e4 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/linux,ubi.yaml
@@ -59,6 +59,16 @@ examples:
ubi-volume-caldata {
volid = <2>;
volname = "rf";
+
+ nvmem-layout {
+ compatible = "fixed-layout";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ eeprom@0 {
+ reg = <0x0 0x1000>;
+ };
+ };
};
};
};
diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
index 1e3f04dedc01d..19736b26056b2 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
@@ -24,6 +24,11 @@ properties:
description:
Match UBI volume ID
+ nvmem-layout:
+ $ref: /schemas/nvmem/layouts/nvmem-layout.yaml#
+ description:
+ This container may reference an NVMEM layout parser.
+
anyOf:
- required:
- volid
--
2.43.0
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 | 136 ++++++++++++++++++++--------------------
drivers/mtd/ubi/kapi.c | 54 +++++++++++-----
drivers/mtd/ubi/ubi.h | 1 +
3 files changed, 106 insertions(+), 85 deletions(-)
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 309a42aeaa4cd..a467df5aa130a 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -65,10 +65,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;
@@ -534,6 +534,70 @@ 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, cur_ubi_num, cur_vol_id;
+
+ if (ubi_num == -1) {
+ /* No ubi num, name must be a vol device path */
+ err = ubi_get_num_by_path(name, &cur_ubi_num, &cur_vol_id);
+ if (err || vi->ubi_num != cur_ubi_num || vi->vol_id != cur_vol_id)
+ return false;
+
+ return true;
+ }
+
+ if (vol_id == -1) {
+ /* Got ubi_num, but no vol_id, name must be volume name */
+ 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)
{
@@ -541,10 +605,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);
@@ -570,56 +631,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;
@@ -645,18 +656,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;
diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
index 5db653eacbd45..fbf3a7fe2af79 100644
--- a/drivers/mtd/ubi/kapi.c
+++ b/drivers/mtd/ubi/kapi.c
@@ -279,6 +279,41 @@ struct ubi_volume_desc *ubi_open_volume_nm(int ubi_num, const char *name,
}
EXPORT_SYMBOL_GPL(ubi_open_volume_nm);
+/**
+ * ubi_get_num_by_path - get UBI device and volume number from device path
+ * @pathname: volume character device node path
+ * @ubi_num: pointer to UBI device number to be set
+ * @vol_id: pointer to UBI volume ID to be set
+ *
+ * Returns 0 on success and sets ubi_num and vol_id, returns error otherwise.
+ */
+int ubi_get_num_by_path(const char *pathname, int *ubi_num, int *vol_id)
+{
+ int error;
+ struct path path;
+ struct kstat stat;
+
+ error = kern_path(pathname, LOOKUP_FOLLOW, &path);
+ if (error)
+ return error;
+
+ error = vfs_getattr(&path, &stat, STATX_TYPE, AT_STATX_SYNC_AS_STAT);
+ path_put(&path);
+ if (error)
+ return error;
+
+ if (!S_ISCHR(stat.mode))
+ return -EINVAL;
+
+ *ubi_num = ubi_major2num(MAJOR(stat.rdev));
+ *vol_id = MINOR(stat.rdev) - 1;
+
+ if (*vol_id < 0 || *ubi_num < 0)
+ return -ENODEV;
+
+ return 0;
+}
+
/**
* ubi_open_volume_path - open UBI volume by its character device node path.
* @pathname: volume character device node path
@@ -290,32 +325,17 @@ EXPORT_SYMBOL_GPL(ubi_open_volume_nm);
struct ubi_volume_desc *ubi_open_volume_path(const char *pathname, int mode)
{
int error, ubi_num, vol_id;
- struct path path;
- struct kstat stat;
dbg_gen("open volume %s, mode %d", pathname, mode);
if (!pathname || !*pathname)
return ERR_PTR(-EINVAL);
- error = kern_path(pathname, LOOKUP_FOLLOW, &path);
- if (error)
- return ERR_PTR(error);
-
- error = vfs_getattr(&path, &stat, STATX_TYPE, AT_STATX_SYNC_AS_STAT);
- path_put(&path);
+ error = ubi_get_num_by_path(pathname, &ubi_num, &vol_id);
if (error)
return ERR_PTR(error);
- if (!S_ISCHR(stat.mode))
- return ERR_PTR(-EINVAL);
-
- ubi_num = ubi_major2num(MAJOR(stat.rdev));
- vol_id = MINOR(stat.rdev) - 1;
-
- if (vol_id >= 0 && ubi_num >= 0)
- return ubi_open_volume(ubi_num, vol_id, mode);
- return ERR_PTR(-ENODEV);
+ return ubi_open_volume(ubi_num, vol_id, mode);
}
EXPORT_SYMBOL_GPL(ubi_open_volume_path);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index a5ec566df0d74..17b463804c979 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -962,6 +962,7 @@ void ubi_free_internal_volumes(struct ubi_device *ubi);
void ubi_do_get_device_info(struct ubi_device *ubi, struct ubi_device_info *di);
void ubi_do_get_volume_info(struct ubi_device *ubi, struct ubi_volume *vol,
struct ubi_volume_info *vi);
+int ubi_get_num_by_path(const char *pathname, int *ubi_num, int *vol_id);
/* scan.c */
int ubi_compare_lebs(struct ubi_device *ubi, const struct ubi_ainf_peb *aeb,
int pnum, const struct ubi_vid_hdr *vid_hdr);
--
2.43.0
Introduce device tree compatible 'linux,ubi' and attach compatible MTD
devices using the MTD add notifier. This is needed for a UBI device to
be available early at boot (and not only after late_initcall), so
volumes on them can be used eg. as NVMEM providers for other drivers.
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/mtd/ubi/build.c | 135 ++++++++++++++++++++++++++++------------
1 file changed, 96 insertions(+), 39 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 7d4ff1193db6f..8c3f763e4ddb8 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -27,6 +27,7 @@
#include <linux/log2.h>
#include <linux/kthread.h>
#include <linux/kernel.h>
+#include <linux/of.h>
#include <linux/slab.h>
#include <linux/major.h>
#include "ubi.h"
@@ -1219,43 +1220,43 @@ static struct mtd_info * __init open_mtd_device(const char *mtd_dev)
return mtd;
}
-static int __init ubi_init(void)
+static void ubi_notify_add(struct mtd_info *mtd)
{
- int err, i, k;
+ struct device_node *np = mtd_get_of_node(mtd);
+ int err;
- /* Ensure that EC and VID headers have correct size */
- BUILD_BUG_ON(sizeof(struct ubi_ec_hdr) != 64);
- BUILD_BUG_ON(sizeof(struct ubi_vid_hdr) != 64);
+ if (!of_device_is_compatible(np, "linux,ubi"))
+ return;
- if (mtd_devs > UBI_MAX_DEVICES) {
- pr_err("UBI error: too many MTD devices, maximum is %d\n",
- UBI_MAX_DEVICES);
- return -EINVAL;
- }
+ /*
+ * we are already holding &mtd_table_mutex, but still need
+ * to bump refcount
+ */
+ err = __get_mtd_device(mtd);
+ if (err)
+ return;
- /* Create base sysfs directory and sysfs files */
- err = class_register(&ubi_class);
+ /* called while holding mtd_table_mutex */
+ mutex_lock_nested(&ubi_devices_mutex, SINGLE_DEPTH_NESTING);
+ err = ubi_attach_mtd_dev(mtd, UBI_DEV_NUM_AUTO, 0, 0, false, false);
+ mutex_unlock(&ubi_devices_mutex);
if (err < 0)
- return err;
-
- err = misc_register(&ubi_ctrl_cdev);
- if (err) {
- pr_err("UBI error: cannot register device\n");
- goto out;
- }
+ __put_mtd_device(mtd);
+}
- ubi_wl_entry_slab = kmem_cache_create("ubi_wl_entry_slab",
- sizeof(struct ubi_wl_entry),
- 0, 0, NULL);
- if (!ubi_wl_entry_slab) {
- err = -ENOMEM;
- goto out_dev_unreg;
- }
+static void ubi_notify_remove(struct mtd_info *mtd)
+{
+ /* do nothing for now */
+}
- err = ubi_debugfs_init();
- if (err)
- goto out_slab;
+static struct mtd_notifier ubi_mtd_notifier = {
+ .add = ubi_notify_add,
+ .remove = ubi_notify_remove,
+};
+static int __init ubi_init_attach(void)
+{
+ int err, i, k;
/* Attach MTD devices */
for (i = 0; i < mtd_devs; i++) {
@@ -1304,25 +1305,79 @@ static int __init ubi_init(void)
}
}
+ return 0;
+
+out_detach:
+ for (k = 0; k < i; k++)
+ if (ubi_devices[k]) {
+ mutex_lock(&ubi_devices_mutex);
+ ubi_detach_mtd_dev(ubi_devices[k]->ubi_num, 1);
+ mutex_unlock(&ubi_devices_mutex);
+ }
+ return err;
+}
+#ifndef CONFIG_MTD_UBI_MODULE
+late_initcall(ubi_init_attach);
+#endif
+
+static int __init ubi_init(void)
+{
+ int err;
+
+ /* Ensure that EC and VID headers have correct size */
+ BUILD_BUG_ON(sizeof(struct ubi_ec_hdr) != 64);
+ BUILD_BUG_ON(sizeof(struct ubi_vid_hdr) != 64);
+
+ if (mtd_devs > UBI_MAX_DEVICES) {
+ pr_err("UBI error: too many MTD devices, maximum is %d\n",
+ UBI_MAX_DEVICES);
+ return -EINVAL;
+ }
+
+ /* Create base sysfs directory and sysfs files */
+ err = class_register(&ubi_class);
+ if (err < 0)
+ return err;
+
+ err = misc_register(&ubi_ctrl_cdev);
+ if (err) {
+ pr_err("UBI error: cannot register device\n");
+ goto out;
+ }
+
+ ubi_wl_entry_slab = kmem_cache_create("ubi_wl_entry_slab",
+ sizeof(struct ubi_wl_entry),
+ 0, 0, NULL);
+ if (!ubi_wl_entry_slab) {
+ err = -ENOMEM;
+ goto out_dev_unreg;
+ }
+
+ err = ubi_debugfs_init();
+ if (err)
+ goto out_slab;
+
err = ubiblock_init();
if (err) {
pr_err("UBI error: block: cannot initialize, error %d\n", err);
/* See comment above re-ubi_is_module(). */
if (ubi_is_module())
- goto out_detach;
+ goto out_slab;
+ }
+
+ register_mtd_user(&ubi_mtd_notifier);
+
+ if (ubi_is_module()) {
+ err = ubi_init_attach();
+ if (err)
+ goto out_mtd_notifier;
}
return 0;
-out_detach:
- for (k = 0; k < i; k++)
- if (ubi_devices[k]) {
- mutex_lock(&ubi_devices_mutex);
- ubi_detach_mtd_dev(ubi_devices[k]->ubi_num, 1);
- mutex_unlock(&ubi_devices_mutex);
- }
- ubi_debugfs_exit();
+out_mtd_notifier:
+ unregister_mtd_user(&ubi_mtd_notifier);
out_slab:
kmem_cache_destroy(ubi_wl_entry_slab);
out_dev_unreg:
@@ -1332,13 +1387,15 @@ static int __init ubi_init(void)
pr_err("UBI error: cannot initialize UBI, error %d\n", err);
return err;
}
-late_initcall(ubi_init);
+device_initcall(ubi_init);
+
static void __exit ubi_exit(void)
{
int i;
ubiblock_exit();
+ unregister_mtd_user(&ubi_mtd_notifier);
for (i = 0; i < UBI_MAX_DEVICES; i++)
if (ubi_devices[i]) {
--
2.43.0
Introduce a new notification type UBI_VOLUME_SHUTDOWN to inform users
that a volume is just about to be removed.
This is needed because users (such as the NVMEM subsystem) expect that
at the time their removal function is called, the parenting device is
still available (for removal of sysfs nodes, for example, in case of
NVMEM which otherwise WARNs on volume removal).
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/mtd/ubi/build.c | 19 ++++++++++++++-----
drivers/mtd/ubi/kapi.c | 2 +-
drivers/mtd/ubi/ubi.h | 2 ++
drivers/mtd/ubi/vmt.c | 17 +++++++++++++++--
include/linux/mtd/ubi.h | 2 ++
5 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 8c3f763e4ddb8..a7e3a6246c0e9 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -93,7 +93,7 @@ static struct ubi_device *ubi_devices[UBI_MAX_DEVICES];
/* Serializes UBI devices creations and removals */
DEFINE_MUTEX(ubi_devices_mutex);
-/* Protects @ubi_devices and @ubi->ref_count */
+/* Protects @ubi_devices, @ubi->ref_count and @ubi->is_dead */
static DEFINE_SPINLOCK(ubi_devices_lock);
/* "Show" method for files in '/<sysfs>/class/ubi/' */
@@ -261,6 +261,9 @@ struct ubi_device *ubi_get_device(int ubi_num)
spin_lock(&ubi_devices_lock);
ubi = ubi_devices[ubi_num];
+ if (ubi && ubi->is_dead)
+ ubi = NULL;
+
if (ubi) {
ubi_assert(ubi->ref_count >= 0);
ubi->ref_count += 1;
@@ -298,7 +301,7 @@ struct ubi_device *ubi_get_by_major(int major)
spin_lock(&ubi_devices_lock);
for (i = 0; i < UBI_MAX_DEVICES; i++) {
ubi = ubi_devices[i];
- if (ubi && MAJOR(ubi->cdev.dev) == major) {
+ if (ubi && !ubi->is_dead && MAJOR(ubi->cdev.dev) == major) {
ubi_assert(ubi->ref_count >= 0);
ubi->ref_count += 1;
get_device(&ubi->dev);
@@ -327,7 +330,7 @@ int ubi_major2num(int major)
for (i = 0; i < UBI_MAX_DEVICES; i++) {
struct ubi_device *ubi = ubi_devices[i];
- if (ubi && MAJOR(ubi->cdev.dev) == major) {
+ if (ubi && !ubi->is_dead && MAJOR(ubi->cdev.dev) == major) {
ubi_num = ubi->ubi_num;
break;
}
@@ -514,7 +517,7 @@ static void ubi_free_volumes_from(struct ubi_device *ubi, int from)
int i;
for (i = from; i < ubi->vtbl_slots + UBI_INT_VOL_COUNT; i++) {
- if (!ubi->volumes[i])
+ if (!ubi->volumes[i] || ubi->volumes[i]->is_dead)
continue;
ubi_eba_replace_table(ubi->volumes[i], NULL);
ubi_fastmap_destroy_checkmap(ubi->volumes[i]);
@@ -1099,7 +1102,6 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
return -EINVAL;
spin_lock(&ubi_devices_lock);
- put_device(&ubi->dev);
ubi->ref_count -= 1;
if (ubi->ref_count) {
if (!anyway) {
@@ -1110,6 +1112,13 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
ubi_err(ubi, "%s reference count %d, destroy anyway",
ubi->ubi_name, ubi->ref_count);
}
+ ubi->is_dead = true;
+ spin_unlock(&ubi_devices_lock);
+
+ ubi_notify_all(ubi, UBI_VOLUME_SHUTDOWN, NULL);
+
+ spin_lock(&ubi_devices_lock);
+ put_device(&ubi->dev);
ubi_devices[ubi_num] = NULL;
spin_unlock(&ubi_devices_lock);
diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
index fbf3a7fe2af79..f1ea8677467fb 100644
--- a/drivers/mtd/ubi/kapi.c
+++ b/drivers/mtd/ubi/kapi.c
@@ -152,7 +152,7 @@ struct ubi_volume_desc *ubi_open_volume(int ubi_num, int vol_id, int mode)
spin_lock(&ubi->volumes_lock);
vol = ubi->volumes[vol_id];
- if (!vol)
+ if (!vol || vol->is_dead)
goto out_unlock;
err = -EBUSY;
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 17b463804c979..56a74a3e2d9ab 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -348,6 +348,7 @@ struct ubi_volume {
int writers;
int exclusive;
int metaonly;
+ bool is_dead;
int reserved_pebs;
int vol_type;
@@ -568,6 +569,7 @@ struct ubi_device {
spinlock_t volumes_lock;
int ref_count;
int image_seq;
+ bool is_dead;
int rsvd_pebs;
int avail_pebs;
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 2c867d16f89f7..ee80c04d1d5d0 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -59,7 +59,7 @@ static ssize_t vol_attribute_show(struct device *dev,
struct ubi_device *ubi = vol->ubi;
spin_lock(&ubi->volumes_lock);
- if (!ubi->volumes[vol->vol_id]) {
+ if (!ubi->volumes[vol->vol_id] || ubi->volumes[vol->vol_id]->is_dead) {
spin_unlock(&ubi->volumes_lock);
return -ENODEV;
}
@@ -189,7 +189,7 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req)
/* Ensure that the name is unique */
for (i = 0; i < ubi->vtbl_slots; i++)
- if (ubi->volumes[i] &&
+ if (ubi->volumes[i] && !ubi->volumes[i]->is_dead &&
ubi->volumes[i]->name_len == req->name_len &&
!strcmp(ubi->volumes[i]->name, req->name)) {
ubi_err(ubi, "volume \"%s\" exists (ID %d)",
@@ -352,6 +352,19 @@ int ubi_remove_volume(struct ubi_volume_desc *desc, int no_vtbl)
err = -EBUSY;
goto out_unlock;
}
+
+ /*
+ * Mark volume as dead at this point to prevent that anyone
+ * can take a reference to the volume from now on.
+ * This is necessary as we have to release the spinlock before
+ * calling ubi_volume_notify.
+ */
+ vol->is_dead = true;
+ spin_unlock(&ubi->volumes_lock);
+
+ ubi_volume_notify(ubi, vol, UBI_VOLUME_SHUTDOWN);
+
+ spin_lock(&ubi->volumes_lock);
ubi->volumes[vol_id] = NULL;
spin_unlock(&ubi->volumes_lock);
diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
index a529347fd75b2..562f92504f2b7 100644
--- a/include/linux/mtd/ubi.h
+++ b/include/linux/mtd/ubi.h
@@ -192,6 +192,7 @@ struct ubi_device_info {
* or a volume was removed)
* @UBI_VOLUME_RESIZED: a volume has been re-sized
* @UBI_VOLUME_RENAMED: a volume has been re-named
+ * @UBI_VOLUME_SHUTDOWN: a volume is going to removed, shutdown users
* @UBI_VOLUME_UPDATED: data has been written to a volume
*
* These constants define which type of event has happened when a volume
@@ -202,6 +203,7 @@ enum {
UBI_VOLUME_REMOVED,
UBI_VOLUME_RESIZED,
UBI_VOLUME_RENAMED,
+ UBI_VOLUME_SHUTDOWN,
UBI_VOLUME_UPDATED,
};
--
2.43.0
Look for the 'volumes' subnode of an MTD partition attached to a UBI
device and attach matching child nodes to UBI volumes.
This allows UBI volumes to be referenced in device tree, e.g. for use
as NVMEM providers.
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/mtd/ubi/vmt.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index ee80c04d1d5d0..7824ff069bd62 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -124,6 +124,31 @@ static void vol_release(struct device *dev)
kfree(vol);
}
+static struct fwnode_handle *find_volume_fwnode(struct ubi_volume *vol)
+{
+ struct fwnode_handle *fw_vols, *fw_vol;
+ const char *volname;
+ u32 volid;
+
+ fw_vols = device_get_named_child_node(vol->dev.parent->parent, "volumes");
+ if (!fw_vols)
+ return NULL;
+
+ fwnode_for_each_child_node(fw_vols, fw_vol) {
+ if (!fwnode_property_read_string(fw_vol, "volname", &volname) &&
+ strncmp(volname, vol->name, vol->name_len))
+ continue;
+
+ if (!fwnode_property_read_u32(fw_vol, "volid", &volid) &&
+ vol->vol_id != volid)
+ continue;
+
+ return fw_vol;
+ }
+
+ return NULL;
+}
+
/**
* ubi_create_volume - create volume.
* @ubi: UBI device description object
@@ -223,6 +248,7 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req)
vol->name_len = req->name_len;
memcpy(vol->name, req->name, vol->name_len);
vol->ubi = ubi;
+ device_set_node(&vol->dev, find_volume_fwnode(vol));
/*
* Finish all pending erases because there may be some LEBs belonging
@@ -605,6 +631,7 @@ int ubi_add_volume(struct ubi_device *ubi, struct ubi_volume *vol)
vol->dev.class = &ubi_class;
vol->dev.groups = volume_dev_groups;
dev_set_name(&vol->dev, "%s_%d", ubi->ubi_name, vol->vol_id);
+ device_set_node(&vol->dev, find_volume_fwnode(vol));
err = device_register(&vol->dev);
if (err) {
cdev_del(&vol->cdev);
--
2.43.0
In an ideal world we would like UBI to be used where ever possible on a
NAND chip. And with UBI support in ARM Trusted Firmware and U-Boot it
is possible to achieve an (almost-)all-UBI flash layout. Hence the need
for a way to also use UBI volumes to store board-level constants, such
as MAC addresses and calibration data of wireless interfaces.
Add UBI volume NVMEM driver module exposing UBI volumes as NVMEM
providers. Allow UBI devices to have a "volumes" firmware subnode with
volumes which may be compatible with "nvmem-cells".
Access to UBI volumes via the NVMEM interface at this point is
read-only, and it is slow, opening and closing the UBI volume for each
access due to limitations of the NVMEM provider API.
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/mtd/ubi/Kconfig | 12 +++
drivers/mtd/ubi/Makefile | 1 +
drivers/mtd/ubi/nvmem.c | 188 +++++++++++++++++++++++++++++++++++++++
3 files changed, 201 insertions(+)
create mode 100644 drivers/mtd/ubi/nvmem.c
diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig
index 2ed77b7b3fcb5..45d939bbfa853 100644
--- a/drivers/mtd/ubi/Kconfig
+++ b/drivers/mtd/ubi/Kconfig
@@ -104,4 +104,16 @@ config MTD_UBI_BLOCK
If in doubt, say "N".
+config MTD_UBI_NVMEM
+ tristate "UBI virtual NVMEM"
+ default n
+ depends on NVMEM
+ help
+ This option enabled an additional driver exposing UBI volumes as NVMEM
+ providers, intended for platforms where UBI is part of the firmware
+ specification and used to store also e.g. MAC addresses or board-
+ specific Wi-Fi calibration data.
+
+ If in doubt, say "N".
+
endif # MTD_UBI
diff --git a/drivers/mtd/ubi/Makefile b/drivers/mtd/ubi/Makefile
index 543673605ca72..4b51aaf00d1a2 100644
--- a/drivers/mtd/ubi/Makefile
+++ b/drivers/mtd/ubi/Makefile
@@ -7,3 +7,4 @@ ubi-$(CONFIG_MTD_UBI_FASTMAP) += fastmap.o
ubi-$(CONFIG_MTD_UBI_BLOCK) += block.o
obj-$(CONFIG_MTD_UBI_GLUEBI) += gluebi.o
+obj-$(CONFIG_MTD_UBI_NVMEM) += nvmem.o
diff --git a/drivers/mtd/ubi/nvmem.c b/drivers/mtd/ubi/nvmem.c
new file mode 100644
index 0000000000000..b7a93c495d172
--- /dev/null
+++ b/drivers/mtd/ubi/nvmem.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Daniel Golle <[email protected]>
+ */
+
+/* UBI NVMEM provider */
+#include "ubi.h"
+#include <linux/nvmem-provider.h>
+#include <asm/div64.h>
+
+/* List of all NVMEM devices */
+static LIST_HEAD(nvmem_devices);
+static DEFINE_MUTEX(devices_mutex);
+
+struct ubi_nvmem {
+ struct nvmem_device *nvmem;
+ int ubi_num;
+ int vol_id;
+ int usable_leb_size;
+ struct list_head list;
+};
+
+static int ubi_nvmem_reg_read(void *priv, unsigned int from,
+ void *val, size_t bytes)
+{
+ int err = 0, lnum = from, offs, bytes_left = bytes, to_read;
+ struct ubi_nvmem *unv = priv;
+ struct ubi_volume_desc *desc;
+
+ desc = ubi_open_volume(unv->ubi_num, unv->vol_id, UBI_READONLY);
+ if (IS_ERR(desc))
+ return PTR_ERR(desc);
+
+ offs = do_div(lnum, unv->usable_leb_size);
+ while (bytes_left) {
+ to_read = unv->usable_leb_size - offs;
+
+ if (to_read > bytes_left)
+ to_read = bytes_left;
+
+ err = ubi_read(desc, lnum, val, offs, to_read);
+ if (err)
+ break;
+
+ lnum += 1;
+ offs = 0;
+ bytes_left -= to_read;
+ val += to_read;
+ }
+ ubi_close_volume(desc);
+
+ if (err)
+ return err;
+
+ return bytes_left == 0 ? 0 : -EIO;
+}
+
+static int ubi_nvmem_add(struct ubi_volume_info *vi)
+{
+ struct device_node *np = dev_of_node(vi->dev);
+ struct nvmem_config config = {};
+ struct ubi_nvmem *unv;
+ int ret;
+
+ if (!np)
+ return 0;
+
+ if (!of_get_child_by_name(np, "nvmem-layout"))
+ return 0;
+
+ if (WARN_ON_ONCE(vi->usable_leb_size <= 0) ||
+ WARN_ON_ONCE(vi->size <= 0))
+ return -EINVAL;
+
+ unv = kzalloc(sizeof(struct ubi_nvmem), GFP_KERNEL);
+ if (!unv)
+ return -ENOMEM;
+
+ config.id = NVMEM_DEVID_NONE;
+ config.dev = vi->dev;
+ config.name = dev_name(vi->dev);
+ config.owner = THIS_MODULE;
+ config.priv = unv;
+ config.reg_read = ubi_nvmem_reg_read;
+ config.size = vi->usable_leb_size * vi->size;
+ config.word_size = 1;
+ config.stride = 1;
+ config.read_only = true;
+ config.root_only = true;
+ config.ignore_wp = true;
+ config.of_node = np;
+
+ unv->ubi_num = vi->ubi_num;
+ unv->vol_id = vi->vol_id;
+ unv->usable_leb_size = vi->usable_leb_size;
+ unv->nvmem = nvmem_register(&config);
+ if (IS_ERR(unv->nvmem)) {
+ ret = dev_err_probe(vi->dev, PTR_ERR(unv->nvmem),
+ "Failed to register NVMEM device\n");
+ kfree(unv);
+ return ret;
+ }
+
+ mutex_lock(&devices_mutex);
+ list_add_tail(&unv->list, &nvmem_devices);
+ mutex_unlock(&devices_mutex);
+
+ return 0;
+}
+
+static void ubi_nvmem_remove(struct ubi_volume_info *vi)
+{
+ struct ubi_nvmem *unv_c, *unv = NULL;
+
+ mutex_lock(&devices_mutex);
+ list_for_each_entry(unv_c, &nvmem_devices, list)
+ if (unv_c->ubi_num == vi->ubi_num && unv_c->vol_id == vi->vol_id) {
+ unv = unv_c;
+ break;
+ }
+
+ if (!unv) {
+ mutex_unlock(&devices_mutex);
+ return;
+ }
+
+ list_del(&unv->list);
+ mutex_unlock(&devices_mutex);
+ nvmem_unregister(unv->nvmem);
+ kfree(unv);
+}
+
+/**
+ * nvmem_notify - UBI notification handler.
+ * @nb: registered notifier block
+ * @l: notification type
+ * @ns_ptr: pointer to the &struct ubi_notification object
+ */
+static int nvmem_notify(struct notifier_block *nb, unsigned long l,
+ void *ns_ptr)
+{
+ struct ubi_notification *nt = ns_ptr;
+
+ switch (l) {
+ case UBI_VOLUME_RESIZED:
+ ubi_nvmem_remove(&nt->vi);
+ fallthrough;
+ case UBI_VOLUME_ADDED:
+ ubi_nvmem_add(&nt->vi);
+ break;
+ case UBI_VOLUME_SHUTDOWN:
+ ubi_nvmem_remove(&nt->vi);
+ break;
+ default:
+ break;
+ }
+ return NOTIFY_OK;
+}
+
+static struct notifier_block nvmem_notifier = {
+ .notifier_call = nvmem_notify,
+};
+
+static int __init ubi_nvmem_init(void)
+{
+ return ubi_register_volume_notifier(&nvmem_notifier, 0);
+}
+
+static void __exit ubi_nvmem_exit(void)
+{
+ struct ubi_nvmem *unv, *tmp;
+
+ mutex_lock(&devices_mutex);
+ list_for_each_entry_safe(unv, tmp, &nvmem_devices, list) {
+ nvmem_unregister(unv->nvmem);
+ list_del(&unv->list);
+ kfree(unv);
+ }
+ mutex_unlock(&devices_mutex);
+
+ ubi_unregister_volume_notifier(&nvmem_notifier);
+}
+
+module_init(ubi_nvmem_init);
+module_exit(ubi_nvmem_exit);
+MODULE_DESCRIPTION("NVMEM layer over UBI volumes");
+MODULE_AUTHOR("Daniel Golle");
+MODULE_LICENSE("GPL");
--
2.43.0
Dear MTD and UBI maintainers,
On Tue, Dec 19, 2023 at 02:31:44AM +0000, Daniel Golle wrote:
> Similar to how MAC addresses and Wi-Fi calibration data would be
> stored inside an MTD partition on devices coming with NOR flash, a UBI
> volume is used by some vendors in the same way on devices with NAND
> flash.
>
> The goal of this series is to support such embedded Linux devices which
> got NVMEM bits stored inside a UBI volume.
>
> Representing the UBI volume in the Device Tree and 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.
>
I hope I don't nag too much but please someone review (and ideally
merge) this very series, also found on Patchwork:
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=387243
In case any part needs further fixing, it would be great to even only
have the dt-bindings part merged already so we can start using this
feature in OpenWrt knowing that dt-bindings will not change any more.
(instead of piling up device-specific fixup-scripts in userspace...)
A good example of why this is needed in the wild is the ASUS RT-AC58U
router which is the first device we found having almost everything
stored in UBI, including NVMEM-areas for WiFi calibration as well as
MAC addresses for both WiFi and Ethernet.
https://openwrt.org/toh/asus/rt-ac58u#flash_layout
Even more devices with all-UBI layout are likely to show up in the
near future as by now even ARM TrustedFirmware-A which is used as
first-stage loader (like MBR on PC or U-Boot SPL on ARMv7) on Aarch64
platforms by now supports UBI to load further bootloader stages. MTD
layout on such devices becomes simply something like
0x0 ~ 0x200000 : ARM TrustedFirmware-A bl2 (redundant if supported
by SoC's BootROM)
0x200000 ~ END : UBI
See this (pending) commit adding UBI support to TF-A:
https://github.com/mtk-openwrt/arm-trusted-firmware/commit/e7a7b94373cf2b8baca66b3b0ced8e70bdbb273a
Obviously this is better than what vendors have been doing previously
(using UBI only for the rootfs, if at all) and having proper support
for that in vanilla Linux would further encourage this development.
PS: Also the planned OpenWrt One router to celebrate the 20th
anniversary of OpenWrt will use an all-UBI layout and we will hence
need to load NVMEM-bits from UBI volumes...
Thank you!
Best regards
Daniel
> In order for NVMEM bits 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" subnode with volumes, and volumes may
> have an "nvmem-layout" object which will trigger the creation of an
> emulated NVMEM device on top of the UBI volume.
>
> 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.
>
> This series is a follow-up and contains most patches of the previous
> series "mtd: ubi: behave like a good MTD citizen"[1] which was meant in
> preparation for implementing the NVMEM provider.
>
> [1]: https://patchwork.ozlabs.org/project/linux-mtd/list/?series=353177&state=%2A&archive=both
>
> Changes since v6:
> * dt-bindings fixes got squashed into the wrong patch, fix that and
> newly introduced YAML white space issues
>
> Changes since v5:
> * fix whitespace problems in dt-schema additions
>
> Changes since v4:
> * split ubi_open_volume_path() breaking out reusable parts for
> new match_volume_desc() function as suggested by Richard Weinberger.
> Doing the same for ubi_open_volume_nm() doesn't work as we are working
> on struct ubi_volume_info in match_volume_desc() while ubi_open_volume_nm()
> is working on struct ubi_volume. That reduces the common part to a string
> comparision and length check which doesn't seem worth breaking out of the
> existing function.
> * drop patches and changes not strictly needed for NVMEM use-case:
> - don't handle ubi detach on MTD removal notification. It was not done
> until now and the locking hell I was facing when trying to implement
> that is non trivial.
> - don't relocate the call to ubiblock device creation to the
> notification handler
> - change ubiblock only as far as needed to handle creation from cmdline
> parameter when a volume is added.
> * improve commit messages and comments
>
> Changes since v3:
> * dt-bindings fixes as requested
>
> 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 (7):
> dt-bindings: mtd: add basic bindings for UBI
> dt-bindings: mtd: ubi-volume: allow UBI volumes to provide NVMEM
> mtd: ubi: block: use notifier to create ubiblock from parameter
> mtd: ubi: attach 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 | 75 +++++++
> .../bindings/mtd/partitions/ubi-volume.yaml | 40 ++++
> drivers/mtd/ubi/Kconfig | 12 ++
> drivers/mtd/ubi/Makefile | 1 +
> drivers/mtd/ubi/block.c | 136 ++++++-------
> drivers/mtd/ubi/build.c | 154 ++++++++++----
> drivers/mtd/ubi/kapi.c | 56 ++++--
> drivers/mtd/ubi/nvmem.c | 188 ++++++++++++++++++
> drivers/mtd/ubi/ubi.h | 3 +
> drivers/mtd/ubi/vmt.c | 44 +++-
> include/linux/mtd/ubi.h | 2 +
> 11 files changed, 579 insertions(+), 132 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.43.0
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Daniel,
[email protected] wrote on Tue, 19 Dec 2023 02:33:48 +0000:
> In an ideal world we would like UBI to be used where ever possible on a
> NAND chip. And with UBI support in ARM Trusted Firmware and U-Boot it
> is possible to achieve an (almost-)all-UBI flash layout. Hence the need
> for a way to also use UBI volumes to store board-level constants, such
> as MAC addresses and calibration data of wireless interfaces.
>
> Add UBI volume NVMEM driver module exposing UBI volumes as NVMEM
> providers. Allow UBI devices to have a "volumes" firmware subnode with
> volumes which may be compatible with "nvmem-cells".
> Access to UBI volumes via the NVMEM interface at this point is
> read-only, and it is slow, opening and closing the UBI volume for each
> access due to limitations of the NVMEM provider API.
I don't feel qualified enough to review the other patches, however this
one looks good to me.
Thanks,
Miquèl
Hi Richard,
On Sun, Feb 25, 2024 at 11:12:54PM +0100, Richard Weinberger wrote:
> ----- Urspr?ngliche Mail -----
> > Von: "Miquel Raynal" <[email protected]>
> > An: "Daniel Golle" <[email protected]>
> > CC: "richard" <[email protected]>, "Vignesh Raghavendra" <[email protected]>, "Rob Herring" <[email protected]>, "Krzysztof
> > Kozlowski" <[email protected]>, "Conor Dooley" <[email protected]>, "linux-mtd"
> > <[email protected]>, "devicetree" <[email protected]>, "linux-kernel"
> > <[email protected]>
> > Gesendet: Montag, 19. Februar 2024 12:01:56
> > Betreff: Re: [PATCH v7 7/7] mtd: ubi: provide NVMEM layer over UBI volumes
>
> > Hi Daniel,
> >
> > [email protected] wrote on Tue, 19 Dec 2023 02:33:48 +0000:
> >
> >> In an ideal world we would like UBI to be used where ever possible on a
> >> NAND chip. And with UBI support in ARM Trusted Firmware and U-Boot it
> >> is possible to achieve an (almost-)all-UBI flash layout. Hence the need
> >> for a way to also use UBI volumes to store board-level constants, such
> >> as MAC addresses and calibration data of wireless interfaces.
> >>
> >> Add UBI volume NVMEM driver module exposing UBI volumes as NVMEM
> >> providers. Allow UBI devices to have a "volumes" firmware subnode with
> >> volumes which may be compatible with "nvmem-cells".
> >> Access to UBI volumes via the NVMEM interface at this point is
> >> read-only, and it is slow, opening and closing the UBI volume for each
> >> access due to limitations of the NVMEM provider API.
> >
> > I don't feel qualified enough to review the other patches, however this
> > one looks good to me.
>
> Finally(!), I had enough time to look.
> Thanks for addressing all my comments form the previous series.
> Patches applied.
It's an enourmous coicident that you are writing just now that I found
a sizeof(int)-related problem which triggers a compiler warning when
building the UBI NVMEM provider on 32-bit platforms. I was just about
to prepare an updated series. Literally in this minute.
Should I still send the whole updates series or only the final patch
(as the necessary change is there) or a follow-up patch fixing the
original patch?
>
> I have only one tiny request, can you share the lockdep spalt
> you encountered in ubi_notify_add() regarding mtd_table_mutex
> and ubi_devices_mutex? The solutions looks okay to me, but
> if you have more details that would be great.
I will setup a test build to reproduce the original warning and
let you know shortly.
Cheers
Daniel
----- Ursprüngliche Mail -----
> Von: "Miquel Raynal" <[email protected]>
> An: "Daniel Golle" <[email protected]>
> CC: "richard" <[email protected]>, "Vignesh Raghavendra" <[email protected]>, "Rob Herring" <[email protected]>, "Krzysztof
> Kozlowski" <[email protected]>, "Conor Dooley" <[email protected]>, "linux-mtd"
> <[email protected]>, "devicetree" <[email protected]>, "linux-kernel"
> <[email protected]>
> Gesendet: Montag, 19. Februar 2024 12:01:56
> Betreff: Re: [PATCH v7 7/7] mtd: ubi: provide NVMEM layer over UBI volumes
> Hi Daniel,
>
> [email protected] wrote on Tue, 19 Dec 2023 02:33:48 +0000:
>
>> In an ideal world we would like UBI to be used where ever possible on a
>> NAND chip. And with UBI support in ARM Trusted Firmware and U-Boot it
>> is possible to achieve an (almost-)all-UBI flash layout. Hence the need
>> for a way to also use UBI volumes to store board-level constants, such
>> as MAC addresses and calibration data of wireless interfaces.
>>
>> Add UBI volume NVMEM driver module exposing UBI volumes as NVMEM
>> providers. Allow UBI devices to have a "volumes" firmware subnode with
>> volumes which may be compatible with "nvmem-cells".
>> Access to UBI volumes via the NVMEM interface at this point is
>> read-only, and it is slow, opening and closing the UBI volume for each
>> access due to limitations of the NVMEM provider API.
>
> I don't feel qualified enough to review the other patches, however this
> one looks good to me.
Finally(!), I had enough time to look.
Thanks for addressing all my comments form the previous series.
Patches applied.
I have only one tiny request, can you share the lockdep spalt
you encountered in ubi_notify_add() regarding mtd_table_mutex
and ubi_devices_mutex? The solutions looks okay to me, but
if you have more details that would be great.
Thanks,
//richard
Hi Richard,
On Sun, Mar 10, 2024 at 10:17:17PM +0100, Richard Weinberger wrote:
> Daniel,
>
> ----- Urspr?ngliche Mail -----
> > Von: "Daniel Golle" <[email protected]>
> >> Finally(!), I had enough time to look.
> >> Thanks for addressing all my comments form the previous series.
> >> Patches applied.
> >
> > It's an enourmous coicident that you are writing just now that I found
> > a sizeof(int)-related problem which triggers a compiler warning when
> > building the UBI NVMEM provider on 32-bit platforms. I was just about
> > to prepare an updated series. Literally in this minute.
> > Should I still send the whole updates series or only the final patch
> > (as the necessary change is there) or a follow-up patch fixing the
> > original patch?
>
> I have just merged your fixup patch. So all good.
Thank you!
>
> >>
> >> I have only one tiny request, can you share the lockdep spalt
> >> you encountered in ubi_notify_add() regarding mtd_table_mutex
> >> and ubi_devices_mutex? The solutions looks okay to me, but
> >> if you have more details that would be great.
> >
> > I will setup a test build to reproduce the original warning and
> > let you know shortly.
>
> Any news on that?
I've tried for days now to reproduce this on recent kernels and fail
to do so. Ie. when using regular mutex_lock() instead of
mutex_lock_nested() I no longer see any lockdep warning with
linux-next. It could be that I'm chasing a lockdep ghost...
> BTW: Is there a nice way to test this with nandsim in qemu?
> I'd love being able to test all ubi attach code paths on my test setup.
From what I can tell 'nandsim' doesn't have a way to be defined in
Device Tree, making it unsuitable to test the attachment of UBI in
this way.
However, QEMU does support emulating TI OMAP's OneNAND controller, eg.
as part of the Nokia N810 hardware supported by qemu-system-arm, see
https://www.qemu.org/docs/master/system/arm/nseries.html
So we could use that and modify the device tree in Linux to have a MTD
partition for UBI and 'compatible = "linux,ubi";' set therein:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ti/omap/omap2420-n8x0-common.dtsi#n84
If you like I can prepare such a test setup.
Is there a repository for MTD/UBI tests to be run on QEMU which I should
contribute this to?
Cheers
Daniel
Daniel,
----- Ursprüngliche Mail -----
> Von: "Daniel Golle" <[email protected]>
>> Finally(!), I had enough time to look.
>> Thanks for addressing all my comments form the previous series.
>> Patches applied.
>
> It's an enourmous coicident that you are writing just now that I found
> a sizeof(int)-related problem which triggers a compiler warning when
> building the UBI NVMEM provider on 32-bit platforms. I was just about
> to prepare an updated series. Literally in this minute.
> Should I still send the whole updates series or only the final patch
> (as the necessary change is there) or a follow-up patch fixing the
> original patch?
I have just merged your fixup patch. So all good.
>>
>> I have only one tiny request, can you share the lockdep spalt
>> you encountered in ubi_notify_add() regarding mtd_table_mutex
>> and ubi_devices_mutex? The solutions looks okay to me, but
>> if you have more details that would be great.
>
> I will setup a test build to reproduce the original warning and
> let you know shortly.
Any news on that?
BTW: Is there a nice way to test this with nandsim in qemu?
I'd love being able to test all ubi attach code paths on my test setup.
Thanks,
//richard
----- Ursprüngliche Mail -----
> Von: "Daniel Golle" <[email protected]>
>> BTW: Is there a nice way to test this with nandsim in qemu?
>> I'd love being able to test all ubi attach code paths on my test setup.
>
> From what I can tell 'nandsim' doesn't have a way to be defined in
> Device Tree, making it unsuitable to test the attachment of UBI in
> this way.
>
> However, QEMU does support emulating TI OMAP's OneNAND controller, eg.
> as part of the Nokia N810 hardware supported by qemu-system-arm, see
>
> https://www.qemu.org/docs/master/system/arm/nseries.html
>
> So we could use that and modify the device tree in Linux to have a MTD
> partition for UBI and 'compatible = "linux,ubi";' set therein:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ti/omap/omap2420-n8x0-common.dtsi#n84
>
> If you like I can prepare such a test setup.
This would be great!
> Is there a repository for MTD/UBI tests to be run on QEMU which I should
> contribute this to?
UBI tests reside in the mtd-utils repository.
http://git.infradead.org/?p=mtd-utils.git;a=tree;f=tests/ubi-tests;h=20fd6a043eeb96a81736dd07885f74e4e0bb0cc0;hb=HEAD
Maybe you can provide a small shell script which configures qemu?
It doesn't have to be fancy, just something David or I can use as staring point.
Thanks,
//richard
On Tue, Mar 19, 2024 at 11:31:18PM +0100, Richard Weinberger wrote:
> ----- Urspr?ngliche Mail -----
> > Von: "Daniel Golle" <[email protected]>
> >> BTW: Is there a nice way to test this with nandsim in qemu?
> >> I'd love being able to test all ubi attach code paths on my test setup.
> >
> > From what I can tell 'nandsim' doesn't have a way to be defined in
> > Device Tree, making it unsuitable to test the attachment of UBI in
> > this way.
> >
> > However, QEMU does support emulating TI OMAP's OneNAND controller, eg.
> > as part of the Nokia N810 hardware supported by qemu-system-arm, see
> >
> > https://www.qemu.org/docs/master/system/arm/nseries.html
> >
> > So we could use that and modify the device tree in Linux to have a MTD
> > partition for UBI and 'compatible = "linux,ubi";' set therein:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ti/omap/omap2420-n8x0-common.dtsi#n84
> >
> > If you like I can prepare such a test setup.
>
> This would be great!
>
> > Is there a repository for MTD/UBI tests to be run on QEMU which I should
> > contribute this to?
>
> UBI tests reside in the mtd-utils repository.
> http://git.infradead.org/?p=mtd-utils.git;a=tree;f=tests/ubi-tests;h=20fd6a043eeb96a81736dd07885f74e4e0bb0cc0;hb=HEAD
>
> Maybe you can provide a small shell script which configures qemu?
> It doesn't have to be fancy, just something David or I can use as staring point.
I'm working on it but it turns out to be a bit more difficult than I
thought it would be, because
* the only devices with NAND flash emulated in QEMU or Nokia N800 and
N810 as well as some even more ancient Intel PXA270 based PDA like
the Sharp 'spitz'.
* QEMU support for the N800 and N810 has apparently been bitrotting and
is broken at least since 2019, nobody seems to care much.
* The spitz predates device tree and hence is unsuitable for testing
attachment of UBI via DT. But it at least boots because Guenter Roeck
makes sure it does[1].
I was about to create a spitz-like imaginary board with DT, but also that
doesn't seem to be completely trivial.
So: hold my beer, I'll be back shortly ;)
If anyone has better ideas on how to utilize support for raw NAND or the
OneNAND controller in QEMU in a device-tree environment which actually
works, that'd be great. Obviously I don't care about other peripherals
like Bluetooth and all the complicated stuff of the N80x...
[1]: https://github.com/groeck/linux-build-test/blob/master/rootfs/arm/run-qemu-arm.sh#L64
----- Ursprüngliche Mail -----
> Von: "Daniel Golle" <[email protected]>
> So: hold my beer, I'll be back shortly ;)
>
> If anyone has better ideas on how to utilize support for raw NAND or the
> OneNAND controller in QEMU in a device-tree environment which actually
> works, that'd be great. Obviously I don't care about other peripherals
> like Bluetooth and all the complicated stuff of the N80x...
Speaking of "hold my beer", maybe we can hack something into nandsim
to act like a device tree attachable device?
In theory, device tree is also available on x86 and other non-embedded archs.
Thanks,
//richard