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

[Boris Brezillon <boris.brezillon@bootlin.com>]

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.