| Message ID | 82ceb13954f7e701bf47c112333e7b15a57fc360.1702952891.git.daniel@makrotopia.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: ubi: allow UBI volumes to provide NVMEM | expand |
Hi Daniel, daniel@makrotopia.org wrote on Tue, 19 Dec 2023 02:33:48 +0000: > In an ideal world we would like UBI to be used where ever possible on a > NAND chip. And with UBI support in ARM Trusted Firmware and U-Boot it > is possible to achieve an (almost-)all-UBI flash layout. Hence the need > for a way to also use UBI volumes to store board-level constants, such > as MAC addresses and calibration data of wireless interfaces. > > Add UBI volume NVMEM driver module exposing UBI volumes as NVMEM > providers. Allow UBI devices to have a "volumes" firmware subnode with > volumes which may be compatible with "nvmem-cells". > Access to UBI volumes via the NVMEM interface at this point is > read-only, and it is slow, opening and closing the UBI volume for each > access due to limitations of the NVMEM provider API. I don't feel qualified enough to review the other patches, however this one looks good to me. Thanks, Miquèl
----- Ursprüngliche Mail ----- > Von: "Miquel Raynal" <miquel.raynal@bootlin.com> > An: "Daniel Golle" <daniel@makrotopia.org> > CC: "richard" <richard@nod.at>, "Vignesh Raghavendra" <vigneshr@ti.com>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof > Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>, "linux-mtd" > <linux-mtd@lists.infradead.org>, "devicetree" <devicetree@vger.kernel.org>, "linux-kernel" > <linux-kernel@vger.kernel.org> > Gesendet: Montag, 19. Februar 2024 12:01:56 > Betreff: Re: [PATCH v7 7/7] mtd: ubi: provide NVMEM layer over UBI volumes > Hi Daniel, > > daniel@makrotopia.org wrote on Tue, 19 Dec 2023 02:33:48 +0000: > >> In an ideal world we would like UBI to be used where ever possible on a >> NAND chip. And with UBI support in ARM Trusted Firmware and U-Boot it >> is possible to achieve an (almost-)all-UBI flash layout. Hence the need >> for a way to also use UBI volumes to store board-level constants, such >> as MAC addresses and calibration data of wireless interfaces. >> >> Add UBI volume NVMEM driver module exposing UBI volumes as NVMEM >> providers. Allow UBI devices to have a "volumes" firmware subnode with >> volumes which may be compatible with "nvmem-cells". >> Access to UBI volumes via the NVMEM interface at this point is >> read-only, and it is slow, opening and closing the UBI volume for each >> access due to limitations of the NVMEM provider API. > > I don't feel qualified enough to review the other patches, however this > one looks good to me. Finally(!), I had enough time to look. Thanks for addressing all my comments form the previous series. Patches applied. I have only one tiny request, can you share the lockdep spalt you encountered in ubi_notify_add() regarding mtd_table_mutex and ubi_devices_mutex? The solutions looks okay to me, but if you have more details that would be great. Thanks, //richard
Hi Richard, On Sun, Feb 25, 2024 at 11:12:54PM +0100, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- > > Von: "Miquel Raynal" <miquel.raynal@bootlin.com> > > An: "Daniel Golle" <daniel@makrotopia.org> > > CC: "richard" <richard@nod.at>, "Vignesh Raghavendra" <vigneshr@ti.com>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof > > Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>, "linux-mtd" > > <linux-mtd@lists.infradead.org>, "devicetree" <devicetree@vger.kernel.org>, "linux-kernel" > > <linux-kernel@vger.kernel.org> > > Gesendet: Montag, 19. Februar 2024 12:01:56 > > Betreff: Re: [PATCH v7 7/7] mtd: ubi: provide NVMEM layer over UBI volumes > > > Hi Daniel, > > > > daniel@makrotopia.org wrote on Tue, 19 Dec 2023 02:33:48 +0000: > > > >> In an ideal world we would like UBI to be used where ever possible on a > >> NAND chip. And with UBI support in ARM Trusted Firmware and U-Boot it > >> is possible to achieve an (almost-)all-UBI flash layout. Hence the need > >> for a way to also use UBI volumes to store board-level constants, such > >> as MAC addresses and calibration data of wireless interfaces. > >> > >> Add UBI volume NVMEM driver module exposing UBI volumes as NVMEM > >> providers. Allow UBI devices to have a "volumes" firmware subnode with > >> volumes which may be compatible with "nvmem-cells". > >> Access to UBI volumes via the NVMEM interface at this point is > >> read-only, and it is slow, opening and closing the UBI volume for each > >> access due to limitations of the NVMEM provider API. > > > > I don't feel qualified enough to review the other patches, however this > > one looks good to me. > > Finally(!), I had enough time to look. > Thanks for addressing all my comments form the previous series. > Patches applied. It's an enourmous coicident that you are writing just now that I found a sizeof(int)-related problem which triggers a compiler warning when building the UBI NVMEM provider on 32-bit platforms. I was just about to prepare an updated series. Literally in this minute. Should I still send the whole updates series or only the final patch (as the necessary change is there) or a follow-up patch fixing the original patch? > > I have only one tiny request, can you share the lockdep spalt > you encountered in ubi_notify_add() regarding mtd_table_mutex > and ubi_devices_mutex? The solutions looks okay to me, but > if you have more details that would be great. I will setup a test build to reproduce the original warning and let you know shortly. Cheers Daniel
Daniel, ----- Ursprüngliche Mail ----- > Von: "Daniel Golle" <daniel@makrotopia.org> >> Finally(!), I had enough time to look. >> Thanks for addressing all my comments form the previous series. >> Patches applied. > > It's an enourmous coicident that you are writing just now that I found > a sizeof(int)-related problem which triggers a compiler warning when > building the UBI NVMEM provider on 32-bit platforms. I was just about > to prepare an updated series. Literally in this minute. > Should I still send the whole updates series or only the final patch > (as the necessary change is there) or a follow-up patch fixing the > original patch? I have just merged your fixup patch. So all good. >> >> I have only one tiny request, can you share the lockdep spalt >> you encountered in ubi_notify_add() regarding mtd_table_mutex >> and ubi_devices_mutex? The solutions looks okay to me, but >> if you have more details that would be great. > > I will setup a test build to reproduce the original warning and > let you know shortly. Any news on that? BTW: Is there a nice way to test this with nandsim in qemu? I'd love being able to test all ubi attach code paths on my test setup. Thanks, //richard
Hi Richard, On Sun, Mar 10, 2024 at 10:17:17PM +0100, Richard Weinberger wrote: > Daniel, > > ----- Ursprüngliche Mail ----- > > Von: "Daniel Golle" <daniel@makrotopia.org> > >> Finally(!), I had enough time to look. > >> Thanks for addressing all my comments form the previous series. > >> Patches applied. > > > > It's an enourmous coicident that you are writing just now that I found > > a sizeof(int)-related problem which triggers a compiler warning when > > building the UBI NVMEM provider on 32-bit platforms. I was just about > > to prepare an updated series. Literally in this minute. > > Should I still send the whole updates series or only the final patch > > (as the necessary change is there) or a follow-up patch fixing the > > original patch? > > I have just merged your fixup patch. So all good. Thank you! > > >> > >> I have only one tiny request, can you share the lockdep spalt > >> you encountered in ubi_notify_add() regarding mtd_table_mutex > >> and ubi_devices_mutex? The solutions looks okay to me, but > >> if you have more details that would be great. > > > > I will setup a test build to reproduce the original warning and > > let you know shortly. > > Any news on that? I've tried for days now to reproduce this on recent kernels and fail to do so. Ie. when using regular mutex_lock() instead of mutex_lock_nested() I no longer see any lockdep warning with linux-next. It could be that I'm chasing a lockdep ghost... > BTW: Is there a nice way to test this with nandsim in qemu? > I'd love being able to test all ubi attach code paths on my test setup. From what I can tell 'nandsim' doesn't have a way to be defined in Device Tree, making it unsuitable to test the attachment of UBI in this way. However, QEMU does support emulating TI OMAP's OneNAND controller, eg. as part of the Nokia N810 hardware supported by qemu-system-arm, see https://www.qemu.org/docs/master/system/arm/nseries.html So we could use that and modify the device tree in Linux to have a MTD partition for UBI and 'compatible = "linux,ubi";' set therein: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ti/omap/omap2420-n8x0-common.dtsi#n84 If you like I can prepare such a test setup. Is there a repository for MTD/UBI tests to be run on QEMU which I should contribute this to? Cheers Daniel
----- Ursprüngliche Mail ----- > Von: "Daniel Golle" <daniel@makrotopia.org> >> BTW: Is there a nice way to test this with nandsim in qemu? >> I'd love being able to test all ubi attach code paths on my test setup. > > From what I can tell 'nandsim' doesn't have a way to be defined in > Device Tree, making it unsuitable to test the attachment of UBI in > this way. > > However, QEMU does support emulating TI OMAP's OneNAND controller, eg. > as part of the Nokia N810 hardware supported by qemu-system-arm, see > > https://www.qemu.org/docs/master/system/arm/nseries.html > > So we could use that and modify the device tree in Linux to have a MTD > partition for UBI and 'compatible = "linux,ubi";' set therein: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ti/omap/omap2420-n8x0-common.dtsi#n84 > > If you like I can prepare such a test setup. This would be great! > Is there a repository for MTD/UBI tests to be run on QEMU which I should > contribute this to? UBI tests reside in the mtd-utils repository. http://git.infradead.org/?p=mtd-utils.git;a=tree;f=tests/ubi-tests;h=20fd6a043eeb96a81736dd07885f74e4e0bb0cc0;hb=HEAD Maybe you can provide a small shell script which configures qemu? It doesn't have to be fancy, just something David or I can use as staring point. Thanks, //richard
On Tue, Mar 19, 2024 at 11:31:18PM +0100, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- > > Von: "Daniel Golle" <daniel@makrotopia.org> > >> BTW: Is there a nice way to test this with nandsim in qemu? > >> I'd love being able to test all ubi attach code paths on my test setup. > > > > From what I can tell 'nandsim' doesn't have a way to be defined in > > Device Tree, making it unsuitable to test the attachment of UBI in > > this way. > > > > However, QEMU does support emulating TI OMAP's OneNAND controller, eg. > > as part of the Nokia N810 hardware supported by qemu-system-arm, see > > > > https://www.qemu.org/docs/master/system/arm/nseries.html > > > > So we could use that and modify the device tree in Linux to have a MTD > > partition for UBI and 'compatible = "linux,ubi";' set therein: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ti/omap/omap2420-n8x0-common.dtsi#n84 > > > > If you like I can prepare such a test setup. > > This would be great! > > > Is there a repository for MTD/UBI tests to be run on QEMU which I should > > contribute this to? > > UBI tests reside in the mtd-utils repository. > http://git.infradead.org/?p=mtd-utils.git;a=tree;f=tests/ubi-tests;h=20fd6a043eeb96a81736dd07885f74e4e0bb0cc0;hb=HEAD > > Maybe you can provide a small shell script which configures qemu? > It doesn't have to be fancy, just something David or I can use as staring point. I'm working on it but it turns out to be a bit more difficult than I thought it would be, because * the only devices with NAND flash emulated in QEMU or Nokia N800 and N810 as well as some even more ancient Intel PXA270 based PDA like the Sharp 'spitz'. * QEMU support for the N800 and N810 has apparently been bitrotting and is broken at least since 2019, nobody seems to care much. * The spitz predates device tree and hence is unsuitable for testing attachment of UBI via DT. But it at least boots because Guenter Roeck makes sure it does[1]. I was about to create a spitz-like imaginary board with DT, but also that doesn't seem to be completely trivial. So: hold my beer, I'll be back shortly ;) If anyone has better ideas on how to utilize support for raw NAND or the OneNAND controller in QEMU in a device-tree environment which actually works, that'd be great. Obviously I don't care about other peripherals like Bluetooth and all the complicated stuff of the N80x... [1]: https://github.com/groeck/linux-build-test/blob/master/rootfs/arm/run-qemu-arm.sh#L64
----- Ursprüngliche Mail ----- > Von: "Daniel Golle" <daniel@makrotopia.org> > So: hold my beer, I'll be back shortly ;) > > If anyone has better ideas on how to utilize support for raw NAND or the > OneNAND controller in QEMU in a device-tree environment which actually > works, that'd be great. Obviously I don't care about other peripherals > like Bluetooth and all the complicated stuff of the N80x... Speaking of "hold my beer", maybe we can hack something into nandsim to act like a device tree attachable device? In theory, device tree is also available on x86 and other non-embedded archs. Thanks, //richard
diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig index 2ed77b7b3fcb5..45d939bbfa853 100644 --- a/drivers/mtd/ubi/Kconfig +++ b/drivers/mtd/ubi/Kconfig @@ -104,4 +104,16 @@ config MTD_UBI_BLOCK If in doubt, say "N". +config MTD_UBI_NVMEM + tristate "UBI virtual NVMEM" + default n + depends on NVMEM + help + This option enabled an additional driver exposing UBI volumes as NVMEM + providers, intended for platforms where UBI is part of the firmware + specification and used to store also e.g. MAC addresses or board- + specific Wi-Fi calibration data. + + If in doubt, say "N". + endif # MTD_UBI diff --git a/drivers/mtd/ubi/Makefile b/drivers/mtd/ubi/Makefile index 543673605ca72..4b51aaf00d1a2 100644 --- a/drivers/mtd/ubi/Makefile +++ b/drivers/mtd/ubi/Makefile @@ -7,3 +7,4 @@ ubi-$(CONFIG_MTD_UBI_FASTMAP) += fastmap.o ubi-$(CONFIG_MTD_UBI_BLOCK) += block.o obj-$(CONFIG_MTD_UBI_GLUEBI) += gluebi.o +obj-$(CONFIG_MTD_UBI_NVMEM) += nvmem.o diff --git a/drivers/mtd/ubi/nvmem.c b/drivers/mtd/ubi/nvmem.c new file mode 100644 index 0000000000000..b7a93c495d172 --- /dev/null +++ b/drivers/mtd/ubi/nvmem.c @@ -0,0 +1,188 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2023 Daniel Golle <daniel@makrotopia.org> + */ + +/* UBI NVMEM provider */ +#include "ubi.h" +#include <linux/nvmem-provider.h> +#include <asm/div64.h> + +/* List of all NVMEM devices */ +static LIST_HEAD(nvmem_devices); +static DEFINE_MUTEX(devices_mutex); + +struct ubi_nvmem { + struct nvmem_device *nvmem; + int ubi_num; + int vol_id; + int usable_leb_size; + struct list_head list; +}; + +static int ubi_nvmem_reg_read(void *priv, unsigned int from, + void *val, size_t bytes) +{ + int err = 0, lnum = from, offs, bytes_left = bytes, to_read; + struct ubi_nvmem *unv = priv; + struct ubi_volume_desc *desc; + + desc = ubi_open_volume(unv->ubi_num, unv->vol_id, UBI_READONLY); + if (IS_ERR(desc)) + return PTR_ERR(desc); + + offs = do_div(lnum, unv->usable_leb_size); + while (bytes_left) { + to_read = unv->usable_leb_size - offs; + + if (to_read > bytes_left) + to_read = bytes_left; + + err = ubi_read(desc, lnum, val, offs, to_read); + if (err) + break; + + lnum += 1; + offs = 0; + bytes_left -= to_read; + val += to_read; + } + ubi_close_volume(desc); + + if (err) + return err; + + return bytes_left == 0 ? 0 : -EIO; +} + +static int ubi_nvmem_add(struct ubi_volume_info *vi) +{ + struct device_node *np = dev_of_node(vi->dev); + struct nvmem_config config = {}; + struct ubi_nvmem *unv; + int ret; + + if (!np) + return 0; + + if (!of_get_child_by_name(np, "nvmem-layout")) + return 0; + + if (WARN_ON_ONCE(vi->usable_leb_size <= 0) || + WARN_ON_ONCE(vi->size <= 0)) + return -EINVAL; + + unv = kzalloc(sizeof(struct ubi_nvmem), GFP_KERNEL); + if (!unv) + return -ENOMEM; + + config.id = NVMEM_DEVID_NONE; + config.dev = vi->dev; + config.name = dev_name(vi->dev); + config.owner = THIS_MODULE; + config.priv = unv; + config.reg_read = ubi_nvmem_reg_read; + config.size = vi->usable_leb_size * vi->size; + config.word_size = 1; + config.stride = 1; + config.read_only = true; + config.root_only = true; + config.ignore_wp = true; + config.of_node = np; + + unv->ubi_num = vi->ubi_num; + unv->vol_id = vi->vol_id; + unv->usable_leb_size = vi->usable_leb_size; + unv->nvmem = nvmem_register(&config); + if (IS_ERR(unv->nvmem)) { + ret = dev_err_probe(vi->dev, PTR_ERR(unv->nvmem), + "Failed to register NVMEM device\n"); + kfree(unv); + return ret; + } + + mutex_lock(&devices_mutex); + list_add_tail(&unv->list, &nvmem_devices); + mutex_unlock(&devices_mutex); + + return 0; +} + +static void ubi_nvmem_remove(struct ubi_volume_info *vi) +{ + struct ubi_nvmem *unv_c, *unv = NULL; + + mutex_lock(&devices_mutex); + list_for_each_entry(unv_c, &nvmem_devices, list) + if (unv_c->ubi_num == vi->ubi_num && unv_c->vol_id == vi->vol_id) { + unv = unv_c; + break; + } + + if (!unv) { + mutex_unlock(&devices_mutex); + return; + } + + list_del(&unv->list); + mutex_unlock(&devices_mutex); + nvmem_unregister(unv->nvmem); + kfree(unv); +} + +/** + * nvmem_notify - UBI notification handler. + * @nb: registered notifier block + * @l: notification type + * @ns_ptr: pointer to the &struct ubi_notification object + */ +static int nvmem_notify(struct notifier_block *nb, unsigned long l, + void *ns_ptr) +{ + struct ubi_notification *nt = ns_ptr; + + switch (l) { + case UBI_VOLUME_RESIZED: + ubi_nvmem_remove(&nt->vi); + fallthrough; + case UBI_VOLUME_ADDED: + ubi_nvmem_add(&nt->vi); + break; + case UBI_VOLUME_SHUTDOWN: + ubi_nvmem_remove(&nt->vi); + break; + default: + break; + } + return NOTIFY_OK; +} + +static struct notifier_block nvmem_notifier = { + .notifier_call = nvmem_notify, +}; + +static int __init ubi_nvmem_init(void) +{ + return ubi_register_volume_notifier(&nvmem_notifier, 0); +} + +static void __exit ubi_nvmem_exit(void) +{ + struct ubi_nvmem *unv, *tmp; + + mutex_lock(&devices_mutex); + list_for_each_entry_safe(unv, tmp, &nvmem_devices, list) { + nvmem_unregister(unv->nvmem); + list_del(&unv->list); + kfree(unv); + } + mutex_unlock(&devices_mutex); + + ubi_unregister_volume_notifier(&nvmem_notifier); +} + +module_init(ubi_nvmem_init); +module_exit(ubi_nvmem_exit); +MODULE_DESCRIPTION("NVMEM layer over UBI volumes"); +MODULE_AUTHOR("Daniel Golle"); +MODULE_LICENSE("GPL");
In an ideal world we would like UBI to be used where ever possible on a NAND chip. And with UBI support in ARM Trusted Firmware and U-Boot it is possible to achieve an (almost-)all-UBI flash layout. Hence the need for a way to also use UBI volumes to store board-level constants, such as MAC addresses and calibration data of wireless interfaces. Add UBI volume NVMEM driver module exposing UBI volumes as NVMEM providers. Allow UBI devices to have a "volumes" firmware subnode with volumes which may be compatible with "nvmem-cells". Access to UBI volumes via the NVMEM interface at this point is read-only, and it is slow, opening and closing the UBI volume for each access due to limitations of the NVMEM provider API. Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- drivers/mtd/ubi/Kconfig | 12 +++ drivers/mtd/ubi/Makefile | 1 + drivers/mtd/ubi/nvmem.c | 188 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 201 insertions(+) create mode 100644 drivers/mtd/ubi/nvmem.c