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 12:42:05 +0000 |
| |
On Wed, Mar 18, 2020 at 09:28:59AM +0000, Srinivas Kandagatla wrote: > > > On 18/03/2020 09:17, Nicholas Johnson wrote: > > 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. > Because you are returning here before device intialize, you would need to do > the cleanup. Disregard, I just noticed this issue has been fixed in 8daa31303194 "nvmem: release the write-protect pin".
However, I found that we leak memory if returning after doing dev_set_name(). I worked around it by returning before it is even done. But for future reference, how would one free the name? > > Latest changes are available in linux-next or > https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git/tree/drivers/nvmem/core.c?h=for-next > > > Please rebase your patches on top of this branch, so its easy for me to > apply without conflicts. Done; there were no conflicts, anyway. Sending soon.
> > --srini > > > > > > > > > + } > > > > + > > > > 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 > > > >
| |