[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 at bootlin.com> wrote:
> 
>> On Tue, 30 Oct 2018 11:59:13 +0100
>> Stefan Roese <sr at denx.de> wrote:
>>
>>> On 30.10.18 11:41, Boris Brezillon wrote:
>>>> On Tue, 30 Oct 2018 11:13:37 +0100
>>>> Stefan Roese <sr at 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 at 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 at denx.de>

Thanks,
Stefan