[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <PSXP216MB04388FD469630B8F353A375A80F10@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM>
Date: Tue, 24 Mar 2020 03:25:31 +0000
From: Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
To: Greg KH <gregkh@...uxfoundation.org>
CC: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/5] nvmem: Add support for write-only instances
On Mon, Mar 23, 2020 at 08:05:05PM +0100, Greg KH wrote:
> On Mon, Mar 23, 2020 at 03:00:07PM +0000, Srinivas Kandagatla wrote:
> > From: Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
> >
> > 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@...look.com.au>
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> > ---
> > 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?
It is the same existing sysfs file, but adding the ability for that
existing sysfs file to have write-only (0200) permissions if the driver
requires it. The perms / groups for the nvmem that is getting registered
in nvmem_register() in drivers/nvmem/core.c are chosen by
nvmem_sysfs_get_groups() in drivers/nvmem/nvmem-sysfs.c, and this new
0200 will get selected if reg_read is NULL and reg_write is provided
(and nvmem->read_only override is not set).
>
>
> > +
> > +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.
This nvmem_sysfs_get_groups() is called by nvmem_register() when the
nvmem is registered by a driver. This returns the filesystem perms for
the nvmem based on the inputs (or NULL to abort). There are four things
we care about in the function arguments:
1. nvmem->reg_read which is a function pointer used when reading from
the nvmem
2. nvmem->reg_write which is a function pointer used when writing to the
nvmem
3. nvmem->read_only which overrides if nvmem is read only
4. config->root_only which indicates if world perms are used
We use these to decide which perms to return. We can determine whether
read-only or write-only or read-write by whether the reg_read and
reg_write are provided, using nvmem->read_only as an override (sometimes
from dtb, both reg_read and reg_write are provided but it can override
to read only with this flag).
Once that is decided, we use config->root_only to determine if
world-accessible. We return the appropriate group, or NULL if
world-writable requested.
Before this patch, we could not return NULL (always had to return a
group, no matter what). This had to be fixed, hence the changes to the
caller (nvmem_register() function in drivers/nvmem/core.c) to check for
NULL group and abort. Before this patch, any (unsupported) request for
write-only was interpreted as read-write, instead of returning an error
and failing cleanly. As of this patch, the above 0200 which I am
introducing is returned.
Please let me know if I have failed to clarify this for you and I will
try again. :)
>
> greg k-h
Kind regards,
Nicholas Johnson
Powered by blists - more mailing lists