On Fri, Aug 14, 2020 at 03:02:25PM +0100, David Woodhouse wrote:
On Wed, 2020-08-05 at 10:52 -0400, Tom Rini wrote:
FAT and ext4 don't need to grow their own config symbols because they
already *have* them. The only reason they're involved here is because of the case where they explicitly want to *abdicate* responsibility and let platform code determine the device to use, not hard-coded config. Like the MT7623 platforms where the preloader tells U-Boot if it was actually loaded from the internal eMMC or external SD card.
If someone is setting up the ENV_FAT_DEVICE_AND_PART to start with a
colon and thus abdicate the device part, but then that decision is only coming from a hard-coded SYS_MMC_ENV_DEV option, then they're probably somewhat confused.
Right, but the default mmc_get_env_dev() you added uses CONFIG_SYS_MMC_ENV_DEV.
Perhaps I could revamp the FAT/ext4 patches so that they don't use
the default weak function, and if you use the "get it at runtime" behaviour on any build which doesn't actually have a real platform mmc_get_env_dev() function, the build fails. That would be sane, but might involve a separate explicit config option instead of just "the DEVICE_AND_PART option starts with a colon".
That sounds like a good idea, thanks!
Hm. I took a quick look at this. Forgetting my ext4 patch for the moment and looking just at what's in HEAD...
The initial naïve approach is to rip out the weak implementation of mmc_get_env_dev() from env/fat.c and trust the compiler spot that this is tautologically false because the strings are constants:
part_str = CONFIG_ENV_FAT_DEVICE_AND_PART; if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc") && part_str[0] == ':') {Sadly, GCC doesn't seem to be clever enough for that.
So the actual code for env_fat_device_and_part() would need some extra compile-time conditional.
It would be possible to make platforms with their own 'real' mmc_get_env_dev() function select a config symbol called something like (imaginatively) PLATFORM_HAS_MMC_GET_ENV_DEV, and then the code in env_fat_device_and_part() could be conditional on that as well as CONFIG_MMC as it is at the moment.
On the whole though, it seems like overkill just to "forbid" a mildly pointless combination of config options. And in fact, perhaps it isn't even that pointless.
I wrote the "find environment FAT partition using mmc_get_env_dev()" support for OpenWrt, specifically on the mt7623 platforms where it is indeed a runtime choice. But it's actually quite sensible for OpenWrt just to have a consistent partition layout across multiple devices, set CONFIG_ENV_FAT_DEVICE_AND_PART to ":2" on *all* of those platforms, and then *only* CONFIG_SYS_MMC_ENV_DEV needs to vary in a platform-specific manner.
I think the best option is probably to consolidate and move the weak mmc_get_env_dev() into drivers/mmc/mmc.c and let env/{fat,ext4,mmc}.c use it from there.
Yes, some people might be able to set CONFIG_ENV_FAT_DEVICE_AND_PART to something starting with a colon, deferring the device number to a hard- coded CONFIG_SYS_MMC_ENV_DEV — which at *runtime* is kind of redundant. But that isn't actually all that insane anyway. We don't *need* to forbid it.
Sounds like a good plan to me!