[PATCH v6 00/15] software node: add support for reference properties

[PATCH v6 00/15] software node: add support for reference properties

[Dmitry Torokhov]

These series implement "references" properties for software nodes as true
properties, instead of managing them completely separately.

The first 10 patches are generic cleanups and consolidation and
unification of the existing code; patch #11 implements moving of small
properties inline when copying property entries; patch #12 implements
PROPERTY_ENTRY_REF() and friends; patch #13 converts the user of
references to the property syntax, and patch #14 removes the remains of
references as entities that are managed separately.

Patch #15 adds unit tests to verify that the handling of property
entries is correct.

Changes in v6:
- rebased onto next-20191023
- fixed patch moving small properties inline
- fixed handling boolean properties after is_array -> is_inline
  conversion
- changed comments around is_inline "stored directly" vs embedded
  in one place (Andy)
- added unit tests for property entries based on KUnit framework
- added Any's reviewed-by/acked-by

Changes in v5:
- rebased onto next-20191011

Changes in v4:
- dealt with union aliasing concerns
- inline small properties on copy

Changes in v3:
- added various cleanups before implementing reference properties

Changes in v2:
- reworked code so that even single-entry reference properties are
  stored as arrays (i.e. the software_node_ref_args instances are
  not part of property_entry structure) to avoid size increase.
  From user's POV nothing is changed, one can still use PROPERTY_ENTRY_REF
  macro to define reference "inline".
- dropped unused DEV_PROP_MAX
- rebased on linux-next

Dmitry Torokhov (15):
  software node: remove DEV_PROP_MAX
  software node: introduce PROPERTY_ENTRY_ARRAY_XXX_LEN()
  efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN
  software node: mark internal macros with double underscores
  software node: clean up property_copy_string_array()
  software node: get rid of property_set_pointer()
  software node: remove property_entry_read_uNN_array functions
  software node: unify PROPERTY_ENTRY_XXX macros
  software node: simplify property_entry_read_string_array()
  software node: rename is_array to is_inline
  software node: move small properties inline when copying
  software node: implement reference properties
  platform/x86: intel_cht_int33fe: use inline reference properties
  software node: remove separate handling of references
  software node: add basic tests for property entries

 drivers/base/swnode.c                         | 263 ++++------
 drivers/base/test/Makefile                    |   2 +
 drivers/base/test/property-entry-test.c       | 472 ++++++++++++++++++
 drivers/firmware/efi/apple-properties.c       |  18 +-
 .../platform/x86/intel_cht_int33fe_typec.c    |  81 +--
 include/linux/property.h                      | 178 +++----
 6 files changed, 702 insertions(+), 312 deletions(-)
 create mode 100644 drivers/base/test/property-entry-test.c

-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 01/15] software node: remove DEV_PROP_MAX

[Dmitry Torokhov]

This definition is not used anywhere, let's remove it.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 include/linux/property.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/property.h b/include/linux/property.h
index 054661109661..0a075fbde57e 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -22,7 +22,6 @@ enum dev_prop_type {
 	DEV_PROP_U32,
 	DEV_PROP_U64,
 	DEV_PROP_STRING,
-	DEV_PROP_MAX,
 };
 
 enum dev_dma_attr {
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 02/15] software node: introduce PROPERTY_ENTRY_ARRAY_XXX_LEN()

[Dmitry Torokhov]

Sometimes we want to initialize property entry array from a regular
pointer, when we can't determine length automatically via ARRAY_SIZE.
Let's introduce PROPERTY_ENTRY_ARRAY_XXX_LEN macros that take explicit
"len" argument.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 include/linux/property.h | 45 +++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/include/linux/property.h b/include/linux/property.h
index 0a075fbde57e..fad2e83a207c 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -261,33 +261,44 @@ struct property_entry {
  * and structs.
  */
 
-#define PROPERTY_ENTRY_INTEGER_ARRAY(_name_, _type_, _Type_, _val_)	\
+#define PROPERTY_ENTRY_ARRAY_LEN(_name_, _type_, _Type_, _val_, _len_)	\
 (struct property_entry) {						\
 	.name = _name_,							\
-	.length = ARRAY_SIZE(_val_) * sizeof(_type_),			\
+	.length = (_len_) * sizeof(_type_),				\
 	.is_array = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .pointer = { ._type_##_data = _val_ } },			\
 }
 
-#define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)			\
-	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u8, U8, _val_)
-#define PROPERTY_ENTRY_U16_ARRAY(_name_, _val_)			\
-	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u16, U16, _val_)
-#define PROPERTY_ENTRY_U32_ARRAY(_name_, _val_)			\
-	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u32, U32, _val_)
-#define PROPERTY_ENTRY_U64_ARRAY(_name_, _val_)			\
-	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u64, U64, _val_)
+#define PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, _len_)		\
+	PROPERTY_ENTRY_ARRAY_LEN(_name_, u8, U8, _val_, _len_)
+#define PROPERTY_ENTRY_U16_ARRAY_LEN(_name_, _val_, _len_)		\
+	PROPERTY_ENTRY_ARRAY_LEN(_name_, u16, U16, _val_, _len_)
+#define PROPERTY_ENTRY_U32_ARRAY_LEN(_name_, _val_, _len_)		\
+	PROPERTY_ENTRY_ARRAY_LEN(_name_, u32, U32, _val_, _len_)
+#define PROPERTY_ENTRY_U64_ARRAY_LEN(_name_, _val_, _len_)		\
+	PROPERTY_ENTRY_ARRAY_LEN(_name_, u64, U64, _val_, _len_)
 
-#define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_)		\
-(struct property_entry) {					\
-	.name = _name_,						\
-	.length = ARRAY_SIZE(_val_) * sizeof(const char *),	\
-	.is_array = true,					\
-	.type = DEV_PROP_STRING,				\
-	{ .pointer = { .str = _val_ } },			\
+#define PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, _len_)		\
+(struct property_entry) {						\
+	.name = _name_,							\
+	.length = (_len_) * sizeof(const char *),			\
+	.is_array = true,						\
+	.type = DEV_PROP_STRING,					\
+	{ .pointer = { .str = _val_ } },				\
 }
 
+#define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)				\
+	PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
+#define PROPERTY_ENTRY_U16_ARRAY(_name_, _val_)				\
+	PROPERTY_ENTRY_U16_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
+#define PROPERTY_ENTRY_U32_ARRAY(_name_, _val_)				\
+	PROPERTY_ENTRY_U32_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
+#define PROPERTY_ENTRY_U64_ARRAY(_name_, _val_)				\
+	PROPERTY_ENTRY_U64_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
+#define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_)			\
+	PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
+
 #define PROPERTY_ENTRY_INTEGER(_name_, _type_, _Type_, _val_)	\
 (struct property_entry) {					\
 	.name = _name_,						\
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 03/15] efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN

[Dmitry Torokhov]

Let's switch to using PROPERTY_ENTRY_U8_ARRAY_LEN() to initialize
property entries. Also, when dumping data, rely on local variables
instead of poking into the property entry structure directly.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/firmware/efi/apple-properties.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
index 0e206c9e0d7a..5ccf39986a14 100644
--- a/drivers/firmware/efi/apple-properties.c
+++ b/drivers/firmware/efi/apple-properties.c
@@ -53,7 +53,8 @@ static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
 
 	for (i = 0; i < dev_header->prop_count; i++) {
 		int remaining = dev_header->len - (ptr - (void *)dev_header);
-		u32 key_len, val_len;
+		u32 key_len, val_len, entry_len;
+		const u8 *entry_data;
 		char *key;
 
 		if (sizeof(key_len) > remaining)
@@ -85,17 +86,14 @@ static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
 		ucs2_as_utf8(key, ptr + sizeof(key_len),
 			     key_len - sizeof(key_len));
 
-		entry[i].name = key;
-		entry[i].length = val_len - sizeof(val_len);
-		entry[i].is_array = !!entry[i].length;
-		entry[i].type = DEV_PROP_U8;
-		entry[i].pointer.u8_data = ptr + key_len + sizeof(val_len);
-
+		entry_data = ptr + key_len + sizeof(val_len);
+		entry_len = val_len - sizeof(val_len);
+		entry[i] = PROPERTY_ENTRY_U8_ARRAY_LEN(key, entry_data,
+						       entry_len);
 		if (dump_properties) {
-			dev_info(dev, "property: %s\n", entry[i].name);
+			dev_info(dev, "property: %s\n", key);
 			print_hex_dump(KERN_INFO, pr_fmt(), DUMP_PREFIX_OFFSET,
-				16, 1, entry[i].pointer.u8_data,
-				entry[i].length, true);
+				16, 1, entry_data, entry_len, true);
 		}
 
 		ptr += key_len + val_len;
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 04/15] software node: mark internal macros with double underscores

[Dmitry Torokhov]

Let's mark PROPERTY_ENTRY_* macros that are internal with double leading
underscores so users are not tempted to use them.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 include/linux/property.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/linux/property.h b/include/linux/property.h
index fad2e83a207c..d6019bacd848 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -261,7 +261,7 @@ struct property_entry {
  * and structs.
  */
 
-#define PROPERTY_ENTRY_ARRAY_LEN(_name_, _type_, _Type_, _val_, _len_)	\
+#define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _type_, _Type_, _val_, _len_)\
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = (_len_) * sizeof(_type_),				\
@@ -271,13 +271,13 @@ struct property_entry {
 }
 
 #define PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, _len_)		\
-	PROPERTY_ENTRY_ARRAY_LEN(_name_, u8, U8, _val_, _len_)
+	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u8, U8, _val_, _len_)
 #define PROPERTY_ENTRY_U16_ARRAY_LEN(_name_, _val_, _len_)		\
-	PROPERTY_ENTRY_ARRAY_LEN(_name_, u16, U16, _val_, _len_)
+	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u16, U16, _val_, _len_)
 #define PROPERTY_ENTRY_U32_ARRAY_LEN(_name_, _val_, _len_)		\
-	PROPERTY_ENTRY_ARRAY_LEN(_name_, u32, U32, _val_, _len_)
+	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u32, U32, _val_, _len_)
 #define PROPERTY_ENTRY_U64_ARRAY_LEN(_name_, _val_, _len_)		\
-	PROPERTY_ENTRY_ARRAY_LEN(_name_, u64, U64, _val_, _len_)
+	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u64, U64, _val_, _len_)
 
 #define PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, _len_)		\
 (struct property_entry) {						\
@@ -299,7 +299,7 @@ struct property_entry {
 #define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_)			\
 	PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
 
-#define PROPERTY_ENTRY_INTEGER(_name_, _type_, _Type_, _val_)	\
+#define __PROPERTY_ENTRY_INTEGER(_name_, _type_, _Type_, _val_)	\
 (struct property_entry) {					\
 	.name = _name_,						\
 	.length = sizeof(_type_),				\
@@ -308,13 +308,13 @@ struct property_entry {
 }
 
 #define PROPERTY_ENTRY_U8(_name_, _val_)		\
-	PROPERTY_ENTRY_INTEGER(_name_, u8, U8, _val_)
+	__PROPERTY_ENTRY_INTEGER(_name_, u8, U8, _val_)
 #define PROPERTY_ENTRY_U16(_name_, _val_)		\
-	PROPERTY_ENTRY_INTEGER(_name_, u16, U16, _val_)
+	__PROPERTY_ENTRY_INTEGER(_name_, u16, U16, _val_)
 #define PROPERTY_ENTRY_U32(_name_, _val_)		\
-	PROPERTY_ENTRY_INTEGER(_name_, u32, U32, _val_)
+	__PROPERTY_ENTRY_INTEGER(_name_, u32, U32, _val_)
 #define PROPERTY_ENTRY_U64(_name_, _val_)		\
-	PROPERTY_ENTRY_INTEGER(_name_, u64, U64, _val_)
+	__PROPERTY_ENTRY_INTEGER(_name_, u64, U64, _val_)
 
 #define PROPERTY_ENTRY_STRING(_name_, _val_)		\
 (struct property_entry) {				\
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 05/15] software node: clean up property_copy_string_array()

[Dmitry Torokhov]

Because property_copy_string_array() stores the newly allocated pointer in the
destination property, we have an awkward code in property_entry_copy_data()
where we fetch the new pointer from dst.

Let's change property_copy_string_array() to return pointer and rely on the
common path in property_entry_copy_data() to store it in destination structure.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 12ca439bab30..f41692e0f63b 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -337,8 +337,8 @@ static void property_entry_free_data(const struct property_entry *p)
 	kfree(p->name);
 }
 
-static int property_copy_string_array(struct property_entry *dst,
-				      const struct property_entry *src)
+static const char * const *
+property_copy_string_array(const struct property_entry *src)
 {
 	const char **d;
 	size_t nval = src->length / sizeof(*d);
@@ -346,7 +346,7 @@ static int property_copy_string_array(struct property_entry *dst,
 
 	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
 	if (!d)
-		return -ENOMEM;
+		return NULL;
 
 	for (i = 0; i < nval; i++) {
 		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
@@ -354,12 +354,11 @@ static int property_copy_string_array(struct property_entry *dst,
 			while (--i >= 0)
 				kfree(d[i]);
 			kfree(d);
-			return -ENOMEM;
+			return NULL;
 		}
 	}
 
-	dst->pointer.str = d;
-	return 0;
+	return d;
 }
 
 static int property_entry_copy_data(struct property_entry *dst,
@@ -367,17 +366,15 @@ static int property_entry_copy_data(struct property_entry *dst,
 {
 	const void *pointer = property_get_pointer(src);
 	const void *new;
-	int error;
 
 	if (src->is_array) {
 		if (!src->length)
 			return -ENODATA;
 
 		if (src->type == DEV_PROP_STRING) {
-			error = property_copy_string_array(dst, src);
-			if (error)
-				return error;
-			new = dst->pointer.str;
+			new = property_copy_string_array(src);
+			if (!new)
+				return -ENOMEM;
 		} else {
 			new = kmemdup(pointer, src->length, GFP_KERNEL);
 			if (!new)
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 06/15] software node: get rid of property_set_pointer()

[Dmitry Torokhov]

Instead of explicitly setting values of integer types when copying
property entries lets just copy entire value union when processing
non-array values.

For value arrays we no longer use union of pointers, but rather a single
void pointer, which allows us to remove property_set_pointer().

In property_get_pointer() we do not need to handle each data type
separately, we can simply return either the pointer or pointer to values
union.

We are not losing anything from removing typed pointer union because the
upper layers do their accesses through void pointers anyway, and we
trust the "type" of the property when interpret the data. We rely on
users of property entries on using PROPERTY_ENTRY_XXX() macros to
properly initialize entries instead of poking in the instances directly.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 90 +++++++++-------------------------------
 include/linux/property.h | 12 ++----
 2 files changed, 22 insertions(+), 80 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index f41692e0f63b..fd667a73a201 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -103,71 +103,15 @@ property_entry_get(const struct property_entry *prop, const char *name)
 	return NULL;
 }
 
-static void
-property_set_pointer(struct property_entry *prop, const void *pointer)
-{
-	switch (prop->type) {
-	case DEV_PROP_U8:
-		if (prop->is_array)
-			prop->pointer.u8_data = pointer;
-		else
-			prop->value.u8_data = *((u8 *)pointer);
-		break;
-	case DEV_PROP_U16:
-		if (prop->is_array)
-			prop->pointer.u16_data = pointer;
-		else
-			prop->value.u16_data = *((u16 *)pointer);
-		break;
-	case DEV_PROP_U32:
-		if (prop->is_array)
-			prop->pointer.u32_data = pointer;
-		else
-			prop->value.u32_data = *((u32 *)pointer);
-		break;
-	case DEV_PROP_U64:
-		if (prop->is_array)
-			prop->pointer.u64_data = pointer;
-		else
-			prop->value.u64_data = *((u64 *)pointer);
-		break;
-	case DEV_PROP_STRING:
-		if (prop->is_array)
-			prop->pointer.str = pointer;
-		else
-			prop->value.str = pointer;
-		break;
-	default:
-		break;
-	}
-}
-
 static const void *property_get_pointer(const struct property_entry *prop)
 {
-	switch (prop->type) {
-	case DEV_PROP_U8:
-		if (prop->is_array)
-			return prop->pointer.u8_data;
-		return &prop->value.u8_data;
-	case DEV_PROP_U16:
-		if (prop->is_array)
-			return prop->pointer.u16_data;
-		return &prop->value.u16_data;
-	case DEV_PROP_U32:
-		if (prop->is_array)
-			return prop->pointer.u32_data;
-		return &prop->value.u32_data;
-	case DEV_PROP_U64:
-		if (prop->is_array)
-			return prop->pointer.u64_data;
-		return &prop->value.u64_data;
-	case DEV_PROP_STRING:
-		if (prop->is_array)
-			return prop->pointer.str;
-		return &prop->value.str;
-	default:
+	if (!prop->length)
 		return NULL;
-	}
+
+	if (prop->is_array)
+		return prop->pointer;
+
+	return &prop->value;
 }
 
 static const void *property_entry_find(const struct property_entry *props,
@@ -322,13 +266,15 @@ static int property_entry_read_string_array(const struct property_entry *props,
 static void property_entry_free_data(const struct property_entry *p)
 {
 	const void *pointer = property_get_pointer(p);
+	const char * const *src_str;
 	size_t i, nval;
 
 	if (p->is_array) {
-		if (p->type == DEV_PROP_STRING && p->pointer.str) {
+		if (p->type == DEV_PROP_STRING && p->pointer) {
+			src_str = p->pointer;
 			nval = p->length / sizeof(const char *);
 			for (i = 0; i < nval; i++)
-				kfree(p->pointer.str[i]);
+				kfree(src_str[i]);
 		}
 		kfree(pointer);
 	} else if (p->type == DEV_PROP_STRING) {
@@ -341,6 +287,7 @@ static const char * const *
 property_copy_string_array(const struct property_entry *src)
 {
 	const char **d;
+	const char * const *src_str = src->pointer;
 	size_t nval = src->length / sizeof(*d);
 	int i;
 
@@ -349,8 +296,8 @@ property_copy_string_array(const struct property_entry *src)
 		return NULL;
 
 	for (i = 0; i < nval; i++) {
-		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
-		if (!d[i] && src->pointer.str[i]) {
+		d[i] = kstrdup(src_str[i], GFP_KERNEL);
+		if (!d[i] && src_str[i]) {
 			while (--i >= 0)
 				kfree(d[i]);
 			kfree(d);
@@ -380,20 +327,21 @@ static int property_entry_copy_data(struct property_entry *dst,
 			if (!new)
 				return -ENOMEM;
 		}
+
+		dst->is_array = true;
+		dst->pointer = new;
 	} else if (src->type == DEV_PROP_STRING) {
 		new = kstrdup(src->value.str, GFP_KERNEL);
 		if (!new && src->value.str)
 			return -ENOMEM;
+
+		dst->value.str = new;
 	} else {
-		new = pointer;
+		dst->value = src->value;
 	}
 
 	dst->length = src->length;
-	dst->is_array = src->is_array;
 	dst->type = src->type;
-
-	property_set_pointer(dst, new);
-
 	dst->name = kstrdup(src->name, GFP_KERNEL);
 	if (!dst->name)
 		goto out_free_data;
diff --git a/include/linux/property.h b/include/linux/property.h
index d6019bacd848..12eff7cbb395 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -238,13 +238,7 @@ struct property_entry {
 	bool is_array;
 	enum dev_prop_type type;
 	union {
-		union {
-			const u8 *u8_data;
-			const u16 *u16_data;
-			const u32 *u32_data;
-			const u64 *u64_data;
-			const char * const *str;
-		} pointer;
+		const void *pointer;
 		union {
 			u8 u8_data;
 			u16 u16_data;
@@ -267,7 +261,7 @@ struct property_entry {
 	.length = (_len_) * sizeof(_type_),				\
 	.is_array = true,						\
 	.type = DEV_PROP_##_Type_,					\
-	{ .pointer = { ._type_##_data = _val_ } },			\
+	{ .pointer = _val_ },						\
 }
 
 #define PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, _len_)		\
@@ -285,7 +279,7 @@ struct property_entry {
 	.length = (_len_) * sizeof(const char *),			\
 	.is_array = true,						\
 	.type = DEV_PROP_STRING,					\
-	{ .pointer = { .str = _val_ } },				\
+	{ .pointer = _val_ },						\
 }
 
 #define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)				\
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 07/15] software node: remove property_entry_read_uNN_array functions

[Dmitry Torokhov]

There is absolutely no reason to have them as we can handle it all nicely in
property_entry_read_int_array().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c | 85 +++++++------------------------------------
 1 file changed, 14 insertions(+), 71 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index fd667a73a201..f59dfd01725f 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -131,66 +131,6 @@ static const void *property_entry_find(const struct property_entry *props,
 	return pointer;
 }
 
-static int property_entry_read_u8_array(const struct property_entry *props,
-					const char *propname,
-					u8 *values, size_t nval)
-{
-	const void *pointer;
-	size_t length = nval * sizeof(*values);
-
-	pointer = property_entry_find(props, propname, length);
-	if (IS_ERR(pointer))
-		return PTR_ERR(pointer);
-
-	memcpy(values, pointer, length);
-	return 0;
-}
-
-static int property_entry_read_u16_array(const struct property_entry *props,
-					 const char *propname,
-					 u16 *values, size_t nval)
-{
-	const void *pointer;
-	size_t length = nval * sizeof(*values);
-
-	pointer = property_entry_find(props, propname, length);
-	if (IS_ERR(pointer))
-		return PTR_ERR(pointer);
-
-	memcpy(values, pointer, length);
-	return 0;
-}
-
-static int property_entry_read_u32_array(const struct property_entry *props,
-					 const char *propname,
-					 u32 *values, size_t nval)
-{
-	const void *pointer;
-	size_t length = nval * sizeof(*values);
-
-	pointer = property_entry_find(props, propname, length);
-	if (IS_ERR(pointer))
-		return PTR_ERR(pointer);
-
-	memcpy(values, pointer, length);
-	return 0;
-}
-
-static int property_entry_read_u64_array(const struct property_entry *props,
-					 const char *propname,
-					 u64 *values, size_t nval)
-{
-	const void *pointer;
-	size_t length = nval * sizeof(*values);
-
-	pointer = property_entry_find(props, propname, length);
-	if (IS_ERR(pointer))
-		return PTR_ERR(pointer);
-
-	memcpy(values, pointer, length);
-	return 0;
-}
-
 static int
 property_entry_count_elems_of_size(const struct property_entry *props,
 				   const char *propname, size_t length)
@@ -209,21 +149,24 @@ static int property_entry_read_int_array(const struct property_entry *props,
 					 unsigned int elem_size, void *val,
 					 size_t nval)
 {
+	const void *pointer;
+	size_t length;
+
 	if (!val)
 		return property_entry_count_elems_of_size(props, name,
 							  elem_size);
-	switch (elem_size) {
-	case sizeof(u8):
-		return property_entry_read_u8_array(props, name, val, nval);
-	case sizeof(u16):
-		return property_entry_read_u16_array(props, name, val, nval);
-	case sizeof(u32):
-		return property_entry_read_u32_array(props, name, val, nval);
-	case sizeof(u64):
-		return property_entry_read_u64_array(props, name, val, nval);
-	}
 
-	return -ENXIO;
+	if (!is_power_of_2(elem_size) || elem_size > sizeof(u64))
+		return -ENXIO;
+
+	length = nval * elem_size;
+
+	pointer = property_entry_find(props, name, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(val, pointer, length);
+	return 0;
 }
 
 static int property_entry_read_string_array(const struct property_entry *props,
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 08/15] software node: unify PROPERTY_ENTRY_XXX macros

[Dmitry Torokhov]

We can unify string properties initializer macros with integer
initializers.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 include/linux/property.h | 64 +++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 37 deletions(-)

diff --git a/include/linux/property.h b/include/linux/property.h
index 12eff7cbb395..48335288c2a9 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -250,37 +250,33 @@ struct property_entry {
 };
 
 /*
- * Note: the below four initializers for the anonymous union are carefully
+ * Note: the below initializers for the anonymous union are carefully
  * crafted to avoid gcc-4.4.4's problems with initialization of anon unions
  * and structs.
  */
 
-#define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _type_, _Type_, _val_, _len_)\
+#define __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_)				\
+	sizeof(((struct property_entry *)NULL)->value._elem_)
+
+#define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _elem_, _Type_, _val_, _len_)\
 (struct property_entry) {						\
 	.name = _name_,							\
-	.length = (_len_) * sizeof(_type_),				\
+	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
 	.is_array = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .pointer = _val_ },						\
 }
 
 #define PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, _len_)		\
-	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u8, U8, _val_, _len_)
+	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u8_data, U8, _val_, _len_)
 #define PROPERTY_ENTRY_U16_ARRAY_LEN(_name_, _val_, _len_)		\
-	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u16, U16, _val_, _len_)
+	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u16_data, U16, _val_, _len_)
 #define PROPERTY_ENTRY_U32_ARRAY_LEN(_name_, _val_, _len_)		\
-	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u32, U32, _val_, _len_)
+	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u32_data, U32, _val_, _len_)
 #define PROPERTY_ENTRY_U64_ARRAY_LEN(_name_, _val_, _len_)		\
-	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u64, U64, _val_, _len_)
-
+	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u64_data, U64, _val_, _len_)
 #define PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, _len_)		\
-(struct property_entry) {						\
-	.name = _name_,							\
-	.length = (_len_) * sizeof(const char *),			\
-	.is_array = true,						\
-	.type = DEV_PROP_STRING,					\
-	{ .pointer = _val_ },						\
-}
+	__PROPERTY_ENTRY_ARRAY_LEN(_name_, str, STRING, _val_, _len_)
 
 #define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)				\
 	PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
@@ -293,30 +289,24 @@ struct property_entry {
 #define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_)			\
 	PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
 
-#define __PROPERTY_ENTRY_INTEGER(_name_, _type_, _Type_, _val_)	\
-(struct property_entry) {					\
-	.name = _name_,						\
-	.length = sizeof(_type_),				\
-	.type = DEV_PROP_##_Type_,				\
-	{ .value = { ._type_##_data = _val_ } },		\
+#define __PROPERTY_ENTRY_ELEMENT(_name_, _elem_, _Type_, _val_)		\
+(struct property_entry) {						\
+	.name = _name_,							\
+	.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),		\
+	.type = DEV_PROP_##_Type_,					\
+	{ .value = { ._elem_ = _val_ } },				\
 }
 
-#define PROPERTY_ENTRY_U8(_name_, _val_)		\
-	__PROPERTY_ENTRY_INTEGER(_name_, u8, U8, _val_)
-#define PROPERTY_ENTRY_U16(_name_, _val_)		\
-	__PROPERTY_ENTRY_INTEGER(_name_, u16, U16, _val_)
-#define PROPERTY_ENTRY_U32(_name_, _val_)		\
-	__PROPERTY_ENTRY_INTEGER(_name_, u32, U32, _val_)
-#define PROPERTY_ENTRY_U64(_name_, _val_)		\
-	__PROPERTY_ENTRY_INTEGER(_name_, u64, U64, _val_)
-
-#define PROPERTY_ENTRY_STRING(_name_, _val_)		\
-(struct property_entry) {				\
-	.name = _name_,					\
-	.length = sizeof(const char *),			\
-	.type = DEV_PROP_STRING,			\
-	{ .value = { .str = _val_ } },			\
-}
+#define PROPERTY_ENTRY_U8(_name_, _val_)				\
+	__PROPERTY_ENTRY_ELEMENT(_name_, u8_data, U8, _val_)
+#define PROPERTY_ENTRY_U16(_name_, _val_)				\
+	__PROPERTY_ENTRY_ELEMENT(_name_, u16_data, U16, _val_)
+#define PROPERTY_ENTRY_U32(_name_, _val_)				\
+	__PROPERTY_ENTRY_ELEMENT(_name_, u32_data, U32, _val_)
+#define PROPERTY_ENTRY_U64(_name_, _val_)				\
+	__PROPERTY_ENTRY_ELEMENT(_name_, u64_data, U64, _val_)
+#define PROPERTY_ENTRY_STRING(_name_, _val_)				\
+	__PROPERTY_ENTRY_ELEMENT(_name_, str, STRING, _val_)
 
 #define PROPERTY_ENTRY_BOOL(_name_)		\
 (struct property_entry) {			\
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 09/15] software node: simplify property_entry_read_string_array()

[Dmitry Torokhov]

There is no need to treat string arrays and single strings separately, we can go
exclusively by the element length in relation to data type size.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index f59dfd01725f..d8d0dc0ca5ac 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -173,28 +173,21 @@ static int property_entry_read_string_array(const struct property_entry *props,
 					    const char *propname,
 					    const char **strings, size_t nval)
 {
-	const struct property_entry *prop;
 	const void *pointer;
-	size_t array_len, length;
+	size_t length;
+	int array_len;
 
 	/* Find out the array length. */
-	prop = property_entry_get(props, propname);
-	if (!prop)
-		return -EINVAL;
-
-	if (prop->is_array)
-		/* Find the length of an array. */
-		array_len = property_entry_count_elems_of_size(props, propname,
-							  sizeof(const char *));
-	else
-		/* The array length for a non-array string property is 1. */
-		array_len = 1;
+	array_len = property_entry_count_elems_of_size(props, propname,
+						       sizeof(const char *));
+	if (array_len < 0)
+		return array_len;
 
 	/* Return how many there are if strings is NULL. */
 	if (!strings)
 		return array_len;
 
-	array_len = min(nval, array_len);
+	array_len = min_t(size_t, nval, array_len);
 	length = array_len * sizeof(*strings);
 
 	pointer = property_entry_find(props, propname, length);
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 10/15] software node: rename is_array to is_inline

[Dmitry Torokhov]

We do not need a special flag to know if we are dealing with an array,
as we can get that data from ratio between element length and the data
size, however we do need a flag to know whether the data is stored
directly inside property_entry or separately.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 12 +++++-------
 include/linux/property.h | 13 ++++++++-----
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index d8d0dc0ca5ac..18a30fb3cc58 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -108,10 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
 	if (!prop->length)
 		return NULL;
 
-	if (prop->is_array)
-		return prop->pointer;
-
-	return &prop->value;
+	return prop->is_inline ? &prop->value : prop->pointer;
 }
 
 static const void *property_entry_find(const struct property_entry *props,
@@ -205,7 +202,7 @@ static void property_entry_free_data(const struct property_entry *p)
 	const char * const *src_str;
 	size_t i, nval;
 
-	if (p->is_array) {
+	if (!p->is_inline) {
 		if (p->type == DEV_PROP_STRING && p->pointer) {
 			src_str = p->pointer;
 			nval = p->length / sizeof(const char *);
@@ -250,7 +247,7 @@ static int property_entry_copy_data(struct property_entry *dst,
 	const void *pointer = property_get_pointer(src);
 	const void *new;
 
-	if (src->is_array) {
+	if (!src->is_inline) {
 		if (!src->length)
 			return -ENODATA;
 
@@ -264,15 +261,16 @@ static int property_entry_copy_data(struct property_entry *dst,
 				return -ENOMEM;
 		}
 
-		dst->is_array = true;
 		dst->pointer = new;
 	} else if (src->type == DEV_PROP_STRING) {
 		new = kstrdup(src->value.str, GFP_KERNEL);
 		if (!new && src->value.str)
 			return -ENOMEM;
 
+		dst->is_inline = true;
 		dst->value.str = new;
 	} else {
+		dst->is_inline = true;
 		dst->value = src->value;
 	}
 
diff --git a/include/linux/property.h b/include/linux/property.h
index 48335288c2a9..dad0ad11b55e 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -227,15 +227,17 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
  * @length: Length of data making up the value.
- * @is_array: True when the property is an array.
+ * @is_inline: True when the property value is embedded in
+ *	&struct property_entry instance.
  * @type: Type of the data in unions.
- * @pointer: Pointer to the property (an array of items of the given type).
- * @value: Value of the property (when it is a single item of the given type).
+ * @pointer: Pointer to the property when it is stored separately from
+ *	the &struct property_entry instance.
+ * @value: Value of the property when it is stored inline.
  */
 struct property_entry {
 	const char *name;
 	size_t length;
-	bool is_array;
+	bool is_inline;
 	enum dev_prop_type type;
 	union {
 		const void *pointer;
@@ -262,7 +264,6 @@ struct property_entry {
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
-	.is_array = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .pointer = _val_ },						\
 }
@@ -293,6 +294,7 @@ struct property_entry {
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),		\
+	.is_inline = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .value = { ._elem_ = _val_ } },				\
 }
@@ -311,6 +313,7 @@ struct property_entry {
 #define PROPERTY_ENTRY_BOOL(_name_)		\
 (struct property_entry) {			\
 	.name = _name_,				\
+	.is_inline = true,			\
 }
 
 struct property_entry *
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 11/15] software node: move small properties inline when copying

[Dmitry Torokhov]

When copying/duplicating set of properties, move smaller properties that
were stored separately directly inside property entry structures. We can
move:

- up to 8 bytes from U8 arrays
- up to 4 words
- up to 2 double words
- one U64 value
- one or 2 strings.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 18a30fb3cc58..49e1108aa4b7 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -280,6 +280,16 @@ static int property_entry_copy_data(struct property_entry *dst,
 	if (!dst->name)
 		goto out_free_data;
 
+	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
+		/* We have an opportunity to move the data inline */
+		const void *tmp = dst->pointer;
+
+		memcpy(&dst->value, tmp, dst->length);
+		dst->is_inline = true;
+
+		kfree(tmp);
+	}
+
 	return 0;
 
 out_free_data:
-- 
2.23.0.866.gb869b98d4c-goog

Re: [PATCH v6 11/15] software node: move small properties inline when copying

[Rafael J. Wysocki]

On Wednesday, October 23, 2019 10:02:29 PM CET Dmitry Torokhov wrote:
> When copying/duplicating set of properties, move smaller properties that
> were stored separately directly inside property entry structures. We can
> move:
> 
> - up to 8 bytes from U8 arrays
> - up to 4 words
> - up to 2 double words
> - one U64 value
> - one or 2 strings.

Yes, we can do that, but how much of a difference does this really make?

Also, how can one distinguish between a single-value property and an inline
array which this change?  By looking at the length?

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/swnode.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 18a30fb3cc58..49e1108aa4b7 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -280,6 +280,16 @@ static int property_entry_copy_data(struct property_entry *dst,
>  	if (!dst->name)
>  		goto out_free_data;
>  
> +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> +		/* We have an opportunity to move the data inline */
> +		const void *tmp = dst->pointer;
> +
> +		memcpy(&dst->value, tmp, dst->length);
> +		dst->is_inline = true;
> +
> +		kfree(tmp);

This would have been more useful if we had been able to avoid making the
allocation altogether.

> +	}
> +
>  	return 0;
>  
>  out_free_data:
> 

Re: [PATCH v6 11/15] software node: move small properties inline when copying

[Dmitry Torokhov]

Hi Rafael,

On Wed, Nov 06, 2019 at 12:42:02AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, October 23, 2019 10:02:29 PM CET Dmitry Torokhov wrote:
> > When copying/duplicating set of properties, move smaller properties that
> > were stored separately directly inside property entry structures. We can
> > move:
> > 
> > - up to 8 bytes from U8 arrays
> > - up to 4 words
> > - up to 2 double words
> > - one U64 value
> > - one or 2 strings.
> 
> Yes, we can do that, but how much of a difference does this really make?

Arguably not much I think, but it was pretty cheap to do.

> 
> Also, how can one distinguish between a single-value property and an inline
> array which this change?  By looking at the length?

We do not really need to distinguish between the 2. The device
properties API is typically wrap single values around arrays (i.e. it is
perfectly fine to use scalar API to fetch first element of array and use
array API to fetch a scalar). So we have property of certain type with
certain number of elements, and it can either be stored inside
property_entry structure, or outside of it. They are 2 orthogonal
concepts.

> 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/base/swnode.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > index 18a30fb3cc58..49e1108aa4b7 100644
> > --- a/drivers/base/swnode.c
> > +++ b/drivers/base/swnode.c
> > @@ -280,6 +280,16 @@ static int property_entry_copy_data(struct property_entry *dst,
> >  	if (!dst->name)
> >  		goto out_free_data;
> >  
> > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > +		/* We have an opportunity to move the data inline */
> > +		const void *tmp = dst->pointer;
> > +
> > +		memcpy(&dst->value, tmp, dst->length);
> > +		dst->is_inline = true;
> > +
> > +		kfree(tmp);
> 
> This would have been more useful if we had been able to avoid making the
> allocation altogether.

OK, I can do that and re-send this patch and the one with the tests. In
the mean time, can you please consider patches 12-14? They can be
applied even if you temporarily drop this one (#11).

Thanks.

-- 
Dmitry

Re: [PATCH v6 11/15] software node: move small properties inline when copying

[Rafael J. Wysocki]

On Wednesday, November 6, 2019 12:56:56 AM CET Dmitry Torokhov wrote:
> Hi Rafael,
> 
> On Wed, Nov 06, 2019 at 12:42:02AM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, October 23, 2019 10:02:29 PM CET Dmitry Torokhov wrote:
> > > When copying/duplicating set of properties, move smaller properties that
> > > were stored separately directly inside property entry structures. We can
> > > move:
> > > 
> > > - up to 8 bytes from U8 arrays
> > > - up to 4 words
> > > - up to 2 double words
> > > - one U64 value
> > > - one or 2 strings.
> > 
> > Yes, we can do that, but how much of a difference does this really make?
> 
> Arguably not much I think, but it was pretty cheap to do.
> 
> > 
> > Also, how can one distinguish between a single-value property and an inline
> > array which this change?  By looking at the length?
> 
> We do not really need to distinguish between the 2. The device
> properties API is typically wrap single values around arrays (i.e. it is
> perfectly fine to use scalar API to fetch first element of array and use
> array API to fetch a scalar). So we have property of certain type with
> certain number of elements, and it can either be stored inside
> property_entry structure, or outside of it. They are 2 orthogonal
> concepts.
> 
> > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/base/swnode.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > > index 18a30fb3cc58..49e1108aa4b7 100644
> > > --- a/drivers/base/swnode.c
> > > +++ b/drivers/base/swnode.c
> > > @@ -280,6 +280,16 @@ static int property_entry_copy_data(struct property_entry *dst,
> > >  	if (!dst->name)
> > >  		goto out_free_data;
> > >  
> > > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > > +		/* We have an opportunity to move the data inline */
> > > +		const void *tmp = dst->pointer;
> > > +
> > > +		memcpy(&dst->value, tmp, dst->length);
> > > +		dst->is_inline = true;
> > > +
> > > +		kfree(tmp);
> > 
> > This would have been more useful if we had been able to avoid making the
> > allocation altogether.
> 
> OK, I can do that and re-send this patch and the one with the tests.

But if you do that, IMO it would be prudent to extend the definition of
struct property_entry like this:

 struct property_entry {
 	const char *name;
 	size_t length;
 	bool is_array;
 	enum dev_prop_type type;
 	union {
 		union {
 			const u8 *u8_data;
 			const u16 *u16_data;
 			const u32 *u32_data;
 			const u64 *u64_data;
 			const char * const *str;
 		} pointer;
 		union {
 			u8 u8_data;
 			u16 u16_data;
 			u32 u32_data;
 			u64 u64_data;
 			const char *str;
+			u8 u8_buf[sizeof(u64)];
+			u16 u16_buf[sizeof(u64)/sizeof(u16)];
+			u32 u32_buf[sizeof(u64)/sizeof(u32)];
+			char char_buf[sizeof(u64)];
 		} value;
 	};
 };

to make it clear that the value field is going to be used as an array in
some cases.

> In the mean time, can you please consider patches 12-14?

I cannot find drivers/platform/x86/intel_cht_int33fe_typec.c in the mainline,
so I cannot apply patch [13/15] now and I'm not sure how useful it would be
to apply patches [10,12/15] without the other two.

Re: [PATCH v6 11/15] software node: move small properties inline when copying

[Dmitry Torokhov]

On Fri, Nov 08, 2019 at 01:04:31AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, November 6, 2019 12:56:56 AM CET Dmitry Torokhov wrote:
> > Hi Rafael,
> > 
> > On Wed, Nov 06, 2019 at 12:42:02AM +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, October 23, 2019 10:02:29 PM CET Dmitry Torokhov wrote:
> > > > When copying/duplicating set of properties, move smaller properties that
> > > > were stored separately directly inside property entry structures. We can
> > > > move:
> > > > 
> > > > - up to 8 bytes from U8 arrays
> > > > - up to 4 words
> > > > - up to 2 double words
> > > > - one U64 value
> > > > - one or 2 strings.
> > > 
> > > Yes, we can do that, but how much of a difference does this really make?
> > 
> > Arguably not much I think, but it was pretty cheap to do.
> > 
> > > 
> > > Also, how can one distinguish between a single-value property and an inline
> > > array which this change?  By looking at the length?
> > 
> > We do not really need to distinguish between the 2. The device
> > properties API is typically wrap single values around arrays (i.e. it is
> > perfectly fine to use scalar API to fetch first element of array and use
> > array API to fetch a scalar). So we have property of certain type with
> > certain number of elements, and it can either be stored inside
> > property_entry structure, or outside of it. They are 2 orthogonal
> > concepts.
> > 
> > > 
> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > ---
> > > >  drivers/base/swnode.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > > > index 18a30fb3cc58..49e1108aa4b7 100644
> > > > --- a/drivers/base/swnode.c
> > > > +++ b/drivers/base/swnode.c
> > > > @@ -280,6 +280,16 @@ static int property_entry_copy_data(struct property_entry *dst,
> > > >  	if (!dst->name)
> > > >  		goto out_free_data;
> > > >  
> > > > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > > > +		/* We have an opportunity to move the data inline */
> > > > +		const void *tmp = dst->pointer;
> > > > +
> > > > +		memcpy(&dst->value, tmp, dst->length);
> > > > +		dst->is_inline = true;
> > > > +
> > > > +		kfree(tmp);
> > > 
> > > This would have been more useful if we had been able to avoid making the
> > > allocation altogether.
> > 
> > OK, I can do that and re-send this patch and the one with the tests.
> 
> But if you do that, IMO it would be prudent to extend the definition of
> struct property_entry like this:
> 
>  struct property_entry {
>  	const char *name;
>  	size_t length;
>  	bool is_array;
>  	enum dev_prop_type type;
>  	union {
>  		union {
>  			const u8 *u8_data;
>  			const u16 *u16_data;
>  			const u32 *u32_data;
>  			const u64 *u64_data;
>  			const char * const *str;
>  		} pointer;
>  		union {
>  			u8 u8_data;
>  			u16 u16_data;
>  			u32 u32_data;
>  			u64 u64_data;
>  			const char *str;
> +			u8 u8_buf[sizeof(u64)];
> +			u16 u16_buf[sizeof(u64)/sizeof(u16)];
> +			u32 u32_buf[sizeof(u64)/sizeof(u32)];
> +			char char_buf[sizeof(u64)];
>  		} value;
>  	};
>  };
> 
> to make it clear that the value field is going to be used as an array in
> some cases.

Sorry, just sent out updated series before receiving your email. I can
cook up new patch cleaning this. I think we can drop scalars and only
have arrays and have initializers use <type>_data[0] to create initial
property entries.

> 
> > In the mean time, can you please consider patches 12-14?
> 
> I cannot find drivers/platform/x86/intel_cht_int33fe_typec.c in the mainline,
> so I cannot apply patch [13/15] now and I'm not sure how useful it would be
> to apply patches [10,12/15] without the other two.

Hmm, drivers/platform/x86/intel_cht_int33fe_typec.c used to be
drivers/platform/x86/intel_cht_int33fe.c I think.

I can either regenerate against your tree instead of -next (but then
there will be merge conflict) or we could postpone #13 and #14 (or #5
and #6 in v7) till after merge window.

Please let me know.

-- 
Dmitry

Re: [PATCH v6 11/15] software node: move small properties inline when copying

[Rafael J. Wysocki]

On Friday, November 8, 2019 1:28:44 AM CET Dmitry Torokhov wrote:
> On Fri, Nov 08, 2019 at 01:04:31AM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, November 6, 2019 12:56:56 AM CET Dmitry Torokhov wrote:
> > > Hi Rafael,
> > > 
> > > On Wed, Nov 06, 2019 at 12:42:02AM +0100, Rafael J. Wysocki wrote:
> > > > On Wednesday, October 23, 2019 10:02:29 PM CET Dmitry Torokhov wrote:
> > > > > When copying/duplicating set of properties, move smaller properties that
> > > > > were stored separately directly inside property entry structures. We can
> > > > > move:
> > > > > 
> > > > > - up to 8 bytes from U8 arrays
> > > > > - up to 4 words
> > > > > - up to 2 double words
> > > > > - one U64 value
> > > > > - one or 2 strings.
> > > > 
> > > > Yes, we can do that, but how much of a difference does this really make?
> > > 
> > > Arguably not much I think, but it was pretty cheap to do.
> > > 
> > > > 
> > > > Also, how can one distinguish between a single-value property and an inline
> > > > array which this change?  By looking at the length?
> > > 
> > > We do not really need to distinguish between the 2. The device
> > > properties API is typically wrap single values around arrays (i.e. it is
> > > perfectly fine to use scalar API to fetch first element of array and use
> > > array API to fetch a scalar). So we have property of certain type with
> > > certain number of elements, and it can either be stored inside
> > > property_entry structure, or outside of it. They are 2 orthogonal
> > > concepts.
> > > 
> > > > 
> > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > > ---
> > > > >  drivers/base/swnode.c | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > > > > index 18a30fb3cc58..49e1108aa4b7 100644
> > > > > --- a/drivers/base/swnode.c
> > > > > +++ b/drivers/base/swnode.c
> > > > > @@ -280,6 +280,16 @@ static int property_entry_copy_data(struct property_entry *dst,
> > > > >  	if (!dst->name)
> > > > >  		goto out_free_data;
> > > > >  
> > > > > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > > > > +		/* We have an opportunity to move the data inline */
> > > > > +		const void *tmp = dst->pointer;
> > > > > +
> > > > > +		memcpy(&dst->value, tmp, dst->length);
> > > > > +		dst->is_inline = true;
> > > > > +
> > > > > +		kfree(tmp);
> > > > 
> > > > This would have been more useful if we had been able to avoid making the
> > > > allocation altogether.
> > > 
> > > OK, I can do that and re-send this patch and the one with the tests.
> > 
> > But if you do that, IMO it would be prudent to extend the definition of
> > struct property_entry like this:
> > 
> >  struct property_entry {
> >  	const char *name;
> >  	size_t length;
> >  	bool is_array;
> >  	enum dev_prop_type type;
> >  	union {
> >  		union {
> >  			const u8 *u8_data;
> >  			const u16 *u16_data;
> >  			const u32 *u32_data;
> >  			const u64 *u64_data;
> >  			const char * const *str;
> >  		} pointer;
> >  		union {
> >  			u8 u8_data;
> >  			u16 u16_data;
> >  			u32 u32_data;
> >  			u64 u64_data;
> >  			const char *str;
> > +			u8 u8_buf[sizeof(u64)];
> > +			u16 u16_buf[sizeof(u64)/sizeof(u16)];
> > +			u32 u32_buf[sizeof(u64)/sizeof(u32)];
> > +			char char_buf[sizeof(u64)];
> >  		} value;
> >  	};
> >  };
> > 
> > to make it clear that the value field is going to be used as an array in
> > some cases.
> 
> Sorry, just sent out updated series before receiving your email. I can
> cook up new patch cleaning this.

I'd prefer a new version of the series, honestly.

> I think we can drop scalars and only have arrays and have initializers use
> <type>_data[0] to create initial property entries.

Why [0]?  IMO it is better to use the exact size (which is known) in this
particular case.

Also note that u64 is naturally a scalar only.
 
> > 
> > > In the mean time, can you please consider patches 12-14?
> > 
> > I cannot find drivers/platform/x86/intel_cht_int33fe_typec.c in the mainline,
> > so I cannot apply patch [13/15] now and I'm not sure how useful it would be
> > to apply patches [10,12/15] without the other two.
> 
> Hmm, drivers/platform/x86/intel_cht_int33fe_typec.c used to be
> drivers/platform/x86/intel_cht_int33fe.c I think.
> 
> I can either regenerate against your tree instead of -next (but then
> there will be merge conflict) or we could postpone #13 and #14 (or #5
> and #6 in v7) till after merge window.
> 
> Please let me know.

I'd rather postpone the whole series to until the dependencies are in,
which may be during the merge window (e.g. if this happens during the
first week of it, waiting for another extra week just for the merge
window to end is not quite useful IMO).

Re: [PATCH v6 11/15] software node: move small properties inline when copying

[Dmitry Torokhov]

On Fri, Nov 08, 2019 at 01:45:03AM +0100, Rafael J. Wysocki wrote:
> On Friday, November 8, 2019 1:28:44 AM CET Dmitry Torokhov wrote:
> > On Fri, Nov 08, 2019 at 01:04:31AM +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, November 6, 2019 12:56:56 AM CET Dmitry Torokhov wrote:
> > > > Hi Rafael,
> > > > 
> > > > On Wed, Nov 06, 2019 at 12:42:02AM +0100, Rafael J. Wysocki wrote:
> > > > > On Wednesday, October 23, 2019 10:02:29 PM CET Dmitry Torokhov wrote:
> > > > > > When copying/duplicating set of properties, move smaller properties that
> > > > > > were stored separately directly inside property entry structures. We can
> > > > > > move:
> > > > > > 
> > > > > > - up to 8 bytes from U8 arrays
> > > > > > - up to 4 words
> > > > > > - up to 2 double words
> > > > > > - one U64 value
> > > > > > - one or 2 strings.
> > > > > 
> > > > > Yes, we can do that, but how much of a difference does this really make?
> > > > 
> > > > Arguably not much I think, but it was pretty cheap to do.
> > > > 
> > > > > 
> > > > > Also, how can one distinguish between a single-value property and an inline
> > > > > array which this change?  By looking at the length?
> > > > 
> > > > We do not really need to distinguish between the 2. The device
> > > > properties API is typically wrap single values around arrays (i.e. it is
> > > > perfectly fine to use scalar API to fetch first element of array and use
> > > > array API to fetch a scalar). So we have property of certain type with
> > > > certain number of elements, and it can either be stored inside
> > > > property_entry structure, or outside of it. They are 2 orthogonal
> > > > concepts.
> > > > 
> > > > > 
> > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > > > ---
> > > > > >  drivers/base/swnode.c | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > > > > > index 18a30fb3cc58..49e1108aa4b7 100644
> > > > > > --- a/drivers/base/swnode.c
> > > > > > +++ b/drivers/base/swnode.c
> > > > > > @@ -280,6 +280,16 @@ static int property_entry_copy_data(struct property_entry *dst,
> > > > > >  	if (!dst->name)
> > > > > >  		goto out_free_data;
> > > > > >  
> > > > > > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > > > > > +		/* We have an opportunity to move the data inline */
> > > > > > +		const void *tmp = dst->pointer;
> > > > > > +
> > > > > > +		memcpy(&dst->value, tmp, dst->length);
> > > > > > +		dst->is_inline = true;
> > > > > > +
> > > > > > +		kfree(tmp);
> > > > > 
> > > > > This would have been more useful if we had been able to avoid making the
> > > > > allocation altogether.
> > > > 
> > > > OK, I can do that and re-send this patch and the one with the tests.
> > > 
> > > But if you do that, IMO it would be prudent to extend the definition of
> > > struct property_entry like this:
> > > 
> > >  struct property_entry {
> > >  	const char *name;
> > >  	size_t length;
> > >  	bool is_array;
> > >  	enum dev_prop_type type;
> > >  	union {
> > >  		union {
> > >  			const u8 *u8_data;
> > >  			const u16 *u16_data;
> > >  			const u32 *u32_data;
> > >  			const u64 *u64_data;
> > >  			const char * const *str;
> > >  		} pointer;
> > >  		union {
> > >  			u8 u8_data;
> > >  			u16 u16_data;
> > >  			u32 u32_data;
> > >  			u64 u64_data;
> > >  			const char *str;
> > > +			u8 u8_buf[sizeof(u64)];
> > > +			u16 u16_buf[sizeof(u64)/sizeof(u16)];
> > > +			u32 u32_buf[sizeof(u64)/sizeof(u32)];
> > > +			char char_buf[sizeof(u64)];
> > >  		} value;
> > >  	};
> > >  };
> > > 
> > > to make it clear that the value field is going to be used as an array in
> > > some cases.
> > 
> > Sorry, just sent out updated series before receiving your email. I can
> > cook up new patch cleaning this.
> 
> I'd prefer a new version of the series, honestly.

OK, sure.

> 
> > I think we can drop scalars and only have arrays and have initializers use
> > <type>_data[0] to create initial property entries.
> 
> Why [0]?  IMO it is better to use the exact size (which is known) in this
> particular case.

diff --git a/include/linux/property.h b/include/linux/property.h
index b315fdc0ec28d..b28c81af7bb68 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -257,11 +257,11 @@ struct property_entry {
        union {
                const void *pointer;
                union {
-                       u8 u8_data;
-                       u16 u16_data;
-                       u32 u32_data;
-                       u64 u64_data;
-                       const char *str;
+                       u8 u8_data[sizeof(u64) / sizeof(u8)];
+                       u16 u16_data[sizeof(u64) / sizeof(u16)];
+                       u32 u32_data[sizeof(u64) / sizeof(u32)];
+                       u64 u64_data[sizeof(u64) / sizeof(u64)];
+                       const char *str[sizeof(u64) / sizeof(char *)];
                } value;
        };
 };
@@ -273,7 +273,7 @@ struct property_entry {
  */

 #define __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_)                          \
-       sizeof(((struct property_entry *)NULL)->value._elem_)
+       sizeof(((struct property_entry *)NULL)->value._elem_[0])

 #define __PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_, _elsize_, _Type_,    \
                                          _val_, _len_)                 \
@@ -323,7 +323,7 @@ struct property_entry {
        .length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),                \
        .is_inline = true,                                              \
        .type = DEV_PROP_##_Type_,                                      \
-       { .value = { ._elem_ = _val_ } },                               \
+       { .value = { ._elem_[0] = _val_ } },                            \
 }

 #define PROPERTY_ENTRY_U8(_name_, _val_)                               \

> 
> Also note that u64 is naturally a scalar only.

It still can be expressed as array of 1 element.

>  
> > > 
> > > > In the mean time, can you please consider patches 12-14?
> > > 
> > > I cannot find drivers/platform/x86/intel_cht_int33fe_typec.c in the mainline,
> > > so I cannot apply patch [13/15] now and I'm not sure how useful it would be
> > > to apply patches [10,12/15] without the other two.
> > 
> > Hmm, drivers/platform/x86/intel_cht_int33fe_typec.c used to be
> > drivers/platform/x86/intel_cht_int33fe.c I think.
> > 
> > I can either regenerate against your tree instead of -next (but then
> > there will be merge conflict) or we could postpone #13 and #14 (or #5
> > and #6 in v7) till after merge window.
> > 
> > Please let me know.
> 
> I'd rather postpone the whole series to until the dependencies are in,
> which may be during the merge window (e.g. if this happens during the
> first week of it, waiting for another extra week just for the merge
> window to end is not quite useful IMO).

Hmm, OK, but I am not sure why we can't apply new functionality now and
get cleanup patches in afterwards...

Thanks.

-- 
Dmitry

Re: [PATCH v6 11/15] software node: move small properties inline when copying

[Rafael J. Wysocki]

On Friday, November 8, 2019 1:49:46 AM CET Dmitry Torokhov wrote:
> On Fri, Nov 08, 2019 at 01:45:03AM +0100, Rafael J. Wysocki wrote:
> > On Friday, November 8, 2019 1:28:44 AM CET Dmitry Torokhov wrote:
> > > On Fri, Nov 08, 2019 at 01:04:31AM +0100, Rafael J. Wysocki wrote:
> > > > On Wednesday, November 6, 2019 12:56:56 AM CET Dmitry Torokhov wrote:
> > > > > Hi Rafael,
> > > > > 
> > > > > On Wed, Nov 06, 2019 at 12:42:02AM +0100, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, October 23, 2019 10:02:29 PM CET Dmitry Torokhov wrote:
> > > > > > > When copying/duplicating set of properties, move smaller properties that
> > > > > > > were stored separately directly inside property entry structures. We can
> > > > > > > move:
> > > > > > > 
> > > > > > > - up to 8 bytes from U8 arrays
> > > > > > > - up to 4 words
> > > > > > > - up to 2 double words
> > > > > > > - one U64 value
> > > > > > > - one or 2 strings.
> > > > > > 
> > > > > > Yes, we can do that, but how much of a difference does this really make?
> > > > > 
> > > > > Arguably not much I think, but it was pretty cheap to do.
> > > > > 
> > > > > > 
> > > > > > Also, how can one distinguish between a single-value property and an inline
> > > > > > array which this change?  By looking at the length?
> > > > > 
> > > > > We do not really need to distinguish between the 2. The device
> > > > > properties API is typically wrap single values around arrays (i.e. it is
> > > > > perfectly fine to use scalar API to fetch first element of array and use
> > > > > array API to fetch a scalar). So we have property of certain type with
> > > > > certain number of elements, and it can either be stored inside
> > > > > property_entry structure, or outside of it. They are 2 orthogonal
> > > > > concepts.
> > > > > 
> > > > > > 
> > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > > > > ---
> > > > > > >  drivers/base/swnode.c | 10 ++++++++++
> > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > > > > > > index 18a30fb3cc58..49e1108aa4b7 100644
> > > > > > > --- a/drivers/base/swnode.c
> > > > > > > +++ b/drivers/base/swnode.c
> > > > > > > @@ -280,6 +280,16 @@ static int property_entry_copy_data(struct property_entry *dst,
> > > > > > >  	if (!dst->name)
> > > > > > >  		goto out_free_data;
> > > > > > >  
> > > > > > > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > > > > > > +		/* We have an opportunity to move the data inline */
> > > > > > > +		const void *tmp = dst->pointer;
> > > > > > > +
> > > > > > > +		memcpy(&dst->value, tmp, dst->length);
> > > > > > > +		dst->is_inline = true;
> > > > > > > +
> > > > > > > +		kfree(tmp);
> > > > > > 
> > > > > > This would have been more useful if we had been able to avoid making the
> > > > > > allocation altogether.
> > > > > 
> > > > > OK, I can do that and re-send this patch and the one with the tests.
> > > > 
> > > > But if you do that, IMO it would be prudent to extend the definition of
> > > > struct property_entry like this:
> > > > 
> > > >  struct property_entry {
> > > >  	const char *name;
> > > >  	size_t length;
> > > >  	bool is_array;
> > > >  	enum dev_prop_type type;
> > > >  	union {
> > > >  		union {
> > > >  			const u8 *u8_data;
> > > >  			const u16 *u16_data;
> > > >  			const u32 *u32_data;
> > > >  			const u64 *u64_data;
> > > >  			const char * const *str;
> > > >  		} pointer;
> > > >  		union {
> > > >  			u8 u8_data;
> > > >  			u16 u16_data;
> > > >  			u32 u32_data;
> > > >  			u64 u64_data;
> > > >  			const char *str;
> > > > +			u8 u8_buf[sizeof(u64)];
> > > > +			u16 u16_buf[sizeof(u64)/sizeof(u16)];
> > > > +			u32 u32_buf[sizeof(u64)/sizeof(u32)];
> > > > +			char char_buf[sizeof(u64)];
> > > >  		} value;
> > > >  	};
> > > >  };
> > > > 
> > > > to make it clear that the value field is going to be used as an array in
> > > > some cases.
> > > 
> > > Sorry, just sent out updated series before receiving your email. I can
> > > cook up new patch cleaning this.
> > 
> > I'd prefer a new version of the series, honestly.
> 
> OK, sure.
> 
> > 
> > > I think we can drop scalars and only have arrays and have initializers use
> > > <type>_data[0] to create initial property entries.
> > 
> > Why [0]?  IMO it is better to use the exact size (which is known) in this
> > particular case.
> 
> diff --git a/include/linux/property.h b/include/linux/property.h
> index b315fdc0ec28d..b28c81af7bb68 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -257,11 +257,11 @@ struct property_entry {
>         union {
>                 const void *pointer;
>                 union {
> -                       u8 u8_data;
> -                       u16 u16_data;
> -                       u32 u32_data;
> -                       u64 u64_data;
> -                       const char *str;
> +                       u8 u8_data[sizeof(u64) / sizeof(u8)];
> +                       u16 u16_data[sizeof(u64) / sizeof(u16)];
> +                       u32 u32_data[sizeof(u64) / sizeof(u32)];
> +                       u64 u64_data[sizeof(u64) / sizeof(u64)];

IMO with a scalar u64 this kind of would explain itself, but with a u64 array
it becomes somewhat confusing.

> +                       const char *str[sizeof(u64) / sizeof(char *)];
>                 } value;
>         };
>  };
> @@ -273,7 +273,7 @@ struct property_entry {
>   */
> 
>  #define __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_)                          \
> -       sizeof(((struct property_entry *)NULL)->value._elem_)
> +       sizeof(((struct property_entry *)NULL)->value._elem_[0])
> 
>  #define __PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_, _elsize_, _Type_,    \
>                                           _val_, _len_)                 \
> @@ -323,7 +323,7 @@ struct property_entry {
>         .length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),                \
>         .is_inline = true,                                              \
>         .type = DEV_PROP_##_Type_,                                      \
> -       { .value = { ._elem_ = _val_ } },                               \
> +       { .value = { ._elem_[0] = _val_ } },                            \
>  }
> 
>  #define PROPERTY_ENTRY_U8(_name_, _val_)                               \
> 
> > 
> > Also note that u64 is naturally a scalar only.
> 
> It still can be expressed as array of 1 element.

It can, but for what purpose?

> >  
> > > > 
> > > > > In the mean time, can you please consider patches 12-14?
> > > > 
> > > > I cannot find drivers/platform/x86/intel_cht_int33fe_typec.c in the mainline,
> > > > so I cannot apply patch [13/15] now and I'm not sure how useful it would be
> > > > to apply patches [10,12/15] without the other two.
> > > 
> > > Hmm, drivers/platform/x86/intel_cht_int33fe_typec.c used to be
> > > drivers/platform/x86/intel_cht_int33fe.c I think.
> > > 
> > > I can either regenerate against your tree instead of -next (but then
> > > there will be merge conflict) or we could postpone #13 and #14 (or #5
> > > and #6 in v7) till after merge window.
> > > 
> > > Please let me know.
> > 
> > I'd rather postpone the whole series to until the dependencies are in,
> > which may be during the merge window (e.g. if this happens during the
> > first week of it, waiting for another extra week just for the merge
> > window to end is not quite useful IMO).
> 
> Hmm, OK, but I am not sure why we can't apply new functionality now and
> get cleanup patches in afterwards...

Because that's harder to follow from the history perspective and makes no
real difference in the end of the day.

Re: [PATCH v6 11/15] software node: move small properties inline when copying

[Dmitry Torokhov]

On Fri, Nov 08, 2019 at 02:34:48AM +0100, Rafael J. Wysocki wrote:
> On Friday, November 8, 2019 1:49:46 AM CET Dmitry Torokhov wrote:
> > On Fri, Nov 08, 2019 at 01:45:03AM +0100, Rafael J. Wysocki wrote:
> > > On Friday, November 8, 2019 1:28:44 AM CET Dmitry Torokhov wrote:
> > > > On Fri, Nov 08, 2019 at 01:04:31AM +0100, Rafael J. Wysocki wrote:
> > > > > On Wednesday, November 6, 2019 12:56:56 AM CET Dmitry Torokhov wrote:
> > > > > > Hi Rafael,
> > > > > > 
> > > > > > On Wed, Nov 06, 2019 at 12:42:02AM +0100, Rafael J. Wysocki wrote:
> > > > > > > On Wednesday, October 23, 2019 10:02:29 PM CET Dmitry Torokhov wrote:
> > > > > > > > When copying/duplicating set of properties, move smaller properties that
> > > > > > > > were stored separately directly inside property entry structures. We can
> > > > > > > > move:
> > > > > > > > 
> > > > > > > > - up to 8 bytes from U8 arrays
> > > > > > > > - up to 4 words
> > > > > > > > - up to 2 double words
> > > > > > > > - one U64 value
> > > > > > > > - one or 2 strings.
> > > > > > > 
> > > > > > > Yes, we can do that, but how much of a difference does this really make?
> > > > > > 
> > > > > > Arguably not much I think, but it was pretty cheap to do.
> > > > > > 
> > > > > > > 
> > > > > > > Also, how can one distinguish between a single-value property and an inline
> > > > > > > array which this change?  By looking at the length?
> > > > > > 
> > > > > > We do not really need to distinguish between the 2. The device
> > > > > > properties API is typically wrap single values around arrays (i.e. it is
> > > > > > perfectly fine to use scalar API to fetch first element of array and use
> > > > > > array API to fetch a scalar). So we have property of certain type with
> > > > > > certain number of elements, and it can either be stored inside
> > > > > > property_entry structure, or outside of it. They are 2 orthogonal
> > > > > > concepts.
> > > > > > 
> > > > > > > 
> > > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > > > > > ---
> > > > > > > >  drivers/base/swnode.c | 10 ++++++++++
> > > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > > > > > > > index 18a30fb3cc58..49e1108aa4b7 100644
> > > > > > > > --- a/drivers/base/swnode.c
> > > > > > > > +++ b/drivers/base/swnode.c
> > > > > > > > @@ -280,6 +280,16 @@ static int property_entry_copy_data(struct property_entry *dst,
> > > > > > > >  	if (!dst->name)
> > > > > > > >  		goto out_free_data;
> > > > > > > >  
> > > > > > > > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > > > > > > > +		/* We have an opportunity to move the data inline */
> > > > > > > > +		const void *tmp = dst->pointer;
> > > > > > > > +
> > > > > > > > +		memcpy(&dst->value, tmp, dst->length);
> > > > > > > > +		dst->is_inline = true;
> > > > > > > > +
> > > > > > > > +		kfree(tmp);
> > > > > > > 
> > > > > > > This would have been more useful if we had been able to avoid making the
> > > > > > > allocation altogether.
> > > > > > 
> > > > > > OK, I can do that and re-send this patch and the one with the tests.
> > > > > 
> > > > > But if you do that, IMO it would be prudent to extend the definition of
> > > > > struct property_entry like this:
> > > > > 
> > > > >  struct property_entry {
> > > > >  	const char *name;
> > > > >  	size_t length;
> > > > >  	bool is_array;
> > > > >  	enum dev_prop_type type;
> > > > >  	union {
> > > > >  		union {
> > > > >  			const u8 *u8_data;
> > > > >  			const u16 *u16_data;
> > > > >  			const u32 *u32_data;
> > > > >  			const u64 *u64_data;
> > > > >  			const char * const *str;
> > > > >  		} pointer;
> > > > >  		union {
> > > > >  			u8 u8_data;
> > > > >  			u16 u16_data;
> > > > >  			u32 u32_data;
> > > > >  			u64 u64_data;
> > > > >  			const char *str;
> > > > > +			u8 u8_buf[sizeof(u64)];
> > > > > +			u16 u16_buf[sizeof(u64)/sizeof(u16)];
> > > > > +			u32 u32_buf[sizeof(u64)/sizeof(u32)];
> > > > > +			char char_buf[sizeof(u64)];
> > > > >  		} value;
> > > > >  	};
> > > > >  };
> > > > > 
> > > > > to make it clear that the value field is going to be used as an array in
> > > > > some cases.
> > > > 
> > > > Sorry, just sent out updated series before receiving your email. I can
> > > > cook up new patch cleaning this.
> > > 
> > > I'd prefer a new version of the series, honestly.
> > 
> > OK, sure.
> > 
> > > 
> > > > I think we can drop scalars and only have arrays and have initializers use
> > > > <type>_data[0] to create initial property entries.
> > > 
> > > Why [0]?  IMO it is better to use the exact size (which is known) in this
> > > particular case.
> > 
> > diff --git a/include/linux/property.h b/include/linux/property.h
> > index b315fdc0ec28d..b28c81af7bb68 100644
> > --- a/include/linux/property.h
> > +++ b/include/linux/property.h
> > @@ -257,11 +257,11 @@ struct property_entry {
> >         union {
> >                 const void *pointer;
> >                 union {
> > -                       u8 u8_data;
> > -                       u16 u16_data;
> > -                       u32 u32_data;
> > -                       u64 u64_data;
> > -                       const char *str;
> > +                       u8 u8_data[sizeof(u64) / sizeof(u8)];
> > +                       u16 u16_data[sizeof(u64) / sizeof(u16)];
> > +                       u32 u32_data[sizeof(u64) / sizeof(u32)];
> > +                       u64 u64_data[sizeof(u64) / sizeof(u64)];
> 
> IMO with a scalar u64 this kind of would explain itself, but with a u64 array
> it becomes somewhat confusing.
> 
> > +                       const char *str[sizeof(u64) / sizeof(char *)];
> >                 } value;
> >         };
> >  };
> > @@ -273,7 +273,7 @@ struct property_entry {
> >   */
> > 
> >  #define __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_)                          \
> > -       sizeof(((struct property_entry *)NULL)->value._elem_)
> > +       sizeof(((struct property_entry *)NULL)->value._elem_[0])
> > 
> >  #define __PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_, _elsize_, _Type_,    \
> >                                           _val_, _len_)                 \
> > @@ -323,7 +323,7 @@ struct property_entry {
> >         .length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),                \
> >         .is_inline = true,                                              \
> >         .type = DEV_PROP_##_Type_,                                      \
> > -       { .value = { ._elem_ = _val_ } },                               \
> > +       { .value = { ._elem_[0] = _val_ } },                            \
> >  }
> > 
> >  #define PROPERTY_ENTRY_U8(_name_, _val_)                               \
> > 
> > > 
> > > Also note that u64 is naturally a scalar only.
> > 
> > It still can be expressed as array of 1 element.
> 
> It can, but for what purpose?

Just so we do not have to special-case handling of U64 in
PROPERTY_ENTRY_Unnn() macros, as I shown in the snippet above.

-- 
Dmitry

[PATCH v6 12/15] software node: implement reference properties

[Dmitry Torokhov]

It is possible to store references to software nodes in the same fashion as
other static properties, so that users do not need to define separate
structures:

static const struct software_node gpio_bank_b_node = {
	.name = "B",
};

static const struct property_entry simone_key_enter_props[] = {
	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
	PROPERTY_ENTRY_STRING("label", "enter"),
	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
	{ }
};

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 48 +++++++++++++++++++++++++++------
 include/linux/property.h | 57 +++++++++++++++++++++++++++++-----------
 2 files changed, 81 insertions(+), 24 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 49e1108aa4b7..8fb4dd532433 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -262,6 +262,12 @@ static int property_entry_copy_data(struct property_entry *dst,
 		}
 
 		dst->pointer = new;
+	} else if (src->type == DEV_PROP_REF) {
+		/*
+		 * Reference properties are never stored inline as
+		 * they are too big.
+		 */
+		return -EINVAL;
 	} else if (src->type == DEV_PROP_STRING) {
 		new = kstrdup(src->value.str, GFP_KERNEL);
 		if (!new && src->value.str)
@@ -492,21 +498,47 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 {
 	struct swnode *swnode = to_swnode(fwnode);
 	const struct software_node_reference *ref;
+	const struct software_node_ref_args *ref_array;
+	const struct software_node_ref_args *ref_args;
 	const struct property_entry *prop;
 	struct fwnode_handle *refnode;
 	int i;
 
-	if (!swnode || !swnode->node->references)
+	if (!swnode)
 		return -ENOENT;
 
-	for (ref = swnode->node->references; ref->name; ref++)
-		if (!strcmp(ref->name, propname))
-			break;
+	prop = property_entry_get(swnode->node->properties, propname);
+	if (prop) {
+		if (prop->type != DEV_PROP_REF)
+			return -EINVAL;
 
-	if (!ref->name || index > (ref->nrefs - 1))
-		return -ENOENT;
+		/*
+		 * We expect that references are never stored inline, even
+		 * single ones, as they are too big.
+		 */
+		if (prop->is_inline)
+			return -EINVAL;
+
+		if (index * sizeof(*ref_args) >= prop->length)
+			return -ENOENT;
+
+		ref_array = prop->pointer;
+		ref_args = &ref_array[index];
+	} else {
+		if (!swnode->node->references)
+			return -ENOENT;
+
+		for (ref = swnode->node->references; ref->name; ref++)
+			if (!strcmp(ref->name, propname))
+				break;
+
+		if (!ref->name || index > (ref->nrefs - 1))
+			return -ENOENT;
+
+		ref_args = &ref->refs[index];
+	}
 
-	refnode = software_node_fwnode(ref->refs[index].node);
+	refnode = software_node_fwnode(ref_args->node);
 	if (!refnode)
 		return -ENOENT;
 
@@ -525,7 +557,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	args->nargs = nargs;
 
 	for (i = 0; i < nargs; i++)
-		args->args[i] = ref->refs[index].args[i];
+		args->args[i] = ref_args->args[i];
 
 	return 0;
 }
diff --git a/include/linux/property.h b/include/linux/property.h
index dad0ad11b55e..68655c094b39 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -22,6 +22,7 @@ enum dev_prop_type {
 	DEV_PROP_U32,
 	DEV_PROP_U64,
 	DEV_PROP_STRING,
+	DEV_PROP_REF,
 };
 
 enum dev_dma_attr {
@@ -223,6 +224,20 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
 	return fwnode_property_read_u64_array(fwnode, propname, NULL, 0);
 }
 
+struct software_node;
+
+/**
+ * struct software_node_ref_args - Reference property with additional arguments
+ * @node: Reference to a software node
+ * @nargs: Number of elements in @args array
+ * @args: Integer arguments
+ */
+struct software_node_ref_args {
+	const struct software_node *node;
+	unsigned int nargs;
+	u64 args[NR_FWNODE_REFERENCE_ARGS];
+};
+
 /**
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
@@ -260,14 +275,20 @@ struct property_entry {
 #define __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_)				\
 	sizeof(((struct property_entry *)NULL)->value._elem_)
 
-#define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _elem_, _Type_, _val_, _len_)\
+#define __PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_, _elsize_, _Type_,	\
+					  _val_, _len_)			\
 (struct property_entry) {						\
 	.name = _name_,							\
-	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
+	.length = (_len_) * (_elsize_),					\
 	.type = DEV_PROP_##_Type_,					\
 	{ .pointer = _val_ },						\
 }
 
+#define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _elem_, _Type_, _val_, _len_)\
+	__PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_,			\
+				__PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
+				_Type_, _val_, _len_)
+
 #define PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, _len_)		\
 	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u8_data, U8, _val_, _len_)
 #define PROPERTY_ENTRY_U16_ARRAY_LEN(_name_, _val_, _len_)		\
@@ -278,6 +299,10 @@ struct property_entry {
 	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u64_data, U64, _val_, _len_)
 #define PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, _len_)		\
 	__PROPERTY_ENTRY_ARRAY_LEN(_name_, str, STRING, _val_, _len_)
+#define PROPERTY_ENTRY_REF_ARRAY_LEN(_name_, _val_, _len_)		\
+	__PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_,			\
+				sizeof(struct software_node_ref_args),	\
+				REF, _val_, _len_)
 
 #define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)				\
 	PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
@@ -289,6 +314,8 @@ struct property_entry {
 	PROPERTY_ENTRY_U64_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
 #define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_)			\
 	PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
+#define PROPERTY_ENTRY_REF_ARRAY(_name_, _val_)			\
+	PROPERTY_ENTRY_REF_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
 
 #define __PROPERTY_ENTRY_ELEMENT(_name_, _elem_, _Type_, _val_)		\
 (struct property_entry) {						\
@@ -316,6 +343,18 @@ struct property_entry {
 	.is_inline = true,			\
 }
 
+#define PROPERTY_ENTRY_REF(_name_, _ref_, ...)				\
+(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__ },				\
+	} },								\
+}
+
 struct property_entry *
 property_entries_dup(const struct property_entry *properties);
 
@@ -379,20 +418,6 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
-struct software_node;
-
-/**
- * struct software_node_ref_args - Reference with additional arguments
- * @node: Reference to a software node
- * @nargs: Number of elements in @args array
- * @args: Integer arguments
- */
-struct software_node_ref_args {
-	const struct software_node *node;
-	unsigned int nargs;
-	u64 args[NR_FWNODE_REFERENCE_ARGS];
-};
-
 /**
  * struct software_node_reference - Named software node reference property
  * @name: Name of the property
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 13/15] platform/x86: intel_cht_int33fe: use inline reference properties

[Dmitry Torokhov]

Now that static device properties allow defining reference properties
together with all other types of properties, instead of managing them
separately, let's adjust the driver.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 .../platform/x86/intel_cht_int33fe_typec.c    | 81 ++++++++++---------
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe_typec.c b/drivers/platform/x86/intel_cht_int33fe_typec.c
index 2d097fc2dd46..04138215956b 100644
--- a/drivers/platform/x86/intel_cht_int33fe_typec.c
+++ b/drivers/platform/x86/intel_cht_int33fe_typec.c
@@ -36,30 +36,6 @@ enum {
 	INT33FE_NODE_MAX,
 };
 
-static const struct software_node nodes[];
-
-static const struct software_node_ref_args pi3usb30532_ref = {
-	&nodes[INT33FE_NODE_PI3USB30532]
-};
-
-static const struct software_node_ref_args dp_ref = {
-	&nodes[INT33FE_NODE_DISPLAYPORT]
-};
-
-static struct software_node_ref_args mux_ref;
-
-static const struct software_node_reference usb_connector_refs[] = {
-	{ "orientation-switch", 1, &pi3usb30532_ref},
-	{ "mode-switch", 1, &pi3usb30532_ref},
-	{ "displayport", 1, &dp_ref},
-	{ }
-};
-
-static const struct software_node_reference fusb302_refs[] = {
-	{ "usb-role-switch", 1, &mux_ref},
-	{ }
-};
-
 /*
  * Grrr I severly dislike buggy BIOS-es. At least one BIOS enumerates
  * the max17047 both through the INT33FE ACPI device (it is right there
@@ -95,8 +71,18 @@ static const struct property_entry max17047_props[] = {
 	{ }
 };
 
+/*
+ * We are not using inline property here because those are constant,
+ * and we need to adjust this one at runtime to point to real
+ * software node.
+ */
+static struct software_node_ref_args fusb302_mux_refs[] = {
+	{ .node = NULL },
+};
+
 static const struct property_entry fusb302_props[] = {
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
+	PROPERTY_ENTRY_REF_ARRAY("usb-role-switch", fusb302_mux_refs),
 	{ }
 };
 
@@ -112,6 +98,8 @@ static const u32 snk_pdo[] = {
 	PDO_VAR(5000, 12000, 3000),
 };
 
+static const struct software_node nodes[];
+
 static const struct property_entry usb_connector_props[] = {
 	PROPERTY_ENTRY_STRING("data-role", "dual"),
 	PROPERTY_ENTRY_STRING("power-role", "dual"),
@@ -119,15 +107,21 @@ static const struct property_entry usb_connector_props[] = {
 	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
 	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
 	PROPERTY_ENTRY_U32("op-sink-microwatt", 2500000),
+	PROPERTY_ENTRY_REF("orientation-switch",
+			   &nodes[INT33FE_NODE_PI3USB30532]),
+	PROPERTY_ENTRY_REF("mode-switch",
+			   &nodes[INT33FE_NODE_PI3USB30532]),
+	PROPERTY_ENTRY_REF("displayport",
+			   &nodes[INT33FE_NODE_DISPLAYPORT]),
 	{ }
 };
 
 static const struct software_node nodes[] = {
-	{ "fusb302", NULL, fusb302_props, fusb302_refs },
+	{ "fusb302", NULL, fusb302_props },
 	{ "max17047", NULL, max17047_props },
 	{ "pi3usb30532" },
 	{ "displayport" },
-	{ "connector", &nodes[0], usb_connector_props, usb_connector_refs },
+	{ "connector", &nodes[0], usb_connector_props },
 	{ }
 };
 
@@ -163,9 +157,10 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 {
 	software_node_unregister_nodes(nodes);
 
-	if (mux_ref.node) {
-		fwnode_handle_put(software_node_fwnode(mux_ref.node));
-		mux_ref.node = NULL;
+	if (fusb302_mux_refs[0].node) {
+		fwnode_handle_put(
+			software_node_fwnode(fusb302_mux_refs[0].node));
+		fusb302_mux_refs[0].node = NULL;
 	}
 
 	if (data->dp) {
@@ -177,25 +172,31 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 
 static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
 {
+	const struct software_node *mux_ref_node;
 	int ret;
 
-	ret = software_node_register_nodes(nodes);
-	if (ret)
-		return ret;
-
-	/* The devices that are not created in this driver need extra steps. */
-
 	/*
 	 * There is no ACPI device node for the USB role mux, so we need to wait
 	 * until the mux driver has created software node for the mux device.
 	 * It means we depend on the mux driver. This function will return
 	 * -EPROBE_DEFER until the mux device is registered.
 	 */
-	mux_ref.node = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
-	if (!mux_ref.node) {
-		ret = -EPROBE_DEFER;
-		goto err_remove_nodes;
-	}
+	mux_ref_node = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
+	if (!mux_ref_node)
+		return -EPROBE_DEFER;
+
+	/*
+	 * Update node used in "usb-role-switch" property. Note that we
+	 * rely on software_node_register_nodes() to use the original
+	 * instance of properties instead of copying them.
+	 */
+	fusb302_mux_refs[0].node = mux_ref_node;
+
+	ret = software_node_register_nodes(nodes);
+	if (ret)
+		return ret;
+
+	/* The devices that are not created in this driver need extra steps. */
 
 	/*
 	 * The DP connector does have ACPI device node. In this case we can just
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 14/15] software node: remove separate handling of references

[Dmitry Torokhov]

Now that all users of references have moved to reference properties,
we can remove separate handling of references.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 46 +++++++++++++++-------------------------
 include/linux/property.h | 14 ------------
 2 files changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 8fb4dd532433..87c715648eb6 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -497,9 +497,8 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 				 struct fwnode_reference_args *args)
 {
 	struct swnode *swnode = to_swnode(fwnode);
-	const struct software_node_reference *ref;
 	const struct software_node_ref_args *ref_array;
-	const struct software_node_ref_args *ref_args;
+	const struct software_node_ref_args *ref;
 	const struct property_entry *prop;
 	struct fwnode_handle *refnode;
 	int i;
@@ -508,37 +507,26 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 		return -ENOENT;
 
 	prop = property_entry_get(swnode->node->properties, propname);
-	if (prop) {
-		if (prop->type != DEV_PROP_REF)
-			return -EINVAL;
-
-		/*
-		 * We expect that references are never stored inline, even
-		 * single ones, as they are too big.
-		 */
-		if (prop->is_inline)
-			return -EINVAL;
-
-		if (index * sizeof(*ref_args) >= prop->length)
-			return -ENOENT;
+	if (!prop)
+		return -ENOENT;
 
-		ref_array = prop->pointer;
-		ref_args = &ref_array[index];
-	} else {
-		if (!swnode->node->references)
-			return -ENOENT;
+	if (prop->type != DEV_PROP_REF)
+		return -EINVAL;
 
-		for (ref = swnode->node->references; ref->name; ref++)
-			if (!strcmp(ref->name, propname))
-				break;
+	/*
+	 * We expect that references are never stored inline, even
+	 * single ones, as they are too big.
+	 */
+	if (prop->is_inline)
+		return -EINVAL;
 
-		if (!ref->name || index > (ref->nrefs - 1))
-			return -ENOENT;
+	if (index * sizeof(*ref) >= prop->length)
+		return -ENOENT;
 
-		ref_args = &ref->refs[index];
-	}
+	ref_array = prop->pointer;
+	ref = &ref_array[index];
 
-	refnode = software_node_fwnode(ref_args->node);
+	refnode = software_node_fwnode(ref->node);
 	if (!refnode)
 		return -ENOENT;
 
@@ -557,7 +545,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	args->nargs = nargs;
 
 	for (i = 0; i < nargs; i++)
-		args->args[i] = ref_args->args[i];
+		args->args[i] = ref->args[i];
 
 	return 0;
 }
diff --git a/include/linux/property.h b/include/linux/property.h
index 68655c094b39..b315fdc0ec28 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -418,30 +418,16 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
-/**
- * struct software_node_reference - Named software node reference property
- * @name: Name of the property
- * @nrefs: Number of elements in @refs array
- * @refs: Array of references with optional arguments
- */
-struct software_node_reference {
-	const char *name;
-	unsigned int nrefs;
-	const struct software_node_ref_args *refs;
-};
-
 /**
  * struct software_node - Software node description
  * @name: Name of the software node
  * @parent: Parent of the software node
  * @properties: Array of device properties
- * @references: Array of software node reference properties
  */
 struct software_node {
 	const char *name;
 	const struct software_node *parent;
 	const struct property_entry *properties;
-	const struct software_node_reference *references;
 };
 
 bool is_software_node(const struct fwnode_handle *fwnode);
-- 
2.23.0.866.gb869b98d4c-goog

[PATCH v6 15/15] software node: add basic tests for property entries

[Dmitry Torokhov]

This adds tests for creating software nodes with properties supplied by
PROPERTY_ENTRY_XXX() macros and fetching and validating data from said
nodes/properties.

We are using KUnit framework for the tests.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/test/Makefile              |   2 +
 drivers/base/test/property-entry-test.c | 472 ++++++++++++++++++++++++
 2 files changed, 474 insertions(+)
 create mode 100644 drivers/base/test/property-entry-test.c

diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
index 0f1f7277a013..22143102e5d2 100644
--- a/drivers/base/test/Makefile
+++ b/drivers/base/test/Makefile
@@ -1,2 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)	+= test_async_driver_probe.o
+
+obj-$(CONFIG_KUNIT) += property-entry-test.o
diff --git a/drivers/base/test/property-entry-test.c b/drivers/base/test/property-entry-test.c
new file mode 100644
index 000000000000..d6ac84c8ebd0
--- /dev/null
+++ b/drivers/base/test/property-entry-test.c
@@ -0,0 +1,472 @@
+// SPDX-License-Identifier: GPL-2.0
+// Unit tests for property entries API
+//
+// Copyright 2019 Google LLC.
+
+#include <kunit/test.h>
+#include <linux/property.h>
+#include <linux/types.h>
+
+static void pe_test_uints(struct kunit *test)
+{
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_U8("prop-u8", 8),
+		PROPERTY_ENTRY_U16("prop-u16", 16),
+		PROPERTY_ENTRY_U32("prop-u32", 32),
+		PROPERTY_ENTRY_U64("prop-u64", 64),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+	u8 val_u8, array_u8[2];
+	u16 val_u16, array_u16[2];
+	u32 val_u32, array_u32[2];
+	u64 val_u64, array_u64[2];
+	int error;
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	error = fwnode_property_read_u8(node, "prop-u8", &val_u8);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u8, 8);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u8[0], 8);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 2);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u8(node, "no-prop-u8", &val_u8);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u8_array(node, "no-prop-u8", array_u8, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16(node, "prop-u16", &val_u16);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u16, 16);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u16[0], 16);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 2);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16(node, "no-prop-u16", &val_u16);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16_array(node, "no-prop-u16", array_u16, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32(node, "prop-u32", &val_u32);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u32, 32);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u32[0], 32);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 2);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32(node, "no-prop-u32", &val_u32);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32_array(node, "no-prop-u32", array_u32, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64(node, "prop-u64", &val_u64);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u64, 64);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u64[0], 64);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 2);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64(node, "no-prop-u64", &val_u64);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64_array(node, "no-prop-u64", array_u64, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	fwnode_remove_software_node(node);
+}
+
+static void pe_test_uint_arrays(struct kunit *test)
+{
+	u8 a_u8[16] = { 8, 9 };
+	u16 a_u16[16] = { 16, 17 };
+	u32 a_u32[16] = { 32, 33 };
+	u64 a_u64[16] = { 64, 65 };
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_U8_ARRAY("prop-u8", a_u8),
+		PROPERTY_ENTRY_U16_ARRAY("prop-u16", a_u16),
+		PROPERTY_ENTRY_U32_ARRAY("prop-u32", a_u32),
+		PROPERTY_ENTRY_U64_ARRAY("prop-u64", a_u64),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+	u8 val_u8, array_u8[32];
+	u16 val_u16, array_u16[32];
+	u32 val_u32, array_u32[32];
+	u64 val_u64, array_u64[32];
+	int error;
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	error = fwnode_property_read_u8(node, "prop-u8", &val_u8);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u8, 8);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u8[0], 8);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 2);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u8[0], 8);
+	KUNIT_EXPECT_EQ(test, (int)array_u8[1], 9);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 17);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u8(node, "no-prop-u8", &val_u8);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u8_array(node, "no-prop-u8", array_u8, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16(node, "prop-u16", &val_u16);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u16, 16);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u16[0], 16);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 2);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u16[0], 16);
+	KUNIT_EXPECT_EQ(test, (int)array_u16[1], 17);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 17);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16(node, "no-prop-u16", &val_u16);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16_array(node, "no-prop-u16", array_u16, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32(node, "prop-u32", &val_u32);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u32, 32);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u32[0], 32);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 2);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u32[0], 32);
+	KUNIT_EXPECT_EQ(test, (int)array_u32[1], 33);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 17);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32(node, "no-prop-u32", &val_u32);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32_array(node, "no-prop-u32", array_u32, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64(node, "prop-u64", &val_u64);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u64, 64);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u64[0], 64);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 2);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u64[0], 64);
+	KUNIT_EXPECT_EQ(test, (int)array_u64[1], 65);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 17);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64(node, "no-prop-u64", &val_u64);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64_array(node, "no-prop-u64", array_u64, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	fwnode_remove_software_node(node);
+}
+
+static void pe_test_strings(struct kunit *test)
+{
+	const char *strings[] = {
+		"string-a",
+		"string-b",
+	};
+
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_STRING("str", "single"),
+		PROPERTY_ENTRY_STRING("empty", ""),
+		PROPERTY_ENTRY_STRING_ARRAY("strs", strings),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+	const char *str;
+	const char *strs[10];
+	int error;
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	error = fwnode_property_read_string(node, "str", &str);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_STREQ(test, str, "single");
+
+	error = fwnode_property_read_string_array(node, "str", strs, 1);
+	KUNIT_EXPECT_EQ(test, error, 1);
+	KUNIT_EXPECT_STREQ(test, strs[0], "single");
+
+	/* asking for more data returns what we have */
+	error = fwnode_property_read_string_array(node, "str", strs, 2);
+	KUNIT_EXPECT_EQ(test, error, 1);
+	KUNIT_EXPECT_STREQ(test, strs[0], "single");
+
+	error = fwnode_property_read_string(node, "no-str", &str);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_string_array(node, "no-str", strs, 1);
+	KUNIT_EXPECT_LT(test, error, 0);
+
+	error = fwnode_property_read_string(node, "empty", &str);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_STREQ(test, str, "");
+
+	error = fwnode_property_read_string_array(node, "strs", strs, 3);
+	KUNIT_EXPECT_EQ(test, error, 2);
+	KUNIT_EXPECT_STREQ(test, strs[0], "string-a");
+	KUNIT_EXPECT_STREQ(test, strs[1], "string-b");
+
+	error = fwnode_property_read_string_array(node, "strs", strs, 1);
+	KUNIT_EXPECT_EQ(test, error, 1);
+	KUNIT_EXPECT_STREQ(test, strs[0], "string-a");
+
+	/* NULL argument -> returns size */
+	error = fwnode_property_read_string_array(node, "strs", NULL, 0);
+	KUNIT_EXPECT_EQ(test, error, 2);
+
+	/* accessing array as single value */
+	error = fwnode_property_read_string(node, "strs", &str);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_STREQ(test, str, "string-a");
+
+	fwnode_remove_software_node(node);
+}
+
+static void pe_test_bool(struct kunit *test)
+{
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_BOOL("prop"),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	KUNIT_EXPECT_TRUE(test, fwnode_property_read_bool(node, "prop"));
+	KUNIT_EXPECT_FALSE(test, fwnode_property_read_bool(node, "not-prop"));
+
+	fwnode_remove_software_node(node);
+}
+
+/* Verifies that small U8 array is stored inline when property is copied */
+static void pe_test_move_inline_u8(struct kunit *test)
+{
+	u8 u8_array_small[8] = { 1, 2, 3, 4 };
+	u8 u8_array_big[128] = { 5, 6, 7, 8 };
+	struct property_entry entries[] = {
+		PROPERTY_ENTRY_U8_ARRAY("small", u8_array_small),
+		PROPERTY_ENTRY_U8_ARRAY("big", u8_array_big),
+		{ }
+	};
+	struct property_entry *copy;
+	const u8 *data_ptr;
+
+	copy = property_entries_dup(entries);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, copy);
+
+	KUNIT_EXPECT_TRUE(test, copy[0].is_inline);
+	data_ptr = (u8 *)&copy[0].value;
+	KUNIT_EXPECT_EQ(test, (int)data_ptr[0], 1);
+	KUNIT_EXPECT_EQ(test, (int)data_ptr[1], 2);
+
+	KUNIT_EXPECT_FALSE(test, copy[1].is_inline);
+	data_ptr = copy[1].pointer;
+	KUNIT_EXPECT_EQ(test, (int)data_ptr[0], 5);
+	KUNIT_EXPECT_EQ(test, (int)data_ptr[1], 6);
+
+	property_entries_free(copy);
+}
+
+/* Verifies that single string array is stored inline when property is copied */
+static void pe_test_move_inline_str(struct kunit *test)
+{
+	char *str_array_small[] = { "a" };
+	char *str_array_big[] = { "b", "c", "d", "e" };
+	char *str_array_small_empty[] = { "" };
+	struct property_entry entries[] = {
+		PROPERTY_ENTRY_STRING_ARRAY("small", str_array_small),
+		PROPERTY_ENTRY_STRING_ARRAY("big", str_array_big),
+		PROPERTY_ENTRY_STRING_ARRAY("small-empty", str_array_small_empty),
+		{ }
+	};
+	struct property_entry *copy;
+	const char * const *data_ptr;
+
+	copy = property_entries_dup(entries);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, copy);
+
+	KUNIT_EXPECT_TRUE(test, copy[0].is_inline);
+	KUNIT_EXPECT_STREQ(test, copy[0].value.str, "a");
+
+	KUNIT_EXPECT_FALSE(test, copy[1].is_inline);
+	data_ptr = copy[1].pointer;
+	KUNIT_EXPECT_STREQ(test, data_ptr[0], "b");
+	KUNIT_EXPECT_STREQ(test, data_ptr[1], "c");
+
+	KUNIT_EXPECT_TRUE(test, copy[2].is_inline);
+	KUNIT_EXPECT_STREQ(test, copy[2].value.str, "");
+
+	property_entries_free(copy);
+}
+
+/* Handling of reference properties */
+static void pe_test_reference(struct kunit *test)
+{
+	const struct software_node nodes[] = {
+		{ .name = "1", },
+		{ .name = "2", },
+	};
+
+	const struct software_node_ref_args refs[] = {
+		{
+			.node = &nodes[0],
+			.nargs = 0,
+		},
+		{
+			.node = &nodes[1],
+			.nargs = 2,
+			.args = { 3, 4 },
+		},
+	};
+
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_REF("ref-1", &nodes[0]),
+		PROPERTY_ENTRY_REF("ref-2", &nodes[1], 1, 2),
+		PROPERTY_ENTRY_REF_ARRAY("ref-3", refs),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+	struct fwnode_reference_args ref;
+	int error;
+
+	error = software_node_register_nodes(nodes);
+	KUNIT_ASSERT_EQ(test, error, 0);
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	error = fwnode_property_get_reference_args(node, "ref-1", NULL,
+						   0, 0, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[0]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 0U);
+
+	/* wrong index */
+	error = fwnode_property_get_reference_args(node, "ref-1", NULL,
+						   0, 1, &ref);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_get_reference_args(node, "ref-2", NULL,
+						   1, 0, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[1]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 1U);
+	KUNIT_EXPECT_EQ(test, ref.args[0], 1LLU);
+
+	/* asking for more args, padded with zero data */
+	error = fwnode_property_get_reference_args(node, "ref-2", NULL,
+						   3, 0, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[1]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 3U);
+	KUNIT_EXPECT_EQ(test, ref.args[0], 1LLU);
+	KUNIT_EXPECT_EQ(test, ref.args[1], 2LLU);
+	KUNIT_EXPECT_EQ(test, ref.args[2], 0LLU);
+
+	/* wrong index */
+	error = fwnode_property_get_reference_args(node, "ref-2", NULL,
+						   2, 1, &ref);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	/* array of references */
+	error = fwnode_property_get_reference_args(node, "ref-3", NULL,
+						   0, 0, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[0]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 0U);
+
+	/* second reference in the array */
+	error = fwnode_property_get_reference_args(node, "ref-3", NULL,
+						   2, 1, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[1]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 2U);
+	KUNIT_EXPECT_EQ(test, ref.args[0], 3LLU);
+	KUNIT_EXPECT_EQ(test, ref.args[1], 4LLU);
+
+	/* wrong index */
+	error = fwnode_property_get_reference_args(node, "ref-1", NULL,
+						   0, 2, &ref);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	fwnode_remove_software_node(node);
+	software_node_unregister_nodes(nodes);
+}
+
+static struct kunit_case property_entry_test_cases[] = {
+	KUNIT_CASE(pe_test_uints),
+	KUNIT_CASE(pe_test_uint_arrays),
+	KUNIT_CASE(pe_test_strings),
+	KUNIT_CASE(pe_test_bool),
+	KUNIT_CASE(pe_test_move_inline_u8),
+	KUNIT_CASE(pe_test_move_inline_str),
+	KUNIT_CASE(pe_test_reference),
+	{ }
+};
+
+static struct kunit_suite property_entry_test_suite = {
+	.name = "property-entry",
+	.test_cases = property_entry_test_cases,
+};
+
+kunit_test_suite(property_entry_test_suite);
-- 
2.23.0.866.gb869b98d4c-goog

Re: [PATCH v6 00/15] software node: add support for reference properties

[Dmitry Torokhov]

Hi Rafael,

On Wed, Oct 23, 2019 at 01:02:18PM -0700, Dmitry Torokhov wrote:
> These series implement "references" properties for software nodes as true
> properties, instead of managing them completely separately.
> 
> The first 10 patches are generic cleanups and consolidation and
> unification of the existing code; patch #11 implements moving of small
> properties inline when copying property entries; patch #12 implements
> PROPERTY_ENTRY_REF() and friends; patch #13 converts the user of
> references to the property syntax, and patch #14 removes the remains of
> references as entities that are managed separately.
> 
> Patch #15 adds unit tests to verify that the handling of property
> entries is correct.

Do you have any concerns with the series? I think Andy did all the
reviewing that he could...

Thanks!

> 
> Changes in v6:
> - rebased onto next-20191023
> - fixed patch moving small properties inline
> - fixed handling boolean properties after is_array -> is_inline
>   conversion
> - changed comments around is_inline "stored directly" vs embedded
>   in one place (Andy)
> - added unit tests for property entries based on KUnit framework
> - added Any's reviewed-by/acked-by
> 
> Changes in v5:
> - rebased onto next-20191011
> 
> Changes in v4:
> - dealt with union aliasing concerns
> - inline small properties on copy
> 
> Changes in v3:
> - added various cleanups before implementing reference properties
> 
> Changes in v2:
> - reworked code so that even single-entry reference properties are
>   stored as arrays (i.e. the software_node_ref_args instances are
>   not part of property_entry structure) to avoid size increase.
>   From user's POV nothing is changed, one can still use PROPERTY_ENTRY_REF
>   macro to define reference "inline".
> - dropped unused DEV_PROP_MAX
> - rebased on linux-next
> 
> Dmitry Torokhov (15):
>   software node: remove DEV_PROP_MAX
>   software node: introduce PROPERTY_ENTRY_ARRAY_XXX_LEN()
>   efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN
>   software node: mark internal macros with double underscores
>   software node: clean up property_copy_string_array()
>   software node: get rid of property_set_pointer()
>   software node: remove property_entry_read_uNN_array functions
>   software node: unify PROPERTY_ENTRY_XXX macros
>   software node: simplify property_entry_read_string_array()
>   software node: rename is_array to is_inline
>   software node: move small properties inline when copying
>   software node: implement reference properties
>   platform/x86: intel_cht_int33fe: use inline reference properties
>   software node: remove separate handling of references
>   software node: add basic tests for property entries
> 
>  drivers/base/swnode.c                         | 263 ++++------
>  drivers/base/test/Makefile                    |   2 +
>  drivers/base/test/property-entry-test.c       | 472 ++++++++++++++++++
>  drivers/firmware/efi/apple-properties.c       |  18 +-
>  .../platform/x86/intel_cht_int33fe_typec.c    |  81 +--
>  include/linux/property.h                      | 178 +++----
>  6 files changed, 702 insertions(+), 312 deletions(-)
>  create mode 100644 drivers/base/test/property-entry-test.c
> 
> -- 
> 2.23.0.866.gb869b98d4c-goog
> 

-- 
Dmitry

Re: [PATCH v6 00/15] software node: add support for reference properties

[Rafael J. Wysocki]

On Wed, Oct 30, 2019 at 11:43 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Rafael,
>
> On Wed, Oct 23, 2019 at 01:02:18PM -0700, Dmitry Torokhov wrote:
> > These series implement "references" properties for software nodes as true
> > properties, instead of managing them completely separately.
> >
> > The first 10 patches are generic cleanups and consolidation and
> > unification of the existing code; patch #11 implements moving of small
> > properties inline when copying property entries; patch #12 implements
> > PROPERTY_ENTRY_REF() and friends; patch #13 converts the user of
> > references to the property syntax, and patch #14 removes the remains of
> > references as entities that are managed separately.
> >
> > Patch #15 adds unit tests to verify that the handling of property
> > entries is correct.
>
> Do you have any concerns with the series? I think Andy did all the
> reviewing that he could...

Yes, he did, and he is unconvinced.

I basically have no problems with patches [1-9/15], so I'm going to
queue them up for 5.5.

Patch [10/15] by itself is kind of pointless IMO, so it'll depend on
what happens to the rest.

Patch [11/15] kind of causes brows to rise, so let me reply to it directly.