Messages in this thread | | | From | Arnd Bergmann <> | Date | Tue, 30 Jun 2020 10:58:17 +0200 | Subject | Re: [PATCH] SPI LPC information kernel module |
| |
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(®->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
| |