[U-Boot] [PATCH 0/3] mtd: Fix partition handling code

[U-Boot] [PATCH 0/3] mtd: Fix partition handling code

[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

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

-- 
2.17.1

[U-Boot] [PATCH 1/3] mtd: Use default mtdparts/mtids when not defined in the environment

[Boris Brezillon]

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 <sr@denx.de>
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Tested-by: Stefan Roese <sr@denx.de>
---
 drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
index 7d7a11c990d6..1d0a505754f2 100644
--- a/drivers/mtd/mtd_uboot.c
+++ b/drivers/mtd/mtd_uboot.c
@@ -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;
 
-- 
2.17.1

[U-Boot] [PATCH 2/3] cmd: ubi: Remove useless call to mtdparts_init()

[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 <sr@denx.de>
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Tested-by: Stefan Roese <sr@denx.de>
---
 cmd/Kconfig | 1 -
 cmd/ubi.c   | 5 -----
 2 files changed, 6 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index d66f710ad0f8..b47e7fe80dbb 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1856,7 +1856,6 @@ endmenu
 
 config CMD_UBI
 	tristate "Enable UBI - Unsorted block images commands"
-	select CMD_MTDPARTS
 	select CRC32
 	select MTD_UBI
 	help
diff --git a/cmd/ubi.c b/cmd/ubi.c
index 767a4a453640..2b74a9814463 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -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
-- 
2.17.1

[U-Boot] [PATCH 3/3] mtd: Drop duplicate MTD_PARTITIONS Kconfig option

[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 <boris.brezillon@bootlin.com>
---
 drivers/mtd/Kconfig | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 11cf12bb5599..0050fb2b9bf1 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -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
-- 
2.17.1

[U-Boot] [PATCH 0/3] mtd: Fix partition handling code

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Wed, 31 Oct 2018
11:07:33 +0100:

> 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

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

> 
>  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 <miquel.raynal@bootlin.com>


Thanks,
Miquèl

[U-Boot] [PATCH 0/3] mtd: Fix partition handling code

[Jagan Teki]

On Wed, Oct 31, 2018 at 3:38 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> 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

Applied to u-boot-spi/master

[U-Boot] [PATCH 0/3] mtd: Fix partition handling code

[Jagan Teki]

On Mon, Nov 5, 2018 at 2:07 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Wed, Oct 31, 2018 at 3:38 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > 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

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

[U-Boot] [PATCH 0/3] mtd: Fix partition handling code

[Boris Brezillon]

On Sat, 10 Nov 2018 15:02:05 +0530
Jagan Teki <jagan@amarulasolutions.com> wrote:

> On Mon, Nov 5, 2018 at 2:07 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Wed, Oct 31, 2018 at 3:38 PM Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:  
> > >
> > > 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  
> 
> 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

[U-Boot] [PATCH 0/3] mtd: Fix partition handling code

[Jagan Teki]

On Sat, Nov 10, 2018 at 4:54 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Sat, 10 Nov 2018 15:02:05 +0530
> Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> > On Mon, Nov 5, 2018 at 2:07 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > On Wed, Oct 31, 2018 at 3:38 PM Boris Brezillon
> > > <boris.brezillon@bootlin.com> wrote:
> > > >
> > > > 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
> >
> > 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

[U-Boot] [PATCH 0/3] mtd: Fix partition handling code

[Boris Brezillon]

On Sat, 10 Nov 2018 16:57:07 +0530
Jagan Teki <jagan@amarulasolutions.com> wrote:

> On Sat, Nov 10, 2018 at 4:54 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > On Sat, 10 Nov 2018 15:02:05 +0530
> > Jagan Teki <jagan@amarulasolutions.com> wrote:
> >  
> > > On Mon, Nov 5, 2018 at 2:07 PM Jagan Teki <jagan@amarulasolutions.com> wrote:  
> > > >
> > > > On Wed, Oct 31, 2018 at 3:38 PM Boris Brezillon
> > > > <boris.brezillon@bootlin.com> wrote:  
> > > > >
> > > > > 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  
> > >
> > > 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]

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.

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