Hi Greg,
Here are some nvmem patches for 5.7 which includes
- sprd nvmem provider fixes
- mxs-ocotp driver cleanup
- add proper checks for read/write callbacks and support root-write only
If its not too late, Can you please queue them up for 5.7.
thanks for you help,
srini
Anson Huang (1):
nvmem: mxs-ocotp: Use devm_add_action_or_reset() for cleanup
Baolin Wang (1):
nvmem: sprd: Determine double data programming from device data
Freeman Liu (2):
nvmem: sprd: Fix the block lock operation
nvmem: sprd: Optimize the block lock operation
Nicholas Johnson (1):
nvmem: Add support for write-only instances
drivers/nvmem/core.c | 10 +++++--
drivers/nvmem/mxs-ocotp.c | 30 ++++++++------------
drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
drivers/nvmem/sprd-efuse.c | 27 ++++++++++++++----
4 files changed, 89 insertions(+), 34 deletions(-)
--
2.21.0
From: Nicholas Johnson <[email protected]>
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 <[email protected]>
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
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,
+};
+
+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;
}
/*
@@ -230,17 +263,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
--
2.21.0
From: Baolin Wang <[email protected]>
We've saved the double data flag in the device data, so we should
use it when programming a block.
Signed-off-by: Baolin Wang <[email protected]>
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
drivers/nvmem/sprd-efuse.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/sprd-efuse.c b/drivers/nvmem/sprd-efuse.c
index 43b3f6ef8c20..925feb21d5ad 100644
--- a/drivers/nvmem/sprd-efuse.c
+++ b/drivers/nvmem/sprd-efuse.c
@@ -324,6 +324,7 @@ static int sprd_efuse_read(void *context, u32 offset, void *val, size_t bytes)
static int sprd_efuse_write(void *context, u32 offset, void *val, size_t bytes)
{
struct sprd_efuse *efuse = context;
+ bool blk_double = efuse->data->blk_double;
bool lock;
int ret;
@@ -348,7 +349,7 @@ static int sprd_efuse_write(void *context, u32 offset, void *val, size_t bytes)
else
lock = true;
- ret = sprd_efuse_raw_prog(efuse, offset, false, lock, val);
+ ret = sprd_efuse_raw_prog(efuse, offset, blk_double, lock, val);
clk_disable_unprepare(efuse->clk);
--
2.21.0
From: Freeman Liu <[email protected]>
According to the Spreadtrum eFuse specification, we should write 0 to
the block to trigger the lock operation.
Fixes: 096030e7f449 ("nvmem: sprd: Add Spreadtrum SoCs eFuse support")
Signed-off-by: Freeman Liu <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
drivers/nvmem/sprd-efuse.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvmem/sprd-efuse.c b/drivers/nvmem/sprd-efuse.c
index 2f1e0fbd1901..7a189ef52333 100644
--- a/drivers/nvmem/sprd-efuse.c
+++ b/drivers/nvmem/sprd-efuse.c
@@ -239,7 +239,7 @@ static int sprd_efuse_raw_prog(struct sprd_efuse *efuse, u32 blk, bool doub,
ret = -EBUSY;
} else {
sprd_efuse_set_prog_lock(efuse, lock);
- writel(*data, efuse->base + SPRD_EFUSE_MEM(blk));
+ writel(0, efuse->base + SPRD_EFUSE_MEM(blk));
sprd_efuse_set_prog_lock(efuse, false);
}
--
2.21.0
On Mon, Mar 23, 2020 at 03:00:03PM +0000, Srinivas Kandagatla wrote:
> From: Freeman Liu <[email protected]>
>
> According to the Spreadtrum eFuse specification, we should write 0 to
> the block to trigger the lock operation.
>
> Fixes: 096030e7f449 ("nvmem: sprd: Add Spreadtrum SoCs eFuse support")
> Signed-off-by: Freeman Liu <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> Signed-off-by: Srinivas Kandagatla <[email protected]>
> ---
> drivers/nvmem/sprd-efuse.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This should go to stable, I'll add the tag...
On Mon, Mar 23, 2020 at 03:00:07PM +0000, Srinivas Kandagatla wrote:
> From: Nicholas Johnson <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Srinivas Kandagatla <[email protected]>
> ---
> 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?
> +
> +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.
greg k-h
On Mon, Mar 23, 2020 at 03:00:02PM +0000, Srinivas Kandagatla wrote:
> Hi Greg,
>
> Here are some nvmem patches for 5.7 which includes
> - sprd nvmem provider fixes
> - mxs-ocotp driver cleanup
> - add proper checks for read/write callbacks and support root-write only
>
> If its not too late, Can you please queue them up for 5.7.
I've applied the first 4 patches, and provided review comments on the
last.
thanks,
greg k-h
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Srinivas Kandagatla <[email protected]>
> > ---
> > 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
On 23/03/2020 19:06, Greg KH wrote:
> On Mon, Mar 23, 2020 at 03:00:02PM +0000, Srinivas Kandagatla wrote:
>> Hi Greg,
>>
>> Here are some nvmem patches for 5.7 which includes
>> - sprd nvmem provider fixes
>> - mxs-ocotp driver cleanup
>> - add proper checks for read/write callbacks and support root-write only
>>
>> If its not too late, Can you please queue them up for 5.7.
>
> I've applied the first 4 patches, and provided review comments on the
> last.
>
Thanks Greg for dropping the last patch and adding stable tag, I did
find a 2 new issues with the last patch. Will discuss with Nicholas and
provide a proper patch once rc1 is out or in next cycle!
thanks,
srini
> thanks,
>
> greg k-h
>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Srinivas Kandagatla <[email protected]>
>> ---
>> 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
>
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Srinivas Kandagatla <[email protected]>
> > > ---
> > > 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
On 24/03/2020 12:29, Greg KH wrote:
>> 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 for the inputs, That definitely sounds much simpler to deal with.
Am guessing you are referring to is_bin_visible callback?
I will try to clean this up!
thanks,
srini
>
> thanks,
>
> greg k-h
On Tue, Mar 24, 2020 at 01:25:46PM +0000, Srinivas Kandagatla wrote:
>
>
> On 24/03/2020 12:29, Greg KH wrote:
> > > 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 for the inputs, That definitely sounds much simpler to deal with.
>
> Am guessing you are referring to is_bin_visible callback?
Yes.
On Tue, Mar 24, 2020 at 01:25:46PM +0000, Srinivas Kandagatla wrote:
>
>
> On 24/03/2020 12:29, Greg KH wrote:
> > > 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 for the inputs, That definitely sounds much simpler to deal with.
>
> Am guessing you are referring to is_bin_visible callback?
>
> I will try to clean this up!
I am still onboard and willing do the work, but we may need to discuss
to be on the same page with new plans. How do you wish to do this?
Does this new approach still allow us to abort if we receive an invalid
configuration? Or do we still need to have something in nvmem_register()
to abort in invalid case?
The documentation of is_bin_visible says only read/write permissions are
accepted. Does this mean that it will not take read-only or write-only?
That is one way of interpreting it.
I am further studying up on what was said in this email chain.
Regards,
Nicholas
>
> thanks,
> srini
> >
> > thanks,
> >
> > greg k-h
On Tue, Mar 24, 2020 at 02:24:21PM +0000, Nicholas Johnson wrote:
> On Tue, Mar 24, 2020 at 01:25:46PM +0000, Srinivas Kandagatla wrote:
> >
> >
> > On 24/03/2020 12:29, Greg KH wrote:
> > > > 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 for the inputs, That definitely sounds much simpler to deal with.
> >
> > Am guessing you are referring to is_bin_visible callback?
> >
> > I will try to clean this up!
> I am still onboard and willing do the work, but we may need to discuss
> to be on the same page with new plans. How do you wish to do this?
>
> Does this new approach still allow us to abort if we receive an invalid
> configuration? Or do we still need to have something in nvmem_register()
> to abort in invalid case?
>
> The documentation of is_bin_visible says only read/write permissions are
> accepted. Does this mean that it will not take read-only or write-only?
> That is one way of interpreting it.
That's a funny way of interpreting it :)
Please be sane, you pass back the permissions of the file, look at all
of the places in the kernel is it used for examples...
thanks,
greg k-h
On Tue, Mar 24, 2020 at 04:18:31PM +0100, Greg KH wrote:
> On Tue, Mar 24, 2020 at 02:24:21PM +0000, Nicholas Johnson wrote:
> > On Tue, Mar 24, 2020 at 01:25:46PM +0000, Srinivas Kandagatla wrote:
> > >
> > >
> > > On 24/03/2020 12:29, Greg KH wrote:
> > > > > 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 for the inputs, That definitely sounds much simpler to deal with.
> > >
> > > Am guessing you are referring to is_bin_visible callback?
> > >
> > > I will try to clean this up!
> > I am still onboard and willing do the work, but we may need to discuss
> > to be on the same page with new plans. How do you wish to do this?
> >
> > Does this new approach still allow us to abort if we receive an invalid
> > configuration? Or do we still need to have something in nvmem_register()
> > to abort in invalid case?
> >
> > The documentation of is_bin_visible says only read/write permissions are
> > accepted. Does this mean that it will not take read-only or write-only?
> > That is one way of interpreting it.
>
> That's a funny way of interpreting it :)
Now that I look back, yes.
>
> Please be sane, you pass back the permissions of the file, look at all
> of the places in the kernel is it used for examples...
It's more inexperience and sleep deprivation than insanity. I am working
on those. :)
There is only one use of is_bin_visible but a lot for is_visible, so I
will go off those.
Regards,
Nicholas
>
> thanks,
>
> greg k-h
On 24/03/2020 14:24, Nicholas Johnson wrote:
>> Am guessing you are referring to is_bin_visible callback?
>>
>> I will try to clean this up!
> I am still onboard and willing do the work, but we may need to discuss
> to be on the same page with new plans. How do you wish to do this?
I have done some cleanup of old code to use is_bin_visible.
https://git.linaro.org/people/srinivas.kandagatla/linux.git/log/?h=topic/is_bin_visible
Can you try these two patches along with your new patch?
--srini