From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Mon, 10 Jun 2019 17:53:58 -0400 Subject: [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV In-Reply-To: <20190610103536.7e25c0f6@donnerap.cambridge.arm.com> References: <20190608012658.5369-1-andre.przywara@arm.com> <20190608012658.5369-2-andre.przywara@arm.com> <20190608131352.GB7115@bill-the-cat> <20190610103536.7e25c0f6@donnerap.cambridge.arm.com> Message-ID: <20190610215358.GQ7115@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Mon, Jun 10, 2019 at 10:35:36AM +0100, Andre Przywara wrote: > On Sat, 8 Jun 2019 09:13:52 -0400 > Tom Rini 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 > > > --- > > > 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: