Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200716223627.253936-1-daniel.gutson@eclypsium.com>
In-Reply-To: <20200716223627.253936-1-daniel.gutson@eclypsium.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 17 Jul 2020 16:48:09 +0200
Message-ID: <CAK8P3a2miOXUESw+ayiEzAZ07NcieztVMEY31wUgrkD2tBAtDg@mail.gmail.com>
Subject: Re: [PATCH] [PATCH] Firmware security information in SYSFS
To:     Daniel Gutson <daniel.gutson@eclypsium.com>
Cc:     Derek Kiernan <derek.kiernan@xilinx.com>,
        Tudor Ambarus <tudor.ambarus@microchip.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Richard Hughes <hughsient@gmail.com>,
        Alex Bazhaniuk <alex@eclypsium.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 17, 2020 at 12:36 AM Daniel Gutson
<daniel.gutson@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