mtd, nand, omap2: parse cmdline partition fail

PATCH Heiko Schocher 3 participants 6 messages Dec 4, 2015 – Dec 11, 2015

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Heiko Schocher

author

2015-12-04 06:48 UTC #0 lore raw

Hello Frans,

I just tried current mainline kernel:
commit 2255702db4014d1c69d6037ed7bdad2d2e271985
Merge: 9e5d25e c86576e
Author: Linus Torvalds <[email protected]>
Date: Mon Nov 30 16:06:44 2015 -0800

    Merge tag 'mn10300-for-linus-v4.4-rc4' of

git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging

on an am3517 based board (mainlining soon). And with your commit:
commit 853f1c58c4b2: mtd: nand: omap2: show parent device structure in sysfs

MTD partitions from cmdline are not longer detected:

[    2.087305] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xcc
[    2.094097] nand: Micron MT29F4G16ABADAWP
[    2.098303] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
[    2.106296] nand: WARNING: MT29F4G16ABADAWP: the ECC used on your system is too weak compared to the one required by the NAND chip
[    2.118674] MT29F4G16ABADAWP: 'partitions' subnode not found on /ocp/gpmc@6e000000/nand@0,0. Trying to parse direct subnodes as partitions.

[...]

before this patch it worked:

[    2.307444] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xcc
[    2.314092] nand: Micron MT29F4G16ABADAWP
[    2.318348] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
[    2.326331] nand: WARNING: omap2-nand.0: the ECC used on your system is too weak compared to the one required by the NAND chip
[    2.338336] 5 cmdlinepart partitions found on MTD device omap2-nand.0
[    2.345129] Creating 5 MTD partitions on "omap2-nand.0":
[    2.350704] 0x000000000000-0x000000080000 : "MLO"
[    2.366877] 0x000000080000-0x000000180000 : "u-boot"
[    2.379179] 0x000000180000-0x0000001c0000 : "env1"
[    2.390627] 0x0000001c0000-0x000000200000 : "env2"
[    2.402255] 0x000000200000-0x000020000000 : "common_data"

Reason is taht the mtd->name has changed from "omap2-nand.0" to the nand chip name ...

If I revert this part from the patch

drivers/mtd/nand/omap2.c +1 -0

index 93f664c..28dcf66 100644
@@ -1685,6 +1685,7 @@ static int omap_nand_probe(struct platform_device *pdev)
        info->ecc_opt           = pdata->ecc_opt;
        mtd                     = &info->mtd;
        mtd->priv               = &info->nand;
+       mtd->name               = dev_name(&pdev->dev);
        mtd->dev.parent         = &pdev->dev;
        nand_chip               = &info->nand;
        nand_chip->ecc.priv     = NULL;

It works again ...

So the question is, is it intended to change the "mtd->name"? But wondering, if there are two or more identical nand chips in the system, they will have the same mtd->name ... which seems buggy to me...

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Frans Klaver

2015-12-04 07:17 UTC #1 lore raw

Quoted 60 lines · Heiko Schocher

Hello Frans,

I just tried current mainline kernel:
commit 2255702db4014d1c69d6037ed7bdad2d2e271985
Merge: 9e5d25e c86576e
Author: Linus Torvalds <[email protected]>
Date: Mon Nov 30 16:06:44 2015 -0800

    Merge tag 'mn10300-for-linus-v4.4-rc4' of

git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging

on an am3517 based board (mainlining soon). And with your commit:
commit 853f1c58c4b2: mtd: nand: omap2: show parent device structure in sysfs

MTD partitions from cmdline are not longer detected:

[    2.087305] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xcc
[    2.094097] nand: Micron MT29F4G16ABADAWP
[    2.098303] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB
size: 64
[    2.106296] nand: WARNING: MT29F4G16ABADAWP: the ECC used on your system
is too weak compared to the one required by the NAND chip
[    2.118674] MT29F4G16ABADAWP: 'partitions' subnode not found on

/ocp/gpmc@6e000000/nand@0,0. Trying to parse direct subnodes as partitions. [...]

before this patch it worked:

[    2.307444] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xcc
[    2.314092] nand: Micron MT29F4G16ABADAWP
[    2.318348] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB
size: 64
[    2.326331] nand: WARNING: omap2-nand.0: the ECC used on your system is
too weak compared to the one required by the NAND chip
[    2.338336] 5 cmdlinepart partitions found on MTD device omap2-nand.0
[    2.345129] Creating 5 MTD partitions on "omap2-nand.0":
[    2.350704] 0x000000000000-0x000000080000 : "MLO"
[    2.366877] 0x000000080000-0x000000180000 : "u-boot"
[    2.379179] 0x000000180000-0x0000001c0000 : "env1"
[    2.390627] 0x0000001c0000-0x000000200000 : "env2"
[    2.402255] 0x000000200000-0x000020000000 : "common_data"

Reason is taht the mtd->name has changed from "omap2-nand.0" to the nand chip name ...

If I revert this part from the patch

drivers/mtd/nand/omap2.c +1 -0

index 93f664c..28dcf66 100644
@@ -1685,6 +1685,7 @@ static int omap_nand_probe(struct platform_device
 *pdev)
        info->ecc_opt           = pdata->ecc_opt;
        mtd                     = &info->mtd;
        mtd->priv               = &info->nand;
+       mtd->name               = dev_name(&pdev->dev);
        mtd->dev.parent         = &pdev->dev;
        nand_chip               = &info->nand;

        nand_chip->ecc.priv     = NULL;

Quoted 3 lines

It works again ...
So the question is, is it intended to change the "mtd->name"?

That's definitely not intended. The expectation with this patch is that nothing really changes, except that a parent device link is available in sysfs. For the name this patch depends on 807f16d4db956 ("mtd: core: set some defaults when dev.parent is set") which does something like:

if (mtd->dev.parent) {
        if (!mtd->name)
                mtd->name = dev_name(mtd->dev.parent);
}

The fact that this produces different names for you is slightly surprising to me, unless mtd->name is already set to something by the time it reaches add_mtd_device(). Or I overlooked something, which is entirely plausible as well.

So effectively this should be the same as doing:

  mtd->dev.parent = &pdev->dev;
  mtd->name = dev_name(mtd->dev.parent);

Quoted 2 lines

But wondering, if there are two or more identical nand chips in the system, they will have the same mtd->name ... which seems buggy to me...

Agree.

Thanks,
Frans

Heiko Schocher

author

2015-12-04 08:42 UTC #2 lore raw

Hello Frans,

Am 04.12.2015 um 08:17 schrieb Frans Klaver:

Quoted 71 lines · Heiko Schocher

Quoted 60 lines
Hello Frans,
I just tried current mainline kernel:
commit 2255702db4014d1c69d6037ed7bdad2d2e271985
Merge: 9e5d25e c86576e
Author: Linus Torvalds <[email protected]>
Date: Mon Nov 30 16:06:44 2015 -0800
     Merge tag 'mn10300-for-linus-v4.4-rc4' of
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging
on an am3517 based board (mainlining soon). And with your commit:
commit 853f1c58c4b2: mtd: nand: omap2: show parent device structure in sysfs
MTD partitions from cmdline are not longer detected:
[    2.087305] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xcc
[    2.094097] nand: Micron MT29F4G16ABADAWP
[    2.098303] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB
size: 64
[    2.106296] nand: WARNING: MT29F4G16ABADAWP: the ECC used on your system
is too weak compared to the one required by the NAND chip
[    2.118674] MT29F4G16ABADAWP: 'partitions' subnode not found on
/ocp/gpmc@6e000000/nand@0,0. Trying to parse direct subnodes as partitions. [...]
before this patch it worked:
[    2.307444] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xcc
[    2.314092] nand: Micron MT29F4G16ABADAWP
[    2.318348] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB
size: 64
[    2.326331] nand: WARNING: omap2-nand.0: the ECC used on your system is
too weak compared to the one required by the NAND chip
[    2.338336] 5 cmdlinepart partitions found on MTD device omap2-nand.0
[    2.345129] Creating 5 MTD partitions on "omap2-nand.0":
[    2.350704] 0x000000000000-0x000000080000 : "MLO"
[    2.366877] 0x000000080000-0x000000180000 : "u-boot"
[    2.379179] 0x000000180000-0x0000001c0000 : "env1"
[    2.390627] 0x0000001c0000-0x000000200000 : "env2"
[    2.402255] 0x000000200000-0x000020000000 : "common_data"
Reason is taht the mtd->name has changed from "omap2-nand.0" to the nand chip name ...
If I revert this part from the patch
drivers/mtd/nand/omap2.c +1 -0
index 93f664c..28dcf66 100644
@@ -1685,6 +1685,7 @@ static int omap_nand_probe(struct platform_device
 *pdev)
         info->ecc_opt           = pdata->ecc_opt;
         mtd                     = &info->mtd;
         mtd->priv               = &info->nand;
+       mtd->name               = dev_name(&pdev->dev);
         mtd->dev.parent         = &pdev->dev;
         nand_chip               = &info->nand;
         nand_chip->ecc.priv     = NULL;
Quoted 3 lines
It works again ...
So the question is, is it intended to change the "mtd->name"?
That's definitely not intended. The expectation with this patch is that nothing really changes, except that a parent device link is available in sysfs. For the name this patch depends on 807f16d4db956 ("mtd: core: set some defaults when dev.parent is set") which does something like:

Quoted 4 lines

if (mtd->dev.parent) {
         if (!mtd->name)
                 mtd->name = dev_name(mtd->dev.parent);
}

commit 807f16d4db956 is in the tree... ok.

Hmm... I see in drivers/mtd/nand/omap2.c omap_nand_probe() info gets allocated with devm_kzalloc(), then info->mtd gets filled.

Without setting "mtd->name = dev_name(&pdev->dev);" mtd->name never gets filled ... or?

It seems to me add_mtd_device() gets only called for the mtd partitions parsed from the cmdline ...

I added to my patch above following debug printk:

drivers/mtd/mtdcore.c +3 -0

index 95c13b2..f1a95eb 100644
@@ -426,6 +426,9 @@ int add_mtd_device(struct mtd_info *mtd)
        mtd->erasesize_mask = (1 << mtd->erasesize_shift) - 1;
        mtd->writesize_mask = (1 << mtd->writesize_shift) - 1;
 
+	printk("%s: ******* mtd->name: %s\n", __func__, mtd->name);
+	if (mtd->dev.parent)
+       	printk("%s: *******parent name: %s\n", __func__, dev_name(mtd->dev.parent));
        if (mtd->dev.parent) {
                if (!mtd->owner && mtd->dev.parent->driver)
                        mtd->owner = mtd->dev.parent->driver->owner;

Log with them:

[    2.613797] mtdoops: mtd device (mtddev=name/number) must be supplied
[    2.623417] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xcc
[    2.630077] nand: Micron MT29F4G16ABADAWP
[    2.634395] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
[    2.642402] nand: WARNING: omap2-nand.0: the ECC used on your system is too weak compared to the one required by the NAND chip
[    2.654412] 5 cmdlinepart partitions found on MTD device omap2-nand.0
[    2.661213] Creating 5 MTD partitions on "omap2-nand.0":
[    2.666785] 0x000000000000-0x000000080000 : "MLO"
[    2.672755] add_mtd_device: ******* mtd->name: MLO
[    2.677775] add_mtd_device: *******parent name: omap2-nand.0
[    2.693967] 0x000000080000-0x000000180000 : "u-boot"
[    2.700097] add_mtd_device: ******* mtd->name: u-boot
[    2.705490] add_mtd_device: *******parent name: omap2-nand.0
[    2.717487] 0x000000180000-0x0000001c0000 : "env1"
[    2.722902] add_mtd_device: ******* mtd->name: env1
[    2.728010] add_mtd_device: *******parent name: omap2-nand.0
[    2.739992] 0x0000001c0000-0x000000200000 : "env2"
[    2.745401] add_mtd_device: ******* mtd->name: env2
[    2.750505] add_mtd_device: *******parent name: omap2-nand.0
[    2.762875] 0x000000200000-0x000020000000 : "common_data"
[    3.218895] add_mtd_device: ******* mtd->name: common_data
[    3.224686] add_mtd_device: *******parent name: omap2-nand.0

No other "add_mtd_device:" output ...

Quoted 7 lines

The fact that this produces different names for you is slightly surprising to me, unless mtd->name is already set to something by the time it reaches add_mtd_device(). Or I overlooked something, which is entirely plausible as well.
So effectively this should be the same as doing:

Quoted 8 lines

   mtd->dev.parent = &pdev->dev;
   mtd->name = dev_name(mtd->dev.parent);
Quoted 2 lines
But wondering, if there are two or more identical nand chips in the system, they will have the same mtd->name ... which seems buggy to me...
Agree.

Good, so we must fix it ;-)

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Brian Norris

2015-12-09 23:19 UTC #3 lore raw

Quoted 18 lines · Heiko Schocher

Am 04.12.2015 um 08:17 schrieb Frans Klaver:
Quoted 10 lines · Heiko Schocher
Quoted 2 lines
So the question is, is it intended to change the "mtd->name"?
That's definitely not intended. The expectation with this patch is that nothing really changes, except that a parent device link is available in sysfs. For the name this patch depends on 807f16d4db956 ("mtd: core: set some defaults when dev.parent is set") which does something like:
Quoted 4 lines
if (mtd->dev.parent) {
        if (!mtd->name)
                mtd->name = dev_name(mtd->dev.parent);
}
commit 807f16d4db956 is in the tree... ok.

Quoted 8 lines

Hmm... I see in drivers/mtd/nand/omap2.c omap_nand_probe() info gets allocated with devm_kzalloc(), then info->mtd gets filled.
Without setting "mtd->name = dev_name(&pdev->dev);" mtd->name never gets filled ... or?
It seems to me add_mtd_device() gets only called for the mtd partitions parsed from the cmdline ...

That's true, when CONFIG_MTD_PARTITIONED_MASTER=n. (I'm really thinking we should accelerate the adoption of PARTITIONED_MASTER... maybe set it to default =y?)

But even with CONFIG_MTD_PARTITIONED_MASTER=y we still have a problem.

[...]

Quoted 9 lines

Quoted 9 lines
The fact that this produces different names for you is slightly surprising to me, unless mtd->name is already set to something by the time it reaches add_mtd_device(). Or I overlooked something, which is entirely plausible as well.
So effectively this should be the same as doing:
  mtd->dev.parent = &pdev->dev;
  mtd->name = dev_name(mtd->dev.parent);

Yes, except for one thing (and this is the key): the mtd->name only gets set *after* the partitions are parsed, using the method from commit 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set"). So the parsers (including cmdlinepart) get run with a blank (NULL) name, and you can't detect any partitions, since the name match will never work.

I have a hack of a patch below (untested) that would hopefully work (based on current l2-mtd.git). I could port this to a base on 4.4-rc1 if it works OK, so we can get the regression fixed in this cycle.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
But wondering, if there are two or more identical nand chips in the system, they will have the same mtd->name ... which seems buggy to me...
Agree.
Good, so we must fix it ;-)

Yeah, that's a bad problem. Most people only plan for one chip and then realize they need 2 later. nand_base should probably support something to do this easily. Unfortunately, making a change like that could cause some problems with cmdline naming across kernel versions, if we start changing the convention for some drivers...(do we consider the MTD name part of the ABI?)

Brian

drivers/mtd/mtdcore.c +9 -0

index 89d811e7b04a..185dc36c687f 100644
@@ -592,6 +592,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 	struct mtd_partitions parsed;
 	int ret;
 
+	if (mtd->dev.parent) {
+		if (!mtd->owner && mtd->dev.parent->driver)
+			mtd->owner = mtd->dev.parent->driver->owner;
+		if (!mtd->name)
+			mtd->name = dev_name(mtd->dev.parent);
+	} else {
+		pr_debug("mtd device won't show a device symlink in sysfs\n");
+	}
+
 	memset(&parsed, 0, sizeof(parsed));
 
 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);

Frans Klaver

2015-12-10 07:13 UTC #4 lore raw

Quoted 22 lines · Brian Norris

Quoted 2 lines · Heiko Schocher
It seems to me add_mtd_device() gets only called for the mtd partitions parsed from the cmdline ...
That's true, when CONFIG_MTD_PARTITIONED_MASTER=n. (I'm really thinking we should accelerate the adoption of PARTITIONED_MASTER... maybe set it to default =y?)
But even with CONFIG_MTD_PARTITIONED_MASTER=y we still have a problem.
[...]
Quoted 9 lines
Quoted 9 lines
The fact that this produces different names for you is slightly surprising to me, unless mtd->name is already set to something by the time it reaches add_mtd_device(). Or I overlooked something, which is entirely plausible as well.
So effectively this should be the same as doing:
  mtd->dev.parent = &pdev->dev;
  mtd->name = dev_name(mtd->dev.parent);

Quoted 6 lines

Yes, except for one thing (and this is the key): the mtd->name only gets set *after* the partitions are parsed, using the method from commit 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set"). So the parsers (including cmdlinepart) get run with a blank (NULL) name, and you can't detect any partitions, since the name match will never work.

Right, that was something we overlooked earlier.

Quoted 3 lines

I have a hack of a patch below (untested) that would hopefully work (based on current l2-mtd.git). I could port this to a base on 4.4-rc1 if it works OK, so we can get the regression fixed in this cycle.

That would be nice.

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
But wondering, if there are two or more identical nand chips in the system, they will have the same mtd->name ... which seems buggy to me...
Agree.

Quoted 8 lines

Quoted 1 line
Good, so we must fix it ;-)
Yeah, that's a bad problem. Most people only plan for one chip and then realize they need 2 later. nand_base should probably support something to do this easily. Unfortunately, making a change like that could cause some problems with cmdline naming across kernel versions, if we start changing the convention for some drivers...(do we consider the MTD name part of the ABI?)

I would expect a name to be just a name; something parsable by humans. By definition that cannot be an ABI. On the other hand, maybe it has grown to become part of the ABI.

Quoted 10 lines

drivers/mtd/mtdcore.c +2 -0

index 89d811e7b04a..185dc36c687f 100644
@@ -592,6 +592,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
        struct mtd_partitions parsed;
        int ret;
 
+       if (mtd->dev.parent) {
+               if (!mtd->owner && mtd->dev.parent->driver)

drivers/mtd/mtdcore.c int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, · line 606

Quoted 10 lines

drivers/mtd/mtdcore.c +7 -0

+                       mtd->owner = mtd->dev.parent->driver->owner;
+               if (!mtd->name)
+                       mtd->name = dev_name(mtd->dev.parent);
+       } else {
+               pr_debug("mtd device won't show a device symlink in sysfs\n");
+       }
+
        memset(&parsed, 0, sizeof(parsed));
 
        ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);

This was the first thing I thought of when this issue was brought up. If you do this, do you still need the chunk of code you copied from in add_mtd_device()? Since we fill in these things on the master, I wouldn't think we do.

Thanks,
Frans

Brian Norris

2015-12-11 23:25 UTC #5 lore raw

Quoted 17 lines · Frans Klaver

Quoted 6 lines · Brian Norris
Quoted 5 lines · Heiko Schocher
Quoted 4 lines
Quoted 2 lines
But wondering, if there are two or more identical nand chips in the system, they will have the same mtd->name ... which seems buggy to me...
Agree.
Quoted 8 lines
Quoted 1 line
Good, so we must fix it ;-)
Yeah, that's a bad problem. Most people only plan for one chip and then realize they need 2 later. nand_base should probably support something to do this easily. Unfortunately, making a change like that could cause some problems with cmdline naming across kernel versions, if we start changing the convention for some drivers...(do we consider the MTD name part of the ABI?)

Quoted 3 lines

I would expect a name to be just a name; something parsable by humans. By definition that cannot be an ABI. On the other hand, maybe it has grown to become part of the ABI.

So far, we've tried not to break it if possible. Perhaps if we come up with a better solution for automatically naming/numbering chips attached to the same device/controller, we can test out whether it hurts to change.

Quoted 19 lines

drivers/mtd/mtdcore.c +9 -0

index 89d811e7b04a..185dc36c687f 100644
@@ -592,6 +592,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
        struct mtd_partitions parsed;
        int ret;
 
+       if (mtd->dev.parent) {
+               if (!mtd->owner && mtd->dev.parent->driver)
+                       mtd->owner = mtd->dev.parent->driver->owner;
+               if (!mtd->name)
+                       mtd->name = dev_name(mtd->dev.parent);
+       } else {
+               pr_debug("mtd device won't show a device symlink in sysfs\n");
+       }
+
        memset(&parsed, 0, sizeof(parsed));

Quoted 6 lines

drivers/mtd/mtdcore.c int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, · line 606
Quoted 1 line
drivers/mtd/mtdcore.c +0 -0
        ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
This was the first thing I thought of when this issue was brought up. If you do this, do you still need the chunk of code you copied from in add_mtd_device()? Since we fill in these things on the master, I wouldn't think we do.

I guess we don't need the code in add_mtd_device().

I'll send a patch against linux-mtd.git shortly (essentially 4.4-rc1).

Brian