software node: add support for reference properties

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 <[email protected]>

---
 include/linux/property.h | 45 +++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 17 deletions(-)

include/linux/property.h +28 -17

index 44c1704f7163..f89b930ca4b7 100644
@@ -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_,						\

Signature · 1 line

2.23.0.700.g56cf767bdb-goog

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 <[email protected]>

---
 drivers/firmware/efi/apple-properties.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

drivers/firmware/efi/apple-properties.c +2 -1

index 0e206c9e0d7a..5ccf39986a14 100644
@@ -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));
 

drivers/firmware/efi/apple-properties.c +2 -3

 		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;

Signature · 1 line

2.23.0.700.g56cf767bdb-goog

Quoted 27 lines · Dmitry Torokhov

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

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/firmware/efi/apple-properties.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

drivers/firmware/efi/apple-properties.c +2 -1

index 0e206c9e0d7a..5ccf39986a14 100644
@@ -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));
 

drivers/firmware/efi/apple-properties.c static void __init unmarshal_key_value_pairs(struct dev_header *dev_header, · line 92

Quoted 10 lines

drivers/firmware/efi/apple-properties.c +4 -6

-		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 <[email protected]>

Quoted 10 lines

drivers/firmware/efi/apple-properties.c +2 -3

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

Quoted 4 lines

drivers/firmware/efi/apple-properties.c +0 -0

 		ptr += key_len + val_len;

Signature · 1 line

2.23.0.700.g56cf767bdb-goog

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 <[email protected]>

---
 include/linux/property.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

include/linux/property.h +10 -10

index f89b930ca4b7..2c9d4d209296 100644
@@ -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) {				\

Signature · 1 line

2.23.0.700.g56cf767bdb-goog

Quoted 2 lines · Dmitry Torokhov

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?

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.

Quoted 3 lines · Dmitry Torokhov

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

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.

Quoted 7 lines · Andy Shevchenko

Quoted 3 lines · Dmitry Torokhov

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

I don't see a problem in this function.

Quoted 2 lines

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.

Quoted 11 lines · Dmitry Torokhov

Quoted 9 lines · Andy Shevchenko

Quoted 3 lines · Dmitry Torokhov

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

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 5 lines · Andy Shevchenko

Quoted 1 line · Dmitry Torokhov

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

Quoted 6 lines

Quoted 4 lines

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

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.

Quoted 11 lines · Dmitry Torokhov

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

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

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

Quoted 5 lines

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.

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

+ * &struct property_entry instance.

+ * @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.

Quoted 7 lines · Dmitry Torokhov

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

drivers/base/swnode.c +1 -1

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

drivers/base/swnode.c static int property_entry_copy_data(struct property_entry *dst, · line 253

Quoted 5 lines

drivers/base/swnode.c +2 -2

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

May we have positive conditionals instead?

Quoted 2 lines

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

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

Quoted 5 lines

+ * &struct property_entry instance.

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

'separately from' -> 'outside' ?

Quoted 2 lines

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

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

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Dmitry Torokhov

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

drivers/base/swnode.c +1 -1

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

Quoted 7 lines

drivers/base/swnode.c static int property_entry_copy_data(struct property_entry *dst, · line 253

Quoted 5 lines

drivers/base/swnode.c +2 -2

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

Quoted 5 lines

Quoted 2 lines

+ * @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".

Quoted 7 lines

Quoted 5 lines

+ * &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.

Quoted 1 line

Quoted 3 lines

Quoted 1 line

+ * @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"?

Quoted 45 lines · Dmitry Torokhov

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Dmitry Torokhov

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

drivers/base/swnode.c +1 -1

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

Quoted 7 lines

drivers/base/swnode.c static int property_entry_copy_data(struct property_entry *dst, · line 253

Quoted 5 lines

drivers/base/swnode.c +2 -2

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

Quoted 5 lines

Quoted 2 lines

+ * @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".

Quoted 7 lines

Quoted 5 lines

+ * &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.

Quoted 4 lines

Quoted 2 lines

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

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

Quoted 3 lines

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

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Dmitry Torokhov

Quoted 3 lines · Andy Shevchenko

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

Quoted 5 lines

Quoted 1 line

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

Quoted 1 line

inline <-> ???

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

Quoted 11 lines · Dmitry Torokhov

Quoted 10 lines · Andy Shevchenko

Quoted 8 lines · Dmitry Torokhov

Quoted 3 lines · Andy Shevchenko

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"?

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

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

Quoted 1 line

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.

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 <[email protected]>

---
 drivers/base/swnode.c    |  9 +++++----
 include/linux/property.h | 12 +++++++-----
 2 files changed, 12 insertions(+), 9 deletions(-)

drivers/base/swnode.c +5 -4

index 8826c9040c80..ae4b24ee2a54 100644
@@ -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;
 	}
 

include/linux/property.h +6 -5

index 238e1507925f..ac7823d58cfe 100644
@@ -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_ },						\
 }

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.

drivers/base/swnode.c +4 -0

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

Quoted 4 lines · Dmitry Torokhov

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

Quoted 5 lines

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

drivers/base/swnode.c static int property_entry_copy_data(struct property_entry *dst, · line 284

Quoted 7 lines

drivers/base/swnode.c +5 -0

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

drivers/base/swnode.c static int property_entry_copy_data(struct property_entry *dst, · line 288

Quoted 5 lines

drivers/base/swnode.c +4 -0

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

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Dmitry Torokhov

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

Quoted 7 lines

Quoted 5 lines

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

Quoted 6 lines

Quoted 5 lines

drivers/base/swnode.c +4 -0

+	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
+		/* We have an opportunity to move the data inline */
+		const void *tmp = dst->pointer;
+

Quoted 3 lines

drivers/base/swnode.c static int property_entry_copy_data(struct property_entry *dst, · line 284

Quoted 1 line

drivers/base/swnode.c +1 -0

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

Quoted 28 lines · Dmitry Torokhov

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Dmitry Torokhov

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

Quoted 7 lines

Quoted 5 lines

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

Quoted 9 lines

drivers/base/swnode.c static int property_entry_copy_data(struct property_entry *dst, · line 284

Quoted 7 lines

drivers/base/swnode.c +5 -0

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

Quoted 3 lines

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.

Quoted 4 lines · Andy Shevchenko

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/[email protected]/

Quoted 14 lines · Dmitry Torokhov

Quoted 4 lines · Andy Shevchenko

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:

Quoted 5 lines

#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/[email protected]/

This is simple not a cast.

Quoted 21 lines · Andy Shevchenko

Quoted 19 lines · Dmitry Torokhov

Quoted 4 lines · Andy Shevchenko

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/[email protected]/

Quoted 1 line

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.

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 <[email protected]>

---
 drivers/base/swnode.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/base/swnode.c +0 -0

index ae4b24ee2a54..546fc1b20095 100644
@@ -277,6 +277,16 @@ static int property_entry_copy_data(struct property_entry *dst,
 		dst->value = src->value;
 	}

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 <[email protected]>

---
 drivers/platform/x86/intel_cht_int33fe.c | 81 ++++++++++++------------
 1 file changed, 41 insertions(+), 40 deletions(-)

drivers/platform/x86/intel_cht_int33fe.c +41 -40

index 1d5d877b9582..4177c5424931 100644
@@ -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

Signature · 1 line

2.23.0.700.g56cf767bdb-goog

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.

Quoted 3 lines · Dmitry Torokhov

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

Quoted 6 lines

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?

Quoted 11 lines · Andy Shevchenko

Quoted 9 lines · Dmitry Torokhov

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

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

Quoted 1 line

Can we get some test cases?

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