linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] SPI LPC information kernel module
@ 2020-06-29 22:59 Daniel Gutson
  2020-06-30  8:02 ` kernel test robot
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: Daniel Gutson @ 2020-06-29 22:59 UTC (permalink / raw)
  To: Daniel Gutson, Derek Kiernan, Arnd Bergmann, Greg Kroah-Hartman,
	Mauro Carvalho Chehab, linux-kernel, Richard Hughes,
	Alex Bazhaniuk

This kernel module exports configuration attributes for the
system SPI chip.
This initial version exports the BIOS Write Enable (bioswe),
BIOS Lock Enable (ble), and the SMM Bios Write Protect (SMM_BWP)
fields of the Bios Control register. The idea is to keep adding more
flags, not only from the BC but also from other registers in following
versions.

The goal is that the attributes are avilable to fwupd when SecureBoot
is turned on.

A technical note: I check if *ppos == BUFFER_SIZE in the reading function
to exit early and avoid an extra access to the HW, for example when using
the 'cat' command, which causes two read operations.

Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
---
 Documentation/ABI/stable/securityfs-spi-lpc |  23 +
 MAINTAINERS                                 |   6 +
 drivers/misc/Kconfig                        |   1 +
 drivers/misc/Makefile                       |   1 +
 drivers/misc/spi_lpc/Kconfig                |  20 +
 drivers/misc/spi_lpc/Makefile               |   8 +
 drivers/misc/spi_lpc/bios_data_access.c     | 559 +++++++++++++++++++
 drivers/misc/spi_lpc/bios_data_access.h     | 181 +++++++
 drivers/misc/spi_lpc/low_level_access.c     |  59 ++
 drivers/misc/spi_lpc/low_level_access.h     |  21 +
 drivers/misc/spi_lpc/spi_lpc_main.c         | 176 ++++++
 drivers/misc/spi_lpc/viddid_arch_map.c      | 566 ++++++++++++++++++++
 drivers/misc/spi_lpc/viddid_arch_map.h      |  17 +
 13 files changed, 1638 insertions(+)
 create mode 100644 Documentation/ABI/stable/securityfs-spi-lpc
 create mode 100644 drivers/misc/spi_lpc/Kconfig
 create mode 100644 drivers/misc/spi_lpc/Makefile
 create mode 100644 drivers/misc/spi_lpc/bios_data_access.c
 create mode 100644 drivers/misc/spi_lpc/bios_data_access.h
 create mode 100644 drivers/misc/spi_lpc/low_level_access.c
 create mode 100644 drivers/misc/spi_lpc/low_level_access.h
 create mode 100644 drivers/misc/spi_lpc/spi_lpc_main.c
 create mode 100644 drivers/misc/spi_lpc/viddid_arch_map.c
 create mode 100644 drivers/misc/spi_lpc/viddid_arch_map.h

diff --git a/Documentation/ABI/stable/securityfs-spi-lpc b/Documentation/ABI/stable/securityfs-spi-lpc
new file mode 100644
index 000000000000..22660a7fd914
--- /dev/null
+++ b/Documentation/ABI/stable/securityfs-spi-lpc
@@ -0,0 +1,23 @@
+What:		/sys/kernel/security/firmware/bioswe
+Date:		June 2020
+KernelVersion:	5.8.0
+Contact:	daniel.gutson@eclypsium.com
+Description:	If the system firmware set BIOS Write Enable.
+		0: writes disabled, 1: writes enabled.
+Users:		https://github.com/fwupd/fwupd
+
+What:		/sys/kernel/security/firmware/ble
+Date:		June 2020
+KernelVersion:	5.8.0
+Contact:	daniel.gutson@eclypsium.com
+Description:	If the system firmware set Bios Lock Enable.
+		0: SMM lock disabled, 1: SMM lock enabled.
+Users:		https://github.com/fwupd/fwupd
+
+What:		/sys/kernel/security/firmware/smm_bwp
+Date:		June 2020
+KernelVersion:	5.8.0
+Contact:	daniel.gutson@eclypsium.com
+Description:	If the system firmware set SMM Bios Write Protect.
+		0: writes disabled unless in SMM, 1: writes enabled.
+Users:		https://github.com/fwupd/fwupd
diff --git a/MAINTAINERS b/MAINTAINERS
index 7b5ffd646c6b..d7b7f943cb75 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16078,6 +16078,12 @@ W:	http://www.st.com/spear
 F:	arch/arm/boot/dts/spear*
 F:	arch/arm/mach-spear/
 
+SPI LPC information kernel MODULE
+M:	Daniel Gutson <daniel.gutson@eclypsium.com>
+S:	Supported
+F:	Documentation/ABI/stable/securityfs-spi-lpc
+F:	drivers/misc/spi_lpc/*
+
 SPI NOR SUBSYSTEM
 M:	Tudor Ambarus <tudor.ambarus@microchip.com>
 L:	linux-mtd@lists.infradead.org
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index e1b1ba5e2b92..228ceeb3d389 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -472,4 +472,5 @@ source "drivers/misc/ocxl/Kconfig"
 source "drivers/misc/cardreader/Kconfig"
 source "drivers/misc/habanalabs/Kconfig"
 source "drivers/misc/uacce/Kconfig"
+source "drivers/misc/spi_lpc/Kconfig"
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c7bd01ac6291..280365e7e53c 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_PVPANIC)   	+= pvpanic.o
 obj-$(CONFIG_HABANA_AI)		+= habanalabs/
 obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
+obj-$(CONFIG_SPI_LPC)		+= spi_lpc/
diff --git a/drivers/misc/spi_lpc/Kconfig b/drivers/misc/spi_lpc/Kconfig
new file mode 100644
index 000000000000..08d74746578d
--- /dev/null
+++ b/drivers/misc/spi_lpc/Kconfig
@@ -0,0 +1,20 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# SPI LPC information kernel module
+#
+
+config SPI_LPC
+	tristate "SPI LPC information"
+	depends on SECURITYFS && CPU_SUP_INTEL
+	help
+	  This kernel modules exports the configuration attributes for the
+	  system SPI chip.
+	  Enable this kernel module to read the BIOSWE, BLE, and SMM_BWP fields
+	  of the Bios Control register, by reading these three files:
+
+    	      /sys/kernel/security/firmware/bioswe
+    	      /sys/kernel/security/firmware/ble
+    	      /sys/kernel/security/firmware/smm_bwp
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called spi_lpc.
diff --git a/drivers/misc/spi_lpc/Makefile b/drivers/misc/spi_lpc/Makefile
new file mode 100644
index 000000000000..65f314d37164
--- /dev/null
+++ b/drivers/misc/spi_lpc/Makefile
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the SPI LPC kernel module
+#
+
+obj-m	:= spi_lpc.o
+
+spi_lpc-y := spi_lpc_main.o bios_data_access.o low_level_access.o viddid_arch_map.o
diff --git a/drivers/misc/spi_lpc/bios_data_access.c b/drivers/misc/spi_lpc/bios_data_access.c
new file mode 100644
index 000000000000..9c5932ad6d61
--- /dev/null
+++ b/drivers/misc/spi_lpc/bios_data_access.c
@@ -0,0 +1,559 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SPI LPC flash platform security driver
+ *
+ * Copyright 2020 (c) Daniel Gutson (daniel.gutson@eclypsium.com)
+ *
+ */
+#include <linux/module.h>
+#include "low_level_access.h"
+#include "bios_data_access.h"
+
+#define get_mask_from_bit_size(type, size)                                     \
+	(((type) ~((type)0)) >> (sizeof(type) * 8 - size))
+
+#define get_mask_from_bit_size_with_offset(type, size, offset)                 \
+	(get_mask_from_bit_size(type, size) << (offset))
+
+#define extract_bits(type, value, start, size)                                 \
+	((value)&get_mask_from_bit_size_with_offset(type, size, start))
+
+#define extract_bits_shifted(type, value, start, size)                         \
+	(extract_bits(type, value, start, size) >> (start))
+
+static int read_spibar(enum PCH_Arch pch_arch, enum CPU_Arch cpu_arch,
+		       u64 *offset);
+
+static int read_sbase_atom_avn_byt(struct sbase_atom_avn_byt *reg)
+{
+	u32 value;
+	const int ret = pci_read_dword(&value, 0x0, 0x1f, 0x0, 0x54);
+
+	if (ret != 0)
+		return ret;
+
+	reg->memi = extract_bits_shifted(u32, value, 0, 1);
+	reg->enable = extract_bits_shifted(u32, value, 1, 1);
+	reg->addrng = extract_bits_shifted(u32, value, 2, 1);
+	reg->pref = extract_bits_shifted(u32, value, 3, 1);
+	reg->base = extract_bits(u32, value, 9, 23);
+
+	return 0;
+}
+
+int spi_read_sbase(enum PCH_Arch pch_arch __maybe_unused,
+		   enum CPU_Arch cpu_arch, struct spi_sbase *reg)
+{
+	int ret = 0;
+
+	reg->register_arch.source = RegSource_CPU;
+	reg->register_arch.cpu_arch = cpu_arch;
+
+	switch (cpu_arch) {
+	case cpu_avn:
+	case cpu_byt:
+		ret = read_sbase_atom_avn_byt(&reg->cpu_byt);
+		break;
+	default:
+		ret = -EIO;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_read_sbase);
+
+static int read_bc_pch_3xx_4xx_5xx(struct bc_pch_3xx_4xx_5xx *reg)
+{
+	u32 value;
+	const int ret = pci_read_dword(&value, 0x0, 0x1f, 0x5, 0xdc);
+
+	if (ret != 0)
+		return ret;
+
+	reg->bioswe = extract_bits_shifted(u32, value, 0, 1);
+	reg->ble = extract_bits_shifted(u32, value, 1, 1);
+	reg->src = extract_bits_shifted(u32, value, 2, 2);
+	reg->tss = extract_bits_shifted(u32, value, 4, 1);
+	reg->smm_bwp = extract_bits_shifted(u32, value, 5, 1);
+	reg->bbs = extract_bits_shifted(u32, value, 6, 1);
+	reg->bild = extract_bits_shifted(u32, value, 7, 1);
+	reg->spi_sync_ss = extract_bits_shifted(u32, value, 8, 1);
+	reg->spi_async_ss = extract_bits_shifted(u32, value, 10, 1);
+	reg->ase_bwp = extract_bits_shifted(u32, value, 11, 1);
+
+	return 0;
+}
+
+static int
+read_bc_cpu_snb_jkt_ivb_ivt_bdx_hsx(struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx *reg)
+{
+	u32 value;
+	const int ret = pci_read_dword(&value, 0x0, 0x1f, 0x5, 0xdc);
+
+	if (ret != 0)
+		return ret;
+
+	reg->bioswe = extract_bits_shifted(u32, value, 0, 1);
+	reg->ble = extract_bits_shifted(u32, value, 1, 1);
+	reg->src = extract_bits_shifted(u32, value, 2, 2);
+	reg->tss = extract_bits_shifted(u32, value, 4, 1);
+	reg->smm_bwp = extract_bits_shifted(u32, value, 5, 1);
+
+	return 0;
+}
+
+static int read_bc_cpu_skl_kbl_cfl(struct bc_cpu_skl_kbl_cfl *reg)
+{
+	u32 value;
+	const int ret = pci_read_dword(&value, 0x0, 0x1f, 0x5, 0xdc);
+
+	if (ret != 0)
+		return ret;
+
+	reg->bioswe = extract_bits_shifted(u32, value, 0, 1);
+	reg->ble = extract_bits_shifted(u32, value, 1, 1);
+	reg->src = extract_bits_shifted(u32, value, 2, 2);
+	reg->tss = extract_bits_shifted(u32, value, 4, 1);
+	reg->smm_bwp = extract_bits_shifted(u32, value, 5, 1);
+	reg->bbs = extract_bits_shifted(u32, value, 6, 1);
+	reg->bild = extract_bits_shifted(u32, value, 7, 1);
+
+	return 0;
+}
+
+static int read_bc_cpu_apl_glk(struct bc_cpu_apl_glk *reg)
+{
+	u32 value;
+	const int ret = pci_read_dword(&value, 0x0, 0xd, 0x2, 0xdc);
+
+	if (ret != 0)
+		return ret;
+
+	reg->bioswe = extract_bits_shifted(u32, value, 0, 1);
+	reg->ble = extract_bits_shifted(u32, value, 1, 1);
+	reg->src = extract_bits_shifted(u32, value, 2, 2);
+	reg->tss = extract_bits_shifted(u32, value, 4, 1);
+	reg->smm_bwp = extract_bits_shifted(u32, value, 5, 1);
+	reg->bbs = extract_bits_shifted(u32, value, 6, 1);
+	reg->bild = extract_bits_shifted(u32, value, 7, 1);
+	reg->spi_sync_ss = extract_bits_shifted(u32, value, 8, 1);
+	reg->osfh = extract_bits_shifted(u32, value, 9, 1);
+	reg->spi_async_ss = extract_bits_shifted(u32, value, 10, 1);
+	reg->ase_bwp = extract_bits_shifted(u32, value, 11, 1);
+
+	return 0;
+}
+
+static int read_bc_cpu_atom_avn(struct bc_cpu_atom_avn *reg,
+				enum PCH_Arch pch_arch, enum CPU_Arch cpu_arch)
+{
+	u8 value;
+	int ret;
+	u64 barOffset;
+
+	ret = read_spibar(pch_arch, cpu_arch, &barOffset);
+	if (ret != 0)
+		return ret;
+
+	ret = mmio_read_byte(barOffset + 0xfc, &value);
+	if (ret != 0)
+		return ret;
+
+	reg->bioswe = extract_bits_shifted(u8, value, 0, 1);
+	reg->ble = extract_bits_shifted(u8, value, 1, 1);
+	reg->src = extract_bits_shifted(u8, value, 2, 2);
+	reg->tss = extract_bits_shifted(u8, value, 4, 1);
+	reg->smm_bwp = extract_bits_shifted(u8, value, 5, 1);
+
+	return 0;
+}
+
+static int read_bc_cpu_atom_byt(struct bc_cpu_atom_byt *reg,
+				enum PCH_Arch pch_arch, enum CPU_Arch cpu_arch)
+{
+	u32 value;
+	int ret;
+	u64 barOffset;
+
+	ret = read_spibar(pch_arch, cpu_arch, &barOffset);
+	if (ret != 0)
+		return ret;
+
+	ret = mmio_read_dword(barOffset + 0xfc, &value);
+	if (ret != 0)
+		return ret;
+
+	reg->bioswe = extract_bits_shifted(u32, value, 0, 1);
+	reg->ble = extract_bits_shifted(u32, value, 1, 1);
+	reg->src = extract_bits_shifted(u32, value, 2, 2);
+	reg->smm_bwp = extract_bits_shifted(u32, value, 5, 1);
+
+	return 0;
+}
+
+int spi_read_bc(enum PCH_Arch pch_arch, enum CPU_Arch cpu_arch,
+		struct spi_bc *reg)
+{
+	int ret = 0;
+
+	reg->register_arch.source = RegSource_PCH;
+	reg->register_arch.pch_arch = pch_arch;
+
+	switch (pch_arch) {
+	case pch_3xx:
+	case pch_4xx:
+	case pch_495:
+	case pch_5xx:
+		ret = read_bc_pch_3xx_4xx_5xx(&reg->pch_5xx);
+		break;
+	default:
+		reg->register_arch.source = RegSource_CPU;
+		reg->register_arch.cpu_arch = cpu_arch;
+
+		switch (cpu_arch) {
+		case cpu_snb:
+		case cpu_jkt:
+		case cpu_ivb:
+		case cpu_ivt:
+		case cpu_bdw:
+		case cpu_bdx:
+		case cpu_hsx:
+		case cpu_hsw:
+			ret = read_bc_cpu_snb_jkt_ivb_ivt_bdx_hsx(
+				&reg->cpu_hsw);
+			break;
+		case cpu_skl:
+		case cpu_kbl:
+		case cpu_cfl:
+			ret = read_bc_cpu_skl_kbl_cfl(&reg->cpu_cfl);
+			break;
+		case cpu_apl:
+		case cpu_glk:
+			ret = read_bc_cpu_apl_glk(&reg->cpu_glk);
+			break;
+		case cpu_avn:
+			ret = read_bc_cpu_atom_avn(&reg->cpu_avn, pch_arch,
+						   cpu_arch);
+			break;
+		case cpu_byt:
+			ret = read_bc_cpu_atom_byt(&reg->cpu_byt, pch_arch,
+						   cpu_arch);
+			break;
+		default:
+			ret = -EIO;
+		}
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_read_bc);
+
+int read_spibar(enum PCH_Arch pch_arch, enum CPU_Arch cpu_arch, u64 *offset)
+{
+	int ret = 0;
+	u64 field_offset;
+
+	switch (cpu_arch) {
+	case cpu_avn:
+	case cpu_byt: {
+		struct spi_sbase reg;
+
+		ret = spi_read_sbase(pch_arch, cpu_arch, &reg);
+		if (ret == 0) {
+			ret = spi_read_sbase_base(&reg, &field_offset);
+			*offset = field_offset + 0;
+		}
+	} break;
+	default:
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+int spi_read_bc_bioswe(const struct spi_bc *reg, u64 *value)
+{
+	int ret = 0;
+
+	switch (reg->register_arch.source) {
+	case RegSource_PCH:
+		switch (reg->register_arch.pch_arch) {
+		case pch_3xx:
+			*value = reg->pch_3xx.bioswe;
+			break;
+		case pch_4xx:
+			*value = reg->pch_4xx.bioswe;
+			break;
+		case pch_495:
+			*value = reg->pch_495.bioswe;
+			break;
+		case pch_5xx:
+			*value = reg->pch_5xx.bioswe;
+			break;
+		default:
+			/* requested PCH arch hasn't field bioswe */
+			ret = -EIO;
+			*value = 0;
+		}
+		break;
+	case RegSource_CPU:
+		switch (reg->register_arch.cpu_arch) {
+		case cpu_snb:
+			*value = reg->cpu_snb.bioswe;
+			break;
+		case cpu_jkt:
+			*value = reg->cpu_jkt.bioswe;
+			break;
+		case cpu_ivb:
+			*value = reg->cpu_ivb.bioswe;
+			break;
+		case cpu_ivt:
+			*value = reg->cpu_ivt.bioswe;
+			break;
+		case cpu_bdw:
+			*value = reg->cpu_bdw.bioswe;
+			break;
+		case cpu_bdx:
+			*value = reg->cpu_bdx.bioswe;
+			break;
+		case cpu_hsx:
+			*value = reg->cpu_hsx.bioswe;
+			break;
+		case cpu_hsw:
+			*value = reg->cpu_hsw.bioswe;
+			break;
+		case cpu_skl:
+			*value = reg->cpu_skl.bioswe;
+			break;
+		case cpu_kbl:
+			*value = reg->cpu_kbl.bioswe;
+			break;
+		case cpu_cfl:
+			*value = reg->cpu_cfl.bioswe;
+			break;
+		case cpu_apl:
+			*value = reg->cpu_apl.bioswe;
+			break;
+		case cpu_glk:
+			*value = reg->cpu_glk.bioswe;
+			break;
+		case cpu_avn:
+			*value = reg->cpu_avn.bioswe;
+			break;
+		case cpu_byt:
+			*value = reg->cpu_byt.bioswe;
+			break;
+		default:
+			/* requested CPU arch hasn't field bioswe */
+			ret = -EIO;
+			*value = 0;
+		}
+		break;
+	default:
+		ret = -EIO; /* should not reach here, it's a bug */
+		*value = 0;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_read_bc_bioswe);
+
+int spi_read_bc_ble(const struct spi_bc *reg, u64 *value)
+{
+	int ret = 0;
+
+	switch (reg->register_arch.source) {
+	case RegSource_PCH:
+		switch (reg->register_arch.pch_arch) {
+		case pch_3xx:
+			*value = reg->pch_3xx.ble;
+			break;
+		case pch_4xx:
+			*value = reg->pch_4xx.ble;
+			break;
+		case pch_495:
+			*value = reg->pch_495.ble;
+			break;
+		case pch_5xx:
+			*value = reg->pch_5xx.ble;
+			break;
+		default:
+			/* requested PCH arch hasn't field ble */
+			ret = -EIO;
+			*value = 0;
+		}
+		break;
+	case RegSource_CPU:
+		switch (reg->register_arch.cpu_arch) {
+		case cpu_snb:
+			*value = reg->cpu_snb.ble;
+			break;
+		case cpu_jkt:
+			*value = reg->cpu_jkt.ble;
+			break;
+		case cpu_ivb:
+			*value = reg->cpu_ivb.ble;
+			break;
+		case cpu_ivt:
+			*value = reg->cpu_ivt.ble;
+			break;
+		case cpu_bdw:
+			*value = reg->cpu_bdw.ble;
+			break;
+		case cpu_bdx:
+			*value = reg->cpu_bdx.ble;
+			break;
+		case cpu_hsx:
+			*value = reg->cpu_hsx.ble;
+			break;
+		case cpu_hsw:
+			*value = reg->cpu_hsw.ble;
+			break;
+		case cpu_skl:
+			*value = reg->cpu_skl.ble;
+			break;
+		case cpu_kbl:
+			*value = reg->cpu_kbl.ble;
+			break;
+		case cpu_cfl:
+			*value = reg->cpu_cfl.ble;
+			break;
+		case cpu_apl:
+			*value = reg->cpu_apl.ble;
+			break;
+		case cpu_glk:
+			*value = reg->cpu_glk.ble;
+			break;
+		case cpu_avn:
+			*value = reg->cpu_avn.ble;
+			break;
+		case cpu_byt:
+			*value = reg->cpu_byt.ble;
+			break;
+		default:
+			/* requested CPU arch hasn't field ble */
+			ret = -EIO;
+			*value = 0;
+		}
+		break;
+	default:
+		ret = -EIO; /* should not reach here, it's a bug */
+		*value = 0;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_read_bc_ble);
+
+int spi_read_bc_smm_bwp(const struct spi_bc *reg, u64 *value)
+{
+	int ret = 0;
+
+	switch (reg->register_arch.source) {
+	case RegSource_PCH:
+		switch (reg->register_arch.pch_arch) {
+		case pch_3xx:
+			*value = reg->pch_3xx.smm_bwp;
+			break;
+		case pch_4xx:
+			*value = reg->pch_4xx.smm_bwp;
+			break;
+		case pch_495:
+			*value = reg->pch_495.smm_bwp;
+			break;
+		case pch_5xx:
+			*value = reg->pch_5xx.smm_bwp;
+			break;
+		default:
+			/* requested PCH arch hasn't field smm_bwp */
+			ret = -EIO;
+			*value = 0;
+		}
+		break;
+	case RegSource_CPU:
+		switch (reg->register_arch.cpu_arch) {
+		case cpu_snb:
+			*value = reg->cpu_snb.smm_bwp;
+			break;
+		case cpu_jkt:
+			*value = reg->cpu_jkt.smm_bwp;
+			break;
+		case cpu_ivb:
+			*value = reg->cpu_ivb.smm_bwp;
+			break;
+		case cpu_ivt:
+			*value = reg->cpu_ivt.smm_bwp;
+			break;
+		case cpu_bdw:
+			*value = reg->cpu_bdw.smm_bwp;
+			break;
+		case cpu_bdx:
+			*value = reg->cpu_bdx.smm_bwp;
+			break;
+		case cpu_hsx:
+			*value = reg->cpu_hsx.smm_bwp;
+			break;
+		case cpu_hsw:
+			*value = reg->cpu_hsw.smm_bwp;
+			break;
+		case cpu_skl:
+			*value = reg->cpu_skl.smm_bwp;
+			break;
+		case cpu_kbl:
+			*value = reg->cpu_kbl.smm_bwp;
+			break;
+		case cpu_cfl:
+			*value = reg->cpu_cfl.smm_bwp;
+			break;
+		case cpu_apl:
+			*value = reg->cpu_apl.smm_bwp;
+			break;
+		case cpu_glk:
+			*value = reg->cpu_glk.smm_bwp;
+			break;
+		case cpu_avn:
+			*value = reg->cpu_avn.smm_bwp;
+			break;
+		case cpu_byt:
+			*value = reg->cpu_byt.smm_bwp;
+			break;
+		default:
+			/* requested CPU arch hasn't field smm_bwp */
+			ret = -EIO;
+			*value = 0;
+		}
+		break;
+	default:
+		ret = -EIO; /* should not reach here, it's a bug */
+		*value = 0;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_read_bc_smm_bwp);
+
+int spi_read_sbase_base(const struct spi_sbase *reg, u64 *value)
+{
+	int ret = 0;
+
+	switch (reg->register_arch.source) {
+	case RegSource_PCH:
+		ret = -EIO; /* no PCH archs have this field */
+		*value = 0;
+		break;
+	case RegSource_CPU:
+		switch (reg->register_arch.cpu_arch) {
+		case cpu_avn:
+			*value = reg->cpu_avn.base;
+			break;
+		case cpu_byt:
+			*value = reg->cpu_byt.base;
+			break;
+		default:
+			/* requested CPU arch hasn't field base */
+			ret = -EIO;
+			*value = 0;
+		}
+		break;
+	default:
+		ret = -EIO; /* should not reach here, it's a bug */
+		*value = 0;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_read_sbase_base);
diff --git a/drivers/misc/spi_lpc/bios_data_access.h b/drivers/misc/spi_lpc/bios_data_access.h
new file mode 100644
index 000000000000..a97da7d9f6be
--- /dev/null
+++ b/drivers/misc/spi_lpc/bios_data_access.h
@@ -0,0 +1,181 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * SPI LPC flash platform security driver
+ *
+ * Copyright 2020 (c) Daniel Gutson (daniel.gutson@eclypsium.com)
+ *
+ */
+#ifndef BIOS_DATA_ACCESS_H
+#define BIOS_DATA_ACCESS_H
+
+#include <linux/types.h>
+
+enum PCH_Arch {
+	pch_none,
+	pch_6_c200,
+	pch_7_c210,
+	pch_c60x_x79,
+	pch_communications_89xx,
+	pch_8_c220,
+	pch_c61x_x99,
+	pch_5_mobile,
+	pch_6_mobile,
+	pch_7_8_mobile,
+	pch_1xx,
+	pch_c620,
+	pch_2xx,
+	pch_3xx,
+	pch_4xx,
+	pch_495,
+	pch_5xx,
+	PCH_Archs_count
+};
+
+enum CPU_Arch {
+	cpu_none,
+	cpu_bdw,
+	cpu_bdx,
+	cpu_hsw,
+	cpu_hsx,
+	cpu_ivt,
+	cpu_jkt,
+	cpu_kbl,
+	cpu_skl,
+	cpu_ivb,
+	cpu_snb,
+	cpu_avn,
+	cpu_cfl,
+	cpu_byt,
+	cpu_whl,
+	cpu_cml,
+	cpu_icl,
+	cpu_apl,
+	cpu_glk,
+	cpu_tgl,
+	cpu_amd,
+	CPU_Archs_count
+};
+
+enum RegisterSource { RegSource_PCH, RegSource_CPU };
+
+struct RegisterArch {
+	enum RegisterSource source;
+
+	union {
+		enum PCH_Arch pch_arch;
+		enum CPU_Arch cpu_arch;
+	};
+};
+
+struct sbase_atom_avn_byt {
+	u64 memi;
+	u64 enable;
+	u64 addrng;
+	u64 pref;
+	u64 base;
+};
+
+struct spi_sbase {
+	struct RegisterArch register_arch;
+
+	union {
+		struct sbase_atom_avn_byt cpu_avn;
+		struct sbase_atom_avn_byt cpu_byt;
+	};
+};
+
+struct bc_pch_3xx_4xx_5xx {
+	u64 bioswe;
+	u64 ble;
+	u64 src;
+	u64 tss;
+	u64 smm_bwp;
+	u64 bbs;
+	u64 bild;
+	u64 spi_sync_ss;
+	u64 spi_async_ss;
+	u64 ase_bwp;
+};
+
+struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx {
+	u64 bioswe;
+	u64 ble;
+	u64 src;
+	u64 tss;
+	u64 smm_bwp;
+};
+
+struct bc_cpu_skl_kbl_cfl {
+	u64 bioswe;
+	u64 ble;
+	u64 src;
+	u64 tss;
+	u64 smm_bwp;
+	u64 bbs;
+	u64 bild;
+};
+
+struct bc_cpu_apl_glk {
+	u64 bioswe;
+	u64 ble;
+	u64 src;
+	u64 tss;
+	u64 smm_bwp;
+	u64 bbs;
+	u64 bild;
+	u64 spi_sync_ss;
+	u64 osfh;
+	u64 spi_async_ss;
+	u64 ase_bwp;
+};
+
+struct bc_cpu_atom_avn {
+	u64 bioswe;
+	u64 ble;
+	u64 src;
+	u64 tss;
+	u64 smm_bwp;
+};
+
+struct bc_cpu_atom_byt {
+	u64 bioswe;
+	u64 ble;
+	u64 src;
+	u64 smm_bwp;
+};
+
+struct spi_bc {
+	struct RegisterArch register_arch;
+
+	union {
+		struct bc_pch_3xx_4xx_5xx pch_3xx;
+		struct bc_pch_3xx_4xx_5xx pch_4xx;
+		struct bc_pch_3xx_4xx_5xx pch_495;
+		struct bc_pch_3xx_4xx_5xx pch_5xx;
+		struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_snb;
+		struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_jkt;
+		struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_ivb;
+		struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_ivt;
+		struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_bdw;
+		struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_bdx;
+		struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_hsx;
+		struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_hsw;
+		struct bc_cpu_skl_kbl_cfl cpu_skl;
+		struct bc_cpu_skl_kbl_cfl cpu_kbl;
+		struct bc_cpu_skl_kbl_cfl cpu_cfl;
+		struct bc_cpu_apl_glk cpu_apl;
+		struct bc_cpu_apl_glk cpu_glk;
+		struct bc_cpu_atom_avn cpu_avn;
+		struct bc_cpu_atom_byt cpu_byt;
+	};
+};
+
+extern int spi_read_sbase(enum PCH_Arch pch_arch, enum CPU_Arch cpu_arch,
+			  struct spi_sbase *reg);
+extern int spi_read_bc(enum PCH_Arch pch_arch, enum CPU_Arch cpu_arch,
+		       struct spi_bc *reg);
+extern int spi_read_bc_bioswe(const struct spi_bc *reg, u64 *value);
+extern int spi_read_bc_ble(const struct spi_bc *reg, u64 *value);
+extern int spi_read_bc_smm_bwp(const struct spi_bc *reg, u64 *value);
+extern int spi_read_sbase_base(const struct spi_sbase *reg, u64 *value);
+#endif /* BIOS_DATA_ACCESS_H */
diff --git a/drivers/misc/spi_lpc/low_level_access.c b/drivers/misc/spi_lpc/low_level_access.c
new file mode 100644
index 000000000000..9d3ce9e69b8c
--- /dev/null
+++ b/drivers/misc/spi_lpc/low_level_access.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SPI LPC flash platform security driver
+ *
+ * Copyright 2020 (c) Daniel Gutson (daniel.gutson@eclypsium.com)
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/version.h>
+#include <linux/pci.h>
+#include "low_level_access.h"
+
+#define GENERIC_MMIO_READ(Type, Suffix, function)                              \
+	int mmio_read_##Suffix(u64 phys_address, Type *value)                  \
+	{                                                                      \
+		int ret = 0;                                                   \
+		void __iomem *mapped_address =                                 \
+			ioremap(phys_address, sizeof(Type));                   \
+		pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
+			 sizeof(Type));                                        \
+		if (mapped_address != NULL) {                                  \
+			*value = function(mapped_address);                     \
+			iounmap(mapped_address);                               \
+		} else {                                                       \
+			pr_err("Failed to MAP IO memory: 0x%llx\n",            \
+			       phys_address);                                  \
+			ret = -EIO;                                            \
+		}                                                              \
+		return ret;                                                    \
+	}
+GENERIC_MMIO_READ(u8, byte, readb)
+GENERIC_MMIO_READ(u16, word, readw)
+GENERIC_MMIO_READ(u32, dword, readl)
+#undef GENERIC_MMIO_READ
+
+#define GENERIC_PCI_READ(Suffix, Type)                                         \
+	int pci_read_##Suffix(Type *value, u64 bus, u64 device, u64 function,  \
+			      u64 offset)                                      \
+	{                                                                      \
+		int ret;                                                       \
+		struct pci_bus *found_bus = pci_find_bus(0, bus);              \
+		pr_debug("Reading PCI 0x%llx 0x%llx 0x%llx 0x%llx\n", bus,     \
+			 device, function, offset);                            \
+		if (found_bus != NULL) {                                       \
+			ret = pci_bus_read_config_##Suffix(                    \
+				found_bus, PCI_DEVFN(device, function),        \
+				offset, value);                                \
+		} else {                                                       \
+			pr_err("Couldn't find Bus 0x%llx\n", bus);             \
+			ret = -EIO;                                            \
+		}                                                              \
+		return ret;                                                    \
+	}
+
+GENERIC_PCI_READ(byte, u8)
+GENERIC_PCI_READ(word, u16)
+GENERIC_PCI_READ(dword, u32)
+#undef GENERIC_PCI_READ
diff --git a/drivers/misc/spi_lpc/low_level_access.h b/drivers/misc/spi_lpc/low_level_access.h
new file mode 100644
index 000000000000..499ca1d84678
--- /dev/null
+++ b/drivers/misc/spi_lpc/low_level_access.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * SPI LPC flash platform security driver
+ *
+ * Copyright 2020 (c) Daniel Gutson (daniel.gutson@eclypsium.com)
+ */
+
+#ifndef LOW_LEVEL_H
+#define LOW_LEVEL_H
+
+#include <linux/types.h>
+
+int pci_read_byte(u8 *value, u64 bus, u64 device, u64 function, u64 offset);
+int pci_read_word(u16 *value, u64 bus, u64 device, u64 function, u64 offset);
+int pci_read_dword(u32 *value, u64 bus, u64 device, u64 function, u64 offset);
+
+int mmio_read_byte(u64 phys_address, u8 *value);
+int mmio_read_word(u64 phys_address, u16 *value);
+int mmio_read_dword(u64 phys_address, u32 *value);
+
+#endif /* LOW_LEVEL_H */
diff --git a/drivers/misc/spi_lpc/spi_lpc_main.c b/drivers/misc/spi_lpc/spi_lpc_main.c
new file mode 100644
index 000000000000..d83ee1eb9768
--- /dev/null
+++ b/drivers/misc/spi_lpc/spi_lpc_main.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SPI LPC flash platform security driver
+ *
+ * Copyright 2020 (c) Richard Hughes (richard@hughsie.com)
+ *                    Daniel Gutson (daniel.gutson@eclypsium.com)
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/security.h>
+#include "bios_data_access.h"
+#include "low_level_access.h"
+#include "viddid_arch_map.h"
+
+#define SIZE_WORD sizeof(u16)
+#define WORD_MASK 0xFFFFu
+#define LOW_WORD(x) ((x) & WORD_MASK)
+#define HIGH_WORD(x) ((x) >> ((SIZE_WORD * 8)) & WORD_MASK)
+
+static enum PCH_Arch pch_arch;
+static enum CPU_Arch cpu_arch;
+
+static struct dentry *spi_dir;
+static struct dentry *spi_bioswe;
+static struct dentry *spi_ble;
+static struct dentry *spi_smm_bwp;
+
+typedef int Read_BC_Flag_Fn(struct spi_bc *bc, u64 *value);
+
+static int get_pci_vid_did(u8 bus, u8 dev, u8 fun, u16 *vid, u16 *did)
+{
+	u32 vid_did;
+	int ret = pci_read_dword(&vid_did, bus, dev, fun, 0);
+
+	if (ret == 0) {
+		*vid = LOW_WORD(vid_did);
+		*did = HIGH_WORD(vid_did);
+	}
+	return ret;
+}
+
+static int get_pch_arch(enum PCH_Arch *pch_arch)
+{
+	u16 pch_vid;
+	u16 pch_did;
+	int ret = get_pci_vid_did(0, 0x1f, 0, &pch_vid, &pch_did);
+
+	if (ret != 0)
+		return ret;
+
+	pr_debug("PCH VID: %x - DID: %x\n", pch_vid, pch_did);
+	ret = viddid2pch_arch(pch_vid, pch_did, pch_arch);
+
+	return ret;
+}
+
+static int get_cpu_arch(enum CPU_Arch *cpu_arch)
+{
+	u16 cpu_vid;
+	u16 cpu_did;
+	int ret = get_pci_vid_did(0, 0, 0, &cpu_vid, &cpu_did);
+
+	if (ret != 0)
+		return ret;
+
+	pr_debug("CPU VID: %x - DID: %x\n", cpu_vid, cpu_did);
+	ret = viddid2cpu_arch(cpu_vid, cpu_did, cpu_arch);
+
+	return ret;
+}
+
+static int get_pch_cpu(enum PCH_Arch *pch_arch, enum CPU_Arch *cpu_arch)
+{
+	const int cpu_res = get_cpu_arch(cpu_arch);
+	const int pch_res = get_pch_arch(pch_arch);
+
+	return cpu_res != 0 && pch_res != 0 ? -EIO : 0;
+}
+
+/* Buffer to return: always 3 because of the following chars:
+ *     value \n \0
+ */
+#define BUFFER_SIZE 3
+
+static ssize_t bc_flag_read(struct file *filp, char __user *buf, size_t count,
+			    loff_t *ppos)
+{
+	char tmp[BUFFER_SIZE];
+	ssize_t ret;
+	u64 value = 0;
+	struct spi_bc bc;
+
+	if (*ppos == BUFFER_SIZE)
+		return 0; /* nothing else to read */
+
+	if (file_inode(filp)->i_private == NULL)
+		return -EIO;
+
+	ret = spi_read_bc(pch_arch, cpu_arch, &bc);
+
+	if (ret == 0)
+		ret = ((Read_BC_Flag_Fn *)file_inode(filp)->i_private)(&bc,
+								       &value);
+
+	if (ret != 0)
+		return ret;
+
+	sprintf(tmp, "%d\n", (int)value & 1);
+	ret = simple_read_from_buffer(buf, count, ppos, tmp, sizeof(tmp));
+
+	return ret;
+}
+
+static const struct file_operations bc_flags_ops = {
+	.read = bc_flag_read,
+};
+
+static int __init mod_init(void)
+{
+	int ret = 0;
+
+	if (get_pch_cpu(&pch_arch, &cpu_arch) != 0) {
+		pr_err("Couldn't detect PCH or CPU\n");
+		return -EIO;
+	}
+
+	spi_dir = securityfs_create_dir("firmware", NULL);
+	if (IS_ERR(spi_dir)) {
+		pr_err("Couldn't create firmware securityfs dir\n");
+		return PTR_ERR(spi_dir);
+	}
+
+#define create_file(name, function)                                            \
+	do {                                                                   \
+		spi_##name = securityfs_create_file(#name, 0600, spi_dir,      \
+						    &function, &bc_flags_ops); \
+		if (IS_ERR(spi_##name)) {                                      \
+			pr_err("Error creating securityfs file " #name "\n");  \
+			ret = PTR_ERR(spi_##name);                             \
+			goto out_##name;                                       \
+		}                                                              \
+	} while (0)
+
+	create_file(bioswe, spi_read_bc_bioswe);
+	create_file(ble, spi_read_bc_ble);
+	create_file(smm_bwp, spi_read_bc_smm_bwp);
+
+	return 0;
+
+out_smm_bwp:
+	securityfs_remove(spi_smm_bwp);
+out_ble:
+	securityfs_remove(spi_ble);
+out_bioswe:
+	securityfs_remove(spi_bioswe);
+	securityfs_remove(spi_dir);
+	return ret;
+}
+
+static void __exit mod_exit(void)
+{
+	securityfs_remove(spi_smm_bwp);
+	securityfs_remove(spi_ble);
+	securityfs_remove(spi_bioswe);
+	securityfs_remove(spi_dir);
+}
+
+module_init(mod_init);
+module_exit(mod_exit);
+
+MODULE_DESCRIPTION("SPI LPC flash platform security driver");
+MODULE_AUTHOR("Richard Hughes <richard@hughsie.com>");
+MODULE_AUTHOR("Daniel Gutson <daniel.gutson@eclypsium.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/misc/spi_lpc/viddid_arch_map.c b/drivers/misc/spi_lpc/viddid_arch_map.c
new file mode 100644
index 000000000000..f7955ef70e2b
--- /dev/null
+++ b/drivers/misc/spi_lpc/viddid_arch_map.c
@@ -0,0 +1,566 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SPI LPC flash platform security driver
+ *
+ * Copyright 2020 (c) Daniel Gutson (daniel.gutson@eclypsium.com)
+ *
+ */
+
+#include <linux/module.h>
+
+#include "viddid_arch_map.h"
+
+int viddid2pch_arch(u64 vid, u64 did, enum PCH_Arch *arch)
+{
+	switch (vid) {
+	case 0x8086: /* INTEL */
+		switch (did) {
+		case 0x1c44:
+		case 0x1c46:
+		case 0x1c47:
+		case 0x1c49:
+		case 0x1c4a:
+		case 0x1c4b:
+		case 0x1c4c:
+		case 0x1c4d:
+		case 0x1c4e:
+		case 0x1c4f:
+		case 0x1c50:
+		case 0x1c52:
+		case 0x1c54:
+		case 0x1c56:
+		case 0x1c5c:
+			*arch = pch_6_c200;
+			return 0;
+		case 0x1e47:
+		case 0x1e48:
+		case 0x1e49:
+		case 0x1e44:
+		case 0x1e46:
+		case 0x1e4a:
+		case 0x1e53:
+		case 0x1e55:
+		case 0x1e58:
+		case 0x1e57:
+		case 0x1e59:
+		case 0x1e5d:
+		case 0x1e5e:
+		case 0x1e56:
+			*arch = pch_7_c210;
+			return 0;
+		case 0x1d41:
+			*arch = pch_c60x_x79;
+			return 0;
+		case 0x2390:
+		case 0x2310:
+			*arch = pch_communications_89xx;
+			return 0;
+		case 0x8c41:
+		case 0x8c42:
+		case 0x8c44:
+		case 0x8c46:
+		case 0x8c49:
+		case 0x8c4a:
+		case 0x8c4b:
+		case 0x8c4c:
+		case 0x8c4e:
+		case 0x8c4f:
+		case 0x8c50:
+		case 0x8c52:
+		case 0x8c54:
+		case 0x8c56:
+		case 0x8c5c:
+		case 0x8cc1:
+		case 0x8cc2:
+		case 0x8cc3:
+		case 0x8cc4:
+		case 0x8cc6:
+			*arch = pch_8_c220;
+			return 0;
+		case 0x8d40:
+		case 0x8d44:
+		case 0x8d47:
+			*arch = pch_c61x_x99;
+			return 0;
+		case 0x9cc3:
+		case 0x9cc5:
+		case 0x9cc7:
+		case 0x9cc9:
+		case 0x9cc1:
+		case 0x9cc2:
+		case 0x9cc6:
+			*arch = pch_5_mobile;
+			return 0;
+		case 0x9d41:
+		case 0x9d43:
+		case 0x9d46:
+		case 0x9d48:
+			*arch = pch_6_mobile;
+			return 0;
+		case 0x9d4b:
+		case 0x9d4e:
+		case 0x9d50:
+		case 0x9d53:
+		case 0x9d56:
+		case 0x9d58:
+			*arch = pch_7_8_mobile;
+			return 0;
+		case 0xa141:
+		case 0xa142:
+		case 0xa143:
+		case 0xa144:
+		case 0xa145:
+		case 0xa146:
+		case 0xa147:
+		case 0xa148:
+		case 0xa149:
+		case 0xa14a:
+		case 0xa14b:
+		case 0xa14c:
+		case 0xa14d:
+		case 0xa14e:
+		case 0xa14f:
+		case 0xa150:
+		case 0xa151:
+		case 0xa152:
+		case 0xa153:
+		case 0xa154:
+		case 0xa155:
+		case 0xa156:
+		case 0xa157:
+		case 0xa158:
+		case 0xa159:
+		case 0xa15a:
+		case 0xa15b:
+		case 0xa15c:
+		case 0xa15d:
+		case 0xa15e:
+		case 0xa15f:
+			*arch = pch_1xx;
+			return 0;
+		case 0xa1c1:
+		case 0xa1c2:
+		case 0xa1c3:
+		case 0xa1c4:
+		case 0xa1c5:
+		case 0xa1c6:
+		case 0xa1c7:
+		case 0xa242:
+		case 0xa243:
+		case 0xa244:
+		case 0xa245:
+		case 0xa246:
+			*arch = pch_c620;
+			return 0;
+		case 0xa2c0:
+		case 0xa2c1:
+		case 0xa2c2:
+		case 0xa2c3:
+		case 0xa2c4:
+		case 0xa2c5:
+		case 0xa2c6:
+		case 0xa2c7:
+		case 0xa2c8:
+		case 0xa2c9:
+		case 0xa2ca:
+		case 0xa2cb:
+		case 0xa2cc:
+		case 0xa2cd:
+		case 0xa2ce:
+		case 0xa2cf:
+		case 0xa2d2:
+		case 0xa2d3:
+			*arch = pch_2xx;
+			return 0;
+		case 0xa300:
+		case 0xa301:
+		case 0xa302:
+		case 0xa303:
+		case 0xa304:
+		case 0xa305:
+		case 0xa306:
+		case 0xa307:
+		case 0xa308:
+		case 0xa309:
+		case 0xa30a:
+		case 0xa30b:
+		case 0xa30c:
+		case 0xa30d:
+		case 0xa30e:
+		case 0xa30f:
+		case 0xa310:
+		case 0xa311:
+		case 0xa312:
+		case 0xa313:
+		case 0xa314:
+		case 0xa315:
+		case 0xa316:
+		case 0xa317:
+		case 0xa318:
+		case 0xa319:
+		case 0xa31a:
+		case 0xa31b:
+		case 0xa31c:
+		case 0xa31d:
+		case 0xa31e:
+		case 0xa31f:
+		case 0x9d81:
+		case 0x9d83:
+		case 0x9d84:
+		case 0x9d85:
+		case 0x9d86:
+			*arch = pch_3xx;
+			return 0;
+		case 0x280:
+		case 0x281:
+		case 0x282:
+		case 0x283:
+		case 0x284:
+		case 0x285:
+		case 0x286:
+		case 0x287:
+		case 0x288:
+		case 0x289:
+		case 0x28a:
+		case 0x28b:
+		case 0x28c:
+		case 0x28d:
+		case 0x28e:
+		case 0x28f:
+		case 0x290:
+		case 0x291:
+		case 0x292:
+		case 0x293:
+		case 0x294:
+		case 0x295:
+		case 0x296:
+		case 0x297:
+		case 0x298:
+		case 0x299:
+		case 0x29a:
+		case 0x29b:
+		case 0x29c:
+		case 0x29d:
+		case 0x29e:
+		case 0x29f:
+		case 0x680:
+		case 0x681:
+		case 0x682:
+		case 0x683:
+		case 0x684:
+		case 0x685:
+		case 0x686:
+		case 0x687:
+		case 0x688:
+		case 0x689:
+		case 0x68a:
+		case 0x68b:
+		case 0x68c:
+		case 0x68d:
+		case 0x68e:
+		case 0x68f:
+		case 0x690:
+		case 0x691:
+		case 0x692:
+		case 0x693:
+		case 0x694:
+		case 0x695:
+		case 0x696:
+		case 0x697:
+		case 0x698:
+		case 0x699:
+		case 0x69a:
+		case 0x69b:
+		case 0x69c:
+		case 0x69d:
+		case 0x69e:
+		case 0x69f:
+		case 0xa3c1:
+		case 0xa3c8:
+		case 0xa3da:
+			*arch = pch_4xx;
+			return 0;
+		case 0x3480:
+		case 0x3481:
+		case 0x3482:
+		case 0x3483:
+		case 0x3484:
+		case 0x3485:
+		case 0x3486:
+		case 0x3487:
+		case 0x3488:
+		case 0x3489:
+		case 0x348a:
+		case 0x348b:
+		case 0x348c:
+		case 0x348d:
+		case 0x348e:
+		case 0x348f:
+		case 0x3490:
+		case 0x3491:
+		case 0x3492:
+		case 0x3493:
+		case 0x3494:
+		case 0x3495:
+		case 0x3496:
+		case 0x3497:
+		case 0x3498:
+		case 0x3499:
+		case 0x349a:
+		case 0x349b:
+		case 0x349c:
+		case 0x349d:
+		case 0x349e:
+		case 0x349f:
+		case 0x3887:
+			*arch = pch_495;
+			return 0;
+		case 0x4380:
+		case 0x4381:
+		case 0x4382:
+		case 0x4383:
+		case 0x4384:
+		case 0x4385:
+		case 0x4386:
+		case 0x4387:
+		case 0x4388:
+		case 0x4389:
+		case 0x438a:
+		case 0x438b:
+		case 0x438c:
+		case 0x438d:
+		case 0x438e:
+		case 0x438f:
+		case 0x4390:
+		case 0x4391:
+		case 0x4392:
+		case 0x4393:
+		case 0x4394:
+		case 0x4395:
+		case 0x4396:
+		case 0x4397:
+		case 0x4398:
+		case 0x4399:
+		case 0x439a:
+		case 0x439b:
+		case 0x439c:
+		case 0x439d:
+		case 0x439e:
+		case 0x439f:
+		case 0xa080:
+		case 0xa081:
+		case 0xa082:
+		case 0xa083:
+		case 0xa084:
+		case 0xa085:
+		case 0xa086:
+		case 0xa087:
+		case 0xa088:
+		case 0xa089:
+		case 0xa08a:
+		case 0xa08b:
+		case 0xa08c:
+		case 0xa08d:
+		case 0xa08e:
+		case 0xa08f:
+		case 0xa090:
+		case 0xa091:
+		case 0xa092:
+		case 0xa093:
+		case 0xa094:
+		case 0xa095:
+		case 0xa096:
+		case 0xa097:
+		case 0xa098:
+		case 0xa099:
+		case 0xa09a:
+		case 0xa09b:
+		case 0xa09c:
+		case 0xa09d:
+		case 0xa09e:
+		case 0xa09f:
+			*arch = pch_5xx;
+			return 0;
+		default:
+			*arch = pch_none;
+			return -EIO; /* DID not found */
+		}
+	case 0x1022: /* AMD */
+		switch (did) {
+		default:
+			*arch = pch_none;
+			return -EIO; /* DID not found */
+		}
+	default:
+		return -EIO; /* VID not supported */
+	}
+}
+
+int viddid2cpu_arch(u64 vid, u64 did, enum CPU_Arch *arch)
+{
+	switch (vid) {
+	case 0x8086: /* INTEL */
+		switch (did) {
+		case 0x1600:
+		case 0x1604:
+		case 0x1610:
+		case 0x1614:
+		case 0x1618:
+			*arch = cpu_bdw;
+			return 0;
+		case 0x6f00:
+			*arch = cpu_bdx;
+			return 0;
+		case 0xa00:
+		case 0xa04:
+		case 0xa08:
+		case 0xc00:
+		case 0xc04:
+		case 0xc08:
+		case 0xd00:
+		case 0xd04:
+		case 0xd08:
+			*arch = cpu_hsw;
+			return 0;
+		case 0x2f00:
+			*arch = cpu_hsx;
+			return 0;
+		case 0xe00:
+			*arch = cpu_ivt;
+			return 0;
+		case 0x3c00:
+			*arch = cpu_jkt;
+			return 0;
+		case 0x5900:
+		case 0x5904:
+		case 0x590c:
+		case 0x590f:
+		case 0x5910:
+		case 0x5914:
+		case 0x5918:
+		case 0x591f:
+			*arch = cpu_kbl;
+			return 0;
+		case 0x1900:
+		case 0x1904:
+		case 0x190c:
+		case 0x190f:
+		case 0x1910:
+		case 0x191f:
+		case 0x1918:
+		case 0x2020:
+			*arch = cpu_skl;
+			return 0;
+		case 0x150:
+		case 0x154:
+		case 0x158:
+			*arch = cpu_ivb;
+			return 0;
+		case 0x100:
+		case 0x104:
+		case 0x108:
+			*arch = cpu_snb;
+			return 0;
+		case 0x1f00:
+		case 0x1f01:
+		case 0x1f02:
+		case 0x1f03:
+		case 0x1f04:
+		case 0x1f05:
+		case 0x1f06:
+		case 0x1f07:
+		case 0x1f08:
+		case 0x1f09:
+		case 0x1f0a:
+		case 0x1f0b:
+		case 0x1f0c:
+		case 0x1f0d:
+		case 0x1f0e:
+		case 0x1f0f:
+			*arch = cpu_avn;
+			return 0;
+		case 0x3e0f:
+		case 0x3e10:
+		case 0x3e18:
+		case 0x3e1f:
+		case 0x3e30:
+		case 0x3e31:
+		case 0x3e32:
+		case 0x3e33:
+		case 0x3ec2:
+		case 0x3ec4:
+		case 0x3ec6:
+		case 0x3eca:
+		case 0x3ecc:
+		case 0x3ed0:
+			*arch = cpu_cfl;
+			return 0;
+		case 0xf00:
+			*arch = cpu_byt;
+			return 0;
+		case 0x3e34:
+		case 0x3e35:
+			*arch = cpu_whl;
+			return 0;
+		case 0x3e20:
+		case 0x9b33:
+		case 0x9b43:
+		case 0x9b44:
+		case 0x9b51:
+		case 0x9b53:
+		case 0x9b54:
+		case 0x9b61:
+		case 0x9b63:
+		case 0x9b64:
+		case 0x9b71:
+		case 0x9b73:
+			*arch = cpu_cml;
+			return 0;
+		case 0x8a00:
+		case 0x8a02:
+		case 0x8a10:
+		case 0x8a12:
+		case 0x8a16:
+			*arch = cpu_icl;
+			return 0;
+		case 0x5af0:
+			*arch = cpu_apl;
+			return 0;
+		case 0x3180:
+		case 0x31f0:
+			*arch = cpu_glk;
+			return 0;
+		case 0x9a02:
+		case 0x9a04:
+		case 0x9a12:
+		case 0x9a14:
+		case 0x9a26:
+		case 0x9a36:
+			*arch = cpu_tgl;
+			return 0;
+		default:
+			*arch = cpu_none;
+			return -EIO; /* DID not found */
+		}
+	case 0x1022: /* AMD */
+		switch (did) {
+		case 0x1410:
+		case 0x1422:
+		case 0x1576:
+		case 0x1510:
+		case 0x1514:
+		case 0x1536:
+		case 0x1566:
+		case 0x1450:
+		case 0x1480:
+		case 0x15d0:
+			*arch = cpu_amd;
+			return 0;
+		default:
+			*arch = cpu_none;
+			return -EIO; /* DID not found */
+		}
+	default:
+		return -EIO; /* VID not supported */
+	}
+}
diff --git a/drivers/misc/spi_lpc/viddid_arch_map.h b/drivers/misc/spi_lpc/viddid_arch_map.h
new file mode 100644
index 000000000000..910f5372462d
--- /dev/null
+++ b/drivers/misc/spi_lpc/viddid_arch_map.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * SPI LPC flash platform security driver
+ *
+ * Copyright 2020 (c) Daniel Gutson (daniel.gutson@eclypsium.com)
+ *
+ */
+
+#ifndef VIDDID_ARCH_MAP_H
+#define VIDDID_ARCH_MAP_H
+
+#include "bios_data_access.h"
+
+int viddid2pch_arch(u64 vid, u64 did, enum PCH_Arch *arch);
+int viddid2cpu_arch(u64 vid, u64 did, enum CPU_Arch *arch);
+
+#endif /* VIDDID_ARCH_MAP_H */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-29 22:59 [PATCH] SPI LPC information kernel module Daniel Gutson
@ 2020-06-30  8:02 ` kernel test robot
  2020-06-30  8:56 ` Greg Kroah-Hartman
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: kernel test robot @ 2020-06-30  8:02 UTC (permalink / raw)
  To: Daniel Gutson, Derek Kiernan, Arnd Bergmann, Greg Kroah-Hartman,
	Mauro Carvalho Chehab, linux-kernel, Richard Hughes,
	Alex Bazhaniuk
  Cc: kbuild-all, linux-media

[-- Attachment #1: Type: text/plain, Size: 39089 bytes --]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on soc/for-next linus/master v5.8-rc3 next-20200629]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Gutson/SPI-LPC-information-kernel-module/20200630-070234
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git ba2104c24aba1fa7e19d53f08c985526a6786d8b
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/misc/vmw_vmci/vmci_context.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'mmio_read_byte':
>> <command-line>: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
>> drivers/misc/spi_lpc/low_level_access.c:8:21: note: in expansion of macro 'KBUILD_MODNAME'
       8 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in expansion of macro 'pr_fmt'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     143 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:153:2: note: in expansion of macro '_dynamic_func_call'
     153 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:419:2: note: in expansion of macro 'dynamic_pr_debug'
     419 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:20:3: note: in expansion of macro 'pr_debug'
      20 |   pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
         |   ^~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:32:1: note: in expansion of macro 'GENERIC_MMIO_READ'
      32 | GENERIC_MMIO_READ(u8, byte, readb)
         | ^~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'mmio_read_word':
>> <command-line>: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
>> drivers/misc/spi_lpc/low_level_access.c:8:21: note: in expansion of macro 'KBUILD_MODNAME'
       8 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in expansion of macro 'pr_fmt'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     143 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:153:2: note: in expansion of macro '_dynamic_func_call'
     153 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:419:2: note: in expansion of macro 'dynamic_pr_debug'
     419 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:20:3: note: in expansion of macro 'pr_debug'
      20 |   pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
         |   ^~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:33:1: note: in expansion of macro 'GENERIC_MMIO_READ'
      33 | GENERIC_MMIO_READ(u16, word, readw)
         | ^~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'mmio_read_dword':
>> <command-line>: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
>> drivers/misc/spi_lpc/low_level_access.c:8:21: note: in expansion of macro 'KBUILD_MODNAME'
       8 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in expansion of macro 'pr_fmt'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     143 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:153:2: note: in expansion of macro '_dynamic_func_call'
     153 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:419:2: note: in expansion of macro 'dynamic_pr_debug'
     419 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:20:3: note: in expansion of macro 'pr_debug'
      20 |   pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
         |   ^~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:34:1: note: in expansion of macro 'GENERIC_MMIO_READ'
      34 | GENERIC_MMIO_READ(u32, dword, readl)
         | ^~~~~~~~~~~~~~~~~
   drivers/misc/ibmasm/event.c:44: warning: Function parameter or member 'sp' not described in 'ibmasm_receive_event'
   drivers/misc/ibmasm/event.c:44: warning: Function parameter or member 'data' not described in 'ibmasm_receive_event'
   drivers/misc/ibmasm/event.c:44: warning: Function parameter or member 'data_size' not described in 'ibmasm_receive_event'
   drivers/misc/ibmasm/event.c:78: warning: Function parameter or member 'sp' not described in 'ibmasm_get_next_event'
   drivers/misc/ibmasm/event.c:78: warning: Function parameter or member 'reader' not described in 'ibmasm_get_next_event'
   drivers/misc/ibmasm/command.c:106: warning: Function parameter or member 'sp' not described in 'ibmasm_exec_command'
   drivers/misc/ibmasm/command.c:106: warning: Function parameter or member 'cmd' not described in 'ibmasm_exec_command'
   drivers/misc/ibmasm/command.c:149: warning: Function parameter or member 'cmd' not described in 'ibmasm_wait_for_response'
   drivers/misc/ibmasm/command.c:149: warning: Function parameter or member 'timeout' not described in 'ibmasm_wait_for_response'
   drivers/misc/ibmasm/command.c:162: warning: Function parameter or member 'sp' not described in 'ibmasm_receive_command_response'
   drivers/misc/ibmasm/command.c:162: warning: Function parameter or member 'response' not described in 'ibmasm_receive_command_response'
   drivers/misc/ibmasm/command.c:162: warning: Function parameter or member 'size' not described in 'ibmasm_receive_command_response'
   In file included from drivers/misc/vmw_vmci/vmci_datagram.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/pti.c:747: warning: Function parameter or member 'port' not described in 'pti_port_activate'
   drivers/misc/pti.c:747: warning: Function parameter or member 'tty' not described in 'pti_port_activate'
   drivers/misc/pti.c:764: warning: Function parameter or member 'port' not described in 'pti_port_shutdown'
   drivers/misc/pti.c:792: warning: Function parameter or member 'pdev' not described in 'pti_pci_probe'
   drivers/misc/pti.c:792: warning: Function parameter or member 'ent' not described in 'pti_pci_probe'
   drivers/misc/pti.c:901: warning: Cannot understand  *
    on line 901 - I thought it was a doc line
   In file included from drivers/misc/vmw_vmci/vmci_driver.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/ibmasm/r_heartbeat.c:49: warning: Function parameter or member 'sp' not described in 'ibmasm_start_reverse_heartbeat'
   drivers/misc/ibmasm/r_heartbeat.c:49: warning: Function parameter or member 'rhb' not described in 'ibmasm_start_reverse_heartbeat'
   In file included from drivers/misc/vmw_vmci/vmci_handle_array.h:11,
                    from drivers/misc/vmw_vmci/vmci_handle_array.c:9:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/misc/vmw_vmci/vmci_event.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/ibmasm/dot_command.c:18: warning: Function parameter or member 'sp' not described in 'ibmasm_receive_message'
   drivers/misc/ibmasm/dot_command.c:18: warning: Function parameter or member 'message' not described in 'ibmasm_receive_message'
   drivers/misc/ibmasm/dot_command.c:18: warning: Function parameter or member 'message_size' not described in 'ibmasm_receive_message'
   drivers/misc/ibmasm/dot_command.c:55: warning: Function parameter or member 'sp' not described in 'ibmasm_send_driver_vpd'
   drivers/misc/ibmasm/dot_command.c:111: warning: Function parameter or member 'sp' not described in 'ibmasm_send_os_state'
   drivers/misc/ibmasm/dot_command.c:111: warning: Function parameter or member 'os_state' not described in 'ibmasm_send_os_state'
   In file included from drivers/misc/vmw_vmci/vmci_resource.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/misc/vmw_vmci/vmci_route.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/misc/vmw_vmci/vmci_host.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/habanalabs/irq.c:24: warning: Function parameter or member 'eq_work' not described in 'hl_eqe_work'
   drivers/misc/habanalabs/irq.c:24: warning: Function parameter or member 'hdev' not described in 'hl_eqe_work'
   drivers/misc/habanalabs/irq.c:24: warning: Function parameter or member 'eq_entry' not described in 'hl_eqe_work'
   drivers/misc/enclosure.c:115: warning: Function parameter or member 'name' not described in 'enclosure_register'
   drivers/misc/enclosure.c:115: warning: Function parameter or member 'cb' not described in 'enclosure_register'
   drivers/misc/enclosure.c:283: warning: Function parameter or member 'number' not described in 'enclosure_component_alloc'
   drivers/misc/enclosure.c:283: warning: Excess function parameter 'num' description in 'enclosure_component_alloc'
   drivers/misc/enclosure.c:363: warning: Function parameter or member 'component' not described in 'enclosure_add_device'
   drivers/misc/enclosure.c:363: warning: Excess function parameter 'num' description in 'enclosure_add_device'
   drivers/misc/enclosure.c:398: warning: Function parameter or member 'dev' not described in 'enclosure_remove_device'
   drivers/misc/enclosure.c:398: warning: Excess function parameter 'num' description in 'enclosure_remove_device'
   drivers/misc/lattice-ecp3-config.c: In function 'firmware_load':
   drivers/misc/lattice-ecp3-config.c:70:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
      70 |  int ret;
         |      ^~~
   drivers/misc/pch_phub.c:145: warning: Function parameter or member 'chip' not described in 'pch_phub_read_modify_write_reg'
   drivers/misc/pch_phub.c:280: warning: Function parameter or member 'chip' not described in 'pch_phub_read_serial_rom'
   drivers/misc/pch_phub.c:294: warning: Function parameter or member 'chip' not described in 'pch_phub_write_serial_rom'
   drivers/misc/pch_phub.c:332: warning: Function parameter or member 'chip' not described in 'pch_phub_read_serial_rom_val'
   drivers/misc/pch_phub.c:348: warning: Function parameter or member 'chip' not described in 'pch_phub_write_serial_rom_val'
   drivers/misc/pch_phub.c:448: warning: Function parameter or member 'chip' not described in 'pch_phub_read_gbe_mac_addr'
   drivers/misc/pch_phub.c:448: warning: Excess function parameter 'offset_address' description in 'pch_phub_read_gbe_mac_addr'
   drivers/misc/pch_phub.c:460: warning: Function parameter or member 'chip' not described in 'pch_phub_write_gbe_mac_addr'
   drivers/misc/pch_phub.c:460: warning: Excess function parameter 'offset_address' description in 'pch_phub_write_gbe_mac_addr'
   In file included from drivers/misc/vmw_balloon.c:35:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/habanalabs/firmware_if.c:26: warning: Function parameter or member 'fw_name' not described in 'hl_fw_load_fw_to_device'
   drivers/misc/habanalabs/firmware_if.c:26: warning: Function parameter or member 'dst' not described in 'hl_fw_load_fw_to_device'
   drivers/misc/habanalabs/goya/goya.c: In function 'goya_debugfs_read32':
   drivers/misc/habanalabs/goya/goya.c:3945:19: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
    3945 |  } else if ((addr >= DRAM_PHYS_BASE) &&
         |                   ^~
   drivers/misc/habanalabs/goya/goya.c: In function 'goya_debugfs_write32':
   drivers/misc/habanalabs/goya/goya.c:4002:19: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
    4002 |  } else if ((addr >= DRAM_PHYS_BASE) &&
         |                   ^~
   drivers/misc/habanalabs/goya/goya.c: In function 'goya_debugfs_read64':
--
   drivers/misc/spi_lpc/low_level_access.c: In function 'mmio_read_byte':
>> <command-line>: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
>> drivers/misc/spi_lpc/low_level_access.c:8:21: note: in expansion of macro 'KBUILD_MODNAME'
       8 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in expansion of macro 'pr_fmt'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     143 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:153:2: note: in expansion of macro '_dynamic_func_call'
     153 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:419:2: note: in expansion of macro 'dynamic_pr_debug'
     419 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:20:3: note: in expansion of macro 'pr_debug'
      20 |   pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
         |   ^~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:32:1: note: in expansion of macro 'GENERIC_MMIO_READ'
      32 | GENERIC_MMIO_READ(u8, byte, readb)
         | ^~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'mmio_read_word':
>> <command-line>: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
>> drivers/misc/spi_lpc/low_level_access.c:8:21: note: in expansion of macro 'KBUILD_MODNAME'
       8 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in expansion of macro 'pr_fmt'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     143 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:153:2: note: in expansion of macro '_dynamic_func_call'
     153 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:419:2: note: in expansion of macro 'dynamic_pr_debug'
     419 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:20:3: note: in expansion of macro 'pr_debug'
      20 |   pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
         |   ^~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:33:1: note: in expansion of macro 'GENERIC_MMIO_READ'
      33 | GENERIC_MMIO_READ(u16, word, readw)
         | ^~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'mmio_read_dword':
>> <command-line>: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
>> drivers/misc/spi_lpc/low_level_access.c:8:21: note: in expansion of macro 'KBUILD_MODNAME'
       8 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in expansion of macro 'pr_fmt'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     143 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:153:2: note: in expansion of macro '_dynamic_func_call'
     153 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:419:2: note: in expansion of macro 'dynamic_pr_debug'
     419 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:20:3: note: in expansion of macro 'pr_debug'
      20 |   pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
         |   ^~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:34:1: note: in expansion of macro 'GENERIC_MMIO_READ'
      34 | GENERIC_MMIO_READ(u32, dword, readl)
         | ^~~~~~~~~~~~~~~~~
--
   drivers/misc/ibmasm/command.c:106: warning: Function parameter or member 'sp' not described in 'ibmasm_exec_command'
   drivers/misc/ibmasm/command.c:106: warning: Function parameter or member 'cmd' not described in 'ibmasm_exec_command'
   drivers/misc/ibmasm/command.c:149: warning: Function parameter or member 'cmd' not described in 'ibmasm_wait_for_response'
   drivers/misc/ibmasm/command.c:149: warning: Function parameter or member 'timeout' not described in 'ibmasm_wait_for_response'
   drivers/misc/ibmasm/command.c:162: warning: Function parameter or member 'sp' not described in 'ibmasm_receive_command_response'
   drivers/misc/ibmasm/command.c:162: warning: Function parameter or member 'response' not described in 'ibmasm_receive_command_response'
   drivers/misc/ibmasm/command.c:162: warning: Function parameter or member 'size' not described in 'ibmasm_receive_command_response'
   drivers/misc/ibmasm/event.c:44: warning: Function parameter or member 'sp' not described in 'ibmasm_receive_event'
   drivers/misc/ibmasm/event.c:44: warning: Function parameter or member 'data' not described in 'ibmasm_receive_event'
   drivers/misc/ibmasm/event.c:44: warning: Function parameter or member 'data_size' not described in 'ibmasm_receive_event'
   drivers/misc/ibmasm/event.c:78: warning: Function parameter or member 'sp' not described in 'ibmasm_get_next_event'
   drivers/misc/ibmasm/event.c:78: warning: Function parameter or member 'reader' not described in 'ibmasm_get_next_event'
   drivers/misc/pti.c:747: warning: Function parameter or member 'port' not described in 'pti_port_activate'
   drivers/misc/pti.c:747: warning: Function parameter or member 'tty' not described in 'pti_port_activate'
   drivers/misc/pti.c:764: warning: Function parameter or member 'port' not described in 'pti_port_shutdown'
   drivers/misc/pti.c:792: warning: Function parameter or member 'pdev' not described in 'pti_pci_probe'
   drivers/misc/pti.c:792: warning: Function parameter or member 'ent' not described in 'pti_pci_probe'
   drivers/misc/pti.c:901: warning: Cannot understand  *
    on line 901 - I thought it was a doc line
   drivers/misc/enclosure.c:115: warning: Function parameter or member 'name' not described in 'enclosure_register'
   drivers/misc/enclosure.c:115: warning: Function parameter or member 'cb' not described in 'enclosure_register'
   drivers/misc/enclosure.c:283: warning: Function parameter or member 'number' not described in 'enclosure_component_alloc'
   drivers/misc/enclosure.c:283: warning: Excess function parameter 'num' description in 'enclosure_component_alloc'
   drivers/misc/enclosure.c:363: warning: Function parameter or member 'component' not described in 'enclosure_add_device'
   drivers/misc/enclosure.c:363: warning: Excess function parameter 'num' description in 'enclosure_add_device'
   drivers/misc/enclosure.c:398: warning: Function parameter or member 'dev' not described in 'enclosure_remove_device'
   drivers/misc/enclosure.c:398: warning: Excess function parameter 'num' description in 'enclosure_remove_device'
   drivers/misc/ibmasm/r_heartbeat.c:49: warning: Function parameter or member 'sp' not described in 'ibmasm_start_reverse_heartbeat'
   drivers/misc/ibmasm/r_heartbeat.c:49: warning: Function parameter or member 'rhb' not described in 'ibmasm_start_reverse_heartbeat'
   drivers/misc/ibmasm/dot_command.c:18: warning: Function parameter or member 'sp' not described in 'ibmasm_receive_message'
   drivers/misc/ibmasm/dot_command.c:18: warning: Function parameter or member 'message' not described in 'ibmasm_receive_message'
   drivers/misc/ibmasm/dot_command.c:18: warning: Function parameter or member 'message_size' not described in 'ibmasm_receive_message'
   drivers/misc/ibmasm/dot_command.c:55: warning: Function parameter or member 'sp' not described in 'ibmasm_send_driver_vpd'
   drivers/misc/ibmasm/dot_command.c:111: warning: Function parameter or member 'sp' not described in 'ibmasm_send_os_state'
   drivers/misc/ibmasm/dot_command.c:111: warning: Function parameter or member 'os_state' not described in 'ibmasm_send_os_state'
   In file included from drivers/misc/vmw_balloon.c:35:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/lattice-ecp3-config.c: In function 'firmware_load':
   drivers/misc/lattice-ecp3-config.c:70:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
      70 |  int ret;
         |      ^~~
   In file included from drivers/misc/vmw_vmci/vmci_context.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/pch_phub.c:145: warning: Function parameter or member 'chip' not described in 'pch_phub_read_modify_write_reg'
   drivers/misc/pch_phub.c:280: warning: Function parameter or member 'chip' not described in 'pch_phub_read_serial_rom'
   drivers/misc/pch_phub.c:294: warning: Function parameter or member 'chip' not described in 'pch_phub_write_serial_rom'
   drivers/misc/pch_phub.c:332: warning: Function parameter or member 'chip' not described in 'pch_phub_read_serial_rom_val'
   drivers/misc/pch_phub.c:348: warning: Function parameter or member 'chip' not described in 'pch_phub_write_serial_rom_val'
   drivers/misc/pch_phub.c:448: warning: Function parameter or member 'chip' not described in 'pch_phub_read_gbe_mac_addr'
   drivers/misc/pch_phub.c:448: warning: Excess function parameter 'offset_address' description in 'pch_phub_read_gbe_mac_addr'
   drivers/misc/pch_phub.c:460: warning: Function parameter or member 'chip' not described in 'pch_phub_write_gbe_mac_addr'
   drivers/misc/pch_phub.c:460: warning: Excess function parameter 'offset_address' description in 'pch_phub_write_gbe_mac_addr'
   In file included from drivers/misc/vmw_vmci/vmci_datagram.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/vmw_balloon.c:262: warning: Function parameter or member '5' not described in 'vmballoon_batch_entry'
   In file included from drivers/misc/vmw_vmci/vmci_event.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/misc/vmw_vmci/vmci_driver.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/misc/vmw_vmci/vmci_handle_array.h:11,
                    from drivers/misc/vmw_vmci/vmci_handle_array.c:9:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/misc/vmw_vmci/vmci_host.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/misc/vmw_vmci/vmci_resource.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/misc/vmw_vmci/vmci_route.c:8:
   include/linux/vmw_vmci_defs.h:162:33: warning: 'VMCI_ANON_SRC_HANDLE' defined but not used [-Wunused-const-variable=]
     162 | static const struct vmci_handle VMCI_ANON_SRC_HANDLE = {
         |                                 ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/misc/mei/mei-trace.h:83,
                    from drivers/misc/mei/mei-trace.c:11:
   include/trace/define_trace.h:95:42: fatal error: ./mei-trace.h: No such file or directory
      95 | #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
         |                                          ^
   compilation terminated.
   make[3]: *** [scripts/Makefile.build:281: drivers/misc/mei/mei-trace.o] Error 1
   drivers/misc/spi_lpc/low_level_access.c: In function 'mmio_read_byte':
>> <command-line>: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
>> drivers/misc/spi_lpc/low_level_access.c:8:21: note: in expansion of macro 'KBUILD_MODNAME'
       8 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in expansion of macro 'pr_fmt'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     143 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:153:2: note: in expansion of macro '_dynamic_func_call'
     153 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:419:2: note: in expansion of macro 'dynamic_pr_debug'
     419 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:20:3: note: in expansion of macro 'pr_debug'
      20 |   pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
         |   ^~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:32:1: note: in expansion of macro 'GENERIC_MMIO_READ'
      32 | GENERIC_MMIO_READ(u8, byte, readb)
         | ^~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'mmio_read_word':
>> <command-line>: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
>> drivers/misc/spi_lpc/low_level_access.c:8:21: note: in expansion of macro 'KBUILD_MODNAME'
       8 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in expansion of macro 'pr_fmt'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     143 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:153:2: note: in expansion of macro '_dynamic_func_call'
     153 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:419:2: note: in expansion of macro 'dynamic_pr_debug'
     419 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:20:3: note: in expansion of macro 'pr_debug'
      20 |   pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
         |   ^~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:33:1: note: in expansion of macro 'GENERIC_MMIO_READ'
      33 | GENERIC_MMIO_READ(u16, word, readw)
         | ^~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'mmio_read_dword':
>> <command-line>: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
>> drivers/misc/spi_lpc/low_level_access.c:8:21: note: in expansion of macro 'KBUILD_MODNAME'
       8 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in expansion of macro 'pr_fmt'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     143 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:153:2: note: in expansion of macro '_dynamic_func_call'
     153 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:419:2: note: in expansion of macro 'dynamic_pr_debug'
     419 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:20:3: note: in expansion of macro 'pr_debug'
      20 |   pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
         |   ^~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:34:1: note: in expansion of macro 'GENERIC_MMIO_READ'
      34 | GENERIC_MMIO_READ(u32, dword, readl)
         | ^~~~~~~~~~~~~~~~~
   In file included from drivers/misc/habanalabs/goya/goya.c:8:
   drivers/misc/habanalabs/goya/goyaP.h:12:10: fatal error: habanalabs.h: No such file or directory
      12 | #include "habanalabs.h"
         |          ^~~~~~~~~~~~~~
   compilation terminated.
   make[3]: *** [scripts/Makefile.build:280: drivers/misc/habanalabs/goya/goya.o] Error 1
   In file included from drivers/misc/habanalabs/goya/goya_security.c:8:
   drivers/misc/habanalabs/goya/goyaP.h:12:10: fatal error: habanalabs.h: No such file or directory
      12 | #include "habanalabs.h"
         |          ^~~~~~~~~~~~~~
   compilation terminated.
   make[3]: *** [scripts/Makefile.build:281: drivers/misc/habanalabs/goya/goya_security.o] Error 1
   In file included from drivers/misc/habanalabs/goya/goya_hwmgr.c:8:
   drivers/misc/habanalabs/goya/goyaP.h:12:10: fatal error: habanalabs.h: No such file or directory
      12 | #include "habanalabs.h"
         |          ^~~~~~~~~~~~~~
   compilation terminated.
   make[3]: *** [scripts/Makefile.build:281: drivers/misc/habanalabs/goya/goya_hwmgr.o] Error 1
   In file included from drivers/misc/habanalabs/goya/goya_coresight.c:8:
   drivers/misc/habanalabs/goya/goyaP.h:12:10: fatal error: habanalabs.h: No such file or directory
      12 | #include "habanalabs.h"
         |          ^~~~~~~~~~~~~~
   compilation terminated.
   make[3]: *** [scripts/Makefile.build:281: drivers/misc/habanalabs/goya/goya_coresight.o] Error 1
   In file included from drivers/misc/habanalabs/gaudi/gaudi.c:8:
   drivers/misc/habanalabs/gaudi/gaudiP.h:12:10: fatal error: habanalabs.h: No such file or directory
      12 | #include "habanalabs.h"
         |          ^~~~~~~~~~~~~~
   compilation terminated.
   make[3]: *** [scripts/Makefile.build:281: drivers/misc/habanalabs/gaudi/gaudi.o] Error 1
   make[3]: *** [scripts/Makefile.build:281: drivers/misc/habanalabs/gaudi/gaudi_security.o] Error 1
   In file included from drivers/misc/habanalabs/gaudi/gaudi_hwmgr.c:8:
   drivers/misc/habanalabs/gaudi/gaudiP.h:12:10: fatal error: habanalabs.h: No such file or directory
      12 | #include "habanalabs.h"
         |          ^~~~~~~~~~~~~~
   compilation terminated.
   In file included from drivers/misc/habanalabs/gaudi/gaudi_security.c:8:
   drivers/misc/habanalabs/gaudi/gaudiP.h:12:10: fatal error: habanalabs.h: No such file or directory
      12 | #include "habanalabs.h"
         |          ^~~~~~~~~~~~~~
   compilation terminated.
   make[3]: *** [scripts/Makefile.build:281: drivers/misc/habanalabs/gaudi/gaudi_hwmgr.o] Error 1
   In file included from drivers/misc/habanalabs/gaudi/gaudi_coresight.c:8:
   drivers/misc/habanalabs/gaudi/gaudiP.h:12:10: fatal error: habanalabs.h: No such file or directory
      12 | #include "habanalabs.h"
         |          ^~~~~~~~~~~~~~
   compilation terminated.
   make[3]: *** [scripts/Makefile.build:281: drivers/misc/habanalabs/gaudi/gaudi_coresight.o] Error 1
   drivers/misc/habanalabs/irq.c:24: warning: Function parameter or member 'eq_work' not described in 'hl_eqe_work'
   drivers/misc/habanalabs/irq.c:24: warning: Function parameter or member 'hdev' not described in 'hl_eqe_work'
   drivers/misc/habanalabs/irq.c:24: warning: Function parameter or member 'eq_entry' not described in 'hl_eqe_work'
   make[2]: *** [scripts/Makefile.build:497: drivers/misc/mei] Error 2
   make[3]: Target '__build' not remade because of errors.
   drivers/misc/habanalabs/pci.c:27: warning: Function parameter or member 'name' not described in 'hl_pci_bars_map'
   drivers/misc/habanalabs/pci.c:27: warning: Excess function parameter 'bar_name' description in 'hl_pci_bars_map'
   drivers/misc/habanalabs/pci.c:147: warning: Function parameter or member 'addr' not described in 'hl_pci_iatu_write'
   drivers/misc/habanalabs/pci.c:147: warning: Function parameter or member 'data' not described in 'hl_pci_iatu_write'
   drivers/misc/habanalabs/pci.c:324: warning: Excess function parameter 'dma_mask' description in 'hl_pci_set_dma_mask'
   drivers/misc/habanalabs/firmware_if.c:26: warning: Function parameter or member 'fw_name' not described in 'hl_fw_load_fw_to_device'
   drivers/misc/habanalabs/firmware_if.c:26: warning: Function parameter or member 'dst' not described in 'hl_fw_load_fw_to_device'
   make[3]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1756: drivers/misc] Error 2
   make[1]: Target '__all' not remade because of errors.
   make[2]: *** [scripts/Makefile.build:497: drivers/misc/habanalabs] Error 2
   make[2]: Target '__build' not remade because of errors.
..

vim +/KBUILD_MODNAME +8 drivers/misc/spi_lpc/low_level_access.c

   > 8	#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
     9	
    10	#include <linux/version.h>
    11	#include <linux/pci.h>
    12	#include "low_level_access.h"
    13	
    14	#define GENERIC_MMIO_READ(Type, Suffix, function)                              \
    15		int mmio_read_##Suffix(u64 phys_address, Type *value)                  \
    16		{                                                                      \
    17			int ret = 0;                                                   \
    18			void __iomem *mapped_address =                                 \
    19				ioremap(phys_address, sizeof(Type));                   \
  > 20			pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
    21				 sizeof(Type));                                        \
    22			if (mapped_address != NULL) {                                  \
    23				*value = function(mapped_address);                     \
    24				iounmap(mapped_address);                               \
    25			} else {                                                       \
    26				pr_err("Failed to MAP IO memory: 0x%llx\n",            \
    27				       phys_address);                                  \
    28				ret = -EIO;                                            \
    29			}                                                              \
    30			return ret;                                                    \
    31		}
  > 32	GENERIC_MMIO_READ(u8, byte, readb)
    33	GENERIC_MMIO_READ(u16, word, readw)
    34	GENERIC_MMIO_READ(u32, dword, readl)
    35	#undef GENERIC_MMIO_READ
    36	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 74094 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-29 22:59 [PATCH] SPI LPC information kernel module Daniel Gutson
  2020-06-30  8:02 ` kernel test robot
@ 2020-06-30  8:56 ` Greg Kroah-Hartman
  2020-06-30 13:57   ` Richard Hughes
       [not found]   ` <CAFmMkTGrnZt7ZaGyYCe-LCHET4yHz9DfanaZwsOS6HCxK40apQ@mail.gmail.com>
  2020-06-30  8:58 ` Arnd Bergmann
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-30  8:56 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Arnd Bergmann, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Mon, Jun 29, 2020 at 07:59:32PM -0300, Daniel Gutson wrote:
> This kernel module exports configuration attributes for the
> system SPI chip.
> This initial version exports the BIOS Write Enable (bioswe),
> BIOS Lock Enable (ble), and the SMM Bios Write Protect (SMM_BWP)
> fields of the Bios Control register. The idea is to keep adding more
> flags, not only from the BC but also from other registers in following
> versions.
> 
> The goal is that the attributes are avilable to fwupd when SecureBoot
> is turned on.
> 
> A technical note: I check if *ppos == BUFFER_SIZE in the reading function
> to exit early and avoid an extra access to the HW, for example when using
> the 'cat' command, which causes two read operations.

Why not use the simple_* functions which should prevent that type of
thing?



> 
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> ---
>  Documentation/ABI/stable/securityfs-spi-lpc |  23 +

Why is this going in securityfs at all?  Why not just sysfs as it is a
CPU attribute, right?


>  MAINTAINERS                                 |   6 +
>  drivers/misc/Kconfig                        |   1 +
>  drivers/misc/Makefile                       |   1 +
>  drivers/misc/spi_lpc/Kconfig                |  20 +
>  drivers/misc/spi_lpc/Makefile               |   8 +
>  drivers/misc/spi_lpc/bios_data_access.c     | 559 +++++++++++++++++++
>  drivers/misc/spi_lpc/bios_data_access.h     | 181 +++++++
>  drivers/misc/spi_lpc/low_level_access.c     |  59 ++
>  drivers/misc/spi_lpc/low_level_access.h     |  21 +
>  drivers/misc/spi_lpc/spi_lpc_main.c         | 176 ++++++
>  drivers/misc/spi_lpc/viddid_arch_map.c      | 566 ++++++++++++++++++++
>  drivers/misc/spi_lpc/viddid_arch_map.h      |  17 +
>  13 files changed, 1638 insertions(+)
>  create mode 100644 Documentation/ABI/stable/securityfs-spi-lpc
>  create mode 100644 drivers/misc/spi_lpc/Kconfig
>  create mode 100644 drivers/misc/spi_lpc/Makefile
>  create mode 100644 drivers/misc/spi_lpc/bios_data_access.c
>  create mode 100644 drivers/misc/spi_lpc/bios_data_access.h
>  create mode 100644 drivers/misc/spi_lpc/low_level_access.c
>  create mode 100644 drivers/misc/spi_lpc/low_level_access.h
>  create mode 100644 drivers/misc/spi_lpc/spi_lpc_main.c
>  create mode 100644 drivers/misc/spi_lpc/viddid_arch_map.c
>  create mode 100644 drivers/misc/spi_lpc/viddid_arch_map.h
> 
> diff --git a/Documentation/ABI/stable/securityfs-spi-lpc b/Documentation/ABI/stable/securityfs-spi-lpc
> new file mode 100644
> index 000000000000..22660a7fd914
> --- /dev/null
> +++ b/Documentation/ABI/stable/securityfs-spi-lpc
> @@ -0,0 +1,23 @@
> +What:		/sys/kernel/security/firmware/bioswe
> +Date:		June 2020
> +KernelVersion:	5.8.0
> +Contact:	daniel.gutson@eclypsium.com
> +Description:	If the system firmware set BIOS Write Enable.
> +		0: writes disabled, 1: writes enabled.

THis is very x86-specific, what about ARM/MIPS/anything else?  Perhaps a
cpu/arch-specific thing instead?

Again, which makes it seem like securityfs is not the thing for this, as
it describes the hardware, not a security model which is what securityfs
has been for in the past, right?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-29 22:59 [PATCH] SPI LPC information kernel module Daniel Gutson
  2020-06-30  8:02 ` kernel test robot
  2020-06-30  8:56 ` Greg Kroah-Hartman
@ 2020-06-30  8:58 ` Arnd Bergmann
       [not found]   ` <CAFmMkTHrQ4LZk4+-3kdJ+dc47MXR1Jd76AXbO-ceT2zsfDRFGQ@mail.gmail.com>
  2020-06-30  8:58 ` Greg Kroah-Hartman
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Arnd Bergmann @ 2020-06-30  8:58 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Greg Kroah-Hartman, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Tue, Jun 30, 2020 at 12:59 AM Daniel Gutson
<daniel.gutson@eclypsium.com> wrote:
>
> This kernel module exports configuration attributes for the
> system SPI chip.
> This initial version exports the BIOS Write Enable (bioswe),
> BIOS Lock Enable (ble), and the SMM Bios Write Protect (SMM_BWP)
> fields of the Bios Control register. The idea is to keep adding more
> flags, not only from the BC but also from other registers in following
> versions.
>
> The goal is that the attributes are avilable to fwupd when SecureBoot
> is turned on.
>
> A technical note: I check if *ppos == BUFFER_SIZE in the reading function
> to exit early and avoid an extra access to the HW, for example when using
> the 'cat' command, which causes two read operations.
>
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>

The description should start with a little more background for those that
don't already know what this driver is for. What is a "system SPI chip"?
Is this an SPI host connected over LPC, or an LPC bus connected over
SPI? Is there a particular spec that this follows?

> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index c7bd01ac6291..280365e7e53c 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_PVPANIC)         += pvpanic.o
>  obj-$(CONFIG_HABANA_AI)                += habanalabs/
>  obj-$(CONFIG_UACCE)            += uacce/
>  obj-$(CONFIG_XILINX_SDFEC)     += xilinx_sdfec.o
> +obj-$(CONFIG_SPI_LPC)          += spi_lpc/

Have you tried finding a more suitable subsystem for it? If this
is for an LPC bus connected over SPI, it should probably go
into drivers/bus, or a new drivers/lpc that could also contain
the existing drivers/mfd/lpc_*.c and drivers/bus/hisi_lpc.c.

If this is for an SPI host connected over LPC, it should be in drivers/spi/

> diff --git a/drivers/misc/spi_lpc/Kconfig b/drivers/misc/spi_lpc/Kconfig
> new file mode 100644
> index 000000000000..08d74746578d
> --- /dev/null
> +++ b/drivers/misc/spi_lpc/Kconfig
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# SPI LPC information kernel module
> +#
> +
> +config SPI_LPC
> +       tristate "SPI LPC information"
> +       depends on SECURITYFS && CPU_SUP_INTEL

Can you ensure it compiles on all architectures by changing the
dependency to (CPU_SUP_INTEL || COMPILE_TEST)

> +       help
> +         This kernel modules exports the configuration attributes for the

             s/modules/module/

> +         system SPI chip.
> +         Enable this kernel module to read the BIOSWE, BLE, and SMM_BWP fields
> +         of the Bios Control register, by reading these three files:
> +
> +             /sys/kernel/security/firmware/bioswe
> +             /sys/kernel/security/firmware/ble
> +             /sys/kernel/security/firmware/smm_bwp

I don't understand the usage of securityfs here. Are you adding a new
"firmware" LSM? If so, please split out the security module into a separate
patch and have it reviewed by the respective maintainers.

> +static int read_spibar(enum PCH_Arch pch_arch, enum CPU_Arch cpu_arch,
> +                      u64 *offset);

Try to avoid forward declarations of static functions by reordering the files.

> +int spi_read_sbase(enum PCH_Arch pch_arch __maybe_unused,
> +                  enum CPU_Arch cpu_arch, struct spi_sbase *reg)
> +{
> +       int ret = 0;
> +
> +       reg->register_arch.source = RegSource_CPU;
> +       reg->register_arch.cpu_arch = cpu_arch;
> +
> +       switch (cpu_arch) {
> +       case cpu_avn:
> +       case cpu_byt:
> +               ret = read_sbase_atom_avn_byt(&reg->cpu_byt);
> +               break;
> +       default:
> +               ret = -EIO;
> +       }
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(spi_read_sbase);

This function seems to be Intel Atom specific but has a rather generic
name for an exported symbol.

> +int spi_read_bc(enum PCH_Arch pch_arch, enum CPU_Arch cpu_arch,
> +               struct spi_bc *reg)

Same here and for a couple of other functions later. Better try to use a
namespace prefix that is more specific to your driver.

> +enum CPU_Arch {
> +       cpu_none,
> +       cpu_bdw,
> +       cpu_bdx,
> +       cpu_hsw,
> +       cpu_hsx,
> +       cpu_ivt,
> +       cpu_jkt,

You might want to avoid having a central instance listing all possible
CPUs. Instead, structure it so that the common parts know nothing
about a specific implementation and each one can be kept in a separate
file for easier extension.

> +enum RegisterSource { RegSource_PCH, RegSource_CPU };
> +
> +struct RegisterArch {
> +       enum RegisterSource source;
> +
> +       union {
> +               enum PCH_Arch pch_arch;
> +               enum CPU_Arch cpu_arch;
> +       };
> +};

Please follow the normal naming of variables and types: use proper
namespace prefixes and lowercase letters instead of CameLcAse.


> +struct spi_bc {
> +       struct RegisterArch register_arch;
> +
> +       union {
> +               struct bc_pch_3xx_4xx_5xx pch_3xx;
> +               struct bc_pch_3xx_4xx_5xx pch_4xx;
> +               struct bc_pch_3xx_4xx_5xx pch_495;
> +               struct bc_pch_3xx_4xx_5xx pch_5xx;
> +               struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_snb;
> +               struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_jkt;
> +               struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_ivb;
> +               struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_ivt;
> +               struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_bdw;
> +               struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_bdx;
> +               struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_hsx;
> +               struct bc_cpu_snb_jkt_ivb_ivt_bdx_hsx cpu_hsw;
> +               struct bc_cpu_skl_kbl_cfl cpu_skl;
> +               struct bc_cpu_skl_kbl_cfl cpu_kbl;
> +               struct bc_cpu_skl_kbl_cfl cpu_cfl;
> +               struct bc_cpu_apl_glk cpu_apl;
> +               struct bc_cpu_apl_glk cpu_glk;
> +               struct bc_cpu_atom_avn cpu_avn;
> +               struct bc_cpu_atom_byt cpu_byt;
> +       };
> +};

Here too, try to avoid having a central data structure that knows about all
possible hardware.

> +#define GENERIC_MMIO_READ(Type, Suffix, function)                              \
> +       int mmio_read_##Suffix(u64 phys_address, Type *value)                  \
> +       {                                                                      \
> +               int ret = 0;                                                   \
> +               void __iomem *mapped_address =                                 \
> +                       ioremap(phys_address, sizeof(Type));                   \
> +               pr_debug("Reading MMIO 0x%llx 0x%lx\n", phys_address,          \
> +                        sizeof(Type));                                        \
> +               if (mapped_address != NULL) {                                  \
> +                       *value = function(mapped_address);                     \
> +                       iounmap(mapped_address);                               \
> +               } else {                                                       \
> +                       pr_err("Failed to MAP IO memory: 0x%llx\n",            \
> +                              phys_address);                                  \
> +                       ret = -EIO;                                            \
> +               }                                                              \
> +               return ret;                                                    \
> +       }
> +GENERIC_MMIO_READ(u8, byte, readb)
> +GENERIC_MMIO_READ(u16, word, readw)
> +GENERIC_MMIO_READ(u32, dword, readl)

This is definitely way too generic to be added in a 'misc' driver. Why would
you even want a function that reads a register by physical address?

The driver that owns the MMIO region normally maps it once during its
probe() function and then keeps a pointer around


+static int __init mod_init(void)
+{
+       int ret = 0;
+
+       if (get_pch_cpu(&pch_arch, &cpu_arch) != 0) {
+               pr_err("Couldn't detect PCH or CPU\n");
+               return -EIO;
+       }

This looks like there should be a PCI (or SPI or LPC) driver
instead of a linux-2.4 style module init that goes scanning the
PCI bus.

> +int viddid2pch_arch(u64 vid, u64 did, enum PCH_Arch *arch)
> +{
> +       switch (vid) {
> +       case 0x8086: /* INTEL */
> +               switch (did) {
> +               case 0x1c44:
> +               case 0x1c46:
> +               case 0x1c47:
> +               case 0x1c49:
> +               case 0x1c4a:
> +               case 0x1c4b:
> +               case 0x1c4c:
> +               case 0x1c4d:
> +               case 0x1c4e:
> +               case 0x1c4f:
> +               case 0x1c50:
> +               case 0x1c52:
> +               case 0x1c54:
> +               case 0x1c56:
> +               case 0x1c5c:
> +                       *arch = pch_6_c200;
> +                       return 0;
> +               case 0x1e47:
> +               case 0x1e48:
> +               case 0x1e49:
> +               case 0x1e44:

Some of these devices seem to be owned by drivers/mfd/lpc_ich.c
(despite a comment in there claiming otherwise).

Can you describe the relation between your code and that one?
Would it be possible to remove support for the parts that already
have a driver and use an mfc driver for those?

       Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-29 22:59 [PATCH] SPI LPC information kernel module Daniel Gutson
                   ` (2 preceding siblings ...)
  2020-06-30  8:58 ` Arnd Bergmann
@ 2020-06-30  8:58 ` Greg Kroah-Hartman
  2020-06-30  8:59 ` Greg Kroah-Hartman
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-30  8:58 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Arnd Bergmann, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Mon, Jun 29, 2020 at 07:59:32PM -0300, Daniel Gutson wrote:
> --- /dev/null
> +++ b/drivers/misc/spi_lpc/bios_data_access.h
> @@ -0,0 +1,181 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * SPI LPC flash platform security driver
> + *
> + * Copyright 2020 (c) Daniel Gutson (daniel.gutson@eclypsium.com)
> + *
> + */
> +#ifndef BIOS_DATA_ACCESS_H
> +#define BIOS_DATA_ACCESS_H

What BIOS?  Is this a UEFI thing?  ACPI?  Uboot?  Fastboot?

> +
> +#include <linux/types.h>
> +
> +enum PCH_Arch {

Why the upper-case name?

> +	pch_none,
> +	pch_6_c200,
> +	pch_7_c210,
> +	pch_c60x_x79,
> +	pch_communications_89xx,
> +	pch_8_c220,
> +	pch_c61x_x99,
> +	pch_5_mobile,
> +	pch_6_mobile,
> +	pch_7_8_mobile,
> +	pch_1xx,
> +	pch_c620,
> +	pch_2xx,
> +	pch_3xx,
> +	pch_4xx,
> +	pch_495,
> +	pch_5xx,
> +	PCH_Archs_count

Do these have real values that the hardware expects?  If so, you need to
assign them numbers, right?  If not, what do all of these mean?

> +};
> +
> +enum CPU_Arch {
> +	cpu_none,
> +	cpu_bdw,
> +	cpu_bdx,
> +	cpu_hsw,
> +	cpu_hsx,
> +	cpu_ivt,
> +	cpu_jkt,
> +	cpu_kbl,
> +	cpu_skl,
> +	cpu_ivb,
> +	cpu_snb,
> +	cpu_avn,
> +	cpu_cfl,
> +	cpu_byt,
> +	cpu_whl,
> +	cpu_cml,
> +	cpu_icl,
> +	cpu_apl,
> +	cpu_glk,
> +	cpu_tgl,
> +	cpu_amd,
> +	CPU_Archs_count
> +};
> +
> +enum RegisterSource { RegSource_PCH, RegSource_CPU };

Again all of the Caps, please don't use, that's not Linux styles (and
checkpatch.pl should have warned you about this, did you run it on your
patch?)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-29 22:59 [PATCH] SPI LPC information kernel module Daniel Gutson
                   ` (3 preceding siblings ...)
  2020-06-30  8:58 ` Greg Kroah-Hartman
@ 2020-06-30  8:59 ` Greg Kroah-Hartman
  2020-06-30  9:00 ` Greg Kroah-Hartman
  2020-07-01  7:35 ` kernel test robot
  6 siblings, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-30  8:59 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Arnd Bergmann, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Mon, Jun 29, 2020 at 07:59:32PM -0300, Daniel Gutson wrote:
> This kernel module exports configuration attributes for the
> system SPI chip.
> This initial version exports the BIOS Write Enable (bioswe),
> BIOS Lock Enable (ble), and the SMM Bios Write Protect (SMM_BWP)
> fields of the Bios Control register. The idea is to keep adding more
> flags, not only from the BC but also from other registers in following
> versions.
> 
> The goal is that the attributes are avilable to fwupd when SecureBoot
> is turned on.
> 
> A technical note: I check if *ppos == BUFFER_SIZE in the reading function
> to exit early and avoid an extra access to the HW, for example when using
> the 'cat' command, which causes two read operations.
> 
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> ---
>  Documentation/ABI/stable/securityfs-spi-lpc |  23 +
>  MAINTAINERS                                 |   6 +
>  drivers/misc/Kconfig                        |   1 +
>  drivers/misc/Makefile                       |   1 +
>  drivers/misc/spi_lpc/Kconfig                |  20 +
>  drivers/misc/spi_lpc/Makefile               |   8 +
>  drivers/misc/spi_lpc/bios_data_access.c     | 559 +++++++++++++++++++
>  drivers/misc/spi_lpc/bios_data_access.h     | 181 +++++++
>  drivers/misc/spi_lpc/low_level_access.c     |  59 ++
>  drivers/misc/spi_lpc/low_level_access.h     |  21 +
>  drivers/misc/spi_lpc/spi_lpc_main.c         | 176 ++++++
>  drivers/misc/spi_lpc/viddid_arch_map.c      | 566 ++++++++++++++++++++
>  drivers/misc/spi_lpc/viddid_arch_map.h      |  17 +
>  13 files changed, 1638 insertions(+)

A single driver that is 1500 lines long is fine, why split this up into
tiny pieces?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-29 22:59 [PATCH] SPI LPC information kernel module Daniel Gutson
                   ` (4 preceding siblings ...)
  2020-06-30  8:59 ` Greg Kroah-Hartman
@ 2020-06-30  9:00 ` Greg Kroah-Hartman
  2020-07-01  7:35 ` kernel test robot
  6 siblings, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-30  9:00 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Arnd Bergmann, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Mon, Jun 29, 2020 at 07:59:32PM -0300, Daniel Gutson wrote:
> +EXPORT_SYMBOL_GPL(spi_read_sbase);

Why are you exporting symbols?  Nothing in this patch uses them.

And that's the spi_* namespace for the SPI core, not your tiny driver,
why pollute the global namespace in a confusing way like this?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-30  8:56 ` Greg Kroah-Hartman
@ 2020-06-30 13:57   ` Richard Hughes
  2020-06-30 14:24     ` Arnd Bergmann
  2020-06-30 15:14     ` Greg Kroah-Hartman
       [not found]   ` <CAFmMkTGrnZt7ZaGyYCe-LCHET4yHz9DfanaZwsOS6HCxK40apQ@mail.gmail.com>
  1 sibling, 2 replies; 20+ messages in thread
From: Richard Hughes @ 2020-06-30 13:57 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Daniel Gutson, Derek Kiernan, Arnd Bergmann,
	Mauro Carvalho Chehab, linux-kernel, Alex Bazhaniuk

On Tue, 30 Jun 2020 at 09:56, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> Again, which makes it seem like securityfs is not the thing for this, as
> it describes the hardware, not a security model which is what securityfs
> has been for in the past, right?

It describes the hardware platform. From a fwupd perspective I don't
mind if the BC attributes are read from securityfs, sysfs or even read
from an offset in a file from /proc... I guess sysfs makes sense if
securityfs is defined for things like the LSM or lockdown status,
although I also thought sysfs was for devices *in* the platform, not
the platform itself. I guess exposing the platform registers in sysfs
is no more weird than exposing things like the mei device and rfkill.

Richard

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-30 13:57   ` Richard Hughes
@ 2020-06-30 14:24     ` Arnd Bergmann
  2020-06-30 15:14     ` Greg Kroah-Hartman
  1 sibling, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2020-06-30 14:24 UTC (permalink / raw)
  To: Richard Hughes
  Cc: Greg Kroah-Hartman, Daniel Gutson, Derek Kiernan,
	Mauro Carvalho Chehab, linux-kernel, Alex Bazhaniuk

On Tue, Jun 30, 2020 at 3:57 PM Richard Hughes <hughsient@gmail.com> wrote:
>
> On Tue, 30 Jun 2020 at 09:56, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > Again, which makes it seem like securityfs is not the thing for this, as
> > it describes the hardware, not a security model which is what securityfs
> > has been for in the past, right?
>
> It describes the hardware platform. From a fwupd perspective I don't
> mind if the BC attributes are read from securityfs, sysfs or even read
> from an offset in a file from /proc... I guess sysfs makes sense if
> securityfs is defined for things like the LSM or lockdown status,
> although I also thought sysfs was for devices *in* the platform, not
> the platform itself. I guess exposing the platform registers in sysfs
> is no more weird than exposing things like the mei device and rfkill.

Why does fwupd care about the platform then? If these are
register values that relate to the flash device and that device is
what the firmware update gets written to, shouldn't it just use
an interface from that device?

       Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
       [not found]   ` <CAFmMkTGrnZt7ZaGyYCe-LCHET4yHz9DfanaZwsOS6HCxK40apQ@mail.gmail.com>
@ 2020-06-30 15:00     ` Arnd Bergmann
  2020-06-30 15:28     ` Greg Kroah-Hartman
  1 sibling, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2020-06-30 15:00 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Greg Kroah-Hartman, Derek Kiernan, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Tue, Jun 30, 2020 at 4:43 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> On Tue, Jun 30, 2020 at 5:56 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>> On Mon, Jun 29, 2020 at 07:59:32PM -0300, Daniel Gutson wrote:
>> Why is this going in securityfs at all?  Why not just sysfs as it is a
>> CPU attribute, right?
>
> Richard already discussed that, but "it" is not only (one) CPU attribute, are SPI chip
> settings and attributes coming from the firmware.
> Please note that I wanted to submit the minimum patch, but I need to add more attributes.

Why can't the SPI chip settings be attributes of the SPI chip device in sysfs?

Which firmware are you actually talking about here? Do you mean firmware running
on a device behind the SPI controller?

      Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-30 13:57   ` Richard Hughes
  2020-06-30 14:24     ` Arnd Bergmann
@ 2020-06-30 15:14     ` Greg Kroah-Hartman
  1 sibling, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-30 15:14 UTC (permalink / raw)
  To: Richard Hughes
  Cc: Daniel Gutson, Derek Kiernan, Arnd Bergmann,
	Mauro Carvalho Chehab, linux-kernel, Alex Bazhaniuk

On Tue, Jun 30, 2020 at 02:57:11PM +0100, Richard Hughes wrote:
> On Tue, 30 Jun 2020 at 09:56, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > Again, which makes it seem like securityfs is not the thing for this, as
> > it describes the hardware, not a security model which is what securityfs
> > has been for in the past, right?
> 
> It describes the hardware platform. From a fwupd perspective I don't
> mind if the BC attributes are read from securityfs, sysfs or even read
> from an offset in a file from /proc... I guess sysfs makes sense if
> securityfs is defined for things like the LSM or lockdown status,
> although I also thought sysfs was for devices *in* the platform, not
> the platform itself.

Have you looked at /sys/devices/system/ in a while?  :)

> I guess exposing the platform registers in sysfs
> is no more weird than exposing things like the mei device and rfkill.

It is attributes that describe the hardware the system is running on,
which is what sysfs is for.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
       [not found]   ` <CAFmMkTGrnZt7ZaGyYCe-LCHET4yHz9DfanaZwsOS6HCxK40apQ@mail.gmail.com>
  2020-06-30 15:00     ` Arnd Bergmann
@ 2020-06-30 15:28     ` Greg Kroah-Hartman
  2020-06-30 15:32       ` Greg Kroah-Hartman
       [not found]       ` <CAFmMkTGy7u8oNSPmBHf9+URzKeNOxy5TJtqF3FCruRkTgJ_wGQ@mail.gmail.com>
  1 sibling, 2 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-30 15:28 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Arnd Bergmann, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Tue, Jun 30, 2020 at 11:42:58AM -0300, Daniel Gutson wrote:
> On Tue, Jun 30, 2020 at 5:56 AM Greg Kroah-Hartman <
> gregkh@linuxfoundation.org> wrote:
> 
> > On Mon, Jun 29, 2020 at 07:59:32PM -0300, Daniel Gutson wrote:
> > > This kernel module exports configuration attributes for the
> > > system SPI chip.
> > > This initial version exports the BIOS Write Enable (bioswe),
> > > BIOS Lock Enable (ble), and the SMM Bios Write Protect (SMM_BWP)
> > > fields of the Bios Control register. The idea is to keep adding more
> > > flags, not only from the BC but also from other registers in following
> > > versions.
> > >
> > > The goal is that the attributes are avilable to fwupd when SecureBoot
> > > is turned on.
> > >
> > > A technical note: I check if *ppos == BUFFER_SIZE in the reading function
> > > to exit early and avoid an extra access to the HW, for example when using
> > > the 'cat' command, which causes two read operations.
> >
> > Why not use the simple_* functions which should prevent that type of
> > thing?
> >
> 
> a hint please? I don't see how to do it with simple_read_from_buffer, I
> need to return in the read fop the amount of read bytes, but don't know
> how to mark EOF. Because of that, 'cat' reads again just for me to tell it
> there's nothing else to read.

That's fine, the kernel does not tell userspace "EOF", that is up to the
libc to determine.  If you read the data from the hardware once, and
keep it in your buffer, simple_read_from_buffer() will handle all of
that logic for you, please let it do that.

> > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> > > ---
> > >  Documentation/ABI/stable/securityfs-spi-lpc |  23 +
> >
> > Why is this going in securityfs at all?  Why not just sysfs as it is a
> > CPU attribute, right?
> >
> 
> Richard already discussed that, but "it" is not only (one) CPU attribute,
> are SPI chip settings and attributes coming from the firmware.

All hardware things, please use sysfs, that is what it is designed for.

> Please note that I wanted to submit the minimum patch, but I need to add
> more attributes.

A patch series is great to create and send showing all of that.

> > > diff --git a/Documentation/ABI/stable/securityfs-spi-lpc
> > b/Documentation/ABI/stable/securityfs-spi-lpc
> > > new file mode 100644
> > > index 000000000000..22660a7fd914
> > > --- /dev/null
> > > +++ b/Documentation/ABI/stable/securityfs-spi-lpc
> > > @@ -0,0 +1,23 @@
> > > +What:                /sys/kernel/security/firmware/bioswe
> > > +Date:                June 2020
> > > +KernelVersion:       5.8.0
> > > +Contact:     daniel.gutson@eclypsium.com
> > > +Description: If the system firmware set BIOS Write Enable.
> > > +             0: writes disabled, 1: writes enabled.
> >
> > THis is very x86-specific, what about ARM/MIPS/anything else?  Perhaps a
> > cpu/arch-specific thing instead?
> >
> 
> We debated this but didn't find a better match, since cpu/arch-specific
> seemed too core to put informational drivers.
> Do you have a suggestion?

Make it explicitly hardware specific in your userspace location.
Otherwise you just defined this for all hardware types, with what you
used above, and I do not think you wanted to do that.

> > Again, which makes it seem like securityfs is not the thing for this, as
> > it describes the hardware, not a security model which is what securityfs
> > has been for in the past, right?
> >
> 
> I prefer to leave this to the other discussion with Richard. It's fine for
> me too to use sysfs.
> FWIW, the driver provides information related to firmware security.

It provides information on what is going on with the firmware, it's up
to userspace to know/determine/care if that means anything with regards
to "security" or not :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-30 15:28     ` Greg Kroah-Hartman
@ 2020-06-30 15:32       ` Greg Kroah-Hartman
       [not found]       ` <CAFmMkTGy7u8oNSPmBHf9+URzKeNOxy5TJtqF3FCruRkTgJ_wGQ@mail.gmail.com>
  1 sibling, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-30 15:32 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Arnd Bergmann, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Tue, Jun 30, 2020 at 05:28:32PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 30, 2020 at 11:42:58AM -0300, Daniel Gutson wrote:
> > On Tue, Jun 30, 2020 at 5:56 AM Greg Kroah-Hartman <
> > gregkh@linuxfoundation.org> wrote:
> > 
> > > On Mon, Jun 29, 2020 at 07:59:32PM -0300, Daniel Gutson wrote:
> > > > This kernel module exports configuration attributes for the
> > > > system SPI chip.
> > > > This initial version exports the BIOS Write Enable (bioswe),
> > > > BIOS Lock Enable (ble), and the SMM Bios Write Protect (SMM_BWP)
> > > > fields of the Bios Control register. The idea is to keep adding more
> > > > flags, not only from the BC but also from other registers in following
> > > > versions.
> > > >
> > > > The goal is that the attributes are avilable to fwupd when SecureBoot
> > > > is turned on.
> > > >
> > > > A technical note: I check if *ppos == BUFFER_SIZE in the reading function
> > > > to exit early and avoid an extra access to the HW, for example when using
> > > > the 'cat' command, which causes two read operations.
> > >
> > > Why not use the simple_* functions which should prevent that type of
> > > thing?
> > >
> > 
> > a hint please? I don't see how to do it with simple_read_from_buffer, I
> > need to return in the read fop the amount of read bytes, but don't know
> > how to mark EOF. Because of that, 'cat' reads again just for me to tell it
> > there's nothing else to read.
> 
> That's fine, the kernel does not tell userspace "EOF", that is up to the
> libc to determine.  If you read the data from the hardware once, and
> keep it in your buffer, simple_read_from_buffer() will handle all of
> that logic for you, please let it do that.
> 
> > > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> > > > ---
> > > >  Documentation/ABI/stable/securityfs-spi-lpc |  23 +
> > >
> > > Why is this going in securityfs at all?  Why not just sysfs as it is a
> > > CPU attribute, right?
> > >
> > 
> > Richard already discussed that, but "it" is not only (one) CPU attribute,
> > are SPI chip settings and attributes coming from the firmware.
> 
> All hardware things, please use sysfs, that is what it is designed for.
> 
> > Please note that I wanted to submit the minimum patch, but I need to add
> > more attributes.
> 
> A patch series is great to create and send showing all of that.
> 
> > > > diff --git a/Documentation/ABI/stable/securityfs-spi-lpc
> > > b/Documentation/ABI/stable/securityfs-spi-lpc
> > > > new file mode 100644
> > > > index 000000000000..22660a7fd914
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/stable/securityfs-spi-lpc
> > > > @@ -0,0 +1,23 @@
> > > > +What:                /sys/kernel/security/firmware/bioswe
> > > > +Date:                June 2020
> > > > +KernelVersion:       5.8.0
> > > > +Contact:     daniel.gutson@eclypsium.com
> > > > +Description: If the system firmware set BIOS Write Enable.
> > > > +             0: writes disabled, 1: writes enabled.
> > >
> > > THis is very x86-specific, what about ARM/MIPS/anything else?  Perhaps a
> > > cpu/arch-specific thing instead?
> > >
> > 
> > We debated this but didn't find a better match, since cpu/arch-specific
> > seemed too core to put informational drivers.
> > Do you have a suggestion?
> 
> Make it explicitly hardware specific in your userspace location.
> Otherwise you just defined this for all hardware types, with what you
> used above, and I do not think you wanted to do that.
> 
> > > Again, which makes it seem like securityfs is not the thing for this, as
> > > it describes the hardware, not a security model which is what securityfs
> > > has been for in the past, right?
> > >
> > 
> > I prefer to leave this to the other discussion with Richard. It's fine for
> > me too to use sysfs.
> > FWIW, the driver provides information related to firmware security.
> 
> It provides information on what is going on with the firmware, it's up
> to userspace to know/determine/care if that means anything with regards
> to "security" or not :)

Also, you seem to have missed /sys/firmware/ on your system :)

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
       [not found]       ` <CAFmMkTGy7u8oNSPmBHf9+URzKeNOxy5TJtqF3FCruRkTgJ_wGQ@mail.gmail.com>
@ 2020-06-30 16:55         ` Greg Kroah-Hartman
  0 siblings, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-30 16:55 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Arnd Bergmann, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Tue, Jun 30, 2020 at 01:18:28PM -0300, Daniel Gutson wrote:
> On Tue, Jun 30, 2020 at 12:28 PM Greg Kroah-Hartman <
> gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Jun 30, 2020 at 11:42:58AM -0300, Daniel Gutson wrote:
> > > On Tue, Jun 30, 2020 at 5:56 AM Greg Kroah-Hartman <
> > > gregkh@linuxfoundation.org> wrote:
> > >
> > > > On Mon, Jun 29, 2020 at 07:59:32PM -0300, Daniel Gutson wrote:
> > > > > This kernel module exports configuration attributes for the
> > > > > system SPI chip.
> > > > > This initial version exports the BIOS Write Enable (bioswe),
> > > > > BIOS Lock Enable (ble), and the SMM Bios Write Protect (SMM_BWP)
> > > > > fields of the Bios Control register. The idea is to keep adding more
> > > > > flags, not only from the BC but also from other registers in
> > following
> > > > > versions.
> > > > >
> > > > > The goal is that the attributes are avilable to fwupd when SecureBoot
> > > > > is turned on.
> > > > >
> > > > > A technical note: I check if *ppos == BUFFER_SIZE in the reading
> > function
> > > > > to exit early and avoid an extra access to the HW, for example when
> > using
> > > > > the 'cat' command, which causes two read operations.
> > > >
> > > > Why not use the simple_* functions which should prevent that type of
> > > > thing?
> > > >
> > >
> > > a hint please? I don't see how to do it with simple_read_from_buffer, I
> > > need to return in the read fop the amount of read bytes, but don't know
> > > how to mark EOF. Because of that, 'cat' reads again just for me to tell
> > it
> > > there's nothing else to read.
> >
> > That's fine, the kernel does not tell userspace "EOF", that is up to the
> > libc to determine.  If you read the data from the hardware once, and
> > keep it in your buffer, simple_read_from_buffer() will handle all of
> > that logic for you, please let it do that.
> >
> 
> The only way I see to do this is to dynamically allocate the buffer in the
> open fop, in order to avoid concurrency issues.
> Is this correct?

Or use a lock, depends on what you want to do here.

But sysfs should handle all of this for you, when you switch to it.

> > > We debated this but didn't find a better match, since cpu/arch-specific
> > > seemed too core to put informational drivers.
> > > Do you have a suggestion?
> >
> > Make it explicitly hardware specific in your userspace location.
> >
> 
> What do you mean by "your userspace location"?

Where your files show up to userspace.  sysfs is a hierarchy, don't put
hardware-specific stuff at the "root" of it, otherwise that doesn't make
any sense.  Look at what is there today for examples of what to do.


good luck!

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
       [not found]   ` <CAFmMkTHrQ4LZk4+-3kdJ+dc47MXR1Jd76AXbO-ceT2zsfDRFGQ@mail.gmail.com>
@ 2020-06-30 20:57     ` Arnd Bergmann
       [not found]       ` <CAFmMkTE3z6OZQ_v3jx-4MzMr8v+4qcF2uLn0ASGydj5oqDnfjg@mail.gmail.com>
  0 siblings, 1 reply; 20+ messages in thread
From: Arnd Bergmann @ 2020-06-30 20:57 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Greg Kroah-Hartman, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Tue, Jun 30, 2020 at 9:08 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> On Tue, Jun 30, 2020 at 5:58 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tue, Jun 30, 2020 at 12:59 AM Daniel Gutson <daniel.gutson@eclypsium.com> wrote:
>> The description should start with a little more background for those that
>> don't already know what this driver is for. What is a "system SPI chip"?
>> Is this an SPI host connected over LPC, or an LPC bus connected over
>> SPI? Is there a particular spec that this follows?
>
>
> "System SPI chip" refers to the main system firmware, which is accessed through an SPI interface.
> AFAIK there's no spec for this, though it's a de-facto standard applying to the earliest days of legacy BIOS.
> This driver provides visibility to the system firmware configuration access.

Oh, so it isn't even the SPI controller, just a flash chip hanging off
a random SPI controller, or possibly something else. I suppose it could
be any MFD device, or possibly something different.

>> > +int spi_read_sbase(enum PCH_Arch pch_arch __maybe_unused,
>> > +                  enum CPU_Arch cpu_arch, struct spi_sbase *reg)
>> > +{
>> > +       int ret = 0;
>> > +
>> > +       reg->register_arch.source = RegSource_CPU;
>> > +       reg->register_arch.cpu_arch = cpu_arch;
>> > +
>> > +       switch (cpu_arch) {
>> > +       case cpu_avn:
>> > +       case cpu_byt:
>> > +               ret = read_sbase_atom_avn_byt(&reg->cpu_byt);
>> > +               break;
>> > +       default:
>> > +               ret = -EIO;
>> > +       }
>> > +       return ret;
>> > +}
>> > +EXPORT_SYMBOL_GPL(spi_read_sbase);
>>
>> This function seems to be Intel Atom specific but has a rather generic
>> name for an exported symbol.
>
>
> It 'currently' is atom specific, but as I mentioned in the mail, the idea was
> to submit an initial patch with some attributes. The more platforms and fields
> I need, this switch will get more and more populated. This is about reading the
>  SBASE register, which in my current state, is only used for Atom. I can submit
> a driver with more fields where these switches get more populated, but I
> thought it would be easier starting with three fields only.
>
> Anyways I'll give up on the exports for now until a kernel module shows up needing the reading layer.

The problem is more the namespace: as the driver has almost nothing
to do with spi, this is not what the function should be named. The other
problem is that the function should not really exist in the first place,
as no driver has any business reading the register base of a random
other device that already has a driver.

>> > +enum CPU_Arch {
>> > +       cpu_none,
>> > +       cpu_bdw,
>> > +       cpu_bdx,
>> > +       cpu_hsw,
>> > +       cpu_hsx,
>> > +       cpu_ivt,
>> > +       cpu_jkt,
>>
>> You might want to avoid having a central instance listing all possible
>> CPUs. Instead, structure it so that the common parts know nothing
>> about a specific implementation and each one can be kept in a separate
>> file for easier extension.
>
>
> CPUs differ in register structure, not only extending, but having different fields too.
> All this information comes from datasheets that don't tell "this is CPU XX with these extensions",
> so sometimes it is easier to copy what the datasheet says.
> I didn't understand what's wrong with having a central enumeration of all available CPUs?

> This is a way of polymorphism, where I can just do a single
> read_[register](struct register* register) and then "browse" inside the register definition by knowing the architecture.
> Could you please explain your alternative with more detail? The user wants its architecture's definition, and will not
...
>> The driver that owns the MMIO region normally maps it once during its
>> probe() function and then keeps a pointer around
>
>
> This case is different. Please note that this is not a "device driver",
> that's why it doesn't own the MMIO region. This driver gathers information
> from the SPI controller only in the current state (it will be expanded).
> That's why it maps and then unmaps in the same operation.

To answer all the above points: This needs to be in a driver, and
in case of pch, that driver is drivers/mtd/spi-nor/intel-spi.c.

This driver already has access to the registers you need, and the
information you pass corresponds to the devices that this driver
manages, which in turn is where user space would logically search
for it. You can read the registers in the intel_spi_probe() function
and then add attributes to the mtd device in sysfs.

I think a good way to handle this in a generic way would be to add
members to the mtd_info structure and then have the attributes
created by the mtd core code for any device that initializes those
struct members, along with the existing 'type', 'size', 'flags',  etc
attributes.

     Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-06-29 22:59 [PATCH] SPI LPC information kernel module Daniel Gutson
                   ` (5 preceding siblings ...)
  2020-06-30  9:00 ` Greg Kroah-Hartman
@ 2020-07-01  7:35 ` kernel test robot
  6 siblings, 0 replies; 20+ messages in thread
From: kernel test robot @ 2020-07-01  7:35 UTC (permalink / raw)
  To: Daniel Gutson, Derek Kiernan, Arnd Bergmann, Greg Kroah-Hartman,
	Mauro Carvalho Chehab, linux-kernel, Richard Hughes,
	Alex Bazhaniuk
  Cc: kbuild-all, linux-media

[-- Attachment #1: Type: text/plain, Size: 24711 bytes --]

Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on soc/for-next linus/master v5.8-rc3 next-20200701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Gutson/SPI-LPC-information-kernel-module/20200630-070234
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git ba2104c24aba1fa7e19d53f08c985526a6786d8b
config: um-allmodconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=um 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |                   ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
      50 |  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
         |  ^~~~~~~~~~~~~~~~
   include/asm-generic/fixmap.h:32:2: note: in expansion of macro 'BUILD_BUG_ON'
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |  ^~~~~~~~~~~~
   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/misc/spi_lpc/spi_lpc_main.c:11:
   include/asm-generic/fixmap.h: In function 'fix_to_virt':
   include/asm-generic/fixmap.h:32:19: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |                   ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
      50 |  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
         |  ^~~~~~~~~~~~~~~~
   include/asm-generic/fixmap.h:32:2: note: in expansion of macro 'BUILD_BUG_ON'
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |  ^~~~~~~~~~~~
   In file included from include/linux/string.h:6,
                    from include/linux/uuid.h:12,
                    from include/linux/mod_devicetable.h:13,
                    from include/linux/pci.h:27,
                    from drivers/misc/spi_lpc/low_level_access.c:11:
   include/asm-generic/fixmap.h: In function 'fix_to_virt':
   include/asm-generic/fixmap.h:32:19: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |                   ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
      50 |  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
         |  ^~~~~~~~~~~~~~~~
   include/asm-generic/fixmap.h:32:2: note: in expansion of macro 'BUILD_BUG_ON'
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |  ^~~~~~~~~~~~
   In file included from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/huge_mm.h:8,
                    from include/linux/mm.h:675,
                    from include/linux/scatterlist.h:8,
                    from include/linux/dma-mapping.h:11,
                    from include/linux/pci-dma-compat.h:8,
                    from include/linux/pci.h:2415,
                    from drivers/misc/spi_lpc/low_level_access.c:11:
   arch/um/include/asm/uaccess.h: In function '__access_ok':
   arch/um/include/asm/uaccess.h:17:29: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
      17 |    (((unsigned long) (addr) >= FIXADDR_USER_START) && \
         |                             ^~
   arch/um/include/asm/uaccess.h:45:3: note: in expansion of macro '__access_ok_vsyscall'
      45 |   __access_ok_vsyscall(addr, size) ||
         |   ^~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/debugfs.h:15,
                    from drivers/misc/eeprom/idt_89hpesx.c:78:
   arch/um/include/asm/uaccess.h: In function '__access_ok':
   arch/um/include/asm/uaccess.h:17:29: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
      17 |    (((unsigned long) (addr) >= FIXADDR_USER_START) && \
         |                             ^~
   arch/um/include/asm/uaccess.h:45:3: note: in expansion of macro '__access_ok_vsyscall'
      45 |   __access_ok_vsyscall(addr, size) ||
         |   ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'pci_read_byte':
>> drivers/misc/spi_lpc/low_level_access.c:42:31: error: implicit declaration of function 'pci_find_bus'; did you mean 'pci_find_next_bus'? [-Werror=implicit-function-declaration]
      42 |   struct pci_bus *found_bus = pci_find_bus(0, bus);              \
         |                               ^~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:56:1: note: in expansion of macro 'GENERIC_PCI_READ'
      56 | GENERIC_PCI_READ(byte, u8)
         | ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:42:31: warning: initialization of 'struct pci_bus *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      42 |   struct pci_bus *found_bus = pci_find_bus(0, bus);              \
         |                               ^~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:56:1: note: in expansion of macro 'GENERIC_PCI_READ'
      56 | GENERIC_PCI_READ(byte, u8)
         | ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:46:10: error: implicit declaration of function 'pci_bus_read_config_byte'; did you mean 'pci_read_config_byte'? [-Werror=implicit-function-declaration]
      46 |    ret = pci_bus_read_config_##Suffix(                    \
         |          ^~~~~~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:56:1: note: in expansion of macro 'GENERIC_PCI_READ'
      56 | GENERIC_PCI_READ(byte, u8)
         | ^~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'pci_read_word':
>> drivers/misc/spi_lpc/low_level_access.c:42:31: warning: initialization of 'struct pci_bus *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      42 |   struct pci_bus *found_bus = pci_find_bus(0, bus);              \
         |                               ^~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:57:1: note: in expansion of macro 'GENERIC_PCI_READ'
      57 | GENERIC_PCI_READ(word, u16)
         | ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:46:10: error: implicit declaration of function 'pci_bus_read_config_word'; did you mean 'pci_read_config_word'? [-Werror=implicit-function-declaration]
      46 |    ret = pci_bus_read_config_##Suffix(                    \
         |          ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:57:1: note: in expansion of macro 'GENERIC_PCI_READ'
      57 | GENERIC_PCI_READ(word, u16)
         | ^~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'pci_read_dword':
>> drivers/misc/spi_lpc/low_level_access.c:42:31: warning: initialization of 'struct pci_bus *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      42 |   struct pci_bus *found_bus = pci_find_bus(0, bus);              \
         |                               ^~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:58:1: note: in expansion of macro 'GENERIC_PCI_READ'
      58 | GENERIC_PCI_READ(dword, u32)
         | ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:46:10: error: implicit declaration of function 'pci_bus_read_config_dword'; did you mean 'pci_read_config_dword'? [-Werror=implicit-function-declaration]
      46 |    ret = pci_bus_read_config_##Suffix(                    \
         |          ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:58:1: note: in expansion of macro 'GENERIC_PCI_READ'
      58 | GENERIC_PCI_READ(dword, u32)
         | ^~~~~~~~~~~~~~~~
   drivers/misc/enclosure.c:115: warning: Function parameter or member 'name' not described in 'enclosure_register'
   drivers/misc/enclosure.c:115: warning: Function parameter or member 'cb' not described in 'enclosure_register'
   drivers/misc/enclosure.c:283: warning: Function parameter or member 'number' not described in 'enclosure_component_alloc'
   drivers/misc/enclosure.c:283: warning: Excess function parameter 'num' description in 'enclosure_component_alloc'
   drivers/misc/enclosure.c:363: warning: Function parameter or member 'component' not described in 'enclosure_add_device'
   drivers/misc/enclosure.c:363: warning: Excess function parameter 'num' description in 'enclosure_add_device'
   drivers/misc/enclosure.c:398: warning: Function parameter or member 'dev' not described in 'enclosure_remove_device'
   drivers/misc/enclosure.c:398: warning: Excess function parameter 'num' description in 'enclosure_remove_device'
   cc1: some warnings being treated as errors
   make[4]: *** [scripts/Makefile.build:280: drivers/misc/spi_lpc/low_level_access.o] Error 1
   make[4]: Target '__build' not remade because of errors.
   make[3]: *** [scripts/Makefile.build:497: drivers/misc/spi_lpc] Error 2
   cc1: warning: arch/um/include/uapi: No such file or directory [-Wmissing-include-dirs]
   cc1: warning: arch/um/include/uapi: No such file or directory [-Wmissing-include-dirs]
   make[2]: *** [scripts/Makefile.build:497: drivers/misc] Error 2
   make[2]: Target '__build' not remade because of errors.
   make[3]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1756: drivers] Error 2
   make[1]: Target 'drivers/misc/' not remade because of errors.
--
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |                   ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
      50 |  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
         |  ^~~~~~~~~~~~~~~~
   include/asm-generic/fixmap.h:32:2: note: in expansion of macro 'BUILD_BUG_ON'
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |  ^~~~~~~~~~~~
   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/misc/spi_lpc/spi_lpc_main.c:11:
   include/asm-generic/fixmap.h: In function 'fix_to_virt':
   include/asm-generic/fixmap.h:32:19: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |                   ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
      50 |  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
         |  ^~~~~~~~~~~~~~~~
   include/asm-generic/fixmap.h:32:2: note: in expansion of macro 'BUILD_BUG_ON'
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |  ^~~~~~~~~~~~
   In file included from include/linux/string.h:6,
                    from include/linux/uuid.h:12,
                    from include/linux/mod_devicetable.h:13,
                    from include/linux/pci.h:27,
                    from drivers/misc/spi_lpc/low_level_access.c:11:
   include/asm-generic/fixmap.h: In function 'fix_to_virt':
   include/asm-generic/fixmap.h:32:19: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |                   ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
      50 |  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
         |  ^~~~~~~~~~~~~~~~
   include/asm-generic/fixmap.h:32:2: note: in expansion of macro 'BUILD_BUG_ON'
      32 |  BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
         |  ^~~~~~~~~~~~
   In file included from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/huge_mm.h:8,
                    from include/linux/mm.h:675,
                    from include/linux/scatterlist.h:8,
                    from include/linux/dma-mapping.h:11,
                    from include/linux/pci-dma-compat.h:8,
                    from include/linux/pci.h:2415,
                    from drivers/misc/spi_lpc/low_level_access.c:11:
   arch/um/include/asm/uaccess.h: In function '__access_ok':
   arch/um/include/asm/uaccess.h:17:29: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
      17 |    (((unsigned long) (addr) >= FIXADDR_USER_START) && \
         |                             ^~
   arch/um/include/asm/uaccess.h:45:3: note: in expansion of macro '__access_ok_vsyscall'
      45 |   __access_ok_vsyscall(addr, size) ||
         |   ^~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/debugfs.h:15,
                    from drivers/misc/eeprom/idt_89hpesx.c:78:
   arch/um/include/asm/uaccess.h: In function '__access_ok':
   arch/um/include/asm/uaccess.h:17:29: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
      17 |    (((unsigned long) (addr) >= FIXADDR_USER_START) && \
         |                             ^~
   arch/um/include/asm/uaccess.h:45:3: note: in expansion of macro '__access_ok_vsyscall'
      45 |   __access_ok_vsyscall(addr, size) ||
         |   ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'pci_read_byte':
>> drivers/misc/spi_lpc/low_level_access.c:42:31: error: implicit declaration of function 'pci_find_bus'; did you mean 'pci_find_next_bus'? [-Werror=implicit-function-declaration]
      42 |   struct pci_bus *found_bus = pci_find_bus(0, bus);              \
         |                               ^~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:56:1: note: in expansion of macro 'GENERIC_PCI_READ'
      56 | GENERIC_PCI_READ(byte, u8)
         | ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:42:31: warning: initialization of 'struct pci_bus *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      42 |   struct pci_bus *found_bus = pci_find_bus(0, bus);              \
         |                               ^~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:56:1: note: in expansion of macro 'GENERIC_PCI_READ'
      56 | GENERIC_PCI_READ(byte, u8)
         | ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:46:10: error: implicit declaration of function 'pci_bus_read_config_byte'; did you mean 'pci_read_config_byte'? [-Werror=implicit-function-declaration]
      46 |    ret = pci_bus_read_config_##Suffix(                    \
         |          ^~~~~~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:56:1: note: in expansion of macro 'GENERIC_PCI_READ'
      56 | GENERIC_PCI_READ(byte, u8)
         | ^~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'pci_read_word':
>> drivers/misc/spi_lpc/low_level_access.c:42:31: warning: initialization of 'struct pci_bus *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      42 |   struct pci_bus *found_bus = pci_find_bus(0, bus);              \
         |                               ^~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:57:1: note: in expansion of macro 'GENERIC_PCI_READ'
      57 | GENERIC_PCI_READ(word, u16)
         | ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:46:10: error: implicit declaration of function 'pci_bus_read_config_word'; did you mean 'pci_read_config_word'? [-Werror=implicit-function-declaration]
      46 |    ret = pci_bus_read_config_##Suffix(                    \
         |          ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:57:1: note: in expansion of macro 'GENERIC_PCI_READ'
      57 | GENERIC_PCI_READ(word, u16)
         | ^~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c: In function 'pci_read_dword':
>> drivers/misc/spi_lpc/low_level_access.c:42:31: warning: initialization of 'struct pci_bus *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      42 |   struct pci_bus *found_bus = pci_find_bus(0, bus);              \
         |                               ^~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:58:1: note: in expansion of macro 'GENERIC_PCI_READ'
      58 | GENERIC_PCI_READ(dword, u32)
         | ^~~~~~~~~~~~~~~~
>> drivers/misc/spi_lpc/low_level_access.c:46:10: error: implicit declaration of function 'pci_bus_read_config_dword'; did you mean 'pci_read_config_dword'? [-Werror=implicit-function-declaration]
      46 |    ret = pci_bus_read_config_##Suffix(                    \
         |          ^~~~~~~~~~~~~~~~~~~~
   drivers/misc/spi_lpc/low_level_access.c:58:1: note: in expansion of macro 'GENERIC_PCI_READ'
      58 | GENERIC_PCI_READ(dword, u32)
         | ^~~~~~~~~~~~~~~~
   drivers/misc/enclosure.c:115: warning: Function parameter or member 'name' not described in 'enclosure_register'
   drivers/misc/enclosure.c:115: warning: Function parameter or member 'cb' not described in 'enclosure_register'
   drivers/misc/enclosure.c:283: warning: Function parameter or member 'number' not described in 'enclosure_component_alloc'
   drivers/misc/enclosure.c:283: warning: Excess function parameter 'num' description in 'enclosure_component_alloc'
   drivers/misc/enclosure.c:363: warning: Function parameter or member 'component' not described in 'enclosure_add_device'
   drivers/misc/enclosure.c:363: warning: Excess function parameter 'num' description in 'enclosure_add_device'
   drivers/misc/enclosure.c:398: warning: Function parameter or member 'dev' not described in 'enclosure_remove_device'
   drivers/misc/enclosure.c:398: warning: Excess function parameter 'num' description in 'enclosure_remove_device'
   cc1: some warnings being treated as errors
   make[4]: *** [scripts/Makefile.build:280: drivers/misc/spi_lpc/low_level_access.o] Error 1
   make[4]: Target '__build' not remade because of errors.
   make[3]: *** [scripts/Makefile.build:497: drivers/misc/spi_lpc] Error 2
   cc1: warning: arch/um/include/uapi: No such file or directory [-Wmissing-include-dirs]
   cc1: warning: arch/um/include/uapi: No such file or directory [-Wmissing-include-dirs]
   make[2]: *** [scripts/Makefile.build:497: drivers/misc] Error 2
   make[2]: Target '__build' not remade because of errors.
   make[3]: Target '__build' not remade because of errors.
..

vim +42 drivers/misc/spi_lpc/low_level_access.c

    36	
    37	#define GENERIC_PCI_READ(Suffix, Type)                                         \
    38		int pci_read_##Suffix(Type *value, u64 bus, u64 device, u64 function,  \
    39				      u64 offset)                                      \
    40		{                                                                      \
    41			int ret;                                                       \
  > 42			struct pci_bus *found_bus = pci_find_bus(0, bus);              \
    43			pr_debug("Reading PCI 0x%llx 0x%llx 0x%llx 0x%llx\n", bus,     \
    44				 device, function, offset);                            \
    45			if (found_bus != NULL) {                                       \
  > 46				ret = pci_bus_read_config_##Suffix(                    \
    47					found_bus, PCI_DEVFN(device, function),        \
    48					offset, value);                                \
    49			} else {                                                       \
    50				pr_err("Couldn't find Bus 0x%llx\n", bus);             \
    51				ret = -EIO;                                            \
    52			}                                                              \
    53			return ret;                                                    \
    54		}
    55	
  > 56	GENERIC_PCI_READ(byte, u8)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23002 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
       [not found]       ` <CAFmMkTE3z6OZQ_v3jx-4MzMr8v+4qcF2uLn0ASGydj5oqDnfjg@mail.gmail.com>
@ 2020-07-06  9:20         ` Arnd Bergmann
  2020-07-06  9:54           ` Arnd Bergmann
  2020-07-06 10:22         ` Arnd Bergmann
  1 sibling, 1 reply; 20+ messages in thread
From: Arnd Bergmann @ 2020-07-06  9:20 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Greg Kroah-Hartman, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Fri, Jul 3, 2020 at 10:43 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> On Tue, Jun 30, 2020 at 5:58 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> After analyzing the intel-spi driver, I came up to these observations that led
> me conclude that it is not what I need to use:
>
> * Some SPI Controllers have 0xFFFF as their VID DID bytes in the PCI
> config space. This causes that the PCI devices enumeration doesn't include them,
> and thus a PCI device driver won't be probed even despite listing the DID
> that the datasheet specifies. I effectively confirmed this by doing a PCI device
> driver and trying the intel-spi in systems with this characteristic. In short, the
> intel-spi driver doesn't work with these systems.

I don't understand what you mean here. How do *you* find the device if the
device-id is 0xffff?

My impression was that the spi-nor host is not a PCI device at all
but instead is a function on the mfd device, and probed as a
platform_driver with the "intel-spi" name.

> * Additionally, there is a difference of functionality: the intel-spi driver is a MTD,
> and -despite it effectively contains some of the DIDs my proposed driver
> supports- it doesn't offer an API to read the BIOS related registers (which is not the
> function read_register, since it reads chip registers). In fact, note that it reads
> HSFS from the PCI config space using MMIO, by just reading the mapped
> memory offset. The read register callback reads a different type of registers
> with a different mechanism (eg it does a writel before a readl). By being a MTD,
> it doesn't need to read for example the Bios Control register (which I need), or
> other registers that I need but are irrelevant for MTD operations.
>
> I should add a new interface to the driver just to read these registers or an
> interface to read an arbitrary offset from the mapped memory.
> * Finally, some of the registers I need are present in the PCI Config Space

Config space of *which* device in particular? Is this the Atom MFD device,
the SPI-NOR host, or something else?

> (others are in other places), but the offsets (and/or the offsets of the fields)
> vary depending on the architecture of the CPU or the PCH. That's why I
> first detect the architecture based on the VID/DID and then use the appropriate
> definition. Moreover, there are cases where the same registers  is obtained
> by reading the PCI CFG in some architectures, whereas it is obtained by
> reading MMIO in other architectures (eg Atom). All this information is provided
> in the Intel's datasheets.

Is this MMIO space part of a PCI device BAR, or how do you find it?
If it belongs to a PCI device, which one is this, and why can't there
be a driver for it?

> Because of these reasons, I'm proposing a misc (not-device) driver that supports
> many Intel architectures (and families) to expose the information.
> I understand your proposal to first enhance existing _device_ drivers, but I
> couldn't find suitable options.

Maybe try adding an interface to one of the drivers at first, and then extend
it to the other hardware after an initial code review. Do not bypass the driver
model or try to do everything at once with a single module that knows
details of multiple unrelated hardware implementations.

     Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
  2020-07-06  9:20         ` Arnd Bergmann
@ 2020-07-06  9:54           ` Arnd Bergmann
       [not found]             ` <CAFmMkTGkmBgmv6wmS1kNWxGm0ktN56u9pJVJQKyPvLipyHsgqw@mail.gmail.com>
  0 siblings, 1 reply; 20+ messages in thread
From: Arnd Bergmann @ 2020-07-06  9:54 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Greg Kroah-Hartman, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Mon, Jul 6, 2020 at 11:20 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> > Because of these reasons, I'm proposing a misc (not-device) driver that supports
> > many Intel architectures (and families) to expose the information.
> > I understand your proposal to first enhance existing _device_ drivers, but I
> > couldn't find suitable options.
>
> Maybe try adding an interface to one of the drivers at first, and then extend
> it to the other hardware after an initial code review. Do not bypass the driver
> model or try to do everything at once with a single module that knows
> details of multiple unrelated hardware implementations.

To clarify further how I think you can have a chance of getting the
interface you want, here's a step-by-step list:

1. keep the current securityfs interface (or any other user space
  ABI if you have already changed it), but put it into a separate loadable
  module with a pair of exported functions:
  spi_lpc_register_info(struct spi_lpc_data *info);
  spi_lpc_unregister_info(struct spi_lpc_data *info);
  The names will have to change later, but the idea is that any driver
  can just pass the information you want to export to user space,
  and the interface module does not care where those values came
  from.

2. For any PCI device ID that is handled by drivers/mtd/spi-nor/intel-spi-pci.c,
  move the code to call the function from your driver into that file,
  reading the registers with pci_read_config_dword() or
  readl(ispi->base + CONSTANT).

3. For any PCI device ID that is handled by drivers/mfd/lpc_ich.c,
  do the same by moving the code to
  drivers/mtd/spi-nor/controllers/intel-spi-platform.c.

4. For any remaining PCI device ID you look at that does not have
  a driver, create a new PCI driver that binds to those IDs and
  does the same thing. If you have multiple device IDs that
  use the same register layout, then handle those with a common
  driver.

      Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
       [not found]       ` <CAFmMkTE3z6OZQ_v3jx-4MzMr8v+4qcF2uLn0ASGydj5oqDnfjg@mail.gmail.com>
  2020-07-06  9:20         ` Arnd Bergmann
@ 2020-07-06 10:22         ` Arnd Bergmann
  1 sibling, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2020-07-06 10:22 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Derek Kiernan, Greg Kroah-Hartman, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk, linux-pci

On Fri, Jul 3, 2020 at 10:43 PM Daniel Gutson <daniel@eclypsium.com> wrote:

> After analyzing the intel-spi driver, I came up to these observations that led me conclude that it is not what I need to use:
>
> * Some SPI Controllers have 0xFFFF as their VID DID bytes in the PCI config space. This causes that the PCI devices enumeration doesn't include them,
> and thus a PCI device driver won't be probed even despite listing the DID that the datasheet specifies. I effectively confirmed this by doing a PCI device driver and
> trying the intel-spi in systems with this characteristic. In short, the intel-spi driver doesn't work with these systems.

Maybe this part can be handled with a fixup in drivers/pci that
changes the effective
PCI device ID on the systems that do need to access the device.

Adding the PCI mailing list for other ideas. Basically it sounds like
the BIOS has
intentionally configured the SPI-NOR device to be hidden from PCI probing to
prevent operating systems from accessing it, but you want to ignore that and
access it anyway, correct?

      Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] SPI LPC information kernel module
       [not found]             ` <CAFmMkTGkmBgmv6wmS1kNWxGm0ktN56u9pJVJQKyPvLipyHsgqw@mail.gmail.com>
@ 2020-07-14  6:38               ` Greg Kroah-Hartman
  0 siblings, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-14  6:38 UTC (permalink / raw)
  To: Daniel Gutson
  Cc: Arnd Bergmann, Derek Kiernan, Mauro Carvalho Chehab,
	linux-kernel, Richard Hughes, Alex Bazhaniuk

On Mon, Jul 13, 2020 at 07:24:11PM -0300, Daniel Gutson wrote:
> On Mon, Jul 6, 2020 at 6:54 AM Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Mon, Jul 6, 2020 at 11:20 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > > Because of these reasons, I'm proposing a misc (not-device) driver
> > that supports
> > > > many Intel architectures (and families) to expose the information.
> > > > I understand your proposal to first enhance existing _device_ drivers,
> > but I
> > > > couldn't find suitable options.
> > >
> > > Maybe try adding an interface to one of the drivers at first, and then
> > extend
> > > it to the other hardware after an initial code review. Do not bypass the
> > driver
> > > model or try to do everything at once with a single module that knows
> > > details of multiple unrelated hardware implementations.
> >
> > To clarify further how I think you can have a chance of getting the
> > interface you want, here's a step-by-step list:
> >
> > 1. keep the current securityfs interface (or any other user space
> >   ABI if you have already changed it), but put it into a separate loadable
> >   module
> 
> 
> If it is a loadable module, how can I ensure that it was loaded before the
> intel-spi driver, so the latter can call
> the API of the former? What if the driver was not loaded, when the
> intel-spi driver will try to call
> your suggested spi_lpc_register_info? In other words, how can I prevent to
> call functions from an unloaded driver?

THe symbol will not be resolved so the module will not be able to be
loaded in the first place, OR your module will be loaded first by the
system to prevent that.

> Do I just add the dependency in the Kconfig? But if so, what about the
> order of initialization?
> A hint please?

Try it and see what happens, it should all "just work" :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-07-14  6:38 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29 22:59 [PATCH] SPI LPC information kernel module Daniel Gutson
2020-06-30  8:02 ` kernel test robot
2020-06-30  8:56 ` Greg Kroah-Hartman
2020-06-30 13:57   ` Richard Hughes
2020-06-30 14:24     ` Arnd Bergmann
2020-06-30 15:14     ` Greg Kroah-Hartman
     [not found]   ` <CAFmMkTGrnZt7ZaGyYCe-LCHET4yHz9DfanaZwsOS6HCxK40apQ@mail.gmail.com>
2020-06-30 15:00     ` Arnd Bergmann
2020-06-30 15:28     ` Greg Kroah-Hartman
2020-06-30 15:32       ` Greg Kroah-Hartman
     [not found]       ` <CAFmMkTGy7u8oNSPmBHf9+URzKeNOxy5TJtqF3FCruRkTgJ_wGQ@mail.gmail.com>
2020-06-30 16:55         ` Greg Kroah-Hartman
2020-06-30  8:58 ` Arnd Bergmann
     [not found]   ` <CAFmMkTHrQ4LZk4+-3kdJ+dc47MXR1Jd76AXbO-ceT2zsfDRFGQ@mail.gmail.com>
2020-06-30 20:57     ` Arnd Bergmann
     [not found]       ` <CAFmMkTE3z6OZQ_v3jx-4MzMr8v+4qcF2uLn0ASGydj5oqDnfjg@mail.gmail.com>
2020-07-06  9:20         ` Arnd Bergmann
2020-07-06  9:54           ` Arnd Bergmann
     [not found]             ` <CAFmMkTGkmBgmv6wmS1kNWxGm0ktN56u9pJVJQKyPvLipyHsgqw@mail.gmail.com>
2020-07-14  6:38               ` Greg Kroah-Hartman
2020-07-06 10:22         ` Arnd Bergmann
2020-06-30  8:58 ` Greg Kroah-Hartman
2020-06-30  8:59 ` Greg Kroah-Hartman
2020-06-30  9:00 ` Greg Kroah-Hartman
2020-07-01  7:35 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).