Re: [PATCH] mtd: ubi: attach MTD partition from device-tree
From: Zhihao Cheng
Date: Sun Apr 23 2023 - 23:26:34 EST
在 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 <daniel@xxxxxxxxxxxxxx>
---
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?
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.
+ 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.
Besides, can we remove any mtd specific driver(eg. phram/nandsim) by
rmmod? 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()? For example, when is phram_remove() called?