Re: [PATCH v3 2/2] firmware: google: Expose coreboot tables over sysfs

[Stephen Boyd <swboyd@chromium.org>]

Quoting patrick.rudolph@9elements.com (2019-11-28 04:50:51)
> diff --git a/Documentation/ABI/stable/sysfs-bus-coreboot b/Documentation/ABI/stable/sysfs-bus-coreboot
> index 1b04b8abc858..0f28601229f3 100644
> --- a/Documentation/ABI/stable/sysfs-bus-coreboot
> +++ b/Documentation/ABI/stable/sysfs-bus-coreboot
> @@ -41,3 +41,33 @@ Description:
>                 buffer.
>                 The file holds a read-only binary representation of the CBMEM
>                 buffer.
> +
> +What:          /sys/bus/coreboot/devices/.../attributes/id
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named attributes/id.
> +               The file holds an ASCII representation of the coreboot table ID
> +               in hex (e.g. 0x000000ef). On coreboot enabled platforms the ID is
> +               usually called TAG.

Why don't we call it 'tag' then?

> +
> +What:          /sys/bus/coreboot/devices/.../attributes/size
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               attributes/size.

Can we drop this first sentence in all the places? I don't see what
value it adds.

> +               The file holds an ASCII representation as decimal number of the
> +               coreboot table size (e.g. 64).
> +
> +What:          /sys/bus/coreboot/devices/.../attributes/data
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               attributes/data.
> +               The file holds a read-only binary representation of the coreboot
> +               table.

Maybe the attributes directory should be called 'table'? Given that
we're exposing the coreboot table contents directly to userspace?

> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index 8d132e4f008a..1f7329d72f54 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -6,6 +6,7 @@
>   *
>   * Copyright 2017 Google Inc.
>   * Copyright 2017 Samuel Holland <samuel@sholland.org>
> + * Copyright 2019 9elements Agency GmbH
>   */
>  
>  #include <linux/acpi.h>
> @@ -84,6 +85,60 @@ void coreboot_driver_unregister(struct coreboot_driver *driver)
>  }
>  EXPORT_SYMBOL(coreboot_driver_unregister);
>  
> +static ssize_t id_show(struct device *dev,
> +                      struct device_attribute *attr, char *buffer)
> +{
> +       struct coreboot_device *device = CB_DEV(dev);
> +
> +       return sprintf(buffer, "0x%08x\n", device->entry.tag);

Use %#08x instead?

> +}
> +
> +static ssize_t size_show(struct device *dev,
> +                        struct device_attribute *attr, char *buffer)
> +{
> +       struct coreboot_device *device = CB_DEV(dev);
> +
> +       return sprintf(buffer, "%u\n", device->entry.size);
> +}
> +
> +static DEVICE_ATTR_RO(id);
> +static DEVICE_ATTR_RO(size);
> +
> +static struct attribute *cb_dev_attrs[] = {
> +       &dev_attr_id.attr,
> +       &dev_attr_size.attr,
> +       NULL
> +};
> +
> +static ssize_t data_read(struct file *filp, struct kobject *kobj,
> +                        struct bin_attribute *bin_attr,
> +                        char *buffer, loff_t offset, size_t count)
> +{
> +       struct device *dev = kobj_to_dev(kobj);
> +       struct coreboot_device *device = CB_DEV(dev);
> +
> +       return memory_read_from_buffer(buffer, count, &offset,
> +                                      &device->entry, device->entry.size);
> +}
> +
> +static BIN_ATTR_RO(data, 0);

Can we fill in the size of the data at runtime somehow? Might require
making a different struct bin_attribute for each coreboot entry I
suppose but then we can probably leave out the 'size' attribute and only
have the 'data' and 'tag' attributes.

> +
> +static struct bin_attribute *cb_dev_bin_attrs[] = {
> +       &bin_attr_data,
> +       NULL
> +};