software node: add support for reference properties

PATCH In review Dmitry Torokhov 7 participants 28 messages Nov 8, 2019 – Nov 10, 2020

Subsystems (7)

USB ACPI Arm Samsung Exynos Driver core +3 more subsystems

lore

Share & tools

lore

Dmitry Torokhov

author

2019-11-08 04:22 UTC #0 lore raw

v8 cover software node: add support for reference properties

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

The first 2 patches do away with separate handling of arrays vs single-value properties, and treat everything as arrays (which really matches how we handle OF or ACPI properties). Instead we recognize that data, if it is small enough, may be embedded into property_entry structure. As a side effect, properties can be converted from having their data stored separately to embedding their data when they are being copied.

Patch #3 implements PROPERTY_ENTRY_REF() and friends; patch #4 converts the user of references to the property syntax, and patch #5 removes the remains of references as entities that are managed separately.

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

Note that patches #4 and $5 can be postponed.

Changes in v8:
- switch data members of "value" union to be arrays to clearly show that even when embedding we may be dealing with more than one element
- when parsing references use property_entry_read_int_array() instead of fetching u32 value directly from nargs property

Changes in v7:
- rebased onto next-20191107
- dropped already applied patches
- reworked patch that moved small properties inline on copying to avoid temporary allocation
- cleaned up logic for embedding vs storing values out-of-line
- fixed handling of embedded 2-element string array on x32

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

Changes in v5:
- rebased onto next-20191011

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

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

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

Dmitry Torokhov (6):
software node: rename is_array to is_inline
software node: allow embedding of small arrays into property_entry software node: implement reference properties platform/x86: intel_cht_int33fe: use inline reference properties software node: remove separate handling of references software node: add basic tests for property entries

 drivers/base/swnode.c                         | 154 +++---
 drivers/base/test/Makefile                    |   2 +
 drivers/base/test/property-entry-test.c       | 472 ++++++++++++++++++
 .../platform/x86/intel_cht_int33fe_typec.c    |  81 +--
 include/linux/property.h                      |  98 ++--
 5 files changed, 655 insertions(+), 152 deletions(-)
 create mode 100644 drivers/base/test/property-entry-test.c

Signature · 1 line

2.24.0.rc1.363.gb1bccd3e3d-goog

Dmitry Torokhov

author

2019-11-08 04:22 UTC #1 lore raw

PATCH v8 1/6 software node: rename is_array to is_inline

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

drivers/base/swnode.c +5 -7

index d8d0dc0ca5acf..18a30fb3cc588 100644
@@ -108,10 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
 	if (!prop->length)
 		return NULL;
 
-	if (prop->is_array)
-		return prop->pointer;
-
-	return &prop->value;
+	return prop->is_inline ? &prop->value : prop->pointer;
 }
 
 static const void *property_entry_find(const struct property_entry *props,

@@ -205,7 +202,7 @@ static void property_entry_free_data(const struct property_entry *p)
 	const char * const *src_str;
 	size_t i, nval;
 
-	if (p->is_array) {
+	if (!p->is_inline) {
 		if (p->type == DEV_PROP_STRING && p->pointer) {
 			src_str = p->pointer;
 			nval = p->length / sizeof(const char *);

@@ -250,7 +247,7 @@ static int property_entry_copy_data(struct property_entry *dst,
 	const void *pointer = property_get_pointer(src);
 	const void *new;
 
-	if (src->is_array) {
+	if (!src->is_inline) {
 		if (!src->length)
 			return -ENODATA;

@@ -264,15 +261,16 @@ static int property_entry_copy_data(struct property_entry *dst,
 				return -ENOMEM;
 		}
 
-		dst->is_array = true;
 		dst->pointer = new;
 	} else if (src->type == DEV_PROP_STRING) {
 		new = kstrdup(src->value.str, GFP_KERNEL);
 		if (!new && src->value.str)
 			return -ENOMEM;
 
+		dst->is_inline = true;
 		dst->value.str = new;
 	} else {
+		dst->is_inline = true;
 		dst->value = src->value;
 	}

include/linux/property.h +8 -5

index 48335288c2a96..dad0ad11b55e2 100644
@@ -227,15 +227,17 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
  * @length: Length of data making up the value.
- * @is_array: True when the property is an array.
+ * @is_inline: True when the property value is embedded in
+ *	&struct property_entry instance.
  * @type: Type of the data in unions.
- * @pointer: Pointer to the property (an array of items of the given type).
- * @value: Value of the property (when it is a single item of the given type).
+ * @pointer: Pointer to the property when it is stored separately from
+ *	the &struct property_entry instance.
+ * @value: Value of the property when it is stored inline.
  */
 struct property_entry {
 	const char *name;
 	size_t length;
-	bool is_array;
+	bool is_inline;
 	enum dev_prop_type type;
 	union {
 		const void *pointer;

@@ -262,7 +264,6 @@ struct property_entry {
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
-	.is_array = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .pointer = _val_ },						\
 }

@@ -293,6 +294,7 @@ struct property_entry {
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),		\
+	.is_inline = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .value = { ._elem_ = _val_ } },				\
 }

@@ -311,6 +313,7 @@ struct property_entry {
 #define PROPERTY_ENTRY_BOOL(_name_)		\
 (struct property_entry) {			\
 	.name = _name_,				\
+	.is_inline = true,			\
 }
 
 struct property_entry *

Signature · 1 line

2.24.0.rc1.363.gb1bccd3e3d-goog

Rafael J. Wysocki

maintainer

2019-11-08 09:49 UTC #2 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

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

So the subject is slightly misleading, because it is not a rename. I would say "replace x with y" instead.

[Arguably I can change that when applying the patch, but since we are going to wait for the dependencies to go in, it should not be a big deal to send an update of this patch alone?]

Quoted 65 lines

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

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

drivers/base/swnode.c +5 -7

index d8d0dc0ca5acf..18a30fb3cc588 100644
@@ -108,10 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
        if (!prop->length)
                return NULL;
 
-       if (prop->is_array)
-               return prop->pointer;
-
-       return &prop->value;
+       return prop->is_inline ? &prop->value : prop->pointer;
 }
 
 static const void *property_entry_find(const struct property_entry *props,

@@ -205,7 +202,7 @@ static void property_entry_free_data(const struct property_entry *p)
        const char * const *src_str;
        size_t i, nval;
 
-       if (p->is_array) {
+       if (!p->is_inline) {
                if (p->type == DEV_PROP_STRING && p->pointer) {
                        src_str = p->pointer;
                        nval = p->length / sizeof(const char *);

@@ -250,7 +247,7 @@ static int property_entry_copy_data(struct property_entry *dst,
        const void *pointer = property_get_pointer(src);
        const void *new;
 
-       if (src->is_array) {
+       if (!src->is_inline) {
                if (!src->length)
                        return -ENODATA;

@@ -264,15 +261,16 @@ static int property_entry_copy_data(struct property_entry *dst,
                                return -ENOMEM;
                }
 
-               dst->is_array = true;
                dst->pointer = new;
        } else if (src->type == DEV_PROP_STRING) {
                new = kstrdup(src->value.str, GFP_KERNEL);
                if (!new && src->value.str)
                        return -ENOMEM;
 
+               dst->is_inline = true;
                dst->value.str = new;
        } else {
+               dst->is_inline = true;
                dst->value = src->value;
        }

include/linux/property.h +0 -0

index 48335288c2a96..dad0ad11b55e2 100644
@@ -227,15 +227,17 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.

include/linux/property.h static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode, · line 235

Quoted 10 lines

include/linux/property.h +5 -3

  * @length: Length of data making up the value.
- * @is_array: True when the property is an array.
+ * @is_inline: True when the property value is embedded in
+ *     &struct property_entry instance.
  * @type: Type of the data in unions.
- * @pointer: Pointer to the property (an array of items of the given type).
- * @value: Value of the property (when it is a single item of the given type).
+ * @pointer: Pointer to the property when it is stored separately from
+ *     the &struct property_entry instance.
+ * @value: Value of the property when it is stored inline.

And while at it, can you please try to make the comments shorter so they each take one line?

Bjørn Mork

2019-11-13 06:52 UTC #3 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

In reply to Dmitry Torokhov

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

Doesn't a non-null prop->pointer tell you this?

And inverting the flag is unnecessarily risky IMHO. An all-zero prop might now result in dereferencing a NULL prop->pointer instead of using the empty prop->value. Now I haven't looked at the code to see if this is a real problem. But I believe it's better not having to do that anyway...

Bjørn

Dmitry Torokhov

author

2019-11-13 08:08 UTC #4 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

Quoted 8 lines · Bjørn Mork

Quoted 4 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.
Doesn't a non-null prop->pointer tell you this?

No it does not because pointer is a part of a union.

Quoted 6 lines

And inverting the flag is unnecessarily risky IMHO. An all-zero prop might now result in dereferencing a NULL prop->pointer instead of using the empty prop->value. Now I haven't looked at the code to see if this is a real problem. But I believe it's better not having to do that anyway...

All-zero property is a terminator and thus we will not dereference anything.

Thanks.

Signature · 1 line

Dmitry

Marek Szyprowski

2019-12-12 11:12 UTC #5 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

In reply to Dmitry Torokhov

Dear All,

Quoted 6 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.
Signed-off-by Dmitry Torokhov <[email protected]>

Today I've noticed that this patch got merged to linux-next as commit e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI driver operation on Samsung Exynos5 SoCs (and probably on other SoCs which use DWC3 in host mode too). I get the following errors during boot:

dwc3 12000000.dwc3: failed to add properties to xHCI dwc3 12000000.dwc3: failed to initialize host
dwc3: probe of 12000000.dwc3 failed with error -61

Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:

https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt

(lack of 'ref' clk is not related nor fatal to the driver operation).

The code which fails after this patch is located in drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

Quoted 100 lines

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

drivers/base/swnode.c +5 -7

index d8d0dc0ca5acf..18a30fb3cc588 100644
@@ -108,10 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
  	if (!prop->length)
  		return NULL;
 
-	if (prop->is_array)
-		return prop->pointer;
-
-	return &prop->value;
+	return prop->is_inline ? &prop->value : prop->pointer;
  }
 
  static const void *property_entry_find(const struct property_entry *props,

@@ -205,7 +202,7 @@ static void property_entry_free_data(const struct property_entry *p)
  	const char * const *src_str;
  	size_t i, nval;
 
-	if (p->is_array) {
+	if (!p->is_inline) {
  		if (p->type == DEV_PROP_STRING && p->pointer) {
  			src_str = p->pointer;
  			nval = p->length / sizeof(const char *);

@@ -250,7 +247,7 @@ static int property_entry_copy_data(struct property_entry *dst,
  	const void *pointer = property_get_pointer(src);
  	const void *new;
 
-	if (src->is_array) {
+	if (!src->is_inline) {
  		if (!src->length)
  			return -ENODATA;

@@ -264,15 +261,16 @@ static int property_entry_copy_data(struct property_entry *dst,
  				return -ENOMEM;
  		}
 
-		dst->is_array = true;
  		dst->pointer = new;
  	} else if (src->type == DEV_PROP_STRING) {
  		new = kstrdup(src->value.str, GFP_KERNEL);
  		if (!new && src->value.str)
  			return -ENOMEM;
 
+		dst->is_inline = true;
  		dst->value.str = new;
  	} else {
+		dst->is_inline = true;
  		dst->value = src->value;
  	}

include/linux/property.h +7 -5

index 48335288c2a96..dad0ad11b55e2 100644
@@ -227,15 +227,17 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
   * struct property_entry - "Built-in" device property representation.
   * @name: Name of the property.
   * @length: Length of data making up the value.
- * @is_array: True when the property is an array.
+ * @is_inline: True when the property value is embedded in
+ *	&struct property_entry instance.
   * @type: Type of the data in unions.
- * @pointer: Pointer to the property (an array of items of the given type).
- * @value: Value of the property (when it is a single item of the given type).
+ * @pointer: Pointer to the property when it is stored separately from
+ *	the &struct property_entry instance.
+ * @value: Value of the property when it is stored inline.
   */
  struct property_entry {
  	const char *name;
  	size_t length;
-	bool is_array;
+	bool is_inline;
  	enum dev_prop_type type;
  	union {
  		const void *pointer;

@@ -262,7 +264,6 @@ struct property_entry {
  (struct property_entry) {						\
  	.name = _name_,							\
  	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
-	.is_array = true,						\
  	.type = DEV_PROP_##_Type_,					\
  	{ .pointer = _val_ },						\
  }

@@ -293,6 +294,7 @@ struct property_entry {
  (struct property_entry) {						\
  	.name = _name_,							\
  	.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),		\
+	.is_inline = true,						\
  	.type = DEV_PROP_##_Type_,					\
  	{ .value = { ._elem_ = _val_ } },				\
  }

include/linux/property.h struct property_entry { · line 319

Quoted 7 lines

include/linux/property.h +1 -0

@@ -311,6 +313,7 @@ struct property_entry {
  #define PROPERTY_ENTRY_BOOL(_name_)		\
  (struct property_entry) {			\
  	.name = _name_,				\
+	.is_inline = true,			\
  }
 
  struct property_entry *

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

Andy Shevchenko

reviewer

2019-12-12 11:28 UTC #6 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

Quoted 19 lines · Marek Szyprowski

Dear All,
Quoted 6 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.
Signed-off-by Dmitry Torokhov <[email protected]>
Today I've noticed that this patch got merged to linux-next as commit e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI driver operation on Samsung Exynos5 SoCs (and probably on other SoCs which use DWC3 in host mode too). I get the following errors during boot:
dwc3 12000000.dwc3: failed to add properties to xHCI dwc3 12000000.dwc3: failed to initialize host
dwc3: probe of 12000000.dwc3 failed with error -61

Quoted 8 lines

Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
(lack of 'ref' clk is not related nor fatal to the driver operation).
The code which fails after this patch is located in drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

Thank you for report.

I think we should not have that patch in the fist place... I used to have a bad feeling about it and then forgot about it existence.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Rafael J. Wysocki

maintainer

2019-12-12 16:41 UTC #7 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

Quoted 27 lines · Andy Shevchenko

Quoted 25 lines · Marek Szyprowski
Dear All,
Quoted 6 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.
Signed-off-by Dmitry Torokhov <[email protected]>
Today I've noticed that this patch got merged to linux-next as commit e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI driver operation on Samsung Exynos5 SoCs (and probably on other SoCs which use DWC3 in host mode too). I get the following errors during boot:
dwc3 12000000.dwc3: failed to add properties to xHCI dwc3 12000000.dwc3: failed to initialize host
dwc3: probe of 12000000.dwc3 failed with error -61
Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
(lack of 'ref' clk is not related nor fatal to the driver operation).

Quoted 7 lines

Quoted 2 lines
The code which fails after this patch is located in drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
Thank you for report.
I think we should not have that patch in the fist place... I used to have a bad feeling about it and then forgot about it existence.

Well, I think you mean the [2/6].

The $subject one really shouldn't change functionality, we must have missed something here.

Anyway, I'll drop this branch from the linux-next one for now.

Marek Szyprowski

2019-12-13 06:47 UTC #8 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

Hi Rafael,

Quoted 27 lines · Rafael J. Wysocki

Quoted 25 lines · Andy Shevchenko
Quoted 24 lines · Marek Szyprowski
Quoted 6 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.
Signed-off-by Dmitry Torokhov <[email protected]>
Today I've noticed that this patch got merged to linux-next as commit e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI driver operation on Samsung Exynos5 SoCs (and probably on other SoCs which use DWC3 in host mode too). I get the following errors during boot:
dwc3 12000000.dwc3: failed to add properties to xHCI dwc3 12000000.dwc3: failed to initialize host
dwc3: probe of 12000000.dwc3 failed with error -61
Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
(lack of 'ref' clk is not related nor fatal to the driver operation).
The code which fails after this patch is located in drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

Quoted 8 lines

Quoted 4 lines
Thank you for report.
I think we should not have that patch in the fist place... I used to have a bad feeling about it and then forgot about it existence.
Well, I think you mean the [2/6].
The $subject one really shouldn't change functionality, we must have missed something here.

Nope, I was really talking about [1/6]. It looks that it revealed an issue in the DWC3 driver pointed by Dmitry.

Quoted 1 line

Anyway, I'll drop this branch from the linux-next one for now.

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

Rafael J. Wysocki

maintainer

2019-12-13 08:37 UTC #9 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

Quoted 35 lines · Marek Szyprowski

Hi Rafael,
Quoted 31 lines · Rafael J. Wysocki
Quoted 29 lines · Andy Shevchenko
Quoted 16 lines · Marek Szyprowski
Quoted 6 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.
Signed-off-by Dmitry Torokhov <[email protected]>
Today I've noticed that this patch got merged to linux-next as commit e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI driver operation on Samsung Exynos5 SoCs (and probably on other SoCs which use DWC3 in host mode too). I get the following errors during boot:
dwc3 12000000.dwc3: failed to add properties to xHCI dwc3 12000000.dwc3: failed to initialize host
dwc3: probe of 12000000.dwc3 failed with error -61
Quoted 8 lines
Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
(lack of 'ref' clk is not related nor fatal to the driver operation).
The code which fails after this patch is located in drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
Thank you for report.
I think we should not have that patch in the fist place... I used to have a bad feeling about it and then forgot about it existence.

Quoted 7 lines

Quoted 4 lines
Well, I think you mean the [2/6].
The $subject one really shouldn't change functionality, we must have missed something here.
Nope, I was really talking about [1/6]. It looks that it revealed an issue in the DWC3 driver pointed by Dmitry.

Right, but I was referring to the Andy's comment.

Cheers!

Dmitry Torokhov

author

2019-12-13 01:24 UTC #10 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

In reply to Marek Szyprowski

Hi Marek,

Quoted 19 lines · Marek Szyprowski

Dear All,
Quoted 6 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.
Signed-off-by Dmitry Torokhov <[email protected]>
Today I've noticed that this patch got merged to linux-next as commit e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI driver operation on Samsung Exynos5 SoCs (and probably on other SoCs which use DWC3 in host mode too). I get the following errors during boot:
dwc3 12000000.dwc3: failed to add properties to xHCI dwc3 12000000.dwc3: failed to initialize host
dwc3: probe of 12000000.dwc3 failed with error -61

Quoted 8 lines

Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
(lack of 'ref' clk is not related nor fatal to the driver operation).
The code which fails after this patch is located in drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

Does the following help? If, as I expect, it does, I'll submit it formally.

---

drivers/usb/dwc3/host.c +3 -3

index 5567ed2cddbec..fa252870c926f 100644
@@ -88,10 +88,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
 
 	if (dwc->usb3_lpm_capable)
-		props[prop_idx++].name = "usb3-lpm-capable";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb3-lpm-capable");
 
 	if (dwc->usb2_lpm_disable)
-		props[prop_idx++].name = "usb2-lpm-disable";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation

@@ -103,7 +103,7 @@ int dwc3_host_init(struct dwc3 *dwc)
 	 * This following flag tells XHCI to do just that.
 	 */
 	if (dwc->revision <= DWC3_REVISION_300A)
-		props[prop_idx++].name = "quirk-broken-port-ped";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped");
 
 	if (prop_idx) {
 		ret = platform_device_add_properties(xhci, props);

Signature · 1 line

Dmitry

Marek Szyprowski

Tested-by

2019-12-13 06:44 UTC #11 lore raw

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

Hi Dmitry,

Quoted 19 lines · Dmitry Torokhov

Quoted 18 lines · Marek Szyprowski
Quoted 6 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.
Signed-off-by Dmitry Torokhov <[email protected]>
Today I've noticed that this patch got merged to linux-next as commit e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI driver operation on Samsung Exynos5 SoCs (and probably on other SoCs which use DWC3 in host mode too). I get the following errors during boot:
dwc3 12000000.dwc3: failed to add properties to xHCI dwc3 12000000.dwc3: failed to initialize host
dwc3: probe of 12000000.dwc3 failed with error -61
Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:

Quoted 8 lines

Quoted 6 lines
https://protect2.fireeye.com/url?k=df93ba76-820d14ec-df923139-0cc47a336fae-f751d8b108bc5bdf&u=https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
(lack of 'ref' clk is not related nor fatal to the driver operation).
The code which fails after this patch is located in drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
Does the following help? If, as I expect, it does, I'll submit it formally.

Yes, it fixes the issue. If possible, please add the following tags:

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

to the final patch.

Quoted 22 lines

---

drivers/usb/dwc3/host.c +2 -2

index 5567ed2cddbec..fa252870c926f 100644
@@ -88,10 +88,10 @@ int dwc3_host_init(struct dwc3 *dwc)
  	memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
 
  	if (dwc->usb3_lpm_capable)
-		props[prop_idx++].name = "usb3-lpm-capable";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb3-lpm-capable");
 
  	if (dwc->usb2_lpm_disable)
-		props[prop_idx++].name = "usb2-lpm-disable";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
  	/**
  	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 109

Quoted 8 lines

drivers/usb/dwc3/host.c +1 -1

@@ -103,7 +103,7 @@ int dwc3_host_init(struct dwc3 *dwc)
  	 * This following flag tells XHCI to do just that.
  	 */
  	if (dwc->revision <= DWC3_REVISION_300A)
-		props[prop_idx++].name = "quirk-broken-port-ped";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped");
 
  	if (prop_idx) {
  		ret = platform_device_add_properties(xhci, props);

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

Dmitry Torokhov

author

2019-11-08 04:22 UTC #12 lore raw

PATCH v8 2/6 software node: allow embedding of small arrays into property_entry

In reply to Dmitry Torokhov

We should not conflate whether a property data is an array or a single value with where it is stored (embedded into property_entry structure or out-of-line). All single-value properties are in effect 1-element arrays, and we can figure the amount of data stored in a property by examining its length and the data type. And arrays can be as easily stored in property entry instances as single values are, provided that we have enough space (we have up to 8 bytes). We can embed:

- up to 8 bytes from U8 arrays
- up to 4 words
- up to 2 double words
- one U64 value
- one (on 64 bit architectures) or 2 (on 32 bit) strings.

This change also has an effect of switching properties with small amount of data to embed it instead of keeping it separate when copying such properties.

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

---
 drivers/base/swnode.c    | 115 +++++++++++++++++++--------------------
 include/linux/property.h |  14 ++---
 2 files changed, 62 insertions(+), 67 deletions(-)

drivers/base/swnode.c +55 ~~-60~~

index 18a30fb3cc588..3d422918a53d9 100644
@@ -198,93 +198,84 @@ 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_inline) {
-		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(src_str[i]);
-		}
-		kfree(pointer);
-	} else if (p->type == DEV_PROP_STRING) {
-		kfree(p->value.str);
+	if (p->type == DEV_PROP_STRING) {
+		src_str = property_get_pointer(p);
+		nval = p->length / sizeof(*src_str);
+		for (i = 0; i < nval; i++)
+			kfree(src_str[i]);
 	}
+
+	if (!p->is_inline)
+		kfree(p->pointer);
+
 	kfree(p->name);
 }
 
-static const char * const *
-property_copy_string_array(const struct property_entry *src)
+static bool property_copy_string_array(const char **dst_ptr,
+				       const char * const *src_ptr,
+				       size_t nval)
 {
-	const char **d;
-	const char * const *src_str = src->pointer;
-	size_t nval = src->length / sizeof(*d);
 	int i;
 
-	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
-	if (!d)
-		return NULL;
-
 	for (i = 0; i < nval; i++) {
-		d[i] = kstrdup(src_str[i], GFP_KERNEL);
-		if (!d[i] && src_str[i]) {
+		dst_ptr[i] = kstrdup(src_ptr[i], GFP_KERNEL);
+		if (!dst_ptr[i] && src_ptr[i]) {
 			while (--i >= 0)
-				kfree(d[i]);
-			kfree(d);
-			return NULL;
+				kfree(dst_ptr[i]);
+			return false;
 		}
 	}
 
-	return d;
+	return true;
 }
 
 static int property_entry_copy_data(struct property_entry *dst,
 				    const struct property_entry *src)
 {
 	const void *pointer = property_get_pointer(src);
-	const void *new;
-
-	if (!src->is_inline) {
-		if (!src->length)
-			return -ENODATA;
-
-		if (src->type == DEV_PROP_STRING) {
-			new = property_copy_string_array(src);
-			if (!new)
-				return -ENOMEM;
-		} else {
-			new = kmemdup(pointer, src->length, GFP_KERNEL);
-			if (!new)
-				return -ENOMEM;
-		}
-
-		dst->pointer = new;
-	} else if (src->type == DEV_PROP_STRING) {
-		new = kstrdup(src->value.str, GFP_KERNEL);
-		if (!new && src->value.str)
+	void *dst_ptr;
+	size_t nval;
+
+	/*
+	 * Properties with no data should not be marked as stored
+	 * out of line.
+	 */
+	if (!src->is_inline && !src->length)
+		return -ENODATA;
+
+	if (src->length <= sizeof(dst->value)) {
+		dst_ptr = &dst->value;
+		dst->is_inline = true;
+	} else {
+		dst_ptr = kmalloc(src->length, GFP_KERNEL);
+		if (!dst_ptr)
 			return -ENOMEM;
+		dst->pointer = dst_ptr;
+	}
 
-		dst->is_inline = true;
-		dst->value.str = new;
+	if (src->type == DEV_PROP_STRING) {
+		nval = src->length / sizeof(const char *);
+		if (!property_copy_string_array(dst_ptr, pointer, nval)) {
+			if (!dst->is_inline)
+				kfree(dst->pointer);
+			return -ENOMEM;
+		}
 	} else {
-		dst->is_inline = true;
-		dst->value = src->value;
+		memcpy(dst_ptr, pointer, src->length);
 	}
 
 	dst->length = src->length;
 	dst->type = src->type;
 	dst->name = kstrdup(src->name, GFP_KERNEL);
-	if (!dst->name)
-		goto out_free_data;
+	if (!dst->name) {
+		property_entry_free_data(dst);
+		return -ENOMEM;
+	}
 
 	return 0;
-
-out_free_data:
-	property_entry_free_data(dst);
-	return -ENOMEM;
 }
 
 /**

@@ -484,6 +475,8 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	const struct software_node_reference *ref;
 	const struct property_entry *prop;
 	struct fwnode_handle *refnode;
+	u32 nargs_prop_val;
+	int error;
 	int i;
 
 	if (!swnode || !swnode->node->references)

@@ -501,11 +494,13 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 		return -ENOENT;
 
 	if (nargs_prop) {
-		prop = property_entry_get(swnode->node->properties, nargs_prop);
-		if (!prop)
-			return -EINVAL;
+		error = property_entry_read_int_array(swnode->node->properties,
+						      nargs_prop, sizeof(u32),
+						      &nargs_prop_val, 1);
+		if (error)
+			return error;
 
-		nargs = prop->value.u32_data;
+		nargs = nargs_prop_val;
 	}
 
 	if (nargs > NR_FWNODE_REFERENCE_ARGS)

include/linux/property.h +7 -7

index dad0ad11b55e2..c592c286e3394 100644
@@ -242,11 +242,11 @@ struct property_entry {
 	union {
 		const void *pointer;
 		union {
-			u8 u8_data;
-			u16 u16_data;
-			u32 u32_data;
-			u64 u64_data;
-			const char *str;
+			u8 u8_data[sizeof(u64) / sizeof(u8)];
+			u16 u16_data[sizeof(u64) / sizeof(u16)];
+			u32 u32_data[sizeof(u64) / sizeof(u32)];
+			u64 u64_data[sizeof(u64) / sizeof(u64)];
+			const char *str[sizeof(u64) / sizeof(char *)];
 		} value;
 	};
 };

@@ -258,7 +258,7 @@ struct property_entry {
  */
 
 #define __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_)				\
-	sizeof(((struct property_entry *)NULL)->value._elem_)
+	sizeof(((struct property_entry *)NULL)->value._elem_[0])
 
 #define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _elem_, _Type_, _val_, _len_)\
 (struct property_entry) {						\

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

Signature · 1 line

2.24.0.rc1.363.gb1bccd3e3d-goog

Dmitry Torokhov

author

2019-11-08 04:22 UTC #13 lore raw

PATCH v8 3/6 software node: implement reference properties

In reply to Dmitry Torokhov

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

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

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

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

---
 drivers/base/swnode.c    | 49 ++++++++++++++++++++++++++++------
 include/linux/property.h | 57 +++++++++++++++++++++++++++++-----------
 2 files changed, 82 insertions(+), 24 deletions(-)

drivers/base/swnode.c +41 -8

index 3d422918a53d9..604d7327bba79 100644
@@ -246,6 +246,13 @@ static int property_entry_copy_data(struct property_entry *dst,
 	if (!src->is_inline && !src->length)
 		return -ENODATA;
 
+	/*
+	 * Reference properties are never stored inline as
+	 * they are too big.
+	 */
+	if (src->type == DEV_PROP_REF && src->is_inline)
+		return -EINVAL;
+
 	if (src->length <= sizeof(dst->value)) {
 		dst_ptr = &dst->value;
 		dst->is_inline = true;

@@ -473,23 +480,49 @@ 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;
 	u32 nargs_prop_val;
 	int error;
 	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;

@@ -510,7 +543,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;
 }

include/linux/property.h +41 ~~-16~~

105 lines not on this page · raw message

Signature · 1 line

2.24.0.rc1.363.gb1bccd3e3d-goog

Lukasz Stelmach

2020-11-09 17:03 UTC #14 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

Quoted 15 lines · when Dmitry Torokhov

It is possible to store references to software nodes in the same fashion as other static properties, so that users do not need to define separate structures:
static const struct software_node gpio_bank_b_node = {
	.name = "B",
};
static const struct property_entry simone_key_enter_props[] = {
	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
	PROPERTY_ENTRY_STRING("label", "enter"),
	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
	{ }
};

Quoted 2 lines

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

I am writing a piece that needs to provide a list of gpios to a diriver. The above example looks like what I need.

At the moment the driver gets the list from fwnode/of_node. The list contain references to phandles which get resolved and and the driver ends up with a bunch of gpio descriptors. Great.

This example looks nice but does the code that reads the reference from the gpios property and returns a gpiod actually exist? If it doesn't, I am willing to write it.

At first glance it makes more sense to me to pass (struct gpiod_lookup *) instead of (struct software_node *) and make gpiolib's gpiod_find() accept lookup tables as parameter instead of searching the gpio_lookup_list? Or do you think such temporary table should be assembled from the above structure and then used in gpiod_find()?

Any other suggestions on how to get a bunch of gpios (the description for gpios is available in the devicetree) for a device described with a software nodes?

Kind regards,

Signature · 3 lines

Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Andy Shevchenko

reviewer

2020-11-09 17:23 UTC #15 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

Quoted 16 lines · Lukasz Stelmach

Quoted 15 lines · when Dmitry Torokhov
It is possible to store references to software nodes in the same fashion as other static properties, so that users do not need to define separate structures:
static const struct software_node gpio_bank_b_node = {
	.name = "B",
};
static const struct property_entry simone_key_enter_props[] = {
	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
	PROPERTY_ENTRY_STRING("label", "enter"),
	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
	{ }
};

Quoted 5 lines

Quoted 2 lines
Signed-off-by Dmitry Torokhov <[email protected]>
---
I am writing a piece that needs to provide a list of gpios to a diriver. The above example looks like what I need.

Nope.

It mustn't be used for GPIOs or PWMs or whatever that either should come via lookup tables or corresponding firmware interface.

Quoted 17 lines

At the moment the driver gets the list from fwnode/of_node. The list contain references to phandles which get resolved and and the driver ends up with a bunch of gpio descriptors. Great.
This example looks nice but does the code that reads the reference from the gpios property and returns a gpiod actually exist? If it doesn't, I am willing to write it.
At first glance it makes more sense to me to pass (struct gpiod_lookup *) instead of (struct software_node *) and make gpiolib's gpiod_find() accept lookup tables as parameter instead of searching the gpio_lookup_list? Or do you think such temporary table should be assembled from the above structure and then used in gpiod_find()?
Any other suggestions on how to get a bunch of gpios (the description for gpios is available in the devicetree) for a device described with a software nodes?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Lukasz Stelmach

2020-11-09 18:19 UTC #16 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

Quoted 20 lines · when Andy Shevchenko

Quoted 19 lines · Lukasz Stelmach
Quoted 17 lines · when Dmitry Torokhov
It is possible to store references to software nodes in the same fashion as other static properties, so that users do not need to define separate structures:
static const struct software_node gpio_bank_b_node = {
	.name = "B",
};
static const struct property_entry simone_key_enter_props[] = {
	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
	PROPERTY_ENTRY_STRING("label", "enter"),
	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
	{ }
};
Signed-off-by Dmitry Torokhov <[email protected]>
---

Quoted 8 lines

Quoted 2 lines
I am writing a piece that needs to provide a list of gpios to a diriver. The above example looks like what I need.
Nope.
It mustn't be used for GPIOs or PWMs or whatever that either should come via lookup tables or corresponding firmware interface.

May I ask why? I've read commit descriptions for drivers/base/swnode.c and the discussion on lkml and I understand software nodes as a way to provide (synthesize) a description for a device that is missing a description in the firmware. Another use case seems to be to replace (in the long run) platform data. That is what I am trying to use it for.

I want my device to be configured with either DT or software_nodes created at run time with configfs. My device is going to use GPIOs described in the DT and it is going to be configured via configfs at run time. I could use platform_data to pass structures from configfs but software nodes would let me save some code in the device driver and use the same paths for both static (DT) and dynamic (configfs) configuration.

Probably I have missed something and I will be greatful, if you tell me where I can find more information about software nodes. There are few users in the kernel and it isn't obvious for me how to use software nodes properly.

Quoted 14 lines

Quoted 14 lines
At the moment the driver gets the list from fwnode/of_node. The list contain references to phandles which get resolved and and the driver ends up with a bunch of gpio descriptors. Great.
This example looks nice but does the code that reads the reference from the gpios property and returns a gpiod actually exist? If it doesn't, I am willing to write it.
At first glance it makes more sense to me to pass (struct gpiod_lookup *) instead of (struct software_node *) and make gpiolib's gpiod_find() accept lookup tables as parameter instead of searching the gpio_lookup_list? Or do you think such temporary table should be assembled from the above structure and then used in gpiod_find()?

Quoted 3 lines

Quoted 3 lines
Any other suggestions on how to get a bunch of gpios (the description for gpios is available in the devicetree) for a device described with a software nodes?

Kind regards,

Signature · 3 lines

Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Dmitry Torokhov

author

2020-11-09 18:53 UTC #17 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

Quoted 44 lines · Lukasz Stelmach

Quoted 20 lines · when Andy Shevchenko
Quoted 19 lines · Lukasz Stelmach
Quoted 17 lines · when Dmitry Torokhov
It is possible to store references to software nodes in the same fashion as other static properties, so that users do not need to define separate structures:
static const struct software_node gpio_bank_b_node = {
	.name = "B",
};
static const struct property_entry simone_key_enter_props[] = {
	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
	PROPERTY_ENTRY_STRING("label", "enter"),
	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
	{ }
};
Signed-off-by Dmitry Torokhov <[email protected]>
---
Quoted 8 lines
Quoted 2 lines
I am writing a piece that needs to provide a list of gpios to a diriver. The above example looks like what I need.
Nope.
It mustn't be used for GPIOs or PWMs or whatever that either should come via lookup tables or corresponding firmware interface.
May I ask why? I've read commit descriptions for drivers/base/swnode.c and the discussion on lkml and I understand software nodes as a way to provide (synthesize) a description for a device that is missing a description in the firmware. Another use case seems to be to replace (in the long run) platform data. That is what I am trying to use it for.
I want my device to be configured with either DT or software_nodes created at run time with configfs. My device is going to use GPIOs described in the DT and it is going to be configured via configfs at run time. I could use platform_data to pass structures from configfs but software nodes would let me save some code in the device driver and use the same paths for both static (DT) and dynamic (configfs) configuration.

Quoted 4 lines

Probably I have missed something and I will be greatful, if you tell me where I can find more information about software nodes. There are few users in the kernel and it isn't obvious for me how to use software nodes properly.

Yeah, I disagree with Andy here. The lookup tables are a crutch that we have until GPIO and PWM a taught to support software nodes (I need to resurrect my patch series for GPIO, if you have time to test that would be awesome).

Thanks.

Signature · 1 line

Dmitry

Andy Shevchenko

reviewer

2020-11-09 19:04 UTC #18 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

Quoted 2 lines · Dmitry Torokhov

Quoted 1 line · Lukasz Stelmach
It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:

...

Quoted 9 lines

Quoted 4 lines
Probably I have missed something and I will be greatful, if you tell me where I can find more information about software nodes. There are few users in the kernel and it isn't obvious for me how to use software nodes properly.
Yeah, I disagree with Andy here. The lookup tables are a crutch that we have until GPIO and PWM a taught to support software nodes (I need to resurrect my patch series for GPIO, if you have time to test that would be awesome).

We don't have support for list of fwnodes, this will probably break things where swnode is already exist.

I think Heikki may send a documentation patch to clarify when swnodes can and can't be used. Maybe I'm mistaken and above is a good use case by design.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Heikki Krogerus

reviewer

2020-11-10 12:39 UTC #19 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

Quoted 16 lines · Andy Shevchenko

Quoted 2 lines · Dmitry Torokhov
Quoted 1 line · Lukasz Stelmach
It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
...
Quoted 9 lines
Quoted 4 lines
Probably I have missed something and I will be greatful, if you tell me where I can find more information about software nodes. There are few users in the kernel and it isn't obvious for me how to use software nodes properly.
Yeah, I disagree with Andy here. The lookup tables are a crutch that we have until GPIO and PWM a taught to support software nodes (I need to resurrect my patch series for GPIO, if you have time to test that would be awesome).

Quoted 5 lines

We don't have support for list of fwnodes, this will probably break things where swnode is already exist.
I think Heikki may send a documentation patch to clarify when swnodes can and can't be used. Maybe I'm mistaken and above is a good use case by design.

Yeah, the problem is that I'm not sure that we can limit the swnodes for any specific purpose, or dictate very strictly how they are used with other possible fwnodes.

Right now I'm thinking we should simply not talk about the relationship a software node should have or can have with other fwnodes (regardless of their type) in the swnode documentation.

Br,

Signature · 1 line

heikki

Rafael J. Wysocki

maintainer

2020-11-10 12:47 UTC #20 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

Quoted 21 lines · Heikki Krogerus

Quoted 19 lines · Andy Shevchenko
Quoted 2 lines · Dmitry Torokhov
Quoted 1 line · Lukasz Stelmach
It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
...
Quoted 9 lines
Quoted 4 lines
Probably I have missed something and I will be greatful, if you tell me where I can find more information about software nodes. There are few users in the kernel and it isn't obvious for me how to use software nodes properly.
Yeah, I disagree with Andy here. The lookup tables are a crutch that we have until GPIO and PWM a taught to support software nodes (I need to resurrect my patch series for GPIO, if you have time to test that would be awesome).
We don't have support for list of fwnodes, this will probably break things where swnode is already exist.

Quoted 6 lines

Quoted 2 lines
I think Heikki may send a documentation patch to clarify when swnodes can and can't be used. Maybe I'm mistaken and above is a good use case by design.
Yeah, the problem is that I'm not sure that we can limit the swnodes for any specific purpose, or dictate very strictly how they are used with other possible fwnodes.

Generally agreed, but if there are known problems, they need to be documented at least for the time being until they are addressed.

Quoted 3 lines

Right now I'm thinking we should simply not talk about the relationship a software node should have or can have with other fwnodes (regardless of their type) in the swnode documentation.

This sounds reasonable to me, with the above exception.

Lukasz Stelmach

2020-11-09 19:55 UTC #21 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

In reply to Dmitry Torokhov

Quoted 50 lines · when Dmitry Torokhov

Quoted 44 lines · Lukasz Stelmach
Quoted 20 lines · when Andy Shevchenko
Quoted 19 lines · Lukasz Stelmach
Quoted 17 lines · when Dmitry Torokhov
It is possible to store references to software nodes in the same fashion as other static properties, so that users do not need to define separate structures:
static const struct software_node gpio_bank_b_node = {
	.name = "B",
};
static const struct property_entry simone_key_enter_props[] = {
	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
	PROPERTY_ENTRY_STRING("label", "enter"),
	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
	{ }
};
Signed-off-by Dmitry Torokhov <[email protected]>
---
Quoted 8 lines
Quoted 2 lines
I am writing a piece that needs to provide a list of gpios to a diriver. The above example looks like what I need.
Nope.
It mustn't be used for GPIOs or PWMs or whatever that either should come via lookup tables or corresponding firmware interface.
May I ask why? I've read commit descriptions for drivers/base/swnode.c and the discussion on lkml and I understand software nodes as a way to provide (synthesize) a description for a device that is missing a description in the firmware. Another use case seems to be to replace (in the long run) platform data. That is what I am trying to use it for.
I want my device to be configured with either DT or software_nodes created at run time with configfs. My device is going to use GPIOs described in the DT and it is going to be configured via configfs at run time. I could use platform_data to pass structures from configfs but software nodes would let me save some code in the device driver and use the same paths for both static (DT) and dynamic (configfs) configuration.
Quoted 4 lines
Probably I have missed something and I will be greatful, if you tell me where I can find more information about software nodes. There are few users in the kernel and it isn't obvious for me how to use software nodes properly.

Quoted 4 lines

Yeah, I disagree with Andy here. The lookup tables are a crutch that we have until GPIO and PWM a taught to support software nodes (I need to resurrect my patch series for GPIO, if you have time to test that would be awesome).

It will be great to help. I am not sure if I have means to test PWMs, but I can do GPIOs.

Signature · 3 lines

Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Andy Shevchenko

reviewer

2020-11-09 19:01 UTC #22 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

In reply to Lukasz Stelmach

Quoted 3 lines · Lukasz Stelmach

Quoted 2 lines · when Andy Shevchenko
Quoted 1 line · Lukasz Stelmach
It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:

...

Quoted 5 lines

Quoted 5 lines
Quoted 2 lines
I am writing a piece that needs to provide a list of gpios to a diriver. The above example looks like what I need.
Nope.

Quoted 8 lines

Quoted 2 lines
It mustn't be used for GPIOs or PWMs or whatever that either should come via lookup tables or corresponding firmware interface.
May I ask why? I've read commit descriptions for drivers/base/swnode.c and the discussion on lkml and I understand software nodes as a way to provide (synthesize) a description for a device that is missing a description in the firmware. Another use case seems to be to replace (in the long run) platform data. That is what I am trying to use it for.

Yes. Both are correct. They are simply not applicable for everything (it's not a silver bullet).

Quoted 2 lines

I want my device to be configured with either DT or software_nodes created at run time with configfs.

Okay.

Quoted 3 lines

My device is going to use GPIOs
described in the DT and it is going to be configured via configfs at run time.

How is this related to swnodes?
Create GPIO lookup table.

Quoted 9 lines

I could use platform_data to pass structures from configfs but software nodes would let me save some code in the device driver and use the same paths for both static (DT) and dynamic (configfs) configuration.
Probably I have missed something and I will be greatful, if you tell me where I can find more information about software nodes. There are few users in the kernel and it isn't obvious for me how to use software nodes properly.

gpiod_add_lookup_table().

Quoted 17 lines

Quoted 17 lines
Quoted 17 lines
At the moment the driver gets the list from fwnode/of_node. The list contain references to phandles which get resolved and and the driver ends up with a bunch of gpio descriptors. Great.
This example looks nice but does the code that reads the reference from the gpios property and returns a gpiod actually exist? If it doesn't, I am willing to write it.
At first glance it makes more sense to me to pass (struct gpiod_lookup *) instead of (struct software_node *) and make gpiolib's gpiod_find() accept lookup tables as parameter instead of searching the gpio_lookup_list? Or do you think such temporary table should be assembled from the above structure and then used in gpiod_find()?
Any other suggestions on how to get a bunch of gpios (the description for gpios is available in the devicetree) for a device described with a software nodes?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Lukasz Stelmach

2020-11-09 19:47 UTC #23 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

Quoted 27 lines · when Andy Shevchenko

Quoted 3 lines · Lukasz Stelmach
Quoted 2 lines · when Andy Shevchenko
Quoted 1 line · Lukasz Stelmach
It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:
...
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
I am writing a piece that needs to provide a list of gpios to a diriver. The above example looks like what I need.
Nope.
Quoted 8 lines
Quoted 2 lines
It mustn't be used for GPIOs or PWMs or whatever that either should come via lookup tables or corresponding firmware interface.
May I ask why? I've read commit descriptions for drivers/base/swnode.c and the discussion on lkml and I understand software nodes as a way to provide (synthesize) a description for a device that is missing a description in the firmware. Another use case seems to be to replace (in the long run) platform data. That is what I am trying to use it for.
Yes. Both are correct. They are simply not applicable for everything (it's not a silver bullet).
Quoted 2 lines
I want my device to be configured with either DT or software_nodes created at run time with configfs.

Quoted 7 lines

Okay.
Quoted 3 lines
My device is going to use GPIOs
described in the DT and it is going to be configured via configfs at run time.
How is this related to swnodes?

I thought I should mention it, because as far as I can tel mixing and merging information from different fwnode backends seems to be tricky if supported at all.

Quoted 7 lines

Create GPIO lookup table.
Quoted 5 lines
I could use platform_data to pass structures from configfs but software nodes would let me save some code in the device driver and use the same paths for both static (DT) and dynamic (configfs) configuration.

Quoted 7 lines

Quoted 4 lines
Probably I have missed something and I will be greatful, if you tell me where I can find more information about software nodes. There are few users in the kernel and it isn't obvious for me how to use software nodes properly.
gpiod_add_lookup_table().

Yes, that is exactly what my POC code does now. But having a lookup table together with the rest of the device structures has several advantages.

1) The device may be hotpluggable and there is no
gpiod_remove_lookup_table().

2) Having the lookup table allocated and managed together with the rest of the device seems like a better way to go than on gpio_lookup_list.

3) As of now I've got a minor issue with device naming. I need to set dev_id of the table before the device is ready and only after it is ready, its name is set (in the hotpluggable use case).

4) Because no other devices would use this lookup table "publishing" it rather than keeping together with the device seems at least slightly odd.

When the lookup table is attached to the devices and can be passed around the final lookup can be done with a function like

static struct gpio_desc *gpiod_find_from_table(struct device *dev,
const char *con_id, unsigned int idx,
unsigned long *flags, struct gpiod_lookup *table)

Quoted 17 lines

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
At the moment the driver gets the list from fwnode/of_node. The list contain references to phandles which get resolved and and the driver ends up with a bunch of gpio descriptors. Great.
This example looks nice but does the code that reads the reference from the gpios property and returns a gpiod actually exist? If it doesn't, I am willing to write it.
At first glance it makes more sense to me to pass (struct gpiod_lookup *) instead of (struct software_node *) and make gpiolib's gpiod_find() accept lookup tables as parameter instead of searching the gpio_lookup_list? Or do you think such temporary table should be assembled from the above structure and then used in gpiod_find()?
Any other suggestions on how to get a bunch of gpios (the description for gpios is available in the devicetree) for a device described with a software nodes?

Signature · 3 lines

Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Andy Shevchenko

reviewer

2020-11-09 21:20 UTC #24 lore raw

Re: [PATCH v8 3/6] software node: implement reference properties

Quoted 5 lines · Lukasz Stelmach

Quoted 4 lines · when Andy Shevchenko
Quoted 3 lines · Lukasz Stelmach
Quoted 2 lines · when Andy Shevchenko
Quoted 1 line · Lukasz Stelmach
It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:

...

Quoted 15 lines

Quoted 7 lines
Create GPIO lookup table.
Quoted 5 lines
I could use platform_data to pass structures from configfs but software nodes would let me save some code in the device driver and use the same paths for both static (DT) and dynamic (configfs) configuration.
Quoted 7 lines
Quoted 4 lines
Probably I have missed something and I will be greatful, if you tell me where I can find more information about software nodes. There are few users in the kernel and it isn't obvious for me how to use software nodes properly.
gpiod_add_lookup_table().

Quoted 6 lines

Yes, that is exactly what my POC code does now. But having a lookup table together with the rest of the device structures has several advantages.
1) The device may be hotpluggable and there is no
gpiod_remove_lookup_table().

% git grep -n -w gpiod_remove_lookup_table

Or I did get it wrong? Did you mean that the removal is not being called?

Quoted 2 lines

2) Having the lookup table allocated and managed together with the rest of the device seems like a better way to go than on gpio_lookup_list.

Nice, what are you going to do with the rest of lookup tables (PWM, regulators, etc)? If you convert, convert them all at least.

Quoted 3 lines

3) As of now I've got a minor issue with device naming. I need to set dev_id of the table before the device is ready and only after it is ready, its name is set (in the hotpluggable use case).

Hotpluggable devices are very much supported by ACPI assistance. DT I have heard has overlays. What's the issue?

Quoted 10 lines

4) Because no other devices would use this lookup table "publishing" it rather than keeping together with the device seems at least slightly odd.
When the lookup table is attached to the devices and can be passed around the final lookup can be done with a function like
static struct gpio_desc *gpiod_find_from_table(struct device *dev,
const char *con_id, unsigned int idx,
unsigned long *flags, struct gpiod_lookup *table)

Something sounds fishy about your case. Why do you need to have board code / platform data in the first place? Sorry, but I didn't get why you should reconstruct DT (or ACPI) at run-time without using proper framework / feature (overlays)?

Quoted 17 lines

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
At the moment the driver gets the list from fwnode/of_node. The list contain references to phandles which get resolved and and the driver ends up with a bunch of gpio descriptors. Great.
This example looks nice but does the code that reads the reference from the gpios property and returns a gpiod actually exist? If it doesn't, I am willing to write it.
At first glance it makes more sense to me to pass (struct gpiod_lookup *) instead of (struct software_node *) and make gpiolib's gpiod_find() accept lookup tables as parameter instead of searching the gpio_lookup_list? Or do you think such temporary table should be assembled from the above structure and then used in gpiod_find()?
Any other suggestions on how to get a bunch of gpios (the description for gpios is available in the devicetree) for a device described with a software nodes?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dmitry Torokhov

author

2019-11-08 04:22 UTC #25 lore raw

PATCH v8 4/6 platform/x86: intel_cht_int33fe: use inline reference properties

In reply to Dmitry Torokhov

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

Acked-by Andy Shevchenko <[email protected]>

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

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

drivers/platform/x86/intel_cht_int33fe_typec.c +41 ~~-40~~

index 2d097fc2dd465..04138215956bf 100644
@@ -36,30 +36,6 @@ enum {
 	INT33FE_NODE_MAX,
 };
 
-static const struct software_node nodes[];
-
-static const struct software_node_ref_args pi3usb30532_ref = {
-	&nodes[INT33FE_NODE_PI3USB30532]
-};
-
-static const struct software_node_ref_args dp_ref = {
-	&nodes[INT33FE_NODE_DISPLAYPORT]
-};
-
-static struct software_node_ref_args mux_ref;
-
-static const struct software_node_reference usb_connector_refs[] = {
-	{ "orientation-switch", 1, &pi3usb30532_ref},
-	{ "mode-switch", 1, &pi3usb30532_ref},
-	{ "displayport", 1, &dp_ref},
-	{ }
-};
-
-static const struct software_node_reference fusb302_refs[] = {
-	{ "usb-role-switch", 1, &mux_ref},
-	{ }
-};
-
 /*
  * Grrr I severly dislike buggy BIOS-es. At least one BIOS enumerates
  * the max17047 both through the INT33FE ACPI device (it is right there

@@ -95,8 +71,18 @@ static const struct property_entry max17047_props[] = {
 	{ }
 };
 
+/*
+ * We are not using inline property here because those are constant,
+ * and we need to adjust this one at runtime to point to real
+ * software node.
+ */
+static struct software_node_ref_args fusb302_mux_refs[] = {
+	{ .node = NULL },
+};
+
 static const struct property_entry fusb302_props[] = {
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
+	PROPERTY_ENTRY_REF_ARRAY("usb-role-switch", fusb302_mux_refs),
 	{ }
 };

@@ -112,6 +98,8 @@ static const u32 snk_pdo[] = {
 	PDO_VAR(5000, 12000, 3000),
 };
 
+static const struct software_node nodes[];
+
 static const struct property_entry usb_connector_props[] = {
 	PROPERTY_ENTRY_STRING("data-role", "dual"),
 	PROPERTY_ENTRY_STRING("power-role", "dual"),

@@ -119,15 +107,21 @@ static const struct property_entry usb_connector_props[] = {
 	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
 	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
 	PROPERTY_ENTRY_U32("op-sink-microwatt", 2500000),
+	PROPERTY_ENTRY_REF("orientation-switch",
+			   &nodes[INT33FE_NODE_PI3USB30532]),
+	PROPERTY_ENTRY_REF("mode-switch",
+			   &nodes[INT33FE_NODE_PI3USB30532]),
+	PROPERTY_ENTRY_REF("displayport",
+			   &nodes[INT33FE_NODE_DISPLAYPORT]),
 	{ }
 };
 
 static const struct software_node nodes[] = {
-	{ "fusb302", NULL, fusb302_props, fusb302_refs },
+	{ "fusb302", NULL, fusb302_props },
 	{ "max17047", NULL, max17047_props },
 	{ "pi3usb30532" },
 	{ "displayport" },
-	{ "connector", &nodes[0], usb_connector_props, usb_connector_refs },
+	{ "connector", &nodes[0], usb_connector_props },
 	{ }
 };

@@ -163,9 +157,10 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 {
 	software_node_unregister_nodes(nodes);
 
-	if (mux_ref.node) {
-		fwnode_handle_put(software_node_fwnode(mux_ref.node));
-		mux_ref.node = NULL;
+	if (fusb302_mux_refs[0].node) {
+		fwnode_handle_put(
+			software_node_fwnode(fusb302_mux_refs[0].node));
+		fusb302_mux_refs[0].node = NULL;
 	}
 
 	if (data->dp) {

@@ -177,25 +172,31 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 
 static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
 {
+	const struct software_node *mux_ref_node;
 	int ret;
 
-	ret = software_node_register_nodes(nodes);
-	if (ret)
-		return ret;
-
-	/* The devices that are not created in this driver need extra steps. */
-
 	/*
 	 * There is no ACPI device node for the USB role mux, so we need to wait
 	 * until the mux driver has created software node for the mux device.
 	 * It means we depend on the mux driver. This function will return
 	 * -EPROBE_DEFER until the mux device is registered.
 	 */
-	mux_ref.node = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
-	if (!mux_ref.node) {
-		ret = -EPROBE_DEFER;
-		goto err_remove_nodes;
-	}
+	mux_ref_node = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
+	if (!mux_ref_node)
+		return -EPROBE_DEFER;
+
+	/*
+	 * Update node used in "usb-role-switch" property. Note that we
+	 * rely on software_node_register_nodes() to use the original
+	 * instance of properties instead of copying them.
+	 */
+	fusb302_mux_refs[0].node = mux_ref_node;
+
+	ret = software_node_register_nodes(nodes);
+	if (ret)
+		return ret;
+
+	/* The devices that are not created in this driver need extra steps. */
 
 	/*
 	 * The DP connector does have ACPI device node. In this case we can just

Signature · 1 line

2.24.0.rc1.363.gb1bccd3e3d-goog

Dmitry Torokhov

author

2019-11-08 04:22 UTC #26 lore raw

PATCH v8 5/6 software node: remove separate handling of references

In reply to Dmitry Torokhov

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

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

---
 drivers/base/swnode.c    | 48 +++++++++++++++-------------------------
 include/linux/property.h | 14 ------------
 2 files changed, 18 insertions(+), 44 deletions(-)

drivers/base/swnode.c +18 ~~-30~~

index 604d7327bba79..0b081dee1e95c 100644
@@ -479,9 +479,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;
 	u32 nargs_prop_val;

@@ -492,37 +491,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;
-
-		ref_array = prop->pointer;
-		ref_args = &ref_array[index];
-	} else {
-		if (!swnode->node->references)
-			return -ENOENT;
+	if (!prop)
+		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;

@@ -543,7 +531,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;
 }

include/linux/property.h +0 ~~-14~~

index 19b9dcc322763..b28c81af7bb68 100644
@@ -418,30 +418,16 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
-/**
- * struct software_node_reference - Named software node reference property
- * @name: Name of the property
- * @nrefs: Number of elements in @refs array
- * @refs: Array of references with optional arguments
- */
-struct software_node_reference {
-	const char *name;
-	unsigned int nrefs;
-	const struct software_node_ref_args *refs;
-};
-
 /**
  * struct software_node - Software node description
  * @name: Name of the software node
  * @parent: Parent of the software node
  * @properties: Array of device properties
- * @references: Array of software node reference properties
  */
 struct software_node {
 	const char *name;
 	const struct software_node *parent;
 	const struct property_entry *properties;
-	const struct software_node_reference *references;
 };
 
 bool is_software_node(const struct fwnode_handle *fwnode);

Signature · 1 line

2.24.0.rc1.363.gb1bccd3e3d-goog

Dmitry Torokhov

author

2019-11-08 04:22 UTC #27 lore raw

PATCH v8 6/6 software node: add basic tests for property entries

In reply to Dmitry Torokhov

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

We are using KUnit framework for the tests.

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

---
 drivers/base/test/Makefile              |   2 +
 drivers/base/test/property-entry-test.c | 472 ++++++++++++++++++++++++
 2 files changed, 474 insertions(+)
 create mode 100644 drivers/base/test/property-entry-test.c

drivers/base/test/Makefile +2 -0

index 0f1f7277a0139..22143102e5d21 100644
@@ -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

drivers/base/test/property-entry-test.c new file +472 -0

472 lines not on this page · raw message

Signature · 1 line

2.24.0.rc1.363.gb1bccd3e3d-goog