Message-ID: <6b8201ab-3350-e3f5-b915-b3e94582518d@huawei.com>
Date:   Tue, 25 Apr 2023 11:01:41 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     Daniel Golle <daniel@...rotopia.org>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        Richard Weinberger <richard@....at>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Rafał Miłecki <zajec5@...il.com>
Subject: Re: [PATCH] mtd: ubi: attach MTD partition from device-tree

在 2023/4/24 21:06, Daniel Golle 写道:
> 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 <daniel@...rotopia.org>
>>>>> ---
>>>>>     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?
> 

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

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.

If someone later want to loading ubiblock automically, UBI_VOLUME_ADDED 
case in ubiblock_notify() is suggested to be implemented.

>>
>>>
>>>>
>>>>> +				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 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.

>>
>> 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.
> 
> 
> .
> 

Powered by blists - more mailing lists