Hello all,
Previous version: https://lkml.org/lkml/2020/3/9/731
Changed since previous version:
- Hopefully fixed memory leak fully. I set CONFIG_DEBUG_KMEMLEAK and
according to the leak detector, it is no longer leaking. I believe the
leak was when setting the device name. I could not find out how to
free that in the required context. But I avoided it by moving the
groups above setting the name.
If you believe there is still a leak, please be specific about what
you think it is. I have been reading up this week about the driver
interfaces and trying to figure it out, but I have had trouble finding
good reading material.
- I squashed the two patches together as advised by Srinivas.
- Changed from -EPERM to -EINVAL for invalid configuration.
Nicholas Johnson (1):
nvmem: Add support for write-only instances
drivers/nvmem/core.c | 9 ++++--
drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
2 files changed, 54 insertions(+), 11 deletions(-)
--
2.25.1
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]>
---
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);
+ }
+
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);
-
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
--
2.25.1
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 <[email protected]>
> ---
> 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);
}
> + }
> +
> 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!
Other than that every thing looks good to me!
--srini
> 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
>
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 <[email protected]>
> > ---
> > 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.
>
> > + }
> > +
> > 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
> >
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 <[email protected]>
>>> ---
>>> 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.
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.
--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
>>>
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 <[email protected]>
> > > > ---
> > > > 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
> > > >