mtd: Fix partition handling code

PATCH Superseded Boris Brezillon 3 participants 10 messages Oct 31, 2018 – Nov 10, 2018 Memory technology devices (MTD)

lore

Share & tools

lore

Boris Brezillon

author

2018-10-31 10:07 UTC #0 lore raw

cover mtd: Fix partition handling code

Hello,

Stefan recently reported a bug when using ubi part on a spi-nand device and proposed this fix[1]. While his solution while working I proposed a different approach to kill the CMD_UBI dependency on CMD_MTDPARTS. This is what patches 1 and 2 are doing.

Patch 3 is just removing the duplicate Kconfig MTD_PARTITIONS entry.

Jagan, can you queue those patches to one of your branch and send a fixes PR, unless Tom prefers to take them directly.

Regards,

Boris

Boris Brezillon (3):
mtd: Use default mtdparts/mtids when not defined in the environment
cmd: ubi: Remove useless call to mtdparts_init()
mtd: Drop duplicate MTD_PARTITIONS Kconfig option

 cmd/Kconfig             |  1 -
 cmd/ubi.c               |  5 ----
 drivers/mtd/Kconfig     |  6 ----
 drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++--
 4 files changed, 60 insertions(+), 14 deletions(-)

Signature · 1 line

2.17.1

Boris Brezillon

author

2018-10-31 10:07 UTC #1 lore raw

PATCH 1/3 mtd: Use default mtdparts/mtids when not defined in the environment

U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.

This implementation is based on the logic found in cmd/mtdparts.c.

Fixes 5db66b3aee6f ("cmd: mtd: add 'mtd' command")

Reported-by Stefan Roese <[email protected]>

Signed-off-by Boris Brezillon <[email protected]>

Tested-by Stefan Roese <[email protected]>

---
 drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

drivers/mtd/mtd_uboot.c +60 -2

index 7d7a11c990d6..1d0a505754f2 100644
@@ -92,12 +92,70 @@ static void mtd_probe_uclass_mtd_devs(void) { }
 #endif
 
 #if defined(CONFIG_MTD_PARTITIONS)
+extern void board_mtdparts_default(const char **mtdids,
+				   const char **mtdparts);
+
+static const char *get_mtdids(void)
+{
+	__maybe_unused const char *mtdparts = NULL;
+	const char *mtdids = env_get("mtdids");
+
+	if (mtdids)
+		return mtdids;
+
+#if defined(CONFIG_SYS_MTDPARTS_RUNTIME)
+	board_mtdparts_default(&mtdids, &mtdparts);
+#elif defined(MTDIDS_DEFAULT)
+	mtdids = MTDIDS_DEFAULT;
+#elif defined(CONFIG_MTDIDS_DEFAULT)
+	mtdids = CONFIG_MTDIDS_DEFAULT;
+#endif
+
+	if (mtdids)
+		env_set("mtdids", mtdids);
+
+	return mtdids;
+}
+
+#define MTDPARTS_MAXLEN         512
+
+static const char *get_mtdparts(void)
+{
+	__maybe_unused const char *mtdids = NULL;
+	static char tmp_parts[MTDPARTS_MAXLEN];
+	static bool use_defaults = true;
+	const char *mtdparts = NULL;
+
+	if (gd->flags & GD_FLG_ENV_READY)
+		mtdparts = env_get("mtdparts");
+	else if (env_get_f("mtdparts", tmp_parts, sizeof(tmp_parts) != -1))
+		mtdparts = tmp_parts;
+
+	if (mtdparts || !use_defaults)
+		return mtdparts;
+
+#if defined(CONFIG_SYS_MTDPARTS_RUNTIME)
+	board_mtdparts_default(&mtdids, &mtdparts);
+#elif defined(MTDPARTS_DEFAULT)
+	mtdparts = MTDPARTS_DEFAULT;
+#elif defined(CONFIG_MTDPARTS_DEFAULT)
+	mtdparts = CONFIG_MTDPARTS_DEFAULT;
+#endif
+
+	if (mtdparts)
+		env_set("mtdparts", mtdparts);
+
+	use_defaults = false;
+
+	return mtdparts;
+}
+
 int mtd_probe_devices(void)
 {
 	static char *old_mtdparts;
 	static char *old_mtdids;
-	const char *mtdparts = env_get("mtdparts");
-	const char *mtdids = env_get("mtdids");
+	const char *mtdparts = get_mtdparts();
+	const char *mtdids = get_mtdids();
 	bool remaining_partitions = true;
 	struct mtd_info *mtd;

Signature · 1 line

2.17.1

Boris Brezillon

author

2018-10-31 10:07 UTC #2 lore raw

PATCH 2/3 cmd: ubi: Remove useless call to mtdparts_init()

In reply to Boris Brezillon

Commit c58fb2cdb3e4 ("cmd: ubi: clean the partition handling") introduced a call to mtd_probe_devices() in the ubi_attach() path and this function takes care of parsing mtdparts/mtdids and creating/registering the associated mtd partitions.

The mtdparts_init() call in the ubi_detach() path is not only unnecessary but can sometimes print error messages even when things work properly (that's the case with SPI NAND devices that have not been probed with 'mtd list'), which is misleading.

Remove this call to mtdparts_init() and drop the dependency on CMD_MTDPARTS.

Fixes c58fb2cdb3e4 ("cmd: ubi: clean the partition handling")

Reported-by Stefan Roese <[email protected]>

Signed-off-by Boris Brezillon <[email protected]>

Tested-by Stefan Roese <[email protected]>

---
 cmd/Kconfig | 1 -
 cmd/ubi.c   | 5 -----
 2 files changed, 6 deletions(-)

cmd/Kconfig +0 -1

index d66f710ad0f8..b47e7fe80dbb 100644
@@ -1856,7 +1856,6 @@ endmenu
 
 config CMD_UBI
 	tristate "Enable UBI - Unsorted block images commands"
-	select CMD_MTDPARTS
 	select CRC32
 	select MTD_UBI
 	help

cmd/ubi.c +0 -5

index 767a4a453640..2b74a9814463 100644
@@ -417,11 +417,6 @@ static int ubi_dev_scan(struct mtd_info *info, const char *vid_header_offset)
 
 int ubi_detach(void)
 {
-	if (mtdparts_init() != 0) {
-		printf("Error initializing mtdparts!\n");
-		return 1;
-	}
-
 #ifdef CONFIG_CMD_UBIFS
 	/*
 	 * Automatically unmount UBIFS partition when user

Signature · 1 line

2.17.1

Boris Brezillon

author

2018-10-31 10:07 UTC #3 lore raw

PATCH 3/3 mtd: Drop duplicate MTD_PARTITIONS Kconfig option

In reply to Boris Brezillon

Commit 9c5b00973bce ("Convert CONFIG_MTD_PARTITIONS et al to Kconfig") introduced a publicly visible Kconfig entry for the CONFIG_MTD_PARTITIONS option, while the rework on MTD partitioning was in progress, and we somehow did not notice that the same Kconfig entry was added by commit 4048a5c519a8 ("mtd: declare MTD_PARTITIONS symbol in Kconfig"), but this time as an invisible entry (this can only be selected by other options).

Keep the non-visible version of this symbol, since MTD_PARTITIONS is not something the user should be able to enable/disable directly.

Fixes 4048a5c519a8 ("mtd: declare MTD_PARTITIONS symbol in Kconfig")

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/mtd/Kconfig | 6 ------
 1 file changed, 6 deletions(-)

drivers/mtd/Kconfig +0 -6

index 11cf12bb5599..0050fb2b9bf1 100644
@@ -22,12 +22,6 @@ config MTD_DEVICE
 	  Adds the MTD device infrastructure from the Linux kernel.
 	  Needed for mtdparts command support.
 
-config MTD_PARTITIONS
-	bool "Add MTD Partioning infrastructure"
-	help
-	  Adds the MTD partitioning infrastructure from the Linux
-	  kernel. Needed for UBI support.
-
 config FLASH_CFI_DRIVER
 	bool "Enable CFI Flash driver"
 	help

Signature · 1 line

2.17.1

Miquel Raynal

maintainer

2018-10-31 10:43 UTC #4 lore raw

cover mtd: Fix partition handling code

In reply to Boris Brezillon

Hi Boris,

Boris Brezillon <[email protected]> wrote on Wed, 31 Oct 2018 11:07:33 +0100:

Quoted 12 lines

Hello,
Stefan recently reported a bug when using ubi part on a spi-nand device and proposed this fix[1]. While his solution while working I proposed a different approach to kill the CMD_UBI dependency on CMD_MTDPARTS. This is what patches 1 and 2 are doing.
Patch 3 is just removing the duplicate Kconfig MTD_PARTITIONS entry.
Jagan, can you queue those patches to one of your branch and send a fixes PR, unless Tom prefers to take them directly.

Quoted 8 lines

Regards,
Boris
Boris Brezillon (3):
mtd: Use default mtdparts/mtids when not defined in the environment
cmd: ubi: Remove useless call to mtdparts_init()
mtd: Drop duplicate MTD_PARTITIONS Kconfig option

Strange, we already had a discussion about it. But indeed this patch is needed.

Quoted 7 lines

 cmd/Kconfig             |  1 -
 cmd/ubi.c               |  5 ----
 drivers/mtd/Kconfig     |  6 ----
 drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++--
 4 files changed, 60 insertions(+), 14 deletions(-)

Reviewed-by Miquel Raynal <[email protected]>

Thanks,
MiquÃ¨l

Jagan Teki

2018-11-05 08:37 UTC #5 lore raw

cover mtd: Fix partition handling code

In reply to Boris Brezillon

Quoted 13 lines · Boris Brezillon

Hello,
Stefan recently reported a bug when using ubi part on a spi-nand device and proposed this fix[1]. While his solution while working I proposed a different approach to kill the CMD_UBI dependency on CMD_MTDPARTS. This is what patches 1 and 2 are doing.
Patch 3 is just removing the duplicate Kconfig MTD_PARTITIONS entry.
Jagan, can you queue those patches to one of your branch and send a fixes PR, unless Tom prefers to take them directly.

Quoted 8 lines

Regards,
Boris
Boris Brezillon (3):
mtd: Use default mtdparts/mtids when not defined in the environment
cmd: ubi: Remove useless call to mtdparts_init()
mtd: Drop duplicate MTD_PARTITIONS Kconfig option

Applied to u-boot-spi/master

Jagan Teki

2018-11-10 09:32 UTC #6 lore raw

cover mtd: Fix partition handling code

Quoted 16 lines · Jagan Teki

Quoted 13 lines · Boris Brezillon
Hello,
Stefan recently reported a bug when using ubi part on a spi-nand device and proposed this fix[1]. While his solution while working I proposed a different approach to kill the CMD_UBI dependency on CMD_MTDPARTS. This is what patches 1 and 2 are doing.
Patch 3 is just removing the duplicate Kconfig MTD_PARTITIONS entry.
Jagan, can you queue those patches to one of your branch and send a fixes PR, unless Tom prefers to take them directly.

Quoted 8 lines

Quoted 8 lines
Regards,
Boris
Boris Brezillon (3):
mtd: Use default mtdparts/mtids when not defined in the environment
cmd: ubi: Remove useless call to mtdparts_init()
mtd: Drop duplicate MTD_PARTITIONS Kconfig option

Did you find this build issues? [1]
https://travis-ci.org/openedev/u-boot-amarula/builds/450798634

Boris Brezillon

author

2018-11-10 11:23 UTC #7 lore raw

cover mtd: Fix partition handling code

Quoted 21 lines · On Sat, 10 Nov 2018 15:02:05 +0530 Jagan Teki <[email protected]>…

Quoted 20 lines · Jagan Teki
Quoted 17 lines · Boris Brezillon
Hello,
Stefan recently reported a bug when using ubi part on a spi-nand device and proposed this fix[1]. While his solution while working I proposed a different approach to kill the CMD_UBI dependency on CMD_MTDPARTS. This is what patches 1 and 2 are doing.
Patch 3 is just removing the duplicate Kconfig MTD_PARTITIONS entry.
Jagan, can you queue those patches to one of your branch and send a fixes PR, unless Tom prefers to take them directly.
Regards,
Boris

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
Boris Brezillon (3):
mtd: Use default mtdparts/mtids when not defined in the environment
cmd: ubi: Remove useless call to mtdparts_init()
mtd: Drop duplicate MTD_PARTITIONS Kconfig option
Did you find this build issues? [1]
https://travis-ci.org/openedev/u-boot-amarula/builds/450798634

If by find you mean notice, then no, but I just sent 2 patches to fix the problems.

Thanks for reporting the bugs.

Boris

Jagan Teki

2018-11-10 11:27 UTC #8 lore raw

cover mtd: Fix partition handling code

Quoted 30 lines · Boris Brezillon

Quoted 26 lines · On Sat, 10 Nov 2018 15:02:05 +0530 Jagan Teki <[email protected]>…
Quoted 24 lines · Jagan Teki
Quoted 21 lines · Boris Brezillon
Hello,
Stefan recently reported a bug when using ubi part on a spi-nand device and proposed this fix[1]. While his solution while working I proposed a different approach to kill the CMD_UBI dependency on CMD_MTDPARTS. This is what patches 1 and 2 are doing.
Patch 3 is just removing the duplicate Kconfig MTD_PARTITIONS entry.
Jagan, can you queue those patches to one of your branch and send a fixes PR, unless Tom prefers to take them directly.
Regards,
Boris
Boris Brezillon (3):
mtd: Use default mtdparts/mtids when not defined in the environment
cmd: ubi: Remove useless call to mtdparts_init()
mtd: Drop duplicate MTD_PARTITIONS Kconfig option

Quoted 7 lines

Quoted 2 lines
Did you find this build issues? [1]
https://travis-ci.org/openedev/u-boot-amarula/builds/450798634
If by find you mean notice, then no, but I just sent 2 patches to fix the problems.
Thanks for reporting the bugs.

Look like enabling CMD_MTDPARTS=y to relevant defconfigs were fine, since other boards did the same. I made change trigger a build [2]

[2] https://travis-ci.org/openedev/u-boot-amarula/builds/453235656

Boris Brezillon

author

2018-11-10 11:33 UTC #9 lore raw

cover mtd: Fix partition handling code

Quoted 35 lines · On Sat, 10 Nov 2018 16:57:07 +0530 Jagan Teki <[email protected]>…

Quoted 33 lines · Boris Brezillon
Quoted 28 lines · On Sat, 10 Nov 2018 15:02:05 +0530 Jagan Teki <[email protected]>…
Quoted 16 lines · Jagan Teki
Quoted 13 lines · Boris Brezillon
Hello,
Stefan recently reported a bug when using ubi part on a spi-nand device and proposed this fix[1]. While his solution while working I proposed a different approach to kill the CMD_UBI dependency on CMD_MTDPARTS. This is what patches 1 and 2 are doing.
Patch 3 is just removing the duplicate Kconfig MTD_PARTITIONS entry.
Jagan, can you queue those patches to one of your branch and send a fixes PR, unless Tom prefers to take them directly.
Quoted 8 lines
Quoted 8 lines
Regards,
Boris
Boris Brezillon (3):
mtd: Use default mtdparts/mtids when not defined in the environment
cmd: ubi: Remove useless call to mtdparts_init()
mtd: Drop duplicate MTD_PARTITIONS Kconfig option
Did you find this build issues? [1]
https://travis-ci.org/openedev/u-boot-amarula/builds/450798634

Quoted 7 lines

Quoted 4 lines
If by find you mean notice, then no, but I just sent 2 patches to fix the problems.
Thanks for reporting the bugs.
Look like enabling CMD_MTDPARTS=y to relevant defconfigs were fine, since other boards did the same. I made change trigger a build [2]

Well, there's still a problem with dependencies. For instance, DFU_NAND should select CMD_MTDPARTS since it's using a function exposed by cmd/mtdparts.c. For sure, adding CMD_MTDPARTS=y to existing defconfigs does the trick, but it's not the correct fix IMHO.

Quoted 2 lines

[2] https://travis-ci.org/openedev/u-boot-amarula/builds/453235656