From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5F5E4C77B73 for ; Tue, 25 Apr 2023 03:02:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232202AbjDYDCL (ORCPT ); Mon, 24 Apr 2023 23:02:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40274 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232851AbjDYDB6 (ORCPT ); Mon, 24 Apr 2023 23:01:58 -0400 Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B128786BC for ; Mon, 24 Apr 2023 20:01:55 -0700 (PDT) Received: from kwepemm600013.china.huawei.com (unknown [172.30.72.53]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4Q569t4nZ0zSvNC; Tue, 25 Apr 2023 10:57:38 +0800 (CST) Received: from [10.174.178.46] (10.174.178.46) by kwepemm600013.china.huawei.com (7.193.23.68) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23; Tue, 25 Apr 2023 11:01:53 +0800 Subject: Re: [PATCH] mtd: ubi: attach MTD partition from device-tree To: Daniel Golle CC: , , Richard Weinberger , Miquel Raynal , Vignesh Raghavendra , =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= References: <9c622f92-382d-8b0d-a438-c7eea0e89b80@huawei.com> From: Zhihao Cheng Message-ID: <6b8201ab-3350-e3f5-b915-b3e94582518d@huawei.com> Date: Tue, 25 Apr 2023 11:01:41 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.174.178.46] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemm600013.china.huawei.com (7.193.23.68) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 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 >>>>> --- >>>>> 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 >>>>> #include >>>>> #include >>>>> +#include >>>>> #include >>>>> #include >>>>> #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. > > > . >