Message-ID: <9f10bef5-afa9-132e-3c61-323a2e56931d@huawei.com>
Date:   Wed, 3 May 2023 10:26:33 +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/5/2 2:40, Daniel Golle 写道:
> 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.

You are right, ubiblock will be loaded in module way if UBI driver is 
compiled in module format, my mistake.

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

I'll take a look at this series.
> 
>>
>>>>
>>>>>
>>>>>>
>>>>>>> +				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/ZFAF6pJxMu1z6k4w@makrotopia.org/
> 
>> 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).
> 

Great.

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

Powered by blists - more mailing lists