On Thu, Aug 20, 2020 at 4:51 PM Daniel Gutson <daniel.gut...@eclypsium.com> wrote: > > This patch exports information about the platform lockdown > firmware configuration in the sysfs filesystem. > In this initial patch, I include some 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. > > The patch provides a new misc driver, as proposed in the previous patch, > that provides a registration function for HW Driver devices to register > class_attributes. > In this case, the intel SPI flash chip (intel-spi) registers three > class_attributes corresponding to the fields mentioned above. > > This version of the patch replaces class attributes by device > attributes. > > Signed-off-by: Daniel Gutson <daniel.gut...@eclypsium.com>
This looks much better than before, thanks for addressing the feedback. > diff --git a/Documentation/ABI/stable/sysfs-class-platform-lockdown > b/Documentation/ABI/stable/sysfs-class-platform-lockdown > new file mode 100644 > index 000000000000..3fe75d775a42 > --- /dev/null > +++ b/Documentation/ABI/stable/sysfs-class-platform-lockdown > @@ -0,0 +1,23 @@ > +What: /sys/class/platform-lockdown/bioswe platform-lockdown is a much better name than the previous suggestions. I'm still hoping for an even better suggestion. Like everything the term "lockdown" is also overloaded a bit, with the other common meaning referring to the effort to give root users less privilege than the kernel itself, see https://lwn.net/Articles/750730/ Shouldn't there be a device name between the class name ("platform-lockdown") and the attribute name? > +PLATFORM LOCKDOWN ATTRIBUTES MODULE > +M: Daniel Gutson <daniel.gut...@eclypsium.com> > +S: Supported > +F: Documentation/ABI/sysfs-class-platform-lockdown > +F: drivers/misc/platform-lockdown-attrs.c > +F: include/linux/platform_data/platform-lockdown-attrs.h include/linux/platform_data/ is not the right place for the header, this is defined to be the place for defining properties of devices that are created from old-style board files. Just put the header into include/linux/ directly. the host. > > +config PLATFORM_LOCKDOWN_ATTRS > + tristate "Platform lockdown information in the sysfs" > + depends on SYSFS > + help > + This kernel module is a helper driver to provide information about > + platform lockdown settings and configuration. > + This module is used by other device drivers -such as the intel-spi- > + to publish the information in /sys/class/platform-lockdown. Maybe mention fwupd in the description in some form. > + > +static struct class platform_lockdown_class = { > + .name = "platform-lockdown", > + .owner = THIS_MODULE, > +}; > + > +struct device *register_platform_lockdown_data_device(struct device *parent, > + const char *name) > +{ > + return device_create(&platform_lockdown_class, parent, MKDEV(0, 0), > + NULL, name); > +} > +EXPORT_SYMBOL_GPL(register_platform_lockdown_data_device); > + > +void unregister_platform_lockdown_data_device(struct device *dev) > +{ > + device_unregister(dev); > +} > +EXPORT_SYMBOL_GPL(unregister_platform_lockdown_data_device); > + > +int register_platform_lockdown_attribute(struct device *dev, > + struct device_attribute *dev_attr) > +{ > + return device_create_file(dev, dev_attr); > +} > +EXPORT_SYMBOL_GPL(register_platform_lockdown_attribute); > + > +void register_platform_lockdown_attributes(struct device *dev, > + struct device_attribute > dev_attrs[]) > +{ > + u32 idx = 0; > + > + while (dev_attrs[idx].attr.name != NULL) { > + register_platform_lockdown_attribute(dev, &dev_attrs[idx]); > + idx++; > + } There is a bit of a race with creating the device first and then the attributes. Generally it seems better to me to use device_create_with_groups() instead so the device shows up with all attributes in place already. > +void register_platform_lockdown_custom_attributes(struct device *dev, > + void *custom_attrs, > + size_t dev_attr_offset, > + size_t custom_attr_size) This interface seems to be overly complex, I would hope it can be avoided. > +static ssize_t cnl_spi_attr_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + u32 bcr; > + struct cnl_spi_attr *cnl_spi_attr = container_of(attr, > + struct cnl_spi_attr, dev_attr); > + > + if (class_child_device != dev) > + return -EIO; > + > + if (dev->parent == NULL) > + return -EIO; > + > + if (pci_read_config_dword(container_of(dev->parent, struct pci_dev, > dev), > + BCR, &bcr) != PCIBIOS_SUCCESSFUL) > + return -EIO; > + > + return sprintf(buf, "%d\n", (int)!!(bcr & cnl_spi_attr->mask)); > +} If I understand it right, that complexity comes from attempting to have a single show callback for three different flags. To me that actually feels more complicated than having an attribute group with three similar but simpler show callbacks. > static void intel_spi_pci_remove(struct pci_dev *pdev) > { > + if (class_child_device != NULL) { Please avoid the global variable here and just add a member in the per-device data. > + unregister_platform_lockdown_custom_attributes( > + class_child_device, > + cnl_spi_attrs, > + offsetof(struct cnl_spi_attr, dev_attr), > + sizeof(struct cnl_spi_attr)); > + > + unregister_platform_lockdown_data_device(class_child_device); It should be possible to just destroy the attributes as part of unregister_platform_lockdown_data_device. Arnd