[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Stefan Roese]

Calling "mtdparts" currently fails when its called before any other mtd
command (or ubi command) has been called. The MTD devices are not
probed at this point and therefore it fails e.g. with this message:

=> mtdparts
Device spi-nand0 not found!

This patch adds a call to mtd_probe_devices() to mtdparts_init() to
solve this issue. This also fixes a problem when calling "ubi part"
as first flash storage related command. Here also the warning from
above is printed without this patch.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: Jagan Teki <jagan@openedev.com>
---
 cmd/mtdparts.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index f7ed1a0779..d90e568143 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -73,6 +73,7 @@
 #include <common.h>
 #include <command.h>
 #include <malloc.h>
+#include <mtd.h>
 #include <jffs2/load_kernel.h>
 #include <linux/list.h>
 #include <linux/ctype.h>
@@ -1726,6 +1727,9 @@ int mtdparts_init(void)
 	char tmp_ep[PARTITION_MAXLEN + 1];
 	char tmp_parts[MTDPARTS_MAXLEN];
 
+	/* First probe all MTD devices */
+	mtd_probe_devices();
+
 	debug("\n---mtdparts_init---\n");
 	if (!initialized) {
 		INIT_LIST_HEAD(&mtdids);
-- 
2.19.1

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Boris Brezillon]

On Tue, 30 Oct 2018 10:51:51 +0100
Stefan Roese <sr@denx.de> wrote:

> Calling "mtdparts" currently fails when its called before any other mtd
> command (or ubi command) has been called. The MTD devices are not
> probed at this point and therefore it fails e.g. with this message:
> 
> => mtdparts  
> Device spi-nand0 not found!

IIRC, we decided that mtdparts should not call mtd_probe_devices() to
encourage people to stop using it.

> 
> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
> solve this issue. This also fixes a problem when calling "ubi part"
> as first flash storage related command.

Hm, this one is unexpected. Miquel, any idea why this happens. Do we
need to enable a specific option if we want mtd_probe_devices() to be
called in the ubi part path?

> Here also the warning from
> above is printed without this patch.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
> Cc: Jagan Teki <jagan@openedev.com>
> ---
>  cmd/mtdparts.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
> index f7ed1a0779..d90e568143 100644
> --- a/cmd/mtdparts.c
> +++ b/cmd/mtdparts.c
> @@ -73,6 +73,7 @@
>  #include <common.h>
>  #include <command.h>
>  #include <malloc.h>
> +#include <mtd.h>
>  #include <jffs2/load_kernel.h>
>  #include <linux/list.h>
>  #include <linux/ctype.h>
> @@ -1726,6 +1727,9 @@ int mtdparts_init(void)
>  	char tmp_ep[PARTITION_MAXLEN + 1];
>  	char tmp_parts[MTDPARTS_MAXLEN];
>  
> +	/* First probe all MTD devices */
> +	mtd_probe_devices();
> +
>  	debug("\n---mtdparts_init---\n");
>  	if (!initialized) {
>  		INIT_LIST_HEAD(&mtdids);

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Stefan Roese]

Hi Boris,

On 30.10.18 11:03, Boris Brezillon wrote:
> On Tue, 30 Oct 2018 10:51:51 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>> Calling "mtdparts" currently fails when its called before any other mtd
>> command (or ubi command) has been called. The MTD devices are not
>> probed at this point and therefore it fails e.g. with this message:
>>
>> => mtdparts
>> Device spi-nand0 not found!
> 
> IIRC, we decided that mtdparts should not call mtd_probe_devices() to
> encourage people to stop using it.

I see. But I don't quite get how this missing call (and reslting
error message) would encourage people to stop using it.  
>>
>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
>> solve this issue. This also fixes a problem when calling "ubi part"
>> as first flash storage related command.
> 
> Hm, this one is unexpected. Miquel, any idea why this happens. Do we
> need to enable a specific option if we want mtd_probe_devices() to be
> called in the ubi part path?

Please note that "ubi part part-foo" does still work. It only
prints this error message before attaching the MTD partition.
The error is printed because of this call-chain:

ubi_part()
->  ubi_detach()
     -> mtdparts_init()

So again, mtdparts_init() is called without the MTD devices
being probed.

Thanks,
Stefan
  
>> Here also the warning from
>> above is printed without this patch.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
>> Cc: Jagan Teki <jagan@openedev.com>
>> ---
>>   cmd/mtdparts.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
>> index f7ed1a0779..d90e568143 100644
>> --- a/cmd/mtdparts.c
>> +++ b/cmd/mtdparts.c
>> @@ -73,6 +73,7 @@
>>   #include <common.h>
>>   #include <command.h>
>>   #include <malloc.h>
>> +#include <mtd.h>
>>   #include <jffs2/load_kernel.h>
>>   #include <linux/list.h>
>>   #include <linux/ctype.h>
>> @@ -1726,6 +1727,9 @@ int mtdparts_init(void)
>>   	char tmp_ep[PARTITION_MAXLEN + 1];
>>   	char tmp_parts[MTDPARTS_MAXLEN];
>>   
>> +	/* First probe all MTD devices */
>> +	mtd_probe_devices();
>> +
>>   	debug("\n---mtdparts_init---\n");
>>   	if (!initialized) {
>>   		INIT_LIST_HEAD(&mtdids);
> 

Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Boris Brezillon]

On Tue, 30 Oct 2018 11:13:37 +0100
Stefan Roese <sr@denx.de> wrote:

> Hi Boris,
> 
> On 30.10.18 11:03, Boris Brezillon wrote:
> > On Tue, 30 Oct 2018 10:51:51 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >> Calling "mtdparts" currently fails when its called before any other mtd
> >> command (or ubi command) has been called. The MTD devices are not
> >> probed at this point and therefore it fails e.g. with this message:
> >>  
> >> => mtdparts  
> >> Device spi-nand0 not found!  
> > 
> > IIRC, we decided that mtdparts should not call mtd_probe_devices() to
> > encourage people to stop using it.  
> 
> I see. But I don't quite get how this missing call (and reslting
> error message) would encourage people to stop using it.

You're right, this message does not encourage people to stop using
mtdparts on existing setups (mtdparts should work just fine on any MTD
devices except SPI NANDs) but it does discourage them from using it on
spi-nand devices since it returns an error.

> >>
> >> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
> >> solve this issue. This also fixes a problem when calling "ubi part"
> >> as first flash storage related command.  
> > 
> > Hm, this one is unexpected. Miquel, any idea why this happens. Do we
> > need to enable a specific option if we want mtd_probe_devices() to be
> > called in the ubi part path?  
> 
> Please note that "ubi part part-foo" does still work. It only
> prints this error message before attaching the MTD partition.
> The error is printed because of this call-chain:
> 
> ubi_part()
> ->  ubi_detach()
>      -> mtdparts_init()  
> 
> So again, mtdparts_init() is called without the MTD devices
> being probed.

I guess we forgot to remove this mtdparts_init() call from the detach
path. I think it's no longer needed since we now call
mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take
care of creating MTD partitions based on the mtdparts= and mtdids=
variables.

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Stefan Roese]

On 30.10.18 11:41, Boris Brezillon wrote:
> On Tue, 30 Oct 2018 11:13:37 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>> Hi Boris,
>>
>> On 30.10.18 11:03, Boris Brezillon wrote:
>>> On Tue, 30 Oct 2018 10:51:51 +0100
>>> Stefan Roese <sr@denx.de> wrote:
>>>    
>>>> Calling "mtdparts" currently fails when its called before any other mtd
>>>> command (or ubi command) has been called. The MTD devices are not
>>>> probed at this point and therefore it fails e.g. with this message:
>>>>   
>>>> => mtdparts
>>>> Device spi-nand0 not found!
>>>
>>> IIRC, we decided that mtdparts should not call mtd_probe_devices() to
>>> encourage people to stop using it.
>>
>> I see. But I don't quite get how this missing call (and reslting
>> error message) would encourage people to stop using it.
> 
> You're right, this message does not encourage people to stop using
> mtdparts on existing setups (mtdparts should work just fine on any MTD
> devices except SPI NANDs) but it does discourage them from using it on
> spi-nand devices since it returns an error.

IMHO, that's more confusing than discouraging.
  
>>>>
>>>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
>>>> solve this issue. This also fixes a problem when calling "ubi part"
>>>> as first flash storage related command.
>>>
>>> Hm, this one is unexpected. Miquel, any idea why this happens. Do we
>>> need to enable a specific option if we want mtd_probe_devices() to be
>>> called in the ubi part path?
>>
>> Please note that "ubi part part-foo" does still work. It only
>> prints this error message before attaching the MTD partition.
>> The error is printed because of this call-chain:
>>
>> ubi_part()
>> ->  ubi_detach()
>>       -> mtdparts_init()
>>
>> So again, mtdparts_init() is called without the MTD devices
>> being probed.
> 
> I guess we forgot to remove this mtdparts_init() call from the detach
> path. I think it's no longer needed since we now call
> mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take
> care of creating MTD partitions based on the mtdparts= and mtdids=
> variables.

A quick test reveals that this removal does not remove the
error message. Instead the command does not work anymore at
all:

=> ubi part nand
Partition nand not found!

Before (and without my patch) its this:

=> ubi part nand
Device spi-nand0 not found!
Error initializing mtdparts!
ubi0: attaching mtd2
ubi0: scanning is finished
ubi0: attached mtd2 (name "nand", size 128 MiB)
...

Thanks,
Stefan

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Boris Brezillon]

On Tue, 30 Oct 2018 11:59:13 +0100
Stefan Roese <sr@denx.de> wrote:

> On 30.10.18 11:41, Boris Brezillon wrote:
> > On Tue, 30 Oct 2018 11:13:37 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >> Hi Boris,
> >>
> >> On 30.10.18 11:03, Boris Brezillon wrote:  
> >>> On Tue, 30 Oct 2018 10:51:51 +0100
> >>> Stefan Roese <sr@denx.de> wrote:
> >>>      
> >>>> Calling "mtdparts" currently fails when its called before any other mtd
> >>>> command (or ubi command) has been called. The MTD devices are not
> >>>> probed at this point and therefore it fails e.g. with this message:
> >>>>     
> >>>> => mtdparts  
> >>>> Device spi-nand0 not found!  
> >>>
> >>> IIRC, we decided that mtdparts should not call mtd_probe_devices() to
> >>> encourage people to stop using it.  
> >>
> >> I see. But I don't quite get how this missing call (and reslting
> >> error message) would encourage people to stop using it.  
> > 
> > You're right, this message does not encourage people to stop using
> > mtdparts on existing setups (mtdparts should work just fine on any MTD
> > devices except SPI NANDs) but it does discourage them from using it on
> > spi-nand devices since it returns an error.  
> 
> IMHO, that's more confusing than discouraging.

Maybe we should have a

	depends on !CMD_MTDPARTS

in the CMD_MTD entry. Since spi-nand can only be accessed from the
cmdline through the mtd command, that should cover most cases.

>   
> >>>>
> >>>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
> >>>> solve this issue. This also fixes a problem when calling "ubi part"
> >>>> as first flash storage related command.  
> >>>
> >>> Hm, this one is unexpected. Miquel, any idea why this happens. Do we
> >>> need to enable a specific option if we want mtd_probe_devices() to be
> >>> called in the ubi part path?  
> >>
> >> Please note that "ubi part part-foo" does still work. It only
> >> prints this error message before attaching the MTD partition.
> >> The error is printed because of this call-chain:
> >>
> >> ubi_part()  
> >> ->  ubi_detach()
> >>       -> mtdparts_init()  
> >>
> >> So again, mtdparts_init() is called without the MTD devices
> >> being probed.  
> > 
> > I guess we forgot to remove this mtdparts_init() call from the detach
> > path. I think it's no longer needed since we now call
> > mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take
> > care of creating MTD partitions based on the mtdparts= and mtdids=
> > variables.  
> 
> A quick test reveals that this removal does not remove the
> error message. Instead the command does not work anymore at
> all:
> 
> => ubi part nand  
> Partition nand not found!

Hm, there's a problem. The partition creation should work even if
mtdparts_init() is not called. Can you check that CONFG_MTD_PARTITIONS
is selected, and if it is, add traces in mtd_probe_devices() to see
where it fails.

> 
> Before (and without my patch) its this:
> 
> => ubi part nand  
> Device spi-nand0 not found!
> Error initializing mtdparts!
> ubi0: attaching mtd2
> ubi0: scanning is finished
> ubi0: attached mtd2 (name "nand", size 128 MiB)
> ...
> 
> Thanks,
> Stefan

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Boris Brezillon]

On Tue, 30 Oct 2018 11:59:13 +0100
Stefan Roese <sr@denx.de> wrote:

> On 30.10.18 11:41, Boris Brezillon wrote:
> > On Tue, 30 Oct 2018 11:13:37 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >> Hi Boris,
> >>
> >> On 30.10.18 11:03, Boris Brezillon wrote:  
> >>> On Tue, 30 Oct 2018 10:51:51 +0100
> >>> Stefan Roese <sr@denx.de> wrote:
> >>>      
> >>>> Calling "mtdparts" currently fails when its called before any other mtd
> >>>> command (or ubi command) has been called. The MTD devices are not
> >>>> probed at this point and therefore it fails e.g. with this message:
> >>>>     
> >>>> => mtdparts  
> >>>> Device spi-nand0 not found!  
> >>>
> >>> IIRC, we decided that mtdparts should not call mtd_probe_devices() to
> >>> encourage people to stop using it.  
> >>
> >> I see. But I don't quite get how this missing call (and reslting
> >> error message) would encourage people to stop using it.  
> > 
> > You're right, this message does not encourage people to stop using
> > mtdparts on existing setups (mtdparts should work just fine on any MTD
> > devices except SPI NANDs) but it does discourage them from using it on
> > spi-nand devices since it returns an error.  
> 
> IMHO, that's more confusing than discouraging.
>   
> >>>>
> >>>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
> >>>> solve this issue. This also fixes a problem when calling "ubi part"
> >>>> as first flash storage related command.  
> >>>
> >>> Hm, this one is unexpected. Miquel, any idea why this happens. Do we
> >>> need to enable a specific option if we want mtd_probe_devices() to be
> >>> called in the ubi part path?  
> >>
> >> Please note that "ubi part part-foo" does still work. It only
> >> prints this error message before attaching the MTD partition.
> >> The error is printed because of this call-chain:
> >>
> >> ubi_part()  
> >> ->  ubi_detach()
> >>       -> mtdparts_init()  
> >>
> >> So again, mtdparts_init() is called without the MTD devices
> >> being probed.  
> > 
> > I guess we forgot to remove this mtdparts_init() call from the detach
> > path. I think it's no longer needed since we now call
> > mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take
> > care of creating MTD partitions based on the mtdparts= and mtdids=
> > variables.  
> 
> A quick test reveals that this removal does not remove the
> error message. Instead the command does not work anymore at
> all:
> 
> => ubi part nand  
> Partition nand not found!
> 
> Before (and without my patch) its this:
> 
> => ubi part nand  
> Device spi-nand0 not found!
> Error initializing mtdparts!
> ubi0: attaching mtd2
> ubi0: scanning is finished
> ubi0: attached mtd2 (name "nand", size 128 MiB)

I think I found what's missing in mtd_probe_devices(): we don't use the
default mtdparts and mtdids when those env vars are NULL (see what's
done in mtdparts_init() to handle this case [1]).

[1]https://elixir.bootlin.com/u-boot/v2018.11-rc3/source/cmd/mtdparts.c#L1763

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Boris Brezillon]

On Tue, 30 Oct 2018 23:02:50 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Tue, 30 Oct 2018 11:59:13 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
> > On 30.10.18 11:41, Boris Brezillon wrote:  
> > > On Tue, 30 Oct 2018 11:13:37 +0100
> > > Stefan Roese <sr@denx.de> wrote:
> > >     
> > >> Hi Boris,
> > >>
> > >> On 30.10.18 11:03, Boris Brezillon wrote:    
> > >>> On Tue, 30 Oct 2018 10:51:51 +0100
> > >>> Stefan Roese <sr@denx.de> wrote:
> > >>>        
> > >>>> Calling "mtdparts" currently fails when its called before any other mtd
> > >>>> command (or ubi command) has been called. The MTD devices are not
> > >>>> probed at this point and therefore it fails e.g. with this message:
> > >>>>       
> > >>>> => mtdparts    
> > >>>> Device spi-nand0 not found!    
> > >>>
> > >>> IIRC, we decided that mtdparts should not call mtd_probe_devices() to
> > >>> encourage people to stop using it.    
> > >>
> > >> I see. But I don't quite get how this missing call (and reslting
> > >> error message) would encourage people to stop using it.    
> > > 
> > > You're right, this message does not encourage people to stop using
> > > mtdparts on existing setups (mtdparts should work just fine on any MTD
> > > devices except SPI NANDs) but it does discourage them from using it on
> > > spi-nand devices since it returns an error.    
> > 
> > IMHO, that's more confusing than discouraging.
> >     
> > >>>>
> > >>>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
> > >>>> solve this issue. This also fixes a problem when calling "ubi part"
> > >>>> as first flash storage related command.    
> > >>>
> > >>> Hm, this one is unexpected. Miquel, any idea why this happens. Do we
> > >>> need to enable a specific option if we want mtd_probe_devices() to be
> > >>> called in the ubi part path?    
> > >>
> > >> Please note that "ubi part part-foo" does still work. It only
> > >> prints this error message before attaching the MTD partition.
> > >> The error is printed because of this call-chain:
> > >>
> > >> ubi_part()    
> > >> ->  ubi_detach()
> > >>       -> mtdparts_init()    
> > >>
> > >> So again, mtdparts_init() is called without the MTD devices
> > >> being probed.    
> > > 
> > > I guess we forgot to remove this mtdparts_init() call from the detach
> > > path. I think it's no longer needed since we now call
> > > mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take
> > > care of creating MTD partitions based on the mtdparts= and mtdids=
> > > variables.    
> > 
> > A quick test reveals that this removal does not remove the
> > error message. Instead the command does not work anymore at
> > all:
> >   
> > => ubi part nand    
> > Partition nand not found!
> > 
> > Before (and without my patch) its this:
> >   
> > => ubi part nand    
> > Device spi-nand0 not found!
> > Error initializing mtdparts!
> > ubi0: attaching mtd2
> > ubi0: scanning is finished
> > ubi0: attached mtd2 (name "nand", size 128 MiB)  
> 
> I think I found what's missing in mtd_probe_devices(): we don't use the
> default mtdparts and mtdids when those env vars are NULL (see what's
> done in mtdparts_init() to handle this case [1]).

Can you try with the following diff applied?

--->8---
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
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;
 

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Stefan Roese]

On 30.10.18 23:43, Boris Brezillon wrote:
> On Tue, 30 Oct 2018 23:02:50 +0100
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
>> On Tue, 30 Oct 2018 11:59:13 +0100
>> Stefan Roese <sr@denx.de> wrote:
>>
>>> On 30.10.18 11:41, Boris Brezillon wrote:
>>>> On Tue, 30 Oct 2018 11:13:37 +0100
>>>> Stefan Roese <sr@denx.de> wrote:
>>>>      
>>>>> Hi Boris,
>>>>>
>>>>> On 30.10.18 11:03, Boris Brezillon wrote:
>>>>>> On Tue, 30 Oct 2018 10:51:51 +0100
>>>>>> Stefan Roese <sr@denx.de> wrote:
>>>>>>         
>>>>>>> Calling "mtdparts" currently fails when its called before any other mtd
>>>>>>> command (or ubi command) has been called. The MTD devices are not
>>>>>>> probed at this point and therefore it fails e.g. with this message:
>>>>>>>        
>>>>>>> => mtdparts
>>>>>>> Device spi-nand0 not found!
>>>>>>
>>>>>> IIRC, we decided that mtdparts should not call mtd_probe_devices() to
>>>>>> encourage people to stop using it.
>>>>>
>>>>> I see. But I don't quite get how this missing call (and reslting
>>>>> error message) would encourage people to stop using it.
>>>>
>>>> You're right, this message does not encourage people to stop using
>>>> mtdparts on existing setups (mtdparts should work just fine on any MTD
>>>> devices except SPI NANDs) but it does discourage them from using it on
>>>> spi-nand devices since it returns an error.
>>>
>>> IMHO, that's more confusing than discouraging.
>>>      
>>>>>>>
>>>>>>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
>>>>>>> solve this issue. This also fixes a problem when calling "ubi part"
>>>>>>> as first flash storage related command.
>>>>>>
>>>>>> Hm, this one is unexpected. Miquel, any idea why this happens. Do we
>>>>>> need to enable a specific option if we want mtd_probe_devices() to be
>>>>>> called in the ubi part path?
>>>>>
>>>>> Please note that "ubi part part-foo" does still work. It only
>>>>> prints this error message before attaching the MTD partition.
>>>>> The error is printed because of this call-chain:
>>>>>
>>>>> ubi_part()
>>>>> ->  ubi_detach()
>>>>>        -> mtdparts_init()
>>>>>
>>>>> So again, mtdparts_init() is called without the MTD devices
>>>>> being probed.
>>>>
>>>> I guess we forgot to remove this mtdparts_init() call from the detach
>>>> path. I think it's no longer needed since we now call
>>>> mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take
>>>> care of creating MTD partitions based on the mtdparts= and mtdids=
>>>> variables.
>>>
>>> A quick test reveals that this removal does not remove the
>>> error message. Instead the command does not work anymore at
>>> all:
>>>    
>>> => ubi part nand
>>> Partition nand not found!
>>>
>>> Before (and without my patch) its this:
>>>    
>>> => ubi part nand
>>> Device spi-nand0 not found!
>>> Error initializing mtdparts!
>>> ubi0: attaching mtd2
>>> ubi0: scanning is finished
>>> ubi0: attached mtd2 (name "nand", size 128 MiB)
>>
>> I think I found what's missing in mtd_probe_devices(): we don't use the
>> default mtdparts and mtdids when those env vars are NULL (see what's
>> done in mtdparts_init() to handle this case [1]).
> 
> Can you try with the following diff applied?
> 
> --->8---
> 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
> 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;
>   
> 

I can confirm that this patch fixes the issue with the
warning upon the first "ubi part part-foo" without first
using some "mtd" command (to probe the MTD devices). So
please feel free to add my t-b tag, if you send a patch
to the list:

Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 30 Oct 2018
23:02:50 +0100:

> On Tue, 30 Oct 2018 11:59:13 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
> > On 30.10.18 11:41, Boris Brezillon wrote:  
> > > On Tue, 30 Oct 2018 11:13:37 +0100
> > > Stefan Roese <sr@denx.de> wrote:
> > >     
> > >> Hi Boris,
> > >>
> > >> On 30.10.18 11:03, Boris Brezillon wrote:    
> > >>> On Tue, 30 Oct 2018 10:51:51 +0100
> > >>> Stefan Roese <sr@denx.de> wrote:
> > >>>        
> > >>>> Calling "mtdparts" currently fails when its called before any other mtd
> > >>>> command (or ubi command) has been called. The MTD devices are not
> > >>>> probed at this point and therefore it fails e.g. with this message:
> > >>>>       
> > >>>> => mtdparts    
> > >>>> Device spi-nand0 not found!    
> > >>>
> > >>> IIRC, we decided that mtdparts should not call mtd_probe_devices() to
> > >>> encourage people to stop using it.    
> > >>
> > >> I see. But I don't quite get how this missing call (and reslting
> > >> error message) would encourage people to stop using it.    
> > > 
> > > You're right, this message does not encourage people to stop using
> > > mtdparts on existing setups (mtdparts should work just fine on any MTD
> > > devices except SPI NANDs) but it does discourage them from using it on
> > > spi-nand devices since it returns an error.    
> > 
> > IMHO, that's more confusing than discouraging.
> >     
> > >>>>
> > >>>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
> > >>>> solve this issue. This also fixes a problem when calling "ubi part"
> > >>>> as first flash storage related command.    
> > >>>
> > >>> Hm, this one is unexpected. Miquel, any idea why this happens. Do we
> > >>> need to enable a specific option if we want mtd_probe_devices() to be
> > >>> called in the ubi part path?    
> > >>
> > >> Please note that "ubi part part-foo" does still work. It only
> > >> prints this error message before attaching the MTD partition.
> > >> The error is printed because of this call-chain:
> > >>
> > >> ubi_part()    
> > >> ->  ubi_detach()
> > >>       -> mtdparts_init()    
> > >>
> > >> So again, mtdparts_init() is called without the MTD devices
> > >> being probed.    
> > > 
> > > I guess we forgot to remove this mtdparts_init() call from the detach
> > > path. I think it's no longer needed since we now call
> > > mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take
> > > care of creating MTD partitions based on the mtdparts= and mtdids=
> > > variables.    
> > 
> > A quick test reveals that this removal does not remove the
> > error message. Instead the command does not work anymore at
> > all:
> >   
> > => ubi part nand    
> > Partition nand not found!
> > 
> > Before (and without my patch) its this:
> >   
> > => ubi part nand    
> > Device spi-nand0 not found!
> > Error initializing mtdparts!
> > ubi0: attaching mtd2
> > ubi0: scanning is finished
> > ubi0: attached mtd2 (name "nand", size 128 MiB)  
> 
> I think I found what's missing in mtd_probe_devices(): we don't use the
> default mtdparts and mtdids when those env vars are NULL (see what's
> done in mtdparts_init() to handle this case [1]).
> 
> [1]https://elixir.bootlin.com/u-boot/v2018.11-rc3/source/cmd/mtdparts.c#L1763

Isn't the right solution to always define these env variables when they
are needed? Defining such default behavior with a Kconfig entry is,
from my opinion, a lot of noise for such an useless feature...


Thanks,
Miquèl

[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Boris Brezillon]

On Tue, 30 Oct 2018 23:43:40 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 30 Oct 2018
> 23:02:50 +0100:
> 
> > On Tue, 30 Oct 2018 11:59:13 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> > > On 30.10.18 11:41, Boris Brezillon wrote:    
> > > > On Tue, 30 Oct 2018 11:13:37 +0100
> > > > Stefan Roese <sr@denx.de> wrote:
> > > >       
> > > >> Hi Boris,
> > > >>
> > > >> On 30.10.18 11:03, Boris Brezillon wrote:      
> > > >>> On Tue, 30 Oct 2018 10:51:51 +0100
> > > >>> Stefan Roese <sr@denx.de> wrote:
> > > >>>          
> > > >>>> Calling "mtdparts" currently fails when its called before any other mtd
> > > >>>> command (or ubi command) has been called. The MTD devices are not
> > > >>>> probed at this point and therefore it fails e.g. with this message:
> > > >>>>         
> > > >>>> => mtdparts      
> > > >>>> Device spi-nand0 not found!      
> > > >>>
> > > >>> IIRC, we decided that mtdparts should not call mtd_probe_devices() to
> > > >>> encourage people to stop using it.      
> > > >>
> > > >> I see. But I don't quite get how this missing call (and reslting
> > > >> error message) would encourage people to stop using it.      
> > > > 
> > > > You're right, this message does not encourage people to stop using
> > > > mtdparts on existing setups (mtdparts should work just fine on any MTD
> > > > devices except SPI NANDs) but it does discourage them from using it on
> > > > spi-nand devices since it returns an error.      
> > > 
> > > IMHO, that's more confusing than discouraging.
> > >       
> > > >>>>
> > > >>>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to
> > > >>>> solve this issue. This also fixes a problem when calling "ubi part"
> > > >>>> as first flash storage related command.      
> > > >>>
> > > >>> Hm, this one is unexpected. Miquel, any idea why this happens. Do we
> > > >>> need to enable a specific option if we want mtd_probe_devices() to be
> > > >>> called in the ubi part path?      
> > > >>
> > > >> Please note that "ubi part part-foo" does still work. It only
> > > >> prints this error message before attaching the MTD partition.
> > > >> The error is printed because of this call-chain:
> > > >>
> > > >> ubi_part()      
> > > >> ->  ubi_detach()
> > > >>       -> mtdparts_init()      
> > > >>
> > > >> So again, mtdparts_init() is called without the MTD devices
> > > >> being probed.      
> > > > 
> > > > I guess we forgot to remove this mtdparts_init() call from the detach
> > > > path. I think it's no longer needed since we now call
> > > > mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take
> > > > care of creating MTD partitions based on the mtdparts= and mtdids=
> > > > variables.      
> > > 
> > > A quick test reveals that this removal does not remove the
> > > error message. Instead the command does not work anymore at
> > > all:
> > >     
> > > => ubi part nand      
> > > Partition nand not found!
> > > 
> > > Before (and without my patch) its this:
> > >     
> > > => ubi part nand      
> > > Device spi-nand0 not found!
> > > Error initializing mtdparts!
> > > ubi0: attaching mtd2
> > > ubi0: scanning is finished
> > > ubi0: attached mtd2 (name "nand", size 128 MiB)    
> > 
> > I think I found what's missing in mtd_probe_devices(): we don't use the
> > default mtdparts and mtdids when those env vars are NULL (see what's
> > done in mtdparts_init() to handle this case [1]).
> > 
> > [1]https://elixir.bootlin.com/u-boot/v2018.11-rc3/source/cmd/mtdparts.c#L1763  
> 
> Isn't the right solution to always define these env variables when they
> are needed? Defining such default behavior with a Kconfig entry is,
> from my opinion, a lot of noise for such an useless feature...

The thing is, we want to support existing setups, and apparently not
everyone define its default mtdids/mtdparts in their default env.