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

From: Nicholas Johnson
Date: Mon Mar 09 2020 - 22:45:14 EST


On Mon, Mar 09, 2020 at 05:50:17PM +0000, 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().
Sorry, this is the comment to remove which only applies to patch 1/2.
Disregard the past message. I will try to get more sleep.

>
> Return NULL from nvmem_sysfs_get_groups() in invalid cases.
Keep this one.

Cheers

>
> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@xxxxxxxxxxxxxx>
> ---
> drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
> 1 file changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> index 9e0c429cd..846112786 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;
>
> - return nvmem->read_only ? nvmem_ro_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 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 -EPERM;
> + } 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 -EPERM;
> +
> nvmem->eeprom.attr.name = "eeprom";
> nvmem->eeprom.size = nvmem->size;
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> --
> 2.25.1
>