| Message ID | 20190906222611.223532-1-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [v2,1/3] software node: implement reference properties | expand |
On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote: > 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), > { } > }; > Thanks for an update, my comments below. > + } else if (src->type == DEV_PROP_REF) { > + /* All reference properties must be arrays */ > + return -EINVAL; Hmm... What about to duplicate pointer under value union and use is_array to distinguish which one to use? Because... > @@ -240,6 +254,7 @@ struct property_entry { > const u32 *u32_data; > const u64 *u64_data; > const char * const *str; > + const struct software_node_ref_args *ref; > } pointer; > +#define PROPERTY_ENTRY_REF(_name_, _ref_, ...) \ > +(struct property_entry) { \ > + .name = _name_, \ > + .length = sizeof(struct software_node_ref_args), \ Is it correct? > + .type = DEV_PROP_REF, \ > + .is_array = true, \ I really don't like this "cheating". > + .type = DEV_PROP_REF, \ > + .pointer.ref = &(const struct software_node_ref_args) { \ > + .node = _ref_, \ > + .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \ Seems like you can drop pair of parentheses. > + .args = { __VA_ARGS__ }, \ > + }, \ > +}
On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote: > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote: > > 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), > > { } > > }; > > > > Thanks for an update, my comments below. > > > + } else if (src->type == DEV_PROP_REF) { > > + /* All reference properties must be arrays */ > > + return -EINVAL; > > Hmm... What about to duplicate pointer under value union and use is_array to > distinguish which one to use? Because... Then we have to special-case copying this entry, similar to the pains we are going with the strings. > > > > @@ -240,6 +254,7 @@ struct property_entry { > > const u32 *u32_data; > > const u64 *u64_data; > > const char * const *str; > > + const struct software_node_ref_args *ref; > > } pointer; > > > +#define PROPERTY_ENTRY_REF(_name_, _ref_, ...) \ > > +(struct property_entry) { \ > > + .name = _name_, \ > > > + .length = sizeof(struct software_node_ref_args), \ > > Is it correct? Yes, we length is element size * number of elements. > > > + .type = DEV_PROP_REF, \ > > > + .is_array = true, \ > > I really don't like this "cheating". This is not cheating. Any single value can be represented as an array of one element. Actually, the only reason we have this "is_array" business is because for scalar values and short strings it is much cheaper to store single value in-line instead of out of line + pointer, especially on 64 bit arches. If you want we can change is_array into is_inline. Thanks.
On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote: > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote: > > > + } else if (src->type == DEV_PROP_REF) { > > > + /* All reference properties must be arrays */ > > > + return -EINVAL; > > > > Hmm... What about to duplicate pointer under value union and use is_array to > > distinguish which one to use? Because... > > Then we have to special-case copying this entry, similar to the pains we > are going with the strings. I can't see it as a pain. Simple do the same kmemdup() for the case when is_array = false and DEV_TYPE_REF? By the way, don't we need to update property_entry_{get,set}_pointer()? > > > + .is_array = true, \ > > > > I really don't like this "cheating". > > This is not cheating. Any single value can be represented as an array of > one element. Actually, the only reason we have this "is_array" business > is because for scalar values and short strings it is much cheaper to > store single value in-line instead of out of line + pointer, especially > on 64 bit arches. Yes, and this is a lot of benefit! > If you want we can change is_array into is_inline. Nope, is_array is exactly what it tells us about the content. Its functional load is to distinguish which union (value vs. pointer) we are using.
On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote: > On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote: > > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote: > > > > > + } else if (src->type == DEV_PROP_REF) { > > > > + /* All reference properties must be arrays */ > > > > + return -EINVAL; > > > > > > Hmm... What about to duplicate pointer under value union and use is_array to > > > distinguish which one to use? Because... > > > > Then we have to special-case copying this entry, similar to the pains we > > are going with the strings. > > I can't see it as a pain. Simple do the same kmemdup() for the case when > is_array = false and DEV_TYPE_REF? And then you need to make sure it is freed on error paths and when we remove property entries. This requires more checks and code. In contrast we already know how to handle out of line objects of arbitrary size. The only reason we have inline strings is because for shorter strings we save 4/8 bytes. > > By the way, don't we need to update property_entry_{get,set}_pointer()? I do not see these, where are they? > > > > > + .is_array = true, \ > > > > > > I really don't like this "cheating". > > > > This is not cheating. Any single value can be represented as an array of > > one element. Actually, the only reason we have this "is_array" business > > is because for scalar values and short strings it is much cheaper to > > store single value in-line instead of out of line + pointer, especially > > on 64 bit arches. > > Yes, and this is a lot of benefit! Yes, nobody argues against it. Here however we are dealing with a larger structure. There is absolutely no benefit of trying to separate single value vs array here. > > > If you want we can change is_array into is_inline. > > Nope, is_array is exactly what it tells us about the content. Its functional > load is to distinguish which union (value vs. pointer) we are using. No, it signifies whether the value is stored within property entry or outside. I can fit probably 8 bytes arrays into property entry structure, in which case is_array will definitely not reflect the data type. It is the type-specific accessors that know how to parse and fetch data from properties. Thanks.
On Sat, Sep 07, 2019 at 10:37:24AM -0700, Dmitry Torokhov wrote: > On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote: > > On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote: > > > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote: > > > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote: > > > > > > > + } else if (src->type == DEV_PROP_REF) { > > > > > + /* All reference properties must be arrays */ > > > > > + return -EINVAL; > > > > > > > > Hmm... What about to duplicate pointer under value union and use is_array to > > > > distinguish which one to use? Because... > > > > > > Then we have to special-case copying this entry, similar to the pains we > > > are going with the strings. > > > > I can't see it as a pain. Simple do the same kmemdup() for the case when > > is_array = false and DEV_TYPE_REF? > > And then you need to make sure it is freed on error paths and when we > remove property entries. This requires more checks and code. In contrast > we already know how to handle out of line objects of arbitrary size. We can put it one level up to be a sibling to value / pointer unions. In that case is_array can be anything (we just don't care). Actually strings aren't inlined. > > By the way, don't we need to update property_entry_{get,set}_pointer()? > > I do not see these, where are they? swnode.c. I meant property_{get,set}_pointer(). > > > > > + .is_array = true, \ > > > > > > > > I really don't like this "cheating". > > > > > > This is not cheating. Any single value can be represented as an array of > > > one element. Actually, the only reason we have this "is_array" business > > > is because for scalar values and short strings it is much cheaper to > > > store single value in-line instead of out of line + pointer, especially > > > on 64 bit arches. > > > > Yes, and this is a lot of benefit! > > Yes, nobody argues against it. Here however we are dealing with a larger > structure. There is absolutely no benefit of trying to separate single > value vs array here. Thus, moving to upper layer makes more sense. Right? > > > If you want we can change is_array into is_inline. > > > > Nope, is_array is exactly what it tells us about the content. Its functional > > load is to distinguish which union (value vs. pointer) we are using. > > No, it signifies whether the value is stored within property entry or > outside. I can fit probably 8 bytes arrays into property entry > structure, in which case is_array will definitely not reflect the data > type. Nope, since strings are not inlined AFAICS. > It is the type-specific accessors that know how to parse and fetch data > from properties.
On Sat, Sep 07, 2019 at 09:03:48PM +0300, Andy Shevchenko wrote: > On Sat, Sep 07, 2019 at 10:37:24AM -0700, Dmitry Torokhov wrote: > > On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote: > > > On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote: > > > > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote: > > > > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote: > > > > > > > > > + } else if (src->type == DEV_PROP_REF) { > > > > > > + /* All reference properties must be arrays */ > > > > > > + return -EINVAL; > > > > > > > > > > Hmm... What about to duplicate pointer under value union and use is_array to > > > > > distinguish which one to use? Because... > > > > > > > > Then we have to special-case copying this entry, similar to the pains we > > > > are going with the strings. > > > > > > I can't see it as a pain. Simple do the same kmemdup() for the case when > > > is_array = false and DEV_TYPE_REF? > > > > And then you need to make sure it is freed on error paths and when we > > remove property entries. This requires more checks and code. In contrast > > we already know how to handle out of line objects of arbitrary size. > > We can put it one level up to be a sibling to value / pointer unions. > In that case is_array can be anything (we just don't care). I think it would be better if you sketched out your proposed data structure(s) so we are talking about the same things. But please note that when you are dealing with property arrays we need to keep the easy way of defining them, which means we should not be splitting individual entries. > > Actually strings aren't inlined. Right. Maybe I should clean it up as well. > > > > By the way, don't we need to update property_entry_{get,set}_pointer()? > > > > I do not see these, where are they? > > swnode.c. I meant property_{get,set}_pointer(). Yes, I think you are right about property_set_pointer() at least. > > > > > > > + .is_array = true, \ > > > > > > > > > > I really don't like this "cheating". > > > > > > > > This is not cheating. Any single value can be represented as an array of > > > > one element. Actually, the only reason we have this "is_array" business > > > > is because for scalar values and short strings it is much cheaper to > > > > store single value in-line instead of out of line + pointer, especially > > > > on 64 bit arches. > > > > > > Yes, and this is a lot of benefit! > > > > Yes, nobody argues against it. Here however we are dealing with a larger > > structure. There is absolutely no benefit of trying to separate single > > value vs array here. > > Thus, moving to upper layer makes more sense. Right? > > > > > If you want we can change is_array into is_inline. > > > > > > Nope, is_array is exactly what it tells us about the content. Its functional > > > load is to distinguish which union (value vs. pointer) we are using. > > > > No, it signifies whether the value is stored within property entry or > > outside. I can fit probably 8 bytes arrays into property entry > > structure, in which case is_array will definitely not reflect the data > > type. > > Nope, since strings are not inlined AFAICS. But u64 values are. Thanks.
On Sat, Sep 07, 2019 at 11:23:35AM -0700, Dmitry Torokhov wrote: > On Sat, Sep 07, 2019 at 09:03:48PM +0300, Andy Shevchenko wrote: > > On Sat, Sep 07, 2019 at 10:37:24AM -0700, Dmitry Torokhov wrote: > > > On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote: > > > > On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote: > > > > > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote: > > > > > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote: > > > > > > > > > > > + } else if (src->type == DEV_PROP_REF) { > > > > > > > + /* All reference properties must be arrays */ > > > > > > > + return -EINVAL; > > > > > > > > > > > > Hmm... What about to duplicate pointer under value union and use is_array to > > > > > > distinguish which one to use? Because... > > > > > > > > > > Then we have to special-case copying this entry, similar to the pains we > > > > > are going with the strings. > > > > > > > > I can't see it as a pain. Simple do the same kmemdup() for the case when > > > > is_array = false and DEV_TYPE_REF? > > > > > > And then you need to make sure it is freed on error paths and when we > > > remove property entries. This requires more checks and code. In contrast > > > we already know how to handle out of line objects of arbitrary size. > > > > We can put it one level up to be a sibling to value / pointer unions. > > In that case is_array can be anything (we just don't care). > > I think it would be better if you sketched out your proposed data > structure(s) so we are talking about the same things. But please note > that when you are dealing with property arrays we need to keep the easy > way of defining them, which means we should not be splitting individual > entries. This one: union { union { const u8 *u8_data; const u16 *u16_data; const u32 *u32_data; const u64 *u64_data; const char * const *str; } pointer; union { u8 u8_data; u16 u16_data; u32 u32_data; u64 u64_data; const char *str; } value; struct ... *ref; };
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index ee2a405cca9a..bd720d995123 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -387,6 +387,9 @@ static int property_entry_copy_data(struct property_entry *dst, new = kstrdup(src->value.str, GFP_KERNEL); if (!new && src->value.str) return -ENOMEM; + } else if (src->type == DEV_PROP_REF) { + /* All reference properties must be arrays */ + return -EINVAL; } else { new = pointer; } @@ -568,21 +571,45 @@ 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_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 all references to be stored as arrays via + * a pointer, even single ones. + */ + if (!prop->is_array) + return -EINVAL; + + if (index * sizeof(*ref_args) >= prop->length) + return -ENOENT; + + ref_args = &prop->pointer.ref[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; @@ -601,7 +628,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode, args->nargs = nargs; for (i = 0; i < nargs; i++) - args->args[i] = ref->refs[index].args[i]; + args->args[i] = ref_args->args[i]; return 0; } diff --git a/include/linux/property.h b/include/linux/property.h index 9b3d4ca3a73a..6658403f6fa9 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -22,7 +22,7 @@ enum dev_prop_type { DEV_PROP_U32, DEV_PROP_U64, DEV_PROP_STRING, - DEV_PROP_MAX, + DEV_PROP_REF, }; enum dev_dma_attr { @@ -219,6 +219,20 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode, return fwnode_property_read_u64_array(fwnode, propname, NULL, 0); } +struct software_node; + +/** + * struct software_node_ref_args - Reference property with additional arguments + * @node: Reference to a software node + * @nargs: Number of elements in @args array + * @args: Integer arguments + */ +struct software_node_ref_args { + const struct software_node *node; + unsigned int nargs; + u64 args[NR_FWNODE_REFERENCE_ARGS]; +}; + /** * struct property_entry - "Built-in" device property representation. * @name: Name of the property. @@ -240,6 +254,7 @@ struct property_entry { const u32 *u32_data; const u64 *u64_data; const char * const *str; + const struct software_node_ref_args *ref; } pointer; union { u8 u8_data; @@ -284,6 +299,15 @@ struct property_entry { { .pointer = { .str = _val_ } }, \ } +#define PROPERTY_ENTRY_REF_ARRAY(_name_, _val_) \ +(struct property_entry) { \ + .name = _name_, \ + .length = ARRAY_SIZE(_val_) * sizeof(struct software_node_ref_args), \ + .is_array = true, \ + .type = DEV_PROP_REF, \ + .pointer.ref = _val_, \ +} + #define PROPERTY_ENTRY_INTEGER(_name_, _type_, _Type_, _val_) \ (struct property_entry) { \ .name = _name_, \ @@ -314,6 +338,20 @@ struct property_entry { .name = _name_, \ } +#define PROPERTY_ENTRY_REF(_name_, _ref_, ...) \ +(struct property_entry) { \ + .name = _name_, \ + .length = sizeof(struct software_node_ref_args), \ + .type = DEV_PROP_REF, \ + .is_array = true, \ + .type = DEV_PROP_REF, \ + .pointer.ref = &(const struct software_node_ref_args) { \ + .node = _ref_, \ + .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \ + .args = { __VA_ARGS__ }, \ + }, \ +} + struct property_entry * property_entries_dup(const struct property_entry *properties); @@ -377,20 +415,6 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode, /* -------------------------------------------------------------------------- */ /* Software fwnode support - when HW description is incomplete or missing */ -struct software_node; - -/** - * struct software_node_ref_args - Reference with additional arguments - * @node: Reference to a software node - * @nargs: Number of elements in @args array - * @args: Integer arguments - */ -struct software_node_ref_args { - const struct software_node *node; - unsigned int nargs; - u64 args[NR_FWNODE_REFERENCE_ARGS]; -}; - /** * struct software_node_reference - Named software node reference property * @name: Name of the property
It is possible to store references to software nodes in the same fashion as other static properties, so that users do not need to define separate structures: static const struct software_node gpio_bank_b_node = { .name = "B", }; static const struct property_entry simone_key_enter_props[] = { PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), PROPERTY_ENTRY_STRING("label", "enter"), PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), { } }; Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- v1->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 drivers/base/swnode.c | 43 ++++++++++++++++++++++++++------ include/linux/property.h | 54 +++++++++++++++++++++++++++++----------- 2 files changed, 74 insertions(+), 23 deletions(-)