Re: [PATCH v6 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

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@xxxxxxxxxxxxx              
[1] https://lkml.kernel.org/r/20180422202609.GX14440@xxxxxxxxxxxxx 

> 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.

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 :)

  Luis