Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[patrick . rudolph]

On Mon, 2019-12-16 at 23:38 -0800, Stephen Boyd wrote:
> Quoting patrick.rudolph@xxxxxxxxxxxxx (2019-11-28 04:50:50)
> > diff --git a/Documentation/ABI/stable/sysfs-bus-coreboot
> > b/Documentation/ABI/stable/sysfs-bus-coreboot
> > new file mode 100644
> > index 000000000000..1b04b8abc858
> > --- /dev/null
> > +++ b/Documentation/ABI/stable/sysfs-bus-coreboot
> > @@ -0,0 +1,43 @@
> > +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/id
> > +Date:          Nov 2019
> > +KernelVersion: 5.5
> > +Contact:       Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> > +Description:
> > +               coreboot device directory can contain a file named
> > +               cbmem_attributes/id if the device corresponds to a
> > CBMEM
> > +               buffer.
> > +               The file holds an ASCII representation of the CBMEM
> > ID in hex
> > +               (e.g. 0xdeadbeef).
> > +
> > +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/size
> > +Date:          Nov 2019
> > +KernelVersion: 5.5
> > +Contact:       Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> > +Description:
> > +               coreboot device directory can contain a file named
> > +               cbmem_attributes/size if the device corresponds to
> > a CBMEM
> > +               buffer.
> > +               The file holds an representation as decimal number
> > of the
> > +               CBMEM buffer size (e.g. 64).
> > +
> > +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/addr
> > ess
> > +Date:          Nov 2019
> > +KernelVersion: 5.5
> > +Contact:       Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> > +Description:
> > +               coreboot device directory can contain a file named
> > +               cbmem_attributes/address if the device corresponds
> > to a CBMEM
> > +               buffer.
> > +               The file holds an ASCII representation of the
> > physical address
> > +               of the CBMEM buffer in hex (e.g.
> > 0x000000008000d000).
> 
> What is the purpose of this field? Can't userspace read the 'data'
> attribute to get the data out?

It's the linear address of the CBMEM buffer. It's more for debugging
purposes and could be matched with entries from the coreboot tables.
The 'data' field only returns the data itself, but says nothing about the
address stored. I'll update the documentation.

> 
> > +
> > +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/data
> > +Date:          Nov 2019
> > +KernelVersion: 5.5
> > +Contact:       Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> > +Description:
> > +               coreboot device directory can contain a file named
> > +               cbmem_attributes/data if the device corresponds to
> > a CBMEM
> > +               buffer.
> > +               The file holds a read-only binary representation of
> > the CBMEM
> > +               buffer.
> > diff --git a/drivers/firmware/google/cbmem-coreboot.c
> > b/drivers/firmware/google/cbmem-coreboot.c
> > new file mode 100644
> > index 000000000000..c67651a9c287
> > --- /dev/null
> > +++ b/drivers/firmware/google/cbmem-coreboot.c
> > @@ -0,0 +1,159 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * fmap-coreboot.c
> > + *
> > + * Exports CBMEM as attributes in sysfs.
> > + *
> > + * Copyright 2012-2013 David Herrmann <dh.herrmann@xxxxxxxxx>
> > + * Copyright 2017 Google Inc.
> > + * Copyright 2019 9elements Agency GmbH
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> 
> Is this include used? Should probably include linux/memremap.h
> instead.
> 
> > +#include <linux/slab.h>
> > +
> > +#include "coreboot_table.h"
> > +
> > +#define CB_TAG_CBMEM_ENTRY 0x31
> > +
> > +struct cb_priv {
> > +       void *remap;
> > +       struct lb_cbmem_entry entry;
> > +};
> > +
> > +static ssize_t id_show(struct device *dev,
> > +                      struct device_attribute *attr, char *buffer)
> > +{
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> 
> Please drop useless casts from void *.
> 
> > +
> > +       return sprintf(buffer, "0x%08x\n", priv->entry.id);
> 
> Use %#08x. Also not sure we need newlines but I guess it's OK.
> 
> > +}
> > +
> > +static ssize_t size_show(struct device *dev,
> > +                        struct device_attribute *attr, char
> > *buffer)
> > +{
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> > +
> > +       return sprintf(buffer, "%u\n", priv->entry.size);
> > +}
> > +
> > +static ssize_t address_show(struct device *dev,
> > +                           struct device_attribute *attr, char
> > *buffer)
> > +{
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> > +
> > +       return sprintf(buffer, "0x%016llx\n", priv->entry.address);
> > +}
> > +
> > +static DEVICE_ATTR_RO(id);
> > +static DEVICE_ATTR_RO(size);
> > +static DEVICE_ATTR_RO(address);
> > +
> > +static struct attribute *cb_mem_attrs[] = {
> > +       &dev_attr_address.attr,
> > +       &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);
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> > +
> > +       return memory_read_from_buffer(buffer, count, &offset,
> > +                                      priv->remap, priv-
> > >entry.size);
> > +}
> > +
> > +static BIN_ATTR_RO(data, 0);
> > +
> > +static struct bin_attribute *cb_mem_bin_attrs[] = {
> > +       &bin_attr_data,
> > +       NULL
> > +};
> > +
> > +static struct attribute_group cb_mem_attr_group = {
> 
> Can it be const?
> 
> > +       .name = "cbmem_attributes",
> > +       .attrs = cb_mem_attrs,
> > +       .bin_attrs = cb_mem_bin_attrs,
> > +};
> > +
> > +static int cbmem_probe(struct coreboot_device *cdev)
> > +{
> > +       struct device *dev = &cdev->dev;
> > +       struct cb_priv *priv;
> > +       int err;
> > +
> > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv-
> > >entry));
> > +
> > +       priv->remap = memremap(priv->entry.address,
> > +                              priv->entry.entry_size,
> > MEMREMAP_WB);
> > +       if (!priv->remap) {
> > +               err = -ENOMEM;
> > +               goto failure;
> > +       }
> > +
> > +       err = sysfs_create_group(&dev->kobj, &cb_mem_attr_group);
> > +       if (err) {
> > +               dev_err(dev, "failed to create sysfs attributes:
> > %d\n", err);
> > +               memunmap(priv->remap);
> > +               goto failure;
> > +       }
> > +
> > +       dev->platform_data = priv;
> 
> Can we use dev_{set,get}_drvdata() instead?
> 
> > +
> > +       return 0;
> > +failure:
> > +       kfree(priv);
> > +       return err;
> > +}
> > +
> > +static int cbmem_remove(struct coreboot_device *cdev)
> > +{
> > +       struct device *dev = &cdev->dev;
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> > +
> > +       sysfs_remove_group(&dev->kobj, &cb_mem_attr_group);
> > +
> > +       if (priv)
> > +               memunmap(priv->remap);
> > +       kfree(priv);
> > +
> > +       dev->platform_data = NULL;
> 
> This shouldn't be necessary if the driver_data APIs are used instead.
> The driver core does it for us then.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static struct coreboot_driver cbmem_driver = {
> > +       .probe = cbmem_probe,
> > +       .remove = cbmem_remove,
> > +       .drv = {
> > +               .name = "cbmem",
> > +       },
> > +       .tag = CB_TAG_CBMEM_ENTRY,
> > +};
> > +
> > +module_coreboot_driver(cbmem_driver);
> > +
> > +MODULE_AUTHOR("9elements Agency GmbH");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/firmware/google/coreboot_table.h
> > b/drivers/firmware/google/coreboot_table.h
> > index 7b7b4a6eedda..506048c724d8 100644
> > --- a/drivers/firmware/google/coreboot_table.h
> > +++ b/drivers/firmware/google/coreboot_table.h
> > @@ -59,6 +59,18 @@ struct lb_framebuffer {
> >         u8  reserved_mask_size;
> >  };
> >  
> > +/* There can be more than one of these records as there is one per
> > cbmem entry.
> 
> Please add a bare /* for multi-line comments.
> 
> > + * Describes a buffer in memory containing runtime data.
> > + */
> > +struct lb_cbmem_entry {
> > +       u32 tag;
> > +       u32 size;
> > +
> > +       u64 address;
> > +       u32 entry_size;
> > +       u32 id;
> > +};
> > +
> >  /* A device, additionally with information from coreboot. */
> >  struct coreboot_device {
> >         struct device dev;

Will send a new patch-set with the requested changes.