Re: [PATCH v6 2/5] efi: Add embedded peripheral firmware support
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.gw14...@wotan.suse.de [1] https://lkml.kernel.org/r/20180422202609.gx14...@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. 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
Re: [PATCH v6 2/5] efi: Add embedded peripheral firmware support
On Fri, Jun 01, 2018 at 02:53:27PM +0200, Hans de Goede wrote: > Just like with PCI options ROMs, which we save in the setup_efi_pci* > functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself > sometimes may contain data which is useful/necessary for peripheral drivers > to have access to. > > Specifically the EFI code may contain an embedded copy of firmware which > needs to be (re)loaded into the peripheral. Normally such firmware would be > part of linux-firmware, but in some cases this is not feasible, for 2 > reasons: > > 1) The firmware is customized for a specific use-case of the chipset / use > with a specific hardware model, so we cannot have a single firmware file > for the chipset. E.g. touchscreen controller firmwares are compiled > specifically for the hardware model they are used with, as they are > calibrated for a specific model digitizer. > > 2) Despite repeated attempts we have failed to get permission to > redistribute the firmware. This is especially a problem with customized > firmwares, these get created by the chip vendor for a specific ODM and the > copyright may partially belong with the ODM, so the chip vendor cannot > give a blanket permission to distribute these. > > This commit adds support for finding peripheral firmware embedded in the > EFI code and making this available to peripheral drivers through the > standard firmware loading mechanism. > > Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end > of start_kernel(), just before calling rest_init(), this is on purpose > because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for > early_memremap(), so the check must be done after mm_init(). This relies > on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services() > is called, which means that this will only work on x86 for now. > > Reported-by: Dave Olsthoorn > Suggested-by: Peter Jones > Acked-by: Ard Biesheuvel > Signed-off-by: Hans de Goede > --- > Changes in v6: > -Rework code to remove casts from if (prefix == mem) comparison > -Use SHA256 hashes instead of crc32 sums > -Add new READING_FIRMWARE_EFI_EMBEDDED read_file_id and use it > -Call security_kernel_read_file(NULL, READING_FIRMWARE_EFI_EMBEDDED) > to check if this is allowed before looking at EFI embedded fw > -Document why we are not using the PI Firmware Volume protocol > > Changes in v5: > -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS > > Changes in v4: > -Drop note in docs about EFI_FIRMWARE_VOLUME_PROTOCOL, it is not part of > UEFI proper, so the EFI maintainers don't want us referring people to it > -Use new EFI_BOOT_SERVICES flag > -Put the new fw_get_efi_embedded_fw() function in its own fallback_efi.c > file which only gets built when EFI_EMBEDDED_FIRMWARE is selected > -Define an empty stub for fw_get_efi_embedded_fw() in fallback.h hwen > EFI_EMBEDDED_FIRMWARE is not selected, to avoid the need for #ifdefs > in firmware_loader/main.c > -Properly call security_kernel_post_read_file() on the firmware returned > by efi_get_embedded_fw() to make sure that we are allowed to use it > > Changes in v3: > -Fix the docs using "efi-embedded-fw" as property name instead of > "efi-embedded-firmware" > > Changes in v2: > -Rebased on driver-core/driver-core-next > -Add documentation describing the EFI embedded firmware mechanism to: > Documentation/driver-api/firmware/request_firmware.rst > -Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded > fw support if this is set. This is an invisible option which should be > selected by drivers which need this > -Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices > from the efi-embedded-fw code, instead drivers using this are expected to > export a dmi_system_id array, with each entries' driver_data pointing to a > efi_embedded_fw_desc struct and register this with the efi-embedded-fw code > -Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware, > this avoids us messing with the EFI memmap and avoids the need to make > changes to efi_mem_desc_lookup() > -Make the firmware-loader code only fallback to efi_get_embedded_fw() if the > passed in device has the "efi-embedded-firmware" device-property bool set > -Skip usermodehelper fallback when "efi-embedded-firmware" device-property > is set > --- > .../driver-api/firmware/request_firmware.rst | 76 + > drivers/base/firmware_loader/Makefile | 1 + > drivers/base/firmware_loader/fallback.h | 12 ++ > drivers/base/firmware_loader/fallback_efi.c | 56 +++ > drivers/base/firmware_loader/main.c | 2 + > drivers/firmware/efi/Kconfig | 3 + > drivers/firmware/efi/Makefile | 1 + > drivers/firmware/efi/embedded-firmware.c | 148 ++ > include/linux/efi.h | 6 + > include/linux/efi_embedded_fw.h | 25 +++ > include/li
