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

[Srinivas Kandagatla]

On 23/03/2020 19:05, Greg KH wrote:
> On Mon, Mar 23, 2020 at 03:00:07PM +0000, Srinivas Kandagatla wrote:
> > From: Nicholas Johnson <nicholas.johnson-opensource@xxxxxxxxxxxxxx>
> >
> > 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>
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
> >   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?

This is not a new file, we already have documentation for this file at
./Documentation/ABI/stable/sysfs-bus-nvmem

Thanks for dropping this patch, I did endup finding some issues with 
this patch.

replied to original patch on the list with CC.

> > +
> > +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.

I agree, its bit confusing to the reader, but it does work.

But the Idea here is :
We ended up with providing different options like read-only,root-only to 
nvmem providers combined with read/write callbacks.
With that, there are some cases which are totally invalid, existing code 
does very minimal check to ensure that before populating with correct 
attributes to sysfs file. One of such case is with thunderbolt provider 
which supports only write callback.

With this new checks in place these flags and callbacks are correctly 
validated, would result in correct file attributes.


thanks,
srini

> greg k-h