[PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Hans de Goede]

Hi All,

Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.

This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
changes for v4.18" series from mcgrof.

It now also depends on the series from Andy Lutomirski which allow using the
sha256 code in a standalone manner. Andy what is the status of those?

Changes since v5:
-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

For reference I've included the coverletter from v4 (which includes
previous covverletters) below.

Regards,

Hans


Previous coverletter:

Here is v5 of my patch-set to add support for EFI embedded fw to the kernel.

Changes since v4:
-Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

So I think this patch-set is getting close to ready for merging, which
brings us to the question of how to merge this, I think that patches 1
and 2 should probably both be merged through the same tree. Then an
unmutable branch should be created on that tree, merged into the
platform/x86 tree and then the last 3 patches can be merged through
that tree.

Ard has already indicated he is fine with the EFI bits going upstream
through another tree, so perhaps patches 1-2 can be merged through the
firmware-loader-tree and then do an unmutable branch on the
firmware-loader-tree for the platform/x86 tree to merge?

I don't think taking all 5 through 1 tree is a good idea because of
the file rename under platform/x86.


For the record here are the cover letters of the previous versions:

Changes since v3:
-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

The 3 most prominent changes in v2 are:

1) Add documentation describing the EFI embedded firmware mechanism to:
   Documentation/driver-api/firmware/request_firmware.rst

2) Instead of having a single dmi_system_id array with its driver_data
   members pointing to efi_embedded_fw_desc structs, have the drivers which
   need EFI embedded-fw support export a dmi_system_id array and register
   that with the EFI embedded-fw code

   This series also includes the first driver to use this, in the form of
   the touchscreen_dmi code (formerly silead_dmi) from drivers/platfrom/x86

3) As discussed during the review of v1 we want to make the firmware_loader
   code fallback to EFI embedded-fw optional.  Rather the adding yet another
   firmware_request_foo variant for this, with the risk of later also needing
   firmware_request_foo_nowait, etc. variants I've decided to make the code
   check if the device has a "efi-embedded-firmware" device-property bool set.

   This also seemed better because the same driver may want to use the
   fallback on some systems, but not on others since e.g. not all (x86)
   systems with a silead touchscreen have their touchscreen firmware embedded
   in their EFI.

   Note that (as discussed) when the EFI fallback path is requested, the
   usermodehelper fallback path is skipped.

Here is the full changelog of patch 2/5 which is where most of the changes are:

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

Patches 3-5 are new and implement using the EFI embedded-fw mechanism for
Silead gslXXXX and Chipone icn8505 touchscreens on x86 devices.

[PATCH v6 1/5] efi: Export boot-services code and data as debugfs-blobs

[Hans de Goede]

Sometimes it is useful to be able to dump the efi boot-services code and
data. This commit adds these as debugfs-blobs to /sys/kernel/debug/efi,
but only if efi=debug is passed on the kernel-commandline as this requires
not freeing those memory-regions, which costs 20+ MB of RAM.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

Changes in v4:
-Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services
 memory segments are available (and thus if it makes sense to register the
 debugfs bits for them)

Changes in v2:
-Do not call pr_err on debugfs call failures
---
 arch/x86/platform/efi/efi.c    |  1 +
 arch/x86/platform/efi/quirks.c |  4 +++
 drivers/firmware/efi/efi.c     | 53 ++++++++++++++++++++++++++++++++++
 include/linux/efi.h            |  1 +
 4 files changed, 59 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 9061babfbc83..82bbbbf0836d 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -208,6 +208,7 @@ int __init efi_memblock_x86_reserve_range(void)
 	     efi.memmap.desc_version);
 
 	memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size);
+	set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags);
 
 	return 0;
 }
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 36c1f8b9f7e0..16bdb9e3b343 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -376,6 +376,10 @@ void __init efi_free_boot_services(void)
 	int num_entries = 0;
 	void *new, *new_md;
 
+	/* Keep all regions for /sys/kernel/debug/efi */
+	if (efi_enabled(EFI_DBG))
+		return;
+
 	for_each_efi_memory_desc(md) {
 		unsigned long long start = md->phys_addr;
 		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 232f4915223b..1590e4d6a857 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -18,6 +18,7 @@
 #include <linux/kobject.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/efi.h>
 #include <linux/of.h>
@@ -325,6 +326,55 @@ static __init int efivar_ssdt_load(void)
 static inline int efivar_ssdt_load(void) { return 0; }
 #endif
 
+#ifdef CONFIG_DEBUG_FS
+
+#define EFI_DEBUGFS_MAX_BLOBS 32
+
+static struct debugfs_blob_wrapper debugfs_blob[EFI_DEBUGFS_MAX_BLOBS];
+
+static void __init efi_debugfs_init(void)
+{
+	struct dentry *efi_debugfs;
+	efi_memory_desc_t *md;
+	char name[32];
+	int type_count[EFI_BOOT_SERVICES_DATA + 1] = {};
+	int i = 0;
+
+	efi_debugfs = debugfs_create_dir("efi", NULL);
+	if (IS_ERR_OR_NULL(efi_debugfs))
+		return;
+
+	for_each_efi_memory_desc(md) {
+		switch (md->type) {
+		case EFI_BOOT_SERVICES_CODE:
+			snprintf(name, sizeof(name), "boot_services_code%d",
+				 type_count[md->type]++);
+			break;
+		case EFI_BOOT_SERVICES_DATA:
+			snprintf(name, sizeof(name), "boot_services_data%d",
+				 type_count[md->type]++);
+			break;
+		default:
+			continue;
+		}
+
+		debugfs_blob[i].size = md->num_pages << EFI_PAGE_SHIFT;
+		debugfs_blob[i].data = memremap(md->phys_addr,
+						debugfs_blob[i].size,
+						MEMREMAP_WB);
+		if (!debugfs_blob[i].data)
+			continue;
+
+		debugfs_create_blob(name, 0400, efi_debugfs, &debugfs_blob[i]);
+		i++;
+		if (i == EFI_DEBUGFS_MAX_BLOBS)
+			break;
+	}
+}
+#else
+static inline void efi_debugfs_init(void) {}
+#endif
+
 /*
  * We register the efi subsystem with the firmware subsystem and the
  * efivars subsystem with the efi subsystem, if the system was booted with
@@ -369,6 +419,9 @@ static int __init efisubsys_init(void)
 		goto err_remove_group;
 	}
 
+	if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
+		efi_debugfs_init();
+
 	return 0;
 
 err_remove_group:
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 56add823f190..d72358e892e7 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1144,6 +1144,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #define EFI_DBG			8	/* Print additional debug info at runtime */
 #define EFI_NX_PE_DATA		9	/* Can runtime data regions be mapped non-executable? */
 #define EFI_MEM_ATTR		10	/* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
+#define EFI_PRESERVE_BS_REGIONS	11	/* Are EFI boot-services memory segments available? */
 
 #ifdef CONFIG_EFI
 /*
-- 
2.17.0

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

[Hans de Goede]

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 <dave@bewaar.me>
Suggested-by: Peter Jones <pjones@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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/linux/fs.h                            |   1 +
 init/main.c                                   |   3 +
 12 files changed, 334 insertions(+)
 create mode 100644 drivers/base/firmware_loader/fallback_efi.c
 create mode 100644 drivers/firmware/efi/embedded-firmware.c
 create mode 100644 include/linux/efi_embedded_fw.h

diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index f62bdcbfed5b..66ab91f3357d 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -73,3 +73,79 @@ If something went wrong request_firmware() returns non-zero and fw_entry
 is set to NULL. Once your driver is done with processing the firmware it
 can call call release_firmware(fw_entry) to release the firmware image
 and any related resource.
+
+EFI embedded firmware support
+=============================
+
+On some devices the system's EFI code / ROM may contain an embedded copy
+of firmware for some of the system's integrated peripheral devices and
+the peripheral's Linux device-driver needs to access this firmware.
+
+A device driver which needs this can describe the firmware it needs
+using an efi_embedded_fw_desc struct:
+
+.. kernel-doc:: include/linux/efi_embedded_fw.h
+   :functions: efi_embedded_fw_desc
+
+The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory
+segments for an eight byte sequence matching prefix, if the prefix is found it
+then does a crc32 over length bytes and if that matches makes a copy of length
+bytes and adds that to its list with found firmwares.
+
+To avoid doing this somewhat expensive scan on all systems, dmi matching is
+used. Drivers are expected to export a dmi_system_id array, with each entries'
+driver_data pointing to an efi_embedded_fw_desc.
+
+To register this array with the efi-embedded-fw code, a driver needs to:
+
+1. Always be builtin to the kernel or store the dmi_system_id array in a
+   separate object file which always gets builtin.
+
+2. Add an extern declaration for the dmi_system_id array to
+   include/linux/efi_embedded_fw.h.
+
+3. Add the dmi_system_id array to the embedded_fw_table in
+   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
+   the driver is being builtin.
+
+4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
+
+The request_firmware() function will always first try to load firmware with
+the specified name directly from the disk, so the EFI embedded-fw can always
+be overridden by placing a file under /lib/firmare.
+
+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(). Note that this disables the
+usual usermodehelper fallback, so you may want to only set this on systems
+which match your dmi_system_id array.
+
+Once the device-property is set, the driver can use the regular
+request_firmware() function to get the firmware, using the name filled in
+in the efi_embedded_fw_desc.
+
+Note that:
+
+1. The code scanning for EFI embbedded-firmware runs near the end
+   of start_kernel(), just before calling rest_init(). For normal drivers and
+   subsystems using subsys_initcall() to register themselves this does not
+   matter. This means that code running earlier cannot use EFI
+   embbedded-firmware.
+
+2. ATM the EFI embedded-fw code assumes that firmwares always start at an offset
+   which is a multiple of 8 bytes, if this is not true for your case send in
+   a patch to fix this.
+
+3. ATM the EFI embedded-fw code only works on x86 because other archs free
+   EFI_BOOT_SERVICES_CODE before the EFI embedded-fw code gets a chance to
+   scan it.
+
+4. The current brute-force scanning of EFI_BOOT_SERVICES_CODE is an ad-hoc
+   brute-force solution. There has been discussion to use the PI spec's
+   Firmware Volume protocol. This has been rejected because the FV Protocol
+   relies on *internal* interfaces of PI spec, and:
+   1. The The PI spec does not define firmware at all
+   2. The internal interfaces of PI Spec does not guarantee any backward
+      compatibility. Any implementation details in FV may be subject to change,
+      and may vary system to system. Supporting the FV Protocol would be
+      difficult as it is purposely ambiguous.
diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
index a97eeb0be1d8..365a040995d3 100644
--- a/drivers/base/firmware_loader/Makefile
+++ b/drivers/base/firmware_loader/Makefile
@@ -5,3 +5,4 @@ obj-y			:= fallback_table.o
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 firmware_class-objs := main.o
 firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
+firmware_class-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += fallback_efi.o
diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
index 21063503e4ea..5103283873d5 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -66,4 +66,16 @@ static inline void unregister_sysfs_loader(void)
 }
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
+			   enum fw_opt *opt_flags, int ret);
+#else
+static inline int fw_get_efi_embedded_fw(struct device *dev,
+					 struct fw_priv *fw_priv,
+					 enum fw_opt *opt_flags, int ret)
+{
+	return ret;
+}
+#endif /* CONFIG_EFI_EMBEDDED_FIRMWARE */
+
 #endif /* __FIRMWARE_FALLBACK_H */
diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
new file mode 100644
index 000000000000..641598b8f746
--- /dev/null
+++ b/drivers/base/firmware_loader/fallback_efi.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/efi_embedded_fw.h>
+#include <linux/property.h>
+#include <linux/security.h>
+#include <linux/vmalloc.h>
+
+#include "fallback.h"
+#include "firmware.h"
+
+int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
+			   enum fw_opt *opt_flags, int ret)
+{
+	size_t size, max = INT_MAX;
+	bool free_on_err = true;
+	int rc;
+
+	if (!dev)
+		return ret;
+
+	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
+		return ret;
+
+	*opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
+
+	rc = security_kernel_read_file(NULL, READING_FIRMWARE_EFI_EMBEDDED);
+	if (rc)
+		return rc;
+
+	/* Already populated data member means we're loading into a buffer */
+	if (fw_priv->data) {
+		max = fw_priv->allocated_size;
+		free_on_err = false;
+	}
+
+	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
+	if (rc) {
+		dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
+		return ret;
+	}
+
+	rc = security_kernel_post_read_file(NULL, fw_priv->data, size,
+					    READING_FIRMWARE_EFI_EMBEDDED);
+	if (rc) {
+		if (free_on_err) {
+			vfree(fw_priv->data);
+			fw_priv->data = NULL;
+		}
+		return rc;
+	}
+
+	dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
+	fw_priv->size = size;
+	fw_state_done(fw_priv);
+	return 0;
+}
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 0943e7065e0e..9dab6efa057c 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -576,6 +576,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 
 	ret = fw_get_filesystem_firmware(device, fw->priv);
+	if (ret)
+		ret = fw_get_efi_embedded_fw(device, fw->priv, &opt_flags, ret);
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 3098410abad8..faefeff7fb34 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -164,6 +164,9 @@ config RESET_ATTACK_MITIGATION
 	  have been evicted, since otherwise it will trigger even on clean
 	  reboots.
 
+config EFI_EMBEDDED_FIRMWARE
+	bool
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index cb805374f4bc..dde12cea8aac 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
 obj-$(CONFIG_EFI_TEST)			+= test/
 obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
 obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
+obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE)	+= embedded-firmware.o
 
 arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
 obj-$(CONFIG_ARM)			+= $(arm-obj-y)
diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
new file mode 100644
index 000000000000..244465a162c5
--- /dev/null
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for extracting embedded firmware for peripherals from EFI code,
+ *
+ * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
+ */
+
+#include <crypto/sha.h>
+#include <linux/dmi.h>
+#include <linux/efi.h>
+#include <linux/efi_embedded_fw.h>
+#include <linux/io.h>
+#include <linux/types.h>
+#include <linux/vmalloc.h>
+
+struct embedded_fw {
+	struct list_head list;
+	const char *name;
+	void *data;
+	size_t length;
+};
+
+static LIST_HEAD(found_fw_list);
+
+static const struct dmi_system_id * const embedded_fw_table[] = {
+	NULL
+};
+
+/*
+ * Note the efi_check_for_embedded_firmwares() code currently makes the
+ * following 2 assumptions. This may needs to be revisited if embedded firmware
+ * is found where this is not true:
+ * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
+ * 2) The firmware always starts at an offset which is a multiple of 8 bytes
+ */
+static int __init efi_check_md_for_embedded_firmware(
+	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
+{
+	const u64 prefix = *((u64 *)desc->prefix);
+	struct sha256_state sctx;
+	struct embedded_fw *fw;
+	u8 sha256[32];
+	u64 i, size;
+	void *map;
+
+	size = md->num_pages << EFI_PAGE_SHIFT;
+	map = memremap(md->phys_addr, size, MEMREMAP_WB);
+	if (!map) {
+		pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
+		return -ENOMEM;
+	}
+
+	size -= desc->length;
+	for (i = 0; i < size; i += 8) {
+		u64 *mem = map + i;
+
+		if (*mem != prefix)
+			continue;
+
+		sha256_init_direct(&sctx);
+		sha256_update_direct(&sctx, map + i, desc->length);
+		sha256_final_direct(&sctx, sha256);
+		if (memcmp(sha256, desc->sha256, 32) == 0)
+			break;
+	}
+	if (i >= size) {
+		memunmap(map);
+		return -ENOENT;
+	}
+
+	pr_info("Found EFI embedded fw '%s'\n", desc->name);
+
+	fw = kmalloc(sizeof(*fw), GFP_KERNEL);
+	if (!fw) {
+		memunmap(map);
+		return -ENOMEM;
+	}
+
+	fw->data = kmemdup(map + i, desc->length, GFP_KERNEL);
+	memunmap(map);
+	if (!fw->data) {
+		kfree(fw);
+		return -ENOMEM;
+	}
+
+	fw->name = desc->name;
+	fw->length = desc->length;
+	list_add(&fw->list, &found_fw_list);
+
+	return 0;
+}
+
+void __init efi_check_for_embedded_firmwares(void)
+{
+	const struct efi_embedded_fw_desc *fw_desc;
+	const struct dmi_system_id *dmi_id;
+	efi_memory_desc_t *md;
+	int i, r;
+
+	for (i = 0; embedded_fw_table[i]; i++) {
+		dmi_id = dmi_first_match(embedded_fw_table[i]);
+		if (!dmi_id)
+			continue;
+
+		fw_desc = dmi_id->driver_data;
+		for_each_efi_memory_desc(md) {
+			if (md->type != EFI_BOOT_SERVICES_CODE)
+				continue;
+
+			r = efi_check_md_for_embedded_firmware(md, fw_desc);
+			if (r == 0)
+				break;
+		}
+	}
+}
+
+int efi_get_embedded_fw(const char *name, void **data, size_t *size,
+			size_t msize)
+{
+	struct embedded_fw *iter, *fw = NULL;
+	void *buf = *data;
+
+	list_for_each_entry(iter, &found_fw_list, list) {
+		if (strcmp(name, iter->name) == 0) {
+			fw = iter;
+			break;
+		}
+	}
+
+	if (!fw)
+		return -ENOENT;
+
+	if (msize && msize < fw->length)
+		return -EFBIG;
+
+	if (!buf) {
+		buf = vmalloc(fw->length);
+		if (!buf)
+			return -ENOMEM;
+	}
+
+	memcpy(buf, fw->data, fw->length);
+	*size = fw->length;
+	*data = buf;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index d72358e892e7..657c98598bc2 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1575,6 +1575,12 @@ static inline void
 efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
 #endif
 
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+void efi_check_for_embedded_firmwares(void);
+#else
+static inline void efi_check_for_embedded_firmwares(void) { }
+#endif
+
 void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
 
 /*
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
new file mode 100644
index 000000000000..6c8b04246fc3
--- /dev/null
+++ b/include/linux/efi_embedded_fw.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_EFI_EMBEDDED_FW_H
+#define _LINUX_EFI_EMBEDDED_FW_H
+
+#include <linux/mod_devicetable.h>
+
+/**
+ * struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
+ *                               code to search for embedded firmwares.
+ *
+ * @name:   Name to register the firmware with if found
+ * @prefix: First 8 bytes of the firmware
+ * @length: Length of the firmware in bytes including prefix
+ * @sha256: SHA256 of the firmware
+ */
+struct efi_embedded_fw_desc {
+	const char *name;
+	u8 prefix[8];
+	u32 length;
+	u8 sha256[32];
+};
+
+int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
+
+#endif
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..ebae9656d3bf 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2810,6 +2810,7 @@ extern int do_pipe_flags(int *, int);
 	id(UNKNOWN, unknown)		\
 	id(FIRMWARE, firmware)		\
 	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
+	id(FIRMWARE_EFI_EMBEDDED, firmware)	\
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
diff --git a/init/main.c b/init/main.c
index 3b4ada11ed52..98183aca5017 100644
--- a/init/main.c
+++ b/init/main.c
@@ -730,6 +730,9 @@ asmlinkage __visible void __init start_kernel(void)
 	arch_post_acpi_subsys_init();
 	sfi_init_late();
 
+	if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
+		efi_check_for_embedded_firmwares();
+
 	if (efi_enabled(EFI_RUNTIME_SERVICES)) {
 		efi_free_boot_services();
 	}
-- 
2.17.0

[PATCH v6 3/5] platform/x86: Rename silead_dmi to touchscreen_dmi

[Hans de Goede]

Not only silead touchscreens need some extra info not available in the
ACPI tables to work properly. X86 devices with a Chipone ICN8505 chip also
need some DMI based extra configuration.

There is no reason to have separate dmi config code per touchscreen
controller vendor. This commit renames silead_dmi to a more generic
touchscreen_dmi name (and Kconfig option) in preparation of adding
info for tablets with an ICN8505 based touchscreen.

Note there are no functional changes all code changes are limited to
removing references to silead where these are no longer applicable.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 MAINTAINERS                                   |  2 +-
 drivers/platform/x86/Kconfig                  | 16 ++---
 drivers/platform/x86/Makefile                 |  2 +-
 .../x86/{silead_dmi.c => touchscreen_dmi.c}   | 72 +++++++++----------
 4 files changed, 46 insertions(+), 46 deletions(-)
 rename drivers/platform/x86/{silead_dmi.c => touchscreen_dmi.c} (88%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9474e9834dc7..6b02f2b9814d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12775,7 +12775,7 @@ L:	linux-input@vger.kernel.org
 L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/input/touchscreen/silead.c
-F:	drivers/platform/x86/silead_dmi.c
+F:	drivers/platform/x86/touchscreen_dmi.c
 
 SILICON MOTION SM712 FRAME BUFFER DRIVER
 M:	Sudip Mukherjee <sudipm.mukherjee@gmail.com>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 566644bb496a..d4e9670ee8cc 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1196,16 +1196,16 @@ config INTEL_TURBO_MAX_3
 	  This driver is only required when the system is not using Hardware
 	  P-States (HWP). In HWP mode, priority can be read from ACPI tables.
 
-config SILEAD_DMI
-	bool "Tablets with Silead touchscreens"
+config TOUCHSCREEN_DMI
+	bool "DMI based touchscreen configuration info"
 	depends on ACPI && DMI && I2C=y && TOUCHSCREEN_SILEAD
 	---help---
-	  Certain ACPI based tablets with Silead touchscreens do not have
-	  enough data in ACPI tables for the touchscreen driver to handle
-	  the touchscreen properly, as OEMs expected the data to be baked
-	  into the tablet model specific version of the driver shipped
-	  with the OS-image for the device. This option supplies the missing
-	  information. Enable this for x86 tablets with Silead touchscreens.
+	  Certain ACPI based tablets with e.g. Silead or Chipone touchscreens
+	  do not have enough data in ACPI tables for the touchscreen driver to
+	  handle the touchscreen properly, as OEMs expect the data to be baked
+	  into the tablet model specific version of the driver shipped with the
+	  the OS-image for the device. This option supplies the missing info.
+	  Enable this for x86 tablets with Silead or Chipone touchscreens.
 
 config INTEL_CHTDC_TI_PWRBTN
 	tristate "Intel Cherry Trail Dollar Cove TI power button driver"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 2ba6cb795338..8d9477114fb5 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -78,7 +78,7 @@ obj-$(CONFIG_INTEL_SMARTCONNECT)	+= intel-smartconnect.o
 obj-$(CONFIG_PVPANIC)           += pvpanic.o
 obj-$(CONFIG_ALIENWARE_WMI)	+= alienware-wmi.o
 obj-$(CONFIG_INTEL_PMC_IPC)	+= intel_pmc_ipc.o
-obj-$(CONFIG_SILEAD_DMI)	+= silead_dmi.o
+obj-$(CONFIG_TOUCHSCREEN_DMI)	+= touchscreen_dmi.o
 obj-$(CONFIG_SURFACE_PRO3_BUTTON)	+= surfacepro3_button.o
 obj-$(CONFIG_SURFACE_3_BUTTON)	+= surface3_button.o
 obj-$(CONFIG_INTEL_PUNIT_IPC)  += intel_punit_ipc.o
diff --git a/drivers/platform/x86/silead_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
similarity index 88%
rename from drivers/platform/x86/silead_dmi.c
rename to drivers/platform/x86/touchscreen_dmi.c
index 853a7ce4601c..8fb489e1e16c 100644
--- a/drivers/platform/x86/silead_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -1,5 +1,5 @@
 /*
- * Silead touchscreen driver DMI based configuration code
+ * Touchscreen driver DMI based configuration code
  *
  * Copyright (c) 2017 Red Hat Inc.
  *
@@ -20,7 +20,7 @@
 #include <linux/property.h>
 #include <linux/string.h>
 
-struct silead_ts_dmi_data {
+struct ts_dmi_data {
 	const char *acpi_name;
 	const struct property_entry *properties;
 };
@@ -34,7 +34,7 @@ static const struct property_entry cube_iwork8_air_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data cube_iwork8_air_data = {
+static const struct ts_dmi_data cube_iwork8_air_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= cube_iwork8_air_props,
 };
@@ -48,7 +48,7 @@ static const struct property_entry jumper_ezpad_mini3_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data jumper_ezpad_mini3_data = {
+static const struct ts_dmi_data jumper_ezpad_mini3_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= jumper_ezpad_mini3_props,
 };
@@ -62,7 +62,7 @@ static const struct property_entry jumper_ezpad_6_pro_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data jumper_ezpad_6_pro_data = {
+static const struct ts_dmi_data jumper_ezpad_6_pro_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= jumper_ezpad_6_pro_props,
 };
@@ -76,7 +76,7 @@ static const struct property_entry dexp_ursus_7w_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data dexp_ursus_7w_data = {
+static const struct ts_dmi_data dexp_ursus_7w_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= dexp_ursus_7w_props,
 };
@@ -91,7 +91,7 @@ static const struct property_entry surftab_twin_10_1_st10432_8_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data surftab_twin_10_1_st10432_8_data = {
+static const struct ts_dmi_data surftab_twin_10_1_st10432_8_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= surftab_twin_10_1_st10432_8_props,
 };
@@ -106,7 +106,7 @@ static const struct property_entry surftab_wintron70_st70416_6_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data surftab_wintron70_st70416_6_data = {
+static const struct ts_dmi_data surftab_wintron70_st70416_6_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= surftab_wintron70_st70416_6_props,
 };
@@ -121,7 +121,7 @@ static const struct property_entry gp_electronic_t701_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data gp_electronic_t701_data = {
+static const struct ts_dmi_data gp_electronic_t701_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= gp_electronic_t701_props,
 };
@@ -136,7 +136,7 @@ static const struct property_entry pipo_w2s_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data pipo_w2s_data = {
+static const struct ts_dmi_data pipo_w2s_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= pipo_w2s_props,
 };
@@ -154,7 +154,7 @@ static const struct property_entry pov_mobii_wintab_p800w_v20_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data pov_mobii_wintab_p800w_v20_data = {
+static const struct ts_dmi_data pov_mobii_wintab_p800w_v20_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= pov_mobii_wintab_p800w_v20_props,
 };
@@ -169,7 +169,7 @@ static const struct property_entry pov_mobii_wintab_p800w_v21_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data pov_mobii_wintab_p800w_v21_data = {
+static const struct ts_dmi_data pov_mobii_wintab_p800w_v21_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= pov_mobii_wintab_p800w_v21_props,
 };
@@ -183,7 +183,7 @@ static const struct property_entry itworks_tw891_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data itworks_tw891_data = {
+static const struct ts_dmi_data itworks_tw891_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= itworks_tw891_props,
 };
@@ -197,7 +197,7 @@ static const struct property_entry chuwi_hi8_pro_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data chuwi_hi8_pro_data = {
+static const struct ts_dmi_data chuwi_hi8_pro_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= chuwi_hi8_pro_props,
 };
@@ -213,7 +213,7 @@ static const struct property_entry digma_citi_e200_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data digma_citi_e200_data = {
+static const struct ts_dmi_data digma_citi_e200_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= digma_citi_e200_props,
 };
@@ -230,7 +230,7 @@ static const struct property_entry onda_obook_20_plus_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data onda_obook_20_plus_data = {
+static const struct ts_dmi_data onda_obook_20_plus_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= onda_obook_20_plus_props,
 };
@@ -244,7 +244,7 @@ static const struct property_entry chuwi_hi8_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data chuwi_hi8_data = {
+static const struct ts_dmi_data chuwi_hi8_data = {
 	.acpi_name      = "MSSL0001:00",
 	.properties     = chuwi_hi8_props,
 };
@@ -259,7 +259,7 @@ static const struct property_entry chuwi_vi8_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data chuwi_vi8_data = {
+static const struct ts_dmi_data chuwi_vi8_data = {
 	.acpi_name      = "MSSL1680:00",
 	.properties     = chuwi_vi8_props,
 };
@@ -274,7 +274,7 @@ static const struct property_entry trekstor_primebook_c13_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data trekstor_primebook_c13_data = {
+static const struct ts_dmi_data trekstor_primebook_c13_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= trekstor_primebook_c13_props,
 };
@@ -290,7 +290,7 @@ static const struct property_entry teclast_x98plus2_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data teclast_x98plus2_data = {
+static const struct ts_dmi_data teclast_x98plus2_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= teclast_x98plus2_props,
 };
@@ -304,7 +304,7 @@ static const struct property_entry teclast_x3_plus_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data teclast_x3_plus_data = {
+static const struct ts_dmi_data teclast_x3_plus_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= teclast_x3_plus_props,
 };
@@ -321,12 +321,12 @@ static const struct property_entry onda_v891w_v1_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data onda_v891w_v1_data = {
+static const struct ts_dmi_data onda_v891w_v1_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= onda_v891w_v1_props,
 };
 
-static const struct dmi_system_id silead_ts_dmi_table[] = {
+static const struct dmi_system_id touchscreen_dmi_table[] = {
 	{
 		/* CUBE iwork8 Air */
 		.driver_data = (void *)&cube_iwork8_air_data,
@@ -557,22 +557,22 @@ static const struct dmi_system_id silead_ts_dmi_table[] = {
 	{ },
 };
 
-static const struct silead_ts_dmi_data *silead_ts_data;
+static const struct ts_dmi_data *ts_data;
 
-static void silead_ts_dmi_add_props(struct i2c_client *client)
+static void ts_dmi_add_props(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	int error;
 
 	if (has_acpi_companion(dev) &&
-	    !strncmp(silead_ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
-		error = device_add_properties(dev, silead_ts_data->properties);
+	    !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
+		error = device_add_properties(dev, ts_data->properties);
 		if (error)
 			dev_err(dev, "failed to add properties: %d\n", error);
 	}
 }
 
-static int silead_ts_dmi_notifier_call(struct notifier_block *nb,
+static int ts_dmi_notifier_call(struct notifier_block *nb,
 				       unsigned long action, void *data)
 {
 	struct device *dev = data;
@@ -582,7 +582,7 @@ static int silead_ts_dmi_notifier_call(struct notifier_block *nb,
 	case BUS_NOTIFY_ADD_DEVICE:
 		client = i2c_verify_client(dev);
 		if (client)
-			silead_ts_dmi_add_props(client);
+			ts_dmi_add_props(client);
 		break;
 
 	default:
@@ -592,22 +592,22 @@ static int silead_ts_dmi_notifier_call(struct notifier_block *nb,
 	return 0;
 }
 
-static struct notifier_block silead_ts_dmi_notifier = {
-	.notifier_call = silead_ts_dmi_notifier_call,
+static struct notifier_block ts_dmi_notifier = {
+	.notifier_call = ts_dmi_notifier_call,
 };
 
-static int __init silead_ts_dmi_init(void)
+static int __init ts_dmi_init(void)
 {
 	const struct dmi_system_id *dmi_id;
 	int error;
 
-	dmi_id = dmi_first_match(silead_ts_dmi_table);
+	dmi_id = dmi_first_match(touchscreen_dmi_table);
 	if (!dmi_id)
 		return 0; /* Not an error */
 
-	silead_ts_data = dmi_id->driver_data;
+	ts_data = dmi_id->driver_data;
 
-	error = bus_register_notifier(&i2c_bus_type, &silead_ts_dmi_notifier);
+	error = bus_register_notifier(&i2c_bus_type, &ts_dmi_notifier);
 	if (error)
 		pr_err("%s: failed to register i2c bus notifier: %d\n",
 			__func__, error);
@@ -620,4 +620,4 @@ static int __init silead_ts_dmi_init(void)
  * itself is ready (which happens at postcore initcall level), but before
  * ACPI starts enumerating devices (at subsys initcall level).
  */
-arch_initcall(silead_ts_dmi_init);
+arch_initcall(ts_dmi_init);
-- 
2.17.0

[PATCH v6 4/5] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support

[Hans de Goede]

Sofar we have been unable to get permission from the vendors to put the
firmware for touchscreens listed in touchscreen_dmi in linux-firmware.

Some of the tablets with such a touchscreen have a touchscreen driver, and
thus a copy of the firmware, as part of their EFI code.

This commit adds the necessary info for the new EFI embedded-firmware code
to extract these firmwares, making the touchscreen work OOTB without the
user needing to manually add the firmware.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
-Switch from crc sums to SHA256 hashes for the firmware hashes
---
 drivers/firmware/efi/embedded-firmware.c |  3 ++
 drivers/platform/x86/Kconfig             |  1 +
 drivers/platform/x86/touchscreen_dmi.c   | 35 +++++++++++++++++++++++-
 include/linux/efi_embedded_fw.h          |  2 ++
 4 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
index 244465a162c5..19168b83cd06 100644
--- a/drivers/firmware/efi/embedded-firmware.c
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -23,6 +23,9 @@ struct embedded_fw {
 static LIST_HEAD(found_fw_list);
 
 static const struct dmi_system_id * const embedded_fw_table[] = {
+#ifdef CONFIG_TOUCHSCREEN_DMI
+	touchscreen_dmi_table,
+#endif
 	NULL
 };
 
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index d4e9670ee8cc..8c2082c8a7fd 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1199,6 +1199,7 @@ config INTEL_TURBO_MAX_3
 config TOUCHSCREEN_DMI
 	bool "DMI based touchscreen configuration info"
 	depends on ACPI && DMI && I2C=y && TOUCHSCREEN_SILEAD
+	select EFI_EMBEDDED_FIRMWARE if EFI
 	---help---
 	  Certain ACPI based tablets with e.g. Silead or Chipone touchscreens
 	  do not have enough data in ACPI tables for the touchscreen driver to
diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 8fb489e1e16c..8eb88bfcacbe 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -15,12 +15,15 @@
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
+#include <linux/efi_embedded_fw.h>
 #include <linux/i2c.h>
 #include <linux/notifier.h>
 #include <linux/property.h>
 #include <linux/string.h>
 
 struct ts_dmi_data {
+	/* The EFI embedded-fw code expects this to be the first member! */
+	struct efi_embedded_fw_desc embedded_fw;
 	const char *acpi_name;
 	const struct property_entry *properties;
 };
@@ -31,10 +34,20 @@ static const struct property_entry cube_iwork8_air_props[] = {
 	PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
 	PROPERTY_ENTRY_STRING("firmware-name", "gsl3670-cube-iwork8-air.fw"),
 	PROPERTY_ENTRY_U32("silead,max-fingers", 10),
+	PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
 	{ }
 };
 
 static const struct ts_dmi_data cube_iwork8_air_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl3670-cube-iwork8-air.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 38808,
+		.sha256	= { 0xff, 0x62, 0x2d, 0xd1, 0x8a, 0x78, 0x04, 0x7b,
+			    0x33, 0x06, 0xb0, 0x4f, 0x7f, 0x02, 0x08, 0x9c,
+			    0x96, 0xd4, 0x9f, 0x04, 0xe1, 0x47, 0x25, 0x25,
+			    0x60, 0x77, 0x41, 0x33, 0xeb, 0x12, 0x82, 0xfc },
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= cube_iwork8_air_props,
 };
@@ -133,10 +146,20 @@ static const struct property_entry pipo_w2s_props[] = {
 	PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
 	PROPERTY_ENTRY_STRING("firmware-name",
 			      "gsl1680-pipo-w2s.fw"),
+	PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
 	{ }
 };
 
 static const struct ts_dmi_data pipo_w2s_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl1680-pipo-w2s.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 39072,
+		.sha256	= { 0xd0, 0x58, 0xc4, 0x7d, 0x55, 0x2d, 0x62, 0x18,
+			    0xd1, 0x6a, 0x71, 0x73, 0x0b, 0x3f, 0xbe, 0x60,
+			    0xbb, 0x45, 0x8c, 0x52, 0x27, 0xb7, 0x18, 0xf4,
+			    0x31, 0x00, 0x6a, 0x49, 0x76, 0xd8, 0x7c, 0xd3 },
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= pipo_w2s_props,
 };
@@ -194,10 +217,20 @@ static const struct property_entry chuwi_hi8_pro_props[] = {
 	PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
 	PROPERTY_ENTRY_STRING("firmware-name", "gsl3680-chuwi-hi8-pro.fw"),
 	PROPERTY_ENTRY_BOOL("silead,home-button"),
+	PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
 	{ }
 };
 
 static const struct ts_dmi_data chuwi_hi8_pro_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl3680-chuwi-hi8-pro.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 39864,
+		.sha256	= { 0xc0, 0x88, 0xc5, 0xef, 0xd1, 0x70, 0x77, 0x59,
+			    0x4e, 0xe9, 0xc4, 0xd8, 0x2e, 0xcd, 0xbf, 0x95,
+			    0x32, 0xd9, 0x03, 0x28, 0x0d, 0x48, 0x9f, 0x92,
+			    0x35, 0x37, 0xf6, 0x8b, 0x2a, 0xe4, 0x73, 0xff },
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= chuwi_hi8_pro_props,
 };
@@ -326,7 +359,7 @@ static const struct ts_dmi_data onda_v891w_v1_data = {
 	.properties	= onda_v891w_v1_props,
 };
 
-static const struct dmi_system_id touchscreen_dmi_table[] = {
+const struct dmi_system_id touchscreen_dmi_table[] = {
 	{
 		/* CUBE iwork8 Air */
 		.driver_data = (void *)&cube_iwork8_air_data,
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
index 6c8b04246fc3..04a74578aaf0 100644
--- a/include/linux/efi_embedded_fw.h
+++ b/include/linux/efi_embedded_fw.h
@@ -20,6 +20,8 @@ struct efi_embedded_fw_desc {
 	u8 sha256[32];
 };
 
+extern const struct dmi_system_id touchscreen_dmi_table[];
+
 int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
 
 #endif
-- 
2.17.0

[PATCH v6 5/5] platform/x86: touchscreen_dmi: Add info for the Chuwi Vi8 Plus tablet

[Hans de Goede]

Add touchscreen info for the Chuwi Vi8 Plus tablet. This tablet uses a
Chipone ICN8505 touchscreen controller, with the firmware used by the
touchscreen embedded in the EFI firmware.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
-Switch from crc sums to SHA256 hashes for the firmware hash
---
 drivers/platform/x86/touchscreen_dmi.c | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 8eb88bfcacbe..99eb73bbd1a0 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -359,6 +359,25 @@ static const struct ts_dmi_data onda_v891w_v1_data = {
 	.properties	= onda_v891w_v1_props,
 };
 
+static const struct property_entry efi_embedded_fw_props[] = {
+	PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
+	{ }
+};
+
+static const struct ts_dmi_data chuwi_vi8_plus_data = {
+	.embedded_fw = {
+		.name	= "chipone/icn8505-HAMP0002.fw",
+		.prefix = { 0xb0, 0x07, 0x00, 0x00, 0xe4, 0x07, 0x00, 0x00 },
+		.length	= 35012,
+		.sha256	= { 0x93, 0xe5, 0x49, 0xe0, 0xb6, 0xa2, 0xb4, 0xb3,
+			    0x88, 0x96, 0x34, 0x97, 0x5e, 0xa8, 0x13, 0x78,
+			    0x72, 0x98, 0xb8, 0x29, 0xeb, 0x5c, 0xa7, 0xf1,
+			    0x25, 0x13, 0x43, 0xf4, 0x30, 0x7c, 0xfc, 0x7c },
+	},
+	.acpi_name	= "CHPN0001:00",
+	.properties	= efi_embedded_fw_props,
+};
+
 const struct dmi_system_id touchscreen_dmi_table[] = {
 	{
 		/* CUBE iwork8 Air */
@@ -587,6 +606,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 			DMI_EXACT_MATCH(DMI_BIOS_VERSION, "ONDA.W89EBBN08"),
 		},
 	},
+	{
+		/* Chuwi Vi8 Plus (CWI506) */
+		.driver_data = (void *)&chuwi_vi8_plus_data,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"),
+			DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
+		},
+	},
 	{ },
 };
 
-- 
2.17.0

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

[Randy Dunlap]

On 06/01/2018 05:53 AM, Hans de Goede wrote:
> 
> Reported-by: Dave Olsthoorn <dave@bewaar.me>
> Suggested-by: Peter Jones <pjones@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> ---
>  .../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/linux/fs.h                            |   1 +
>  init/main.c                                   |   3 +
>  12 files changed, 334 insertions(+)
>  create mode 100644 drivers/base/firmware_loader/fallback_efi.c
>  create mode 100644 drivers/firmware/efi/embedded-firmware.c
>  create mode 100644 include/linux/efi_embedded_fw.h
> 
> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
> index f62bdcbfed5b..66ab91f3357d 100644
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -73,3 +73,79 @@ If something went wrong request_firmware() returns non-zero and fw_entry
>  is set to NULL. Once your driver is done with processing the firmware it
>  can call call release_firmware(fw_entry) to release the firmware image
>  and any related resource.
> +
> +EFI embedded firmware support
> +=============================
> +
> +On some devices the system's EFI code / ROM may contain an embedded copy
> +of firmware for some of the system's integrated peripheral devices and
> +the peripheral's Linux device-driver needs to access this firmware.
> +
> +A device driver which needs this can describe the firmware it needs
> +using an efi_embedded_fw_desc struct:
> +
> +.. kernel-doc:: include/linux/efi_embedded_fw.h
> +   :functions: efi_embedded_fw_desc
> +
> +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory
> +segments for an eight byte sequence matching prefix, if the prefix is found it

                                                prefix; if

> +then does a crc32 over length bytes and if that matches makes a copy of length
> +bytes and adds that to its list with found firmwares.
> +
> +To avoid doing this somewhat expensive scan on all systems, dmi matching is
> +used. Drivers are expected to export a dmi_system_id array, with each entries'
> +driver_data pointing to an efi_embedded_fw_desc.
> +
> +To register this array with the efi-embedded-fw code, a driver needs to:
> +
> +1. Always be builtin to the kernel or store the dmi_system_id array in a
> +   separate object file which always gets builtin.
> +
> +2. Add an extern declaration for the dmi_system_id array to
> +   include/linux/efi_embedded_fw.h.
> +
> +3. Add the dmi_system_id array to the embedded_fw_table in
> +   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
> +   the driver is being builtin.
> +
> +4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
> +
> +The request_firmware() function will always first try to load firmware with
> +the specified name directly from the disk, so the EFI embedded-fw can always
> +be overridden by placing a file under /lib/firmare.

                                         /lib/firmware.

> +
> +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(). Note that this disables the
> +usual usermodehelper fallback, so you may want to only set this on systems
> +which match your dmi_system_id array.
> +
> +Once the device-property is set, the driver can use the regular
> +request_firmware() function to get the firmware, using the name filled in
> +in the efi_embedded_fw_desc.
> +
> +Note that:
> +
> +1. The code scanning for EFI embbedded-firmware runs near the end
> +   of start_kernel(), just before calling rest_init(). For normal drivers and
> +   subsystems using subsys_initcall() to register themselves this does not
> +   matter. This means that code running earlier cannot use EFI
> +   embbedded-firmware.
> +
> +2. ATM the EFI embedded-fw code assumes that firmwares always start at an offset

s/ATM/At the moment/

> +   which is a multiple of 8 bytes, if this is not true for your case send in

                               bytes; if

> +   a patch to fix this.
> +
> +3. ATM the EFI embedded-fw code only works on x86 because other archs free

At the moment

> +   EFI_BOOT_SERVICES_CODE before the EFI embedded-fw code gets a chance to
> +   scan it.
> +
> +4. The current brute-force scanning of EFI_BOOT_SERVICES_CODE is an ad-hoc
> +   brute-force solution. There has been discussion to use the PI spec's
> +   Firmware Volume protocol. This has been rejected because the FV Protocol
> +   relies on *internal* interfaces of PI spec, and:
> +   1. The The PI spec does not define firmware at all
> +   2. The internal interfaces of PI Spec does not guarantee any backward
> +      compatibility. Any implementation details in FV may be subject to change,
> +      and may vary system to system. Supporting the FV Protocol would be
> +      difficult as it is purposely ambiguous.

What/where is this PI spec?

thanks,
-- 
~Randy

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Andy Lutomirski]

On Fri, Jun 1, 2018 at 5:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
>
> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
> changes for v4.18" series from mcgrof.
>
> It now also depends on the series from Andy Lutomirski which allow using the
> sha256 code in a standalone manner. Andy what is the status of those?

They're currently sort of on hold, since Jason Donenfeld is working on
a more comprehensive solution to the same problem.  Jason, I don't
suppose the sha256 part of your patch set is ready?

Ard, if Jason's patches are too far in the future, would you be okay
with merging a cleaned-up version of my patch for sha256 even if the
sha512 equivalent isn't there?

--Andy

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Andy Lutomirski]

On Fri, Jun 1, 2018 at 5:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
>
> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
> changes for v4.18" series from mcgrof.
>
> It now also depends on the series from Andy Lutomirski which allow using the
> sha256 code in a standalone manner. Andy what is the status of those?

They're currently sort of on hold, since Jason Donenfeld is working on
a more comprehensive solution to the same problem.  Jason, I don't
suppose the sha256 part of your patch set is ready?

Ard, if Jason's patches are too far in the future, would you be okay
with merging a cleaned-up version of my patch for sha256 even if the
sha512 equivalent isn't there?

--Andy

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Ard Biesheuvel]

On 2 June 2018 at 05:39, Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Jun 1, 2018 at 5:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
>>
>> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
>> changes for v4.18" series from mcgrof.
>>
>> It now also depends on the series from Andy Lutomirski which allow using the
>> sha256 code in a standalone manner. Andy what is the status of those?
>
> They're currently sort of on hold, since Jason Donenfeld is working on
> a more comprehensive solution to the same problem.  Jason, I don't
> suppose the sha256 part of your patch set is ready?
>
> Ard, if Jason's patches are too far in the future, would you be okay
> with merging a cleaned-up version of my patch for sha256 even if the
> sha512 equivalent isn't there?
>

I'm not sure what you are asking me here. I'd be fine merging a
version of Hans's patches that uses a suitable cryptographic hash
algorithm, but the implementation of that would likely belong
somewhere under crypto/, and so it is ultimately someone else's call
(although I'm happy to review)

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Ard Biesheuvel]

On 2 June 2018 at 05:39, Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Jun 1, 2018 at 5:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
>>
>> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
>> changes for v4.18" series from mcgrof.
>>
>> It now also depends on the series from Andy Lutomirski which allow using the
>> sha256 code in a standalone manner. Andy what is the status of those?
>
> They're currently sort of on hold, since Jason Donenfeld is working on
> a more comprehensive solution to the same problem.  Jason, I don't
> suppose the sha256 part of your patch set is ready?
>
> Ard, if Jason's patches are too far in the future, would you be okay
> with merging a cleaned-up version of my patch for sha256 even if the
> sha512 equivalent isn't there?
>

I'm not sure what you are asking me here. I'd be fine merging a
version of Hans's patches that uses a suitable cryptographic hash
algorithm, but the implementation of that would likely belong
somewhere under crypto/, and so it is ultimately someone else's call
(although I'm happy to review)

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Luis R. Rodriguez]

On Fri, Jun 01, 2018 at 02:53:25PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
> 
> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
> changes for v4.18" series from mcgrof.
> 
> It now also depends on the series from Andy Lutomirski which allow using the
> sha256 code in a standalone manner. Andy what is the status of those?
> 
> Changes since v5:
> -Rework code to remove casts from if (prefix == mem) comparison
> -Use SHA256 hashes instead of crc32 sums

Nice! I see no updates on this progress, but it would seem this
may then mean this cannot be merged until the release after?

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

There's a discussion over having security_kernel_read_file(NULL,
READING_WHATEVER) become another LSM hook. So your series would conflict with
that at the moment.

So yet another piece of code which this series depends on.

> -Document why we are not using the PI Firmware Volume protocol
> 
> For reference I've included the coverletter from v4 (which includes
> previous covverletters) below.
> 
> Regards,
> 
> Hans
> 
> 
> Previous coverletter:
> 
> Here is v5 of my patch-set to add support for EFI embedded fw to the kernel.
> 
> Changes since v4:
> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS
> 
> So I think this patch-set is getting close to ready for merging, which
> brings us to the question of how to merge this, I think that patches 1
> and 2 should probably both be merged through the same tree. Then an
> unmutable branch should be created on that tree, merged into the
> platform/x86 tree and then the last 3 patches can be merged through
> that tree.
> 
> Ard has already indicated he is fine with the EFI bits going upstream
> through another tree, so perhaps patches 1-2 can be merged through the
> firmware-loader-tree and then do an unmutable branch on the
> firmware-loader-tree for the platform/x86 tree to merge?
> 
> I don't think taking all 5 through 1 tree is a good idea because of
> the file rename under platform/x86.
> 
> 
> For the record here are the cover letters of the previous versions:
> 
> Changes since v3:
> -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
> 
> The 3 most prominent changes in v2 are:
> 
> 1) Add documentation describing the EFI embedded firmware mechanism to:
>    Documentation/driver-api/firmware/request_firmware.rst
> 
> 2) Instead of having a single dmi_system_id array with its driver_data
>    members pointing to efi_embedded_fw_desc structs, have the drivers which
>    need EFI embedded-fw support export a dmi_system_id array and register
>    that with the EFI embedded-fw code
> 
>    This series also includes the first driver to use this, in the form of
>    the touchscreen_dmi code (formerly silead_dmi) from drivers/platfrom/x86
> 
> 3) As discussed during the review of v1 we want to make the firmware_loader
>    code fallback to EFI embedded-fw optional.  Rather the adding yet another
>    firmware_request_foo variant for this, with the risk of later also needing
>    firmware_request_foo_nowait, etc. variants I've decided to make the code
>    check if the device has a "efi-embedded-firmware" device-property bool set.
> 
>    This also seemed better because the same driver may want to use the
>    fallback on some systems, but not on others since e.g. not all (x86)
>    systems with a silead touchscreen have their touchscreen firmware embedded
>    in their EFI.
> 
>    Note that (as discussed) when the EFI fallback path is requested, the
>    usermodehelper fallback path is skipped.
> 
> Here is the full changelog of patch 2/5 which is where most of the changes are:
> 
> 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
> 
> Patches 3-5 are new and implement using the EFI embedded-fw mechanism for
> Silead gslXXXX and Chipone icn8505 touchscreens on x86 devices.
> 
> 

-- 
Do not panic

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Luis R. Rodriguez]

On Fri, Jun 01, 2018 at 02:53:25PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
> 
> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
> changes for v4.18" series from mcgrof.
> 
> It now also depends on the series from Andy Lutomirski which allow using the
> sha256 code in a standalone manner. Andy what is the status of those?
> 
> Changes since v5:
> -Rework code to remove casts from if (prefix == mem) comparison
> -Use SHA256 hashes instead of crc32 sums

Nice! I see no updates on this progress, but it would seem this
may then mean this cannot be merged until the release after?

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

There's a discussion over having security_kernel_read_file(NULL,
READING_WHATEVER) become another LSM hook. So your series would conflict with
that at the moment.

So yet another piece of code which this series depends on.

> -Document why we are not using the PI Firmware Volume protocol
> 
> For reference I've included the coverletter from v4 (which includes
> previous covverletters) below.
> 
> Regards,
> 
> Hans
> 
> 
> Previous coverletter:
> 
> Here is v5 of my patch-set to add support for EFI embedded fw to the kernel.
> 
> Changes since v4:
> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS
> 
> So I think this patch-set is getting close to ready for merging, which
> brings us to the question of how to merge this, I think that patches 1
> and 2 should probably both be merged through the same tree. Then an
> unmutable branch should be created on that tree, merged into the
> platform/x86 tree and then the last 3 patches can be merged through
> that tree.
> 
> Ard has already indicated he is fine with the EFI bits going upstream
> through another tree, so perhaps patches 1-2 can be merged through the
> firmware-loader-tree and then do an unmutable branch on the
> firmware-loader-tree for the platform/x86 tree to merge?
> 
> I don't think taking all 5 through 1 tree is a good idea because of
> the file rename under platform/x86.
> 
> 
> For the record here are the cover letters of the previous versions:
> 
> Changes since v3:
> -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
> 
> The 3 most prominent changes in v2 are:
> 
> 1) Add documentation describing the EFI embedded firmware mechanism to:
>    Documentation/driver-api/firmware/request_firmware.rst
> 
> 2) Instead of having a single dmi_system_id array with its driver_data
>    members pointing to efi_embedded_fw_desc structs, have the drivers which
>    need EFI embedded-fw support export a dmi_system_id array and register
>    that with the EFI embedded-fw code
> 
>    This series also includes the first driver to use this, in the form of
>    the touchscreen_dmi code (formerly silead_dmi) from drivers/platfrom/x86
> 
> 3) As discussed during the review of v1 we want to make the firmware_loader
>    code fallback to EFI embedded-fw optional.  Rather the adding yet another
>    firmware_request_foo variant for this, with the risk of later also needing
>    firmware_request_foo_nowait, etc. variants I've decided to make the code
>    check if the device has a "efi-embedded-firmware" device-property bool set.
> 
>    This also seemed better because the same driver may want to use the
>    fallback on some systems, but not on others since e.g. not all (x86)
>    systems with a silead touchscreen have their touchscreen firmware embedded
>    in their EFI.
> 
>    Note that (as discussed) when the EFI fallback path is requested, the
>    usermodehelper fallback path is skipped.
> 
> Here is the full changelog of patch 2/5 which is where most of the changes are:
> 
> 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
> 
> Patches 3-5 are new and implement using the EFI embedded-fw mechanism for
> Silead gslXXXX and Chipone icn8505 touchscreens on x86 devices.
> 
> 

-- 
Do not panic

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

[Luis R. Rodriguez]

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 <dave@bewaar.me>
> Suggested-by: Peter Jones <pjones@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> 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/linux/fs.h                            |   1 +
>  init/main.c                                   |   3 +
>  12 files changed, 334 insertions(+)
>  create mode 100644 drivers/base/firmware_loader/fallback_efi.c
>  create mode 100644 drivers/firmware/efi/embedded-firmware.c
>  create mode 100644 include/linux/efi_embedded_fw.h
> 
> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
> index f62bdcbfed5b..66ab91f3357d 100644
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -73,3 +73,79 @@ If something went wrong request_firmware() returns non-zero and fw_entry
>  is set to NULL. Once your driver is done with processing the firmware it
>  can call call release_firmware(fw_entry) to release the firmware image
>  and any related resource.
> +
> +EFI embedded firmware support
> +=============================
> +
> +On some devices the system's EFI code / ROM may contain an embedded copy
> +of firmware for some of the system's integrated peripheral devices and
> +the peripheral's Linux device-driver needs to access this firmware.
> +
> +A device driver which needs this can describe the firmware it needs
> +using an efi_embedded_fw_desc struct:
> +
> +.. kernel-doc:: include/linux/efi_embedded_fw.h
> +   :functions: efi_embedded_fw_desc
> +
> +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory
> +segments for an eight byte sequence matching prefix, if the prefix is found it
> +then does a crc32 

You still refer to crc32 here. This needs to be updated.

> over length bytes and if that matches makes a copy of length
> +bytes and adds that to its list with found firmwares.
> +
> +To avoid doing this somewhat expensive scan on all systems, dmi matching is
> +used. Drivers are expected to export a dmi_system_id array, with each entries'
> +driver_data pointing to an efi_embedded_fw_desc.
> +
> +To register this array with the efi-embedded-fw code, a driver needs to:
> +
> +1. Always be builtin to the kernel or store the dmi_system_id array in a
> +   separate object file which always gets builtin.
> +
> +2. Add an extern declaration for the dmi_system_id array to
> +   include/linux/efi_embedded_fw.h.
> +
> +3. Add the dmi_system_id array to the embedded_fw_table in
> +   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
> +   the driver is being builtin.
> +
> +4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
> +
> +The request_firmware() function will always first try to load firmware with
> +the specified name directly from the disk, so the EFI embedded-fw can always
> +be overridden by placing a file under /lib/firmare.
> +
> +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.

So NACK for now.

  Luis

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

[Luis R. Rodriguez]

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 <dave@bewaar.me>
> Suggested-by: Peter Jones <pjones@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> 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/linux/fs.h                            |   1 +
>  init/main.c                                   |   3 +
>  12 files changed, 334 insertions(+)
>  create mode 100644 drivers/base/firmware_loader/fallback_efi.c
>  create mode 100644 drivers/firmware/efi/embedded-firmware.c
>  create mode 100644 include/linux/efi_embedded_fw.h
> 
> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
> index f62bdcbfed5b..66ab91f3357d 100644
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -73,3 +73,79 @@ If something went wrong request_firmware() returns non-zero and fw_entry
>  is set to NULL. Once your driver is done with processing the firmware it
>  can call call release_firmware(fw_entry) to release the firmware image
>  and any related resource.
> +
> +EFI embedded firmware support
> +=============================
> +
> +On some devices the system's EFI code / ROM may contain an embedded copy
> +of firmware for some of the system's integrated peripheral devices and
> +the peripheral's Linux device-driver needs to access this firmware.
> +
> +A device driver which needs this can describe the firmware it needs
> +using an efi_embedded_fw_desc struct:
> +
> +.. kernel-doc:: include/linux/efi_embedded_fw.h
> +   :functions: efi_embedded_fw_desc
> +
> +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory
> +segments for an eight byte sequence matching prefix, if the prefix is found it
> +then does a crc32 

You still refer to crc32 here. This needs to be updated.

> over length bytes and if that matches makes a copy of length
> +bytes and adds that to its list with found firmwares.
> +
> +To avoid doing this somewhat expensive scan on all systems, dmi matching is
> +used. Drivers are expected to export a dmi_system_id array, with each entries'
> +driver_data pointing to an efi_embedded_fw_desc.
> +
> +To register this array with the efi-embedded-fw code, a driver needs to:
> +
> +1. Always be builtin to the kernel or store the dmi_system_id array in a
> +   separate object file which always gets builtin.
> +
> +2. Add an extern declaration for the dmi_system_id array to
> +   include/linux/efi_embedded_fw.h.
> +
> +3. Add the dmi_system_id array to the embedded_fw_table in
> +   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
> +   the driver is being builtin.
> +
> +4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
> +
> +The request_firmware() function will always first try to load firmware with
> +the specified name directly from the disk, so the EFI embedded-fw can always
> +be overridden by placing a file under /lib/firmare.
> +
> +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.

So NACK for now.

  Luis

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Hans de Goede]

Hi,

On 05-06-18 22:46, Luis R. Rodriguez wrote:
> On Fri, Jun 01, 2018 at 02:53:25PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
>>
>> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
>> changes for v4.18" series from mcgrof.
>>
>> It now also depends on the series from Andy Lutomirski which allow using the
>> sha256 code in a standalone manner. Andy what is the status of those?
>>
>> Changes since v5:
>> -Rework code to remove casts from if (prefix == mem) comparison
>> -Use SHA256 hashes instead of crc32 sums
> 
> Nice! I see no updates on this progress, but it would seem this
> may then mean this cannot be merged until the release after?

Once the sha256 bits are in place the subsys tree which has them
merged can create an immutable branch for Greg to merge and
then these can be applied on top of that merge.

But yes this means that these probably won't go in for another
cycle or 2, that is fine.

>> -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
> 
> There's a discussion over having security_kernel_read_file(NULL,
> READING_WHATEVER) become another LSM hook. So your series would conflict with
> that at the moment.
> 
> So yet another piece of code which this series depends on.

Ah well, I'm in no big hurry to get this merged. OTOH if this is
ready and that discussion is not yet finished it might be better
to merge this as is and then have the security_kernel_read_file / LSM
hook series fix this up as necessary when it is merged.

Regards,

Hans



> 
>> -Document why we are not using the PI Firmware Volume protocol
>>
>> For reference I've included the coverletter from v4 (which includes
>> previous covverletters) below.
>>
>> Regards,
>>
>> Hans
>>
>>
>> Previous coverletter:
>>
>> Here is v5 of my patch-set to add support for EFI embedded fw to the kernel.
>>
>> Changes since v4:
>> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS
>>
>> So I think this patch-set is getting close to ready for merging, which
>> brings us to the question of how to merge this, I think that patches 1
>> and 2 should probably both be merged through the same tree. Then an
>> unmutable branch should be created on that tree, merged into the
>> platform/x86 tree and then the last 3 patches can be merged through
>> that tree.
>>
>> Ard has already indicated he is fine with the EFI bits going upstream
>> through another tree, so perhaps patches 1-2 can be merged through the
>> firmware-loader-tree and then do an unmutable branch on the
>> firmware-loader-tree for the platform/x86 tree to merge?
>>
>> I don't think taking all 5 through 1 tree is a good idea because of
>> the file rename under platform/x86.
>>
>>
>> For the record here are the cover letters of the previous versions:
>>
>> Changes since v3:
>> -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
>>
>> The 3 most prominent changes in v2 are:
>>
>> 1) Add documentation describing the EFI embedded firmware mechanism to:
>>     Documentation/driver-api/firmware/request_firmware.rst
>>
>> 2) Instead of having a single dmi_system_id array with its driver_data
>>     members pointing to efi_embedded_fw_desc structs, have the drivers which
>>     need EFI embedded-fw support export a dmi_system_id array and register
>>     that with the EFI embedded-fw code
>>
>>     This series also includes the first driver to use this, in the form of
>>     the touchscreen_dmi code (formerly silead_dmi) from drivers/platfrom/x86
>>
>> 3) As discussed during the review of v1 we want to make the firmware_loader
>>     code fallback to EFI embedded-fw optional.  Rather the adding yet another
>>     firmware_request_foo variant for this, with the risk of later also needing
>>     firmware_request_foo_nowait, etc. variants I've decided to make the code
>>     check if the device has a "efi-embedded-firmware" device-property bool set.
>>
>>     This also seemed better because the same driver may want to use the
>>     fallback on some systems, but not on others since e.g. not all (x86)
>>     systems with a silead touchscreen have their touchscreen firmware embedded
>>     in their EFI.
>>
>>     Note that (as discussed) when the EFI fallback path is requested, the
>>     usermodehelper fallback path is skipped.
>>
>> Here is the full changelog of patch 2/5 which is where most of the changes are:
>>
>> 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
>>
>> Patches 3-5 are new and implement using the EFI embedded-fw mechanism for
>> Silead gslXXXX and Chipone icn8505 touchscreens on x86 devices.
>>
>>
> 

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Hans de Goede]

Hi,

On 05-06-18 22:46, Luis R. Rodriguez wrote:
> On Fri, Jun 01, 2018 at 02:53:25PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
>>
>> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
>> changes for v4.18" series from mcgrof.
>>
>> It now also depends on the series from Andy Lutomirski which allow using the
>> sha256 code in a standalone manner. Andy what is the status of those?
>>
>> Changes since v5:
>> -Rework code to remove casts from if (prefix == mem) comparison
>> -Use SHA256 hashes instead of crc32 sums
> 
> Nice! I see no updates on this progress, but it would seem this
> may then mean this cannot be merged until the release after?

Once the sha256 bits are in place the subsys tree which has them
merged can create an immutable branch for Greg to merge and
then these can be applied on top of that merge.

But yes this means that these probably won't go in for another
cycle or 2, that is fine.

>> -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
> 
> There's a discussion over having security_kernel_read_file(NULL,
> READING_WHATEVER) become another LSM hook. So your series would conflict with
> that at the moment.
> 
> So yet another piece of code which this series depends on.

Ah well, I'm in no big hurry to get this merged. OTOH if this is
ready and that discussion is not yet finished it might be better
to merge this as is and then have the security_kernel_read_file / LSM
hook series fix this up as necessary when it is merged.

Regards,

Hans



> 
>> -Document why we are not using the PI Firmware Volume protocol
>>
>> For reference I've included the coverletter from v4 (which includes
>> previous covverletters) below.
>>
>> Regards,
>>
>> Hans
>>
>>
>> Previous coverletter:
>>
>> Here is v5 of my patch-set to add support for EFI embedded fw to the kernel.
>>
>> Changes since v4:
>> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS
>>
>> So I think this patch-set is getting close to ready for merging, which
>> brings us to the question of how to merge this, I think that patches 1
>> and 2 should probably both be merged through the same tree. Then an
>> unmutable branch should be created on that tree, merged into the
>> platform/x86 tree and then the last 3 patches can be merged through
>> that tree.
>>
>> Ard has already indicated he is fine with the EFI bits going upstream
>> through another tree, so perhaps patches 1-2 can be merged through the
>> firmware-loader-tree and then do an unmutable branch on the
>> firmware-loader-tree for the platform/x86 tree to merge?
>>
>> I don't think taking all 5 through 1 tree is a good idea because of
>> the file rename under platform/x86.
>>
>>
>> For the record here are the cover letters of the previous versions:
>>
>> Changes since v3:
>> -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
>>
>> The 3 most prominent changes in v2 are:
>>
>> 1) Add documentation describing the EFI embedded firmware mechanism to:
>>     Documentation/driver-api/firmware/request_firmware.rst
>>
>> 2) Instead of having a single dmi_system_id array with its driver_data
>>     members pointing to efi_embedded_fw_desc structs, have the drivers which
>>     need EFI embedded-fw support export a dmi_system_id array and register
>>     that with the EFI embedded-fw code
>>
>>     This series also includes the first driver to use this, in the form of
>>     the touchscreen_dmi code (formerly silead_dmi) from drivers/platfrom/x86
>>
>> 3) As discussed during the review of v1 we want to make the firmware_loader
>>     code fallback to EFI embedded-fw optional.  Rather the adding yet another
>>     firmware_request_foo variant for this, with the risk of later also needing
>>     firmware_request_foo_nowait, etc. variants I've decided to make the code
>>     check if the device has a "efi-embedded-firmware" device-property bool set.
>>
>>     This also seemed better because the same driver may want to use the
>>     fallback on some systems, but not on others since e.g. not all (x86)
>>     systems with a silead touchscreen have their touchscreen firmware embedded
>>     in their EFI.
>>
>>     Note that (as discussed) when the EFI fallback path is requested, the
>>     usermodehelper fallback path is skipped.
>>
>> Here is the full changelog of patch 2/5 which is where most of the changes are:
>>
>> 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
>>
>> Patches 3-5 are new and implement using the EFI embedded-fw mechanism for
>> Silead gslXXXX and Chipone icn8505 touchscreens on x86 devices.
>>
>>
> 

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Luis R. Rodriguez]

On Wed, Jun 06, 2018 at 08:17:26PM +0200, Hans de Goede wrote:
> But yes this means that these probably won't go in for another
> cycle or 2, that is fine.
> 
> > > -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
> > 
> > There's a discussion over having security_kernel_read_file(NULL,
> > READING_WHATEVER) become another LSM hook. So your series would conflict with
> > that at the moment.
> > 
> > So yet another piece of code which this series depends on.
> 
> Ah well, I'm in no big hurry to get this merged. OTOH if this is
> ready and that discussion is not yet finished it might be better
> to merge this as is and then have the security_kernel_read_file / LSM
> hook series fix this up as necessary when it is merged.

True, there is also value in getting this series reviewed so that all
that is needed is to consider merging it, so if you address the new
call as I requested in a next series I'll review the series then.

  Luis

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Luis R. Rodriguez]

On Wed, Jun 06, 2018 at 08:17:26PM +0200, Hans de Goede wrote:
> But yes this means that these probably won't go in for another
> cycle or 2, that is fine.
> 
> > > -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
> > 
> > There's a discussion over having security_kernel_read_file(NULL,
> > READING_WHATEVER) become another LSM hook. So your series would conflict with
> > that at the moment.
> > 
> > So yet another piece of code which this series depends on.
> 
> Ah well, I'm in no big hurry to get this merged. OTOH if this is
> ready and that discussion is not yet finished it might be better
> to merge this as is and then have the security_kernel_read_file / LSM
> hook series fix this up as necessary when it is merged.

True, there is also value in getting this series reviewed so that all
that is needed is to consider merging it, so if you address the new
call as I requested in a next series I'll review the series then.

  Luis

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

[Hans de Goede]

Hi,

On 05-06-18 23:07, Luis R. Rodriguez wrote:
> 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 <dave@bewaar.me>
>> Suggested-by: Peter Jones <pjones@redhat.com>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> 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/linux/fs.h                            |   1 +
>>   init/main.c                                   |   3 +
>>   12 files changed, 334 insertions(+)
>>   create mode 100644 drivers/base/firmware_loader/fallback_efi.c
>>   create mode 100644 drivers/firmware/efi/embedded-firmware.c
>>   create mode 100644 include/linux/efi_embedded_fw.h
>>
>> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
>> index f62bdcbfed5b..66ab91f3357d 100644
>> --- a/Documentation/driver-api/firmware/request_firmware.rst
>> +++ b/Documentation/driver-api/firmware/request_firmware.rst
>> @@ -73,3 +73,79 @@ If something went wrong request_firmware() returns non-zero and fw_entry
>>   is set to NULL. Once your driver is done with processing the firmware it
>>   can call call release_firmware(fw_entry) to release the firmware image
>>   and any related resource.
>> +
>> +EFI embedded firmware support
>> +=============================
>> +
>> +On some devices the system's EFI code / ROM may contain an embedded copy
>> +of firmware for some of the system's integrated peripheral devices and
>> +the peripheral's Linux device-driver needs to access this firmware.
>> +
>> +A device driver which needs this can describe the firmware it needs
>> +using an efi_embedded_fw_desc struct:
>> +
>> +.. kernel-doc:: include/linux/efi_embedded_fw.h
>> +   :functions: efi_embedded_fw_desc
>> +
>> +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory
>> +segments for an eight byte sequence matching prefix, if the prefix is found it
>> +then does a crc32
> 
> You still refer to crc32 here. This needs to be updated.

Ack.

>> over length bytes and if that matches makes a copy of length
>> +bytes and adds that to its list with found firmwares.
>> +
>> +To avoid doing this somewhat expensive scan on all systems, dmi matching is
>> +used. Drivers are expected to export a dmi_system_id array, with each entries'
>> +driver_data pointing to an efi_embedded_fw_desc.
>> +
>> +To register this array with the efi-embedded-fw code, a driver needs to:
>> +
>> +1. Always be builtin to the kernel or store the dmi_system_id array in a
>> +   separate object file which always gets builtin.
>> +
>> +2. Add an extern declaration for the dmi_system_id array to
>> +   include/linux/efi_embedded_fw.h.
>> +
>> +3. Add the dmi_system_id array to the embedded_fw_table in
>> +   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
>> +   the driver is being builtin.
>> +
>> +4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
>> +
>> +The request_firmware() function will always first try to load firmware with
>> +the specified name directly from the disk, so the EFI embedded-fw can always
>> +be overridden by placing a file under /lib/firmare.
>> +
>> +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? 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.

And then have this pass a new FW_OPT_SYS_FW_FALLBACK flag to
_request_firmware(), right ?

2) Should this flag then be checked inside _request_firmware() before it
calls fw_get_efi_embedded_fw() (which may be an empty stub), or
inside fw_get_efi_embedded_fw()?

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 ?

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() ?

(note this is the wrapper which calls efi_get_embedded_fw()
which itself will not be renamed of course).

Regards,

Hans

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

[Hans de Goede]

Hi,

On 05-06-18 23:07, Luis R. Rodriguez wrote:
> 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 <dave@bewaar.me>
>> Suggested-by: Peter Jones <pjones@redhat.com>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> 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/linux/fs.h                            |   1 +
>>   init/main.c                                   |   3 +
>>   12 files changed, 334 insertions(+)
>>   create mode 100644 drivers/base/firmware_loader/fallback_efi.c
>>   create mode 100644 drivers/firmware/efi/embedded-firmware.c
>>   create mode 100644 include/linux/efi_embedded_fw.h
>>
>> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
>> index f62bdcbfed5b..66ab91f3357d 100644
>> --- a/Documentation/driver-api/firmware/request_firmware.rst
>> +++ b/Documentation/driver-api/firmware/request_firmware.rst
>> @@ -73,3 +73,79 @@ If something went wrong request_firmware() returns non-zero and fw_entry
>>   is set to NULL. Once your driver is done with processing the firmware it
>>   can call call release_firmware(fw_entry) to release the firmware image
>>   and any related resource.
>> +
>> +EFI embedded firmware support
>> +=============================
>> +
>> +On some devices the system's EFI code / ROM may contain an embedded copy
>> +of firmware for some of the system's integrated peripheral devices and
>> +the peripheral's Linux device-driver needs to access this firmware.
>> +
>> +A device driver which needs this can describe the firmware it needs
>> +using an efi_embedded_fw_desc struct:
>> +
>> +.. kernel-doc:: include/linux/efi_embedded_fw.h
>> +   :functions: efi_embedded_fw_desc
>> +
>> +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory
>> +segments for an eight byte sequence matching prefix, if the prefix is found it
>> +then does a crc32
> 
> You still refer to crc32 here. This needs to be updated.

Ack.

>> over length bytes and if that matches makes a copy of length
>> +bytes and adds that to its list with found firmwares.
>> +
>> +To avoid doing this somewhat expensive scan on all systems, dmi matching is
>> +used. Drivers are expected to export a dmi_system_id array, with each entries'
>> +driver_data pointing to an efi_embedded_fw_desc.
>> +
>> +To register this array with the efi-embedded-fw code, a driver needs to:
>> +
>> +1. Always be builtin to the kernel or store the dmi_system_id array in a
>> +   separate object file which always gets builtin.
>> +
>> +2. Add an extern declaration for the dmi_system_id array to
>> +   include/linux/efi_embedded_fw.h.
>> +
>> +3. Add the dmi_system_id array to the embedded_fw_table in
>> +   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
>> +   the driver is being builtin.
>> +
>> +4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
>> +
>> +The request_firmware() function will always first try to load firmware with
>> +the specified name directly from the disk, so the EFI embedded-fw can always
>> +be overridden by placing a file under /lib/firmare.
>> +
>> +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? 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.

And then have this pass a new FW_OPT_SYS_FW_FALLBACK flag to
_request_firmware(), right ?

2) Should this flag then be checked inside _request_firmware() before it
calls fw_get_efi_embedded_fw() (which may be an empty stub), or
inside fw_get_efi_embedded_fw()?

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 ?

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() ?

(note this is the wrapper which calls efi_get_embedded_fw()
which itself will not be renamed of course).

Regards,

Hans

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Andy Lutomirski]

On Wed, Jun 6, 2018 at 11:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 05-06-18 22:46, Luis R. Rodriguez wrote:
> > On Fri, Jun 01, 2018 at 02:53:25PM +0200, Hans de Goede wrote:
> >> Hi All,
> >>
> >> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
> >>
> >> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
> >> changes for v4.18" series from mcgrof.
> >>
> >> It now also depends on the series from Andy Lutomirski which allow using the
> >> sha256 code in a standalone manner. Andy what is the status of those?
> >>
> >> Changes since v5:
> >> -Rework code to remove casts from if (prefix == mem) comparison
> >> -Use SHA256 hashes instead of crc32 sums
> >
> > Nice! I see no updates on this progress, but it would seem this
> > may then mean this cannot be merged until the release after?
>
> Once the sha256 bits are in place the subsys tree which has them
> merged can create an immutable branch for Greg to merge and
> then these can be applied on top of that merge.
>
> But yes this means that these probably won't go in for another
> cycle or 2, that is fine.
>
> >> -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
> >
> > There's a discussion over having security_kernel_read_file(NULL,
> > READING_WHATEVER) become another LSM hook. So your series would conflict with
> > that at the moment.
> >
> > So yet another piece of code which this series depends on.
>
> Ah well, I'm in no big hurry to get this merged. OTOH if this is
> ready and that discussion is not yet finished it might be better
> to merge this as is and then have the security_kernel_read_file / LSM
> hook series fix this up as necessary when it is merged.
>

Let's give Jason a bit longer to reply.  I know he's actively working
on this thing, but it's part of a bigger project.

Re: [PATCH v6 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Andy Lutomirski]

On Wed, Jun 6, 2018 at 11:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 05-06-18 22:46, Luis R. Rodriguez wrote:
> > On Fri, Jun 01, 2018 at 02:53:25PM +0200, Hans de Goede wrote:
> >> Hi All,
> >>
> >> Here is v6 of my patch-set to add support for EFI embedded fw to the kernel.
> >>
> >> This patch-set applies on top of the "[PATCH v7 00/14] firmware_loader
> >> changes for v4.18" series from mcgrof.
> >>
> >> It now also depends on the series from Andy Lutomirski which allow using the
> >> sha256 code in a standalone manner. Andy what is the status of those?
> >>
> >> Changes since v5:
> >> -Rework code to remove casts from if (prefix == mem) comparison
> >> -Use SHA256 hashes instead of crc32 sums
> >
> > Nice! I see no updates on this progress, but it would seem this
> > may then mean this cannot be merged until the release after?
>
> Once the sha256 bits are in place the subsys tree which has them
> merged can create an immutable branch for Greg to merge and
> then these can be applied on top of that merge.
>
> But yes this means that these probably won't go in for another
> cycle or 2, that is fine.
>
> >> -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
> >
> > There's a discussion over having security_kernel_read_file(NULL,
> > READING_WHATEVER) become another LSM hook. So your series would conflict with
> > that at the moment.
> >
> > So yet another piece of code which this series depends on.
>
> Ah well, I'm in no big hurry to get this merged. OTOH if this is
> ready and that discussion is not yet finished it might be better
> to merge this as is and then have the security_kernel_read_file / LSM
> hook series fix this up as necessary when it is merged.
>

Let's give Jason a bit longer to reply.  I know he's actively working
on this thing, but it's part of a bigger project.

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

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

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

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

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

[Hans de Goede]

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

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

[Luis R. Rodriguez]

On Thu, Jun 07, 2018 at 03:46:11PM +0200, Hans de Goede wrote:
> 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:
> > > 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.

Fine by me, so in your case the syfs fallback will be ignored.

Which gets me thinking that perhaps we should have a separate
syfs fallback call. There are only two drivers that use it
explicitly:

  o CONFIG_LEDS_LP55XX_COMMON
	request_firmware_nowait(THIS_MODULE, false, ...);
  o CONFIG_DELL_RBU
	request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG, ...)

And FW_ACTION_NOHOTPLUG is 0.

The above revelation of the async call being data driven is a good
opportunity to break that tradition to the more preferred functional
one. And so we'd also get rid of the FW_ACTION_NOHOTPLUG option
all together.

So a new firmware_request_sysfs_async() I suppose.

  Luis

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

[Luis R. Rodriguez]

On Thu, Jun 07, 2018 at 03:46:11PM +0200, Hans de Goede wrote:
> 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:
> > > 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.

Fine by me, so in your case the syfs fallback will be ignored.

Which gets me thinking that perhaps we should have a separate
syfs fallback call. There are only two drivers that use it
explicitly:

  o CONFIG_LEDS_LP55XX_COMMON
	request_firmware_nowait(THIS_MODULE, false, ...);
  o CONFIG_DELL_RBU
	request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG, ...)

And FW_ACTION_NOHOTPLUG is 0.

The above revelation of the async call being data driven is a good
opportunity to break that tradition to the more preferred functional
one. And so we'd also get rid of the FW_ACTION_NOHOTPLUG option
all together.

So a new firmware_request_sysfs_async() I suppose.

  Luis