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

[Greg KH]

On Tue, Mar 24, 2020 at 12:24:00PM +0000, Srinivas Kandagatla wrote:
> 
> 
> 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.

Why this crazy set of different groups?  You can set the mode of a sysfs
file in the callback for when the file is about to be created, that's so
much simpler and is what it is for.  This feels really hacky and almost
impossible to follow :(

thanks,

greg k-h