diff mbox series

[1/2] mtd: call of_platform_populate() for MTD partitions

Message ID 20220406143225.28107-1-zajec5@gmail.com
State New
Headers show
Series [1/2] mtd: call of_platform_populate() for MTD partitions | expand

Commit Message

Rafał Miłecki April 6, 2022, 2:32 p.m. UTC 
From: Rafał Miłecki <rafal@milecki.pl>

Until this change MTD subsystem supported handling partitions only with
MTD partitions parsers. That's a specific / limited API designed around
partitions.

Some MTD partitions may however require different handling. They may
contain specific data that needs to be parsed and somehow extracted. For
that purpose MTD subsystem should allow binding of standard platform
drivers.

An example can be U-Boot (sub)partition with environment variables.
There exist a "u-boot,env" DT binding for MTD (sub)partition that
requires an NVMEM driver.

Ref: 5db1c2dbc04c ("dt-bindings: nvmem: add U-Boot environment variables binding")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/mtdpart.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Miquel Raynal April 11, 2022, 9 a.m. UTC | #1 
On Wed, 2022-04-06 at 14:32:24 UTC, =?utf-8?b?UmFmYcWCIE1pxYJlY2tp?= wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Until this change MTD subsystem supported handling partitions only with
> MTD partitions parsers. That's a specific / limited API designed around
> partitions.
> 
> Some MTD partitions may however require different handling. They may
> contain specific data that needs to be parsed and somehow extracted. For
> that purpose MTD subsystem should allow binding of standard platform
> drivers.
> 
> An example can be U-Boot (sub)partition with environment variables.
> There exist a "u-boot,env" DT binding for MTD (sub)partition that
> requires an NVMEM driver.
> 
> Ref: 5db1c2dbc04c ("dt-bindings: nvmem: add U-Boot environment variables binding")
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel
Srinivas Kandagatla April 11, 2022, 10:18 a.m. UTC | #2 
On 06/04/2022 15:32, Rafał Miłecki wrote:
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 555aa77a574d..17a78b1ba077 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -324,4 +324,16 @@ config NVMEM_SUNPLUS_OCOTP
>   	  This driver can also be built as a module. If so, the module
>   	  will be called nvmem-sunplus-ocotp.
>   
> +config NVMEM_U_BOOT_ENV
> +	tristate "U-Boot environment variables support"
> +	depends on ARCH_BCM4908 || COMPILE_TEST

This nvmem provider seems more generic, so why ARCH_BCM4908 dependency here?


--srini
> +	depends on OF && MTD
> +	select CRC32
> +	help
> +	  U-Boot stores its setup as environment variables. This driver adds
> +	  support for verifying & exporting such data. It also exposes variables
> +	  as NVMEM cells so they can be referenced by other drivers.
> +
> +	  If compiled as module it will be called nvmem_u-boot-env.
> +
>   endif
Miquel Raynal April 25, 2022, 8:39 a.m. UTC | #3 
Hi Daniel,

daniel@makrotopia.org wrote on Mon, 25 Apr 2022 02:20:34 +0100:

> Hi Rafal,
> Hi Miguel,
> 
> 
> On Mon, Apr 11, 2022 at 11:00:32AM +0200, Miquel Raynal wrote:
> > On Wed, 2022-04-06 at 14:32:24 UTC, =?utf-8?b?UmFmYcWCIE1pxYJlY2tp?= wrote:  
> > > From: Rafał Miłecki <rafal@milecki.pl>
> > > 
> > > Until this change MTD subsystem supported handling partitions only with
> > > MTD partitions parsers. That's a specific / limited API designed around
> > > partitions.
> > > 
> > > Some MTD partitions may however require different handling. They may
> > > contain specific data that needs to be parsed and somehow extracted. For
> > > that purpose MTD subsystem should allow binding of standard platform
> > > drivers.
> > > 
> > > An example can be U-Boot (sub)partition with environment variables.
> > > There exist a "u-boot,env" DT binding for MTD (sub)partition that
> > > requires an NVMEM driver.
> > > 
> > > Ref: 5db1c2dbc04c ("dt-bindings: nvmem: add U-Boot environment variables binding")
> > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl>  
> > 
> > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.  
> 
> I'm trying to use next-20220422 and noticed a few new oops'es.
> Turns out it could be a problem with this commit according to
> 
> [daniel@box linux.git]$ git bisect good
> 68471517e883902cdff6ea399d043b17f803b1a8 is the first bad commit
> commit 68471517e883902cdff6ea399d043b17f803b1a8
> Author: Rafał Miłecki <rafal@milecki.pl>
> Date:   Wed Apr 6 16:32:24 2022 +0200
> 
>     mtd: call of_platform_populate() for MTD partitions
> [...]
> ---
> 
> So when ever there is at least one 'compatible' node for any of the
> mtd partitions I get the oops messages below. It doesn't really matter
> what the compatible string is, "nvmem-cells" as well as "denx,fit"
> (used for OpenWrt mtdsplit not even present in linux-next, so just a
> dead hint in DTS) make the kernel to oops.
> 
> Despite the messages being shown, both accessing MTD partitions and
> also eth0 MAC address populated via NVMEM seem to work without
> problems (at least looks like it on first sight).
> 
> Find the full device tree here:
> 
> https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/dts/mt7622-ubnt-unifi-6-lr-ubootmod.dts

Even though these compatibles are not mainline, it feels like the
problem exists here as well. I'm dropping this change for now, let's
fix this first.

> 
> ---
> [...]
> [    0.549448] mtk-spi-nor 11014000.spi: IRQ not available.
> [    0.556396] spi-nor spi0.0: w25q512jvq (65536 Kbytes)
> [    0.933381] Freeing initrd memory: 2124K
> [    0.941567] 7 fixed-partitions partitions found on MTD device spi0.0
> [    0.947966] OF: Bad cell count for /spi@11014000/flash@0/partitions
> [    0.954286] OF: Bad cell count for /spi@11014000/flash@0/partitions
> [    0.960583] ------------[ cut here ]------------
> [    0.965192] kobject: '(null)' (0000000097a89bbf): is not initialized, yet kobject_get() is being called.
> [    0.974688] WARNING: CPU: 0 PID: 1 at kobject_get+0x68/0x94
> [    0.980263] Modules linked in:
> [    0.983313] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S                5.18.0-rc1+ #0
> [    0.991049] Hardware name: Ubiquiti UniFi 6 LR (U-Boot mod) (DT)
> [    0.997046] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    1.004000] pc : kobject_get+0x68/0x94
> [    1.007743] lr : kobject_get+0x68/0x94
> [    1.011484] sp : ffffffc008bdb4a0
> [    1.014789] x29: ffffffc008bdb4a0 x28: 0000000000000000 x27: ffffff8005c57810
> [    1.021920] x26: ffffff8005c74a20 x25: 0000000000000000 x24: 0000000000000001
> [    1.029050] x23: 0000000000000000 x22: 0000000000000000 x21: 0000000000000000
> [    1.036182] x20: ffffff8005c74a20 x19: ffffff8005c74a20 x18: ffffffc008ab9630
> [    1.043312] x17: 6f6b20746579202c x16: 64657a696c616974 x15: 0000000000000074
> [    1.050443] x14: 000000000000015c x13: 0000000000000074 x12: 00000000ffffffea
> [    1.057574] x11: 00000000ffffefff x10: 00000000ffffefff x9 : ffffffc008b11630
> [    1.064705] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 0000000000057fa8
> [    1.071835] x5 : 0000000000000fff x4 : 0000000000000000 x3 : ffffffc008bdb1c0
> [    1.078966] x2 : 0000000000000000 x1 : ffffffc008ab9580 x0 : 000000000000005c
> [    1.086097] Call trace:
> [    1.088534]  kobject_get+0x68/0x94
> [    1.091930]  device_add+0xa4/0x840
> [    1.095328]  of_device_add+0x4c/0x5c
> [    1.098898]  of_platform_device_create_pdata+0xb8/0xf0
> [    1.104029]  of_platform_bus_create+0x104/0x350
> [    1.108552]  of_platform_populate+0x54/0xe0
> [    1.112728]  parse_mtd_partitions+0x430/0x490
> [    1.117080]  mtd_device_parse_register+0x90/0x2b0
> [    1.121777]  spi_nor_probe+0x1f8/0x2b0
> [    1.125521]  spi_mem_probe+0x68/0xa0
> [    1.129092]  spi_probe+0x80/0xdc
> [    1.132314]  really_probe.part.0+0x98/0x280
> [    1.136490]  __driver_probe_device+0x94/0x140
> [    1.140839]  driver_probe_device+0x40/0x114
> [    1.145014]  __device_attach_driver+0xb0/0x10c
> [    1.149450]  bus_for_each_drv+0x64/0xa0
> [    1.153280]  __device_attach+0xa8/0x16c
> [    1.157108]  device_initial_probe+0x10/0x20
> [    1.161283]  bus_probe_device+0x94/0x9c
> [    1.165114]  device_add+0x35c/0x840
> [    1.168596]  __spi_add_device+0x70/0x114
> [    1.172510]  spi_add_device+0x5c/0x90
> [    1.176165]  of_register_spi_device+0x204/0x330
> [    1.180688]  spi_register_controller+0x3e8/0x6dc
> [    1.185299]  devm_spi_register_controller+0x20/0x74
> [    1.190169]  mtk_nor_probe+0x344/0x4c0
> [    1.193911]  platform_probe+0x64/0xd0
> [    1.197566]  really_probe.part.0+0x98/0x280
> [    1.201741]  __driver_probe_device+0x94/0x140
> [    1.206089]  driver_probe_device+0x40/0x114
> [    1.210264]  __driver_attach+0xf0/0x180
> [    1.214092]  bus_for_each_dev+0x5c/0x90
> [    1.217922]  driver_attach+0x20/0x2c
> [    1.221491]  bus_add_driver+0x140/0x1ec
> [    1.225320]  driver_register+0x74/0x120
> [    1.229149]  __platform_driver_register+0x24/0x30
> [    1.233845]  mtk_nor_driver_init+0x18/0x20
> [    1.237938]  do_one_initcall+0x4c/0x1c0
> [    1.241768]  kernel_init_freeable+0x230/0x294
> [    1.246120]  kernel_init+0x20/0x120
> [    1.249601]  ret_from_fork+0x10/0x20
> [    1.253170] ---[ end trace 0000000000000000 ]---
> [    1.257796] ------------[ cut here ]------------


Thanks,
Miquèl
Rafał Miłecki May 4, 2022, 1:40 p.m. UTC | #4 
On 25.04.2022 03:20, Daniel Golle wrote:
> On Mon, Apr 11, 2022 at 11:00:32AM +0200, Miquel Raynal wrote:
>> On Wed, 2022-04-06 at 14:32:24 UTC, =?utf-8?b?UmFmYcWCIE1pxYJlY2tp?= wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Until this change MTD subsystem supported handling partitions only with
>>> MTD partitions parsers. That's a specific / limited API designed around
>>> partitions.
>>>
>>> Some MTD partitions may however require different handling. They may
>>> contain specific data that needs to be parsed and somehow extracted. For
>>> that purpose MTD subsystem should allow binding of standard platform
>>> drivers.
>>>
>>> An example can be U-Boot (sub)partition with environment variables.
>>> There exist a "u-boot,env" DT binding for MTD (sub)partition that
>>> requires an NVMEM driver.
>>>
>>> Ref: 5db1c2dbc04c ("dt-bindings: nvmem: add U-Boot environment variables binding")
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>
>> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.
> 
> I'm trying to use next-20220422 and noticed a few new oops'es.
> Turns out it could be a problem with this commit according to
> 
> [daniel@box linux.git]$ git bisect good
> 68471517e883902cdff6ea399d043b17f803b1a8 is the first bad commit
> commit 68471517e883902cdff6ea399d043b17f803b1a8
> Author: Rafał Miłecki <rafal@milecki.pl>
> Date:   Wed Apr 6 16:32:24 2022 +0200
> 
>      mtd: call of_platform_populate() for MTD partitions
> [...]
> ---
> 
> So when ever there is at least one 'compatible' node for any of the
> mtd partitions I get the oops messages below. It doesn't really matter
> what the compatible string is, "nvmem-cells" as well as "denx,fit"
> (used for OpenWrt mtdsplit not even present in linux-next, so just a
> dead hint in DTS) make the kernel to oops.
> 
> Despite the messages being shown, both accessing MTD partitions and
> also eth0 MAC address populated via NVMEM seem to work without
> problems (at least looks like it on first sight).
> 
> Find the full device tree here:
> 
> https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/dts/mt7622-ubnt-unifi-6-lr-ubootmod.dts

I found it! It used to happen (before dropping patch) with:
# CONFIG_MTD_PARTITIONED_MASTER is not set

I'll work on V2 which doesn't require
CONFIG_MTD_PARTITIONED_MASTER=y
diff mbox series

Patch

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 357661b62c94..9fce946fa69c 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -17,6 +17,7 @@ 
 #include <linux/mtd/partitions.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 
 #include "mtdcore.h"
 
@@ -593,6 +594,7 @@  static int mtd_part_of_parse(struct mtd_info *master,
 			continue;
 		ret = mtd_part_do_parse(parser, master, pparts, NULL);
 		if (ret > 0) {
+			of_platform_populate(np, NULL, NULL, &master->dev);
 			of_node_put(np);
 			return ret;
 		}
@@ -600,6 +602,7 @@  static int mtd_part_of_parse(struct mtd_info *master,
 		if (ret < 0 && !err)
 			err = ret;
 	}
+	of_platform_populate(np, NULL, NULL, &master->dev);
 	of_node_put(np);
 
 	/*