This very small series comes to fix the very annyoing problem of
partitions declared by parser at runtime NOT supporting nvmem cells
definition.
The current implementation is very generic. The idea is to provide an of
node if defined for everyone and not strictly limit this to nvmem stuff.
But still the actual change is done only for nvmem-cells mtd. (just to
make sure) This can totally change by removing the compatible check.
The idea here is that a user can still use these dynamic parsers
instead of declaring a fixed-partition and also declare how nvmem-cells
are defined for the partition.
This live with the assumption that dynamic partition have always the
same name and they are known. (this is the case for smem-part partition
that would require a bootloader reflash to change and for parsers like
cmdlinepart where the name is always the same.)
With this assumption, it's easy to fix this problem. Just introduce a
new partition node that will declare just these special partition.
Mtdcore then will check if these special declaration are present and
connect the dynamic partition with the OF node present in the dts. Nvmem
will automagically find the OF node and cells will be works based on the
data provided by the parser.
The initial idea was to create a special nvmem driver with a special
compatible where a user would declare the mtd partition name and this
driver would search it and register the nvmem cells but that became
difficult really fast, mtd notifier system is problematic for this kind
of stuff. So here is the better implementation. A variant of this is
already tested on openwrt where we have devices that use cmdlinepart.
(that current variant have defined in the dts the exact copy of
cmdlinepart in the fixed-partition scheme and we patched the cmdlinepart
parser to scan this fixed-partition node (that is ignored as cmdlinepart
have priority) and connect the dynamic partition with the dts node)
I provided an example of this in the documentation commit.
In short it's needed to add to the partitions where the compatible parser
is declared, a partition with just the label declared (instead of the reg).
Then declare some nvmem-cells and it will all work at runtime.
Mtdcore will check if a node with the same label is present and assign an
OF node to the MTD.
I currently tested this on my device that have smem-part and the
gmac driver use nvmem to get the mac-address. This works correctly and
the same address is provided.
v5:
- Simplify Documentation and move it generic partition.yaml
- Split commit and move smem example to dedicated commit.
v4:
- Make it simple. No suffix. Make label mandatory again.
- Update Documentation with new implementation.
- Rename files to a better and correct name
v3:
- Fix warning from bot (function not declared as static)
- Updated code to support also node name
- Made partition label optional
v2:
- Simplify this. Drop dynamic-partition
- Fix problem with parser with ko
- Do not pollude mtd_get_of_node
- Fix problem with Documentation
Ansuel Smith (3):
dt-bindings: mtd: partitions: Support label only partition
dt-bindings: mtd: partitions: add additional example for
qcom,smem-part
mtd: core: introduce of support for dynamic partitions
.../bindings/mtd/partitions/partition.yaml | 16 +++++-
.../mtd/partitions/qcom,smem-part.yaml | 27 ++++++++++
drivers/mtd/mtdcore.c | 49 +++++++++++++++++++
3 files changed, 90 insertions(+), 2 deletions(-)
--
2.36.1
Add additional example for qcom,smem-part to declare a dynamic
partition to provide NVMEM cells for the commonly ART partition
provided by this parser.
Signed-off-by: Ansuel Smith <[email protected]>
---
.../mtd/partitions/qcom,smem-part.yaml | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/Documentation/devicetree/bindings/mtd/partitions/qcom,smem-part.yaml b/Documentation/devicetree/bindings/mtd/partitions/qcom,smem-part.yaml
index cf3f8c1e035d..dc07909af023 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/qcom,smem-part.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/qcom,smem-part.yaml
@@ -19,6 +19,10 @@ properties:
compatible:
const: qcom,smem-part
+patternProperties:
+ "^partition-[0-9a-z]+$":
+ $ref: partition.yaml#
+
required:
- compatible
@@ -31,3 +35,26 @@ examples:
compatible = "qcom,smem-part";
};
};
+
+ - |
+ /* Example declaring dynamic partition */
+ flash {
+ partitions {
+ compatible = "qcom,smem-part";
+
+ partition-art {
+ compatible = "nvmem-cells";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ label = "0:art";
+
+ macaddr_art_0: macaddr@0 {
+ reg = <0x0 0x6>;
+ };
+
+ macaddr_art_6: macaddr@6 {
+ reg = <0x6 0x6>;
+ };
+ };
+ };
+ };
--
2.36.1
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 <[email protected]>
---
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)
--
2.36.1
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: a2af0cae87a0205f2f4b89da6e628229a2f8c47f ("[PATCH v5 3/3] mtd: core: introduce of support for dynamic partitions")
url: https://github.com/intel-lab-lkp/linux/commits/Ansuel-Smith/Add-nvmem-support-for-dynamic-partitions/20220606-232755
base: https://git.kernel.org/cgit/linux/kernel/git/mtd/linux.git mtd/next
patch link: https://lore.kernel.org/linux-mtd/[email protected]
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+---------------------------------------------+------------+------------+
| | 96a45c785b | a2af0cae87 |
+---------------------------------------------+------------+------------+
| boot_successes | 25 | 0 |
| boot_failures | 0 | 12 |
| BUG:kernel_NULL_pointer_dereference,address | 0 | 12 |
| Oops:#[##] | 0 | 12 |
| EIP:add_mtd_device | 0 | 12 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 12 |
+---------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 2.820056][ T1] BUG: kernel NULL pointer dereference, address: 00000360
[ 2.821034][ T1] #PF: supervisor read access in kernel mode
[ 2.821862][ T1] #PF: error_code(0x0000) - not-present page
[ 2.822468][ T1] *pde = 00000000
[ 2.822468][ T1] Oops: 0000 [#1]
[ 2.822468][ T1] CPU: 0 PID: 1 Comm: swapper Not tainted 5.18.0-rc3-00074-ga2af0cae87a0 #1
[ 2.822468][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014
[ 2.822468][ T1] EIP: add_mtd_device+0x32d/0x6a0
[ 2.822468][ T1] Code: 01 ff ff ff 8d 50 ff 85 c2 0f 84 ec fe ff ff 31 c0 e9 ef fe ff ff 8d b4 26 00 00 00 00 8b 83 cc 03 00 00 3d 08 ff ff ff 74 0e <8b> 80 60 03 00 00 85 c0 0f 85 65 01 00 00 8b 7d 84 89 f8 e8 3b 61
[ 2.822468][ T1] EAX: 00000000 EBX: c6eaa000 ECX: 00000001 EDX: 00000000
[ 2.822468][ T1] ESI: 00000000 EDI: c6eaa0f8 EBP: c3f0becc ESP: c3f0be44
[ 2.822468][ T1] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010213
[ 2.822468][ T1] CR0: 80050033 CR2: 00000360 CR3: 0345a000 CR4: 000406d0
[ 2.822468][ T1] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 2.822468][ T1] DR6: fffe0ff0 DR7: 00000400
[ 2.822468][ T1] Call Trace:
[ 2.822468][ T1] ? parse_ofoldpart_partitions+0x60/0x60
[ 2.822468][ T1] ? mtd_part_of_parse+0x118/0x240
[ 2.822468][ T1] ? mtd_part_of_parse+0x20e/0x240
[ 2.822468][ T1] ? parse_mtd_partitions+0xfa/0x200
[ 2.822468][ T1] mtd_device_parse_register+0x19f/0x2b0
[ 2.822468][ T1] mtdram_init_device+0xb7/0x100
[ 2.822468][ T1] init_mtdram+0x67/0xaf
[ 2.822468][ T1] ? init_phram+0x54/0x54
[ 2.822468][ T1] do_one_initcall+0x58/0x240
[ 2.822468][ T1] ? rdinit_setup+0x38/0x38
[ 2.822468][ T1] do_initcalls+0xf3/0x112
[ 2.822468][ T1] kernel_init_freeable+0xbd/0xec
[ 2.822468][ T1] ? rest_init+0x100/0x100
[ 2.822468][ T1] kernel_init+0x12/0xf0
[ 2.822468][ T1] ret_from_fork+0x1c/0x30
[ 2.822468][ T1] Modules linked in:
[ 2.822468][ T1] CR2: 0000000000000360
[ 2.822468][ T1] ---[ end trace 0000000000000000 ]---
[ 2.822468][ T1] EIP: add_mtd_device+0x32d/0x6a0
[ 2.822468][ T1] Code: 01 ff ff ff 8d 50 ff 85 c2 0f 84 ec fe ff ff 31 c0 e9 ef fe ff ff 8d b4 26 00 00 00 00 8b 83 cc 03 00 00 3d 08 ff ff ff 74 0e <8b> 80 60 03 00 00 85 c0 0f 85 65 01 00 00 8b 7d 84 89 f8 e8 3b 61
[ 2.822468][ T1] EAX: 00000000 EBX: c6eaa000 ECX: 00000001 EDX: 00000000
[ 2.822468][ T1] ESI: 00000000 EDI: c6eaa0f8 EBP: c3f0becc ESP: c3f0be44
[ 2.822468][ T1] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010213
[ 2.822468][ T1] CR0: 80050033 CR2: 00000360 CR3: 0345a000 CR4: 000406d0
[ 2.822468][ T1] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 2.822468][ T1] DR6: fffe0ff0 DR7: 00000400
[ 2.822468][ T1] Kernel panic - not syncing: Fatal exception
[ 2.822468][ T1] Kernel Offset: disabled
To reproduce:
# build kernel
cd linux
cp config-5.18.0-rc3-00074-ga2af0cae87a0 .config
make HOSTCC=gcc-11 CC=gcc-11 ARCH=i386 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-11 CC=gcc-11 ARCH=i386 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Ansuel,
[email protected] wrote on Wed, 8 Jun 2022 23:44:04 +0800:
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-11):
>
> commit: a2af0cae87a0205f2f4b89da6e628229a2f8c47f ("[PATCH v5 3/3] mtd: core: introduce of support for dynamic partitions")
> url: https://github.com/intel-lab-lkp/linux/commits/Ansuel-Smith/Add-nvmem-support-for-dynamic-partitions/20220606-232755
> base: https://git.kernel.org/cgit/linux/kernel/git/mtd/linux.git mtd/next
> patch link: https://lore.kernel.org/linux-mtd/[email protected]
>
> in testcase: boot
>
> on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
>
> caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
Can you look into this?
Thanks,
Miquèl
>
>
> +---------------------------------------------+------------+------------+
> | | 96a45c785b | a2af0cae87 |
> +---------------------------------------------+------------+------------+
> | boot_successes | 25 | 0 |
> | boot_failures | 0 | 12 |
> | BUG:kernel_NULL_pointer_dereference,address | 0 | 12 |
> | Oops:#[##] | 0 | 12 |
> | EIP:add_mtd_device | 0 | 12 |
> | Kernel_panic-not_syncing:Fatal_exception | 0 | 12 |
> +---------------------------------------------+------------+------------+
>
>
> If you fix the issue, kindly add following tag
> Reported-by: kernel test robot <[email protected]>
>
>
> [ 2.820056][ T1] BUG: kernel NULL pointer dereference, address: 00000360
> [ 2.821034][ T1] #PF: supervisor read access in kernel mode
> [ 2.821862][ T1] #PF: error_code(0x0000) - not-present page
> [ 2.822468][ T1] *pde = 00000000
> [ 2.822468][ T1] Oops: 0000 [#1]
> [ 2.822468][ T1] CPU: 0 PID: 1 Comm: swapper Not tainted 5.18.0-rc3-00074-ga2af0cae87a0 #1
> [ 2.822468][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014
> [ 2.822468][ T1] EIP: add_mtd_device+0x32d/0x6a0
> [ 2.822468][ T1] Code: 01 ff ff ff 8d 50 ff 85 c2 0f 84 ec fe ff ff 31 c0 e9 ef fe ff ff 8d b4 26 00 00 00 00 8b 83 cc 03 00 00 3d 08 ff ff ff 74 0e <8b> 80 60 03 00 00 85 c0 0f 85 65 01 00 00 8b 7d 84 89 f8 e8 3b 61
> [ 2.822468][ T1] EAX: 00000000 EBX: c6eaa000 ECX: 00000001 EDX: 00000000
> [ 2.822468][ T1] ESI: 00000000 EDI: c6eaa0f8 EBP: c3f0becc ESP: c3f0be44
> [ 2.822468][ T1] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010213
> [ 2.822468][ T1] CR0: 80050033 CR2: 00000360 CR3: 0345a000 CR4: 000406d0
> [ 2.822468][ T1] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [ 2.822468][ T1] DR6: fffe0ff0 DR7: 00000400
> [ 2.822468][ T1] Call Trace:
> [ 2.822468][ T1] ? parse_ofoldpart_partitions+0x60/0x60
> [ 2.822468][ T1] ? mtd_part_of_parse+0x118/0x240
> [ 2.822468][ T1] ? mtd_part_of_parse+0x20e/0x240
> [ 2.822468][ T1] ? parse_mtd_partitions+0xfa/0x200
> [ 2.822468][ T1] mtd_device_parse_register+0x19f/0x2b0
> [ 2.822468][ T1] mtdram_init_device+0xb7/0x100
> [ 2.822468][ T1] init_mtdram+0x67/0xaf
> [ 2.822468][ T1] ? init_phram+0x54/0x54
> [ 2.822468][ T1] do_one_initcall+0x58/0x240
> [ 2.822468][ T1] ? rdinit_setup+0x38/0x38
> [ 2.822468][ T1] do_initcalls+0xf3/0x112
> [ 2.822468][ T1] kernel_init_freeable+0xbd/0xec
> [ 2.822468][ T1] ? rest_init+0x100/0x100
> [ 2.822468][ T1] kernel_init+0x12/0xf0
> [ 2.822468][ T1] ret_from_fork+0x1c/0x30
> [ 2.822468][ T1] Modules linked in:
> [ 2.822468][ T1] CR2: 0000000000000360
> [ 2.822468][ T1] ---[ end trace 0000000000000000 ]---
> [ 2.822468][ T1] EIP: add_mtd_device+0x32d/0x6a0
> [ 2.822468][ T1] Code: 01 ff ff ff 8d 50 ff 85 c2 0f 84 ec fe ff ff 31 c0 e9 ef fe ff ff 8d b4 26 00 00 00 00 8b 83 cc 03 00 00 3d 08 ff ff ff 74 0e <8b> 80 60 03 00 00 85 c0 0f 85 65 01 00 00 8b 7d 84 89 f8 e8 3b 61
> [ 2.822468][ T1] EAX: 00000000 EBX: c6eaa000 ECX: 00000001 EDX: 00000000
> [ 2.822468][ T1] ESI: 00000000 EDI: c6eaa0f8 EBP: c3f0becc ESP: c3f0be44
> [ 2.822468][ T1] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010213
> [ 2.822468][ T1] CR0: 80050033 CR2: 00000360 CR3: 0345a000 CR4: 000406d0
> [ 2.822468][ T1] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [ 2.822468][ T1] DR6: fffe0ff0 DR7: 00000400
> [ 2.822468][ T1] Kernel panic - not syncing: Fatal exception
> [ 2.822468][ T1] Kernel Offset: disabled
>
>
>
> To reproduce:
>
> # build kernel
> cd linux
> cp config-5.18.0-rc3-00074-ga2af0cae87a0 .config
> make HOSTCC=gcc-11 CC=gcc-11 ARCH=i386 olddefconfig prepare modules_prepare bzImage modules
> make HOSTCC=gcc-11 CC=gcc-11 ARCH=i386 INSTALL_MOD_PATH=<mod-install-dir> modules_install
> cd <mod-install-dir>
> find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
>
>
> git clone https://github.com/intel/lkp-tests.git
> cd lkp-tests
> bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
>
> # if come across any failure that blocks the test,
> # please remove ~/.lkp and /lkp dir to run from a clean state.
>
>
>
On Mon, 06 Jun 2022 17:14:16 +0200, Ansuel Smith wrote:
> Add additional example for qcom,smem-part to declare a dynamic
> partition to provide NVMEM cells for the commonly ART partition
> provided by this parser.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> .../mtd/partitions/qcom,smem-part.yaml | 27 +++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> > > ---
> > > 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.
>
>