On Fri, Jul 17, 2020 at 12:36 AM Daniel Gutson
<daniel.gut...@eclypsium.com> wrote:
>
> +static ssize_t internal_callback(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct firmware_security_data *fwsd =
> + container_of(attr, struct firmware_security_data, kobj_attr);
> + return fwsd->callback(buf, fwsd->private_data);
> +}
> +
> +int register_firmware_security_data_callback(
> + const char *name, ssize_t (*callback)(char *buf, void *private_data),
> + void *private_data)
Why do you have a callback interface here? I would have expected
all the data to be static during the kernel run time, so just passing the
information here would make it much simpler.
> +static ssize_t cnl_read_field(char *buf, struct pci_dev *pdev, u32 mask)
> +{
> + u32 bcr;
> +
> + if (pci_read_config_dword(pdev, BCR, &bcr) != PCIBIOS_SUCCESSFUL)
> + return -EIO;
> +
> + return sprintf(buf, "%d\n", (int)!!(bcr & mask));
> +}
> +
> +static ssize_t cnl_wpd_firmware_security_callbacks(char *buf,
> + void *private_data)
> +{
> + return cnl_read_field(buf, (struct pci_dev *)private_data, BCR_WPD);
> +}
> +
> +static ssize_t cnl_ble_firmware_security_callbacks(char *buf,
> + void *private_data)
> +{
> + return cnl_read_field(buf, (struct pci_dev *)private_data, BCR_BLE);
> +}
> +
> +static ssize_t cnl_smm_bwp_firmware_security_callbacks(char *buf,
> + void *private_data)
> +{
> + return cnl_read_field(buf, (struct pci_dev *)private_data,
> BCR_SMM_BWP);
> +}
> +
> +static void register_firmware_security_data_callbacks(struct pci_dev *pdev)
> +{
> + register_firmware_security_data_callback(
> + "bioswe", &cnl_wpd_firmware_security_callbacks, pdev);
> + register_firmware_security_data_callback(
> + "ble", &cnl_ble_firmware_security_callbacks, pdev);
> + register_firmware_security_data_callback(
> + "smm_bwp", &cnl_smm_bwp_firmware_security_callbacks, pdev);
> +}
All of this should probably become a single function that reads the
registers and then passes the contents into whatever common function
you have that makes them visible to user space.
> @@ -76,7 +119,7 @@ static const struct pci_device_id intel_spi_pci_ids[] = {
> { PCI_VDEVICE(INTEL, 0xa224), (unsigned long)&bxt_info },
> { PCI_VDEVICE(INTEL, 0xa324), (unsigned long)&cnl_info },
> { PCI_VDEVICE(INTEL, 0xa3a4), (unsigned long)&bxt_info },
> - { },
> + {},
> };
...
> #include "intel-spi.h"
>
> @@ -48,17 +49,17 @@
>
> #define FADDR 0x08
> #define DLOCK 0x0c
> -#define FDATA(n) (0x10 + ((n) * 4))
> +#define FDATA(n) (0x10 + ((n)*4))
>
> #define FRACC 0x50
>
> -#define FREG(n) (0x54 + ((n) * 4))
> +#define FREG(n) (0x54 + ((n)*4))
> #define FREG_BASE_MASK 0x3fff
> #define FREG_LIMIT_SHIFT 16
> #define FREG_LIMIT_MASK (0x03fff << FREG_LIMIT_SHIFT)
>
Accidental whitespace change, please skip these changes?
> @@ -161,7 +164,8 @@ struct intel_spi {
>
> static bool writeable;
> module_param(writeable, bool, 0);
> -MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip
> (default=0)");
> +MODULE_PARM_DESC(writeable,
> + "Enable write access to SPI flash chip (default=0)");
>
> static void intel_spi_dump_regs(struct intel_spi *ispi)
> {
This looks like a useful change, but it should be a separate patch.
> @@ -179,8 +183,8 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
> dev_dbg(ispi->dev, "DLOCK=0x%08x\n", readl(ispi->base + DLOCK));
>
> for (i = 0; i < 16; i++)
> - dev_dbg(ispi->dev, "FDATA(%d)=0x%08x\n",
> - i, readl(ispi->base + FDATA(i)));
> + dev_dbg(ispi->dev, "FDATA(%d)=0x%08x\n", i,
> + readl(ispi->base + FDATA(i)));
>
> dev_dbg(ispi->dev, "FRACC=0x%08x\n", readl(ispi->base + FRACC));
>
> @@ -220,9 +224,8 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
> base = value & PR_BASE_MASK;
>
> dev_dbg(ispi->dev, " %02d base: 0x%08x limit: 0x%08x
> [%c%c]\n",
> - i, base << 12, (limit << 12) | 0xfff,
> - value & PR_WPE ? 'W' : '.',
> - value & PR_RPE ? 'R' : '.');
> + i, base << 12, (limit << 12) | 0xfff,
> + value & PR_WPE ? 'W' : '.', value & PR_RPE ? 'R' :
> '.');
> }
>
> dev_dbg(ispi->dev, "Flash regions:\n");
More whitespace changes.
Arnd
