Messages in this thread Patch in this message | | | From | Stephen Boyd <> | Subject | Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs | Date | Mon, 16 Dec 2019 23:38:31 -0800 |
| |
Quoting patrick.rudolph@9elements.com (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@9elements.com> > +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@9elements.com> > +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/address > +Date: Nov 2019 > +KernelVersion: 5.5 > +Contact: Patrick Rudolph <patrick.rudolph@9elements.com> > +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?
> + > +What: /sys/bus/coreboot/devices/.../cbmem_attributes/data > +Date: Nov 2019 > +KernelVersion: 5.5 > +Contact: Patrick Rudolph <patrick.rudolph@9elements.com> > +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@gmail.com> > + * 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;
| |