[U-Boot] [U-Boot, v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it

[Tom Rini]

On Sun, Feb 25, 2018 at 03:50:41PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20180225134810.GU4311 at bill-the-cat> you wrote:
> > 
> > > We should keep the code clean and orthogonal.  Driver init code has
> > > no place in file system code.
> > >
> > > If needed, the drivers have to make sure to auto--initialize on
> > > first access.
> > >
> > > I hold my NAK on this patch.  It is the wrong thing to do.
> > 
> > I think you have this a little bit wrong.  But, it's also where we are
> > with the DM conversion.  This _is_ the first place we're trying to
> > access the MMC.  And it's not in the filesystem code, it's in the
> > environment code.
> 
> Maybe I was not really clear.  You are right as this is not generic
> file system code.  Instead, it is the EXT4 support code for the
> environment handling.
> 
> The patch adds mmc_initialize() to env_ext4_load(), so whenever we
> try to load the environment from an EXT4 file system, it will
> initialize the MMC subsystem.
> 
> However - what if we want to load the environment from an EXT4 file
> system on any other storage device - say, USB, or SATA, or flash?

Good question, and part of why after re-reading the code, I want to see
just what the combination of hardware Faiz is trying is.  MMC should
already have been initialized.  Unless I'm missing the specific init
path he has.  This does also highlight that env on fat/ext4 as only
likely been tested on MMC devices, even prior to the recent changes.

> In all such cases the initialization of MMC would be plain wrong.

Correct, and we don't try and initialize MMC if we aren't told that the
environment resides on mmc.

> And what if we want to load the environment from any other type of
> file system - say, cramfs, zfs, etc. - on SDCard or eMMC?  These do
> not initialize MMC, so it would fail?
> 
> Yes, we have the same crappy code in env/fat.c, but this is the
> wrong thing to do, and must be cleaned up there as well.

Yes, one of the things I've suggested before is that ENV_IS_IN_FAT and
ENV_IS_IN_EXT should be able to be rewritten to use the generic FS
operation API we have so that zfs, etc, could be automatically
supported.

> This is what I meant when I wrote that the file system (interface)
> code and the storage device support code are independent of each
> other, and code should be kept orthogonal - storage support like MMC
> etc. has no place in code dealing with a specific file system type.

The problem we have today, and I hope we can more cleverly resolve once
more/everything has been migrated to DM is that don't yet have a good
way to say "retry $X later" or "retry $X after $Y".  Because setting
aside the specific issue Faiz ran into, FS+SATA has never worked because
initr_scsi is well after initr_env, and that's why the sata env code
(which uses blocks and not fs) has always had to sata_initialize().

> I still think my NAK is reasonable.

Conceptually, I disagree because we don't yet have a more nicer solution
available yet.  With this specific patch, it might be a problem with the
board code, as mmc_initialize() should already have been called.  So
maybe this needs to come out.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180225/b7506fad/attachment.sig>