software node: add support for reference properties
v7 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.
Changed 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 | 142 +++--- 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 | 84 ++-- 5 files changed, 640 insertions(+), 141 deletions(-) create mode 100644 drivers/base/test/property-entry-test.c
Signature · 1 line
2.24.0.rc1.363.gb1bccd3e3d-goog
PATCH v7 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.
--- drivers/base/swnode.c | 12 +++++------- include/linux/property.h | 13 ++++++++----- 2 files changed, 13 insertions(+), 12 deletions(-)
drivers/base/swnode.c +5 -7
@@ -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
@@ -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
PATCH v7 2/6 software node: allow embedding of small arrays into property_entry
In reply to Dmitry TorokhovWe 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.
--- drivers/base/swnode.c | 103 +++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 56 deletions(-)
drivers/base/swnode.c +47 -56
@@ -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 ; }/**
Signature · 1 line
2.24.0.rc1.363.gb1bccd3e3d-goog
PATCH v7 3/6 software node: implement reference properties
In reply to Dmitry TorokhovIt 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),
{ }
};--- drivers/base/swnode.c | 49 ++++++++++++++++++++++++++++------ include/linux/property.h | 57 +++++++++++++++++++++++++++++----------- 2 files changed, 82 insertions(+), 24 deletions(-)
drivers/base/swnode.c +41 -8
@@ -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,21 +480,47 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode, {struct swnode *swnode =to_swnode (fwnode);const struct software_node_reference *ref; +const struct software_node_ref_args *ref_array; +const struct software_node_ref_args *ref_args;const struct property_entry *prop;struct fwnode_handle *refnode;int i; -if (!swnode || !swnode->node->references) +if (!swnode)return -ENOENT ; -for (ref = swnode->node->references; ref->name; ref++) -if (!strcmp (ref->name, propname)) -break ; + prop =property_entry_get (swnode->node->properties, propname); +if (prop) { +if (prop->type !=DEV_PROP_REF ) +return -EINVAL ; -if (!ref->name || index > (ref->nrefs -1 )) -return -ENOENT ; +/* +* We expect that references are never stored inline, even +* single ones, as they are too big. +*/ +if (prop->is_inline) +return -EINVAL ; + +if (index *sizeof (*ref_args) >= prop->length) +return -ENOENT ; + + ref_array = prop->pointer; + ref_args = &ref_array[index]; + }else { +if (!swnode->node->references) +return -ENOENT ; + +for (ref = swnode->node->references; ref->name; ref++) +if (!strcmp (ref->name, propname)) +break ; + +if (!ref->name || index > (ref->nrefs -1 )) +return -ENOENT ; + + ref_args = &ref->refs[index]; + } - refnode =software_node_fwnode (ref->refs[index].node); + refnode =software_node_fwnode (ref_args->node);if (!refnode)return -ENOENT ;
@@ -506,7 +539,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
PATCH v7 4/6 platform/x86: intel_cht_int33fe: use inline reference properties
In reply to Dmitry TorokhovNow 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.
--- .../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
@@ -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
PATCH v7 5/6 software node: remove separate handling of references
In reply to Dmitry TorokhovNow that all users of references have moved to reference properties, we can remove separate handling of references.
--- drivers/base/swnode.c | 46 +++++++++++++++------------------------- include/linux/property.h | 14 ------------ 2 files changed, 17 insertions(+), 43 deletions(-)
drivers/base/swnode.c +17 -29
@@ -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;int i;
@@ -490,37 +489,26 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,return -ENOENT ; prop =property_entry_get (swnode->node->properties, propname); -if (prop) { -if (prop->type !=DEV_PROP_REF ) -return -EINVAL ; - -/* -* We expect that references are never stored inline, even -* single ones, as they are too big. -*/ -if (prop->is_inline) -return -EINVAL ; - -if (index *sizeof (*ref_args) >= prop->length) -return -ENOENT ; +if (!prop) +return -ENOENT ; - ref_array = prop->pointer; - ref_args = &ref_array[index]; - }else { -if (!swnode->node->references) -return -ENOENT ; +if (prop->type !=DEV_PROP_REF ) +return -EINVAL ; -for (ref = swnode->node->references; ref->name; ref++) -if (!strcmp (ref->name, propname)) -break ; +/* +* We expect that references are never stored inline, even +* single ones, as they are too big. +*/ +if (prop->is_inline) +return -EINVAL ; -if (!ref->name || index > (ref->nrefs -1 )) -return -ENOENT ; +if (index *sizeof (*ref) >= prop->length) +return -ENOENT ; - ref_args = &ref->refs[index]; - } + ref_array = prop->pointer; + ref = &ref_array[index]; - refnode =software_node_fwnode (ref_args->node); + refnode =software_node_fwnode (ref->node);if (!refnode)return -ENOENT ;
@@ -539,7 +527,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
@@ -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
PATCH v7 6/6 software node: add basic tests for property entries
In reply to Dmitry TorokhovThis 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.
--- 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
@@ -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