Re: [PATCH v4 1/1] nvmem: Add support for write-only instances

[Nicholas Johnson]

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@xxxxxxxxxxxxxx>
> > > > ---
> > > >    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
> > > >