[PATCH v11 00/10] efi/firmware/platform-x86: Add EFI embedded fw support

[PATCH v11 00/10] efi/firmware/platform-x86: Add EFI embedded fw support

[Hans de Goede]

Hi All,

Here is v11 of my patch-set to add support for EFI embedded fw to the
kernel. This version has been rebased on Ardb's latest efi/next branch to
fix a (trivial) conflict caused by the
"efi/libstub: Remove 'sys_table_arg' from all function prototypes" commit in
efi/next. The only other change in v11 is dropping a few empty lines which
snuck into the "test_firmware: add support for firmware_request_platform"
patch.

I believe that this series is ready for merging now, patches 3-8
and patches 9-10 depend on the first 2 patches, so we need to get those
merged first.

Ingo, can you create an immutable branch targetting 5.6 for Greg /
the driver-platform-x86 maintainers with these 2 patches in it ?

Note as mentioned I've based this version on Ard's efi/next branch so
if you decide to do the immutable-branch directly on yop of 5.5-rc1 you need
to manually fixup a conflict (or I can send you a version based on 5.5-rc1).

Once the immutable-branch is in place then Greg can merge 3-8 through his
driver core tree and 9 and 10 can be merged through the drivers-platform-x86
tree (there are no compile time dependencies between these 2 sets).

Regards,

Hans

---

Changes in v11:
- Rebase on top of Ardb's efi/next
- Drop a couple of empty lines which snuck into:
  "test_firmware: add support for firmware_request_platform"

Changes in v10:
- Rebase on top of 5.5-rc1

Changes in v9:
- Add 2 new patches adding selftests
- At least touchscreen_dmi.c uses the same dmi_table for its own private
  data and the fw_desc structs, putting the fw_desc struct first in the
  data driver_data points to so that the dmi_table can be shared with
  efi_check_for_embedded_firmwares(). But not all entries there have
  embedded-fw so in some cases the fw_desc is empty (zero-ed out).
  This can lead to a possible crash because fw_desc->length now is
  less then 8, so if the segment size is close enough to a multiple of the
  page_size, then the memcmp to check the prefix my segfault. Crashing the
  machine. v9 checks for and skips these empty fw_desc entries avoiding this.
- Add static inline wrapper for firmware_request_platform() to firmware.h,
  for when CONFIG_FW_LOADER is not set

Changes in v8:
- Add pr_warn if there are mode then EFI_DEBUGFS_MAX_BLOBS boot service segments
- Document how the EFI debugfs boot_service_code? files can be used to check for
  embedded firmware
- Properly deal with the case of an EFI segment being smaller then the fw we
  are looking for
- Log a warning when efi_get_embedded_fw get called while we did not (yet)
  check for embedded firmwares
- Only build fallback_platform.c if CONFIG_EFI_EMBEDDED_FIRMWARE is defined,
  otherwise make firmware_fallback_platform() a static inline stub

Changes in v7:
- Split drivers/firmware/efi and drivers/base/firmware_loader changes into
  2 patches
- Use new, standalone, lib/crypto/sha256.c code
- Address kdoc comments from Randy Dunlap
- Add new FW_OPT_FALLBACK_PLATFORM flag and firmware_request_platform()
  _request_firmware() wrapper, as requested by Luis R. Rodriguez
- Stop using "efi-embedded-firmware" device-property, now that drivers need to
  use the new firmware_request_platform() to enable fallback to a device fw
  copy embedded in the platform's main firmware, we no longer need a property
  on the device to trigger this behavior
- Use security_kernel_load_data instead of calling
  security_kernel_read_file with a NULL file pointer argument
- Move the docs to Documentation/driver-api/firmware/fallback-mechanisms.rst
- Document the new firmware_request_platform() function in
  Documentation/driver-api/firmware/request_firmware.rst
- Add 2 new patches for the silead and chipone-icn8505 touchscreen drivers
  to use the new firmware_request_platform() method
- Rebased on top of 5.4-rc1

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

[PATCH v11 01/10] 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 v8:
-Add pr_warn if there are mode then EFI_DEBUGFS_MAX_BLOBS boot service segments
-Document how the boot_service_code? files can be used to check for embedded
 firmware. Note since this is related to the firmware EFI embedded fw support,
 these docs are added in the 4th patch of this patch-set, not in this one.

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     | 57 ++++++++++++++++++++++++++++++++++
 include/linux/efi.h            |  1 +
 4 files changed, 63 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 59f7f6d60cf6..51ee2e2a18d6 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -243,6 +243,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 46807b7606da..d5e822373450 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -409,6 +409,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 621220ab3d0e..201a6799671d 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -17,6 +17,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,59 @@ 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;
+		}
+
+		if (i >= EFI_DEBUGFS_MAX_BLOBS) {
+			pr_warn("More then %d EFI boot service segments, only showing first %d in debugfs\n",
+				EFI_DEBUGFS_MAX_BLOBS, EFI_DEBUGFS_MAX_BLOBS);
+			break;
+		}
+
+		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++;
+	}
+}
+#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
@@ -381,6 +435,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 7efd7072cca5..a6e1a2d8511e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1124,6 +1124,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #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_MEM_NO_SOFT_RESERVE	11	/* Is the kernel configured to ignore soft reservations? */
+#define EFI_PRESERVE_BS_REGIONS	12	/* Are EFI boot-services memory segments available? */
 
 #ifdef CONFIG_EFI
 /*
-- 
2.24.1

[PATCH v11 02/10] 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 makes the found firmware available through the new
efi_get_embedded_fw() function.

Support for loading these firmwares through the standard firmware loading
mechanism is added in a follow-up commit in this patch-series.

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 v9:
- At least touchscreen_dmi.c uses the same dmi_table for its own private
  data and the fw_desc structs, putting the fw_desc struct first in the
  data driver_data points to so that the dmi_table can be shared with
  efi_check_for_embedded_firmwares(). But not all entries there have
  embedded-fw so in some cases the fw_desc is empty (zero-ed out).
  This can lead to a possible crash because fw_desc->length now is
  less then 8, so if the segment size is close enough to a multiple of the
  page_size, then the memcmp to check the prefix my segfault. Crashing the
  machine. v9 checks for and skips these empty fw_desc entries avoiding this.
- Rename found_fw_list to efi_embedded_fw_list and export it for use by
  lib/test_firmware.c

Changes in v8:
- Properly deal with the (theoretical?) case of an EFI segment being
  smaller then the fw we are looking for
- Log a warning when efi_get_embedded_fw get called while we did not (yet)
  check for embedded firmwares

Changes in v7:
- Split drivers/firmware/efi and drivers/base/firmware_loader changes into
  2 patches
- Use new, standalone, lib/crypto/sha256.c code

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 UEFI 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
---
 arch/x86/platform/efi/efi.c              |   1 +
 drivers/firmware/efi/Kconfig             |   5 +
 drivers/firmware/efi/Makefile            |   1 +
 drivers/firmware/efi/embedded-firmware.c | 156 +++++++++++++++++++++++
 include/linux/efi.h                      |   6 +
 include/linux/efi_embedded_fw.h          |  39 ++++++
 6 files changed, 208 insertions(+)
 create mode 100644 drivers/firmware/efi/embedded-firmware.c
 create mode 100644 include/linux/efi_embedded_fw.h

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 51ee2e2a18d6..cc10a925d9e7 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -944,6 +944,7 @@ static void __init __efi_enter_virtual_mode(void)
 		goto err;
 	}
 
+	efi_check_for_embedded_firmwares();
 	efi_free_boot_services();
 
 	/*
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index ecc83e2f032c..613828d3f106 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -239,6 +239,11 @@ config EFI_DISABLE_PCI_DMA
 
 endmenu
 
+config EFI_EMBEDDED_FIRMWARE
+	bool
+	depends on EFI
+	select CRYPTO_LIB_SHA256
+
 config UEFI_CPER
 	bool
 
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 554d795270d9..256d6121b2b3 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -26,6 +26,7 @@ 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_RCI2_TABLE)		+= rci2-table.o
+obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE)	+= embedded-firmware.o
 
 fake_map-y				+= fake_mem.o
 fake_map-$(CONFIG_X86)			+= x86_fake_mem.o
diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
new file mode 100644
index 000000000000..6668ad48133f
--- /dev/null
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -0,0 +1,156 @@
+// 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 <linux/dmi.h>
+#include <linux/efi.h>
+#include <linux/efi_embedded_fw.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/vmalloc.h>
+#include <crypto/sha.h>
+
+/* Exported for use by lib/test_firmware.c only */
+LIST_HEAD(efi_embedded_fw_list);
+EXPORT_SYMBOL_GPL(efi_embedded_fw_list);
+
+static bool checked_for_fw;
+
+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 efi_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;
+	}
+
+	for (i = 0; (i + desc->length) <= size; i += 8) {
+		u64 *mem = map + i;
+
+		if (*mem != prefix)
+			continue;
+
+		sha256_init(&sctx);
+		sha256_update(&sctx, map + i, desc->length);
+		sha256_final(&sctx, sha256);
+		if (memcmp(sha256, desc->sha256, 32) == 0)
+			break;
+	}
+	if ((i + desc->length) > 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, &efi_embedded_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;
+
+		/*
+		 * In some drivers the struct driver_data contains may contain
+		 * other driver specific data after the fw_desc struct; and
+		 * the fw_desc struct itself may be empty, skip these.
+		 */
+		if (!fw_desc->name)
+			continue;
+
+		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;
+		}
+	}
+
+	checked_for_fw = true;
+}
+
+int efi_get_embedded_fw(const char *name, void **data, size_t *size)
+{
+	struct efi_embedded_fw *iter, *fw = NULL;
+	void *buf = *data;
+
+	if (!checked_for_fw) {
+		pr_warn("Warning %s called while we did not check for embedded fw\n",
+			__func__);
+		return -ENOENT;
+	}
+
+	list_for_each_entry(iter, &efi_embedded_fw_list, list) {
+		if (strcmp(name, iter->name) == 0) {
+			fw = iter;
+			break;
+		}
+	}
+
+	if (!fw)
+		return -ENOENT;
+
+	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 a6e1a2d8511e..23392b88bcc0 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1554,6 +1554,12 @@ static inline void
 efi_enable_reset_attack_mitigation(void) { }
 #endif
 
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+void efi_check_for_embedded_firmwares(void);
+#else
+static inline void efi_check_for_embedded_firmwares(void) { }
+#endif
+
 efi_status_t efi_random_get_seed(void);
 
 void efi_retrieve_tpm2_eventlog(void);
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
new file mode 100644
index 000000000000..5a8ae662911b
--- /dev/null
+++ b/include/linux/efi_embedded_fw.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_EFI_EMBEDDED_FW_H
+#define _LINUX_EFI_EMBEDDED_FW_H
+
+#include <linux/list.h>
+#include <linux/mod_devicetable.h>
+
+/*
+ * This struct and efi_embedded_fw_list are private to the efi-embedded fw
+ * implementation they are in this header for use by lib/test_firmware.c only!
+ */
+struct efi_embedded_fw {
+	struct list_head list;
+	const char *name;
+	void *data;
+	size_t length;
+};
+
+extern struct list_head efi_embedded_fw_list;
+
+/**
+ * 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);
+
+#endif
-- 
2.24.1

[PATCH v11 03/10] firmware: Rename FW_OPT_NOFALLBACK to FW_OPT_NOFALLBACK_SYSFS

[Hans de Goede]

This is a preparation patch for adding a new platform fallback mechanism,
which will have its own enable/disable FW_OPT_xxx option.

Note this also fixes a typo in one of the re-wordwrapped comments:
enfoce -> enforce.

Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/base/firmware_loader/fallback.c | 11 ++++++-----
 drivers/base/firmware_loader/firmware.h | 16 ++++++++--------
 drivers/base/firmware_loader/main.c     |  2 +-
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 62ee90b4db56..8704e1bae175 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -606,7 +606,7 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
 		return false;
 	}
 
-	if ((opt_flags & FW_OPT_NOFALLBACK))
+	if ((opt_flags & FW_OPT_NOFALLBACK_SYSFS))
 		return false;
 
 	/* Also permit LSMs and IMA to fail firmware sysfs fallback */
@@ -630,10 +630,11 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
  * interface. Userspace is in charge of loading the firmware through the sysfs
  * loading interface. This sysfs fallback mechanism may be disabled completely
  * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
- * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK
- * flag, if so it would also disable the fallback mechanism. A system may want
- * to enfoce the sysfs fallback mechanism at all times, it can do this by
- * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true.
+ * If this is false we check if the internal API caller set the
+ * @FW_OPT_NOFALLBACK_SYSFS flag, if so it would also disable the fallback
+ * mechanism. A system may want to enforce the sysfs fallback mechanism at all
+ * times, it can do this by setting ignore_sysfs_fallback to false and
+ * force_sysfs_fallback to true.
  * Enabling force_sysfs_fallback is functionally equivalent to build a kernel
  * with CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
  **/
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 7ecd590e67fe..8656e5239a80 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -27,16 +27,16 @@
  *	firmware file lookup on storage is avoided. Used for calls where the
  *	file may be too big, or where the driver takes charge of its own
  *	firmware caching mechanism.
- * @FW_OPT_NOFALLBACK: Disable the fallback mechanism. Takes precedence over
- *	&FW_OPT_UEVENT and &FW_OPT_USERHELPER.
+ * @FW_OPT_NOFALLBACK_SYSFS: Disable the sysfs fallback mechanism. Takes
+ *	precedence over &FW_OPT_UEVENT and &FW_OPT_USERHELPER.
  */
 enum fw_opt {
-	FW_OPT_UEVENT =         BIT(0),
-	FW_OPT_NOWAIT =         BIT(1),
-	FW_OPT_USERHELPER =     BIT(2),
-	FW_OPT_NO_WARN =        BIT(3),
-	FW_OPT_NOCACHE =        BIT(4),
-	FW_OPT_NOFALLBACK =     BIT(5),
+	FW_OPT_UEVENT			= BIT(0),
+	FW_OPT_NOWAIT			= BIT(1),
+	FW_OPT_USERHELPER		= BIT(2),
+	FW_OPT_NO_WARN			= BIT(3),
+	FW_OPT_NOCACHE			= BIT(4),
+	FW_OPT_NOFALLBACK_SYSFS		= BIT(5),
 };
 
 enum fw_status {
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 249add8c5e05..57133a9dad09 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -877,7 +877,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
 				FW_OPT_UEVENT | FW_OPT_NO_WARN |
-				FW_OPT_NOFALLBACK);
+				FW_OPT_NOFALLBACK_SYSFS);
 	module_put(THIS_MODULE);
 	return ret;
 }
-- 
2.24.1

[PATCH v11 04/10] firmware: Add new platform fallback mechanism and firmware_request_platform()

[Hans de Goede]

In some cases the platform's main firmware (e.g. the UEFI fw) may contain
an embedded copy of device 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 a new platform fallback mechanism to the firmware loader
which will try to lookup a device fw copy embedded in the platform's main
firmware if direct filesystem lookup fails.

Drivers which need such embedded fw copies can enable this fallback
mechanism by using the new firmware_request_platform() function.

Note that for now this is only supported on EFI platforms and even on
these platforms firmware_fallback_platform() only works if
CONFIG_EFI_EMBEDDED_FIRMWARE is enabled (this gets selected by drivers
which need this), in all other cases firmware_fallback_platform() simply
always returns -ENOENT.

Reported-by: Dave Olsthoorn <dave@bewaar.me>
Suggested-by: Peter Jones <pjones@redhat.com>
Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v9:
- Add static inline wrapper for firmware_request_platform() to firmware.h,
  for when CONFIG_FW_LOADER is not set

Changes in v8:
- Only build fallback_platform.c if CONFIG_EFI_EMBEDDED_FIRMWARE is defined,
  otherwise make firmware_fallback_platform() a static inline stub
- Add documentation to Documentation/driver-api/firmware/fallback-mechanisms.rst
  on how the boot_service_code? files exported by EFI debugfs can be used to
  check if there is an embedded firmware and to get the embedded firmware
  length and sha256sum

Changes in v7:
- Split drivers/firmware/efi and drivers/base/firmware_loader changes into
  2 patches
- Address kdoc comments from Randy Dunlap
- Add new FW_OPT_FALLBACK_PLATFORM flag and firmware_request_platform()
  _request_firmware() wrapper, as requested by Luis R. Rodriguez
- Stop using "efi-embedded-firmware" device-property, now that drivers need to
  use the new firmware_request_platform() to enable fallback to a device fw
  copy embedded in the platform's main firmware, we no longer need a property
  on the device to trigger this behavior
- Use security_kernel_load_data instead of calling
  security_kernel_read_file with a NULL file pointer argument
- Move the docs to Documentation/driver-api/firmware/fallback-mechanisms.rst
- Document the new firmware_request_platform() function in
  Documentation/driver-api/firmware/request_firmware.rst

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 UEFI 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
---
 .../firmware/fallback-mechanisms.rst          | 103 ++++++++++++++++++
 .../driver-api/firmware/lookup-order.rst      |   2 +
 .../driver-api/firmware/request_firmware.rst  |   5 +
 drivers/base/firmware_loader/Makefile         |   1 +
 drivers/base/firmware_loader/fallback.h       |  10 ++
 .../base/firmware_loader/fallback_platform.c  |  29 +++++
 drivers/base/firmware_loader/firmware.h       |   4 +
 drivers/base/firmware_loader/main.c           |  27 +++++
 include/linux/firmware.h                      |   9 ++
 include/linux/fs.h                            |   1 +
 10 files changed, 191 insertions(+)
 create mode 100644 drivers/base/firmware_loader/fallback_platform.c

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index 8b041d0ab426..036383dad6d6 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -202,3 +202,106 @@ the following file:
 
 If you echo 0 into it means MAX_JIFFY_OFFSET will be used. The data type
 for the timeout is an int.
+
+EFI embedded firmware fallback mechanism
+========================================
+
+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.
+
+Device drivers which need such firmware can use the
+firmware_request_platform() function for this, note that this is a
+separate fallback mechanism from the other fallback mechanisms and
+this does not use the sysfs interface.
+
+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 sha256 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 firmware_request_platform() 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/firmware.
+
+Note that:
+
+1. The code scanning for EFI embedded-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
+   embedded-firmware.
+
+2. At the moment 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. At the moment 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 UEFI Platform
+   Initialization (PI) spec's Firmware Volume protocol. This has been rejected
+   because the FV Protocol relies on *internal* interfaces of the PI spec, and:
+   1. The PI spec does not define peripheral firmware at all
+   2. The internal interfaces of the PI spec do 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.
+
+Example how to check for and extract embedded firmware
+------------------------------------------------------
+
+To check for, for example Silead touchscreen controller embedded firmware,
+do the following:
+
+1. Boot the system with efi=debug on the kernel commandline
+
+2. cp /sys/kernel/debug/efi/boot_services_code? to your home dir
+
+3. Open the boot_services_code? files in a hex-editor, search for the
+   magic prefix for Silead firmware: F0 00 00 00 02 00 00 00, this gives you
+   the beginning address of the firmware inside the boot_services_code? file.
+
+4. The firmware has a specific pattern, it starts with a 8 byte page-address,
+   typically F0 00 00 00 02 00 00 00 for the first page followed by 32-bit
+   word-address + 32-bit value pairs. With the word-address incrementing 4
+   bytes (1 word) for each pair until a page is complete. A complete page is
+   followed by a new page-address, followed by more word + value pairs. This
+   leads to a very distinct pattern. Scroll down until this pattern stops,
+   this gives you the end of the firmware inside the boot_services_code? file.
+
+5. "dd if=boot_services_code? of=firmware bs=1 skip=<begin-addr> count=<len>"
+   will extract the firmware for you. Inspect the firmware file in a
+   hexeditor to make sure you got the dd parameters correct.
+
+6. Copy it to /lib/firmware under the expected name to test it.
+
+7. If the extracted firmware works, you can use the found info to fill an
+   efi_embedded_fw_desc struct to describe it, run "sha256sum firmware"
+   to get the sha256sum to put in the sha256 field.
diff --git a/Documentation/driver-api/firmware/lookup-order.rst b/Documentation/driver-api/firmware/lookup-order.rst
index 88c81739683c..6064672a782e 100644
--- a/Documentation/driver-api/firmware/lookup-order.rst
+++ b/Documentation/driver-api/firmware/lookup-order.rst
@@ -12,6 +12,8 @@ a driver issues a firmware API call.
   return it immediately
 * The ''Direct filesystem lookup'' is performed next, if found we
   return it immediately
+* The ''Platform firmware fallback'' is performed next, but only when
+  firmware_request_platform() is used, if found we return it immediately
 * If no firmware has been found and the fallback mechanism was enabled
   the sysfs interface is created. After this either a kobject uevent
   is issued or the custom firmware loading is relied upon for firmware
diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index f62bdcbfed5b..cd076462d235 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -25,6 +25,11 @@ firmware_request_nowarn
 .. kernel-doc:: drivers/base/firmware_loader/main.c
    :functions: firmware_request_nowarn
 
+firmware_request_platform
+-------------------------
+.. kernel-doc:: drivers/base/firmware_loader/main.c
+   :functions: firmware_request_platform
+
 request_firmware_direct
 -----------------------
 .. kernel-doc:: drivers/base/firmware_loader/main.c
diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
index 0b2dfa6259c9..e87843408fe6 100644
--- a/drivers/base/firmware_loader/Makefile
+++ b/drivers/base/firmware_loader/Makefile
@@ -5,5 +5,6 @@ obj-$(CONFIG_FW_LOADER_USER_HELPER) += 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_platform.o
 
 obj-y += builtin/
diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
index 21063503e4ea..06f4577733a8 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -66,4 +66,14 @@ static inline void unregister_sysfs_loader(void)
 }
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+int firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags);
+#else
+static inline int firmware_fallback_platform(struct fw_priv *fw_priv,
+					     enum fw_opt opt_flags)
+{
+	return -ENOENT;
+}
+#endif
+
 #endif /* __FIRMWARE_FALLBACK_H */
diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
new file mode 100644
index 000000000000..37746efaf8e3
--- /dev/null
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -0,0 +1,29 @@
+// 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 firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags)
+{
+	int rc;
+
+	if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
+		return -ENOENT;
+
+	rc = security_kernel_load_data(LOADING_FIRMWARE_EFI_EMBEDDED);
+	if (rc)
+		return rc;
+
+	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data,
+				 &fw_priv->size);
+	if (rc)
+		return rc; /* rc == -ENOENT when the fw was not found */
+
+	fw_state_done(fw_priv);
+	return 0;
+}
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 8656e5239a80..25836a6afc9f 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -29,6 +29,9 @@
  *	firmware caching mechanism.
  * @FW_OPT_NOFALLBACK_SYSFS: Disable the sysfs fallback mechanism. Takes
  *	precedence over &FW_OPT_UEVENT and &FW_OPT_USERHELPER.
+ * @FW_OPT_FALLBACK_PLATFORM: Enable fallback to device fw copy embedded in
+ *	the platform's main firmware. If both this fallback and the sysfs
+ *      fallback are enabled, then this fallback will be tried first.
  */
 enum fw_opt {
 	FW_OPT_UEVENT			= BIT(0),
@@ -37,6 +40,7 @@ enum fw_opt {
 	FW_OPT_NO_WARN			= BIT(3),
 	FW_OPT_NOCACHE			= BIT(4),
 	FW_OPT_NOFALLBACK_SYSFS		= BIT(5),
+	FW_OPT_FALLBACK_PLATFORM	= BIT(6),
 };
 
 enum fw_status {
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 57133a9dad09..cfdbf0010545 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -776,6 +776,9 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 						 fw_decompress_xz);
 #endif
 
+	if (ret == -ENOENT)
+		ret = firmware_fallback_platform(fw->priv, opt_flags);
+
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
@@ -883,6 +886,30 @@ int request_firmware_direct(const struct firmware **firmware_p,
 }
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
+/**
+ * firmware_request_platform() - request firmware with platform-fw fallback
+ * @firmware: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware, except that if
+ * direct filesystem lookup fails, it will fallback to looking for a copy of the
+ * requested firmware embedded in the platform's main (e.g. UEFI) firmware.
+ **/
+int firmware_request_platform(const struct firmware **firmware,
+			      const char *name, struct device *device)
+{
+	int ret;
+
+	/* Need to pin this module until return */
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware, name, device, NULL, 0,
+				FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(firmware_request_platform);
+
 /**
  * firmware_request_cache() - cache firmware for suspend so resume can use it
  * @name: name of firmware file
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 2dd566c91d44..4bbd0afd91b7 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -44,6 +44,8 @@ int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
 int firmware_request_nowarn(const struct firmware **fw, const char *name,
 			    struct device *device);
+int firmware_request_platform(const struct firmware **fw, const char *name,
+			      struct device *device);
 int request_firmware_nowait(
 	struct module *module, bool uevent,
 	const char *name, struct device *device, gfp_t gfp, void *context,
@@ -69,6 +71,13 @@ static inline int firmware_request_nowarn(const struct firmware **fw,
 	return -EINVAL;
 }
 
+static inline int firmware_request_platform(const struct firmware **fw,
+					    const char *name,
+					    struct device *device)
+{
+	return -EINVAL;
+}
+
 static inline int request_firmware_nowait(
 	struct module *module, bool uevent,
 	const char *name, struct device *device, gfp_t gfp, void *context,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98e0349adb52..4b4f11a80a2c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2964,6 +2964,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)	\
-- 
2.24.1

[PATCH v11 05/10] test_firmware: add support for firmware_request_platform

[Hans de Goede]

Add support for testing firmware_request_platform through a new
trigger_request_platform trigger.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v11:
- Drop a few empty lines which were accidentally introduced

Changes in v10:
- New patch in v10 of this patch-set
---
 lib/test_firmware.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 251213c872b5..6042840f861c 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -24,6 +24,7 @@
 #include <linux/delay.h>
 #include <linux/kthread.h>
 #include <linux/vmalloc.h>
+#include <linux/efi_embedded_fw.h>
 
 #define TEST_FIRMWARE_NAME	"test-firmware.bin"
 #define TEST_FIRMWARE_NUM_REQS	4
@@ -507,6 +508,61 @@ static ssize_t trigger_request_store(struct device *dev,
 }
 static DEVICE_ATTR_WO(trigger_request);
 
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+static ssize_t trigger_request_platform_store(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t count)
+{
+	static const u8 test_data[] = {
+		0x55, 0xaa, 0x55, 0xaa, 0x01, 0x02, 0x03, 0x04,
+		0x55, 0xaa, 0x55, 0xaa, 0x05, 0x06, 0x07, 0x08,
+		0x55, 0xaa, 0x55, 0xaa, 0x10, 0x20, 0x30, 0x40,
+		0x55, 0xaa, 0x55, 0xaa, 0x50, 0x60, 0x70, 0x80
+	};
+	struct efi_embedded_fw fw;
+	int rc;
+	char *name;
+
+	name = kstrndup(buf, count, GFP_KERNEL);
+	if (!name)
+		return -ENOSPC;
+
+	pr_info("inserting test platform fw '%s'\n", name);
+	fw.name = name;
+	fw.data = (void *)test_data;
+	fw.length = sizeof(test_data);
+	list_add(&fw.list, &efi_embedded_fw_list);
+
+	pr_info("loading '%s'\n", name);
+
+	mutex_lock(&test_fw_mutex);
+	release_firmware(test_firmware);
+	test_firmware = NULL;
+	rc = firmware_request_platform(&test_firmware, name, dev);
+	if (rc) {
+		pr_info("load of '%s' failed: %d\n", name, rc);
+		goto out;
+	}
+	if (test_firmware->size != sizeof(test_data) ||
+	    memcmp(test_firmware->data, test_data, sizeof(test_data)) != 0) {
+		pr_info("firmware contents mismatch for '%s'\n", name);
+		rc = -EINVAL;
+		goto out;
+	}
+	pr_info("loaded: %zu\n", test_firmware->size);
+	rc = count;
+
+out:
+	mutex_unlock(&test_fw_mutex);
+
+	list_del(&fw.list);
+	kfree(name);
+
+	return rc;
+}
+static DEVICE_ATTR_WO(trigger_request_platform);
+#endif
+
 static DECLARE_COMPLETION(async_fw_done);
 
 static void trigger_async_request_cb(const struct firmware *fw, void *context)
@@ -903,6 +959,9 @@ static struct attribute *test_dev_attrs[] = {
 	TEST_FW_DEV_ATTR(trigger_request),
 	TEST_FW_DEV_ATTR(trigger_async_request),
 	TEST_FW_DEV_ATTR(trigger_custom_fallback),
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+	TEST_FW_DEV_ATTR(trigger_request_platform),
+#endif
 
 	/* These use the config and can use the test_result */
 	TEST_FW_DEV_ATTR(trigger_batched_requests),
-- 
2.24.1

[PATCH v11 06/10] selftests: firmware: Add firmware_request_platform tests

[Hans de Goede]

Add tests cases for checking the new firmware_request_platform api.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../selftests/firmware/fw_filesystem.sh       | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index 56894477c8bd..fcc281373b4d 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -86,6 +86,29 @@ else
 	fi
 fi
 
+# Try platform (EFI embedded fw) loading too
+if [ ! -e "$DIR"/trigger_request_platform ]; then
+	echo "$0: firmware loading: platform trigger not present, ignoring test" >&2
+else
+	if printf '\000' >"$DIR"/trigger_request_platform 2> /dev/null; then
+		echo "$0: empty filename should not succeed (platform)" >&2
+		exit 1
+	fi
+
+	# Note we echo a non-existing name, since files on the file-system
+	# are preferred over firmware embedded inside the platform's firmware
+	# The test adds a fake entry with the requested name to the platform's
+	# fw list, so the name does not matter as long as it does not exist
+	if ! echo -n "nope-$NAME" >"$DIR"/trigger_request_platform ; then
+		echo "$0: could not trigger request platform" >&2
+		exit 1
+	fi
+
+	# The test verifies itself that the loaded firmware contents matches
+	# the contents for the fake platform fw entry it added.
+	echo "$0: platform loading works"
+fi
+
 ### Batched requests tests
 test_config_present()
 {
-- 
2.24.1

[PATCH v11 07/10] Input: silead - Switch to firmware_request_platform for retreiving the fw

[Hans de Goede]

Unfortunately sofar we have been unable to get permission to redistribute
Silead touchscreen firmwares in linux-firmware. This means that people
need to find and install the firmware themselves before the touchscreen
will work

Some UEFI/x86 tablets with a Silead touchscreen have a copy of the fw
embedded in their UEFI boot-services code.

This commit makes the silead driver use the new firmware_request_platform
function, which will fallback to looking for such an embedded copy when
direct filesystem lookup fails. This will make the touchscreen work OOTB
on devices where there is a fw copy embedded in the UEFI code.

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/silead.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index ad8b6a2bfd36..8fa2f3b7cfd8 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -288,7 +288,7 @@ static int silead_ts_load_fw(struct i2c_client *client)
 
 	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
 
-	error = request_firmware(&fw, data->fw_name, dev);
+	error = firmware_request_platform(&fw, data->fw_name, dev);
 	if (error) {
 		dev_err(dev, "Firmware request error %d\n", error);
 		return error;
-- 
2.24.1

[PATCH v11 08/10] Input: icn8505 - Switch to firmware_request_platform for retreiving the fw

[Hans de Goede]

Unfortunately sofar we have been unable to get permission to redistribute
icn8505 touchscreen firmwares in linux-firmware. This means that people
need to find and install the firmware themselves before the touchscreen
will work

Some UEFI/x86 tablets with an icn8505 touchscreen have a copy of the fw
embedded in their UEFI boot-services code.

This commit makes the icn8505 driver use the new firmware_request_platform
function, which will fallback to looking for such an embedded copy when
direct filesystem lookup fails. This will make the touchscreen work OOTB
on devices where there is a fw copy embedded in the UEFI code.

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/chipone_icn8505.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c
index c768186ce856..f9ca5502ac8c 100644
--- a/drivers/input/touchscreen/chipone_icn8505.c
+++ b/drivers/input/touchscreen/chipone_icn8505.c
@@ -288,7 +288,7 @@ static int icn8505_upload_fw(struct icn8505_data *icn8505)
 	 * we may need it at resume. Having loaded it once will make the
 	 * firmware class code cache it at suspend/resume.
 	 */
-	error = request_firmware(&fw, icn8505->firmware_name, dev);
+	error = firmware_request_platform(&fw, icn8505->firmware_name, dev);
 	if (error) {
 		dev_err(dev, "Firmware request error %d\n", error);
 		return error;
-- 
2.24.1

[PATCH v11 09/10] 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 v7:
- Remove adding of PROPERTY_ENTRY_BOOL("efi-embedded-firmware"), to touchscreen
  props, as this is no longer necessary

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   | 41 +++++++++++++++++++++++-
 include/linux/efi_embedded_fw.h          |  2 ++
 4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
index 6668ad48133f..9592c6d12840 100644
--- a/drivers/firmware/efi/embedded-firmware.c
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -21,6 +21,9 @@ EXPORT_SYMBOL_GPL(efi_embedded_fw_list);
 static bool checked_for_fw;
 
 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 27d5b40fb717..a65f4ffb289a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1253,6 +1253,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 72205771d03d..4449e4c0b26b 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -11,12 +11,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;
 };
@@ -64,6 +67,15 @@ static const struct property_entry chuwi_hi8_pro_props[] = {
 };
 
 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,
 };
@@ -181,6 +193,15 @@ static const struct property_entry cube_iwork8_air_props[] = {
 };
 
 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,
 };
@@ -390,6 +411,15 @@ static const struct property_entry onda_v80_plus_v3_props[] = {
 };
 
 static const struct ts_dmi_data onda_v80_plus_v3_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl3676-onda-v80-plus-v3.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 37224,
+		.sha256	= { 0x8f, 0xbd, 0x8f, 0x0c, 0x6b, 0xba, 0x5b, 0xf5,
+			    0xa3, 0xc7, 0xa3, 0xc0, 0x4f, 0xcd, 0xdf, 0x32,
+			    0xcc, 0xe4, 0x70, 0xd6, 0x46, 0x9c, 0xd7, 0xa7,
+			    0x4b, 0x82, 0x3f, 0xab, 0xc7, 0x90, 0xea, 0x23 },
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= onda_v80_plus_v3_props,
 };
@@ -456,6 +486,15 @@ static const struct property_entry pipo_w2s_props[] = {
 };
 
 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,
 };
@@ -642,7 +681,7 @@ static const struct ts_dmi_data trekstor_surftab_wintron70_data = {
 };
 
 /* NOTE: Please keep this table sorted alphabetically */
-static const struct dmi_system_id touchscreen_dmi_table[] = {
+const struct dmi_system_id touchscreen_dmi_table[] = {
 	{
 		/* Chuwi Hi8 */
 		.driver_data = (void *)&chuwi_hi8_data,
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
index 5a8ae662911b..c4737fd07e33 100644
--- a/include/linux/efi_embedded_fw.h
+++ b/include/linux/efi_embedded_fw.h
@@ -34,6 +34,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);
 
 #endif
-- 
2.24.1

[PATCH v11 10/10] 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 v7:
- Remove PROPERTY_ENTRY_BOOL("efi-embedded-firmware") properties entry,
  as this is no longer necessary

Changes in v6:
- Switch from crc sums to SHA256 hashes for the firmware hash
---
 drivers/platform/x86/touchscreen_dmi.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 4449e4c0b26b..4a09b479cda5 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -132,6 +132,18 @@ static const struct ts_dmi_data chuwi_vi8_data = {
 	.properties     = chuwi_vi8_props,
 };
 
+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 },
+	},
+};
+
 static const struct property_entry chuwi_vi10_props[] = {
 	PROPERTY_ENTRY_U32("touchscreen-min-x", 0),
 	PROPERTY_ENTRY_U32("touchscreen-min-y", 4),
@@ -743,6 +755,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 			DMI_MATCH(DMI_BIOS_VERSION, "CHUWI.D86JLBNR"),
 		},
 	},
+	{
+		/* Chuwi Vi8 Plus (CWI519) */
+		.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"),
+		},
+	},
 	{
 		/* Chuwi Vi10 (CWI505) */
 		.driver_data = (void *)&chuwi_vi10_data,
@@ -1137,6 +1158,9 @@ static int __init ts_dmi_init(void)
 		return 0; /* Not an error */
 
 	ts_data = dmi_id->driver_data;
+	/* Some dmi table entries only provide an efi_embedded_fw_desc */
+	if (!ts_data->properties)
+		return 0;
 
 	error = bus_register_notifier(&i2c_bus_type, &ts_dmi_notifier);
 	if (error)
-- 
2.24.1

Re: [PATCH v11 02/10] efi: Add embedded peripheral firmware support

[Andy Lutomirski]

On Sat, Jan 11, 2020 at 6:57 AM Hans de Goede <hdegoede@redhat.com> 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 makes the found firmware available through the new
> efi_get_embedded_fw() function.
>
> Support for loading these firmwares through the standard firmware loading
> mechanism is added in a follow-up commit in this patch-series.
>
> 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.
>

A couple general comments:

How does this interact with modules?  It looks like you're referencing
the dmi table from otherwise modular or potentially modular code in
early EFI code, which seems like it will either prevent modular builds
or will actually fail at compile time depending on config.  Perhaps
you should have a single collection of EFI firmware references in a
separate place in the kernel tree and reference *that* from the EFI
code.

In the event you have many DMI matches (e.g. if anyone ends up using
this in a case where the DMI match has to match very broad things),
you'll iterate over the EFI code and data multiple times and
performance will suck.  It would be much better to iterate once and
search for everything.

I suspect that a rolling hash would be better than the prefix you're
using, but this could be changed later, assuming someone can actually
find all the firmware needed.

> +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 efi_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;
> +       }
> +
> +       for (i = 0; (i + desc->length) <= size; i += 8) {
> +               u64 *mem = map + i;
> +
> +               if (*mem != prefix)
> +                       continue;

This is very ugly.  You're casting a pointer to a bunch of bytes to
u64* and then dereferencing it like it's an integer.  This has major
endian issues with are offset by the similar endianness issues when
you type-pun prefix to u64.  You should instead just memcmp the
pointer with the data.  This will get rid of all the type punning in
this function.  You will also fail to detect firmware that isn't
8-byte-aligned.

So perhaps just use memmem() to replace this whole mess?

> +
> +               sha256_init(&sctx);
> +               sha256_update(&sctx, map + i, desc->length);
> +               sha256_final(&sctx, sha256);
> +               if (memcmp(sha256, desc->sha256, 32) == 0)
> +                       break;
> +       }
> +       if ((i + desc->length) > size) {
> +               memunmap(map);
> +               return -ENOENT;
> +       }
> +
> +       pr_info("Found EFI embedded fw '%s'\n", desc->name);
> +

It might be nice to also log which EFI section it was in?

> +       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, &efi_embedded_fw_list);
> +

If you actually copy the firmware name instead of just a pointer to
it, then you could potentially free the list of EFI firmwares.

Why are you copying the firmware into linear (kmemdup) memory here
just to copy it to vmalloc space down below...

> +       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;
> +
> +               /*
> +                * In some drivers the struct driver_data contains may contain
> +                * other driver specific data after the fw_desc struct; and
> +                * the fw_desc struct itself may be empty, skip these.
> +                */
> +               if (!fw_desc->name)
> +                       continue;
> +
> +               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;
> +               }
> +       }
> +
> +       checked_for_fw = true;
> +}

Have you measured how long this takes on a typical system per matching DMI id?

> +
> +int efi_get_embedded_fw(const char *name, void **data, size_t *size)
> +{
> +       struct efi_embedded_fw *iter, *fw = NULL;
> +       void *buf = *data;
> +
> +       if (!checked_for_fw) {

WARN_ON_ONCE?  A stack dump would be quite nice here.

> +       buf = vmalloc(fw->length);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       memcpy(buf, fw->data, fw->length);
> +       *size = fw->length;
> +       *data = buf;

See above.  What's vmalloc() for?  Where's the vfree()?

BTW, it would be very nice to have a straightforward way
(/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares.

Re: [PATCH v11 04/10] firmware: Add new platform fallback mechanism and firmware_request_platform()

[Andy Lutomirski]

On Sat, Jan 11, 2020 at 6:57 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> In some cases the platform's main firmware (e.g. the UEFI fw) may contain
> an embedded copy of device 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 a new platform fallback mechanism to the firmware loader
> which will try to lookup a device fw copy embedded in the platform's main
> firmware if direct filesystem lookup fails.
>
> Drivers which need such embedded fw copies can enable this fallback
> mechanism by using the new firmware_request_platform() function.

After finally wrapping my head around how this all fits together:

Early in boot, you check a bunch of firmware descriptors, bundled with
drivers, to search EFI code and data for firmware before you free said
code and data.  You catalogue it by name.  Later on, you use this list
as a fallback, again catalogued by name.  I think it would be rather
nicer if you simply had a list in a single file containing all these
descriptors rather than commingling it with the driver's internal dmi
data.  This gets rid of all the ifdeffery and module issues.  It also
avoids any potential nastiness when you have a dmi entry that contains
driver_data that points into the driver when the driver is a module.

And you can mark the entire list __initdata.  And you can easily (now
or later on) invert the code flow so you map each EFI region exactly
once and then search for all the firmware in it.

--Andy

Re: [PATCH v11 05/10] test_firmware: add support for firmware_request_platform

[Luis Chamberlain]

On Sat, Jan 11, 2020 at 03:56:58PM +0100, Hans de Goede wrote:
> Add support for testing firmware_request_platform through a new
> trigger_request_platform trigger.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v11:
> - Drop a few empty lines which were accidentally introduced

But you didn't address my other feedback.

> --- a/lib/test_firmware.c
> +++ b/lib/test_firmware.c
> @@ -507,6 +508,61 @@ static ssize_t trigger_request_store(struct device *dev,
>  }
>  static DEVICE_ATTR_WO(trigger_request);
>  
> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> +static ssize_t trigger_request_platform_store(struct device *dev,
> +					      struct device_attribute *attr,
> +					      const char *buf, size_t count)
> +{
> +	static const u8 test_data[] = {
> +		0x55, 0xaa, 0x55, 0xaa, 0x01, 0x02, 0x03, 0x04,
> +		0x55, 0xaa, 0x55, 0xaa, 0x05, 0x06, 0x07, 0x08,
> +		0x55, 0xaa, 0x55, 0xaa, 0x10, 0x20, 0x30, 0x40,
> +		0x55, 0xaa, 0x55, 0xaa, 0x50, 0x60, 0x70, 0x80
> +	};
> +	struct efi_embedded_fw fw;
> +	int rc;
> +	char *name;
> +
> +	name = kstrndup(buf, count, GFP_KERNEL);
> +	if (!name)
> +		return -ENOSPC;
> +
> +	pr_info("inserting test platform fw '%s'\n", name);
> +	fw.name = name;
> +	fw.data = (void *)test_data;
> +	fw.length = sizeof(test_data);
> +	list_add(&fw.list, &efi_embedded_fw_list);
> +
> +	pr_info("loading '%s'\n", name);
> +

I mentioned this in my last review, and it seems you forgot to address
this. But now some more feedback:

These two:

> +	mutex_lock(&test_fw_mutex);
> +	release_firmware(test_firmware);

You are doing this because this is a test, but a typical driver will
do this after, and we don't loose anything in doing this after. Can you
move the mutex lock and assign the pointer to a temporary used pointer
for the call, *after* your call.

But since your test is not using any interfaces to query information
about the firmware, and you are just doing the test in C code right
away, instead of say, using a trigger for later use in userspace,
you can just do away with the mutex lock and make the call use its
own pointer:

	rc = firmware_request_platform(&tmp_test_firmware, name, dev);
	if (rc) {
		...
	}
	/* Your test branch code goes here */

I see no reason why you use the test_firmware pointer.

> +	test_firmware = NULL;
> +	rc = firmware_request_platform(&test_firmware, name, dev);
> +	if (rc) {
> +		pr_info("load of '%s' failed: %d\n", name, rc);
> +		goto out;
> +	}
> +	if (test_firmware->size != sizeof(test_data) ||
> +	    memcmp(test_firmware->data, test_data, sizeof(test_data)) != 0) {
> +		pr_info("firmware contents mismatch for '%s'\n", name);
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +	pr_info("loaded: %zu\n", test_firmware->size);
> +	rc = count;
> +
> +out:
> +	mutex_unlock(&test_fw_mutex);
> +
> +	list_del(&fw.list);
> +	kfree(name);
> +
> +	return rc;
> +}

Re: [PATCH v11 05/10] test_firmware: add support for firmware_request_platform

[Hans de Goede]

Hi,

On 13-01-2020 15:53, Luis Chamberlain wrote:
> On Sat, Jan 11, 2020 at 03:56:58PM +0100, Hans de Goede wrote:
>> Add support for testing firmware_request_platform through a new
>> trigger_request_platform trigger.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v11:
>> - Drop a few empty lines which were accidentally introduced
> 
> But you didn't address my other feedback.
> 
>> --- a/lib/test_firmware.c
>> +++ b/lib/test_firmware.c
>> @@ -507,6 +508,61 @@ static ssize_t trigger_request_store(struct device *dev,
>>   }
>>   static DEVICE_ATTR_WO(trigger_request);
>>   
>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>> +static ssize_t trigger_request_platform_store(struct device *dev,
>> +					      struct device_attribute *attr,
>> +					      const char *buf, size_t count)
>> +{
>> +	static const u8 test_data[] = {
>> +		0x55, 0xaa, 0x55, 0xaa, 0x01, 0x02, 0x03, 0x04,
>> +		0x55, 0xaa, 0x55, 0xaa, 0x05, 0x06, 0x07, 0x08,
>> +		0x55, 0xaa, 0x55, 0xaa, 0x10, 0x20, 0x30, 0x40,
>> +		0x55, 0xaa, 0x55, 0xaa, 0x50, 0x60, 0x70, 0x80
>> +	};
>> +	struct efi_embedded_fw fw;
>> +	int rc;
>> +	char *name;
>> +
>> +	name = kstrndup(buf, count, GFP_KERNEL);
>> +	if (!name)
>> +		return -ENOSPC;
>> +
>> +	pr_info("inserting test platform fw '%s'\n", name);
>> +	fw.name = name;
>> +	fw.data = (void *)test_data;
>> +	fw.length = sizeof(test_data);
>> +	list_add(&fw.list, &efi_embedded_fw_list);
>> +
>> +	pr_info("loading '%s'\n", name);
>> +
> 
> I mentioned this in my last review, and it seems you forgot to address
> this.

I did address this in my reply to your review, as explained there,
the check + free on test_firmware before calling firmware_request_platform()
is necessary because test_firmware may be non NULL when entering
the function (continued below) ...

> But now some more feedback:
> 
> These two:
> 
>> +	mutex_lock(&test_fw_mutex);
>> +	release_firmware(test_firmware);
> 
> You are doing this because this is a test, but a typical driver will
> do this after, and we don't loose anything in doing this after. Can you
> move the mutex lock and assign the pointer to a temporary used pointer
> for the call, *after* your call.
> 
> But since your test is not using any interfaces to query information
> about the firmware, and you are just doing the test in C code right
> away, instead of say, using a trigger for later use in userspace,
> you can just do away with the mutex lock and make the call use its
> own pointer:
> 
> 	rc = firmware_request_platform(&tmp_test_firmware, name, dev);
> 	if (rc) {
> 		...
> 	}
> 	/* Your test branch code goes here */
> 
> I see no reason why you use the test_firmware pointer.

I agree that using a private/local firmware pointer instead of
test_firmware and dropping the mutex calls is better. I will make
this change for v12 of this series.

I'll send out a v12 once the remarks from Andy Lutomirski's
have also been discussed.

Regards,

Hans


> 
>> +	test_firmware = NULL;
>> +	rc = firmware_request_platform(&test_firmware, name, dev);
>> +	if (rc) {
>> +		pr_info("load of '%s' failed: %d\n", name, rc);
>> +		goto out;
>> +	}
>> +	if (test_firmware->size != sizeof(test_data) ||
>> +	    memcmp(test_firmware->data, test_data, sizeof(test_data)) != 0) {
>> +		pr_info("firmware contents mismatch for '%s'\n", name);
>> +		rc = -EINVAL;
>> +		goto out;
>> +	}
>> +	pr_info("loaded: %zu\n", test_firmware->size);
>> +	rc = count;
>> +
>> +out:
>> +	mutex_unlock(&test_fw_mutex);
>> +
>> +	list_del(&fw.list);
>> +	kfree(name);
>> +
>> +	return rc;
>> +}
> 

Re: [PATCH v11 05/10] test_firmware: add support for firmware_request_platform

[Luis Chamberlain]

On Mon, Jan 13, 2020 at 04:22:36PM +0100, Hans de Goede wrote:
> 
> test_firmware and dropping the mutex calls is better. I will make
> this change for v12 of this series.
> 
> I'll send out a v12 once the remarks from Andy Lutomirski's
> have also been discussed.

Sure, just think twice about loosing the ability to access the
test_firmware pointer from userspace. If you can find value
in extending your tests then keep it, otherwise if its just
to do the actual test in C in the call itself, it makes sense
to avoid it for that test case.

  Luis

Re: [PATCH v11 02/10] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi Andy,

Thank you for your feedback.

On 12-01-2020 23:45, Andy Lutomirski wrote:
> On Sat, Jan 11, 2020 at 6:57 AM Hans de Goede <hdegoede@redhat.com> 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 makes the found firmware available through the new
>> efi_get_embedded_fw() function.
>>
>> Support for loading these firmwares through the standard firmware loading
>> mechanism is added in a follow-up commit in this patch-series.
>>
>> 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.
>>
> 
> A couple general comments:
> 
> How does this interact with modules?  It looks like you're referencing
> the dmi table from otherwise modular or potentially modular code in
> early EFI code, which seems like it will either prevent modular builds
> or will actually fail at compile time depending on config.  Perhaps
> you should have a single collection of EFI firmware references in a
> separate place in the kernel tree and reference *that* from the EFI
> code.

You are right that this currently relies on the DMI tables
added to the embedded_fw_table[] being built in.

There currently really only is one (niche) group of X86 devices which
benefit from this patch-set. On some cheap X86 tablets and 2-in-1s
cheap touchscreen controllers are used which do not have their
(model specific) firmware in (p)ROM instead it needs to be loaded
by the touchscreen driver. We load the firmware using a model-specific
firmware filename through the standard firmware loading mechanism.
This is painful for end users of such devices, since they need to get
the firmware from somewhere (despite repeated tries we do not
have redistribution rights to put them in linux-firmware).

Dave Olsthoorn pointed out to me that on his device model the touchscreen
worked in Refind, so there is an EFI driver for it, so the firmware should
be somewhere in the EFI firmware addressspace. That resulted in this
patch-set which will make the touchscreen work OOTB on devices where
the firmware is embedded in the UEFI code somewhere.

So (for now) this new mechanism is only used by a small niche
group of devices.

The X86 devices which these touchscreens already need a bunch of
device model specific metadata, like the model specific firmware
filename and also the resolution of the touchscreen, orientation, etc.

We already have a "driver" which adds this info to the device
during enumeration using device-properties:
drivers/platform/x86/touchscreen_dmi.c

This code already is builtin as it needs to add a bus-notifier
before i2c bus enumeration to be able to add the device-properties
before the devices are proped (it uses an arch_initcall).

So here we already have a dmi_system_id table with matches for
all devices in our small niche group. The current mechanism
where a driver "registers" its dmi_system_id table in the
embedded_fw_table[] table is based on this use-case.

This avoids needing to duplicate the dmi-match strings in 2
places and allows us keeping all the info related to a
touchscreen in a single place, e.g. the entry for the Cube
iWorks 8 air looks like this:

static const struct property_entry cube_iwork8_air_props[] = {
         PROPERTY_ENTRY_U32("touchscreen-min-x", 1),
         PROPERTY_ENTRY_U32("touchscreen-min-y", 3),
         PROPERTY_ENTRY_U32("touchscreen-size-x", 1664),
         PROPERTY_ENTRY_U32("touchscreen-size-y", 896),
         PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
         PROPERTY_ENTRY_STRING("firmware-name", "gsl3670-cube-iwork8-air.fw"),
         PROPERTY_ENTRY_U32("silead,max-fingers", 10),
         { }
};

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,
};

And then the driver_data for the DMI match for this device
points to cube_iwork8_air_data.

For now doing it this way makes more sense IMHO then duplicating the
DMI matches inside drivers/firmware/efi/embedded-firmware.c and
putting the embedded_fw info there, where the rest of the info
lives in drivers/platform/x86/touchscreen_dmi.c.

This is all internal kernel API (not even exported to modules) so
if more users of the EFI-embedded-fw mechanism show up and this is a
poor fit for them, then we can re-evaluate this.

Also note that the drivers/platform/x86/touchscreen_dmi.c sees a number
of commits each kernel cycle, keeping the churn for adding touchscreen
info for new device models located to 1 file also is a good thing
about the current approach. So far the drivers/platform/x86 maintainers
have not complained about the churn being concentrated there...

> In the event you have many DMI matches (e.g. if anyone ends up using
> this in a case where the DMI match has to match very broad things),
> you'll iterate over the EFI code and data multiple times and
> performance will suck.  It would be much better to iterate once and
> search for everything.

As I said this is targeting a small niche group of X86 device models,
so this is very much optimized for there being zero DMI matches and
since all the touchscreen info is model specific we use narrow DMI
matches up to matching the exact BIOS version and/or date when the
other DMI info is too generic.

And since ATM touchscreen drivers are the only user there should
never be more then 1 DMI match. Note this is also assumed in
the code, it only calls dmi_first_match() once on each registered
dmi_system_id table and ATM we only have 0 or 1 registered
dmi_system_id tables.

Even if we get more use of this the chances of 1 device model using
more then 1 embedded fw are small. Where as if we first map the
EFI_BOOT_SERVICES_CODE segment before doing the DMI match then we
do this for all EFI_BOOT_SERVICES_CODE segments on all hardware.
The current implementation is very much optimized to do as little
work as possible when there is no DMI match, which will be true
on almost all devices out there.

> I suspect that a rolling hash would be better than the prefix you're
> using, but this could be changed later, assuming someone can actually
> find all the firmware needed.
> 
>> +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 efi_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;
>> +       }
>> +
>> +       for (i = 0; (i + desc->length) <= size; i += 8) {
>> +               u64 *mem = map + i;
>> +
>> +               if (*mem != prefix)
>> +                       continue;
> 
> This is very ugly.  You're casting a pointer to a bunch of bytes to
> u64* and then dereferencing it like it's an integer.  This has major
> endian issues with are offset by the similar endianness issues when
> you type-pun prefix to u64.  You should instead just memcmp the
> pointer with the data.  This will get rid of all the type punning in
> this function.

Ok, I will switch to memcmp for the next version.

> You will also fail to detect firmware that isn't
> 8-byte-aligned.

This is by design, currently being 8 byte aligned holds true for
all devices on which this is used, also see the kdoc added in
"[PATCH v11 04/10] firmware: Add new platform fallback mechanism and firmware_request_platform()"

Specifically:

+EFI embedded firmware fallback mechanism
+========================================
+
+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.
+
+Device drivers which need such firmware can use the
+firmware_request_platform() function for this, note that this is a
+separate fallback mechanism from the other fallback mechanisms and
+this does not use the sysfs interface.
+
+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 sha256 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 firmware_request_platform() 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/firmware.
+
+Note that:
+
+1. The code scanning for EFI embedded-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
+   embedded-firmware.
+
+2. At the moment 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.


Note this also documents your concern about the dmi_system_id
table needing to be builtin.

> So perhaps just use memmem() to replace this whole mess?

If we hit firmware which is not 8 byte aligned, then yes that would be
a good idea, but for now I feel that that would just cause a slowdown
in the scanning without any benefits.

>> +
>> +               sha256_init(&sctx);
>> +               sha256_update(&sctx, map + i, desc->length);
>> +               sha256_final(&sctx, sha256);
>> +               if (memcmp(sha256, desc->sha256, 32) == 0)
>> +                       break;
>> +       }
>> +       if ((i + desc->length) > size) {
>> +               memunmap(map);
>> +               return -ENOENT;
>> +       }
>> +
>> +       pr_info("Found EFI embedded fw '%s'\n", desc->name);
>> +
> 
> It might be nice to also log which EFI section it was in?

The EFI sections do not have names, so all I could is log
the section number which does not really feel useful?

>> +       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, &efi_embedded_fw_list);
>> +
> 
> If you actually copy the firmware name instead of just a pointer to
> it, then you could potentially free the list of EFI firmwares.

If we move to having a separate dmi_system_id table for this then
that would be true. ATM the dmi_system_id from
drivers/platform/x86/touchscreen_dmi.c
is not freed as it is referenced from a bus-notifier.

> Why are you copying the firmware into linear (kmemdup) memory here

The kmemdup is because the EFI_BOOT_SERVICES_CODE section is
free-ed almost immediately after we run.

> just to copy it to vmalloc space down below...

The vmalloc is because the efi_get_embedded_fw() function is
a helper for later implementing firmware_Request_platform
which returns a struct firmware *fw and release_firmware()
uses vfree() to free the firmware contents.

I guess we could have efi_get_embedded_fw() return an const u8 *
and have the firmware code do the vmalloc + memcpy but it boils
down to the same thing.


> 
>> +       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;
>> +
>> +               /*
>> +                * In some drivers the struct driver_data contains may contain
>> +                * other driver specific data after the fw_desc struct; and
>> +                * the fw_desc struct itself may be empty, skip these.
>> +                */
>> +               if (!fw_desc->name)
>> +                       continue;
>> +
>> +               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;
>> +               }
>> +       }
>> +
>> +       checked_for_fw = true;
>> +}
> 
> Have you measured how long this takes on a typical system per matching DMI id?

I originally wrote this approx. 18 months ago, it has been on hold for a while
since it needed a sha256 method which would work before subsys_initcall-s
run so I couldn't use the standard crypto_alloc_shash() stuff. In the end
I ended up merging the duplicate purgatory and crypto/sha256_generic.c
generic C SHA256 implementation into a set of new directly callable lib
functions in lib/crypto/sha256.c, just so that I could move forward with
this...

Anyways the reason for this background info is that because this is a while
ago I do not remember exactly how, but yes I did measure this (but not
very scientifically) and there was no discernible difference in boot
to init (from the initrd) with this in place vs without this.

>> +
>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size)
>> +{
>> +       struct efi_embedded_fw *iter, *fw = NULL;
>> +       void *buf = *data;
>> +
>> +       if (!checked_for_fw) {
> 
> WARN_ON_ONCE?  A stack dump would be quite nice here.

As discussed when this check was added (discussion in v7 of
the set I guess, check added in v8) we can also hit this without
it being a bug, e.g. when booted through kexec the whole
efi_check_for_embedded_firmwares() call we be skipped, hence the
pr_warn.


>> +       buf = vmalloc(fw->length);
>> +       if (!buf)
>> +               return -ENOMEM;
>> +
>> +       memcpy(buf, fw->data, fw->length);
>> +       *size = fw->length;
>> +       *data = buf;
> 
> See above.  What's vmalloc() for?  Where's the vfree()?

See above for my answer. I'm fine with moving this into the
firmware code, since that is where the matching vfree is, please
let me know how you want to proceed with this.

> BTW, it would be very nice to have a straightforward way
> (/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares.

That is an interesting idea, we could add a subsys_init call or
some such to add this to debugfs (efi_check_for_embedded_firmwares runs
too early).

But given how long this patch-set has been in the making I would really
like to get this (or at least the first 2 patches as a start) upstream,
so for now I would like to keep the changes to a minimum. Are you ok
with me adding the debugfs support with a follow-up patch ?

Let me also reply to your summary comment elsewhere in the thread here:

 > Early in boot, you check a bunch of firmware descriptors, bundled with
 > drivers, to search EFI code and data for firmware before you free said
 > code and data.  You catalogue it by name.  Later on, you use this list
 > as a fallback, again catalogued by name.  I think it would be rather
 > nicer if you simply had a list in a single file containing all these
 > descriptors rather than commingling it with the driver's internal dmi
 > data.  This gets rid of all the ifdeffery and module issues.  It also
 > avoids any potential nastiness when you have a dmi entry that contains
 > driver_data that points into the driver when the driver is a module.
 >
 > And you can mark the entire list __initdata.  And you can easily (now
 > or later on) invert the code flow so you map each EFI region exactly
 > once and then search for all the firmware in it.

I believe we have mostly discussed this above. At least for the
X86 touchscreen case, which is the only user of this for now, I
believe that re-using the table from drivers/platform/x86/touchscreen_dmi.c
is better as it avoids duplication of DMI strings and it keeps all
the info in one place (also avoiding churn in 2 files / 2 different
trees when new models are added).

I agree that when looking at this as a generic mechanism which may be
used elsewhere, your suggestion makes a lot of sense. But even though
this is very much written so that it can be used as a generic mechanism
I'm not sure if other users will appear. So for now, with only the X86
touchscreen use-case actually using this I believe the current
implementation is best, but I'm definitely open to changing this around
if more users show up.

Regards,

Hans

Re: [PATCH v11 02/10] efi: Add embedded peripheral firmware support

[Andy Shevchenko]

On Tue, Jan 14, 2020 at 2:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Andy,
>
> Thank you for your feedback.

Another Andy is here.
Sorry for the summary at the top of letter, I left below specifically
for Sakari to have a bit of context.

Sakari, do you know if IPU firmware is going to utilize something like
touchscreen devices?

Hans, I am completely in favour of more generic approach with less
quirks applied.
For now the burden on PDx86, from maintenance perspective, is not big,
we may survive.

>
> On 12-01-2020 23:45, Andy Lutomirski wrote:
> > On Sat, Jan 11, 2020 at 6:57 AM Hans de Goede <hdegoede@redhat.com> 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 makes the found firmware available through the new
> >> efi_get_embedded_fw() function.
> >>
> >> Support for loading these firmwares through the standard firmware loading
> >> mechanism is added in a follow-up commit in this patch-series.
> >>
> >> 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.
> >>
> >
> > A couple general comments:
> >
> > How does this interact with modules?  It looks like you're referencing
> > the dmi table from otherwise modular or potentially modular code in
> > early EFI code, which seems like it will either prevent modular builds
> > or will actually fail at compile time depending on config.  Perhaps
> > you should have a single collection of EFI firmware references in a
> > separate place in the kernel tree and reference *that* from the EFI
> > code.
>
> You are right that this currently relies on the DMI tables
> added to the embedded_fw_table[] being built in.
>
> There currently really only is one (niche) group of X86 devices which
> benefit from this patch-set. On some cheap X86 tablets and 2-in-1s
> cheap touchscreen controllers are used which do not have their
> (model specific) firmware in (p)ROM instead it needs to be loaded
> by the touchscreen driver. We load the firmware using a model-specific
> firmware filename through the standard firmware loading mechanism.
> This is painful for end users of such devices, since they need to get
> the firmware from somewhere (despite repeated tries we do not
> have redistribution rights to put them in linux-firmware).
>
> Dave Olsthoorn pointed out to me that on his device model the touchscreen
> worked in Refind, so there is an EFI driver for it, so the firmware should
> be somewhere in the EFI firmware addressspace. That resulted in this
> patch-set which will make the touchscreen work OOTB on devices where
> the firmware is embedded in the UEFI code somewhere.
>
> So (for now) this new mechanism is only used by a small niche
> group of devices.
>
> The X86 devices which these touchscreens already need a bunch of
> device model specific metadata, like the model specific firmware
> filename and also the resolution of the touchscreen, orientation, etc.
>
> We already have a "driver" which adds this info to the device
> during enumeration using device-properties:
> drivers/platform/x86/touchscreen_dmi.c
>
> This code already is builtin as it needs to add a bus-notifier
> before i2c bus enumeration to be able to add the device-properties
> before the devices are proped (it uses an arch_initcall).
>
> So here we already have a dmi_system_id table with matches for
> all devices in our small niche group. The current mechanism
> where a driver "registers" its dmi_system_id table in the
> embedded_fw_table[] table is based on this use-case.
>
> This avoids needing to duplicate the dmi-match strings in 2
> places and allows us keeping all the info related to a
> touchscreen in a single place, e.g. the entry for the Cube
> iWorks 8 air looks like this:
>
> static const struct property_entry cube_iwork8_air_props[] = {
>          PROPERTY_ENTRY_U32("touchscreen-min-x", 1),
>          PROPERTY_ENTRY_U32("touchscreen-min-y", 3),
>          PROPERTY_ENTRY_U32("touchscreen-size-x", 1664),
>          PROPERTY_ENTRY_U32("touchscreen-size-y", 896),
>          PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
>          PROPERTY_ENTRY_STRING("firmware-name", "gsl3670-cube-iwork8-air.fw"),
>          PROPERTY_ENTRY_U32("silead,max-fingers", 10),
>          { }
> };
>
> 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,
> };
>
> And then the driver_data for the DMI match for this device
> points to cube_iwork8_air_data.
>
> For now doing it this way makes more sense IMHO then duplicating the
> DMI matches inside drivers/firmware/efi/embedded-firmware.c and
> putting the embedded_fw info there, where the rest of the info
> lives in drivers/platform/x86/touchscreen_dmi.c.
>
> This is all internal kernel API (not even exported to modules) so
> if more users of the EFI-embedded-fw mechanism show up and this is a
> poor fit for them, then we can re-evaluate this.
>
> Also note that the drivers/platform/x86/touchscreen_dmi.c sees a number
> of commits each kernel cycle, keeping the churn for adding touchscreen
> info for new device models located to 1 file also is a good thing
> about the current approach. So far the drivers/platform/x86 maintainers
> have not complained about the churn being concentrated there...
>
> > In the event you have many DMI matches (e.g. if anyone ends up using
> > this in a case where the DMI match has to match very broad things),
> > you'll iterate over the EFI code and data multiple times and
> > performance will suck.  It would be much better to iterate once and
> > search for everything.
>
> As I said this is targeting a small niche group of X86 device models,
> so this is very much optimized for there being zero DMI matches and
> since all the touchscreen info is model specific we use narrow DMI
> matches up to matching the exact BIOS version and/or date when the
> other DMI info is too generic.
>
> And since ATM touchscreen drivers are the only user there should
> never be more then 1 DMI match. Note this is also assumed in
> the code, it only calls dmi_first_match() once on each registered
> dmi_system_id table and ATM we only have 0 or 1 registered
> dmi_system_id tables.
>
> Even if we get more use of this the chances of 1 device model using
> more then 1 embedded fw are small. Where as if we first map the
> EFI_BOOT_SERVICES_CODE segment before doing the DMI match then we
> do this for all EFI_BOOT_SERVICES_CODE segments on all hardware.
> The current implementation is very much optimized to do as little
> work as possible when there is no DMI match, which will be true
> on almost all devices out there.
>
> > I suspect that a rolling hash would be better than the prefix you're
> > using, but this could be changed later, assuming someone can actually
> > find all the firmware needed.
> >
> >> +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 efi_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;
> >> +       }
> >> +
> >> +       for (i = 0; (i + desc->length) <= size; i += 8) {
> >> +               u64 *mem = map + i;
> >> +
> >> +               if (*mem != prefix)
> >> +                       continue;
> >
> > This is very ugly.  You're casting a pointer to a bunch of bytes to
> > u64* and then dereferencing it like it's an integer.  This has major
> > endian issues with are offset by the similar endianness issues when
> > you type-pun prefix to u64.  You should instead just memcmp the
> > pointer with the data.  This will get rid of all the type punning in
> > this function.
>
> Ok, I will switch to memcmp for the next version.
>
> > You will also fail to detect firmware that isn't
> > 8-byte-aligned.
>
> This is by design, currently being 8 byte aligned holds true for
> all devices on which this is used, also see the kdoc added in
> "[PATCH v11 04/10] firmware: Add new platform fallback mechanism and firmware_request_platform()"
>
> Specifically:
>
> +EFI embedded firmware fallback mechanism
> +========================================
> +
> +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.
> +
> +Device drivers which need such firmware can use the
> +firmware_request_platform() function for this, note that this is a
> +separate fallback mechanism from the other fallback mechanisms and
> +this does not use the sysfs interface.
> +
> +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 sha256 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 firmware_request_platform() 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/firmware.
> +
> +Note that:
> +
> +1. The code scanning for EFI embedded-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
> +   embedded-firmware.
> +
> +2. At the moment 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.
>
>
> Note this also documents your concern about the dmi_system_id
> table needing to be builtin.
>
> > So perhaps just use memmem() to replace this whole mess?
>
> If we hit firmware which is not 8 byte aligned, then yes that would be
> a good idea, but for now I feel that that would just cause a slowdown
> in the scanning without any benefits.
>
> >> +
> >> +               sha256_init(&sctx);
> >> +               sha256_update(&sctx, map + i, desc->length);
> >> +               sha256_final(&sctx, sha256);
> >> +               if (memcmp(sha256, desc->sha256, 32) == 0)
> >> +                       break;
> >> +       }
> >> +       if ((i + desc->length) > size) {
> >> +               memunmap(map);
> >> +               return -ENOENT;
> >> +       }
> >> +
> >> +       pr_info("Found EFI embedded fw '%s'\n", desc->name);
> >> +
> >
> > It might be nice to also log which EFI section it was in?
>
> The EFI sections do not have names, so all I could is log
> the section number which does not really feel useful?
>
> >> +       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, &efi_embedded_fw_list);
> >> +
> >
> > If you actually copy the firmware name instead of just a pointer to
> > it, then you could potentially free the list of EFI firmwares.
>
> If we move to having a separate dmi_system_id table for this then
> that would be true. ATM the dmi_system_id from
> drivers/platform/x86/touchscreen_dmi.c
> is not freed as it is referenced from a bus-notifier.
>
> > Why are you copying the firmware into linear (kmemdup) memory here
>
> The kmemdup is because the EFI_BOOT_SERVICES_CODE section is
> free-ed almost immediately after we run.
>
> > just to copy it to vmalloc space down below...
>
> The vmalloc is because the efi_get_embedded_fw() function is
> a helper for later implementing firmware_Request_platform
> which returns a struct firmware *fw and release_firmware()
> uses vfree() to free the firmware contents.
>
> I guess we could have efi_get_embedded_fw() return an const u8 *
> and have the firmware code do the vmalloc + memcpy but it boils
> down to the same thing.
>
>
> >
> >> +       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;
> >> +
> >> +               /*
> >> +                * In some drivers the struct driver_data contains may contain
> >> +                * other driver specific data after the fw_desc struct; and
> >> +                * the fw_desc struct itself may be empty, skip these.
> >> +                */
> >> +               if (!fw_desc->name)
> >> +                       continue;
> >> +
> >> +               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;
> >> +               }
> >> +       }
> >> +
> >> +       checked_for_fw = true;
> >> +}
> >
> > Have you measured how long this takes on a typical system per matching DMI id?
>
> I originally wrote this approx. 18 months ago, it has been on hold for a while
> since it needed a sha256 method which would work before subsys_initcall-s
> run so I couldn't use the standard crypto_alloc_shash() stuff. In the end
> I ended up merging the duplicate purgatory and crypto/sha256_generic.c
> generic C SHA256 implementation into a set of new directly callable lib
> functions in lib/crypto/sha256.c, just so that I could move forward with
> this...
>
> Anyways the reason for this background info is that because this is a while
> ago I do not remember exactly how, but yes I did measure this (but not
> very scientifically) and there was no discernible difference in boot
> to init (from the initrd) with this in place vs without this.
>
> >> +
> >> +int efi_get_embedded_fw(const char *name, void **data, size_t *size)
> >> +{
> >> +       struct efi_embedded_fw *iter, *fw = NULL;
> >> +       void *buf = *data;
> >> +
> >> +       if (!checked_for_fw) {
> >
> > WARN_ON_ONCE?  A stack dump would be quite nice here.
>
> As discussed when this check was added (discussion in v7 of
> the set I guess, check added in v8) we can also hit this without
> it being a bug, e.g. when booted through kexec the whole
> efi_check_for_embedded_firmwares() call we be skipped, hence the
> pr_warn.
>
>
> >> +       buf = vmalloc(fw->length);
> >> +       if (!buf)
> >> +               return -ENOMEM;
> >> +
> >> +       memcpy(buf, fw->data, fw->length);
> >> +       *size = fw->length;
> >> +       *data = buf;
> >
> > See above.  What's vmalloc() for?  Where's the vfree()?
>
> See above for my answer. I'm fine with moving this into the
> firmware code, since that is where the matching vfree is, please
> let me know how you want to proceed with this.
>
> > BTW, it would be very nice to have a straightforward way
> > (/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares.
>
> That is an interesting idea, we could add a subsys_init call or
> some such to add this to debugfs (efi_check_for_embedded_firmwares runs
> too early).
>
> But given how long this patch-set has been in the making I would really
> like to get this (or at least the first 2 patches as a start) upstream,
> so for now I would like to keep the changes to a minimum. Are you ok
> with me adding the debugfs support with a follow-up patch ?
>
> Let me also reply to your summary comment elsewhere in the thread here:
>
>  > Early in boot, you check a bunch of firmware descriptors, bundled with
>  > drivers, to search EFI code and data for firmware before you free said
>  > code and data.  You catalogue it by name.  Later on, you use this list
>  > as a fallback, again catalogued by name.  I think it would be rather
>  > nicer if you simply had a list in a single file containing all these
>  > descriptors rather than commingling it with the driver's internal dmi
>  > data.  This gets rid of all the ifdeffery and module issues.  It also
>  > avoids any potential nastiness when you have a dmi entry that contains
>  > driver_data that points into the driver when the driver is a module.
>  >
>  > And you can mark the entire list __initdata.  And you can easily (now
>  > or later on) invert the code flow so you map each EFI region exactly
>  > once and then search for all the firmware in it.
>
> I believe we have mostly discussed this above. At least for the
> X86 touchscreen case, which is the only user of this for now, I
> believe that re-using the table from drivers/platform/x86/touchscreen_dmi.c
> is better as it avoids duplication of DMI strings and it keeps all
> the info in one place (also avoiding churn in 2 files / 2 different
> trees when new models are added).
>
> I agree that when looking at this as a generic mechanism which may be
> used elsewhere, your suggestion makes a lot of sense. But even though
> this is very much written so that it can be used as a generic mechanism
> I'm not sure if other users will appear. So for now, with only the X86
> touchscreen use-case actually using this I believe the current
> implementation is best, but I'm definitely open to changing this around
> if more users show up.
>
> Regards,
>
> Hans
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 02/10] efi: Add embedded peripheral firmware support

[Andy Lutomirski]

> On Jan 14, 2020, at 4:25 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Andy,
>

> Even if we get more use of this the chances of 1 device model using
> more then 1 embedded fw are small. Where as if we first map the
> EFI_BOOT_SERVICES_CODE segment before doing the DMI match then we
> do this for all EFI_BOOT_SERVICES_CODE segments on all hardware.
> The current implementation is very much optimized to do as little
> work as possible when there is no DMI match, which will be true
> on almost all devices out there.

Fine with me.


> If we hit firmware which is not 8 byte aligned, then yes that would be
> a good idea, but for now I feel that that would just cause a slowdown
> in the scanning without any benefits.
>

It would shorten the code and remove a comment :). Also, a good memmem
implementation should be very fast, potentially faster than your loop.
I suspect the latter is only true in user code where SSE would get
used, but still.

it would also be unfortunate if some firmware update switches to
4-byte alignment and touchscreens stop working with no explanation.


>>> +
>>> +               sha256_init(&sctx);
>>> +               sha256_update(&sctx, map + i, desc->length);
>>> +               sha256_final(&sctx, sha256);
>>> +               if (memcmp(sha256, desc->sha256, 32) == 0)
>>> +                       break;
>>> +       }
>>> +       if ((i + desc->length) > size) {
>>> +               memunmap(map);
>>> +               return -ENOENT;
>>> +       }
>>> +
>>> +       pr_info("Found EFI embedded fw '%s'\n", desc->name);
>>> +
>> It might be nice to also log which EFI section it was in?
>
> The EFI sections do not have names, so all I could is log
> the section number which does not really feel useful?
>
>>> +       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, &efi_embedded_fw_list);
>>> +
>> If you actually copy the firmware name instead of just a pointer to
>> it, then you could potentially free the list of EFI firmwares.
>
> If we move to having a separate dmi_system_id table for this then
> that would be true. ATM the dmi_system_id from
> drivers/platform/x86/touchscreen_dmi.c
> is not freed as it is referenced from a bus-notifier.
>
>> Why are you copying the firmware into linear (kmemdup) memory here
>
> The kmemdup is because the EFI_BOOT_SERVICES_CODE section is
> free-ed almost immediately after we run.
>
>> just to copy it to vmalloc space down below...
>
> The vmalloc is because the efi_get_embedded_fw() function is
> a helper for later implementing firmware_Request_platform
> which returns a struct firmware *fw and release_firmware()
> uses vfree() to free the firmware contents.
>
> I guess we could have efi_get_embedded_fw() return an const u8 *
> and have the firmware code do the vmalloc + memcpy but it boils
> down to the same thing.
>
>
>>> +       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;
>>> +
>>> +               /*
>>> +                * In some drivers the struct driver_data contains may contain
>>> +                * other driver specific data after the fw_desc struct; and
>>> +                * the fw_desc struct itself may be empty, skip these.
>>> +                */
>>> +               if (!fw_desc->name)
>>> +                       continue;
>>> +
>>> +               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;
>>> +               }
>>> +       }
>>> +
>>> +       checked_for_fw = true;
>>> +}
>> Have you measured how long this takes on a typical system per matching DMI id?
>
> I originally wrote this approx. 18 months ago, it has been on hold for a while
> since it needed a sha256 method which would work before subsys_initcall-s
> run so I couldn't use the standard crypto_alloc_shash() stuff. In the end
> I ended up merging the duplicate purgatory and crypto/sha256_generic.c
> generic C SHA256 implementation into a set of new directly callable lib
> functions in lib/crypto/sha256.c, just so that I could move forward with
> this...
>
> Anyways the reason for this background info is that because this is a while
> ago I do not remember exactly how, but yes I did measure this (but not
> very scientifically) and there was no discernible difference in boot
> to init (from the initrd) with this in place vs without this.
>
>>> +
>>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size)
>>> +{
>>> +       struct efi_embedded_fw *iter, *fw = NULL;
>>> +       void *buf = *data;
>>> +
>>> +       if (!checked_for_fw) {
>> WARN_ON_ONCE?  A stack dump would be quite nice here.
>
> As discussed when this check was added (discussion in v7 of
> the set I guess, check added in v8) we can also hit this without
> it being a bug, e.g. when booted through kexec the whole
> efi_check_for_embedded_firmwares() call we be skipped, hence the
> pr_warn.
>
>
>>> +       buf = vmalloc(fw->length);
>>> +       if (!buf)
>>> +               return -ENOMEM;
>>> +
>>> +       memcpy(buf, fw->data, fw->length);
>>> +       *size = fw->length;
>>> +       *data = buf;
>> See above.  What's vmalloc() for?  Where's the vfree()?
>
> See above for my answer. I'm fine with moving this into the
> firmware code, since that is where the matching vfree is, please
> let me know how you want to proceed with this.
>
>> BTW, it would be very nice to have a straightforward way
>> (/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares.
>
> That is an interesting idea, we could add a subsys_init call or
> some such to add this to debugfs (efi_check_for_embedded_firmwares runs
> too early).
>
> But given how long this patch-set has been in the making I would really
> like to get this (or at least the first 2 patches as a start) upstream,
> so for now I would like to keep the changes to a minimum. Are you ok
> with me adding the debugfs support with a follow-up patch ?
>
> Let me also reply to your summary comment elsewhere in the thread here:
>
> > Early in boot, you check a bunch of firmware descriptors, bundled with
> > drivers, to search EFI code and data for firmware before you free said
> > code and data.  You catalogue it by name.  Later on, you use this list
> > as a fallback, again catalogued by name.  I think it would be rather
> > nicer if you simply had a list in a single file containing all these
> > descriptors rather than commingling it with the driver's internal dmi
> > data.  This gets rid of all the ifdeffery and module issues.  It also
> > avoids any potential nastiness when you have a dmi entry that contains
> > driver_data that points into the driver when the driver is a module.
> >
> > And you can mark the entire list __initdata.  And you can easily (now
> > or later on) invert the code flow so you map each EFI region exactly
> > once and then search for all the firmware in it.
>
> I believe we have mostly discussed this above. At least for the
> X86 touchscreen case, which is the only user of this for now, I
> believe that re-using the table from drivers/platform/x86/touchscreen_dmi.c
> is better as it avoids duplication of DMI strings and it keeps all
> the info in one place (also avoiding churn in 2 files / 2 different
> trees when new models are added).
>
> I agree that when looking at this as a generic mechanism which may be
> used elsewhere, your suggestion makes a lot of sense. But even though
> this is very much written so that it can be used as a generic mechanism
> I'm not sure if other users will appear. So for now, with only the X86
> touchscreen use-case actually using this I believe the current
> implementation is best, but I'm definitely open to changing this around
> if more users show up.
>
> Regards,
>
> Hans
>

Re: [PATCH v11 02/10] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi Andy,

On 17-01-2020 21:06, Andy Lutomirski wrote:
>> On Jan 14, 2020, at 4:25 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Andy,
>>
> 
>> Even if we get more use of this the chances of 1 device model using
>> more then 1 embedded fw are small. Where as if we first map the
>> EFI_BOOT_SERVICES_CODE segment before doing the DMI match then we
>> do this for all EFI_BOOT_SERVICES_CODE segments on all hardware.
>> The current implementation is very much optimized to do as little
>> work as possible when there is no DMI match, which will be true
>> on almost all devices out there.
> 
> Fine with me.
> 
> 
>> If we hit firmware which is not 8 byte aligned, then yes that would be
>> a good idea, but for now I feel that that would just cause a slowdown
>> in the scanning without any benefits.
>>
> 
> It would shorten the code and remove a comment :). Also, a good memmem
> implementation should be very fast, potentially faster than your loop.
> I suspect the latter is only true in user code where SSE would get
> used, but still.
> 
> it would also be unfortunate if some firmware update switches to
> 4-byte alignment and touchscreens stop working with no explanation.

So I was thinking sure, fine lets replace the loop with memmem,
but the kernel has no memmem, it has memscan but that takes an
int to search for, and reducing the prefix to an int seems likely
to cause more false positives and unnecessary sha256summing.

So I believe it is best to keep this as is for now, we can always
change the alignment requirement later, now that we use memcmp
to check the prefix changing it is just a matter of changing the
i += 8 to e.g. i += 4.

Regards,

Hans


>>>> +
>>>> +               sha256_init(&sctx);
>>>> +               sha256_update(&sctx, map + i, desc->length);
>>>> +               sha256_final(&sctx, sha256);
>>>> +               if (memcmp(sha256, desc->sha256, 32) == 0)
>>>> +                       break;
>>>> +       }
>>>> +       if ((i + desc->length) > size) {
>>>> +               memunmap(map);
>>>> +               return -ENOENT;
>>>> +       }
>>>> +
>>>> +       pr_info("Found EFI embedded fw '%s'\n", desc->name);
>>>> +
>>> It might be nice to also log which EFI section it was in?
>>
>> The EFI sections do not have names, so all I could is log
>> the section number which does not really feel useful?
>>
>>>> +       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, &efi_embedded_fw_list);
>>>> +
>>> If you actually copy the firmware name instead of just a pointer to
>>> it, then you could potentially free the list of EFI firmwares.
>>
>> If we move to having a separate dmi_system_id table for this then
>> that would be true. ATM the dmi_system_id from
>> drivers/platform/x86/touchscreen_dmi.c
>> is not freed as it is referenced from a bus-notifier.
>>
>>> Why are you copying the firmware into linear (kmemdup) memory here
>>
>> The kmemdup is because the EFI_BOOT_SERVICES_CODE section is
>> free-ed almost immediately after we run.
>>
>>> just to copy it to vmalloc space down below...
>>
>> The vmalloc is because the efi_get_embedded_fw() function is
>> a helper for later implementing firmware_Request_platform
>> which returns a struct firmware *fw and release_firmware()
>> uses vfree() to free the firmware contents.
>>
>> I guess we could have efi_get_embedded_fw() return an const u8 *
>> and have the firmware code do the vmalloc + memcpy but it boils
>> down to the same thing.
>>
>>
>>>> +       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;
>>>> +
>>>> +               /*
>>>> +                * In some drivers the struct driver_data contains may contain
>>>> +                * other driver specific data after the fw_desc struct; and
>>>> +                * the fw_desc struct itself may be empty, skip these.
>>>> +                */
>>>> +               if (!fw_desc->name)
>>>> +                       continue;
>>>> +
>>>> +               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;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       checked_for_fw = true;
>>>> +}
>>> Have you measured how long this takes on a typical system per matching DMI id?
>>
>> I originally wrote this approx. 18 months ago, it has been on hold for a while
>> since it needed a sha256 method which would work before subsys_initcall-s
>> run so I couldn't use the standard crypto_alloc_shash() stuff. In the end
>> I ended up merging the duplicate purgatory and crypto/sha256_generic.c
>> generic C SHA256 implementation into a set of new directly callable lib
>> functions in lib/crypto/sha256.c, just so that I could move forward with
>> this...
>>
>> Anyways the reason for this background info is that because this is a while
>> ago I do not remember exactly how, but yes I did measure this (but not
>> very scientifically) and there was no discernible difference in boot
>> to init (from the initrd) with this in place vs without this.
>>
>>>> +
>>>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size)
>>>> +{
>>>> +       struct efi_embedded_fw *iter, *fw = NULL;
>>>> +       void *buf = *data;
>>>> +
>>>> +       if (!checked_for_fw) {
>>> WARN_ON_ONCE?  A stack dump would be quite nice here.
>>
>> As discussed when this check was added (discussion in v7 of
>> the set I guess, check added in v8) we can also hit this without
>> it being a bug, e.g. when booted through kexec the whole
>> efi_check_for_embedded_firmwares() call we be skipped, hence the
>> pr_warn.
>>
>>
>>>> +       buf = vmalloc(fw->length);
>>>> +       if (!buf)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       memcpy(buf, fw->data, fw->length);
>>>> +       *size = fw->length;
>>>> +       *data = buf;
>>> See above.  What's vmalloc() for?  Where's the vfree()?
>>
>> See above for my answer. I'm fine with moving this into the
>> firmware code, since that is where the matching vfree is, please
>> let me know how you want to proceed with this.
>>
>>> BTW, it would be very nice to have a straightforward way
>>> (/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares.
>>
>> That is an interesting idea, we could add a subsys_init call or
>> some such to add this to debugfs (efi_check_for_embedded_firmwares runs
>> too early).
>>
>> But given how long this patch-set has been in the making I would really
>> like to get this (or at least the first 2 patches as a start) upstream,
>> so for now I would like to keep the changes to a minimum. Are you ok
>> with me adding the debugfs support with a follow-up patch ?
>>
>> Let me also reply to your summary comment elsewhere in the thread here:
>>
>>> Early in boot, you check a bunch of firmware descriptors, bundled with
>>> drivers, to search EFI code and data for firmware before you free said
>>> code and data.  You catalogue it by name.  Later on, you use this list
>>> as a fallback, again catalogued by name.  I think it would be rather
>>> nicer if you simply had a list in a single file containing all these
>>> descriptors rather than commingling it with the driver's internal dmi
>>> data.  This gets rid of all the ifdeffery and module issues.  It also
>>> avoids any potential nastiness when you have a dmi entry that contains
>>> driver_data that points into the driver when the driver is a module.
>>>
>>> And you can mark the entire list __initdata.  And you can easily (now
>>> or later on) invert the code flow so you map each EFI region exactly
>>> once and then search for all the firmware in it.
>>
>> I believe we have mostly discussed this above. At least for the
>> X86 touchscreen case, which is the only user of this for now, I
>> believe that re-using the table from drivers/platform/x86/touchscreen_dmi.c
>> is better as it avoids duplication of DMI strings and it keeps all
>> the info in one place (also avoiding churn in 2 files / 2 different
>> trees when new models are added).
>>
>> I agree that when looking at this as a generic mechanism which may be
>> used elsewhere, your suggestion makes a lot of sense. But even though
>> this is very much written so that it can be used as a generic mechanism
>> I'm not sure if other users will appear. So for now, with only the X86
>> touchscreen use-case actually using this I believe the current
>> implementation is best, but I'm definitely open to changing this around
>> if more users show up.
>>
>> Regards,
>>
>> Hans
>>
>