[U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV

[Andre Przywara]

On Sat, 8 Jun 2019 09:13:52 -0400
Tom Rini <trini at konsulko.com> wrote:

Hi Tom,

thanks for having a look!

> On Sat, Jun 08, 2019 at 02:26:54AM +0100, Andre Przywara wrote:
> > So far we are required to always define the CONFIG_SYS_MMC_ENV_DEV
> > variable, even if a platform specific function overrides the weak
> > function that is using it.
> > 
> > Check for the existence of this Kconfig variable, eliminating the need
> > to define a dummy value.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > ---
> >  env/mmc.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/env/mmc.c b/env/mmc.c
> > index c3cf35d01b..122fec3af8 100644
> > --- a/env/mmc.c
> > +++ b/env/mmc.c
> > @@ -124,7 +124,11 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
> >  
> >  __weak int mmc_get_env_dev(void)
> >  {
> > +#ifdef CONFIG_SYS_MMC_ENV_DEV
> >  	return CONFIG_SYS_MMC_ENV_DEV;
> > +#else
> > +	return 0;
> > +#endif
> >  }
> >  
> >  #ifdef CONFIG_SYS_MMC_ENV_PART  
> 
> Since 0 is a valid device, I'm concerned this might lead to unintended
> behavior.  Can we return some error code here and catch it later?

I see your point. I think originally my idea was that 0 would hopefully be a valid device in any case, so it would just work in those cases. But I see the trouble that this papers over a bug (namely CONFIG_SYS_MMC_ENV_DEV not being defined).
Looking at all those users I find it rather dangerous (and tedious) to check for this in all callers.

What about printing an error message, here in that function? Ideally this would be spotted immediately upon initial testing, so would never be exposed to a real user.
Something like "Please either define CONFIG_SYS_MMC_ENV_DEV or provide a mmc_get_en_dev() implementation."?

Cheers,
Andre.