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

[Tom Rini]

On Mon, Jun 10, 2019 at 10:35:36AM +0100, Andre Przywara wrote:
> 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."?

That might be OK.  Just check the resulting binaries that the string
does get discarded from the build when we have a non-weak function in
that case.  Thanks!

-- 
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/20190610/045325fc/attachment.sig>