Split ubi_init() function into early function to be called by
device_initcall() and keep cmdline attachment in late_initcall().
(when building ubi as module, both is still done in a single
module_init() call)
Register MTD notifier and attach MTD devices which are marked as
compatible with 'linux,ubi' in OF device-tree when being added, detach
UBI device from MTD device when it is being removed.
Keep behavior regarding ubiblock creation and attaching of UBI device
from kernel or module cmdline unchanged.
For existing users this should not change anything besides automatic
removal of (dead) UBI devices when their underlying MTD devices are
already gone, e.g. in case of MTD driver module or (SPI) bus driver
module being removed.
For new users this opens up the option to attach UBI using device-tree
which then happens early and in parallel with other drivers being
probed which slightly reduces the total boot time.
Attachment no longer happening late is also a requirement for other
drivers to make use of UBI, e.g. drivers/nvmem/u-boot-env.c can now
be extended to support U-Boot environment stored in UBI volumes.
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/mtd/ubi/block.c | 20 ++---
drivers/mtd/ubi/build.c | 163 +++++++++++++++++++++++++++++-----------
drivers/mtd/ubi/cdev.c | 2 +-
drivers/mtd/ubi/ubi.h | 4 +-
4 files changed, 132 insertions(+), 57 deletions(-)
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 3711d7f746003..b24b8b8f54b77 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;
@@ -582,7 +582,7 @@ open_volume_desc(const char *name, int ubi_num, int vol_id)
return ubi_open_volume(ubi_num, vol_id, UBI_READONLY);
}
-static void __init ubiblock_create_from_param(void)
+void ubiblock_create_from_param(void)
{
int i, ret = 0;
struct ubiblock_param *p;
@@ -611,9 +611,10 @@ static void __init ubiblock_create_from_param(void)
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);
+ if (ret != -EEXIST && ret != -ENOENT)
+ 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;
}
}
@@ -644,13 +645,6 @@ 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.
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 9cd565daad368..a764f97eee791 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"
@@ -1065,6 +1066,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
* ubi_detach_mtd_dev - detach an MTD device.
* @ubi_num: UBI device number to detach from
* @anyway: detach MTD even if device reference count is not zero
+ * @have_lock: called by MTD notifier holding mtd_table_mutex
*
* This function destroys an UBI device number @ubi_num and detaches the
* underlying MTD device. Returns zero in case of success and %-EBUSY if the
@@ -1074,7 +1076,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
* Note, the invocations of this function has to be serialized by the
* @ubi_devices_mutex.
*/
-int ubi_detach_mtd_dev(int ubi_num, int anyway)
+int ubi_detach_mtd_dev(int ubi_num, int anyway, bool have_lock)
{
struct ubi_device *ubi;
@@ -1108,7 +1110,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
* EC updates that have been made since the last written fastmap.
* In case of fastmap debugging we omit the update to simulate an
* unclean shutdown. */
- if (!ubi_dbg_chk_fastmap(ubi))
+ if (!have_lock && !ubi_dbg_chk_fastmap(ubi))
ubi_update_fastmap(ubi);
#endif
/*
@@ -1130,7 +1132,11 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
vfree(ubi->peb_buf);
vfree(ubi->fm_buf);
ubi_msg(ubi, "mtd%d is detached", ubi->mtd->index);
- put_mtd_device(ubi->mtd);
+ if (have_lock)
+ __put_mtd_device(ubi->mtd);
+ else
+ put_mtd_device(ubi->mtd);
+
put_device(&ubi->dev);
return 0;
}
@@ -1207,43 +1213,55 @@ 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;
+
+ /* 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);
+ mutex_unlock(&ubi_devices_mutex);
+ if (err < 0) {
+ __put_mtd_device(mtd);
+ return;
}
- /* Create base sysfs directory and sysfs files */
- err = class_register(&ubi_class);
- if (err < 0)
- return err;
+ ubiblock_create_from_param();
+}
- err = misc_register(&ubi_ctrl_cdev);
- if (err) {
- pr_err("UBI error: cannot register device\n");
- goto out;
- }
+static void ubi_notify_remove(struct mtd_info *mtd)
+{
+ int i;
- 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;
- }
+ /* called while holding mtd_table_mutex */
+ mutex_lock_nested(&ubi_devices_mutex, SINGLE_DEPTH_NESTING);
+ for (i = 0; i < UBI_MAX_DEVICES; i++)
+ if (ubi_devices[i] &&
+ ubi_devices[i]->mtd->index == mtd->index)
+ ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1, true);
+ mutex_unlock(&ubi_devices_mutex);
+}
- 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++) {
@@ -1291,25 +1309,84 @@ static int __init ubi_init(void)
}
}
+ /*
+ * Attach block devices from 'block=' module param.
+ */
+ ubiblock_create_from_param();
+
+ 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, false);
+ 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:
@@ -1319,18 +1396,20 @@ 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]) {
mutex_lock(&ubi_devices_mutex);
- ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1);
+ ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1, false);
mutex_unlock(&ubi_devices_mutex);
}
ubi_debugfs_exit();
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index f43430b9c1e65..2630f1cd5bac7 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -1065,7 +1065,7 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd,
}
mutex_lock(&ubi_devices_mutex);
- err = ubi_detach_mtd_dev(ubi_num, 0);
+ err = ubi_detach_mtd_dev(ubi_num, 0, false);
mutex_unlock(&ubi_devices_mutex);
break;
}
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index c8f1bd4fa1008..7c8bd5e8d2251 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -939,7 +939,7 @@ int ubi_io_write_vid_hdr(struct ubi_device *ubi, int pnum,
int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
int vid_hdr_offset, int max_beb_per1024,
bool disable_fm);
-int ubi_detach_mtd_dev(int ubi_num, int anyway);
+int ubi_detach_mtd_dev(int ubi_num, int anyway, bool have_lock);
struct ubi_device *ubi_get_device(int ubi_num);
void ubi_put_device(struct ubi_device *ubi);
struct ubi_device *ubi_get_by_major(int major);
@@ -976,11 +976,13 @@ static inline void ubi_fastmap_destroy_checkmap(struct ubi_volume *vol) {}
/* block.c */
#ifdef CONFIG_MTD_UBI_BLOCK
+void ubiblock_create_from_param(void);
int ubiblock_init(void);
void ubiblock_exit(void);
int ubiblock_create(struct ubi_volume_info *vi);
int ubiblock_remove(struct ubi_volume_info *vi);
#else
+static inline void ubiblock_create_from_param(void) {}
static inline int ubiblock_init(void) { return 0; }
static inline void ubiblock_exit(void) {}
static inline int ubiblock_create(struct ubi_volume_info *vi)
base-commit: 67d5d9f013d6c3829383c08162939cabff14fccc
--
2.40.0
Hi Zhihao,
thank you for taking the time to review this patch.
On Sun, Apr 23, 2023 at 04:35:28PM +0800, Zhihao Cheng wrote:
> Hi Daniel,
> > Split ubi_init() function into early function to be called by
> > device_initcall() and keep cmdline attachment in late_initcall().
> > (when building ubi as module, both is still done in a single
> > module_init() call)
> >
> > Register MTD notifier and attach MTD devices which are marked as
> > compatible with 'linux,ubi' in OF device-tree when being added, detach
> > UBI device from MTD device when it is being removed.
> >
> > Keep behavior regarding ubiblock creation and attaching of UBI device
> > from kernel or module cmdline unchanged.
> >
> > For existing users this should not change anything besides automatic
> > removal of (dead) UBI devices when their underlying MTD devices are
> > already gone, e.g. in case of MTD driver module or (SPI) bus driver
> > module being removed.
> >
> > For new users this opens up the option to attach UBI using device-tree
> > which then happens early and in parallel with other drivers being
> > probed which slightly reduces the total boot time.
> >
> > Attachment no longer happening late is also a requirement for other
> > drivers to make use of UBI, e.g. drivers/nvmem/u-boot-env.c can now
> > be extended to support U-Boot environment stored in UBI volumes.
> >
>
> Let me try to understand this patch, the main purpose is to add a new way to
> load ubi according to device tree, am I right?
You are right about one aspect of this patch, yes it does add another
way to attach MTD devices, by utilizing a devicetree compatible
'linux,ubi' which can be set for MTD devices (or partitions).
The other, and maybe even more significant aspect is that it moves
attaching the MTD device from being a late_initcall() to rather register
an MTD user driver instead and hence happen as a direct consequence of
the MTD device showing up.
This will be important when using UBI e.g. as an NVMEM provider:
E.g. on Wi-Fi routers it has become common for vendors to store the
bootloader environment, Ethernet MAC addresses or Wi-Fi calibration
data inside an UBI volume (and I don't mean as a file inside a
filesystem, but really just the UBI volume exposing nvmem cells for
other drivers). See [1] for an example.
>
> > Signed-off-by: Daniel Golle <[email protected]>
> > ---
> > drivers/mtd/ubi/block.c | 20 ++---
> > drivers/mtd/ubi/build.c | 163 +++++++++++++++++++++++++++++-----------
> > drivers/mtd/ubi/cdev.c | 2 +-
> > drivers/mtd/ubi/ubi.h | 4 +-
> > 4 files changed, 132 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
> > index 3711d7f746003..b24b8b8f54b77 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;
> > @@ -582,7 +582,7 @@ open_volume_desc(const char *name, int ubi_num, int vol_id)
> > return ubi_open_volume(ubi_num, vol_id, UBI_READONLY);
> > }
> > -static void __init ubiblock_create_from_param(void)
> > +void ubiblock_create_from_param(void)
> > {
> > int i, ret = 0;
> > struct ubiblock_param *p;
> > @@ -611,9 +611,10 @@ static void __init ubiblock_create_from_param(void)
> > 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);
> > + if (ret != -EEXIST && ret != -ENOENT)
>
> The EEXIST and ENOENT types of error messages are filtered, why? After this
> patch applied, the user won't be aware of which ubi volume failed to create
> ubiblock.
The EEXIST and ENOENT errors as filtered as they may happen now without
that actually being an error.
ubiblock_create_from_param is potentially being called multiple times,
each time a new UBI device is attached in the "new" MTD user/device-tree
way, and also once in "classic" late_initcall way.
One way to partially resolve this could be to add a boolean parameter to
ubiblock_create_from_param which indicates if -ENOENT should be treated
as an error, and set that parameter only when calling from
late_initcall.
Being able to warn users if they added the same ubiblock device more
than once **on the cmdline or module parameter** is more tricky and
would require tracking why a ubiblock device has previously been
created. However, I don't think that emitting a warning in this case
is crucial at all, the user definitely wanted the ubiblock device to be
created and saying that more than once is unneccesary but yet also
doesn't leave room for any interpretation other than just that the user
*really* wants the ubiblock to be created.
>
> > + 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;
> > }
> > }
> > @@ -644,13 +645,6 @@ 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.
> > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> > index 9cd565daad368..a764f97eee791 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"
> > @@ -1065,6 +1066,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
> > * ubi_detach_mtd_dev - detach an MTD device.
> > * @ubi_num: UBI device number to detach from
> > * @anyway: detach MTD even if device reference count is not zero
> > + * @have_lock: called by MTD notifier holding mtd_table_mutex
> > *
> > * This function destroys an UBI device number @ubi_num and detaches the
> > * underlying MTD device. Returns zero in case of success and %-EBUSY if the
> > @@ -1074,7 +1076,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
> > * Note, the invocations of this function has to be serialized by the
> > * @ubi_devices_mutex.
> > */
> > -int ubi_detach_mtd_dev(int ubi_num, int anyway)
> > +int ubi_detach_mtd_dev(int ubi_num, int anyway, bool have_lock)
> > {
> > struct ubi_device *ubi;
> > @@ -1108,7 +1110,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
> > * EC updates that have been made since the last written fastmap.
> > * In case of fastmap debugging we omit the update to simulate an
> > * unclean shutdown. */
> > - if (!ubi_dbg_chk_fastmap(ubi))
> > + if (!have_lock && !ubi_dbg_chk_fastmap(ubi))
> > ubi_update_fastmap(ubi);
>
> Why do you skip updating fastmap if ubi is detached in mtd notification way?
The reason here is simple: Once we receive a notification about the MTD
device being removed it is too late to update fastmap. The device is
gone, nothing we can do about that any more. Just like removing a
device holding a filesystem without having priorly unmounted it, e.g.
if you 'rmmod usb-storage' (just that block devices unfortunately still
lack notifications about removal or size changes...)
>
> [...]> +static void ubi_notify_remove(struct mtd_info *mtd)
> > +{
> > + int i;
> > - 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;
> > - }
> > + /* called while holding mtd_table_mutex */
> > + mutex_lock_nested(&ubi_devices_mutex, SINGLE_DEPTH_NESTING);
> > + for (i = 0; i < UBI_MAX_DEVICES; i++)
> > + if (ubi_devices[i] &&
> > + ubi_devices[i]->mtd->index == mtd->index)
> > + ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1, true);
>
> Add a 'break'?
Good point :)
Will add 'break' statement here.
[1]: https://forum.openwrt.org/t/asus-tuf-ax4200-support/155738/44?u=daniel
On Mon, Apr 24, 2023 at 11:26:13AM +0800, Zhihao Cheng wrote:
> 在 2023/4/24 7:16, Daniel Golle 写道:
> > Hi Zhihao,
> >
> > thank you for taking the time to review this patch.
> >
> > On Sun, Apr 23, 2023 at 04:35:28PM +0800, Zhihao Cheng wrote:
> > > Hi Daniel,
> > > > Split ubi_init() function into early function to be called by
> > > > device_initcall() and keep cmdline attachment in late_initcall().
> > > > (when building ubi as module, both is still done in a single
> > > > module_init() call)
> > > >
> > > > Register MTD notifier and attach MTD devices which are marked as
> > > > compatible with 'linux,ubi' in OF device-tree when being added, detach
> > > > UBI device from MTD device when it is being removed.
> > > >
> > > > Keep behavior regarding ubiblock creation and attaching of UBI device
> > > > from kernel or module cmdline unchanged.
> > > >
> > > > For existing users this should not change anything besides automatic
> > > > removal of (dead) UBI devices when their underlying MTD devices are
> > > > already gone, e.g. in case of MTD driver module or (SPI) bus driver
> > > > module being removed.
> > > >
> > > > For new users this opens up the option to attach UBI using device-tree
> > > > which then happens early and in parallel with other drivers being
> > > > probed which slightly reduces the total boot time.
> > > >
> > > > Attachment no longer happening late is also a requirement for other
> > > > drivers to make use of UBI, e.g. drivers/nvmem/u-boot-env.c can now
> > > > be extended to support U-Boot environment stored in UBI volumes.
> > > >
> > >
> > > Let me try to understand this patch, the main purpose is to add a new way to
> > > load ubi according to device tree, am I right?
> >
> > You are right about one aspect of this patch, yes it does add another
> > way to attach MTD devices, by utilizing a devicetree compatible
> > 'linux,ubi' which can be set for MTD devices (or partitions).
> > The other, and maybe even more significant aspect is that it moves
> > attaching the MTD device from being a late_initcall() to rather register
> > an MTD user driver instead and hence happen as a direct consequence of
> > the MTD device showing up.
>
> It means mtd device probing and loading will autimically triggers ubi
> attaching by ubi_notify_add(), right?
> If so, I thought of a scenario like this: The mtd device probing/removing
> will autimically loading/removing UBI device, now it can be done without any
> orders(cmdlines/ioctl) in running time. I think it's valuable.
>
> > This will be important when using UBI e.g. as an NVMEM provider:
> > E.g. on Wi-Fi routers it has become common for vendors to store the
> > bootloader environment, Ethernet MAC addresses or Wi-Fi calibration
> > data inside an UBI volume (and I don't mean as a file inside a
> > filesystem, but really just the UBI volume exposing nvmem cells for
> > other drivers). See [1] for an example.
> >
>
> Thanks for introducing the background.
>
> > >
> > > > Signed-off-by: Daniel Golle <[email protected]>
> > > > ---
> > > > drivers/mtd/ubi/block.c | 20 ++---
> > > > drivers/mtd/ubi/build.c | 163 +++++++++++++++++++++++++++++-----------
> > > > drivers/mtd/ubi/cdev.c | 2 +-
> > > > drivers/mtd/ubi/ubi.h | 4 +-
> > > > 4 files changed, 132 insertions(+), 57 deletions(-)
> > > >
> > > > diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
> > > > index 3711d7f746003..b24b8b8f54b77 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;
> > > > @@ -582,7 +582,7 @@ open_volume_desc(const char *name, int ubi_num, int vol_id)
> > > > return ubi_open_volume(ubi_num, vol_id, UBI_READONLY);
> > > > }
> > > > -static void __init ubiblock_create_from_param(void)
> > > > +void ubiblock_create_from_param(void)
> > > > {
> > > > int i, ret = 0;
> > > > struct ubiblock_param *p;
> > > > @@ -611,9 +611,10 @@ static void __init ubiblock_create_from_param(void)
> > > > 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);
> > > > + if (ret != -EEXIST && ret != -ENOENT)
> > >
> > > The EEXIST and ENOENT types of error messages are filtered, why? After this
> > > patch applied, the user won't be aware of which ubi volume failed to create
> > > ubiblock.
> >
> > The EEXIST and ENOENT errors as filtered as they may happen now without
> > that actually being an error.
> > ubiblock_create_from_param is potentially being called multiple times,
> > each time a new UBI device is attached in the "new" MTD user/device-tree
> > way, and also once in "classic" late_initcall way.
> >
>
> I agree, the second caller could trigger EEXIST. But how ENOENT happens, do
> you mean ubi_notify_add->ubiblock_create_from_param->open_volume_desc? Could
> you show me path?
You are right, EEXIST will actually never happen. If at all then we
should check if open_volume_desc returns ERR_PTR(-ENODEV).
However, we can never know if the UBI device containing the volume we want to
have exposed as ubiblock device is still going to be added later when the
driver module for the MTD device is loaded. Hence I suggest to simply
not call pr_err(...) in case open_volume_desc returns ERR_PTR(-ENODEV)
as well as for the case that ubiblock_create returns -EEXIST.
> > One way to partially resolve this could be to add a boolean parameter to
> > ubiblock_create_from_param which indicates if -ENOENT should be treated
> > as an error, and set that parameter only when calling from
> > late_initcall.
> >
> > Being able to warn users if they added the same ubiblock device more
> > than once **on the cmdline or module parameter** is more tricky and
> > would require tracking why a ubiblock device has previously been
> > created. However, I don't think that emitting a warning in this case
> > is crucial at all, the user definitely wanted the ubiblock device to be
> > created and saying that more than once is unneccesary but yet also
> > doesn't leave room for any interpretation other than just that the user
> > *really* wants the ubiblock to be created.
>
> Emm, I prefer to fix it, it's weird that ubiblock_create_from_param() being
> executed multiple times(Each ubi_notify_add() will call once, late_initcall
> will call it again). User could see many false positive error messages.
Given my suggestion above (not printing error in case the UBI device or
volume doesn't exist as well as in case in case the ubiblock device has
already been created) there won't be any false positives.
What is still true is that other errors (eg. invalid string format of
the ubiblock parameter) will be printed multiple times.
Do you think it is worth putting much effort into avoiding that?
If so, any idea how?
>
> >
> > >
> > > > + 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;
> > > > }
> > > > }
> > > > @@ -644,13 +645,6 @@ 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.
> > > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> > > > index 9cd565daad368..a764f97eee791 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"
> > > > @@ -1065,6 +1066,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
> > > > * ubi_detach_mtd_dev - detach an MTD device.
> > > > * @ubi_num: UBI device number to detach from
> > > > * @anyway: detach MTD even if device reference count is not zero
> > > > + * @have_lock: called by MTD notifier holding mtd_table_mutex
> > > > *
> > > > * This function destroys an UBI device number @ubi_num and detaches the
> > > > * underlying MTD device. Returns zero in case of success and %-EBUSY if the
> > > > @@ -1074,7 +1076,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
> > > > * Note, the invocations of this function has to be serialized by the
> > > > * @ubi_devices_mutex.
> > > > */
> > > > -int ubi_detach_mtd_dev(int ubi_num, int anyway)
> > > > +int ubi_detach_mtd_dev(int ubi_num, int anyway, bool have_lock)
> > > > {
> > > > struct ubi_device *ubi;
> > > > @@ -1108,7 +1110,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
> > > > * EC updates that have been made since the last written fastmap.
> > > > * In case of fastmap debugging we omit the update to simulate an
> > > > * unclean shutdown. */
> > > > - if (!ubi_dbg_chk_fastmap(ubi))
> > > > + if (!have_lock && !ubi_dbg_chk_fastmap(ubi))
> > > > ubi_update_fastmap(ubi);
> > >
> > > Why do you skip updating fastmap if ubi is detached in mtd notification way?
> >
> > The reason here is simple: Once we receive a notification about the MTD
> > device being removed it is too late to update fastmap. The device is
> > gone, nothing we can do about that any more. Just like removing a
> > device holding a filesystem without having priorly unmounted it, e.g.
> > if you 'rmmod usb-storage' (just that block devices unfortunately still
> > lack notifications about removal or size changes...)
> >
>
> Another path still operate mtd device:
> ubi_detach_mtd_dev -> ubi_wl_close -> shutdown_work -> wrk->func ->
> erase_worker -> sync_erase -> ubi_io_sync_erase/ubi_io_write_ec_hdr.
True, but I didn't see this being a problem, at least in the sense
that I didn't see a kernel panic. We could try to make
ubi_update_fastmap more robust to gracefully return in case the device
is already gone.
>
> Besides, can we remove any mtd specific driver(eg. phram/nandsim) by rmmod?
My test-case was to remove the SPI bus driver on which the SPI-NAND chip
is connected to. Afaik there is also no way to prevent this at this
point, we'd have to propagate the usecount of the MTD device down to
the SPI bus hosting it.
> I notice that get_mtd_device(called before ubi attaching) will increase
> specific mtd driver's module refcnt.
> If we physically remove the mtd device, will it trigger del_mtd_device()?
Yes, in case what you mean is an SPI-NAND flash connected to an SPI-USB
adapter, and then pulling the USB device.
> For example, when is phram_remove() called?
From what I can see:
phram_remove -> mtd_device_unregister -> del_mtd_device -> [for all
elements of list mtd_notifiers call their 'remove' function]
After this we should always end up with mtd->usecount == 0 and hence
proceed. If usecount != 0 at this point there is currently no meaningful
error handling in mtdcore.c as far as my interpretation goes.
On Tue, Apr 25, 2023 at 11:01:41AM +0800, Zhihao Cheng wrote:
> 在 2023/4/24 21:06, Daniel Golle 写道:
> > On Mon, Apr 24, 2023 at 11:26:13AM +0800, Zhihao Cheng wrote:
> > > > [...]
> > > > One way to partially resolve this could be to add a boolean parameter to
> > > > ubiblock_create_from_param which indicates if -ENOENT should be treated
> > > > as an error, and set that parameter only when calling from
> > > > late_initcall.
> > > >
> > > > Being able to warn users if they added the same ubiblock device more
> > > > than once **on the cmdline or module parameter** is more tricky and
> > > > would require tracking why a ubiblock device has previously been
> > > > created. However, I don't think that emitting a warning in this case
> > > > is crucial at all, the user definitely wanted the ubiblock device to be
> > > > created and saying that more than once is unneccesary but yet also
> > > > doesn't leave room for any interpretation other than just that the user
> > > > *really* wants the ubiblock to be created.
> > >
> > > Emm, I prefer to fix it, it's weird that ubiblock_create_from_param() being
> > > executed multiple times(Each ubi_notify_add() will call once, late_initcall
> > > will call it again). User could see many false positive error messages.
> >
> > Given my suggestion above (not printing error in case the UBI device or
> > volume doesn't exist as well as in case in case the ubiblock device has
> > already been created) there won't be any false positives.
> > What is still true is that other errors (eg. invalid string format of
> > the ubiblock parameter) will be printed multiple times.
> >
> > Do you think it is worth putting much effort into avoiding that?
> > If so, any idea how?
> >
>
> Currently, there are two timings to load ubiblock:
> 1. boot cmdline: One-time loading
> 2. ioctl UBI_IOCVOLCRBLK: Can be called multiple times at runtime
> PS: ubiblock cannot be compiled in module, there are only two options: Y and
> N
It's true that ubiblock is a boolean option, however, Y or N decides
whether support for ubiblock will be included with the ubi driver, which
can well be a kernel module. In that case, ubiblock is loaded and removed
together with the rest of the ubi driver.
>
> How about deleting 'ubiblock_create_from_param' from ubi_notify_add(), we
> only call ubiblock_create_from_param() once in ubi_init_attach(). This can
> keep the ubiblock loading timings are unchanged, and ENODEV and EEXIST won't
> be false positive.
I also thought that and it works well under the assumption that all
drivers providing MTD devices are built-in or inserted before the ubi
driver is inserted. However, this may not always be true. Think of,
for example, an SPI bus driver which is built as a module and loaded
during boot (eg. from initramfs). Your suggestion below solves that.
>
> If someone later want to loading ubiblock automically, UBI_VOLUME_ADDED case
> in ubiblock_notify() is suggested to be implemented.
I've implemented your suggestion and it works fine, to me even looks
better than the current code which gets/puts the UBI volume just to
compare if its info matches the cmdline parameter. I will post this as
a preliminary patch in the series, together with an other fix and an
updated version of this patch.
>
> > >
> > > >
> > > > >
> > > > > > + 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;
> > > > > > }
> > > > > > }
> > > > > > @@ -644,13 +645,6 @@ 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.
> > > > > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> > > > > > index 9cd565daad368..a764f97eee791 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"
> > > > > > @@ -1065,6 +1066,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
> > > > > > * ubi_detach_mtd_dev - detach an MTD device.
> > > > > > * @ubi_num: UBI device number to detach from
> > > > > > * @anyway: detach MTD even if device reference count is not zero
> > > > > > + * @have_lock: called by MTD notifier holding mtd_table_mutex
> > > > > > *
> > > > > > * This function destroys an UBI device number @ubi_num and detaches the
> > > > > > * underlying MTD device. Returns zero in case of success and %-EBUSY if the
> > > > > > @@ -1074,7 +1076,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
> > > > > > * Note, the invocations of this function has to be serialized by the
> > > > > > * @ubi_devices_mutex.
> > > > > > */
> > > > > > -int ubi_detach_mtd_dev(int ubi_num, int anyway)
> > > > > > +int ubi_detach_mtd_dev(int ubi_num, int anyway, bool have_lock)
> > > > > > {
> > > > > > struct ubi_device *ubi;
> > > > > > @@ -1108,7 +1110,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
> > > > > > * EC updates that have been made since the last written fastmap.
> > > > > > * In case of fastmap debugging we omit the update to simulate an
> > > > > > * unclean shutdown. */
> > > > > > - if (!ubi_dbg_chk_fastmap(ubi))
> > > > > > + if (!have_lock && !ubi_dbg_chk_fastmap(ubi))
> > > > > > ubi_update_fastmap(ubi);
> > > > >
> > > > > Why do you skip updating fastmap if ubi is detached in mtd notification way?
> > > >
> > > > The reason here is simple: Once we receive a notification about the MTD
> > > > device being removed it is too late to update fastmap. The device is
> > > > gone, nothing we can do about that any more. Just like removing a
> > > > device holding a filesystem without having priorly unmounted it, e.g.
> > > > if you 'rmmod usb-storage' (just that block devices unfortunately still
> > > > lack notifications about removal or size changes...)
> > > >
> > >
> > > Another path still operate mtd device:
> > > ubi_detach_mtd_dev -> ubi_wl_close -> shutdown_work -> wrk->func ->
> > > erase_worker -> sync_erase -> ubi_io_sync_erase/ubi_io_write_ec_hdr.
> >
> > True, but I didn't see this being a problem, at least in the sense
> > that I didn't see a kernel panic. We could try to make
> > ubi_update_fastmap more robust to gracefully return in case the device
> > is already gone.
> >
>
> You mean ubi_update_fastmap() could trigger panic in ubi_notify_remove()
> when you remove the SPI bus dirver? May I have the error message?
I spent a day stairing at the kernel Oops and found that it was unrelated
to UBI, but rather a bug in the SPI driver. I've posted the fix:
https://patchwork.kernel.org/project/spi-devel-general/patch/[email protected]/
> I thought both ubi_update_fastmap() and ubi_wl_close() could operate the
> removed mtd device, it will be better to notify user error messages of
> ubi_update_fastmap() casued by removing under layer device.
You were right, there is no problem there and we don't need to skip
ubi_update_fastmap() now that the SPI driver makes sure to complete
any ongoing operation before unloading (see above).
>
> > >
> > > Besides, can we remove any mtd specific driver(eg. phram/nandsim) by rmmod?
> >
> > My test-case was to remove the SPI bus driver on which the SPI-NAND chip
> > is connected to. Afaik there is also no way to prevent this at this
> > point, we'd have to propagate the usecount of the MTD device down to
> > the SPI bus hosting it.
> >
> > > I notice that get_mtd_device(called before ubi attaching) will increase
> > > specific mtd driver's module refcnt.
> > > If we physically remove the mtd device, will it trigger del_mtd_device()?
> >
> > Yes, in case what you mean is an SPI-NAND flash connected to an SPI-USB
> > adapter, and then pulling the USB device.
> >
> > > For example, when is phram_remove() called?
> >
> > > From what I can see:
> > phram_remove -> mtd_device_unregister -> del_mtd_device -> [for all
> > elements of list mtd_notifiers call their 'remove' function]
> >
> > After this we should always end up with mtd->usecount == 0 and hence
> > proceed. If usecount != 0 at this point there is currently no meaningful
> > error handling in mtdcore.c as far as my interpretation goes.
I've found a similar issue with "fake error handling" on removal which
should rather be skipped in drivers/mtd/ubi/block.c. I will post the
fix also in the upcoming series which I'm preparing right now.