Messages in this thread | | | From | Nicholas Johnson <> | Subject | Re: [PATCH 5/5] nvmem: Add support for write-only instances | Date | Tue, 24 Mar 2020 03:25:31 +0000 |
| |
On Mon, Mar 23, 2020 at 08:05:05PM +0100, Greg KH wrote: > On Mon, Mar 23, 2020 at 03:00:07PM +0000, Srinivas Kandagatla wrote: > > From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> > > > > There is at least one real-world use-case for write-only nvmem > > instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if > > non-active NVMem file is read"). > > > > Add support for write-only nvmem instances by adding attrs for 0200. > > > > Change nvmem_register() to abort if NULL group is returned from > > nvmem_sysfs_get_groups(). > > > > Return NULL from nvmem_sysfs_get_groups() in invalid cases. > > > > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > --- > > drivers/nvmem/core.c | 10 +++++-- > > drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------ > > 2 files changed, 56 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > > index 77d890d3623d..ddc7be5149d5 100644 > > --- a/drivers/nvmem/core.c > > +++ b/drivers/nvmem/core.c > > @@ -381,6 +381,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) > > nvmem->type = config->type; > > nvmem->reg_read = config->reg_read; > > nvmem->reg_write = config->reg_write; > > + nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config); > > + if (!nvmem->dev.groups) { > > + ida_simple_remove(&nvmem_ida, nvmem->id); > > + gpiod_put(nvmem->wp_gpio); > > + kfree(nvmem); > > + return ERR_PTR(-EINVAL); > > + } > > + > > if (!config->no_of_node) > > nvmem->dev.of_node = config->dev->of_node; > > > > @@ -395,8 +403,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) > > nvmem->read_only = device_property_present(config->dev, "read-only") || > > config->read_only || !nvmem->reg_write; > > > > - nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config); > > - > > device_initialize(&nvmem->dev); > > > > dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name); > > diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c > > index 8759c4470012..9728da948988 100644 > > --- a/drivers/nvmem/nvmem-sysfs.c > > +++ b/drivers/nvmem/nvmem-sysfs.c > > @@ -202,16 +202,49 @@ static const struct attribute_group *nvmem_ro_root_dev_groups[] = { > > NULL, > > }; > > > > +/* write only permission, root only */ > > +static struct bin_attribute bin_attr_wo_root_nvmem = { > > + .attr = { > > + .name = "nvmem", > > + .mode = 0200, > > + }, > > + .write = bin_attr_nvmem_write, > > +}; > > You are adding a new sysfs file without a Documentation/ABI/ new entry > as well? It is the same existing sysfs file, but adding the ability for that existing sysfs file to have write-only (0200) permissions if the driver requires it. The perms / groups for the nvmem that is getting registered in nvmem_register() in drivers/nvmem/core.c are chosen by nvmem_sysfs_get_groups() in drivers/nvmem/nvmem-sysfs.c, and this new 0200 will get selected if reg_read is NULL and reg_write is provided (and nvmem->read_only override is not set).
> > > > + > > +static struct bin_attribute *nvmem_bin_wo_root_attributes[] = { > > + &bin_attr_wo_root_nvmem, > > + NULL, > > +}; > > + > > +static const struct attribute_group nvmem_bin_wo_root_group = { > > + .bin_attrs = nvmem_bin_wo_root_attributes, > > + .attrs = nvmem_attrs, > > +}; > > + > > +static const struct attribute_group *nvmem_wo_root_dev_groups[] = { > > + &nvmem_bin_wo_root_group, > > + NULL, > > +}; > > + > > const struct attribute_group **nvmem_sysfs_get_groups( > > struct nvmem_device *nvmem, > > const struct nvmem_config *config) > > { > > - if (config->root_only) > > - return nvmem->read_only ? > > - nvmem_ro_root_dev_groups : > > - nvmem_rw_root_dev_groups; > > + /* Read-only */ > > + if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only)) > > + return config->root_only ? > > + nvmem_ro_root_dev_groups : nvmem_ro_dev_groups; > > + > > + /* Read-write */ > > + if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) > > + return config->root_only ? > > + nvmem_rw_root_dev_groups : nvmem_rw_dev_groups; > > + > > + /* Write-only, do not honour request for global writable entry */ > > + if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) > > + return config->root_only ? nvmem_wo_root_dev_groups : NULL; > > > What is this supposed to be doing, I am totally lost. This nvmem_sysfs_get_groups() is called by nvmem_register() when the nvmem is registered by a driver. This returns the filesystem perms for the nvmem based on the inputs (or NULL to abort). There are four things we care about in the function arguments:
1. nvmem->reg_read which is a function pointer used when reading from the nvmem
2. nvmem->reg_write which is a function pointer used when writing to the nvmem
3. nvmem->read_only which overrides if nvmem is read only
4. config->root_only which indicates if world perms are used
We use these to decide which perms to return. We can determine whether read-only or write-only or read-write by whether the reg_read and reg_write are provided, using nvmem->read_only as an override (sometimes from dtb, both reg_read and reg_write are provided but it can override to read only with this flag).
Once that is decided, we use config->root_only to determine if world-accessible. We return the appropriate group, or NULL if world-writable requested.
Before this patch, we could not return NULL (always had to return a group, no matter what). This had to be fixed, hence the changes to the caller (nvmem_register() function in drivers/nvmem/core.c) to check for NULL group and abort. Before this patch, any (unsupported) request for write-only was interpreted as read-write, instead of returning an error and failing cleanly. As of this patch, the above 0200 which I am introducing is returned.
Please let me know if I have failed to clarify this for you and I will try again. :) > > greg k-h
Kind regards, Nicholas Johnson
| |