lkml.org 
[lkml]   [2020]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v4 1/1] nvmem: Add support for write-only instances
From
Date


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 <nicholas.johnson-opensource@outlook.com.au>
> ---
> 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
>

\
 
 \ /
  Last update: 2020-03-17 15:36    [W:0.042 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site