Message ID | 20220606151417.19227-1-ansuelsmth@gmail.com |
---|---|
Headers | show |
Series | Add nvmem support for dynamic partitions | expand |
On Thu, Jun 09, 2022 at 12:32:52PM -0600, Rob Herring wrote: > On Mon, Jun 06, 2022 at 05:14:15PM +0200, Ansuel Smith wrote: > > Document new partition nodes that declare only the label instead of the > > reg used to provide an OF node for partition registred at runtime by > > parsers. This is required for nvmem system to declare and detect > > nvmem-cells. > > > > With these special partitions, the reg / offset is not required. > > The label binding is used to match the partition allocated by the > > parser at runtime and the parser will provide reg and offset of the mtd. > > > > NVMEM will use the data from the parser and provide the NVMEM cells > > declared in the DTS, "connecting" the dynamic partition with a > > static declaration of cells in them. > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > .../bindings/mtd/partitions/partition.yaml | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/partition.yaml b/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > > index e1ac08064425..bff6fb980e6b 100644 > > --- a/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > > +++ b/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > > @@ -11,6 +11,13 @@ description: | > > relative offset and size specified. Depending on partition function extra > > properties can be used. > > > > + A partition may be dynamically allocated by a specific parser at runtime. > > + In this specific case, the label is required instead of the reg. > > + This is used to assign an OF node to the dynamiccally allocated partition > > + so that subsystem like NVMEM can provide an OF node and declare NVMEM cells. > > + The OF node will be assigned only if the partition label declared match the > > + one assigned by the parser at runtime. > > + > > maintainers: > > - Rafał Miłecki <rafal@milecki.pl> > > > > @@ -22,6 +29,8 @@ properties: > > label: > > description: The label / name for this partition. If omitted, the label > > is taken from the node name (excluding the unit address). > > + With dynamically allocated partition the label is required and won't > > + fallback to the node name. > > Generally, label is never required being something for humans rather > than the s/w to consume. I don't see any reason why we can't still use > the node name (with 'partition-' stripped off). > How to enforce the use of 'partition-'? Should the driver then check the node name and reject any wrong node name (and return error)? > If the purpose is to define what the partition contains, then > 'compatible' is the right thing for that. > Introducing a compatible means creating another scheme I think or we can add that special compatible in the partition scheme? > > > > read-only: > > description: This parameter, if present, is a hint that this partition > > @@ -41,7 +50,10 @@ properties: > > immune to paired-pages corruptions > > type: boolean > > > > -required: > > - - reg > > +if: > > + not: > > + required: [ reg ] > > +then: > > + required: [ label ] > > > > additionalProperties: true > > -- > > 2.36.1 > > > >
On Fri, Jun 10, 2022 at 11:02:05AM -0600, Rob Herring wrote: > On Thu, Jun 09, 2022 at 09:57:52PM +0200, Ansuel Smith wrote: > > On Thu, Jun 09, 2022 at 12:32:52PM -0600, Rob Herring wrote: > > > On Mon, Jun 06, 2022 at 05:14:15PM +0200, Ansuel Smith wrote: > > > > Document new partition nodes that declare only the label instead of the > > > > reg used to provide an OF node for partition registred at runtime by > > > > parsers. This is required for nvmem system to declare and detect > > > > nvmem-cells. > > > > > > > > With these special partitions, the reg / offset is not required. > > > > The label binding is used to match the partition allocated by the > > > > parser at runtime and the parser will provide reg and offset of the mtd. > > > > > > > > NVMEM will use the data from the parser and provide the NVMEM cells > > > > declared in the DTS, "connecting" the dynamic partition with a > > > > static declaration of cells in them. > > > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > --- > > > > .../bindings/mtd/partitions/partition.yaml | 16 ++++++++++++++-- > > > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/partition.yaml b/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > > > > index e1ac08064425..bff6fb980e6b 100644 > > > > --- a/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > > > > +++ b/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > > > > @@ -11,6 +11,13 @@ description: | > > > > relative offset and size specified. Depending on partition function extra > > > > properties can be used. > > > > > > > > + A partition may be dynamically allocated by a specific parser at runtime. > > > > + In this specific case, the label is required instead of the reg. > > > > + This is used to assign an OF node to the dynamiccally allocated partition > > > > + so that subsystem like NVMEM can provide an OF node and declare NVMEM cells. > > > > + The OF node will be assigned only if the partition label declared match the > > > > + one assigned by the parser at runtime. > > > > + > > > > maintainers: > > > > - Rafał Miłecki <rafal@milecki.pl> > > > > > > > > @@ -22,6 +29,8 @@ properties: > > > > label: > > > > description: The label / name for this partition. If omitted, the label > > > > is taken from the node name (excluding the unit address). > > > > + With dynamically allocated partition the label is required and won't > > > > + fallback to the node name. > > > > > > Generally, label is never required being something for humans rather > > > than the s/w to consume. I don't see any reason why we can't still use > > > the node name (with 'partition-' stripped off). > > > > > > > How to enforce the use of 'partition-'? Should the driver then check the > > node name and reject any wrong node name (and return error)? > > The schema can do it either in the parent (of partition nodes) schema or > with $nodename 'property'. > > $nodename: > oneOf: > - pattern: '^.*@.*$' > - pattern: '^partition-.*$' > > or: > > if: > not: > required: > - reg > then: > properties: > $nodename: > pattern: '^partition-.*$' > > > The latter is a bit clearer on the intent I think. > > > > If the purpose is to define what the partition contains, then > > > 'compatible' is the right thing for that. > > > > > > > Introducing a compatible means creating another scheme I think or we can > > add that special compatible in the partition scheme? > > It would be another schema. You could make 'compatible' required here > perhaps, but maybe there's a use for an empty node? > > Rob Think I will just add the $nodename properties just to now complicate things with another scheme.
On 22/06/06 05:14PM, Ansuel Smith wrote: > We have many parser that register mtd partitions at runtime. One example > is the cmdlinepart or the smem-part parser where the compatible is defined > in the dts and the partitions gets detected and registered by the > parser. This is problematic for the NVMEM subsystem that requires an OF node > to detect NVMEM cells. > > To fix this problem, introduce an additional logic that will try to > assign an OF node to the MTD if declared. > > On MTD addition, it will be checked if the MTD has an OF node and if > not declared will check if a partition with the same label is > declared in DTS. If an exact match is found, the partition dynamically > allocated by the parser will have a connected OF node. > > The NVMEM subsystem will detect the OF node and register any NVMEM cells > declared statically in the DTS. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/mtd/mtdcore.c | 49 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 7731796024e0..807194efb580 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -546,6 +546,54 @@ static int mtd_nvmem_add(struct mtd_info *mtd) > return 0; > } > > +static void mtd_check_of_node(struct mtd_info *mtd) > +{ > + struct device_node *partitions, *parent_dn, *mtd_dn = NULL; > + struct mtd_info *parent; > + const char *mtd_name; > + bool found = false; > + int plen; > + > + /* Check if MTD already has a device node */ > + if (dev_of_node(&mtd->dev)) > + return; > + > + /* Check if a partitions node exist */ > + parent = mtd->parent; > + parent_dn = dev_of_node(&parent->dev); > + if (!parent_dn) > + return; > + > + partitions = of_get_child_by_name(parent_dn, "partitions"); > + if (!partitions) > + goto exit_parent; > + > + /* Search if a partition is defined with the same name */ > + for_each_child_of_node(partitions, mtd_dn) { > + /* Skip partition with no label */ > + mtd_name = of_get_property(mtd_dn, "label", &plen); > + if (!mtd_name) > + continue; > + > + if (!strncmp(mtd->name, mtd_name, plen)) { > + found = true; > + break; > + } > + } > + > + if (!found) > + goto exit_partitions; > + > + /* Set of_node only for nvmem */ > + if (of_device_is_compatible(mtd_dn, "nvmem-cells")) > + mtd_set_of_node(mtd, mtd_dn); > + > +exit_partitions: > + of_node_put(partitions); > +exit_parent: > + of_node_put(parent_dn); > +} > + > /** > * add_mtd_device - register an MTD device > * @mtd: pointer to new MTD device info structure > @@ -651,6 +699,7 @@ int add_mtd_device(struct mtd_info *mtd) > mtd->dev.devt = MTD_DEVT(i); > dev_set_name(&mtd->dev, "mtd%d", i); > dev_set_drvdata(&mtd->dev, mtd); > + mtd_check_of_node(mtd); > of_node_get(mtd_get_of_node(mtd)); > error = device_register(&mtd->dev); > if (error) NXP GPMI NAND controller with 6 cmdline partitions meets refcount underflow with this patch. The of_node_put(parent_dn) doesn't work with cmdline parser and causes the issue. [ 1.078910] 6 cmdlinepart partitions found on MTD device gpmi-nand [ 1.085116] Creating 6 MTD partitions on "gpmi-nand": [ 1.090181] 0x000000000000-0x000008000000 : "nandboot" [ 1.096952] 0x000008000000-0x000009000000 : "nandfit" [ 1.103547] 0x000009000000-0x00000b000000 : "nandkernel" [ 1.110317] 0x00000b000000-0x00000c000000 : "nanddtb" [ 1.115525] ------------[ cut here ]------------ [ 1.120141] refcount_t: addition on 0; use-after-free. [ 1.125328] WARNING: CPU: 0 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0xdc/0x148 [ 1.133528] Modules linked in: [ 1.136589] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc7-next-20220930-04543-g8cf3f7 [ 1.146342] Hardware name: Freescale i.MX8DXL DDR3L EVK (DT) [ 1.151999] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 1.158965] pc : refcount_warn_saturate+0xdc/0x148 [ 1.163760] lr : refcount_warn_saturate+0xdc/0x148 [ 1.168556] sp : ffff800009ddb080 [ 1.171866] x29: ffff800009ddb080 x28: ffff800009ddb35a x27: 0000000000000002 [ 1.179015] x26: ffff8000098b06ad x25: ffffffffffffffff x24: ffff0a00ffffff05 [ 1.186165] x23: ffff00001fdf6470 x22: ffff800009ddb367 x21: 0000000000000000 [ 1.193314] x20: ffff00001fdfebe8 x19: ffff00001fdfec50 x18: ffffffffffffffff [ 1.200464] x17: 0000000000000000 x16: 0000000000000118 x15: 0000000000000004 [ 1.207614] x14: 0000000000000fff x13: ffff800009bca248 x12: 0000000000000003 [ 1.214764] x11: 00000000ffffefff x10: c0000000ffffefff x9 : 4762cb2ccb52de00 [ 1.221914] x8 : 4762cb2ccb52de00 x7 : 205d313431303231 x6 : 312e31202020205b [ 1.229063] x5 : ffff800009d55c1f x4 : 0000000000000001 x3 : 0000000000000000 [ 1.236213] x2 : 0000000000000000 x1 : ffff800009954be6 x0 : 000000000000002a [ 1.243365] Call trace: [ 1.245806] refcount_warn_saturate+0xdc/0x148 [ 1.250253] kobject_get+0x98/0x9c [ 1.253658] of_node_get+0x20/0x34 [ 1.257072] of_fwnode_get+0x3c/0x54 [ 1.260652] fwnode_get_nth_parent+0xd8/0xf4 [ 1.264926] fwnode_full_name_string+0x3c/0xb4 [ 1.269373] device_node_string+0x498/0x5b4 [ 1.273561] pointer+0x41c/0x5d0 [ 1.276793] vsnprintf+0x4d8/0x694 [ 1.280198] vprintk_store+0x164/0x528 [ 1.283951] vprintk_emit+0x98/0x164 [ 1.287530] vprintk_default+0x44/0x6c [ 1.291284] vprintk+0xf0/0x134 [ 1.294428] _printk+0x54/0x7c [ 1.297486] of_node_release+0xe8/0x128 [ 1.301326] kobject_put+0x98/0xfc [ 1.304732] of_node_put+0x1c/0x28 [ 1.308137] add_mtd_device+0x484/0x6d4 [ 1.311977] add_mtd_partitions+0xf0/0x1d0 [ 1.316078] parse_mtd_partitions+0x45c/0x518 [ 1.320439] mtd_device_parse_register+0xb0/0x274 [ 1.325147] gpmi_nand_probe+0x51c/0x650 [ 1.329074] platform_probe+0xa8/0xd0 [ 1.332740] really_probe+0x130/0x334 [ 1.336406] __driver_probe_device+0xb4/0xe0 [ 1.340681] driver_probe_device+0x3c/0x1f8 [ 1.344869] __driver_attach+0xdc/0x1a4 [ 1.348708] bus_for_each_dev+0x80/0xcc [ 1.352548] driver_attach+0x24/0x30 [ 1.356127] bus_add_driver+0x108/0x1f4 [ 1.359967] driver_register+0x78/0x114 [ 1.363807] __platform_driver_register+0x24/0x30 [ 1.368515] gpmi_nand_driver_init+0x1c/0x28 [ 1.372798] do_one_initcall+0xbc/0x238 [ 1.376638] do_initcall_level+0x94/0xb4 [ 1.380565] do_initcalls+0x54/0x94 [ 1.384058] do_basic_setup+0x1c/0x28 [ 1.387724] kernel_init_freeable+0x110/0x188 [ 1.392084] kernel_init+0x20/0x1a0 [ 1.395578] ret_from_fork+0x10/0x20 [ 1.399157] ---[ end trace 0000000000000000 ]--- [ 1.403782] ------------[ cut here ]------------ > -- > 2.36.1 > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 17.10.2022 21:59, han.xu wrote: > On 22/06/06 05:14PM, Ansuel Smith wrote: >> We have many parser that register mtd partitions at runtime. One example >> is the cmdlinepart or the smem-part parser where the compatible is defined >> in the dts and the partitions gets detected and registered by the >> parser. This is problematic for the NVMEM subsystem that requires an OF node >> to detect NVMEM cells. >> >> To fix this problem, introduce an additional logic that will try to >> assign an OF node to the MTD if declared. >> >> On MTD addition, it will be checked if the MTD has an OF node and if >> not declared will check if a partition with the same label is >> declared in DTS. If an exact match is found, the partition dynamically >> allocated by the parser will have a connected OF node. >> >> The NVMEM subsystem will detect the OF node and register any NVMEM cells >> declared statically in the DTS. >> >> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> >> --- >> drivers/mtd/mtdcore.c | 49 +++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c >> index 7731796024e0..807194efb580 100644 >> --- a/drivers/mtd/mtdcore.c >> +++ b/drivers/mtd/mtdcore.c >> @@ -546,6 +546,54 @@ static int mtd_nvmem_add(struct mtd_info *mtd) >> return 0; >> } >> >> +static void mtd_check_of_node(struct mtd_info *mtd) >> +{ >> + struct device_node *partitions, *parent_dn, *mtd_dn = NULL; >> + struct mtd_info *parent; >> + const char *mtd_name; >> + bool found = false; >> + int plen; >> + >> + /* Check if MTD already has a device node */ >> + if (dev_of_node(&mtd->dev)) >> + return; >> + >> + /* Check if a partitions node exist */ >> + parent = mtd->parent; >> + parent_dn = dev_of_node(&parent->dev); >> + if (!parent_dn) >> + return; >> + >> + partitions = of_get_child_by_name(parent_dn, "partitions"); >> + if (!partitions) >> + goto exit_parent; >> + >> + /* Search if a partition is defined with the same name */ >> + for_each_child_of_node(partitions, mtd_dn) { >> + /* Skip partition with no label */ >> + mtd_name = of_get_property(mtd_dn, "label", &plen); >> + if (!mtd_name) >> + continue; >> + >> + if (!strncmp(mtd->name, mtd_name, plen)) { >> + found = true; >> + break; >> + } >> + } >> + >> + if (!found) >> + goto exit_partitions; >> + >> + /* Set of_node only for nvmem */ >> + if (of_device_is_compatible(mtd_dn, "nvmem-cells")) >> + mtd_set_of_node(mtd, mtd_dn); >> + >> +exit_partitions: >> + of_node_put(partitions); >> +exit_parent: >> + of_node_put(parent_dn); >> +} >> + >> /** >> * add_mtd_device - register an MTD device >> * @mtd: pointer to new MTD device info structure >> @@ -651,6 +699,7 @@ int add_mtd_device(struct mtd_info *mtd) >> mtd->dev.devt = MTD_DEVT(i); >> dev_set_name(&mtd->dev, "mtd%d", i); >> dev_set_drvdata(&mtd->dev, mtd); >> + mtd_check_of_node(mtd); >> of_node_get(mtd_get_of_node(mtd)); >> error = device_register(&mtd->dev); >> if (error) > > NXP GPMI NAND controller with 6 cmdline partitions meets refcount underflow > with this patch. The of_node_put(parent_dn) doesn't work with cmdline parser and > causes the issue. Can you try: diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 18aa54460d36..0b4ca0aa4132 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -562,7 +562,7 @@ static void mtd_check_of_node(struct mtd_info *mtd) if (!mtd_is_partition(mtd)) return; parent = mtd->parent; - parent_dn = dev_of_node(&parent->dev); + parent_dn = of_node_get(dev_of_node(&parent->dev)); if (!parent_dn) return;
On 22/10/17 11:52PM, Rafał Miłecki wrote: > On 17.10.2022 21:59, han.xu wrote: > > On 22/06/06 05:14PM, Ansuel Smith wrote: > > > We have many parser that register mtd partitions at runtime. One example > > > is the cmdlinepart or the smem-part parser where the compatible is defined > > > in the dts and the partitions gets detected and registered by the > > > parser. This is problematic for the NVMEM subsystem that requires an OF node > > > to detect NVMEM cells. > > > > > > To fix this problem, introduce an additional logic that will try to > > > assign an OF node to the MTD if declared. > > > > > > On MTD addition, it will be checked if the MTD has an OF node and if > > > not declared will check if a partition with the same label is > > > declared in DTS. If an exact match is found, the partition dynamically > > > allocated by the parser will have a connected OF node. > > > > > > The NVMEM subsystem will detect the OF node and register any NVMEM cells > > > declared statically in the DTS. > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > --- > > > drivers/mtd/mtdcore.c | 49 +++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 49 insertions(+) > > > > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > > > index 7731796024e0..807194efb580 100644 > > > --- a/drivers/mtd/mtdcore.c > > > +++ b/drivers/mtd/mtdcore.c > > > @@ -546,6 +546,54 @@ static int mtd_nvmem_add(struct mtd_info *mtd) > > > return 0; > > > } > > > +static void mtd_check_of_node(struct mtd_info *mtd) > > > +{ > > > + struct device_node *partitions, *parent_dn, *mtd_dn = NULL; > > > + struct mtd_info *parent; > > > + const char *mtd_name; > > > + bool found = false; > > > + int plen; > > > + > > > + /* Check if MTD already has a device node */ > > > + if (dev_of_node(&mtd->dev)) > > > + return; > > > + > > > + /* Check if a partitions node exist */ > > > + parent = mtd->parent; > > > + parent_dn = dev_of_node(&parent->dev); > > > + if (!parent_dn) > > > + return; > > > + > > > + partitions = of_get_child_by_name(parent_dn, "partitions"); > > > + if (!partitions) > > > + goto exit_parent; > > > + > > > + /* Search if a partition is defined with the same name */ > > > + for_each_child_of_node(partitions, mtd_dn) { > > > + /* Skip partition with no label */ > > > + mtd_name = of_get_property(mtd_dn, "label", &plen); > > > + if (!mtd_name) > > > + continue; > > > + > > > + if (!strncmp(mtd->name, mtd_name, plen)) { > > > + found = true; > > > + break; > > > + } > > > + } > > > + > > > + if (!found) > > > + goto exit_partitions; > > > + > > > + /* Set of_node only for nvmem */ > > > + if (of_device_is_compatible(mtd_dn, "nvmem-cells")) > > > + mtd_set_of_node(mtd, mtd_dn); > > > + > > > +exit_partitions: > > > + of_node_put(partitions); > > > +exit_parent: > > > + of_node_put(parent_dn); > > > +} > > > + > > > /** > > > * add_mtd_device - register an MTD device > > > * @mtd: pointer to new MTD device info structure > > > @@ -651,6 +699,7 @@ int add_mtd_device(struct mtd_info *mtd) > > > mtd->dev.devt = MTD_DEVT(i); > > > dev_set_name(&mtd->dev, "mtd%d", i); > > > dev_set_drvdata(&mtd->dev, mtd); > > > + mtd_check_of_node(mtd); > > > of_node_get(mtd_get_of_node(mtd)); > > > error = device_register(&mtd->dev); > > > if (error) > > > > NXP GPMI NAND controller with 6 cmdline partitions meets refcount underflow > > with this patch. The of_node_put(parent_dn) doesn't work with cmdline parser and > > causes the issue. > > Can you try: > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 18aa54460d36..0b4ca0aa4132 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -562,7 +562,7 @@ static void mtd_check_of_node(struct mtd_info *mtd) > if (!mtd_is_partition(mtd)) > return; > parent = mtd->parent; > - parent_dn = dev_of_node(&parent->dev); > + parent_dn = of_node_get(dev_of_node(&parent->dev)); > if (!parent_dn) > return; Yes, with of_node_get() the refcount issue gone. > >