linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 00/14] software node: add support for reference properties
@ 2019-10-11 23:07 Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 01/14] software node: remove DEV_PROP_MAX Dmitry Torokhov
                   ` (14 more replies)
  0 siblings, 15 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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.

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

 drivers/base/swnode.c                    | 266 ++++++++---------------
 drivers/firmware/efi/apple-properties.c  |  18 +-
 drivers/platform/x86/intel_cht_int33fe.c |  81 +++----
 include/linux/property.h                 | 177 +++++++--------
 4 files changed, 230 insertions(+), 312 deletions(-)

-- 
2.23.0.700.g56cf767bdb-goog


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v5 01/14] software node: remove DEV_PROP_MAX
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 02/14] software node: introduce PROPERTY_ENTRY_ARRAY_XXX_LEN() Dmitry Torokhov
                   ` (13 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 9b3d4ca3a73a..44c1704f7163 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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 02/14] software node: introduce PROPERTY_ENTRY_ARRAY_XXX_LEN()
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 01/14] software node: remove DEV_PROP_MAX Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-15 12:02   ` Andy Shevchenko
  2019-10-11 23:07 ` [PATCH v5 03/14] efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN Dmitry Torokhov
                   ` (12 subsequent siblings)
  14 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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.

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 44c1704f7163..f89b930ca4b7 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -256,33 +256,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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 03/14] efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 01/14] software node: remove DEV_PROP_MAX Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 02/14] software node: introduce PROPERTY_ENTRY_ARRAY_XXX_LEN() Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-15 12:01   ` Andy Shevchenko
  2019-10-11 23:07 ` [PATCH v5 04/14] software node: mark internal macros with double underscores Dmitry Torokhov
                   ` (11 subsequent siblings)
  14 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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.

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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 04/14] software node: mark internal macros with double underscores
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (2 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 03/14] efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-15 12:03   ` Andy Shevchenko
  2019-10-11 23:07 ` [PATCH v5 05/14] software node: clean up property_copy_string_array() Dmitry Torokhov
                   ` (10 subsequent siblings)
  14 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 f89b930ca4b7..2c9d4d209296 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -256,7 +256,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_),				\
@@ -266,13 +266,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) {						\
@@ -294,7 +294,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_),				\
@@ -303,13 +303,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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 05/14] software node: clean up property_copy_string_array()
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (3 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 04/14] software node: mark internal macros with double underscores Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-15 12:07   ` Andy Shevchenko
  2019-10-11 23:07 ` [PATCH v5 06/14] software node: get rid of property_set_pointer() Dmitry Torokhov
                   ` (9 subsequent siblings)
  14 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 a1f3f0994f9f..2f2248c9003c 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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 06/14] software node: get rid of property_set_pointer()
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (4 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 05/14] software node: clean up property_copy_string_array() Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-15 12:09   ` Andy Shevchenko
  2019-10-11 23:07 ` [PATCH v5 07/14] software node: remove property_entry_read_uNN_array functions Dmitry Torokhov
                   ` (8 subsequent siblings)
  14 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 2f2248c9003c..26e56dd66436 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 2c9d4d209296..ec8f84d564a8 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -233,13 +233,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;
@@ -262,7 +256,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_)		\
@@ -280,7 +274,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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 07/14] software node: remove property_entry_read_uNN_array functions
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (5 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 06/14] software node: get rid of property_set_pointer() Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 08/14] software node: unify PROPERTY_ENTRY_XXX macros Dmitry Torokhov
                   ` (7 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 26e56dd66436..b11cc4dbff08 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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 08/14] software node: unify PROPERTY_ENTRY_XXX macros
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (6 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 07/14] software node: remove property_entry_read_uNN_array functions Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 09/14] software node: simplify property_entry_read_string_array() Dmitry Torokhov
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 ec8f84d564a8..238e1507925f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -245,37 +245,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_))
@@ -288,30 +284,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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 09/14] software node: simplify property_entry_read_string_array()
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (7 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 08/14] software node: unify PROPERTY_ENTRY_XXX macros Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 10/14] software node: rename is_array to is_inline Dmitry Torokhov
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 b11cc4dbff08..8826c9040c80 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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 10/14] software node: rename is_array to is_inline
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (8 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 09/14] software node: simplify property_entry_read_string_array() Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-14  7:37   ` Andy Shevchenko
  2019-10-17 16:02   ` Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 11/14] software node: move small properties inline when copying Dmitry Torokhov
                   ` (4 subsequent siblings)
  14 siblings, 2 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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    |  9 +++++----
 include/linux/property.h | 12 +++++++-----
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 8826c9040c80..ae4b24ee2a54 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -108,7 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
 	if (!prop->length)
 		return NULL;
 
-	if (prop->is_array)
+	if (!prop->is_inline)
 		return prop->pointer;
 
 	return &prop->value;
@@ -205,7 +205,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 +250,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 +264,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 238e1507925f..ac7823d58cfe 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -222,15 +222,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 stored directly 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;
@@ -257,7 +259,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_ },						\
 }
@@ -288,6 +289,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_ } },				\
 }
-- 
2.23.0.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 11/14] software node: move small properties inline when copying
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (9 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 10/14] software node: rename is_array to is_inline Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-15 12:20   ` Andy Shevchenko
  2019-10-17 16:04   ` Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 12/14] software node: implement reference properties Dmitry Torokhov
                   ` (3 subsequent siblings)
  14 siblings, 2 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 ae4b24ee2a54..546fc1b20095 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -277,6 +277,16 @@ static int property_entry_copy_data(struct property_entry *dst,
 		dst->value = src->value;
 	}
 
+	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);
+	}
+
 	dst->length = src->length;
 	dst->type = src->type;
 	dst->name = kstrdup(src->name, GFP_KERNEL);
-- 
2.23.0.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 12/14] software node: implement reference properties
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (10 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 11/14] software node: move small properties inline when copying Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-11 23:07 ` [PATCH v5 13/14] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 546fc1b20095..a5b592a4e956 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -265,6 +265,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)
@@ -460,21 +466,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;
 
@@ -493,7 +525,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 ac7823d58cfe..08d3e9d126ef 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 {
@@ -218,6 +219,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.
@@ -255,14 +270,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_)		\
@@ -273,6 +294,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_))
@@ -284,6 +309,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) {						\
@@ -310,6 +337,18 @@ struct property_entry {
 	.name = _name_,				\
 }
 
+#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);
 
@@ -373,20 +412,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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 13/14] platform/x86: intel_cht_int33fe: use inline reference properties
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (11 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 12/14] software node: implement reference properties Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-16  8:01   ` Andy Shevchenko
  2019-10-11 23:07 ` [PATCH v5 14/14] software node: remove separate handling of references Dmitry Torokhov
  2019-10-14  7:38 ` [PATCH v5 00/14] software node: add support for reference properties Andy Shevchenko
  14 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 81 ++++++++++++------------
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 1d5d877b9582..4177c5424931 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -46,30 +46,6 @@ struct cht_int33fe_data {
 	struct fwnode_handle *dp;
 };
 
-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
@@ -105,8 +81,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),
 	{ }
 };
 
@@ -122,6 +108,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"),
@@ -129,15 +117,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 },
 	{ }
 };
 
@@ -173,9 +167,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) {
@@ -187,25 +182,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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v5 14/14] software node: remove separate handling of references
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (12 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 13/14] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
@ 2019-10-11 23:07 ` Dmitry Torokhov
  2019-10-14  7:38 ` [PATCH v5 00/14] software node: add support for reference properties Andy Shevchenko
  14 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-11 23:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

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 a5b592a4e956..4cb62cd105db 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -465,9 +465,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;
@@ -476,37 +475,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;
 
@@ -525,7 +513,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 08d3e9d126ef..fa5a2ddc0c7b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -412,30 +412,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.700.g56cf767bdb-goog


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 10/14] software node: rename is_array to is_inline
  2019-10-11 23:07 ` [PATCH v5 10/14] software node: rename is_array to is_inline Dmitry Torokhov
@ 2019-10-14  7:37   ` Andy Shevchenko
  2019-10-15 18:22     ` Dmitry Torokhov
  2019-10-17 16:02   ` Dmitry Torokhov
  1 sibling, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-14  7:37 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> 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.

> -	if (prop->is_array)
> +	if (!prop->is_inline)

> -	if (p->is_array) {
> +	if (!p->is_inline) {

> -	if (src->is_array) {
> +	if (!src->is_inline) {

May we have positive conditionals instead?

> + * @is_inline: True when the property value is stored directly in

I think word 'directly' is superfluous here.
Or, perhaps, 'stored directly' -> 'embedded'

> + *     &struct property_entry instance.

> + * @pointer: Pointer to the property when it is stored separately from
> + *     the &struct property_entry instance.

'separately from' -> 'outside' ?

> + * @value: Value of the property when it is stored inline.

'stored inline' -> 'embedded in the &struct...' ?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 00/14] software node: add support for reference properties
  2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
                   ` (13 preceding siblings ...)
  2019-10-11 23:07 ` [PATCH v5 14/14] software node: remove separate handling of references Dmitry Torokhov
@ 2019-10-14  7:38 ` Andy Shevchenko
  2019-10-14 23:57   ` Dmitry Torokhov
  14 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-14  7:38 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:07PM -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.

Can we get some test cases?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 00/14] software node: add support for reference properties
  2019-10-14  7:38 ` [PATCH v5 00/14] software node: add support for reference properties Andy Shevchenko
@ 2019-10-14 23:57   ` Dmitry Torokhov
  2019-10-16  8:02     ` Andy Shevchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-14 23:57 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Mon, Oct 14, 2019 at 10:38:37AM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:07PM -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.
> 
> Can we get some test cases?

Something like this? (I'll beef it up if we decide KUnit is OK for
this).

From 0b8256ceed44760e63becb5b9636099d9fc17a4c Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Mon, 14 Oct 2019 16:55:12 -0700
Subject: [PATCH] software node: add basic init tests

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/test/Makefile              |  2 +
 drivers/base/test/property-entry-test.c | 56 +++++++++++++++++++++++++
 2 files changed, 58 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..cd6a405734a0
--- /dev/null
+++ b/drivers/base/test/property-entry-test.c
@@ -0,0 +1,56 @@
+// 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_move_inline_u8(struct kunit *test)
+{
+	u8 u8_array_small[8] = { 0 };
+	u8 u8_array_big[128] = { 0 };
+	struct property_entry entries[] = {
+		PROPERTY_ENTRY_U8_ARRAY("small", u8_array_small),
+		PROPERTY_ENTRY_U8_ARRAY("big", u8_array_big),
+		{ }
+	};
+	struct property_entry *copy;
+
+	copy = property_entries_dup(entries);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, copy);
+	KUNIT_EXPECT_TRUE(test, copy[0].is_inline);
+	KUNIT_EXPECT_FALSE(test, copy[1].is_inline);
+}
+
+static void pe_test_move_inline_str(struct kunit *test)
+{
+	char *str_array_small[] = { "a" };
+	char *str_array_big[] = { "a", "b", "c", "d" };
+	struct property_entry entries[] = {
+		PROPERTY_ENTRY_STRING_ARRAY("small", str_array_small),
+		PROPERTY_ENTRY_STRING_ARRAY("big", str_array_big),
+		{ }
+	};
+	struct property_entry *copy;
+
+	copy = property_entries_dup(entries);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, copy);
+	KUNIT_EXPECT_TRUE(test, copy[0].is_inline);
+	KUNIT_EXPECT_FALSE(test, copy[1].is_inline);
+}
+
+
+static struct kunit_case property_entry_test_cases[] = {
+	KUNIT_CASE(pe_test_move_inline_u8),
+	KUNIT_CASE(pe_test_move_inline_str),
+	{ }
+};
+
+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.700.g56cf767bdb-goog


-- 
Dmitry

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 03/14] efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN
  2019-10-11 23:07 ` [PATCH v5 03/14] efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN Dmitry Torokhov
@ 2019-10-15 12:01   ` Andy Shevchenko
  2019-10-15 18:02     ` Dmitry Torokhov
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-15 12:01 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:10PM -0700, Dmitry Torokhov wrote:
> 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.
> 
> 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);

I would rather leave on one line.
Nevertheless,

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

>  		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.700.g56cf767bdb-goog
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 02/14] software node: introduce PROPERTY_ENTRY_ARRAY_XXX_LEN()
  2019-10-11 23:07 ` [PATCH v5 02/14] software node: introduce PROPERTY_ENTRY_ARRAY_XXX_LEN() Dmitry Torokhov
@ 2019-10-15 12:02   ` Andy Shevchenko
  0 siblings, 0 replies; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-15 12:02 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:09PM -0700, Dmitry Torokhov wrote:
> 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 44c1704f7163..f89b930ca4b7 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -256,33 +256,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.700.g56cf767bdb-goog
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 04/14] software node: mark internal macros with double underscores
  2019-10-11 23:07 ` [PATCH v5 04/14] software node: mark internal macros with double underscores Dmitry Torokhov
@ 2019-10-15 12:03   ` Andy Shevchenko
  2019-10-15 18:09     ` Dmitry Torokhov
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-15 12:03 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:11PM -0700, Dmitry Torokhov wrote:
> Let's mark PROPERTY_ENTRY_* macros that are internal with double leading
> underscores so users are not tempted to use them.

We may undef them at the end of file, right?

> 
> 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 f89b930ca4b7..2c9d4d209296 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -256,7 +256,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_),				\
> @@ -266,13 +266,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) {						\
> @@ -294,7 +294,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_),				\
> @@ -303,13 +303,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.700.g56cf767bdb-goog
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 05/14] software node: clean up property_copy_string_array()
  2019-10-11 23:07 ` [PATCH v5 05/14] software node: clean up property_copy_string_array() Dmitry Torokhov
@ 2019-10-15 12:07   ` Andy Shevchenko
  2019-10-15 18:12     ` Dmitry Torokhov
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-15 12:07 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:12PM -0700, Dmitry Torokhov wrote:
> 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.

I don't see a problem in this function.

Rather 'awkward code' is a result of use property_set_pointer() which relies on
data type.

> 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.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 06/14] software node: get rid of property_set_pointer()
  2019-10-11 23:07 ` [PATCH v5 06/14] software node: get rid of property_set_pointer() Dmitry Torokhov
@ 2019-10-15 12:09   ` Andy Shevchenko
  2019-10-15 18:14     ` Dmitry Torokhov
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-15 12:09 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:13PM -0700, Dmitry Torokhov wrote:
> 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.

I'm not sure about this change since the struct definition is still available
to use. If we would change it to be opaque pointer, it will be possible to get
rid of the type differentiation in cleaner way.

Anyway, perhaps somebody else can look at this.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 11/14] software node: move small properties inline when copying
  2019-10-11 23:07 ` [PATCH v5 11/14] software node: move small properties inline when copying Dmitry Torokhov
@ 2019-10-15 12:20   ` Andy Shevchenko
  2019-10-15 18:25     ` Dmitry Torokhov
  2019-10-17 16:04   ` Dmitry Torokhov
  1 sibling, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-15 12:20 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:18PM -0700, 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.

Can you show where you extract such values?

> +	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);

...because this is strange trick.

> +		dst->is_inline = true;
> +
> +		kfree(tmp);
> +	}

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 03/14] efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN
  2019-10-15 12:01   ` Andy Shevchenko
@ 2019-10-15 18:02     ` Dmitry Torokhov
  0 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-15 18:02 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Tue, Oct 15, 2019 at 03:01:49PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:10PM -0700, Dmitry Torokhov wrote:
> > 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.
> > 
> > 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);
> 
> I would rather leave on one line.

I am trying to stay withing 80 char limit by default, but do not have
strong opinion on the code I do not maintain. Up to Ard I suppose.

> Nevertheless,
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you for looking the patches over.

> 
> >  		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.700.g56cf767bdb-goog
> > 
> 

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 04/14] software node: mark internal macros with double underscores
  2019-10-15 12:03   ` Andy Shevchenko
@ 2019-10-15 18:09     ` Dmitry Torokhov
  0 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-15 18:09 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Tue, Oct 15, 2019 at 03:03:50PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:11PM -0700, Dmitry Torokhov wrote:
> > Let's mark PROPERTY_ENTRY_* macros that are internal with double leading
> > underscores so users are not tempted to use them.
> 
> We may undef them at the end of file, right?

No... the macro substitution happens at the point of final use, so
somewhere in .c file.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 05/14] software node: clean up property_copy_string_array()
  2019-10-15 12:07   ` Andy Shevchenko
@ 2019-10-15 18:12     ` Dmitry Torokhov
  2019-10-16  7:53       ` Andy Shevchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-15 18:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Tue, Oct 15, 2019 at 03:07:26PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:12PM -0700, Dmitry Torokhov wrote:
> > 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.
> 
> I don't see a problem in this function.
> 
> Rather 'awkward code' is a result of use property_set_pointer() which relies on
> data type.

No, the awkwardness is that we set the pointer once in
property_copy_string_array(), then fetch it in
property_entry_copy_data() only to set it again via
property_set_pointer(). This is confising and awkward and I believe it
is cleaner for property_copy_string_array() to give a pointer to a copy
of a string array, and then property_entry_copy_data() use it when
handling the destination structure.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 06/14] software node: get rid of property_set_pointer()
  2019-10-15 12:09   ` Andy Shevchenko
@ 2019-10-15 18:14     ` Dmitry Torokhov
  0 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-15 18:14 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Tue, Oct 15, 2019 at 03:09:49PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:13PM -0700, Dmitry Torokhov wrote:
> > 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.
> 
> I'm not sure about this change since the struct definition is still available
> to use. If we would change it to be opaque pointer, it will be possible to get
> rid of the type differentiation in cleaner way.

We expose type and other fields that should not be manipulated directly,
for the benefit of being able to use static initializers. Having value
structure allows for not too ugly non-array initializers.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 10/14] software node: rename is_array to is_inline
  2019-10-14  7:37   ` Andy Shevchenko
@ 2019-10-15 18:22     ` Dmitry Torokhov
  2019-10-16  7:59       ` Andy Shevchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-15 18:22 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> > 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.
> 
> > -	if (prop->is_array)
> > +	if (!prop->is_inline)
> 
> > -	if (p->is_array) {
> > +	if (!p->is_inline) {
> 
> > -	if (src->is_array) {
> > +	if (!src->is_inline) {
> 
> May we have positive conditionals instead?

I was trying to limit the context churn. I can definitely change
property_get_pointer(), but the other 2 I think are better in the
current form.

> 
> > + * @is_inline: True when the property value is stored directly in
> 
> I think word 'directly' is superfluous here.
> Or, perhaps, 'stored directly' -> 'embedded'

I'm OK with "embedded".

> 
> > + *     &struct property_entry instance.
> 
> > + * @pointer: Pointer to the property when it is stored separately from
> > + *     the &struct property_entry instance.
> 
> 'separately from' -> 'outside' ?

Umm, I think I prefer "separately" actually.

> 
> > + * @value: Value of the property when it is stored inline.
> 
> 'stored inline' -> 'embedded in the &struct...' ?

I was trying to have a link "stored inline" -> "is_inline".

Do we want to change the flag to be "is_embedded"?

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 11/14] software node: move small properties inline when copying
  2019-10-15 12:20   ` Andy Shevchenko
@ 2019-10-15 18:25     ` Dmitry Torokhov
  2019-10-16  7:48       ` Andy Shevchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-15 18:25 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Tue, Oct 15, 2019 at 03:20:28PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:18PM -0700, 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.
> 
> Can you show where you extract such values?

the "value" union's largest member is u64, which is 8 bytes. Strings are
pointers, so on 32-bit arches you can stuff 2 pointers into 8 bytes,
while on 64-bits you have space for only one.

> 
> > +	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);
> 
> ...because this is strange trick.

Not sure what is so strange about it. You just take data that is stored
separately and move it into the structure, provided that it is not too
big (i.e. it does not exceed sizeof(value union) size).

> 
> > +		dst->is_inline = true;
> > +
> > +		kfree(tmp);
> > +	}
> 

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 11/14] software node: move small properties inline when copying
  2019-10-15 18:25     ` Dmitry Torokhov
@ 2019-10-16  7:48       ` Andy Shevchenko
  2019-10-16 16:01         ` Dmitry Torokhov
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-16  7:48 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 15, 2019 at 03:20:28PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 11, 2019 at 04:07:18PM -0700, 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.
> > 
> > Can you show where you extract such values?
> 
> the "value" union's largest member is u64, which is 8 bytes. Strings are
> pointers, so on 32-bit arches you can stuff 2 pointers into 8 bytes,
> while on 64-bits you have space for only one.
> 
> > 
> > > +	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);
> > 
> > ...because this is strange trick.
> 
> Not sure what is so strange about it. You just take data that is stored
> separately and move it into the structure, provided that it is not too
> big (i.e. it does not exceed sizeof(value union) size).

You store a value as union, but going to read as a member of union?
I'm pretty sure it breaks standard rules.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 05/14] software node: clean up property_copy_string_array()
  2019-10-15 18:12     ` Dmitry Torokhov
@ 2019-10-16  7:53       ` Andy Shevchenko
  2019-10-16 17:00         ` Dmitry Torokhov
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-16  7:53 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Tue, Oct 15, 2019 at 11:12:11AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 15, 2019 at 03:07:26PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 11, 2019 at 04:07:12PM -0700, Dmitry Torokhov wrote:
> > > 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.
> > 
> > I don't see a problem in this function.
> > 
> > Rather 'awkward code' is a result of use property_set_pointer() which relies on
> > data type.
> 
> No, the awkwardness is that we set the pointer once in
> property_copy_string_array(), then fetch it in
> property_entry_copy_data() only to set it again via
> property_set_pointer().

Yes, since property_set_pointer is called independently
on the type of the value.


> This is confising and awkward and I believe it
> is cleaner for property_copy_string_array() to give a pointer to a copy
> of a string array, and then property_entry_copy_data() use it when
> handling the destination structure.

We probably need a 3rd opinion here.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 10/14] software node: rename is_array to is_inline
  2019-10-15 18:22     ` Dmitry Torokhov
@ 2019-10-16  7:59       ` Andy Shevchenko
  2019-10-16 16:54         ` Dmitry Torokhov
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-16  7:59 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Tue, Oct 15, 2019 at 11:22:06AM -0700, Dmitry Torokhov wrote:
> On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> > > 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.
> > 
> > > -	if (prop->is_array)
> > > +	if (!prop->is_inline)
> > 
> > > -	if (p->is_array) {
> > > +	if (!p->is_inline) {
> > 
> > > -	if (src->is_array) {
> > > +	if (!src->is_inline) {
> > 
> > May we have positive conditionals instead?
> 
> I was trying to limit the context churn. I can definitely change
> property_get_pointer(), but the other 2 I think are better in the
> current form.
> 
> > 
> > > + * @is_inline: True when the property value is stored directly in
> > 
> > I think word 'directly' is superfluous here.
> > Or, perhaps, 'stored directly' -> 'embedded'
> 
> I'm OK with "embedded".
> 
> > 
> > > + *     &struct property_entry instance.
> > 
> > > + * @pointer: Pointer to the property when it is stored separately from
> > > + *     the &struct property_entry instance.
> > 
> > 'separately from' -> 'outside' ?
> 
> Umm, I think I prefer "separately" actually.
> 
> > 
> > > + * @value: Value of the property when it is stored inline.
> > 
> > 'stored inline' -> 'embedded in the &struct...' ?
> 
> I was trying to have a link "stored inline" -> "is_inline".
> 
> Do we want to change the flag to be "is_embedded"?

In dictionaries I have

embedded <-> unilateral
inline <-> ???

Perhaps native speaker can jump in and help here.

My point is to be consistent in the terms we are using.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 13/14] platform/x86: intel_cht_int33fe: use inline reference properties
  2019-10-11 23:07 ` [PATCH v5 13/14] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
@ 2019-10-16  8:01   ` Andy Shevchenko
  0 siblings, 0 replies; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-16  8:01 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:20PM -0700, Dmitry Torokhov wrote:
> 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>

as an idea, whatever implementation we will settle on.

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/platform/x86/intel_cht_int33fe.c | 81 ++++++++++++------------
>  1 file changed, 41 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 1d5d877b9582..4177c5424931 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -46,30 +46,6 @@ struct cht_int33fe_data {
>  	struct fwnode_handle *dp;
>  };
>  
> -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
> @@ -105,8 +81,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),
>  	{ }
>  };
>  
> @@ -122,6 +108,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"),
> @@ -129,15 +117,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 },
>  	{ }
>  };
>  
> @@ -173,9 +167,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) {
> @@ -187,25 +182,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.700.g56cf767bdb-goog
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 00/14] software node: add support for reference properties
  2019-10-14 23:57   ` Dmitry Torokhov
@ 2019-10-16  8:02     ` Andy Shevchenko
  0 siblings, 0 replies; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-16  8:02 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Mon, Oct 14, 2019 at 04:57:47PM -0700, Dmitry Torokhov wrote:
> On Mon, Oct 14, 2019 at 10:38:37AM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 11, 2019 at 04:07:07PM -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.
> > 
> > Can we get some test cases?
> 
> Something like this? (I'll beef it up if we decide KUnit is OK for
> this).

As a starter, yes.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 11/14] software node: move small properties inline when copying
  2019-10-16  7:48       ` Andy Shevchenko
@ 2019-10-16 16:01         ` Dmitry Torokhov
  2019-10-16 16:18           ` Andy Shevchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-16 16:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Wed, Oct 16, 2019 at 10:48:57AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 15, 2019 at 03:20:28PM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 11, 2019 at 04:07:18PM -0700, 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.
> > > 
> > > Can you show where you extract such values?
> > 
> > the "value" union's largest member is u64, which is 8 bytes. Strings are
> > pointers, so on 32-bit arches you can stuff 2 pointers into 8 bytes,
> > while on 64-bits you have space for only one.
> > 
> > > 
> > > > +	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);
> > > 
> > > ...because this is strange trick.
> > 
> > Not sure what is so strange about it. You just take data that is stored
> > separately and move it into the structure, provided that it is not too
> > big (i.e. it does not exceed sizeof(value union) size).
> 
> You store a value as union, but going to read as a member of union?
> I'm pretty sure it breaks standard rules.

No, I move the values _in place_ of the union, and the data is always
fetched via void pointers. And copying data via char * or memcpy() is
allowed even in C99 and C11.

But I am wondering why are we actually worrying about all of this? The
kernel is gnu89 and I think is going to stay this way because we use
initializers with a cast in a lot of places:

#define __RAW_SPIN_LOCK_UNLOCKED(lockname)      \
        (raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)

and C99 and gnu99 do not allow this. See
https://lore.kernel.org/lkml/20141019231031.GB9319@node.dhcp.inet.fi/

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 11/14] software node: move small properties inline when copying
  2019-10-16 16:01         ` Dmitry Torokhov
@ 2019-10-16 16:18           ` Andy Shevchenko
  2019-10-16 16:23             ` Andy Shevchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-16 16:18 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Wed, Oct 16, 2019 at 09:01:26AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 16, 2019 at 10:48:57AM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:

> > You store a value as union, but going to read as a member of union?
> > I'm pretty sure it breaks standard rules.
> 
> No, I move the values _in place_ of the union, and the data is always
> fetched via void pointers. And copying data via char * or memcpy() is
> allowed even in C99 and C11.
> 
> But I am wondering why are we actually worrying about all of this? The
> kernel is gnu89 and I think is going to stay this way because we use
> initializers with a cast in a lot of places:
> 
> #define __RAW_SPIN_LOCK_UNLOCKED(lockname)      \
>         (raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)
> 
> and C99 and gnu99 do not allow this. See
> https://lore.kernel.org/lkml/20141019231031.GB9319@node.dhcp.inet.fi/

This is simple not a cast.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 11/14] software node: move small properties inline when copying
  2019-10-16 16:18           ` Andy Shevchenko
@ 2019-10-16 16:23             ` Andy Shevchenko
  2019-10-16 16:44               ` Dmitry Torokhov
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-16 16:23 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Wed, Oct 16, 2019 at 07:18:45PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 16, 2019 at 09:01:26AM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 16, 2019 at 10:48:57AM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:
> 
> > > You store a value as union, but going to read as a member of union?
> > > I'm pretty sure it breaks standard rules.
> > 
> > No, I move the values _in place_ of the union, and the data is always
> > fetched via void pointers. And copying data via char * or memcpy() is
> > allowed even in C99 and C11.
> > 
> > But I am wondering why are we actually worrying about all of this? The
> > kernel is gnu89 and I think is going to stay this way because we use
> > initializers with a cast in a lot of places:
> > 
> > #define __RAW_SPIN_LOCK_UNLOCKED(lockname)      \
> >         (raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)
> > 
> > and C99 and gnu99 do not allow this. See
> > https://lore.kernel.org/lkml/20141019231031.GB9319@node.dhcp.inet.fi/
> 
> This is simple not a cast.

4.62 Compound literals in C99
ISO C99 supports compound literals. A compound literal looks like a cast
followed by an initializer. Its value is an object of the type specified in the
cast, containing the elements specified in the initializer. It is an lvalue.


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 11/14] software node: move small properties inline when copying
  2019-10-16 16:23             ` Andy Shevchenko
@ 2019-10-16 16:44               ` Dmitry Torokhov
  0 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-16 16:44 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Wed, Oct 16, 2019 at 07:23:08PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 16, 2019 at 07:18:45PM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 16, 2019 at 09:01:26AM -0700, Dmitry Torokhov wrote:
> > > On Wed, Oct 16, 2019 at 10:48:57AM +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:
> > 
> > > > You store a value as union, but going to read as a member of union?
> > > > I'm pretty sure it breaks standard rules.
> > > 
> > > No, I move the values _in place_ of the union, and the data is always
> > > fetched via void pointers. And copying data via char * or memcpy() is
> > > allowed even in C99 and C11.
> > > 
> > > But I am wondering why are we actually worrying about all of this? The
> > > kernel is gnu89 and I think is going to stay this way because we use
> > > initializers with a cast in a lot of places:
> > > 
> > > #define __RAW_SPIN_LOCK_UNLOCKED(lockname)      \
> > >         (raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)
> > > 
> > > and C99 and gnu99 do not allow this. See
> > > https://lore.kernel.org/lkml/20141019231031.GB9319@node.dhcp.inet.fi/
> > 
> > This is simple not a cast.
> 
> 4.62 Compound literals in C99
> ISO C99 supports compound literals. A compound literal looks like a cast
> followed by an initializer. Its value is an object of the type specified in the
> cast, containing the elements specified in the initializer. It is an lvalue.

Yes, these are compound literals. And they can not be used as
initializers:

https://lore.kernel.org/lkml/CAHk-=wgXBV57mz46ZB5XivjiSBGkM0cjuvnU2OWyfRF=+41NPQ@mail.gmail.com/

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 10/14] software node: rename is_array to is_inline
  2019-10-16  7:59       ` Andy Shevchenko
@ 2019-10-16 16:54         ` Dmitry Torokhov
  2019-10-17  7:16           ` Andy Shevchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-16 16:54 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Wed, Oct 16, 2019 at 10:59:40AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 15, 2019 at 11:22:06AM -0700, Dmitry Torokhov wrote:
> > On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> > > > 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.
> > > 
> > > > -	if (prop->is_array)
> > > > +	if (!prop->is_inline)
> > > 
> > > > -	if (p->is_array) {
> > > > +	if (!p->is_inline) {
> > > 
> > > > -	if (src->is_array) {
> > > > +	if (!src->is_inline) {
> > > 
> > > May we have positive conditionals instead?
> > 
> > I was trying to limit the context churn. I can definitely change
> > property_get_pointer(), but the other 2 I think are better in the
> > current form.
> > 
> > > 
> > > > + * @is_inline: True when the property value is stored directly in
> > > 
> > > I think word 'directly' is superfluous here.
> > > Or, perhaps, 'stored directly' -> 'embedded'
> > 
> > I'm OK with "embedded".
> > 
> > > 
> > > > + *     &struct property_entry instance.
> > > 
> > > > + * @pointer: Pointer to the property when it is stored separately from
> > > > + *     the &struct property_entry instance.
> > > 
> > > 'separately from' -> 'outside' ?
> > 
> > Umm, I think I prefer "separately" actually.
> > 
> > > 
> > > > + * @value: Value of the property when it is stored inline.
> > > 
> > > 'stored inline' -> 'embedded in the &struct...' ?
> > 
> > I was trying to have a link "stored inline" -> "is_inline".
> > 
> > Do we want to change the flag to be "is_embedded"?
> 
> In dictionaries I have
> 
> embedded <-> unilateral

Are you trying to show synonym or antonym here? But I am pretty sure
"unilateral" is either.

Antonyms for our use of "embedded" are likely "detached" or
"disconnected".

> inline <-> ???

"out of line" but I still believe "stored separately" explains precisely
what we have here.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 05/14] software node: clean up property_copy_string_array()
  2019-10-16  7:53       ` Andy Shevchenko
@ 2019-10-16 17:00         ` Dmitry Torokhov
  2019-10-17  7:02           ` Andy Shevchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-16 17:00 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Wed, Oct 16, 2019 at 10:53:00AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 15, 2019 at 11:12:11AM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 15, 2019 at 03:07:26PM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 11, 2019 at 04:07:12PM -0700, Dmitry Torokhov wrote:
> > > > 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.
> > > 
> > > I don't see a problem in this function.
> > > 
> > > Rather 'awkward code' is a result of use property_set_pointer() which relies on
> > > data type.
> > 
> > No, the awkwardness is that we set the pointer once in
> > property_copy_string_array(), then fetch it in
> > property_entry_copy_data() only to set it again via
> > property_set_pointer().
> 
> Yes, since property_set_pointer is called independently
> on the type of the value.

We still call property_set_pointer() independently of the type of the
value even with this patch. The point is that we do not set the pointer
in property_copy_string_array(), so we only set the pointer once.

We used to have essentially for string arrays:

	copy data
	set pointer in dst
	get pointer from dst
	set pointer in dst

With this patch we have:

	copy data
	set pointer in dst

> 
> 
> > This is confising and awkward and I believe it
> > is cleaner for property_copy_string_array() to give a pointer to a copy
> > of a string array, and then property_entry_copy_data() use it when
> > handling the destination structure.
> 
> We probably need a 3rd opinion here.

I think I can still convince you ;)

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 05/14] software node: clean up property_copy_string_array()
  2019-10-16 17:00         ` Dmitry Torokhov
@ 2019-10-17  7:02           ` Andy Shevchenko
  0 siblings, 0 replies; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-17  7:02 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Wed, Oct 16, 2019 at 10:00:59AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 16, 2019 at 10:53:00AM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 15, 2019 at 11:12:11AM -0700, Dmitry Torokhov wrote:
> > > On Tue, Oct 15, 2019 at 03:07:26PM +0300, Andy Shevchenko wrote:

> > Yes, since property_set_pointer is called independently
> > on the type of the value.
> 
> We still call property_set_pointer() independently of the type of the
> value even with this patch. The point is that we do not set the pointer
> in property_copy_string_array(), so we only set the pointer once.
> 
> We used to have essentially for string arrays:
> 
> 	copy data
> 	set pointer in dst
> 	get pointer from dst
> 	set pointer in dst
> 
> With this patch we have:
> 
> 	copy data
> 	set pointer in dst

> > > This is confising and awkward and I believe it
> > > is cleaner for property_copy_string_array() to give a pointer to a copy
> > > of a string array, and then property_entry_copy_data() use it when
> > > handling the destination structure.
> > 
> > We probably need a 3rd opinion here.
> 
> I think I can still convince you ;)

Probably this is fine.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 10/14] software node: rename is_array to is_inline
  2019-10-16 16:54         ` Dmitry Torokhov
@ 2019-10-17  7:16           ` Andy Shevchenko
  2019-10-17 16:00             ` Dmitry Torokhov
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2019-10-17  7:16 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Wed, Oct 16, 2019 at 09:54:30AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 16, 2019 at 10:59:40AM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 15, 2019 at 11:22:06AM -0700, Dmitry Torokhov wrote:
> > > On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote:
> > > > On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:

> > > > 'stored inline' -> 'embedded in the &struct...' ?
> > > 
> > > I was trying to have a link "stored inline" -> "is_inline".
> > > 
> > > Do we want to change the flag to be "is_embedded"?
> > 
> > In dictionaries I have
> > 
> > embedded <-> unilateral
> 
> Are you trying to show synonym or antonym here? But I am pretty sure
> "unilateral" is either.

Antonyms. The 'unilateral' is marked as so in the dictionary.

> Antonyms for our use of "embedded" are likely "detached" or
> "disconnected".
> 
> > inline <-> ???
> 
> "out of line" but I still believe "stored separately" explains precisely
> what we have here.

No, 'out of line' is idiom with a special meaning.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 10/14] software node: rename is_array to is_inline
  2019-10-17  7:16           ` Andy Shevchenko
@ 2019-10-17 16:00             ` Dmitry Torokhov
  0 siblings, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-17 16:00 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Mika Westerberg,
	Linus Walleij, Ard Biesheuvel, linux-acpi, linux-kernel,
	platform-driver-x86

On Thu, Oct 17, 2019 at 10:16:28AM +0300, Andy Shevchenko wrote:
> On Wed, Oct 16, 2019 at 09:54:30AM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 16, 2019 at 10:59:40AM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 15, 2019 at 11:22:06AM -0700, Dmitry Torokhov wrote:
> > > > On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> 
> > > > > 'stored inline' -> 'embedded in the &struct...' ?
> > > > 
> > > > I was trying to have a link "stored inline" -> "is_inline".
> > > > 
> > > > Do we want to change the flag to be "is_embedded"?
> > > 
> > > In dictionaries I have
> > > 
> > > embedded <-> unilateral
> > 
> > Are you trying to show synonym or antonym here? But I am pretty sure
> > "unilateral" is either.
> 
> Antonyms. The 'unilateral' is marked as so in the dictionary.

OK, that is not something that I would ever think of as an antonym, so
even though I am not a native speaker I do not think we should be using
it here as documentation and comments are supposed to be understood by
all people from around the world and not by English majors only.

Out of curiosity, is this dictionary available online? I would really
want to see to what particular meaning of "embedded" they assign
"unilateral" as antonym so that I know better next time I see it used.

> 
> > Antonyms for our use of "embedded" are likely "detached" or
> > "disconnected".
> > 
> > > inline <-> ???
> > 
> > "out of line" but I still believe "stored separately" explains precisely
> > what we have here.
> 
> No, 'out of line' is idiom with a special meaning.

Yes, and it is also a well defined term in CS.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 10/14] software node: rename is_array to is_inline
  2019-10-11 23:07 ` [PATCH v5 10/14] software node: rename is_array to is_inline Dmitry Torokhov
  2019-10-14  7:37   ` Andy Shevchenko
@ 2019-10-17 16:02   ` Dmitry Torokhov
  1 sibling, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-17 16:02 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> 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    |  9 +++++----
>  include/linux/property.h | 12 +++++++-----
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 8826c9040c80..ae4b24ee2a54 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -108,7 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
>  	if (!prop->length)
>  		return NULL;
>  
> -	if (prop->is_array)
> +	if (!prop->is_inline)
>  		return prop->pointer;
>  
>  	return &prop->value;
> @@ -205,7 +205,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 +250,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 +264,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 238e1507925f..ac7823d58cfe 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -222,15 +222,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 stored directly 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;
> @@ -257,7 +259,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_ },						\
>  }
> @@ -288,6 +289,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_ } },				\
>  }

The patch also needs to set is_inline in PROPERTY_ENTRY_BOOL(), I'll
send updated version after I get more feedback.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v5 11/14] software node: move small properties inline when copying
  2019-10-11 23:07 ` [PATCH v5 11/14] software node: move small properties inline when copying Dmitry Torokhov
  2019-10-15 12:20   ` Andy Shevchenko
@ 2019-10-17 16:04   ` Dmitry Torokhov
  1 sibling, 0 replies; 46+ messages in thread
From: Dmitry Torokhov @ 2019-10-17 16:04 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Ard Biesheuvel,
	linux-acpi, linux-kernel, platform-driver-x86

On Fri, Oct 11, 2019 at 04:07:18PM -0700, 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.
> 
> 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 ae4b24ee2a54..546fc1b20095 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -277,6 +277,16 @@ static int property_entry_copy_data(struct property_entry *dst,
>  		dst->value = src->value;
>  	}
>  
> +	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 chunk needs to be moved to after dst->length is assigned.  I'll
send updated version after I get more feedback.

> +
>  	dst->length = src->length;
>  	dst->type = src->type;
>  	dst->name = kstrdup(src->name, GFP_KERNEL);

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 46+ messages in thread

end of thread, other threads:[~2019-10-17 16:04 UTC | newest]

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-11 23:07 [PATCH v5 00/14] software node: add support for reference properties Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 01/14] software node: remove DEV_PROP_MAX Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 02/14] software node: introduce PROPERTY_ENTRY_ARRAY_XXX_LEN() Dmitry Torokhov
2019-10-15 12:02   ` Andy Shevchenko
2019-10-11 23:07 ` [PATCH v5 03/14] efi/apple-properties: use PROPERTY_ENTRY_U8_ARRAY_LEN Dmitry Torokhov
2019-10-15 12:01   ` Andy Shevchenko
2019-10-15 18:02     ` Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 04/14] software node: mark internal macros with double underscores Dmitry Torokhov
2019-10-15 12:03   ` Andy Shevchenko
2019-10-15 18:09     ` Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 05/14] software node: clean up property_copy_string_array() Dmitry Torokhov
2019-10-15 12:07   ` Andy Shevchenko
2019-10-15 18:12     ` Dmitry Torokhov
2019-10-16  7:53       ` Andy Shevchenko
2019-10-16 17:00         ` Dmitry Torokhov
2019-10-17  7:02           ` Andy Shevchenko
2019-10-11 23:07 ` [PATCH v5 06/14] software node: get rid of property_set_pointer() Dmitry Torokhov
2019-10-15 12:09   ` Andy Shevchenko
2019-10-15 18:14     ` Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 07/14] software node: remove property_entry_read_uNN_array functions Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 08/14] software node: unify PROPERTY_ENTRY_XXX macros Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 09/14] software node: simplify property_entry_read_string_array() Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 10/14] software node: rename is_array to is_inline Dmitry Torokhov
2019-10-14  7:37   ` Andy Shevchenko
2019-10-15 18:22     ` Dmitry Torokhov
2019-10-16  7:59       ` Andy Shevchenko
2019-10-16 16:54         ` Dmitry Torokhov
2019-10-17  7:16           ` Andy Shevchenko
2019-10-17 16:00             ` Dmitry Torokhov
2019-10-17 16:02   ` Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 11/14] software node: move small properties inline when copying Dmitry Torokhov
2019-10-15 12:20   ` Andy Shevchenko
2019-10-15 18:25     ` Dmitry Torokhov
2019-10-16  7:48       ` Andy Shevchenko
2019-10-16 16:01         ` Dmitry Torokhov
2019-10-16 16:18           ` Andy Shevchenko
2019-10-16 16:23             ` Andy Shevchenko
2019-10-16 16:44               ` Dmitry Torokhov
2019-10-17 16:04   ` Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 12/14] software node: implement reference properties Dmitry Torokhov
2019-10-11 23:07 ` [PATCH v5 13/14] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
2019-10-16  8:01   ` Andy Shevchenko
2019-10-11 23:07 ` [PATCH v5 14/14] software node: remove separate handling of references Dmitry Torokhov
2019-10-14  7:38 ` [PATCH v5 00/14] software node: add support for reference properties Andy Shevchenko
2019-10-14 23:57   ` Dmitry Torokhov
2019-10-16  8:02     ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).