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>
 <CAK8P3a2miOXUESw+ayiEzAZ07NcieztVMEY31wUgrkD2tBAtDg@mail.gmail.com> <CAFmMkTG3rMtmNxYf0jXgGN7zJgK2U8Ogtrd4yH0Ee1rC7pf9Mg@mail.gmail.com>
In-Reply-To: <CAFmMkTG3rMtmNxYf0jXgGN7zJgK2U8Ogtrd4yH0Ee1rC7pf9Mg@mail.gmail.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 17 Jul 2020 17:39:37 +0200
Message-ID: <CAK8P3a384fKBEYU22UDOyr2vpdo+A+keoF_4jhcevG-w9rvtdw@mail.gmail.com>
Subject: Re: [PATCH] [PATCH] Firmware security information in SYSFS
To:     Daniel Gutson <daniel@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 5:21 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> On Fri, Jul 17, 2020 at 11:48 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> 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?
>
>
> Because that was what I understood from you :) and it made sense for me.
> Callbacks depend on the DID, so it can't be static. The returned value is dynamic too.
> The callback idea here is like the show() callback of the kobjects.
>
>>
>> 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.
>
>
> What information are you referring to?
> Please give me more details about your idea because using a callback is the only solution I can think about.

I still don't understand what the values are that you export. Can
you try to describe better what you are actually exporting to user
space, and how they change at runtime?

My impression was that the state of the registers you read would
be the same if you read them multiple times, at least until you
reboot and the firmware has a chance to change them.

>> > @@ -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?
>
>
> This came from clang-format, I thought it was a requirement so I left this
> fixed. Should I revert the changes from clang-format?

Yes, you should ensure that newly added code does not introduce
incorrect whitespace, but never mix cosmetic changes to existing
code with newly added functionality.

>> > @@ -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.
>
>
> More clang-format.

Ah, I had misread the change, thinking that you added the description, but
you really only change the whitespace, and in fact make it worse. ;-)

       Arnd