[PATCH 1/2] sysfs: add sysfs_add_bin_file_to_group()

[PATCH 1/2] sysfs: add sysfs_add_bin_file_to_group()

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

There already is sysfs_add_file_to_group() for adding "attribute" to a
group. This new function allows adding "bin_attribute" as well.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 fs/sysfs/file.c       | 31 +++++++++++++++++++++++++++----
 include/linux/sysfs.h |  3 +++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 42dcf96881b6..30c798c38d89 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -376,14 +376,19 @@ EXPORT_SYMBOL_GPL(sysfs_create_files);
  * @attr: attribute descriptor.
  * @group: group name.
  */
-int sysfs_add_file_to_group(struct kobject *kobj,
-		const struct attribute *attr, const char *group)
+int __sysfs_add_file_to_group(struct kobject *kobj,
+			      const struct attribute *attr,
+			      const struct bin_attribute *battr,
+			      const char *group)
 {
 	struct kernfs_node *parent;
 	kuid_t uid;
 	kgid_t gid;
 	int error;
 
+	if (WARN_ON((attr && battr) || (!attr && !battr)))
+		return -EINVAL;
+
 	if (group) {
 		parent = kernfs_find_and_get(kobj->sd, group);
 	} else {
@@ -395,14 +400,32 @@ int sysfs_add_file_to_group(struct kobject *kobj,
 		return -ENOENT;
 
 	kobject_get_ownership(kobj, &uid, &gid);
-	error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
-				       NULL);
+	if (attr)
+		error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid,
+					       gid, NULL);
+	else
+		error = sysfs_add_bin_file_mode_ns(parent, battr, battr->attr.mode,
+						   uid, gid, NULL);
 	kernfs_put(parent);
 
 	return error;
 }
+
+int sysfs_add_file_to_group(struct kobject *kobj, const struct attribute *attr,
+			    const char *group)
+{
+	return __sysfs_add_file_to_group(kobj, attr, NULL, group);
+}
 EXPORT_SYMBOL_GPL(sysfs_add_file_to_group);
 
+int sysfs_add_bin_file_to_group(struct kobject *kobj,
+				const struct bin_attribute *battr,
+				const char *group)
+{
+	return __sysfs_add_file_to_group(kobj, NULL, battr, group);
+}
+EXPORT_SYMBOL_GPL(sysfs_add_bin_file_to_group);
+
 /**
  * sysfs_chmod_file - update the modified mode value on an object attribute.
  * @kobj: object we're acting for.
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index e3f1e8ac1f85..9b4f9d405604 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -302,6 +302,9 @@ void sysfs_remove_groups(struct kobject *kobj,
 			 const struct attribute_group **groups);
 int sysfs_add_file_to_group(struct kobject *kobj,
 			const struct attribute *attr, const char *group);
+int sysfs_add_bin_file_to_group(struct kobject *kobj,
+				const struct bin_attribute *battr,
+				const char *group);
 void sysfs_remove_file_from_group(struct kobject *kobj,
 			const struct attribute *attr, const char *group);
 int sysfs_merge_group(struct kobject *kobj,
-- 
2.31.1

[PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This allows reading NVMEM cells using /sys/bus/nvmem/devices/*/cells/*
which may be helpful for userspace & debugging purposes.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 Documentation/driver-api/nvmem.rst | 11 ++++++
 drivers/nvmem/core.c               | 60 ++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/Documentation/driver-api/nvmem.rst b/Documentation/driver-api/nvmem.rst
index 287e86819640..20f7d68143be 100644
--- a/Documentation/driver-api/nvmem.rst
+++ b/Documentation/driver-api/nvmem.rst
@@ -185,6 +185,17 @@ ex::
   *
   0001000
 
+Single cells can be read using files located at::
+
+	/sys/bus/nvmem/devices/*/cells/*
+
+ex::
+
+  hexdump -C /sys/bus/nvmem/devices/mtd0/cells/mac
+
+  00000000  10 7b 44 c4 8a b0                                 |.{D...|
+  00000006
+
 7. DeviceTree Binding
 =====================
 
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 23a38dcf0fc4..785a56e33f69 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -55,6 +55,7 @@ struct nvmem_cell_entry {
 	struct device_node	*np;
 	struct nvmem_device	*nvmem;
 	struct list_head	node;
+	struct bin_attribute	battr;
 };
 
 struct nvmem_cell {
@@ -73,6 +74,10 @@ static LIST_HEAD(nvmem_lookup_list);
 
 static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
 
+static int __nvmem_cell_read(struct nvmem_device *nvmem,
+		      struct nvmem_cell_entry *cell,
+		      void *buf, size_t *len, const char *id);
+
 static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
 			    void *val, size_t bytes)
 {
@@ -338,8 +343,18 @@ static const struct attribute_group nvmem_bin_group = {
 	.is_bin_visible = nvmem_bin_attr_is_visible,
 };
 
+static struct bin_attribute *nvmem_cells_bin_attrs[] = {
+	NULL,
+};
+
+static const struct attribute_group nvmem_cells_group = {
+	.name		= "cells",
+	.bin_attrs	= nvmem_cells_bin_attrs,
+};
+
 static const struct attribute_group *nvmem_dev_groups[] = {
 	&nvmem_bin_group,
+	&nvmem_cells_group,
 	NULL,
 };
 
@@ -431,7 +446,13 @@ static struct bus_type nvmem_bus_type = {
 
 static void nvmem_cell_entry_drop(struct nvmem_cell_entry *cell)
 {
+	struct device *dev = &cell->nvmem->dev;
+
 	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_REMOVE, cell);
+
+	sysfs_remove_file_from_group(&dev->kobj, &cell->battr.attr,
+				     nvmem_cells_group.name);
+
 	mutex_lock(&nvmem_mutex);
 	list_del(&cell->node);
 	mutex_unlock(&nvmem_mutex);
@@ -448,11 +469,50 @@ static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem)
 		nvmem_cell_entry_drop(cell);
 }
 
+static ssize_t nvmem_cell_attr_read(struct file *filp, struct kobject *kobj,
+				    struct bin_attribute *battr, char *buf,
+				    loff_t pos, size_t count)
+{
+	struct nvmem_cell_entry *cell = container_of(battr, struct nvmem_cell_entry, battr);
+	struct nvmem_device *nvmem = cell->nvmem;
+	size_t bytes;
+	u8 *data;
+	int err;
+
+	if (pos >= cell->bytes)
+		return 0;
+
+	data = kzalloc(cell->bytes, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	err = __nvmem_cell_read(nvmem, cell, data, &bytes, NULL);
+	if (!err)
+		memcpy(buf, data + pos, count - pos);
+
+	kfree(data);
+
+	return err ? err : bytes;
+}
+
 static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
 {
+	struct device *dev = &cell->nvmem->dev;
+	int err;
+
 	mutex_lock(&nvmem_mutex);
 	list_add_tail(&cell->node, &cell->nvmem->cells);
 	mutex_unlock(&nvmem_mutex);
+
+	sysfs_attr_init(&cell->battr.attr);
+	cell->battr.attr.name = cell->name;
+	cell->battr.attr.mode = 0400;
+	cell->battr.read = nvmem_cell_attr_read;
+	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
+					  nvmem_cells_group.name);
+	if (err)
+		dev_warn(dev, "Failed to add %s cell: %d\n", cell->name, err);
+
 	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_ADD, cell);
 }
 
-- 
2.31.1

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Greg Kroah-Hartman]

On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This allows reading NVMEM cells using /sys/bus/nvmem/devices/*/cells/*
> which may be helpful for userspace & debugging purposes.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  Documentation/driver-api/nvmem.rst | 11 ++++++
>  drivers/nvmem/core.c               | 60 ++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/Documentation/driver-api/nvmem.rst b/Documentation/driver-api/nvmem.rst
> index 287e86819640..20f7d68143be 100644
> --- a/Documentation/driver-api/nvmem.rst
> +++ b/Documentation/driver-api/nvmem.rst
> @@ -185,6 +185,17 @@ ex::
>    *
>    0001000
>  
> +Single cells can be read using files located at::
> +
> +	/sys/bus/nvmem/devices/*/cells/*
> +
> +ex::
> +
> +  hexdump -C /sys/bus/nvmem/devices/mtd0/cells/mac
> +
> +  00000000  10 7b 44 c4 8a b0                                 |.{D...|
> +  00000006
> +
>  7. DeviceTree Binding
>  =====================
>  

sysfs apis are documented in Documenatation/ABI/ not in other random
files.  Please fix this.

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 23a38dcf0fc4..785a56e33f69 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -55,6 +55,7 @@ struct nvmem_cell_entry {
>  	struct device_node	*np;
>  	struct nvmem_device	*nvmem;
>  	struct list_head	node;
> +	struct bin_attribute	battr;
>  };
>  
>  struct nvmem_cell {
> @@ -73,6 +74,10 @@ static LIST_HEAD(nvmem_lookup_list);
>  
>  static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
>  
> +static int __nvmem_cell_read(struct nvmem_device *nvmem,
> +		      struct nvmem_cell_entry *cell,
> +		      void *buf, size_t *len, const char *id);
> +
>  static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
>  			    void *val, size_t bytes)
>  {
> @@ -338,8 +343,18 @@ static const struct attribute_group nvmem_bin_group = {
>  	.is_bin_visible = nvmem_bin_attr_is_visible,
>  };
>  
> +static struct bin_attribute *nvmem_cells_bin_attrs[] = {
> +	NULL,
> +};
> +
> +static const struct attribute_group nvmem_cells_group = {
> +	.name		= "cells",
> +	.bin_attrs	= nvmem_cells_bin_attrs,
> +};
> +
>  static const struct attribute_group *nvmem_dev_groups[] = {
>  	&nvmem_bin_group,
> +	&nvmem_cells_group,
>  	NULL,
>  };
>  
> @@ -431,7 +446,13 @@ static struct bus_type nvmem_bus_type = {
>  
>  static void nvmem_cell_entry_drop(struct nvmem_cell_entry *cell)
>  {
> +	struct device *dev = &cell->nvmem->dev;
> +
>  	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_REMOVE, cell);
> +
> +	sysfs_remove_file_from_group(&dev->kobj, &cell->battr.attr,
> +				     nvmem_cells_group.name);
> +
>  	mutex_lock(&nvmem_mutex);
>  	list_del(&cell->node);
>  	mutex_unlock(&nvmem_mutex);
> @@ -448,11 +469,50 @@ static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem)
>  		nvmem_cell_entry_drop(cell);
>  }
>  
> +static ssize_t nvmem_cell_attr_read(struct file *filp, struct kobject *kobj,
> +				    struct bin_attribute *battr, char *buf,
> +				    loff_t pos, size_t count)
> +{
> +	struct nvmem_cell_entry *cell = container_of(battr, struct nvmem_cell_entry, battr);
> +	struct nvmem_device *nvmem = cell->nvmem;
> +	size_t bytes;
> +	u8 *data;
> +	int err;
> +
> +	if (pos >= cell->bytes)
> +		return 0;
> +
> +	data = kzalloc(cell->bytes, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	err = __nvmem_cell_read(nvmem, cell, data, &bytes, NULL);
> +	if (!err)
> +		memcpy(buf, data + pos, count - pos);
> +
> +	kfree(data);
> +
> +	return err ? err : bytes;
> +}
> +
>  static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
>  {
> +	struct device *dev = &cell->nvmem->dev;
> +	int err;
> +
>  	mutex_lock(&nvmem_mutex);
>  	list_add_tail(&cell->node, &cell->nvmem->cells);
>  	mutex_unlock(&nvmem_mutex);
> +
> +	sysfs_attr_init(&cell->battr.attr);
> +	cell->battr.attr.name = cell->name;
> +	cell->battr.attr.mode = 0400;
> +	cell->battr.read = nvmem_cell_attr_read;
> +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
> +					  nvmem_cells_group.name);

Why not just use the is_bin_visible attribute instead to determine if
the attribute should be shown or not instead of having to add it
after-the-fact which will race with userspace and loose?

thanks,

greg k-h

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Rafał Miłecki]

Hi Greg,

On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
> On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
>>   static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
>>   {
>> +	struct device *dev = &cell->nvmem->dev;
>> +	int err;
>> +
>>   	mutex_lock(&nvmem_mutex);
>>   	list_add_tail(&cell->node, &cell->nvmem->cells);
>>   	mutex_unlock(&nvmem_mutex);
>> +
>> +	sysfs_attr_init(&cell->battr.attr);
>> +	cell->battr.attr.name = cell->name;
>> +	cell->battr.attr.mode = 0400;
>> +	cell->battr.read = nvmem_cell_attr_read;
>> +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
>> +					  nvmem_cells_group.name);
> 
> Why not just use the is_bin_visible attribute instead to determine if
> the attribute should be shown or not instead of having to add it
> after-the-fact which will race with userspace and loose?

I'm sorry I really don't see how you suggest to get it done.

I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
I don't understand addig-after-the-fact part. How is .is_bin_visible()
related to adding attributes for newly created cells? Do you mean I can
avoid calling sysfs_add_bin_file_to_group()? Do you recall any existing
example of such solution?

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Greg Kroah-Hartman]

On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
> Hi Greg,
> 
> On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
> > On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
> > >   static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
> > >   {
> > > +	struct device *dev = &cell->nvmem->dev;
> > > +	int err;
> > > +
> > >   	mutex_lock(&nvmem_mutex);
> > >   	list_add_tail(&cell->node, &cell->nvmem->cells);
> > >   	mutex_unlock(&nvmem_mutex);
> > > +
> > > +	sysfs_attr_init(&cell->battr.attr);
> > > +	cell->battr.attr.name = cell->name;
> > > +	cell->battr.attr.mode = 0400;
> > > +	cell->battr.read = nvmem_cell_attr_read;
> > > +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
> > > +					  nvmem_cells_group.name);
> > 
> > Why not just use the is_bin_visible attribute instead to determine if
> > the attribute should be shown or not instead of having to add it
> > after-the-fact which will race with userspace and loose?
> 
> I'm sorry I really don't see how you suggest to get it done.
> 
> I can use .is_bin_visible() callback indeed to respect nvmem->root_only.

Great.

> I don't understand addig-after-the-fact part. How is .is_bin_visible()
> related to adding attributes for newly created cells?

You are adding a sysfs attribute to a device that is already registered
in the driver core, and so the creation of that attribute is never seen
by userspace.  The attribute needs to be attached to the device _BEFORE_
it is registered.

Also, huge hint, if a driver has to call as sysfs_*() call, something is
wrong.

> Do you mean I can
> avoid calling sysfs_add_bin_file_to_group()?

Yes.

> Do you recall any existing example of such solution?

Loads.

Just add this attribute group to your driver as a default attribute
group and the driver core will create it for you if needed.

Or if you always need it, no need to mess sith is_bin_visible() at all,
I can't really understand what you are trying to do here at all.

thanks,

greg k-h

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Rafał Miłecki]

On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
> On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
>> Hi Greg,
>>
>> On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
>>> On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
>>>>    static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
>>>>    {
>>>> +	struct device *dev = &cell->nvmem->dev;
>>>> +	int err;
>>>> +
>>>>    	mutex_lock(&nvmem_mutex);
>>>>    	list_add_tail(&cell->node, &cell->nvmem->cells);
>>>>    	mutex_unlock(&nvmem_mutex);
>>>> +
>>>> +	sysfs_attr_init(&cell->battr.attr);
>>>> +	cell->battr.attr.name = cell->name;
>>>> +	cell->battr.attr.mode = 0400;
>>>> +	cell->battr.read = nvmem_cell_attr_read;
>>>> +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
>>>> +					  nvmem_cells_group.name);
>>>
>>> Why not just use the is_bin_visible attribute instead to determine if
>>> the attribute should be shown or not instead of having to add it
>>> after-the-fact which will race with userspace and loose?
>>
>> I'm sorry I really don't see how you suggest to get it done.
>>
>> I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
> 
> Great.
> 
>> I don't understand addig-after-the-fact part. How is .is_bin_visible()
>> related to adding attributes for newly created cells?
> 
> You are adding a sysfs attribute to a device that is already registered
> in the driver core, and so the creation of that attribute is never seen
> by userspace.  The attribute needs to be attached to the device _BEFORE_
> it is registered.
> 
> Also, huge hint, if a driver has to call as sysfs_*() call, something is
> wrong.
> 
>> Do you mean I can
>> avoid calling sysfs_add_bin_file_to_group()?
> 
> Yes.
> 
>> Do you recall any existing example of such solution?
> 
> Loads.
> 
> Just add this attribute group to your driver as a default attribute
> group and the driver core will create it for you if needed.
> 
> Or if you always need it, no need to mess sith is_bin_visible() at all,
> I can't really understand what you are trying to do here at all.

Thanks a lot! In nvmem_register() first there is a call to the
device_register() and only later cells get added. I suppose I just have
to rework nvmem_register() order so that:
1. Cells are collected earlier. For each cell I allocate group attribute
2. device_register() gets called

Thank you for explaining that with patience.

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 07:39:24AM +0100, Rafał Miłecki wrote:
> On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
> > On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
> > > Hi Greg,
> > > 
> > > On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
> > > > On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
> > > > >    static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
> > > > >    {
> > > > > +	struct device *dev = &cell->nvmem->dev;
> > > > > +	int err;
> > > > > +
> > > > >    	mutex_lock(&nvmem_mutex);
> > > > >    	list_add_tail(&cell->node, &cell->nvmem->cells);
> > > > >    	mutex_unlock(&nvmem_mutex);
> > > > > +
> > > > > +	sysfs_attr_init(&cell->battr.attr);
> > > > > +	cell->battr.attr.name = cell->name;
> > > > > +	cell->battr.attr.mode = 0400;
> > > > > +	cell->battr.read = nvmem_cell_attr_read;
> > > > > +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
> > > > > +					  nvmem_cells_group.name);
> > > > 
> > > > Why not just use the is_bin_visible attribute instead to determine if
> > > > the attribute should be shown or not instead of having to add it
> > > > after-the-fact which will race with userspace and loose?
> > > 
> > > I'm sorry I really don't see how you suggest to get it done.
> > > 
> > > I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
> > 
> > Great.
> > 
> > > I don't understand addig-after-the-fact part. How is .is_bin_visible()
> > > related to adding attributes for newly created cells?
> > 
> > You are adding a sysfs attribute to a device that is already registered
> > in the driver core, and so the creation of that attribute is never seen
> > by userspace.  The attribute needs to be attached to the device _BEFORE_
> > it is registered.
> > 
> > Also, huge hint, if a driver has to call as sysfs_*() call, something is
> > wrong.
> > 
> > > Do you mean I can
> > > avoid calling sysfs_add_bin_file_to_group()?
> > 
> > Yes.
> > 
> > > Do you recall any existing example of such solution?
> > 
> > Loads.
> > 
> > Just add this attribute group to your driver as a default attribute
> > group and the driver core will create it for you if needed.
> > 
> > Or if you always need it, no need to mess sith is_bin_visible() at all,
> > I can't really understand what you are trying to do here at all.
> 
> Thanks a lot! In nvmem_register() first there is a call to the
> device_register() and only later cells get added. I suppose I just have
> to rework nvmem_register() order so that:
> 1. Cells are collected earlier. For each cell I allocate group attribute

No, add all of the attributes to the device at the beginning before you
register it, there's no need to allocate anything.

> 2. device_register() gets called

Yes.

thanks,

greg k-h

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Rafał Miłecki]

On 21.12.2021 07:45, Greg Kroah-Hartman wrote:
> On Tue, Dec 21, 2021 at 07:39:24AM +0100, Rafał Miłecki wrote:
>> On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
>>> On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
>>>> Hi Greg,
>>>>
>>>> On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
>>>>> On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
>>>>>>     static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
>>>>>>     {
>>>>>> +	struct device *dev = &cell->nvmem->dev;
>>>>>> +	int err;
>>>>>> +
>>>>>>     	mutex_lock(&nvmem_mutex);
>>>>>>     	list_add_tail(&cell->node, &cell->nvmem->cells);
>>>>>>     	mutex_unlock(&nvmem_mutex);
>>>>>> +
>>>>>> +	sysfs_attr_init(&cell->battr.attr);
>>>>>> +	cell->battr.attr.name = cell->name;
>>>>>> +	cell->battr.attr.mode = 0400;
>>>>>> +	cell->battr.read = nvmem_cell_attr_read;
>>>>>> +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
>>>>>> +					  nvmem_cells_group.name);
>>>>>
>>>>> Why not just use the is_bin_visible attribute instead to determine if
>>>>> the attribute should be shown or not instead of having to add it
>>>>> after-the-fact which will race with userspace and loose?
>>>>
>>>> I'm sorry I really don't see how you suggest to get it done.
>>>>
>>>> I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
>>>
>>> Great.
>>>
>>>> I don't understand addig-after-the-fact part. How is .is_bin_visible()
>>>> related to adding attributes for newly created cells?
>>>
>>> You are adding a sysfs attribute to a device that is already registered
>>> in the driver core, and so the creation of that attribute is never seen
>>> by userspace.  The attribute needs to be attached to the device _BEFORE_
>>> it is registered.
>>>
>>> Also, huge hint, if a driver has to call as sysfs_*() call, something is
>>> wrong.
>>>
>>>> Do you mean I can
>>>> avoid calling sysfs_add_bin_file_to_group()?
>>>
>>> Yes.
>>>
>>>> Do you recall any existing example of such solution?
>>>
>>> Loads.
>>>
>>> Just add this attribute group to your driver as a default attribute
>>> group and the driver core will create it for you if needed.
>>>
>>> Or if you always need it, no need to mess sith is_bin_visible() at all,
>>> I can't really understand what you are trying to do here at all.
>>
>> Thanks a lot! In nvmem_register() first there is a call to the
>> device_register() and only later cells get added. I suppose I just have
>> to rework nvmem_register() order so that:
>> 1. Cells are collected earlier. For each cell I allocate group attribute
> 
> No, add all of the attributes to the device at the beginning before you
> register it, there's no need to allocate anything.

If you mean static structures I can't do that, because cells almost
never are static. They are not known in advance. nvmem allows cells to
be:
1. Specified in OF
2. Submitted as list while registering a NVMEM device

So every cells gets its own structure allocated dynamically. My plan is
to put bin_attribute in that struct and then create a group collecting
all those cells.


>> 2. device_register() gets called
> 
> Yes.

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 07:53:32AM +0100, Rafał Miłecki wrote:
> On 21.12.2021 07:45, Greg Kroah-Hartman wrote:
> > On Tue, Dec 21, 2021 at 07:39:24AM +0100, Rafał Miłecki wrote:
> > > On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
> > > > On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
> > > > > Hi Greg,
> > > > > 
> > > > > On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
> > > > > > On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
> > > > > > >     static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
> > > > > > >     {
> > > > > > > +	struct device *dev = &cell->nvmem->dev;
> > > > > > > +	int err;
> > > > > > > +
> > > > > > >     	mutex_lock(&nvmem_mutex);
> > > > > > >     	list_add_tail(&cell->node, &cell->nvmem->cells);
> > > > > > >     	mutex_unlock(&nvmem_mutex);
> > > > > > > +
> > > > > > > +	sysfs_attr_init(&cell->battr.attr);
> > > > > > > +	cell->battr.attr.name = cell->name;
> > > > > > > +	cell->battr.attr.mode = 0400;
> > > > > > > +	cell->battr.read = nvmem_cell_attr_read;
> > > > > > > +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
> > > > > > > +					  nvmem_cells_group.name);
> > > > > > 
> > > > > > Why not just use the is_bin_visible attribute instead to determine if
> > > > > > the attribute should be shown or not instead of having to add it
> > > > > > after-the-fact which will race with userspace and loose?
> > > > > 
> > > > > I'm sorry I really don't see how you suggest to get it done.
> > > > > 
> > > > > I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
> > > > 
> > > > Great.
> > > > 
> > > > > I don't understand addig-after-the-fact part. How is .is_bin_visible()
> > > > > related to adding attributes for newly created cells?
> > > > 
> > > > You are adding a sysfs attribute to a device that is already registered
> > > > in the driver core, and so the creation of that attribute is never seen
> > > > by userspace.  The attribute needs to be attached to the device _BEFORE_
> > > > it is registered.
> > > > 
> > > > Also, huge hint, if a driver has to call as sysfs_*() call, something is
> > > > wrong.
> > > > 
> > > > > Do you mean I can
> > > > > avoid calling sysfs_add_bin_file_to_group()?
> > > > 
> > > > Yes.
> > > > 
> > > > > Do you recall any existing example of such solution?
> > > > 
> > > > Loads.
> > > > 
> > > > Just add this attribute group to your driver as a default attribute
> > > > group and the driver core will create it for you if needed.
> > > > 
> > > > Or if you always need it, no need to mess sith is_bin_visible() at all,
> > > > I can't really understand what you are trying to do here at all.
> > > 
> > > Thanks a lot! In nvmem_register() first there is a call to the
> > > device_register() and only later cells get added. I suppose I just have
> > > to rework nvmem_register() order so that:
> > > 1. Cells are collected earlier. For each cell I allocate group attribute
> > 
> > No, add all of the attributes to the device at the beginning before you
> > register it, there's no need to allocate anything.
> 
> If you mean static structures I can't do that, because cells almost
> never are static. They are not known in advance. nvmem allows cells to
> be:
> 1. Specified in OF
> 2. Submitted as list while registering a NVMEM device
> 
> So every cells gets its own structure allocated dynamically. My plan is
> to put bin_attribute in that struct and then create a group collecting
> all those cells.

A device has a driver associated with it, and that driver has default
groups associated with it.  Use that, I am not saying to use static
structures, that is not how the driver model works at all.

thanks,

greg k-h

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Rafał Miłecki]

On 21.12.2021 08:13, Greg Kroah-Hartman wrote:
> On Tue, Dec 21, 2021 at 07:53:32AM +0100, Rafał Miłecki wrote:
>> On 21.12.2021 07:45, Greg Kroah-Hartman wrote:
>>> On Tue, Dec 21, 2021 at 07:39:24AM +0100, Rafał Miłecki wrote:
>>>> On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
>>>>> On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
>>>>>>> On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
>>>>>>>>      static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
>>>>>>>>      {
>>>>>>>> +	struct device *dev = &cell->nvmem->dev;
>>>>>>>> +	int err;
>>>>>>>> +
>>>>>>>>      	mutex_lock(&nvmem_mutex);
>>>>>>>>      	list_add_tail(&cell->node, &cell->nvmem->cells);
>>>>>>>>      	mutex_unlock(&nvmem_mutex);
>>>>>>>> +
>>>>>>>> +	sysfs_attr_init(&cell->battr.attr);
>>>>>>>> +	cell->battr.attr.name = cell->name;
>>>>>>>> +	cell->battr.attr.mode = 0400;
>>>>>>>> +	cell->battr.read = nvmem_cell_attr_read;
>>>>>>>> +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
>>>>>>>> +					  nvmem_cells_group.name);
>>>>>>>
>>>>>>> Why not just use the is_bin_visible attribute instead to determine if
>>>>>>> the attribute should be shown or not instead of having to add it
>>>>>>> after-the-fact which will race with userspace and loose?
>>>>>>
>>>>>> I'm sorry I really don't see how you suggest to get it done.
>>>>>>
>>>>>> I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
>>>>>
>>>>> Great.
>>>>>
>>>>>> I don't understand addig-after-the-fact part. How is .is_bin_visible()
>>>>>> related to adding attributes for newly created cells?
>>>>>
>>>>> You are adding a sysfs attribute to a device that is already registered
>>>>> in the driver core, and so the creation of that attribute is never seen
>>>>> by userspace.  The attribute needs to be attached to the device _BEFORE_
>>>>> it is registered.
>>>>>
>>>>> Also, huge hint, if a driver has to call as sysfs_*() call, something is
>>>>> wrong.
>>>>>
>>>>>> Do you mean I can
>>>>>> avoid calling sysfs_add_bin_file_to_group()?
>>>>>
>>>>> Yes.
>>>>>
>>>>>> Do you recall any existing example of such solution?
>>>>>
>>>>> Loads.
>>>>>
>>>>> Just add this attribute group to your driver as a default attribute
>>>>> group and the driver core will create it for you if needed.
>>>>>
>>>>> Or if you always need it, no need to mess sith is_bin_visible() at all,
>>>>> I can't really understand what you are trying to do here at all.
>>>>
>>>> Thanks a lot! In nvmem_register() first there is a call to the
>>>> device_register() and only later cells get added. I suppose I just have
>>>> to rework nvmem_register() order so that:
>>>> 1. Cells are collected earlier. For each cell I allocate group attribute
>>>
>>> No, add all of the attributes to the device at the beginning before you
>>> register it, there's no need to allocate anything.
>>
>> If you mean static structures I can't do that, because cells almost
>> never are static. They are not known in advance. nvmem allows cells to
>> be:
>> 1. Specified in OF
>> 2. Submitted as list while registering a NVMEM device
>>
>> So every cells gets its own structure allocated dynamically. My plan is
>> to put bin_attribute in that struct and then create a group collecting
>> all those cells.
> 
> A device has a driver associated with it, and that driver has default
> groups associated with it.  Use that, I am not saying to use static
> structures, that is not how the driver model works at all.

I'm helpless on dealing with attributes.

I tried building a list of attributes dynamically but that of course
fails:

drivers/nvmem/core.c: In function ‘nvmem_register’:
drivers/nvmem/core.c:930:31: error: assignment of member ‘bin_attrs’ in read-only object
   930 |   nvmem_cells_group.bin_attrs = nvmem->cells_bin_attrs;
       |                               ^


What I'm trying to achieve is having
/sys/bus/nvmem/devices/*/cells/*
with each file being an attribute.


Please kindly point me to a single example of "struct attribute_group"
that has a variable list of attributes with each attribute having
runtime set name.

Almost all cases in kernel look like:
static const struct attribute_group foo_attr_group = {
	.attrs = foo_attrs,
};
with "foo_attrs" being a list of attributes with *predefined* names.

Every example of dynamic attributes (runtime created) I see in a kernel
(e.g. drivers/base/node.c) uses sysfs_*().

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 01:24:00PM +0100, Rafał Miłecki wrote:
> On 21.12.2021 08:13, Greg Kroah-Hartman wrote:
> > On Tue, Dec 21, 2021 at 07:53:32AM +0100, Rafał Miłecki wrote:
> > > On 21.12.2021 07:45, Greg Kroah-Hartman wrote:
> > > > On Tue, Dec 21, 2021 at 07:39:24AM +0100, Rafał Miłecki wrote:
> > > > > On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
> > > > > > On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
> > > > > > > Hi Greg,
> > > > > > > 
> > > > > > > On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
> > > > > > > > On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
> > > > > > > > >      static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
> > > > > > > > >      {
> > > > > > > > > +	struct device *dev = &cell->nvmem->dev;
> > > > > > > > > +	int err;
> > > > > > > > > +
> > > > > > > > >      	mutex_lock(&nvmem_mutex);
> > > > > > > > >      	list_add_tail(&cell->node, &cell->nvmem->cells);
> > > > > > > > >      	mutex_unlock(&nvmem_mutex);
> > > > > > > > > +
> > > > > > > > > +	sysfs_attr_init(&cell->battr.attr);
> > > > > > > > > +	cell->battr.attr.name = cell->name;
> > > > > > > > > +	cell->battr.attr.mode = 0400;
> > > > > > > > > +	cell->battr.read = nvmem_cell_attr_read;
> > > > > > > > > +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
> > > > > > > > > +					  nvmem_cells_group.name);
> > > > > > > > 
> > > > > > > > Why not just use the is_bin_visible attribute instead to determine if
> > > > > > > > the attribute should be shown or not instead of having to add it
> > > > > > > > after-the-fact which will race with userspace and loose?
> > > > > > > 
> > > > > > > I'm sorry I really don't see how you suggest to get it done.
> > > > > > > 
> > > > > > > I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
> > > > > > 
> > > > > > Great.
> > > > > > 
> > > > > > > I don't understand addig-after-the-fact part. How is .is_bin_visible()
> > > > > > > related to adding attributes for newly created cells?
> > > > > > 
> > > > > > You are adding a sysfs attribute to a device that is already registered
> > > > > > in the driver core, and so the creation of that attribute is never seen
> > > > > > by userspace.  The attribute needs to be attached to the device _BEFORE_
> > > > > > it is registered.
> > > > > > 
> > > > > > Also, huge hint, if a driver has to call as sysfs_*() call, something is
> > > > > > wrong.
> > > > > > 
> > > > > > > Do you mean I can
> > > > > > > avoid calling sysfs_add_bin_file_to_group()?
> > > > > > 
> > > > > > Yes.
> > > > > > 
> > > > > > > Do you recall any existing example of such solution?
> > > > > > 
> > > > > > Loads.
> > > > > > 
> > > > > > Just add this attribute group to your driver as a default attribute
> > > > > > group and the driver core will create it for you if needed.
> > > > > > 
> > > > > > Or if you always need it, no need to mess sith is_bin_visible() at all,
> > > > > > I can't really understand what you are trying to do here at all.
> > > > > 
> > > > > Thanks a lot! In nvmem_register() first there is a call to the
> > > > > device_register() and only later cells get added. I suppose I just have
> > > > > to rework nvmem_register() order so that:
> > > > > 1. Cells are collected earlier. For each cell I allocate group attribute
> > > > 
> > > > No, add all of the attributes to the device at the beginning before you
> > > > register it, there's no need to allocate anything.
> > > 
> > > If you mean static structures I can't do that, because cells almost
> > > never are static. They are not known in advance. nvmem allows cells to
> > > be:
> > > 1. Specified in OF
> > > 2. Submitted as list while registering a NVMEM device
> > > 
> > > So every cells gets its own structure allocated dynamically. My plan is
> > > to put bin_attribute in that struct and then create a group collecting
> > > all those cells.
> > 
> > A device has a driver associated with it, and that driver has default
> > groups associated with it.  Use that, I am not saying to use static
> > structures, that is not how the driver model works at all.
> 
> I'm helpless on dealing with attributes.
> 
> I tried building a list of attributes dynamically but that of course
> fails:
> 
> drivers/nvmem/core.c: In function ‘nvmem_register’:
> drivers/nvmem/core.c:930:31: error: assignment of member ‘bin_attrs’ in read-only object
>   930 |   nvmem_cells_group.bin_attrs = nvmem->cells_bin_attrs;
>       |                               ^
> 
> 
> What I'm trying to achieve is having
> /sys/bus/nvmem/devices/*/cells/*
> with each file being an attribute.

What is the full path here that you are looking to add these attributes
to?  Where is the struct device in play?  What .c file should I look at?

> Please kindly point me to a single example of "struct attribute_group"
> that has a variable list of attributes with each attribute having
> runtime set name.

Why would you ever want each attribute have a runtime-set name?  That's
not what attributes are for.  Think of them as "key/value" pairs.  The
"key" part is the name (i.e. filename), that is well known to everyone,
unique to that struct device type, and documented in Documentation/ABI/.
The "value" part is the value you read from the file (or write to it.)

That's it, it's not all that complex.

> Almost all cases in kernel look like:
> static const struct attribute_group foo_attr_group = {
> 	.attrs = foo_attrs,
> };
> with "foo_attrs" being a list of attributes with *predefined* names.

Yes, because that is what you really want.

Why do you feel this device is somehow unique to deserve attributes that
are not predefined?  And if they are not predefined, how are you going
to define them when you create them in the code and document them?   :)

> Every example of dynamic attributes (runtime created) I see in a kernel
> (e.g. drivers/base/node.c) uses sysfs_*().

drivers/base/* is not the best place to look at for how to implement
bus/driver logic, look at existing busses and drivers instead please.
We have a few hundred to choose from :)

So, let's break it down, what exactly are you wanting your device on
your bus to look like?  What will be the attributes you want to expose,
and what are the values of those attributes?  You have to start with
that, as Documentation/ABI/ is going to require you to write them down.

thanks,

greg k-h

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Rafał Miłecki]

On 21.12.2021 13:56, Greg Kroah-Hartman wrote:
> On Tue, Dec 21, 2021 at 01:24:00PM +0100, Rafał Miłecki wrote:
>> On 21.12.2021 08:13, Greg Kroah-Hartman wrote:
>>> On Tue, Dec 21, 2021 at 07:53:32AM +0100, Rafał Miłecki wrote:
>>>> On 21.12.2021 07:45, Greg Kroah-Hartman wrote:
>>>>> On Tue, Dec 21, 2021 at 07:39:24AM +0100, Rafał Miłecki wrote:
>>>>>> On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
>>>>>>> On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
>>>>>>>> Hi Greg,
>>>>>>>>
>>>>>>>> On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
>>>>>>>>> On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
>>>>>>>>>>       static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
>>>>>>>>>>       {
>>>>>>>>>> +	struct device *dev = &cell->nvmem->dev;
>>>>>>>>>> +	int err;
>>>>>>>>>> +
>>>>>>>>>>       	mutex_lock(&nvmem_mutex);
>>>>>>>>>>       	list_add_tail(&cell->node, &cell->nvmem->cells);
>>>>>>>>>>       	mutex_unlock(&nvmem_mutex);
>>>>>>>>>> +
>>>>>>>>>> +	sysfs_attr_init(&cell->battr.attr);
>>>>>>>>>> +	cell->battr.attr.name = cell->name;
>>>>>>>>>> +	cell->battr.attr.mode = 0400;
>>>>>>>>>> +	cell->battr.read = nvmem_cell_attr_read;
>>>>>>>>>> +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
>>>>>>>>>> +					  nvmem_cells_group.name);
>>>>>>>>>
>>>>>>>>> Why not just use the is_bin_visible attribute instead to determine if
>>>>>>>>> the attribute should be shown or not instead of having to add it
>>>>>>>>> after-the-fact which will race with userspace and loose?
>>>>>>>>
>>>>>>>> I'm sorry I really don't see how you suggest to get it done.
>>>>>>>>
>>>>>>>> I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
>>>>>>>
>>>>>>> Great.
>>>>>>>
>>>>>>>> I don't understand addig-after-the-fact part. How is .is_bin_visible()
>>>>>>>> related to adding attributes for newly created cells?
>>>>>>>
>>>>>>> You are adding a sysfs attribute to a device that is already registered
>>>>>>> in the driver core, and so the creation of that attribute is never seen
>>>>>>> by userspace.  The attribute needs to be attached to the device _BEFORE_
>>>>>>> it is registered.
>>>>>>>
>>>>>>> Also, huge hint, if a driver has to call as sysfs_*() call, something is
>>>>>>> wrong.
>>>>>>>
>>>>>>>> Do you mean I can
>>>>>>>> avoid calling sysfs_add_bin_file_to_group()?
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>> Do you recall any existing example of such solution?
>>>>>>>
>>>>>>> Loads.
>>>>>>>
>>>>>>> Just add this attribute group to your driver as a default attribute
>>>>>>> group and the driver core will create it for you if needed.
>>>>>>>
>>>>>>> Or if you always need it, no need to mess sith is_bin_visible() at all,
>>>>>>> I can't really understand what you are trying to do here at all.
>>>>>>
>>>>>> Thanks a lot! In nvmem_register() first there is a call to the
>>>>>> device_register() and only later cells get added. I suppose I just have
>>>>>> to rework nvmem_register() order so that:
>>>>>> 1. Cells are collected earlier. For each cell I allocate group attribute
>>>>>
>>>>> No, add all of the attributes to the device at the beginning before you
>>>>> register it, there's no need to allocate anything.
>>>>
>>>> If you mean static structures I can't do that, because cells almost
>>>> never are static. They are not known in advance. nvmem allows cells to
>>>> be:
>>>> 1. Specified in OF
>>>> 2. Submitted as list while registering a NVMEM device
>>>>
>>>> So every cells gets its own structure allocated dynamically. My plan is
>>>> to put bin_attribute in that struct and then create a group collecting
>>>> all those cells.
>>>
>>> A device has a driver associated with it, and that driver has default
>>> groups associated with it.  Use that, I am not saying to use static
>>> structures, that is not how the driver model works at all.
>>
>> I'm helpless on dealing with attributes.
>>
>> I tried building a list of attributes dynamically but that of course
>> fails:
>>
>> drivers/nvmem/core.c: In function ‘nvmem_register’:
>> drivers/nvmem/core.c:930:31: error: assignment of member ‘bin_attrs’ in read-only object
>>    930 |   nvmem_cells_group.bin_attrs = nvmem->cells_bin_attrs;
>>        |                               ^
>>
>>
>> What I'm trying to achieve is having
>> /sys/bus/nvmem/devices/*/cells/*
>> with each file being an attribute.
> 
> What is the full path here that you are looking to add these attributes
> to?  Where is the struct device in play?  What .c file should I look at?
> 
>> Please kindly point me to a single example of "struct attribute_group"
>> that has a variable list of attributes with each attribute having
>> runtime set name.
> 
> Why would you ever want each attribute have a runtime-set name?  That's
> not what attributes are for.  Think of them as "key/value" pairs.  The
> "key" part is the name (i.e. filename), that is well known to everyone,
> unique to that struct device type, and documented in Documentation/ABI/.
> The "value" part is the value you read from the file (or write to it.)
> 
> That's it, it's not all that complex.
> 
>> Almost all cases in kernel look like:
>> static const struct attribute_group foo_attr_group = {
>> 	.attrs = foo_attrs,
>> };
>> with "foo_attrs" being a list of attributes with *predefined* names.
> 
> Yes, because that is what you really want.
> 
> Why do you feel this device is somehow unique to deserve attributes that
> are not predefined?  And if they are not predefined, how are you going
> to define them when you create them in the code and document them?   :)
> 
>> Every example of dynamic attributes (runtime created) I see in a kernel
>> (e.g. drivers/base/node.c) uses sysfs_*().
> 
> drivers/base/* is not the best place to look at for how to implement
> bus/driver logic, look at existing busses and drivers instead please.
> We have a few hundred to choose from :)
> 
> So, let's break it down, what exactly are you wanting your device on
> your bus to look like?  What will be the attributes you want to expose,
> and what are the values of those attributes?  You have to start with
> that, as Documentation/ABI/ is going to require you to write them down.

This patch subject / body is a basic summary. It's about
drivers/nvmem/core.c .


Let me explain it with more details:

NVMEM is a data blob.
NVMEM consists of entries called cells.


Example:

U-Boot environment variables is NVMEM. Example:
00000000  c4 09 30 54 62 6f 6f 74  63 6d 64 3d 74 66 74 70  |..0Tbootcmd=tftp|
00000010  00 62 6f 6f 74 64 65 6c  61 79 3d 35 00 65 74 68  |.bootdelay=5.eth|

A single environment variable is NVMEM cell. Example:
bootcmd=tftp
bootdelay=5


Every NVMEM device is exposed in sysfs as:
/sys/bus/nvmem/devices/*/

You can read NVMEM blob doing
cat /sys/bus/nvmem/devices/*/nvmem | hexdump -C


What I'm trying to do is to expose NVMEM cells in sysfs as:
/sys/bus/nvmem/devices/cells/*

Example:
$ cat /sys/bus/nvmem/devices/foo/cells/bootcmd
tftp
$ cat /sys/bus/nvmem/devices/foo/cells/bootdelay
5

As you can see above NVMEM cells are not known at compilation time.


So I believe the question is: how can I expose cells in sysfs?

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 02:05:34PM +0100, Rafał Miłecki wrote:
> On 21.12.2021 13:56, Greg Kroah-Hartman wrote:
> > On Tue, Dec 21, 2021 at 01:24:00PM +0100, Rafał Miłecki wrote:
> > > On 21.12.2021 08:13, Greg Kroah-Hartman wrote:
> > > > On Tue, Dec 21, 2021 at 07:53:32AM +0100, Rafał Miłecki wrote:
> > > > > On 21.12.2021 07:45, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Dec 21, 2021 at 07:39:24AM +0100, Rafał Miłecki wrote:
> > > > > > > On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
> > > > > > > > On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
> > > > > > > > > Hi Greg,
> > > > > > > > > 
> > > > > > > > > On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
> > > > > > > > > > On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
> > > > > > > > > > >       static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
> > > > > > > > > > >       {
> > > > > > > > > > > +	struct device *dev = &cell->nvmem->dev;
> > > > > > > > > > > +	int err;
> > > > > > > > > > > +
> > > > > > > > > > >       	mutex_lock(&nvmem_mutex);
> > > > > > > > > > >       	list_add_tail(&cell->node, &cell->nvmem->cells);
> > > > > > > > > > >       	mutex_unlock(&nvmem_mutex);
> > > > > > > > > > > +
> > > > > > > > > > > +	sysfs_attr_init(&cell->battr.attr);
> > > > > > > > > > > +	cell->battr.attr.name = cell->name;
> > > > > > > > > > > +	cell->battr.attr.mode = 0400;
> > > > > > > > > > > +	cell->battr.read = nvmem_cell_attr_read;
> > > > > > > > > > > +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
> > > > > > > > > > > +					  nvmem_cells_group.name);
> > > > > > > > > > 
> > > > > > > > > > Why not just use the is_bin_visible attribute instead to determine if
> > > > > > > > > > the attribute should be shown or not instead of having to add it
> > > > > > > > > > after-the-fact which will race with userspace and loose?
> > > > > > > > > 
> > > > > > > > > I'm sorry I really don't see how you suggest to get it done.
> > > > > > > > > 
> > > > > > > > > I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
> > > > > > > > 
> > > > > > > > Great.
> > > > > > > > 
> > > > > > > > > I don't understand addig-after-the-fact part. How is .is_bin_visible()
> > > > > > > > > related to adding attributes for newly created cells?
> > > > > > > > 
> > > > > > > > You are adding a sysfs attribute to a device that is already registered
> > > > > > > > in the driver core, and so the creation of that attribute is never seen
> > > > > > > > by userspace.  The attribute needs to be attached to the device _BEFORE_
> > > > > > > > it is registered.
> > > > > > > > 
> > > > > > > > Also, huge hint, if a driver has to call as sysfs_*() call, something is
> > > > > > > > wrong.
> > > > > > > > 
> > > > > > > > > Do you mean I can
> > > > > > > > > avoid calling sysfs_add_bin_file_to_group()?
> > > > > > > > 
> > > > > > > > Yes.
> > > > > > > > 
> > > > > > > > > Do you recall any existing example of such solution?
> > > > > > > > 
> > > > > > > > Loads.
> > > > > > > > 
> > > > > > > > Just add this attribute group to your driver as a default attribute
> > > > > > > > group and the driver core will create it for you if needed.
> > > > > > > > 
> > > > > > > > Or if you always need it, no need to mess sith is_bin_visible() at all,
> > > > > > > > I can't really understand what you are trying to do here at all.
> > > > > > > 
> > > > > > > Thanks a lot! In nvmem_register() first there is a call to the
> > > > > > > device_register() and only later cells get added. I suppose I just have
> > > > > > > to rework nvmem_register() order so that:
> > > > > > > 1. Cells are collected earlier. For each cell I allocate group attribute
> > > > > > 
> > > > > > No, add all of the attributes to the device at the beginning before you
> > > > > > register it, there's no need to allocate anything.
> > > > > 
> > > > > If you mean static structures I can't do that, because cells almost
> > > > > never are static. They are not known in advance. nvmem allows cells to
> > > > > be:
> > > > > 1. Specified in OF
> > > > > 2. Submitted as list while registering a NVMEM device
> > > > > 
> > > > > So every cells gets its own structure allocated dynamically. My plan is
> > > > > to put bin_attribute in that struct and then create a group collecting
> > > > > all those cells.
> > > > 
> > > > A device has a driver associated with it, and that driver has default
> > > > groups associated with it.  Use that, I am not saying to use static
> > > > structures, that is not how the driver model works at all.
> > > 
> > > I'm helpless on dealing with attributes.
> > > 
> > > I tried building a list of attributes dynamically but that of course
> > > fails:
> > > 
> > > drivers/nvmem/core.c: In function ‘nvmem_register’:
> > > drivers/nvmem/core.c:930:31: error: assignment of member ‘bin_attrs’ in read-only object
> > >    930 |   nvmem_cells_group.bin_attrs = nvmem->cells_bin_attrs;
> > >        |                               ^
> > > 
> > > 
> > > What I'm trying to achieve is having
> > > /sys/bus/nvmem/devices/*/cells/*
> > > with each file being an attribute.
> > 
> > What is the full path here that you are looking to add these attributes
> > to?  Where is the struct device in play?  What .c file should I look at?
> > 
> > > Please kindly point me to a single example of "struct attribute_group"
> > > that has a variable list of attributes with each attribute having
> > > runtime set name.
> > 
> > Why would you ever want each attribute have a runtime-set name?  That's
> > not what attributes are for.  Think of them as "key/value" pairs.  The
> > "key" part is the name (i.e. filename), that is well known to everyone,
> > unique to that struct device type, and documented in Documentation/ABI/.
> > The "value" part is the value you read from the file (or write to it.)
> > 
> > That's it, it's not all that complex.
> > 
> > > Almost all cases in kernel look like:
> > > static const struct attribute_group foo_attr_group = {
> > > 	.attrs = foo_attrs,
> > > };
> > > with "foo_attrs" being a list of attributes with *predefined* names.
> > 
> > Yes, because that is what you really want.
> > 
> > Why do you feel this device is somehow unique to deserve attributes that
> > are not predefined?  And if they are not predefined, how are you going
> > to define them when you create them in the code and document them?   :)
> > 
> > > Every example of dynamic attributes (runtime created) I see in a kernel
> > > (e.g. drivers/base/node.c) uses sysfs_*().
> > 
> > drivers/base/* is not the best place to look at for how to implement
> > bus/driver logic, look at existing busses and drivers instead please.
> > We have a few hundred to choose from :)
> > 
> > So, let's break it down, what exactly are you wanting your device on
> > your bus to look like?  What will be the attributes you want to expose,
> > and what are the values of those attributes?  You have to start with
> > that, as Documentation/ABI/ is going to require you to write them down.
> 
> This patch subject / body is a basic summary. It's about
> drivers/nvmem/core.c .
> 
> 
> Let me explain it with more details:
> 
> NVMEM is a data blob.
> NVMEM consists of entries called cells.
> 
> 
> Example:
> 
> U-Boot environment variables is NVMEM. Example:
> 00000000  c4 09 30 54 62 6f 6f 74  63 6d 64 3d 74 66 74 70  |..0Tbootcmd=tftp|
> 00000010  00 62 6f 6f 74 64 65 6c  61 79 3d 35 00 65 74 68  |.bootdelay=5.eth|
> 
> A single environment variable is NVMEM cell. Example:
> bootcmd=tftp
> bootdelay=5
> 
> 
> Every NVMEM device is exposed in sysfs as:
> /sys/bus/nvmem/devices/*/
> 
> You can read NVMEM blob doing
> cat /sys/bus/nvmem/devices/*/nvmem | hexdump -C
> 
> 
> What I'm trying to do is to expose NVMEM cells in sysfs as:
> /sys/bus/nvmem/devices/cells/*

You forgot "foo" in there :)

How are nvmem devices named?

> Example:
> $ cat /sys/bus/nvmem/devices/foo/cells/bootcmd
> tftp
> $ cat /sys/bus/nvmem/devices/foo/cells/bootdelay
> 5
> 
> As you can see above NVMEM cells are not known at compilation time.

Why do you want to expose these in a way that forces the kernel to parse
these key/value pairs?  Why not just do it all in userspace like you can
today?  What forces the kernel to do it and not a perl script?

> So I believe the question is: how can I expose cells in sysfs?

You can do this by dynamically creating the attributes on the fly, but
your show function is going to be rough and it's not going to be simple
to do so.  One example will be the code that creates the
/sys/devices/system/machinecheck/machinecheckXX/bank* files.

But I will push back again, why not just do it all in userspace?  What
userspace tool is requiring the kernel to do this work for it?

And none of this is a binary attribute file, which is what you were
trying to do in your original patch.  binary attribute files treat the
kernel as a pass-through, no parsing of the data is allowed by the
kernel.  Which is fine for the "nvmem" sysfs file, but not for your
key/value pairs.

thanks,

greg k-h

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Rafał Miłecki]

On 21.12.2021 14:27, Greg Kroah-Hartman wrote:
> On Tue, Dec 21, 2021 at 02:05:34PM +0100, Rafał Miłecki wrote:
>> On 21.12.2021 13:56, Greg Kroah-Hartman wrote:
>>> On Tue, Dec 21, 2021 at 01:24:00PM +0100, Rafał Miłecki wrote:
>>>> On 21.12.2021 08:13, Greg Kroah-Hartman wrote:
>>>>> On Tue, Dec 21, 2021 at 07:53:32AM +0100, Rafał Miłecki wrote:
>>>>>> On 21.12.2021 07:45, Greg Kroah-Hartman wrote:
>>>>>>> On Tue, Dec 21, 2021 at 07:39:24AM +0100, Rafał Miłecki wrote:
>>>>>>>> On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
>>>>>>>>> On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
>>>>>>>>>> Hi Greg,
>>>>>>>>>>
>>>>>>>>>> On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
>>>>>>>>>>> On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
>>>>>>>>>>>>        static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
>>>>>>>>>>>>        {
>>>>>>>>>>>> +	struct device *dev = &cell->nvmem->dev;
>>>>>>>>>>>> +	int err;
>>>>>>>>>>>> +
>>>>>>>>>>>>        	mutex_lock(&nvmem_mutex);
>>>>>>>>>>>>        	list_add_tail(&cell->node, &cell->nvmem->cells);
>>>>>>>>>>>>        	mutex_unlock(&nvmem_mutex);
>>>>>>>>>>>> +
>>>>>>>>>>>> +	sysfs_attr_init(&cell->battr.attr);
>>>>>>>>>>>> +	cell->battr.attr.name = cell->name;
>>>>>>>>>>>> +	cell->battr.attr.mode = 0400;
>>>>>>>>>>>> +	cell->battr.read = nvmem_cell_attr_read;
>>>>>>>>>>>> +	err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
>>>>>>>>>>>> +					  nvmem_cells_group.name);
>>>>>>>>>>>
>>>>>>>>>>> Why not just use the is_bin_visible attribute instead to determine if
>>>>>>>>>>> the attribute should be shown or not instead of having to add it
>>>>>>>>>>> after-the-fact which will race with userspace and loose?
>>>>>>>>>>
>>>>>>>>>> I'm sorry I really don't see how you suggest to get it done.
>>>>>>>>>>
>>>>>>>>>> I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
>>>>>>>>>
>>>>>>>>> Great.
>>>>>>>>>
>>>>>>>>>> I don't understand addig-after-the-fact part. How is .is_bin_visible()
>>>>>>>>>> related to adding attributes for newly created cells?
>>>>>>>>>
>>>>>>>>> You are adding a sysfs attribute to a device that is already registered
>>>>>>>>> in the driver core, and so the creation of that attribute is never seen
>>>>>>>>> by userspace.  The attribute needs to be attached to the device _BEFORE_
>>>>>>>>> it is registered.
>>>>>>>>>
>>>>>>>>> Also, huge hint, if a driver has to call as sysfs_*() call, something is
>>>>>>>>> wrong.
>>>>>>>>>
>>>>>>>>>> Do you mean I can
>>>>>>>>>> avoid calling sysfs_add_bin_file_to_group()?
>>>>>>>>>
>>>>>>>>> Yes.
>>>>>>>>>
>>>>>>>>>> Do you recall any existing example of such solution?
>>>>>>>>>
>>>>>>>>> Loads.
>>>>>>>>>
>>>>>>>>> Just add this attribute group to your driver as a default attribute
>>>>>>>>> group and the driver core will create it for you if needed.
>>>>>>>>>
>>>>>>>>> Or if you always need it, no need to mess sith is_bin_visible() at all,
>>>>>>>>> I can't really understand what you are trying to do here at all.
>>>>>>>>
>>>>>>>> Thanks a lot! In nvmem_register() first there is a call to the
>>>>>>>> device_register() and only later cells get added. I suppose I just have
>>>>>>>> to rework nvmem_register() order so that:
>>>>>>>> 1. Cells are collected earlier. For each cell I allocate group attribute
>>>>>>>
>>>>>>> No, add all of the attributes to the device at the beginning before you
>>>>>>> register it, there's no need to allocate anything.
>>>>>>
>>>>>> If you mean static structures I can't do that, because cells almost
>>>>>> never are static. They are not known in advance. nvmem allows cells to
>>>>>> be:
>>>>>> 1. Specified in OF
>>>>>> 2. Submitted as list while registering a NVMEM device
>>>>>>
>>>>>> So every cells gets its own structure allocated dynamically. My plan is
>>>>>> to put bin_attribute in that struct and then create a group collecting
>>>>>> all those cells.
>>>>>
>>>>> A device has a driver associated with it, and that driver has default
>>>>> groups associated with it.  Use that, I am not saying to use static
>>>>> structures, that is not how the driver model works at all.
>>>>
>>>> I'm helpless on dealing with attributes.
>>>>
>>>> I tried building a list of attributes dynamically but that of course
>>>> fails:
>>>>
>>>> drivers/nvmem/core.c: In function ‘nvmem_register’:
>>>> drivers/nvmem/core.c:930:31: error: assignment of member ‘bin_attrs’ in read-only object
>>>>     930 |   nvmem_cells_group.bin_attrs = nvmem->cells_bin_attrs;
>>>>         |                               ^
>>>>
>>>>
>>>> What I'm trying to achieve is having
>>>> /sys/bus/nvmem/devices/*/cells/*
>>>> with each file being an attribute.
>>>
>>> What is the full path here that you are looking to add these attributes
>>> to?  Where is the struct device in play?  What .c file should I look at?
>>>
>>>> Please kindly point me to a single example of "struct attribute_group"
>>>> that has a variable list of attributes with each attribute having
>>>> runtime set name.
>>>
>>> Why would you ever want each attribute have a runtime-set name?  That's
>>> not what attributes are for.  Think of them as "key/value" pairs.  The
>>> "key" part is the name (i.e. filename), that is well known to everyone,
>>> unique to that struct device type, and documented in Documentation/ABI/.
>>> The "value" part is the value you read from the file (or write to it.)
>>>
>>> That's it, it's not all that complex.
>>>
>>>> Almost all cases in kernel look like:
>>>> static const struct attribute_group foo_attr_group = {
>>>> 	.attrs = foo_attrs,
>>>> };
>>>> with "foo_attrs" being a list of attributes with *predefined* names.
>>>
>>> Yes, because that is what you really want.
>>>
>>> Why do you feel this device is somehow unique to deserve attributes that
>>> are not predefined?  And if they are not predefined, how are you going
>>> to define them when you create them in the code and document them?   :)
>>>
>>>> Every example of dynamic attributes (runtime created) I see in a kernel
>>>> (e.g. drivers/base/node.c) uses sysfs_*().
>>>
>>> drivers/base/* is not the best place to look at for how to implement
>>> bus/driver logic, look at existing busses and drivers instead please.
>>> We have a few hundred to choose from :)
>>>
>>> So, let's break it down, what exactly are you wanting your device on
>>> your bus to look like?  What will be the attributes you want to expose,
>>> and what are the values of those attributes?  You have to start with
>>> that, as Documentation/ABI/ is going to require you to write them down.
>>
>> This patch subject / body is a basic summary. It's about
>> drivers/nvmem/core.c .
>>
>>
>> Let me explain it with more details:
>>
>> NVMEM is a data blob.
>> NVMEM consists of entries called cells.
>>
>>
>> Example:
>>
>> U-Boot environment variables is NVMEM. Example:
>> 00000000  c4 09 30 54 62 6f 6f 74  63 6d 64 3d 74 66 74 70  |..0Tbootcmd=tftp|
>> 00000010  00 62 6f 6f 74 64 65 6c  61 79 3d 35 00 65 74 68  |.bootdelay=5.eth|
>>
>> A single environment variable is NVMEM cell. Example:
>> bootcmd=tftp
>> bootdelay=5
>>
>>
>> Every NVMEM device is exposed in sysfs as:
>> /sys/bus/nvmem/devices/*/
>>
>> You can read NVMEM blob doing
>> cat /sys/bus/nvmem/devices/*/nvmem | hexdump -C
>>
>>
>> What I'm trying to do is to expose NVMEM cells in sysfs as:
>> /sys/bus/nvmem/devices/cells/*
> 
> You forgot "foo" in there :)

Right!


> How are nvmem devices named?

$ ls /sys/bus/nvmem/devices/
brcm-nvram0
mtd0
mtd1
u-boot-envvar0


>> Example:
>> $ cat /sys/bus/nvmem/devices/foo/cells/bootcmd
>> tftp
>> $ cat /sys/bus/nvmem/devices/foo/cells/bootdelay
>> 5
>>
>> As you can see above NVMEM cells are not known at compilation time.
> 
> Why do you want to expose these in a way that forces the kernel to parse
> these key/value pairs?  Why not just do it all in userspace like you can
> today?  What forces the kernel to do it and not a perl script?
> 
>> So I believe the question is: how can I expose cells in sysfs?
> 
> You can do this by dynamically creating the attributes on the fly, but
> your show function is going to be rough and it's not going to be simple
> to do so.  One example will be the code that creates the
> /sys/devices/system/machinecheck/machinecheckXX/bank* files.
> 
> But I will push back again, why not just do it all in userspace?  What
> userspace tool is requiring the kernel to do this work for it?

Environment data contains info that may be required by kernel.

For example some home routers store two firmwares on flash. Kernel needs
to read index of currently booted firmware to make sure MTD subsystem
creates partitions correctly.

Another example: MAC address. Ethernet subsystem supports reading MAC
from NVMEM cell.

One could argue those tasks could be handled from userspace but that
would get tricky. Sure - we have API for setting MAC address. However
other cases (like setting active firmware partition and asking MTD to
parse it into subpartitions) would require new user <-> kernel
interfaces.


> And none of this is a binary attribute file, which is what you were
> trying to do in your original patch.  binary attribute files treat the
> kernel as a pass-through, no parsing of the data is allowed by the
> kernel.  Which is fine for the "nvmem" sysfs file, but not for your
> key/value pairs.

Oops, that should be standard attribute then.

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 02:52:05PM +0100, Rafał Miłecki wrote:
> > How are nvmem devices named?
> 
> $ ls /sys/bus/nvmem/devices/
> brcm-nvram0
> mtd0
> mtd1
> u-boot-envvar0

So no naming scheme at all.

{sigh}

> > > Example:
> > > $ cat /sys/bus/nvmem/devices/foo/cells/bootcmd
> > > tftp
> > > $ cat /sys/bus/nvmem/devices/foo/cells/bootdelay
> > > 5
> > > 
> > > As you can see above NVMEM cells are not known at compilation time.
> > 
> > Why do you want to expose these in a way that forces the kernel to parse
> > these key/value pairs?  Why not just do it all in userspace like you can
> > today?  What forces the kernel to do it and not a perl script?
> > 
> > > So I believe the question is: how can I expose cells in sysfs?
> > 
> > You can do this by dynamically creating the attributes on the fly, but
> > your show function is going to be rough and it's not going to be simple
> > to do so.  One example will be the code that creates the
> > /sys/devices/system/machinecheck/machinecheckXX/bank* files.
> > 
> > But I will push back again, why not just do it all in userspace?  What
> > userspace tool is requiring the kernel to do this work for it?
> 
> Environment data contains info that may be required by kernel.
> 
> For example some home routers store two firmwares on flash. Kernel needs
> to read index of currently booted firmware to make sure MTD subsystem
> creates partitions correctly.

You are talking about a kernel<->kernel api here, that's not what sysfs
is for at all.

> Another example: MAC address. Ethernet subsystem supports reading MAC
> from NVMEM cell.

Again, internal kernel api, nothing sysfs is ever involved in.

> One could argue those tasks could be handled from userspace but that
> would get tricky. Sure - we have API for setting MAC address. However
> other cases (like setting active firmware partition and asking MTD to
> parse it into subpartitions) would require new user <-> kernel
> interfaces.

Ok, but again, sysfs is for userspace to get access to these values.
That's what I'm concerned about.  If you want to make an in-kernel api
for other subsystems to get these key/value pairs, wonderful, that has
nothing to do with sysfs.

So I ask again, why do you want to expose these to userspace through
sysfs in a new format from what you have today.  Who is going to use
that information and what is it going to be used for.

thanks,

greg k-h

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Rafał Miłecki]

On 21.12.2021 15:27, Greg Kroah-Hartman wrote:
> On Tue, Dec 21, 2021 at 02:52:05PM +0100, Rafał Miłecki wrote:
>>> How are nvmem devices named?
>>
>> $ ls /sys/bus/nvmem/devices/
>> brcm-nvram0
>> mtd0
>> mtd1
>> u-boot-envvar0
> 
> So no naming scheme at all.
> 
> {sigh}
> 
>>>> Example:
>>>> $ cat /sys/bus/nvmem/devices/foo/cells/bootcmd
>>>> tftp
>>>> $ cat /sys/bus/nvmem/devices/foo/cells/bootdelay
>>>> 5
>>>>
>>>> As you can see above NVMEM cells are not known at compilation time.
>>>
>>> Why do you want to expose these in a way that forces the kernel to parse
>>> these key/value pairs?  Why not just do it all in userspace like you can
>>> today?  What forces the kernel to do it and not a perl script?
>>>
>>>> So I believe the question is: how can I expose cells in sysfs?
>>>
>>> You can do this by dynamically creating the attributes on the fly, but
>>> your show function is going to be rough and it's not going to be simple
>>> to do so.  One example will be the code that creates the
>>> /sys/devices/system/machinecheck/machinecheckXX/bank* files.
>>>
>>> But I will push back again, why not just do it all in userspace?  What
>>> userspace tool is requiring the kernel to do this work for it?
>>
>> Environment data contains info that may be required by kernel.
>>
>> For example some home routers store two firmwares on flash. Kernel needs
>> to read index of currently booted firmware to make sure MTD subsystem
>> creates partitions correctly.
> 
> You are talking about a kernel<->kernel api here, that's not what sysfs
> is for at all.
> 
>> Another example: MAC address. Ethernet subsystem supports reading MAC
>> from NVMEM cell.
> 
> Again, internal kernel api, nothing sysfs is ever involved in.
> 
>> One could argue those tasks could be handled from userspace but that
>> would get tricky. Sure - we have API for setting MAC address. However
>> other cases (like setting active firmware partition and asking MTD to
>> parse it into subpartitions) would require new user <-> kernel
>> interfaces.
> 
> Ok, but again, sysfs is for userspace to get access to these values.
> That's what I'm concerned about.  If you want to make an in-kernel api
> for other subsystems to get these key/value pairs, wonderful, that has
> nothing to do with sysfs.
> 
> So I ask again, why do you want to expose these to userspace through
> sysfs in a new format from what you have today.  Who is going to use
> that information and what is it going to be used for.

Sorry, you asked why I need parsing in kernel and I focused on that part
only.

For user space there are also some relevant U-Boot environment entries
(NVMEM cells). Examples: serial number, country code (e.g. WiFi
purposes), default passwords (as printed on labels).

So both: kernel and user space need to access U-Boot environment
variables (NVMEM cells). Each for its own purposes.

Kernel goes first so it needs its own parser of NVMEM content (data).

User space can either: get NVMEM cells exposed by kernel OR parse NVMEM
content on its own. I thought it'd be nice to avoid parsing code
duplication in user space and let kernel expose NVMEM cells.

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 04:09:13PM +0100, Rafał Miłecki wrote:
> So both: kernel and user space need to access U-Boot environment
> variables (NVMEM cells). Each for its own purposes.
> 
> Kernel goes first so it needs its own parser of NVMEM content (data).
> 
> User space can either: get NVMEM cells exposed by kernel OR parse NVMEM
> content on its own. I thought it'd be nice to avoid parsing code
> duplication in user space and let kernel expose NVMEM cells.

Ah, so you already have the data parsed, and you just want to also
expose it to userspace.  That makes more sense (sorry, it's been a long
day of reviewing crappy patches, not yours of course...)

So sure, you can dynamically create attributes and then add them to the
device before you register it with the driver core.  Be sure to
initialize them properly with the call I pointed out previously and you
should be good to go.  You will have to keep a list of them around and
then free them yourself when the device is cleaned up, so watch out for
that.

And again, don't use a binary attribute, that's not what it is for.

thanks,

greg k-h

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[Rafał Miłecki]

On 21.12.2021 16:18, Greg Kroah-Hartman wrote:
> On Tue, Dec 21, 2021 at 04:09:13PM +0100, Rafał Miłecki wrote:
>> So both: kernel and user space need to access U-Boot environment
>> variables (NVMEM cells). Each for its own purposes.
>>
>> Kernel goes first so it needs its own parser of NVMEM content (data).
>>
>> User space can either: get NVMEM cells exposed by kernel OR parse NVMEM
>> content on its own. I thought it'd be nice to avoid parsing code
>> duplication in user space and let kernel expose NVMEM cells.
> 
> Ah, so you already have the data parsed, and you just want to also
> expose it to userspace.  That makes more sense (sorry, it's been a long
> day of reviewing crappy patches, not yours of course...)
> 
> So sure, you can dynamically create attributes and then add them to the
> device before you register it with the driver core.  Be sure to
> initialize them properly with the call I pointed out previously and you
> should be good to go.  You will have to keep a list of them around and
> then free them yourself when the device is cleaned up, so watch out for
> that.
> 
> And again, don't use a binary attribute, that's not what it is for.

Thanks for review & discussing this! I really appreciate you getting
this patch details out of me so it's clear how to proceed.

Lesson learnt: spend more time on describing my commits.

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[kernel test robot]

Hi "Rafał,

I love your patch! Yet something to improve:

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on char-misc/char-misc-testing linux/master linus/master v5.16-rc6 next-20211217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git aa483f3ce655ed9ee4f32d050d1822eec2d20ada
config: m68k-m5208evb_defconfig (https://download.01.org/0day-ci/archive/20211220/202112202227.nt77aHJ2-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/8408088794497c22b0fd47fe2f5e5add1ab00ec9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
        git checkout 8408088794497c22b0fd47fe2f5e5add1ab00ec9
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash drivers/nvmem/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/nvmem/core.c: In function 'nvmem_cell_entry_drop':
   drivers/nvmem/core.c:452:38: error: 'nvmem_cells_group' undeclared (first use in this function); did you mean 'nvmem_cell_lookup'?
     452 |                                      nvmem_cells_group.name);
         |                                      ^~~~~~~~~~~~~~~~~
         |                                      nvmem_cell_lookup
   drivers/nvmem/core.c:452:38: note: each undeclared identifier is reported only once for each function it appears in
   drivers/nvmem/core.c: In function 'nvmem_cell_entry_add':
>> drivers/nvmem/core.c:509:15: error: implicit declaration of function 'sysfs_add_bin_file_to_group'; did you mean 'sysfs_add_file_to_group'? [-Werror=implicit-function-declaration]
     509 |         err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
         |               sysfs_add_file_to_group
   drivers/nvmem/core.c:510:43: error: 'nvmem_cells_group' undeclared (first use in this function); did you mean 'nvmem_cell_lookup'?
     510 |                                           nvmem_cells_group.name);
         |                                           ^~~~~~~~~~~~~~~~~
         |                                           nvmem_cell_lookup
   cc1: some warnings being treated as errors


vim +509 drivers/nvmem/core.c

   495	
   496	static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
   497	{
   498		struct device *dev = &cell->nvmem->dev;
   499		int err;
   500	
   501		mutex_lock(&nvmem_mutex);
   502		list_add_tail(&cell->node, &cell->nvmem->cells);
   503		mutex_unlock(&nvmem_mutex);
   504	
   505		sysfs_attr_init(&cell->battr.attr);
   506		cell->battr.attr.name = cell->name;
   507		cell->battr.attr.mode = 0400;
   508		cell->battr.read = nvmem_cell_attr_read;
 > 509		err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
   510						  nvmem_cells_group.name);
   511		if (err)
   512			dev_warn(dev, "Failed to add %s cell: %d\n", cell->name, err);
   513	
   514		blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_ADD, cell);
   515	}
   516	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4002 bytes --]

Hi "Rafał,

I love your patch! Yet something to improve:

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on char-misc/char-misc-testing linux/master linus/master v5.16-rc6 next-20211217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git aa483f3ce655ed9ee4f32d050d1822eec2d20ada
config: m68k-m5208evb_defconfig (https://download.01.org/0day-ci/archive/20211220/202112202227.nt77aHJ2-lkp(a)intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/8408088794497c22b0fd47fe2f5e5add1ab00ec9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
        git checkout 8408088794497c22b0fd47fe2f5e5add1ab00ec9
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash drivers/nvmem/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/nvmem/core.c: In function 'nvmem_cell_entry_drop':
   drivers/nvmem/core.c:452:38: error: 'nvmem_cells_group' undeclared (first use in this function); did you mean 'nvmem_cell_lookup'?
     452 |                                      nvmem_cells_group.name);
         |                                      ^~~~~~~~~~~~~~~~~
         |                                      nvmem_cell_lookup
   drivers/nvmem/core.c:452:38: note: each undeclared identifier is reported only once for each function it appears in
   drivers/nvmem/core.c: In function 'nvmem_cell_entry_add':
>> drivers/nvmem/core.c:509:15: error: implicit declaration of function 'sysfs_add_bin_file_to_group'; did you mean 'sysfs_add_file_to_group'? [-Werror=implicit-function-declaration]
     509 |         err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
         |               sysfs_add_file_to_group
   drivers/nvmem/core.c:510:43: error: 'nvmem_cells_group' undeclared (first use in this function); did you mean 'nvmem_cell_lookup'?
     510 |                                           nvmem_cells_group.name);
         |                                           ^~~~~~~~~~~~~~~~~
         |                                           nvmem_cell_lookup
   cc1: some warnings being treated as errors


vim +509 drivers/nvmem/core.c

   495	
   496	static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
   497	{
   498		struct device *dev = &cell->nvmem->dev;
   499		int err;
   500	
   501		mutex_lock(&nvmem_mutex);
   502		list_add_tail(&cell->node, &cell->nvmem->cells);
   503		mutex_unlock(&nvmem_mutex);
   504	
   505		sysfs_attr_init(&cell->battr.attr);
   506		cell->battr.attr.name = cell->name;
   507		cell->battr.attr.mode = 0400;
   508		cell->battr.read = nvmem_cell_attr_read;
 > 509		err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
   510						  nvmem_cells_group.name);
   511		if (err)
   512			dev_warn(dev, "Failed to add %s cell: %d\n", cell->name, err);
   513	
   514		blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_ADD, cell);
   515	}
   516	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 2/2] nvmem: expose NVMEM cells in sysfs

[John Thomson]

Hi Rafał,

Thank you for working on this.
I hacked together the same functionality based on kernel 5.10 some months ago,
while I was testing an MTD parser based TLV NVMEM cells parser,
and have some general comments.

On Mon, 20 Dec 2021, at 06:47, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This allows reading NVMEM cells using /sys/bus/nvmem/devices/*/cells/*
> which may be helpful for userspace & debugging purposes.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  Documentation/driver-api/nvmem.rst | 11 ++++++
>  drivers/nvmem/core.c               | 60 ++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
>
> diff --git a/Documentation/driver-api/nvmem.rst 
> b/Documentation/driver-api/nvmem.rst
> index 287e86819640..20f7d68143be 100644
> --- a/Documentation/driver-api/nvmem.rst
> +++ b/Documentation/driver-api/nvmem.rst
> @@ -185,6 +185,17 @@ ex::
>    *
>    0001000
> 
> +Single cells can be read using files located at::
> +
> +	/sys/bus/nvmem/devices/*/cells/*
> +
> +ex::
> +
> +  hexdump -C /sys/bus/nvmem/devices/mtd0/cells/mac

I found that NVMEM cell names did not need to be unique, but sysfs entry names do.
I am not sure if there are characters that NVMEM cell names allow,
but sysfs entry names forbid.
I used an ID allocator, in a similar style to what is done for MTD sysfs devices in mtdcore.c.

Could the cell (data) binary attribute be in a subfolder for each cell?
Example:
/sys/bus/nvmem/devices/mtd0/cells/cell0/cell
or
/sys/bus/nvmem/devices/mtd0/cells/cell0/cell_data

This way, we can easily expose additional properties of the cell.
I exposed the name, offset, bytes, bit_offset, and an of_node link (where available) this way.
I only needed these for testing, but the cell length (bytes) provided a cheap means for initial validation.

I did not look into cell post-processing, but it should be considered:
https://lore.kernel.org/all/20211006144729.15268-1-srinivas.kandagatla@linaro.org/
and RFC https://lore.kernel.org/all/CAL_JsqL55mZJ6jUyQACer2pKMNDV08-FgwBREsJVgitnuF18Cg@mail.gmail.com/
Will the cell data exposed here be prior to post-processing?


Thank you

Cheers,
-- 
  John Thomson

Re: [PATCH 1/2] sysfs: add sysfs_add_bin_file_to_group()

[Greg Kroah-Hartman]

On Mon, Dec 20, 2021 at 07:47:29AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> There already is sysfs_add_file_to_group() for adding "attribute" to a
> group. This new function allows adding "bin_attribute" as well.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  fs/sysfs/file.c       | 31 +++++++++++++++++++++++++++----
>  include/linux/sysfs.h |  3 +++
>  2 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 42dcf96881b6..30c798c38d89 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -376,14 +376,19 @@ EXPORT_SYMBOL_GPL(sysfs_create_files);
>   * @attr: attribute descriptor.
>   * @group: group name.
>   */
> -int sysfs_add_file_to_group(struct kobject *kobj,
> -		const struct attribute *attr, const char *group)
> +int __sysfs_add_file_to_group(struct kobject *kobj,
> +			      const struct attribute *attr,
> +			      const struct bin_attribute *battr,
> +			      const char *group)
>  {
>  	struct kernfs_node *parent;
>  	kuid_t uid;
>  	kgid_t gid;
>  	int error;
>  
> +	if (WARN_ON((attr && battr) || (!attr && !battr)))
> +		return -EINVAL;

How can this ever happen?

Re: [PATCH 1/2] sysfs: add sysfs_add_bin_file_to_group()

[kernel test robot]

Hi "Rafał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on char-misc/char-misc-testing linux/master linus/master v5.16-rc6 next-20211217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git aa483f3ce655ed9ee4f32d050d1822eec2d20ada
config: nds32-allnoconfig (https://download.01.org/0day-ci/archive/20211220/202112202251.hdWPQftO-lkp@intel.com/config)
compiler: nds32le-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a9802080b6d35af5dfd7ae847a0978e47caafd59
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
        git checkout a9802080b6d35af5dfd7ae847a0978e47caafd59
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nds32 SHELL=/bin/bash fs/sysfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/sysfs/file.c:379:5: warning: no previous prototype for '__sysfs_add_file_to_group' [-Wmissing-prototypes]
     379 | int __sysfs_add_file_to_group(struct kobject *kobj,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~
--
   fs/sysfs/file.c:383: warning: Function parameter or member 'battr' not described in '__sysfs_add_file_to_group'
>> fs/sysfs/file.c:383: warning: expecting prototype for sysfs_add_file_to_group(). Prototype was for __sysfs_add_file_to_group() instead


vim +/__sysfs_add_file_to_group +379 fs/sysfs/file.c

   372	
   373	/**
   374	 * sysfs_add_file_to_group - add an attribute file to a pre-existing group.
   375	 * @kobj: object we're acting for.
   376	 * @attr: attribute descriptor.
   377	 * @group: group name.
   378	 */
 > 379	int __sysfs_add_file_to_group(struct kobject *kobj,
   380				      const struct attribute *attr,
   381				      const struct bin_attribute *battr,
   382				      const char *group)
 > 383	{
   384		struct kernfs_node *parent;
   385		kuid_t uid;
   386		kgid_t gid;
   387		int error;
   388	
   389		if (WARN_ON((attr && battr) || (!attr && !battr)))
   390			return -EINVAL;
   391	
   392		if (group) {
   393			parent = kernfs_find_and_get(kobj->sd, group);
   394		} else {
   395			parent = kobj->sd;
   396			kernfs_get(parent);
   397		}
   398	
   399		if (!parent)
   400			return -ENOENT;
   401	
   402		kobject_get_ownership(kobj, &uid, &gid);
   403		if (attr)
   404			error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid,
   405						       gid, NULL);
   406		else
   407			error = sysfs_add_bin_file_mode_ns(parent, battr, battr->attr.mode,
   408							   uid, gid, NULL);
   409		kernfs_put(parent);
   410	
   411		return error;
   412	}
   413	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/2] sysfs: add sysfs_add_bin_file_to_group()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3605 bytes --]

Hi "Rafał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on char-misc/char-misc-testing linux/master linus/master v5.16-rc6 next-20211217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git aa483f3ce655ed9ee4f32d050d1822eec2d20ada
config: nds32-allnoconfig (https://download.01.org/0day-ci/archive/20211220/202112202251.hdWPQftO-lkp(a)intel.com/config)
compiler: nds32le-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a9802080b6d35af5dfd7ae847a0978e47caafd59
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
        git checkout a9802080b6d35af5dfd7ae847a0978e47caafd59
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nds32 SHELL=/bin/bash fs/sysfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/sysfs/file.c:379:5: warning: no previous prototype for '__sysfs_add_file_to_group' [-Wmissing-prototypes]
     379 | int __sysfs_add_file_to_group(struct kobject *kobj,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~
--
   fs/sysfs/file.c:383: warning: Function parameter or member 'battr' not described in '__sysfs_add_file_to_group'
>> fs/sysfs/file.c:383: warning: expecting prototype for sysfs_add_file_to_group(). Prototype was for __sysfs_add_file_to_group() instead


vim +/__sysfs_add_file_to_group +379 fs/sysfs/file.c

   372	
   373	/**
   374	 * sysfs_add_file_to_group - add an attribute file to a pre-existing group.
   375	 * @kobj: object we're acting for.
   376	 * @attr: attribute descriptor.
   377	 * @group: group name.
   378	 */
 > 379	int __sysfs_add_file_to_group(struct kobject *kobj,
   380				      const struct attribute *attr,
   381				      const struct bin_attribute *battr,
   382				      const char *group)
 > 383	{
   384		struct kernfs_node *parent;
   385		kuid_t uid;
   386		kgid_t gid;
   387		int error;
   388	
   389		if (WARN_ON((attr && battr) || (!attr && !battr)))
   390			return -EINVAL;
   391	
   392		if (group) {
   393			parent = kernfs_find_and_get(kobj->sd, group);
   394		} else {
   395			parent = kobj->sd;
   396			kernfs_get(parent);
   397		}
   398	
   399		if (!parent)
   400			return -ENOENT;
   401	
   402		kobject_get_ownership(kobj, &uid, &gid);
   403		if (attr)
   404			error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid,
   405						       gid, NULL);
   406		else
   407			error = sysfs_add_bin_file_mode_ns(parent, battr, battr->attr.mode,
   408							   uid, gid, NULL);
   409		kernfs_put(parent);
   410	
   411		return error;
   412	}
   413	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 1/2] sysfs: add sysfs_add_bin_file_to_group()

[kernel test robot]

Hi "Rafał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on char-misc/char-misc-testing linux/master linus/master v5.16-rc6 next-20211217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git aa483f3ce655ed9ee4f32d050d1822eec2d20ada
config: x86_64-randconfig-r024-20211220 (https://download.01.org/0day-ci/archive/20211220/202112202254.l5IadaDs-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 555eacf75f21cd1dfc6363d73ad187b730349543)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a9802080b6d35af5dfd7ae847a0978e47caafd59
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
        git checkout a9802080b6d35af5dfd7ae847a0978e47caafd59
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/sysfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/sysfs/file.c:379:5: warning: no previous prototype for function '__sysfs_add_file_to_group' [-Wmissing-prototypes]
   int __sysfs_add_file_to_group(struct kobject *kobj,
       ^
   fs/sysfs/file.c:379:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int __sysfs_add_file_to_group(struct kobject *kobj,
   ^
   static 
   1 warning generated.


vim +/__sysfs_add_file_to_group +379 fs/sysfs/file.c

   372	
   373	/**
   374	 * sysfs_add_file_to_group - add an attribute file to a pre-existing group.
   375	 * @kobj: object we're acting for.
   376	 * @attr: attribute descriptor.
   377	 * @group: group name.
   378	 */
 > 379	int __sysfs_add_file_to_group(struct kobject *kobj,
   380				      const struct attribute *attr,
   381				      const struct bin_attribute *battr,
   382				      const char *group)
   383	{
   384		struct kernfs_node *parent;
   385		kuid_t uid;
   386		kgid_t gid;
   387		int error;
   388	
   389		if (WARN_ON((attr && battr) || (!attr && !battr)))
   390			return -EINVAL;
   391	
   392		if (group) {
   393			parent = kernfs_find_and_get(kobj->sd, group);
   394		} else {
   395			parent = kobj->sd;
   396			kernfs_get(parent);
   397		}
   398	
   399		if (!parent)
   400			return -ENOENT;
   401	
   402		kobject_get_ownership(kobj, &uid, &gid);
   403		if (attr)
   404			error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid,
   405						       gid, NULL);
   406		else
   407			error = sysfs_add_bin_file_mode_ns(parent, battr, battr->attr.mode,
   408							   uid, gid, NULL);
   409		kernfs_put(parent);
   410	
   411		return error;
   412	}
   413	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/2] sysfs: add sysfs_add_bin_file_to_group()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3624 bytes --]

Hi "Rafał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on char-misc/char-misc-testing linux/master linus/master v5.16-rc6 next-20211217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git aa483f3ce655ed9ee4f32d050d1822eec2d20ada
config: x86_64-randconfig-r024-20211220 (https://download.01.org/0day-ci/archive/20211220/202112202254.l5IadaDs-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 555eacf75f21cd1dfc6363d73ad187b730349543)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a9802080b6d35af5dfd7ae847a0978e47caafd59
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
        git checkout a9802080b6d35af5dfd7ae847a0978e47caafd59
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/sysfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/sysfs/file.c:379:5: warning: no previous prototype for function '__sysfs_add_file_to_group' [-Wmissing-prototypes]
   int __sysfs_add_file_to_group(struct kobject *kobj,
       ^
   fs/sysfs/file.c:379:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int __sysfs_add_file_to_group(struct kobject *kobj,
   ^
   static 
   1 warning generated.


vim +/__sysfs_add_file_to_group +379 fs/sysfs/file.c

   372	
   373	/**
   374	 * sysfs_add_file_to_group - add an attribute file to a pre-existing group.
   375	 * @kobj: object we're acting for.
   376	 * @attr: attribute descriptor.
   377	 * @group: group name.
   378	 */
 > 379	int __sysfs_add_file_to_group(struct kobject *kobj,
   380				      const struct attribute *attr,
   381				      const struct bin_attribute *battr,
   382				      const char *group)
   383	{
   384		struct kernfs_node *parent;
   385		kuid_t uid;
   386		kgid_t gid;
   387		int error;
   388	
   389		if (WARN_ON((attr && battr) || (!attr && !battr)))
   390			return -EINVAL;
   391	
   392		if (group) {
   393			parent = kernfs_find_and_get(kobj->sd, group);
   394		} else {
   395			parent = kobj->sd;
   396			kernfs_get(parent);
   397		}
   398	
   399		if (!parent)
   400			return -ENOENT;
   401	
   402		kobject_get_ownership(kobj, &uid, &gid);
   403		if (attr)
   404			error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid,
   405						       gid, NULL);
   406		else
   407			error = sysfs_add_bin_file_mode_ns(parent, battr, battr->attr.mode,
   408							   uid, gid, NULL);
   409		kernfs_put(parent);
   410	
   411		return error;
   412	}
   413	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[RFC PATCH] sysfs: __sysfs_add_file_to_group() can be static

[kernel test robot]

fs/sysfs/file.c:379:5: warning: symbol '__sysfs_add_file_to_group' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 file.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 30c798c38d89d..fe58c4fd4c60d 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -376,10 +376,10 @@ EXPORT_SYMBOL_GPL(sysfs_create_files);
  * @attr: attribute descriptor.
  * @group: group name.
  */
-int __sysfs_add_file_to_group(struct kobject *kobj,
-			      const struct attribute *attr,
-			      const struct bin_attribute *battr,
-			      const char *group)
+static int __sysfs_add_file_to_group(struct kobject *kobj,
+				     const struct attribute *attr,
+				     const struct bin_attribute *battr,
+				     const char *group)
 {
 	struct kernfs_node *parent;
 	kuid_t uid;

[RFC PATCH] sysfs: __sysfs_add_file_to_group() can be static

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

fs/sysfs/file.c:379:5: warning: symbol '__sysfs_add_file_to_group' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 file.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 30c798c38d89d..fe58c4fd4c60d 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -376,10 +376,10 @@ EXPORT_SYMBOL_GPL(sysfs_create_files);
  * @attr: attribute descriptor.
  * @group: group name.
  */
-int __sysfs_add_file_to_group(struct kobject *kobj,
-			      const struct attribute *attr,
-			      const struct bin_attribute *battr,
-			      const char *group)
+static int __sysfs_add_file_to_group(struct kobject *kobj,
+				     const struct attribute *attr,
+				     const struct bin_attribute *battr,
+				     const char *group)
 {
 	struct kernfs_node *parent;
 	kuid_t uid;

Re: [PATCH 1/2] sysfs: add sysfs_add_bin_file_to_group()

[kernel test robot]

Hi "Rafał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on char-misc/char-misc-testing linux/master linus/master v5.16-rc6 next-20211220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git aa483f3ce655ed9ee4f32d050d1822eec2d20ada
config: x86_64-randconfig-s021-20211219 (https://download.01.org/0day-ci/archive/20211221/202112210456.vwGC0F3M-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/a9802080b6d35af5dfd7ae847a0978e47caafd59
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
        git checkout a9802080b6d35af5dfd7ae847a0978e47caafd59
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/sysfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> fs/sysfs/file.c:379:5: sparse: sparse: symbol '__sysfs_add_file_to_group' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/2] sysfs: add sysfs_add_bin_file_to_group()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1908 bytes --]

Hi "Rafał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on char-misc/char-misc-testing linux/master linus/master v5.16-rc6 next-20211220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git aa483f3ce655ed9ee4f32d050d1822eec2d20ada
config: x86_64-randconfig-s021-20211219 (https://download.01.org/0day-ci/archive/20211221/202112210456.vwGC0F3M-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/a9802080b6d35af5dfd7ae847a0978e47caafd59
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafa-Mi-ecki/sysfs-add-sysfs_add_bin_file_to_group/20211220-144856
        git checkout a9802080b6d35af5dfd7ae847a0978e47caafd59
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/sysfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> fs/sysfs/file.c:379:5: sparse: sparse: symbol '__sysfs_add_file_to_group' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org