Re: [PATCH] SPI LPC information kernel module
From: Arnd Bergmann
Date: Tue Jun 30 2020 - 04:58:41 EST
On Tue, Jun 30, 2020 at 12:59 AM Daniel Gutson
<daniel.gutson@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
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(®->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