mtd: fix cmdlinepart parser, early naming for auto-filled MTD

PATCH In review Brian Norris 2 participants 6 messages Dec 11, 2015 – Jan 4, 2016

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Brian Norris

author

2015-12-11 23:58 UTC #0 lore raw

PATCH 1/1 mtd: fix cmdlinepart parser, early naming for auto-filled MTD

Commit 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set") attempted to provide some default settings for MTDs that
(a) assign the parent device and
(b) don't provide their own name or owner

However, this isn't a perfect drop-in replacement for the boilerplate found in some drivers, because the MTD name is used by partition parsers like cmdlinepart, but the name isn't set until add_mtd_device(), after the parsing is completed. This means cmdlinepart sees a NULL name and therefore will not work properly.

Fix this by moving the default name and owner assignment to be first in the MTD registration process.

Fixes 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set")

Reported-by Heiko Schocher <[email protected]>

Signed-off-by Brian Norris <[email protected]>

Cc Heiko Schocher <[email protected]>

Cc Frans Klaver <[email protected]>

---
Heiko, can you provide testing feedback (e.g., 'Tested-by: ...')?

In testing this myself, it looks like cmdlinepart.c can actually work OK with a single-MTD system, even when mtd->name isn't set. See this snippet:

        /*
         * Search for the partition definition matching master->name.
         * If master->name is not set, stop at first partition definition.
         */
        for (part = partitions; part; part = part->next) {
                if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id)))
                        break;
        }

But, I don't know *why* it does that...

 drivers/mtd/mtdcore.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

drivers/mtd/mtdcore.c +17 -9

index 95c13b2ffa79..ffa288474820 100644
@@ -426,15 +426,6 @@ int add_mtd_device(struct mtd_info *mtd)
 	mtd->erasesize_mask = (1 << mtd->erasesize_shift) - 1;
 	mtd->writesize_mask = (1 << mtd->writesize_shift) - 1;
 
-	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");
-	}
-
 	/* Some chips always power up locked. Unlock them now */
 	if ((mtd->flags & MTD_WRITEABLE) && (mtd->flags & MTD_POWERUP_LOCK)) {
 		error = mtd_unlock(mtd, 0, mtd->size);

@@ -549,6 +540,21 @@ static int mtd_add_device_partitions(struct mtd_info *mtd,
 	return 0;
 }
 
+/*
+ * Set a few defaults based on the parent devices, if not provided by the
+ * driver
+ */
+static void mtd_set_dev_defaults(struct mtd_info *mtd)
+{
+	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");
+	}
+}
 
 /**
  * mtd_device_parse_register - parse partitions and register an MTD device.

@@ -587,6 +593,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 	int ret;
 	struct mtd_partition *real_parts = NULL;
 
+	mtd_set_dev_defaults(mtd);
+
 	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
 	if (ret <= 0 && nr_parts && parts) {
 		real_parts = kmemdup(parts, sizeof(*parts) * nr_parts,

Signature · 1 line

2.6.0.rc2.230.g3dd15c0

Heiko Schocher

2015-12-12 04:45 UTC #1 lore raw

Hello Brian,

Am 12.12.2015 um 00:58 schrieb Brian Norris:

Quoted 14 lines

Commit 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set") attempted to provide some default settings for MTDs that
(a) assign the parent device and
(b) don't provide their own name or owner
However, this isn't a perfect drop-in replacement for the boilerplate found in some drivers, because the MTD name is used by partition parsers like cmdlinepart, but the name isn't set until add_mtd_device(), after the parsing is completed. This means cmdlinepart sees a NULL name and therefore will not work properly.
Fix this by moving the default name and owner assignment to be first in the MTD registration process.

Quoted 7 lines

Fixes 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set")
Reported-by Heiko Schocher <[email protected]>
Signed-off-by Brian Norris <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Frans Klaver <[email protected]>
---
Heiko, can you provide testing feedback (e.g., 'Tested-by: ...')?

Sorry, does not work for me:

Based on:
pollux:linux hs [20151212] $ git describe master
v4.4-rc4-135-gb9d8545

and this patch, shows the same problem,

Adding:

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;

and it works again ...

bye,
Heiko

Quoted 70 lines

In testing this myself, it looks like cmdlinepart.c can actually work OK with a single-MTD system, even when mtd->name isn't set. See this snippet:

         /*
          * Search for the partition definition matching master->name.
          * If master->name is not set, stop at first partition definition.
          */
         for (part = partitions; part; part = part->next) {
                 if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id)))
                         break;
         }

But, I don't know *why* it does that...

  drivers/mtd/mtdcore.c | 26 +++++++++++++++++---------
  1 file changed, 17 insertions(+), 9 deletions(-)

drivers/mtd/mtdcore.c +17 -9

index 95c13b2ffa79..ffa288474820 100644
@@ -426,15 +426,6 @@ int add_mtd_device(struct mtd_info *mtd)
  	mtd->erasesize_mask = (1 << mtd->erasesize_shift) - 1;
  	mtd->writesize_mask = (1 << mtd->writesize_shift) - 1;
 
-	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");
-	}
-
  	/* Some chips always power up locked. Unlock them now */
  	if ((mtd->flags & MTD_WRITEABLE) && (mtd->flags & MTD_POWERUP_LOCK)) {
  		error = mtd_unlock(mtd, 0, mtd->size);

@@ -549,6 +540,21 @@ static int mtd_add_device_partitions(struct mtd_info *mtd,
  	return 0;
  }
 
+/*
+ * Set a few defaults based on the parent devices, if not provided by the
+ * driver
+ */
+static void mtd_set_dev_defaults(struct mtd_info *mtd)
+{
+	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");
+	}
+}
 
  /**
   * mtd_device_parse_register - parse partitions and register an MTD device.

@@ -587,6 +593,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
  	int ret;
  	struct mtd_partition *real_parts = NULL;
 
+	mtd_set_dev_defaults(mtd);
+
  	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
  	if (ret <= 0 && nr_parts && parts) {
  		real_parts = kmemdup(parts, sizeof(*parts) * nr_parts,

Signature · 2 lines

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

Brian Norris

author

2015-12-12 05:39 UTC #2 lore raw

Quoted 23 lines · Heiko Schocher

Am 12.12.2015 um 00:58 schrieb Brian Norris:
Quoted 14 lines
Commit 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set") attempted to provide some default settings for MTDs that
(a) assign the parent device and
(b) don't provide their own name or owner
However, this isn't a perfect drop-in replacement for the boilerplate found in some drivers, because the MTD name is used by partition parsers like cmdlinepart, but the name isn't set until add_mtd_device(), after the parsing is completed. This means cmdlinepart sees a NULL name and therefore will not work properly.
Fix this by moving the default name and owner assignment to be first in the MTD registration process.
Quoted 7 lines
Fixes 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set")
Reported-by Heiko Schocher <[email protected]>
Signed-off-by Brian Norris <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Frans Klaver <[email protected]>
---
Heiko, can you provide testing feedback (e.g., 'Tested-by: ...')?

Quoted 7 lines

Sorry, does not work for me:
Based on:
pollux:linux hs [20151212] $ git describe master
v4.4-rc4-135-gb9d8545
and this patch, shows the same problem,

[...]

Ugh, I see the problem. In nand_base.c, nand_get_flash_type():

	if (!mtd->name)
		mtd->name = type->name;

Brian

Brian Norris

author

2015-12-14 19:24 UTC #3 lore raw

Quoted 26 lines · Brian Norris

Quoted 25 lines · Heiko Schocher
Am 12.12.2015 um 00:58 schrieb Brian Norris:
Quoted 14 lines
Commit 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set") attempted to provide some default settings for MTDs that
(a) assign the parent device and
(b) don't provide their own name or owner
However, this isn't a perfect drop-in replacement for the boilerplate found in some drivers, because the MTD name is used by partition parsers like cmdlinepart, but the name isn't set until add_mtd_device(), after the parsing is completed. This means cmdlinepart sees a NULL name and therefore will not work properly.
Fix this by moving the default name and owner assignment to be first in the MTD registration process.
Quoted 7 lines
Fixes 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set")
Reported-by Heiko Schocher <[email protected]>
Signed-off-by Brian Norris <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Frans Klaver <[email protected]>
---
Heiko, can you provide testing feedback (e.g., 'Tested-by: ...')?
Sorry, does not work for me:

Quoted 7 lines

Quoted 5 lines
Based on:
pollux:linux hs [20151212] $ git describe master
v4.4-rc4-135-gb9d8545
and this patch, shows the same problem,
[...]

BTW, can you please include the relevant log snippets when replying in the future?

Quoted 4 lines

Ugh, I see the problem. In nand_base.c, nand_get_flash_type():
	if (!mtd->name)
		mtd->name = type->name;

Specifically, I think we could hack around this with something like the following additional patch. Untested:

drivers/mtd/nand/nand_base.c +3 -0

index ece544efccc3..9f169566fba4 100644
@@ -3826,6 +3826,9 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	if (!type)
 		type = nand_flash_ids;
 
+	if (!mtd->name && mtd->dev.parent)
+		mtd->name = dev_name(mtd->dev.parent);
+
 	for (; type->name != NULL; type++) {
 		if (is_full_id_nand(type)) {
 			if (find_full_id_nand(mtd, chip, type, id_data, &busw))

Heiko Schocher

2015-12-15 06:51 UTC #4 lore raw

Hello Brian,

Am 14.12.2015 um 20:24 schrieb Brian Norris:

Quoted 31 lines · Brian Norris

Quoted 30 lines · Heiko Schocher
Quoted 29 lines
Am 12.12.2015 um 00:58 schrieb Brian Norris:
Quoted 14 lines
Commit 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set") attempted to provide some default settings for MTDs that
(a) assign the parent device and
(b) don't provide their own name or owner
However, this isn't a perfect drop-in replacement for the boilerplate found in some drivers, because the MTD name is used by partition parsers like cmdlinepart, but the name isn't set until add_mtd_device(), after the parsing is completed. This means cmdlinepart sees a NULL name and therefore will not work properly.
Fix this by moving the default name and owner assignment to be first in the MTD registration process.
Quoted 7 lines
Fixes 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set")
Reported-by Heiko Schocher <[email protected]>
Signed-off-by Brian Norris <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Frans Klaver <[email protected]>
---
Heiko, can you provide testing feedback (e.g., 'Tested-by: ...')?
Sorry, does not work for me:
Based on:
pollux:linux hs [20151212] $ git describe master
v4.4-rc4-135-gb9d8545

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
and this patch, shows the same problem,
[...]
BTW, can you please include the relevant log snippets when replying in the future?

Uh, Sorry for that, yes of course ...

Quoted 14 lines

Quoted 4 lines
Ugh, I see the problem. In nand_base.c, nand_get_flash_type():
	if (!mtd->name)
		mtd->name = type->name;
Specifically, I think we could hack around this with something like the following additional patch. Untested:
drivers/mtd/nand/nand_base.c +0 -0
index ece544efccc3..9f169566fba4 100644

drivers/mtd/nand/nand_base.c static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, · line 3834

Quoted 9 lines

drivers/mtd/nand/nand_base.c +3 -0

@@ -3826,6 +3826,9 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
  	if (!type)
  		type = nand_flash_ids;
 
+	if (!mtd->name && mtd->dev.parent)
+		mtd->name = dev_name(mtd->dev.parent);
+
  	for (; type->name != NULL; type++) {
  		if (is_full_id_nand(type)) {
  			if (find_full_id_nand(mtd, chip, type, id_data, &busw))

Sorry, I had not yet the time to look into the issue deeper, Thanks for digging into it. I tried therefore your patch (automated with tbot see below [1]) on linux "v4.4-rc5" and

your patch works!

bye,
Heiko

[1] automated test with tbot [2]
http://xeidos.ddns.net/buildbot/builders/mcx/builds/7/steps/shell/logs/tbotlog

as there is a lot of things done automatically, search for "Micron MT29F4G16ABADAWP"

All you see in the log is done automatically with tbot. The mcx board is in munich, while I am doing the test from hungary. In my testsetup tbot is integrated into buildbot, which is used for the webserver and triggering builds.

What is done:
- get linux code (if it is not already checked out)
- apply patches
(I have also a testcase, which download all patches from patchwork,
which are delegated to me (for u-boot), check them with checkpatch and apply it ... very helpful, if this is done in a nightly build, you have only to look in the morning, if all is green, and you know that all patches are checkpatch clean, apply celan, compile clean and do not break the boards you have in your nightly build ... also there is a testcase which starts a git bisect test [3] ...)

As I did not mainlined the board yet, there are a bunch of checkpatch errors, so I disabled that the testcase fails, if there are errors.

- compile linux
- copy files to tftpdir
- boot them
- execute testcases
for example check in dmesg some strings with:
https://github.com/hsdenx/tbot/blob/master/src/tc/tc_lx_dmesg_grep.py

So detected our current bug ;-)

check register settings with devmem2:
https://github.com/hsdenx/tbot/blob/master/src/tc/tc_lx_check_reg_file.py

Thats very nice, as you can check for example complete pinmux register. see as an example [4]

last but not least, tbot (and buildbot incl. webserver) is running on a raspberry pi at my home! So no expensive hw costs.

[2] https://github.com/hsdenx/tbot
[3] git bisect automated:
https://github.com/hsdenx/tbot/blob/master/src/tc/tc_board_git_bisect.py
log as an example for U-Boot:
http://xeidos.ddns.net/buildbot/builders/tqm5200s/builds/3/steps/shell/logs/tbotlog
[4] register file
https://github.com/hsdenx/tbot/blob/master/src/files/ccu1_pinmux_scm.reg

Signature · 2 lines

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

Brian Norris

author

2016-01-04 18:56 UTC #5 lore raw

In reply to Brian Norris

Quoted 14 lines · Brian Norris

Commit 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set") attempted to provide some default settings for MTDs that
(a) assign the parent device and
(b) don't provide their own name or owner
However, this isn't a perfect drop-in replacement for the boilerplate found in some drivers, because the MTD name is used by partition parsers like cmdlinepart, but the name isn't set until add_mtd_device(), after the parsing is completed. This means cmdlinepart sees a NULL name and therefore will not work properly.
Fix this by moving the default name and owner assignment to be first in the MTD registration process.

Quoted 7 lines

Fixes 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set")
Reported-by Heiko Schocher <[email protected]>
Signed-off-by Brian Norris <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Frans Klaver <[email protected]>
---
Heiko, can you provide testing feedback (e.g., 'Tested-by: ...')?

Applied this patch to linux-mtd.git, even though it doesn't completely fix the reported problem -- it fixes many other MTD drivers, but drivers/mtd/nand/ has an additional issue. I'll resend the additional patch that fixes Heiko's problem completely in a bit.

Brian