[PATCH v1 1/8] software node: Free resources explicitly when swnode_register() fails

[PATCH v1 1/8] software node: Free resources explicitly when swnode_register() fails

[Andy Shevchenko]

Currently we have a slightly twisted logic in swnode_register().
It frees resources that it doesn't allocate on error path and
in once case it relies on the ->release() implementation.

Untwist the logic by freeing resources explicitly when swnode_register()
fails. Currently it happens only in fwnode_create_software_node().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/swnode.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index fa3719ef80e4..456f5fe58b58 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -767,22 +767,19 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 	int ret;
 
 	swnode = kzalloc(sizeof(*swnode), GFP_KERNEL);
-	if (!swnode) {
-		ret = -ENOMEM;
-		goto out_err;
-	}
+	if (!swnode)
+		return ERR_PTR(-ENOMEM);
 
 	ret = ida_simple_get(parent ? &parent->child_ids : &swnode_root_ids,
 			     0, 0, GFP_KERNEL);
 	if (ret < 0) {
 		kfree(swnode);
-		goto out_err;
+		return ERR_PTR(ret);
 	}
 
 	swnode->id = ret;
 	swnode->node = node;
 	swnode->parent = parent;
-	swnode->allocated = allocated;
 	swnode->kobj.kset = swnode_kset;
 	fwnode_init(&swnode->fwnode, &software_node_ops);
 
@@ -803,16 +800,17 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 		return ERR_PTR(ret);
 	}
 
+	/*
+	 * Assign the flag only in the successful case, so
+	 * the above kobject_put() won't mess up with properties.
+	 */
+	swnode->allocated = allocated;
+
 	if (parent)
 		list_add_tail(&swnode->entry, &parent->children);
 
 	kobject_uevent(&swnode->kobj, KOBJ_ADD);
 	return &swnode->fwnode;
-
-out_err:
-	if (allocated)
-		property_entries_free(node->properties);
-	return ERR_PTR(ret);
 }
 
 /**
@@ -963,6 +961,7 @@ struct fwnode_handle *
 fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent)
 {
+	struct fwnode_handle *fwnode;
 	struct software_node *node;
 	struct swnode *p = NULL;
 	int ret;
@@ -987,7 +986,13 @@ fwnode_create_software_node(const struct property_entry *properties,
 
 	node->parent = p ? p->node : NULL;
 
-	return swnode_register(node, p, 1);
+	fwnode = swnode_register(node, p, 1);
+	if (IS_ERR(fwnode)) {
+		property_entries_free(node->properties);
+		kfree(node);
+	}
+
+	return fwnode;
 }
 EXPORT_SYMBOL_GPL(fwnode_create_software_node);
 
-- 
2.30.2

[PATCH v1 2/8] software node: Introduce software_node_alloc()/software_node_free()

[Andy Shevchenko]

Introduce software_node_alloc() and software_node_free() helpers.
This will help with code readability and maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/swnode.c | 47 ++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 456f5fe58b58..19aa44bc2628 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -720,19 +720,30 @@ software_node_find_by_name(const struct software_node *parent, const char *name)
 }
 EXPORT_SYMBOL_GPL(software_node_find_by_name);
 
-static int
-software_node_register_properties(struct software_node *node,
-				  const struct property_entry *properties)
+static struct software_node *software_node_alloc(const struct property_entry *properties)
 {
 	struct property_entry *props;
+	struct software_node *node;
 
 	props = property_entries_dup(properties);
 	if (IS_ERR(props))
-		return PTR_ERR(props);
+		return ERR_CAST(props);
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node) {
+		property_entries_free(props);
+		return ERR_PTR(-ENOMEM);
+	}
 
 	node->properties = props;
 
-	return 0;
+	return node;
+}
+
+static void software_node_free(const struct software_node *node)
+{
+	property_entries_free(node->properties);
+	kfree(node);
 }
 
 static void software_node_release(struct kobject *kobj)
@@ -746,10 +757,9 @@ static void software_node_release(struct kobject *kobj)
 		ida_simple_remove(&swnode_root_ids, swnode->id);
 	}
 
-	if (swnode->allocated) {
-		property_entries_free(swnode->node->properties);
-		kfree(swnode->node);
-	}
+	if (swnode->allocated)
+		software_node_free(swnode->node);
+
 	ida_destroy(&swnode->child_ids);
 	kfree(swnode);
 }
@@ -964,7 +974,6 @@ fwnode_create_software_node(const struct property_entry *properties,
 	struct fwnode_handle *fwnode;
 	struct software_node *node;
 	struct swnode *p = NULL;
-	int ret;
 
 	if (parent) {
 		if (IS_ERR(parent))
@@ -974,23 +983,15 @@ fwnode_create_software_node(const struct property_entry *properties,
 		p = to_swnode(parent);
 	}
 
-	node = kzalloc(sizeof(*node), GFP_KERNEL);
-	if (!node)
-		return ERR_PTR(-ENOMEM);
-
-	ret = software_node_register_properties(node, properties);
-	if (ret) {
-		kfree(node);
-		return ERR_PTR(ret);
-	}
+	node = software_node_alloc(properties);
+	if (IS_ERR(node))
+		return ERR_CAST(node);
 
 	node->parent = p ? p->node : NULL;
 
 	fwnode = swnode_register(node, p, 1);
-	if (IS_ERR(fwnode)) {
-		property_entries_free(node->properties);
-		kfree(node);
-	}
+	if (IS_ERR(fwnode))
+		software_node_free(node);
 
 	return fwnode;
 }
-- 
2.30.2

[PATCH v1 3/8] software node: Show properties and their values in sysfs

[Andy Shevchenko]

It's very convenient to see what properties and their values
are currently being assigned in the registered software nodes.

Show properties and their values in sysfs.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/swnode.c | 137 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 132 insertions(+), 5 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 19aa44bc2628..d7fe1a887d2d 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include <linux/sysfs.h>
 
 struct swnode {
 	int id;
@@ -17,6 +18,10 @@ struct swnode {
 	struct fwnode_handle fwnode;
 	const struct software_node *node;
 
+	/* properties in sysfs */
+	struct kobj_attribute *property_attrs;
+	struct attribute_group property_group;
+
 	/* hierarchy */
 	struct ida child_ids;
 	struct list_head entry;
@@ -25,6 +30,7 @@ struct swnode {
 
 	unsigned int allocated:1;
 	unsigned int managed:1;
+	unsigned int properties:1;
 };
 
 static DEFINE_IDA(swnode_root_ids);
@@ -299,6 +305,18 @@ static int property_entry_copy_data(struct property_entry *dst,
 	return 0;
 }
 
+static int property_entries_count(const struct property_entry *properties)
+{
+	int n = 0;
+
+	if (properties) {
+		while (properties[n].name)
+			n++;
+	}
+
+	return n;
+}
+
 /**
  * property_entries_dup - duplicate array of properties
  * @properties: array of properties to copy
@@ -310,15 +328,13 @@ struct property_entry *
 property_entries_dup(const struct property_entry *properties)
 {
 	struct property_entry *p;
-	int i, n = 0;
+	int i, n;
 	int ret;
 
-	if (!properties)
+	n = property_entries_count(properties);
+	if (n == 0)
 		return NULL;
 
-	while (properties[n].name)
-		n++;
-
 	p = kcalloc(n + 1, sizeof(*p), GFP_KERNEL);
 	if (!p)
 		return ERR_PTR(-ENOMEM);
@@ -746,6 +762,108 @@ static void software_node_free(const struct software_node *node)
 	kfree(node);
 }
 
+static ssize_t
+swnode_property_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct swnode *swnode = kobj_to_swnode(kobj);
+	const struct property_entry *prop;
+	const void *pointer;
+	ssize_t len = 0;
+	int i;
+
+	prop = property_entry_get(swnode->node->properties, attr->attr.name);
+	if (!prop)
+		return -EINVAL;
+
+	/* We can't fail here, because it means boolean property */
+	pointer = property_get_pointer(prop);
+	if (!pointer)
+		return sysfs_emit(buf, "\n");
+
+	switch (prop->type) {
+	case DEV_PROP_U8:
+		for (i = 0; i < prop->length / sizeof(u8); i++)
+			len += sysfs_emit_at(buf, len, "%u,", ((u8 *)pointer)[i]);
+		break;
+	case DEV_PROP_U16:
+		for (i = 0; i < prop->length / sizeof(u16); i++)
+			len += sysfs_emit_at(buf, len, "%u,", ((u16 *)pointer)[i]);
+		break;
+	case DEV_PROP_U32:
+		for (i = 0; i < prop->length / sizeof(u32); i++)
+			len += sysfs_emit_at(buf, len, "%u,", ((u32 *)pointer)[i]);
+		break;
+	case DEV_PROP_U64:
+		for (i = 0; i < prop->length / sizeof(u64); i++)
+			len += sysfs_emit_at(buf, len, "%llu,", ((u64 *)pointer)[i]);
+		break;
+	case DEV_PROP_STRING:
+		for (i = 0; i < prop->length / sizeof(const char **); i++)
+			len += sysfs_emit_at(buf, len, "\"%s\",", ((const char **)pointer)[i]);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	sysfs_emit_at(buf, len ? len - 1 : 0, "\n");
+	return len;
+}
+
+static int swnode_register_properties(struct swnode *swnode)
+{
+	const struct property_entry *props = swnode->node->properties;
+	struct attribute **group_attrs;
+	struct kobj_attribute *attrs;
+	int n;
+	int ret;
+
+	n = property_entries_count(props);
+	if (n == 0)
+		return 0;
+
+	group_attrs = kcalloc(n + 1, sizeof(*group_attrs), GFP_KERNEL);
+	if (!group_attrs)
+		return -ENOMEM;
+
+	attrs = kcalloc(n, sizeof(*attrs), GFP_KERNEL);
+	if (!attrs) {
+		kfree(group_attrs);
+		return -ENOMEM;
+	}
+
+	while (n--) {
+		sysfs_attr_init(&attrs[n]);
+		attrs[n].attr.mode = 0444;
+		attrs[n].attr.name = props[n].name;
+		attrs[n].show = swnode_property_show;
+		group_attrs[n] = &attrs[n].attr;
+	}
+
+	swnode->property_group.name = "properties";
+	swnode->property_group.attrs = group_attrs;
+
+	ret = sysfs_create_group(&swnode->kobj, &swnode->property_group);
+	if (ret) {
+		kfree(attrs);
+		kfree(group_attrs);
+		return ret;
+	}
+
+	swnode->property_attrs = attrs;
+	swnode->properties = true;
+	return 0;
+}
+
+static void swnode_unregister_properties(struct swnode *swnode)
+{
+	/*
+	 * Nodes under sysfs are removed by __kobject_del(),
+	 * we don't need to call sysfs_remove_group() explicitly.
+	 */
+	kfree(swnode->property_group.attrs);
+	kfree(swnode->property_attrs);
+}
+
 static void software_node_release(struct kobject *kobj)
 {
 	struct swnode *swnode = kobj_to_swnode(kobj);
@@ -757,6 +875,9 @@ static void software_node_release(struct kobject *kobj)
 		ida_simple_remove(&swnode_root_ids, swnode->id);
 	}
 
+	if (swnode->properties)
+		swnode_unregister_properties(swnode);
+
 	if (swnode->allocated)
 		software_node_free(swnode->node);
 
@@ -810,6 +931,12 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 		return ERR_PTR(ret);
 	}
 
+	ret = swnode_register_properties(swnode);
+	if (ret) {
+		kobject_put(&swnode->kobj);
+		return ERR_PTR(ret);
+	}
+
 	/*
 	 * Assign the flag only in the successful case, so
 	 * the above kobject_put() won't mess up with properties.
-- 
2.30.2

[PATCH v1 4/8] software node: Deduplicate code in fwnode_create_software_node()

[Andy Shevchenko]

Deduplicate conditional and assignment in fwnode_create_software_node(),
i.e. parent is checked in two out of three cases and parent software node
is assigned by to_swnode() call.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/swnode.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index d7fe1a887d2d..22f81688af2c 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -1100,15 +1100,14 @@ fwnode_create_software_node(const struct property_entry *properties,
 {
 	struct fwnode_handle *fwnode;
 	struct software_node *node;
-	struct swnode *p = NULL;
-
-	if (parent) {
-		if (IS_ERR(parent))
-			return ERR_CAST(parent);
-		if (!is_software_node(parent))
-			return ERR_PTR(-EINVAL);
-		p = to_swnode(parent);
-	}
+	struct swnode *p;
+
+	if (IS_ERR(parent))
+		return ERR_CAST(parent);
+
+	p = to_swnode(parent);
+	if (parent && !p)
+		return ERR_PTR(-EINVAL);
 
 	node = software_node_alloc(properties);
 	if (IS_ERR(node))
-- 
2.30.2

[PATCH v1 5/8] software node: Imply kobj_to_swnode() to be no-op

[Andy Shevchenko]

Since we don't use structure field layout randomization
the manual shuffling can affect some macros, in particular
kobj_to_swnode(), which becomes a no-op when kobj member
is the first one in the struct swnode.

Bloat-o-meter statistics:

  add/remove: 0/0 grow/shrink: 2/10 up/down: 9/-100 (-91)
  Total: Before=7217, After=7126, chg -1.26%

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/swnode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 22f81688af2c..ae53c48f84b1 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -13,10 +13,10 @@
 #include <linux/sysfs.h>
 
 struct swnode {
-	int id;
 	struct kobject kobj;
 	struct fwnode_handle fwnode;
 	const struct software_node *node;
+	int id;
 
 	/* properties in sysfs */
 	struct kobj_attribute *property_attrs;
-- 
2.30.2

[PATCH v1 6/8] software node: Simplify swnode_register() a bit

[Andy Shevchenko]

By introducing two temporary variables simplify swnode_register() a bit.
No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/swnode.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index ae53c48f84b1..1e81aaf5f6a1 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -894,6 +894,8 @@ static struct fwnode_handle *
 swnode_register(const struct software_node *node, struct swnode *parent,
 		unsigned int allocated)
 {
+	struct ida *ids = parent ? &parent->child_ids : &swnode_root_ids;
+	struct kobject *kobj_parent = parent ? &parent->kobj : NULL;
 	struct swnode *swnode;
 	int ret;
 
@@ -901,8 +903,7 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 	if (!swnode)
 		return ERR_PTR(-ENOMEM);
 
-	ret = ida_simple_get(parent ? &parent->child_ids : &swnode_root_ids,
-			     0, 0, GFP_KERNEL);
+	ret = ida_simple_get(ids, 0, 0, GFP_KERNEL);
 	if (ret < 0) {
 		kfree(swnode);
 		return ERR_PTR(ret);
@@ -920,12 +921,10 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 
 	if (node->name)
 		ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
-					   parent ? &parent->kobj : NULL,
-					   "%s", node->name);
+					   kobj_parent, "%s", node->name);
 	else
 		ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
-					   parent ? &parent->kobj : NULL,
-					   "node%d", swnode->id);
+					   kobj_parent, "node%d", swnode->id);
 	if (ret) {
 		kobject_put(&swnode->kobj);
 		return ERR_PTR(ret);
-- 
2.30.2

[PATCH v1 7/8] software node: Introduce SOFTWARE_NODE_REFERENCE() helper macro

[Andy Shevchenko]

This is useful to assign software node reference with arguments
in a common way. Moreover, we have already couple of users that
may be converted. And by the fact, one of them is moved right here
to use the helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/test/property-entry-test.c | 11 ++---------
 include/linux/property.h                | 13 ++++++++-----
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/base/test/property-entry-test.c b/drivers/base/test/property-entry-test.c
index abe03315180f..c2e455d46ffd 100644
--- a/drivers/base/test/property-entry-test.c
+++ b/drivers/base/test/property-entry-test.c
@@ -370,15 +370,8 @@ static void pe_test_reference(struct kunit *test)
 	};
 
 	static const struct software_node_ref_args refs[] = {
-		{
-			.node = &nodes[0],
-			.nargs = 0,
-		},
-		{
-			.node = &nodes[1],
-			.nargs = 2,
-			.args = { 3, 4 },
-		},
+		SOFTWARE_NODE_REFERENCE(&nodes[0]),
+		SOFTWARE_NODE_REFERENCE(&nodes[1], 3, 4),
 	};
 
 	const struct property_entry entries[] = {
diff --git a/include/linux/property.h b/include/linux/property.h
index dd4687b56239..0d876316e61d 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -254,6 +254,13 @@ struct software_node_ref_args {
 	u64 args[NR_FWNODE_REFERENCE_ARGS];
 };
 
+#define SOFTWARE_NODE_REFERENCE(_ref_, ...)			\
+(const struct software_node_ref_args) {				\
+	.node = _ref_,						\
+	.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
+	.args = { __VA_ARGS__ },				\
+}
+
 /**
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
@@ -362,11 +369,7 @@ struct property_entry {
 	.name = _name_,							\
 	.length = sizeof(struct software_node_ref_args),		\
 	.type = DEV_PROP_REF,						\
-	{ .pointer = &(const struct software_node_ref_args) {		\
-		.node = _ref_,						\
-		.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
-		.args = { __VA_ARGS__ },				\
-	} },								\
+	{ .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), },	\
 }
 
 struct property_entry *
-- 
2.30.2

[PATCH v1 8/8] media: ipu3-cio2: Switch to use SOFTWARE_NODE_REFERENCE()

[Andy Shevchenko]

This is useful to assign software node reference with arguments
in a common way. Switch to use SOFTWARE_NODE_REFERENCE() here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/pci/intel/ipu3/cio2-bridge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index c2199042d3db..e8511787c1e4 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -79,8 +79,8 @@ static void cio2_bridge_create_fwnode_properties(
 {
 	sensor->prop_names = prop_names;
 
-	sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
-	sensor->remote_ref[0].node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT];
+	sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_CIO2_ENDPOINT]);
+	sensor->remote_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_SENSOR_ENDPOINT]);
 
 	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(
 					sensor->prop_names.clock_frequency,
-- 
2.30.2

Re: [PATCH v1 3/8] software node: Show properties and their values in sysfs

[kernel test robot]

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

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on linuxtv-media/master linux/master linus/master v5.12-rc4 next-20210326]
[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/Andy-Shevchenko/software-node-Free-resources-explicitly-when-swnode_register-fails/20210328-062322
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git ecdc996baf291b903342cc704f4086a88c361967
config: microblaze-randconfig-r016-20210328 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 9.3.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/1d48129700f39fc70d26e5faee27e6fd7d8d5234
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/software-node-Free-resources-explicitly-when-swnode_register-fails/20210328-062322
        git checkout 1d48129700f39fc70d26e5faee27e6fd7d8d5234
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=microblaze 

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 >>):

   In file included from include/linux/kobject.h:20,
                    from include/linux/energy_model.h:7,
                    from include/linux/device.h:16,
                    from drivers/base/swnode.c:9:
   drivers/base/swnode.c: In function 'swnode_register_properties':
>> include/linux/sysfs.h:55:8: error: 'struct kobj_attribute' has no member named 'key'
      55 |  (attr)->key = &__key;    \
         |        ^~
   drivers/base/swnode.c:835:3: note: in expansion of macro 'sysfs_attr_init'
     835 |   sysfs_attr_init(&attrs[n]);
         |   ^~~~~~~~~~~~~~~


vim +55 include/linux/sysfs.h

^1da177e4c3f41 Linus Torvalds    2005-04-16  39  
35960258ed388c Eric W. Biederman 2010-02-12  40  /**
35960258ed388c Eric W. Biederman 2010-02-12  41   *	sysfs_attr_init - initialize a dynamically allocated sysfs attribute
35960258ed388c Eric W. Biederman 2010-02-12  42   *	@attr: struct attribute to initialize
35960258ed388c Eric W. Biederman 2010-02-12  43   *
35960258ed388c Eric W. Biederman 2010-02-12  44   *	Initialize a dynamically allocated struct attribute so we can
35960258ed388c Eric W. Biederman 2010-02-12  45   *	make lockdep happy.  This is a new requirement for attributes
35960258ed388c Eric W. Biederman 2010-02-12  46   *	and initially this is only needed when lockdep is enabled.
35960258ed388c Eric W. Biederman 2010-02-12  47   *	Lockdep gives a nice error when your attribute is added to
35960258ed388c Eric W. Biederman 2010-02-12  48   *	sysfs if you don't have this.
35960258ed388c Eric W. Biederman 2010-02-12  49   */
6992f5334995af Eric W. Biederman 2010-02-11  50  #ifdef CONFIG_DEBUG_LOCK_ALLOC
6992f5334995af Eric W. Biederman 2010-02-11  51  #define sysfs_attr_init(attr)				\
6992f5334995af Eric W. Biederman 2010-02-11  52  do {							\
6992f5334995af Eric W. Biederman 2010-02-11  53  	static struct lock_class_key __key;		\
6992f5334995af Eric W. Biederman 2010-02-11  54  							\
6992f5334995af Eric W. Biederman 2010-02-11 @55  	(attr)->key = &__key;				\
6992f5334995af Eric W. Biederman 2010-02-11  56  } while (0)
6992f5334995af Eric W. Biederman 2010-02-11  57  #else
6992f5334995af Eric W. Biederman 2010-02-11  58  #define sysfs_attr_init(attr) do {} while (0)
6992f5334995af Eric W. Biederman 2010-02-11  59  #endif
6992f5334995af Eric W. Biederman 2010-02-11  60  

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27999 bytes --]

Re: [PATCH v1 3/8] software node: Show properties and their values in sysfs

[Greg Kroah-Hartman]

On Sun, Mar 28, 2021 at 12:20:07AM +0200, Andy Shevchenko wrote:
> It's very convenient to see what properties and their values
> are currently being assigned in the registered software nodes.
> 
> Show properties and their values in sysfs.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/swnode.c | 137 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 132 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 19aa44bc2628..d7fe1a887d2d 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -10,6 +10,7 @@
>  #include <linux/kernel.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> +#include <linux/sysfs.h>
>  
>  struct swnode {
>  	int id;
> @@ -17,6 +18,10 @@ struct swnode {
>  	struct fwnode_handle fwnode;
>  	const struct software_node *node;
>  
> +	/* properties in sysfs */
> +	struct kobj_attribute *property_attrs;
> +	struct attribute_group property_group;
> +
>  	/* hierarchy */
>  	struct ida child_ids;
>  	struct list_head entry;
> @@ -25,6 +30,7 @@ struct swnode {
>  
>  	unsigned int allocated:1;
>  	unsigned int managed:1;
> +	unsigned int properties:1;
>  };
>  
>  static DEFINE_IDA(swnode_root_ids);
> @@ -299,6 +305,18 @@ static int property_entry_copy_data(struct property_entry *dst,
>  	return 0;
>  }
>  
> +static int property_entries_count(const struct property_entry *properties)
> +{
> +	int n = 0;
> +
> +	if (properties) {
> +		while (properties[n].name)
> +			n++;
> +	}
> +
> +	return n;
> +}
> +
>  /**
>   * property_entries_dup - duplicate array of properties
>   * @properties: array of properties to copy
> @@ -310,15 +328,13 @@ struct property_entry *
>  property_entries_dup(const struct property_entry *properties)
>  {
>  	struct property_entry *p;
> -	int i, n = 0;
> +	int i, n;
>  	int ret;
>  
> -	if (!properties)
> +	n = property_entries_count(properties);
> +	if (n == 0)
>  		return NULL;
>  
> -	while (properties[n].name)
> -		n++;
> -
>  	p = kcalloc(n + 1, sizeof(*p), GFP_KERNEL);
>  	if (!p)
>  		return ERR_PTR(-ENOMEM);
> @@ -746,6 +762,108 @@ static void software_node_free(const struct software_node *node)
>  	kfree(node);
>  }
>  
> +static ssize_t
> +swnode_property_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct swnode *swnode = kobj_to_swnode(kobj);
> +	const struct property_entry *prop;
> +	const void *pointer;
> +	ssize_t len = 0;
> +	int i;
> +
> +	prop = property_entry_get(swnode->node->properties, attr->attr.name);
> +	if (!prop)
> +		return -EINVAL;
> +
> +	/* We can't fail here, because it means boolean property */
> +	pointer = property_get_pointer(prop);
> +	if (!pointer)
> +		return sysfs_emit(buf, "\n");
> +
> +	switch (prop->type) {
> +	case DEV_PROP_U8:
> +		for (i = 0; i < prop->length / sizeof(u8); i++)
> +			len += sysfs_emit_at(buf, len, "%u,", ((u8 *)pointer)[i]);

No, sysfs is "one value per file", and that is not what you are showing
here at all :(

Also, there is no Documentation/ABI/ entries for your new sysfs files,
so that means we couldn't take this patcheset anyway :(

greg k-h

Re: [PATCH v1 5/8] software node: Imply kobj_to_swnode() to be no-op

[Greg Kroah-Hartman]

On Sun, Mar 28, 2021 at 12:20:09AM +0200, Andy Shevchenko wrote:
> Since we don't use structure field layout randomization
> the manual shuffling can affect some macros, in particular
> kobj_to_swnode(), which becomes a no-op when kobj member
> is the first one in the struct swnode.
> 
> Bloat-o-meter statistics:
> 
>   add/remove: 0/0 grow/shrink: 2/10 up/down: 9/-100 (-91)
>   Total: Before=7217, After=7126, chg -1.26%
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/swnode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 22f81688af2c..ae53c48f84b1 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -13,10 +13,10 @@
>  #include <linux/sysfs.h>
>  
>  struct swnode {
> -	int id;
>  	struct kobject kobj;
>  	struct fwnode_handle fwnode;
>  	const struct software_node *node;
> +	int id;

So you remove one math operation on a pointer and get a 1% size decrease
of the whole kernel?  Or just one file?

thanks,

greg k-h

Re: [PATCH v1 6/8] software node: Simplify swnode_register() a bit

[Greg Kroah-Hartman]

On Sun, Mar 28, 2021 at 12:20:10AM +0200, Andy Shevchenko wrote:
> By introducing two temporary variables simplify swnode_register() a bit.
> No functional change intended.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/swnode.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index ae53c48f84b1..1e81aaf5f6a1 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -894,6 +894,8 @@ static struct fwnode_handle *
>  swnode_register(const struct software_node *node, struct swnode *parent,
>  		unsigned int allocated)
>  {
> +	struct ida *ids = parent ? &parent->child_ids : &swnode_root_ids;
> +	struct kobject *kobj_parent = parent ? &parent->kobj : NULL;

?: operations are horrid.  Please spell this out in real if statements
so that we can properly understand and maintain them for the next 20+
years.

thanks,

greg k-h

Re: [PATCH v1 5/8] software node: Imply kobj_to_swnode() to be no-op

[Andy Shevchenko]

On Sun, Mar 28, 2021 at 11:47 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Mar 28, 2021 at 12:20:09AM +0200, Andy Shevchenko wrote:
> > Since we don't use structure field layout randomization
> > the manual shuffling can affect some macros, in particular
> > kobj_to_swnode(), which becomes a no-op when kobj member
> > is the first one in the struct swnode.
> >
> > Bloat-o-meter statistics:
> >
> >   add/remove: 0/0 grow/shrink: 2/10 up/down: 9/-100 (-91)
> >   Total: Before=7217, After=7126, chg -1.26%
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/base/swnode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > index 22f81688af2c..ae53c48f84b1 100644
> > --- a/drivers/base/swnode.c
> > +++ b/drivers/base/swnode.c
> > @@ -13,10 +13,10 @@
> >  #include <linux/sysfs.h>
> >
> >  struct swnode {
> > -     int id;
> >       struct kobject kobj;
> >       struct fwnode_handle fwnode;
> >       const struct software_node *node;
> > +     int id;
>
> So you remove one math operation on a pointer and get a 1% size decrease
> of the whole kernel?  Or just one file?

One file, swnode.o. I'll clarify this in the commit message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 6/8] software node: Simplify swnode_register() a bit

[Andy Shevchenko]

On Sun, Mar 28, 2021 at 11:48 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Mar 28, 2021 at 12:20:10AM +0200, Andy Shevchenko wrote:
> > By introducing two temporary variables simplify swnode_register() a bit.
> > No functional change intended.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/base/swnode.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > index ae53c48f84b1..1e81aaf5f6a1 100644
> > --- a/drivers/base/swnode.c
> > +++ b/drivers/base/swnode.c
> > @@ -894,6 +894,8 @@ static struct fwnode_handle *
> >  swnode_register(const struct software_node *node, struct swnode *parent,
> >               unsigned int allocated)
> >  {
> > +     struct ida *ids = parent ? &parent->child_ids : &swnode_root_ids;
> > +     struct kobject *kobj_parent = parent ? &parent->kobj : NULL;
>
> ?: operations are horrid.  Please spell this out in real if statements
> so that we can properly understand and maintain them for the next 20+
> years.

Will do, thanks!

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/8] software node: Show properties and their values in sysfs

[Andy Shevchenko]

On Sun, Mar 28, 2021 at 9:47 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Mar 28, 2021 at 12:20:07AM +0200, Andy Shevchenko wrote:
> > It's very convenient to see what properties and their values
> > are currently being assigned in the registered software nodes.
> >
> > Show properties and their values in sysfs.

...

> > +             for (i = 0; i < prop->length / sizeof(u8); i++)
> > +                     len += sysfs_emit_at(buf, len, "%u,", ((u8 *)pointer)[i]);
>
> No, sysfs is "one value per file", and that is not what you are showing
> here at all :(

It is following: it's a "one value" for property in question,

As we may read in [1]: "...so it is socially acceptable to express an
array of values of the same type."

And here is exactly the case: *values of the same type*.

> Also, there is no Documentation/ABI/ entries for your new sysfs files,
> so that means we couldn't take this patcheset anyway :(

True, I'll fix this, thanks!

[1]: https://www.kernel.org/doc/html/latest/filesystems/sysfs.html

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/8] software node: Show properties and their values in sysfs

[Greg Kroah-Hartman]

On Sun, Mar 28, 2021 at 03:56:26PM +0300, Andy Shevchenko wrote:
> On Sun, Mar 28, 2021 at 9:47 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Mar 28, 2021 at 12:20:07AM +0200, Andy Shevchenko wrote:
> > > It's very convenient to see what properties and their values
> > > are currently being assigned in the registered software nodes.
> > >
> > > Show properties and their values in sysfs.
> 
> ...
> 
> > > +             for (i = 0; i < prop->length / sizeof(u8); i++)
> > > +                     len += sysfs_emit_at(buf, len, "%u,", ((u8 *)pointer)[i]);
> >
> > No, sysfs is "one value per file", and that is not what you are showing
> > here at all :(
> 
> It is following: it's a "one value" for property in question,
> 
> As we may read in [1]: "...so it is socially acceptable to express an
> array of values of the same type."
> 
> And here is exactly the case: *values of the same type*.

So what is it going to look like exactly?  And what tool is going to be
there to parse this mess?  Who is going to to use it?

Arrays of data are not "one value".

thanks,

greg k-h

Re: [PATCH v1 3/8] software node: Show properties and their values in sysfs

[Andy Shevchenko]

On Sun, Mar 28, 2021 at 03:02:24PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Mar 28, 2021 at 03:56:26PM +0300, Andy Shevchenko wrote:
> > On Sun, Mar 28, 2021 at 9:47 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Mar 28, 2021 at 12:20:07AM +0200, Andy Shevchenko wrote:
> > > > It's very convenient to see what properties and their values
> > > > are currently being assigned in the registered software nodes.
> > > >
> > > > Show properties and their values in sysfs.
> > 
> > ...
> > 
> > > > +             for (i = 0; i < prop->length / sizeof(u8); i++)
> > > > +                     len += sysfs_emit_at(buf, len, "%u,", ((u8 *)pointer)[i]);
> > >
> > > No, sysfs is "one value per file", and that is not what you are showing
> > > here at all :(
> > 
> > It is following: it's a "one value" for property in question,
> > 
> > As we may read in [1]: "...so it is socially acceptable to express an
> > array of values of the same type."
> > 
> > And here is exactly the case: *values of the same type*.
> 
> So what is it going to look like exactly?

Basically we have two approaches (already done in the kernel!) use space or
comma for a separator. So:
 - for boolean it will be an empty string (and it's one value always)
 - for integers it will be, for example, '0,1,2' (w/o single quotes)
   for property array with values 0, 1, and 2
 - for plain integers or arrays out of 1 element it will be plain integer
 - for strings it will be, for example, '"str1","str2"' (w/o single quotes)
   for array of string { "str1", "str2" }
 - for single string or array out of 1 element, it will be '"str"' (w/o single
   quotes)

This should be a part of documentation.

> And what tool is going to be
> there to parse this mess?  Who is going to to use it?

I guess something like hwinfo (needs a patch).

The idea behind that this is following what ACPI and DT provides to the users
via /sys/firmware/ (however, in binary format). I can re-do to provide a
binary, and it will effectively make software nodes in align with the rest.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/8] software node: Show properties and their values in sysfs

[Greg Kroah-Hartman]

On Mon, Mar 29, 2021 at 04:01:18PM +0300, Andy Shevchenko wrote:
> On Sun, Mar 28, 2021 at 03:02:24PM +0200, Greg Kroah-Hartman wrote:
> > On Sun, Mar 28, 2021 at 03:56:26PM +0300, Andy Shevchenko wrote:
> > > On Sun, Mar 28, 2021 at 9:47 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Sun, Mar 28, 2021 at 12:20:07AM +0200, Andy Shevchenko wrote:
> > > > > It's very convenient to see what properties and their values
> > > > > are currently being assigned in the registered software nodes.
> > > > >
> > > > > Show properties and their values in sysfs.
> > > 
> > > ...
> > > 
> > > > > +             for (i = 0; i < prop->length / sizeof(u8); i++)
> > > > > +                     len += sysfs_emit_at(buf, len, "%u,", ((u8 *)pointer)[i]);
> > > >
> > > > No, sysfs is "one value per file", and that is not what you are showing
> > > > here at all :(
> > > 
> > > It is following: it's a "one value" for property in question,
> > > 
> > > As we may read in [1]: "...so it is socially acceptable to express an
> > > array of values of the same type."
> > > 
> > > And here is exactly the case: *values of the same type*.
> > 
> > So what is it going to look like exactly?
> 
> Basically we have two approaches (already done in the kernel!) use space or
> comma for a separator. So:
>  - for boolean it will be an empty string (and it's one value always)
>  - for integers it will be, for example, '0,1,2' (w/o single quotes)
>    for property array with values 0, 1, and 2
>  - for plain integers or arrays out of 1 element it will be plain integer
>  - for strings it will be, for example, '"str1","str2"' (w/o single quotes)
>    for array of string { "str1", "str2" }
>  - for single string or array out of 1 element, it will be '"str"' (w/o single
>    quotes)
> 
> This should be a part of documentation.

And I will complain then too, these "lists of values" are not for sysfs,
sorry.

> > And what tool is going to be
> > there to parse this mess?  Who is going to to use it?
> 
> I guess something like hwinfo (needs a patch).

If nothing needs this, then why are you adding these?

> The idea behind that this is following what ACPI and DT provides to the users
> via /sys/firmware/ (however, in binary format). I can re-do to provide a
> binary, and it will effectively make software nodes in align with the rest.

binary files in sysfs are only to be used as a "pass through" from
hardware to userspace.  That does not seem relevant here.

sorry, please keep this out of sysfs for now.

greg k-h

Re: [PATCH v1 3/8] software node: Show properties and their values in sysfs

[Andy Shevchenko]

On Mon, Mar 29, 2021 at 4:46 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Mar 29, 2021 at 04:01:18PM +0300, Andy Shevchenko wrote:
> > On Sun, Mar 28, 2021 at 03:02:24PM +0200, Greg Kroah-Hartman wrote:
> > > On Sun, Mar 28, 2021 at 03:56:26PM +0300, Andy Shevchenko wrote:
> > > > On Sun, Mar 28, 2021 at 9:47 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > > On Sun, Mar 28, 2021 at 12:20:07AM +0200, Andy Shevchenko wrote:
> > > > > > It's very convenient to see what properties and their values
> > > > > > are currently being assigned in the registered software nodes.

...

> > > > > > +             for (i = 0; i < prop->length / sizeof(u8); i++)
> > > > > > +                     len += sysfs_emit_at(buf, len, "%u,", ((u8 *)pointer)[i]);
> > > > >
> > > > > No, sysfs is "one value per file", and that is not what you are showing
> > > > > here at all :(
> > > >
> > > > It is following: it's a "one value" for property in question,
> > > >
> > > > As we may read in [1]: "...so it is socially acceptable to express an
> > > > array of values of the same type."
> > > >
> > > > And here is exactly the case: *values of the same type*.
> > >
> > > So what is it going to look like exactly?
> >
> > Basically we have two approaches (already done in the kernel!) use space or
> > comma for a separator. So:
> >  - for boolean it will be an empty string (and it's one value always)
> >  - for integers it will be, for example, '0,1,2' (w/o single quotes)
> >    for property array with values 0, 1, and 2
> >  - for plain integers or arrays out of 1 element it will be plain integer
> >  - for strings it will be, for example, '"str1","str2"' (w/o single quotes)
> >    for array of string { "str1", "str2" }
> >  - for single string or array out of 1 element, it will be '"str"' (w/o single
> >    quotes)
> >
> > This should be a part of documentation.
>
> And I will complain then too, these "lists of values" are not for sysfs,
> sorry.

How can you recommend showing an array(s) of values of the same type?

> > > And what tool is going to be
> > > there to parse this mess?  Who is going to to use it?
> >
> > I guess something like hwinfo (needs a patch).
>
> If nothing needs this, then why are you adding these?

OK, I will prepare tools first, but I have no idea for the format. So,
how can I be sure that tools will accept the patch if there is no
kernel interface available? Seems like a stale case.

> > The idea behind that this is following what ACPI and DT provides to the users
> > via /sys/firmware/ (however, in binary format). I can re-do to provide a
> > binary, and it will effectively make software nodes in align with the rest.
>
> binary files in sysfs are only to be used as a "pass through" from
> hardware to userspace.  That does not seem relevant here.

This makes sense.

> sorry, please keep this out of sysfs for now.

OK. For now I will resubmit the rest with addressed comments.
Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/8] software node: Show properties and their values in sysfs

[Greg Kroah-Hartman]

On Mon, Mar 29, 2021 at 05:51:01PM +0300, Andy Shevchenko wrote:
> On Mon, Mar 29, 2021 at 4:46 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Mar 29, 2021 at 04:01:18PM +0300, Andy Shevchenko wrote:
> > > On Sun, Mar 28, 2021 at 03:02:24PM +0200, Greg Kroah-Hartman wrote:
> > > > On Sun, Mar 28, 2021 at 03:56:26PM +0300, Andy Shevchenko wrote:
> > > > > On Sun, Mar 28, 2021 at 9:47 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > On Sun, Mar 28, 2021 at 12:20:07AM +0200, Andy Shevchenko wrote:
> > > > > > > It's very convenient to see what properties and their values
> > > > > > > are currently being assigned in the registered software nodes.
> 
> ...
> 
> > > > > > > +             for (i = 0; i < prop->length / sizeof(u8); i++)
> > > > > > > +                     len += sysfs_emit_at(buf, len, "%u,", ((u8 *)pointer)[i]);
> > > > > >
> > > > > > No, sysfs is "one value per file", and that is not what you are showing
> > > > > > here at all :(
> > > > >
> > > > > It is following: it's a "one value" for property in question,
> > > > >
> > > > > As we may read in [1]: "...so it is socially acceptable to express an
> > > > > array of values of the same type."
> > > > >
> > > > > And here is exactly the case: *values of the same type*.
> > > >
> > > > So what is it going to look like exactly?
> > >
> > > Basically we have two approaches (already done in the kernel!) use space or
> > > comma for a separator. So:
> > >  - for boolean it will be an empty string (and it's one value always)
> > >  - for integers it will be, for example, '0,1,2' (w/o single quotes)
> > >    for property array with values 0, 1, and 2
> > >  - for plain integers or arrays out of 1 element it will be plain integer
> > >  - for strings it will be, for example, '"str1","str2"' (w/o single quotes)
> > >    for array of string { "str1", "str2" }
> > >  - for single string or array out of 1 element, it will be '"str"' (w/o single
> > >    quotes)
> > >
> > > This should be a part of documentation.
> >
> > And I will complain then too, these "lists of values" are not for sysfs,
> > sorry.
> 
> How can you recommend showing an array(s) of values of the same type?

I do not think that is something that should be shown in sysfs if at all
possible.

> > > > And what tool is going to be
> > > > there to parse this mess?  Who is going to to use it?
> > >
> > > I guess something like hwinfo (needs a patch).
> >
> > If nothing needs this, then why are you adding these?
> 
> OK, I will prepare tools first, but I have no idea for the format. So,
> how can I be sure that tools will accept the patch if there is no
> kernel interface available? Seems like a stale case.

Develop both together.  Do not think that you will get the kernel format
right without actually knowing what userspace needs/wants.  That is
crazy.

thanks,

greg k-h