Messages in this thread Patch in this message | | | From | Nicholas Johnson <> | Subject | Re: [PATCH v4 1/1] nvmem: Add support for write-only instances | Date | Wed, 18 Mar 2020 09:17:48 +0000 |
| |
On Tue, Mar 17, 2020 at 02:35:44PM +0000, Srinivas Kandagatla wrote: > > > On 17/03/2020 01:01, Nicholas Johnson wrote: > > 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> > > --- > > drivers/nvmem/core.c | 9 ++++-- > > drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------ > > 2 files changed, 54 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > > index ef326f243..95ea9a3b2 100644 > > --- a/drivers/nvmem/core.c > > +++ b/drivers/nvmem/core.c > > @@ -373,6 +373,12 @@ 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) { > > + kfree(nvmem); > > + return ERR_PTR(-EINVAL); > > When I meant leaking here, not just memory but other resources like idr and > gpio. > > You should do something like this: > > if (!nvmem->dev.groups) { > ida_simple_remove(&nvmem_ida, nvmem->id); > gpiod_put(nvmem->wp_gpio); > kfree(nvmem); > return ERR_PTR(-EINVAL); > } Thanks. I am guessing these are called indirectly when there are other failures (goto err_*). We have ida_simple_remove() in nvmem_release() but I cannot find gpiod_put anywhere in drivers/nvmem.
> > > + } > > + > > if (!config->no_of_node) > > nvmem->dev.of_node = config->dev->of_node; > > @@ -387,8 +393,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); > > @@ -430,7 +434,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) > > device_del(&nvmem->dev); > > err_put_device: > > put_device(&nvmem->dev); > > - > unnecessary cleanup here! Sorry, I let this mistake slip, despite checking my work.
> > Other than that every thing looks good to me! > > > --srini
Cheers, Nicholas > > > return ERR_PTR(rval); > > } > > EXPORT_SYMBOL_GPL(nvmem_register); > > diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c > > index 9e0c429cd..4ceac8680 100644 > > --- a/drivers/nvmem/nvmem-sysfs.c > > +++ b/drivers/nvmem/nvmem-sysfs.c > > @@ -196,16 +196,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, > > +}; > > + > > +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; > > - return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups; > > + return NULL; > > } > > /* > > @@ -224,17 +257,24 @@ int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem, > > if (!config->base_dev) > > return -EINVAL; > > - if (nvmem->read_only) { > > + if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only)) { > > if (config->root_only) > > nvmem->eeprom = bin_attr_ro_root_nvmem; > > else > > nvmem->eeprom = bin_attr_ro_nvmem; > > - } else { > > + } else if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) { > > + if (config->root_only) > > + nvmem->eeprom = bin_attr_wo_root_nvmem; > > + else > > + return -EINVAL; > > + } else if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) { > > if (config->root_only) > > nvmem->eeprom = bin_attr_rw_root_nvmem; > > else > > nvmem->eeprom = bin_attr_rw_nvmem; > > - } > > + } else > > + return -EINVAL; > > + > > nvmem->eeprom.attr.name = "eeprom"; > > nvmem->eeprom.size = nvmem->size; > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > >
| |