Messages in this thread | | | Subject | Re: [PATCH v6 2/5] efi: Add embedded peripheral firmware support | From | Hans de Goede <> | Date | Thu, 7 Jun 2018 15:46:11 +0200 |
| |
Hi,
On 06-06-18 23:42, Luis R. Rodriguez wrote: > On Wed, Jun 06, 2018 at 08:39:15PM +0200, Hans de Goede wrote: >> On 05-06-18 23:07, Luis R. Rodriguez wrote: >>>> +To make request_firmware() fallback to trying EFI embedded firmwares after this, >>>> +the driver must set a boolean "efi-embedded-firmware" device-property on the >>>> +device before passing it to request_firmware(). >>> >>> No, as I have requested before I don't want this, it is silly to have such >>> functionality always be considered as a fallback if we only have 2 drivers >>> which need this. > Please add a call which only if used would then *evaluate* >>> such fallback prospect. >> >> Ok, so I've a few questions about this: >> >> 1) You want me to add a: >> >> int firmware_request_with_system_firmware_fallback(struct device *device, const char *name); >> >> function? > > The idea is correct, the name however is obviously terrible. > > This is functionality that is very specialized and only *two* device drivers > need it that we are aware of which would be upstream. Experience has shown > fallback mechanisms *can* be a pain, and if we add this we will be supporting > this for *life*, as such I'd very much prefer to: > > a) *Clearly* reduce the scope of functionality clearly *beyond* what you > have done. > > b) Have access to one simple call which folks can use to *clearly* and > quickly grep for those oddball drivers using this new interface. > > We can do this by using a separate function for it. > > Before you claim something seems unreasonable from the above logic, please read > the latest state of affairs with respect to data driven Vs functional API > evolution discussion over the firmware API [0] as well as my latests > recommendation for what to do for the async firmware API [1]. > > The skinny of it is that long ago I actually proposed having only *two* > firmware API calls, an async and a sync call and having all functionality > fleshed out through a structure of parameters. The issue with that strategy was > it was *too* data driven, and as per Greg's request we'll instead be exposing > new symbols per functionality for the firmware API with his justification > that this is just what is traditionally done on Linux. Hence we have > firmware_request_nowarn() now for just a slight variation for a sync call. > > Despite Greg's recommendation -- for the respective async functionality I > suggested this is not going to scale well -- it is also just dumb to follow the > same approach there for a few reasons. > > 1) We have only *one* async call and had decided to *not* provide a structure > for that call since its inception > > 2) Over time have evolved this single async call each time we have a new feature, > causing a slew of collateral evolutions. > > So, while we like it or not, it turns out the async call to the firmware API > is already completely data driven. Extending it with just another argument > would just be silly now. > > So refer to my recommendations to Andres for how to evolve the async API if > you need it, however from a quick review you don't need async calls, so you > won't have to address any of that. > > [0] https://lkml.kernel.org/r/20180421173650.GW14440@wotan.suse.de > [1] https://lkml.kernel.org/r/20180422202609.GX14440@wotan.suse.de > >> Note I've deliberately named it with_system_firmware_fallback >> and not with_efi_fallback to have the name be platform agnostic in >> case we want something similar on other platforms in the future. > > firmware_request_platform() ? > >> And then have this pass a new FW_OPT_SYS_FW_FALLBACK flag to >> _request_firmware(), right ? > > Yeap. > >> 2) Should this flag then be checked inside _request_firmware() before it >> calls fw_get_efi_embedded_fw() (which may be an empty stub), > > You are the architect behind this call, so its up to you. > > To answer this you have to review the other flags and see if other users of the > other flags may want your functionality. For instance the Android folks for > instance rely on the FW_OPT_NOFALLBACK - the sysfs fallback mechanism to > account for odd partition layouts. Could they ever want to use your fallback > mechanism? Granted your mechanism is for x86, but they could eventually add > support for it on ARM. > > Checking if the firmware is on the EFI platform firmware list is much faster > than the fallback mechanism, that would be one gain for them, as such it may > make sense to check for firmware_request_platform() before using the sysfs > fallback mechanism. However if Android folks want to always override the > platform firmware with the sysfs fallback interface we'd need another flag > added and call to then change the order later if we checked for for the > platform firmware first.
I believe we agreed a while back that the platform fallback would replace the sysfs one when requested. I believe that still makes sense. If a driver wants both it can simply call request_firmware_foo itself twice and determine the order itself.
> If you however are 100% sure they won't need it, than checking > firmware_request_platform() first would make sense now if you are > certain these same devices won't need the sysfs fallback interface > and don't want to use it to override the platform firmware. > >> or >> inside fw_get_efi_embedded_fw()? > > You'll definitely want to check it there for sure to no-op if you > don't have it set and error out with the same error passed before > so it is not lost, as is done now for the sysfs fallback mechanism. > >> 3) Also should I then still check device_property_read_bool(dev, >> "efi-embedded-firmware") inside fw_get_efi_embedded_fw(), or do you >> want to simply always try the fallback if the driver indicates this ? >> >> The reason I'm asking this is because the presence or not of the >> firmware inside the system-firmware is a per board thing, not >> a per driver thing. But since the firmware is unique per board >> anyways it would be fine to skip the "efi-embedded-firmware" >> device-prop check. If we're going to have a separate >> firmware_request_with_system_firmware_fallback() function for >> this then I'm leaning towards dropping the whole check myself. >> >> So are you ok with dropping the device-prop check then ? > > Yes I was actually not sure why you added it, but given you say that its per > board, it makes sense. Drivers still would still need to enable the new > fallback via the new call, say firmware_request_platform(). I suppose it would > depend on how many boards out there *don't* have the firmware. Since we are > aware of only two drivers enabling this I'm going to suspect we have only a few > boards out there with this firmware... > > The value of the device-prop check then would be to *avoid* running this code > even further for boards where it does not make sense. This is indeed valuable, > so unless others have issue with it I'd say leave it and document this exact > thing. > > The documentation should then reflect firmware_request_platform() enables the > fallback for the driver, however boards must use the efi-embedded-firmware > property to further activate the fallback for a specific board. > >> 4) Since I'm naming the new function >> int firmware_request_with_system_firmware_fallback() >> I'm thinking that fw_get_efi_embedded_fw() should be renamed >> to firmware_fallback_system_firmware() >> and the EFI based code >> will just be the first (and for now only) provider of this >> fallback with an empty stub (inline in the .h file) for when >> there is no provider defined. Do you agree with renaming >> fw_get_efi_embedded_fw() to >> firmware_request_with_system_firmware_fallback() ? > > That's obviously too long, so perhaps firmware_fallback_platform()? > > Note that Andres recently renamed fw_sysfs_fallback() to > firmware_fallback_sysfs() so yes, I was expectign this. > > Also notice all that new shiny kdoc on firmware_fallback_sysfs()? Please be > sure to add your own and refer to it on the kernel documentation as well -- it > seems we forgot to reference firmware_fallback_sysfs() on Documentation/ I'll > go and correct that. > > While at it, I just noticed your most of your changes should actually > go into Documentation/driver-api/firmware/fallback-mechanisms.rst and only > this new call firmware_request_platform() on > Documentation/driver-api/firmware/request_firmware.rst > >> (note this is the wrapper which calls efi_get_embedded_fw() >> which itself will not be renamed of course). > > Yeap :)
Thank you for your answers. I will prepare a v7 of this series with the changes as discussed. I've several higher priority things to fix first though, so it may be a while before I post v7.
Regards,
Hans
| |