lkml.org 
[lkml]   [2020]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] [PATCH] Firmware security information in SYSFS
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

\
 
 \ /
  Last update: 2020-07-17 17:40    [W:0.078 / U:0.764 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site