[PATCH v2 0/2] nvmem: use is_bin_visible callback

[PATCH v2 0/2] nvmem: use is_bin_visible callback

[Srinivas Kandagatla]

Hi Greg,

As suggested I managed to use is_bin_visible for the existing code
and also added few more checks for callbacks before setting
permissions on the file. Which also means that Thunderbolt case
for write-only should be fixed automatically with this patch.

Changes since v1:
	- Updated permissions setup logic as suggested by Greg
	- Added checks for callbacks.

Thanks,
srini

Srinivas Kandagatla (2):
  nvmem: core: add root_only member to nvmem device struct
  nvmem: core: use is_bin_visible for permissions

 drivers/nvmem/core.c        |  1 +
 drivers/nvmem/nvmem-sysfs.c | 85 +++++++++++++------------------------
 drivers/nvmem/nvmem.h       |  1 +
 3 files changed, 31 insertions(+), 56 deletions(-)

-- 
2.21.0

[PATCH v2 1/2] nvmem: core: add root_only member to nvmem device struct

[Srinivas Kandagatla]

As we are planning to move to use sysfs is_bin_visible callback,
having root_only as part of nvmem_device will help decide correct
permissions.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/core.c  | 1 +
 drivers/nvmem/nvmem.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index e8f7bea93abf..7d28e1cca4e0 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -377,6 +377,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	nvmem->dev.type = &nvmem_provider_type;
 	nvmem->dev.bus = &nvmem_bus_type;
 	nvmem->dev.parent = config->dev;
+	nvmem->root_only = config->root_only;
 	nvmem->priv = config->priv;
 	nvmem->type = config->type;
 	nvmem->reg_read = config->reg_read;
diff --git a/drivers/nvmem/nvmem.h b/drivers/nvmem/nvmem.h
index be0d66d75c8a..16c0d3ad6679 100644
--- a/drivers/nvmem/nvmem.h
+++ b/drivers/nvmem/nvmem.h
@@ -20,6 +20,7 @@ struct nvmem_device {
 	struct kref		refcnt;
 	size_t			size;
 	bool			read_only;
+	bool			root_only;
 	int			flags;
 	enum nvmem_type		type;
 	struct bin_attribute	eeprom;
-- 
2.21.0

[PATCH v2 2/2] nvmem: core: use is_bin_visible for permissions

[Srinivas Kandagatla]

By using is_bin_visible callback to set permissions will remove a
large list of attribute groups. These group permissions can be
dynamically derived in the callback.

Also add checks for read/write callbacks and set permissions accordingly.

Suggested-by: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Changes since v1:
	- Updated permissions setup logic as suggested by Greg
	- Added checks for callbacks.

 drivers/nvmem/nvmem-sysfs.c | 85 +++++++++++++------------------------
 1 file changed, 29 insertions(+), 56 deletions(-)

diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
index 8759c4470012..68ad8aef79d4 100644
--- a/drivers/nvmem/nvmem-sysfs.c
+++ b/drivers/nvmem/nvmem-sysfs.c
@@ -104,6 +104,28 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 	return count;
 }
 
+static umode_t nvmem_bin_attr_is_visible(struct kobject *kobj,
+					 struct bin_attribute *attr, int i)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct nvmem_device *nvmem = to_nvmem_device(dev);
+	umode_t mode = 0400;
+
+	if (!nvmem->root_only)
+		mode |= 0044;
+
+	if (!nvmem->read_only)
+		mode |= 0200;
+
+	if (!nvmem->reg_write)
+		mode &= ~0200;
+
+	if (!nvmem->reg_read)
+		mode &= ~0444;
+
+	return mode;
+}
+
 /* default read/write permissions */
 static struct bin_attribute bin_attr_rw_nvmem = {
 	.attr	= {
@@ -114,18 +136,19 @@ static struct bin_attribute bin_attr_rw_nvmem = {
 	.write	= bin_attr_nvmem_write,
 };
 
-static struct bin_attribute *nvmem_bin_rw_attributes[] = {
+static struct bin_attribute *nvmem_bin_attributes[] = {
 	&bin_attr_rw_nvmem,
 	NULL,
 };
 
-static const struct attribute_group nvmem_bin_rw_group = {
-	.bin_attrs	= nvmem_bin_rw_attributes,
+static const struct attribute_group nvmem_bin_group = {
+	.bin_attrs	= nvmem_bin_attributes,
 	.attrs		= nvmem_attrs,
+	.is_bin_visible = nvmem_bin_attr_is_visible,
 };
 
-static const struct attribute_group *nvmem_rw_dev_groups[] = {
-	&nvmem_bin_rw_group,
+static const struct attribute_group *nvmem_dev_groups[] = {
+	&nvmem_bin_group,
 	NULL,
 };
 
@@ -138,21 +161,6 @@ static struct bin_attribute bin_attr_ro_nvmem = {
 	.read	= bin_attr_nvmem_read,
 };
 
-static struct bin_attribute *nvmem_bin_ro_attributes[] = {
-	&bin_attr_ro_nvmem,
-	NULL,
-};
-
-static const struct attribute_group nvmem_bin_ro_group = {
-	.bin_attrs	= nvmem_bin_ro_attributes,
-	.attrs		= nvmem_attrs,
-};
-
-static const struct attribute_group *nvmem_ro_dev_groups[] = {
-	&nvmem_bin_ro_group,
-	NULL,
-};
-
 /* default read/write permissions, root only */
 static struct bin_attribute bin_attr_rw_root_nvmem = {
 	.attr	= {
@@ -163,21 +171,6 @@ static struct bin_attribute bin_attr_rw_root_nvmem = {
 	.write	= bin_attr_nvmem_write,
 };
 
-static struct bin_attribute *nvmem_bin_rw_root_attributes[] = {
-	&bin_attr_rw_root_nvmem,
-	NULL,
-};
-
-static const struct attribute_group nvmem_bin_rw_root_group = {
-	.bin_attrs	= nvmem_bin_rw_root_attributes,
-	.attrs		= nvmem_attrs,
-};
-
-static const struct attribute_group *nvmem_rw_root_dev_groups[] = {
-	&nvmem_bin_rw_root_group,
-	NULL,
-};
-
 /* read only permission, root only */
 static struct bin_attribute bin_attr_ro_root_nvmem = {
 	.attr	= {
@@ -187,31 +180,11 @@ static struct bin_attribute bin_attr_ro_root_nvmem = {
 	.read	= bin_attr_nvmem_read,
 };
 
-static struct bin_attribute *nvmem_bin_ro_root_attributes[] = {
-	&bin_attr_ro_root_nvmem,
-	NULL,
-};
-
-static const struct attribute_group nvmem_bin_ro_root_group = {
-	.bin_attrs	= nvmem_bin_ro_root_attributes,
-	.attrs		= nvmem_attrs,
-};
-
-static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
-	&nvmem_bin_ro_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;
-
-	return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
+	return nvmem_dev_groups;
 }
 
 /*
-- 
2.21.0

Re: [PATCH v2 2/2] nvmem: core: use is_bin_visible for permissions

[Greg KH]

On Wed, Mar 25, 2020 at 10:01:38AM +0000, Srinivas Kandagatla wrote:
> By using is_bin_visible callback to set permissions will remove a
> large list of attribute groups. These group permissions can be
> dynamically derived in the callback.
> 
> Also add checks for read/write callbacks and set permissions accordingly.
> 
> Suggested-by: Greg KH <gregkh@linuxfoundation.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> Changes since v1:
> 	- Updated permissions setup logic as suggested by Greg
> 	- Added checks for callbacks.
> 
>  drivers/nvmem/nvmem-sysfs.c | 85 +++++++++++++------------------------
>  1 file changed, 29 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> index 8759c4470012..68ad8aef79d4 100644
> --- a/drivers/nvmem/nvmem-sysfs.c
> +++ b/drivers/nvmem/nvmem-sysfs.c
> @@ -104,6 +104,28 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
>  	return count;
>  }
>  
> +static umode_t nvmem_bin_attr_is_visible(struct kobject *kobj,
> +					 struct bin_attribute *attr, int i)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct nvmem_device *nvmem = to_nvmem_device(dev);
> +	umode_t mode = 0400;
> +
> +	if (!nvmem->root_only)
> +		mode |= 0044;
> +
> +	if (!nvmem->read_only)
> +		mode |= 0200;
> +
> +	if (!nvmem->reg_write)
> +		mode &= ~0200;
> +
> +	if (!nvmem->reg_read)
> +		mode &= ~0444;
> +
> +	return mode;
> +}
> +
>  /* default read/write permissions */
>  static struct bin_attribute bin_attr_rw_nvmem = {
>  	.attr	= {
> @@ -114,18 +136,19 @@ static struct bin_attribute bin_attr_rw_nvmem = {
>  	.write	= bin_attr_nvmem_write,
>  };
>  
> -static struct bin_attribute *nvmem_bin_rw_attributes[] = {
> +static struct bin_attribute *nvmem_bin_attributes[] = {
>  	&bin_attr_rw_nvmem,
>  	NULL,
>  };
>  
> -static const struct attribute_group nvmem_bin_rw_group = {
> -	.bin_attrs	= nvmem_bin_rw_attributes,
> +static const struct attribute_group nvmem_bin_group = {
> +	.bin_attrs	= nvmem_bin_attributes,
>  	.attrs		= nvmem_attrs,
> +	.is_bin_visible = nvmem_bin_attr_is_visible,
>  };
>  
> -static const struct attribute_group *nvmem_rw_dev_groups[] = {
> -	&nvmem_bin_rw_group,
> +static const struct attribute_group *nvmem_dev_groups[] = {
> +	&nvmem_bin_group,
>  	NULL,
>  };
>  
> @@ -138,21 +161,6 @@ static struct bin_attribute bin_attr_ro_nvmem = {
>  	.read	= bin_attr_nvmem_read,
>  };
>  
> -static struct bin_attribute *nvmem_bin_ro_attributes[] = {
> -	&bin_attr_ro_nvmem,
> -	NULL,
> -};
> -
> -static const struct attribute_group nvmem_bin_ro_group = {
> -	.bin_attrs	= nvmem_bin_ro_attributes,
> -	.attrs		= nvmem_attrs,
> -};
> -
> -static const struct attribute_group *nvmem_ro_dev_groups[] = {
> -	&nvmem_bin_ro_group,
> -	NULL,
> -};
> -
>  /* default read/write permissions, root only */
>  static struct bin_attribute bin_attr_rw_root_nvmem = {
>  	.attr	= {
> @@ -163,21 +171,6 @@ static struct bin_attribute bin_attr_rw_root_nvmem = {
>  	.write	= bin_attr_nvmem_write,
>  };
>  
> -static struct bin_attribute *nvmem_bin_rw_root_attributes[] = {
> -	&bin_attr_rw_root_nvmem,
> -	NULL,
> -};
> -
> -static const struct attribute_group nvmem_bin_rw_root_group = {
> -	.bin_attrs	= nvmem_bin_rw_root_attributes,
> -	.attrs		= nvmem_attrs,
> -};
> -
> -static const struct attribute_group *nvmem_rw_root_dev_groups[] = {
> -	&nvmem_bin_rw_root_group,
> -	NULL,
> -};
> -
>  /* read only permission, root only */
>  static struct bin_attribute bin_attr_ro_root_nvmem = {
>  	.attr	= {
> @@ -187,31 +180,11 @@ static struct bin_attribute bin_attr_ro_root_nvmem = {
>  	.read	= bin_attr_nvmem_read,
>  };
>  
> -static struct bin_attribute *nvmem_bin_ro_root_attributes[] = {
> -	&bin_attr_ro_root_nvmem,
> -	NULL,
> -};
> -
> -static const struct attribute_group nvmem_bin_ro_root_group = {
> -	.bin_attrs	= nvmem_bin_ro_root_attributes,
> -	.attrs		= nvmem_attrs,
> -};
> -
> -static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
> -	&nvmem_bin_ro_root_group,
> -	NULL,
> -};
> -
>  const struct attribute_group **nvmem_sysfs_get_groups(
>  					struct nvmem_device *nvmem,
>  					const struct nvmem_config *config)

You no longer need any parameters for this function, right?

Also, you really don't even need the function, just point to the
variable instead.

thanks,

greg k-h

Re: [PATCH v2 2/2] nvmem: core: use is_bin_visible for permissions

[Srinivas Kandagatla]

On 25/03/2020 10:21, Greg KH wrote:
>> -
>>   const struct attribute_group **nvmem_sysfs_get_groups(
>>   					struct nvmem_device *nvmem,
>>   					const struct nvmem_config *config)
> You no longer need any parameters for this function, right?
no we do not need that, I can update that in next version.

> 
> Also, you really don't even need the function, just point to the
> variable instead.
We have a use case where in the user can chose to not have sysfs entry, 
specially if we have hypervisor trapping access to some range of entries 
in nvmem. This is enforced using CONFIG_NVMEM_SYSFS option.

Currently in upstream we have a stub function when CONFIG_NVMEM_SYSFS is 
not selected returning NULL.

If we want to remove this function and subsequently the nvmem-sysfs.c 
file then we have to have #ifdef in the code which am okay to do if that 
is something that you are keen on.


thanks,
srini

> 
> thanks,

Re: [PATCH v2 2/2] nvmem: core: use is_bin_visible for permissions

[Greg KH]

On Wed, Mar 25, 2020 at 10:40:10AM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 25/03/2020 10:21, Greg KH wrote:
> > > -
> > >   const struct attribute_group **nvmem_sysfs_get_groups(
> > >   					struct nvmem_device *nvmem,
> > >   					const struct nvmem_config *config)
> > You no longer need any parameters for this function, right?
> no we do not need that, I can update that in next version.

Ok.

> > Also, you really don't even need the function, just point to the
> > variable instead.
> We have a use case where in the user can chose to not have sysfs entry,
> specially if we have hypervisor trapping access to some range of entries in
> nvmem. This is enforced using CONFIG_NVMEM_SYSFS option.
> 
> Currently in upstream we have a stub function when CONFIG_NVMEM_SYSFS is not
> selected returning NULL.
> 
> If we want to remove this function and subsequently the nvmem-sysfs.c file
> then we have to have #ifdef in the code which am okay to do if that is
> something that you are keen on.

You can still do it in the .h file, defining the variable as NULL if
that config option is not enabled.

thanks,

greg k-h