| Message ID | 20190909081557.93766-4-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | software node: add support for reference properties | expand |
On Mon, Sep 09, 2019 at 01:15:47AM -0700, Dmitry Torokhov wrote: > Instead of explicitly setting values of integer types when copying property > entries lets just copy entire value union when processing non-array values. > > When handling array values assign the pointer there using the newly introduced > "raw" pointer union member. This allows us to remove property_set_pointer(). Is this reincarnation of 318a19718261? Have you read 63dcc7090137? From me NAK.
On Mon, Sep 09, 2019 at 12:55:05PM +0300, Andy Shevchenko wrote: > On Mon, Sep 09, 2019 at 01:15:47AM -0700, Dmitry Torokhov wrote: > > Instead of explicitly setting values of integer types when copying property > > entries lets just copy entire value union when processing non-array values. > > > > When handling array values assign the pointer there using the newly introduced > > "raw" pointer union member. This allows us to remove property_set_pointer(). > > Is this reincarnation of 318a19718261? > Have you read 63dcc7090137? Okay, I think if I squash this and the followup patch to property_get_data() then we'll only go through the "raw" pointer to get to the non-inline data and therefore we will not have the union aliasing issue. The in-line values never change their type when storing/accessing.
On Mon, Sep 09, 2019 at 03:15:55AM -0700, Dmitry Torokhov wrote: > On Mon, Sep 09, 2019 at 12:55:05PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 09, 2019 at 01:15:47AM -0700, Dmitry Torokhov wrote: > > > Instead of explicitly setting values of integer types when copying property > > > entries lets just copy entire value union when processing non-array values. > > > > > > When handling array values assign the pointer there using the newly introduced > > > "raw" pointer union member. This allows us to remove property_set_pointer(). > > > > Is this reincarnation of 318a19718261? > > Have you read 63dcc7090137? > > Okay, I think if I squash this and the followup patch to > property_get_data() then we'll only go through the "raw" pointer to get > to the non-inline data and therefore we will not have the union aliasing > issue. > > The in-line values never change their type when storing/accessing. It might work, though it prevents to do type checking at compile time. So, basically something like struct obscure_things { u8 *prop_array_val; bla bla bla }; struct property_entry entry; struct obscure_things things; ... entry.pointer.raw = &things; which shouldn't be possible. I dunno what others think about your proposal.
On Mon, Sep 09, 2019 at 02:36:52PM +0300, Andy Shevchenko wrote: > On Mon, Sep 09, 2019 at 03:15:55AM -0700, Dmitry Torokhov wrote: > > On Mon, Sep 09, 2019 at 12:55:05PM +0300, Andy Shevchenko wrote: > > > On Mon, Sep 09, 2019 at 01:15:47AM -0700, Dmitry Torokhov wrote: > > > > Instead of explicitly setting values of integer types when copying property > > > > entries lets just copy entire value union when processing non-array values. > > > > > > > > When handling array values assign the pointer there using the newly introduced > > > > "raw" pointer union member. This allows us to remove property_set_pointer(). > > > > > > Is this reincarnation of 318a19718261? > > > Have you read 63dcc7090137? > > > > Okay, I think if I squash this and the followup patch to > > property_get_data() then we'll only go through the "raw" pointer to get > > to the non-inline data and therefore we will not have the union aliasing > > issue. > > > > The in-line values never change their type when storing/accessing. > > It might work, though it prevents to do type checking at compile time. So, > basically something like > > struct obscure_things { > u8 *prop_array_val; > bla bla bla > }; > > struct property_entry entry; > struct obscure_things things; > ... > entry.pointer.raw = &things; > > which shouldn't be possible. I think type checking is a red herring as we still can't validate the type. I believe the answer here is to not allow external users to poke in property_entry and use PROPERTY_ENTRY_XXX macros to construct entires, as I have done for the Apple EFI driver. > > I dunno what others think about your proposal. Thanks.
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 7bad41a8f65d..a8d12046105e 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -103,45 +103,6 @@ property_entry_get(const struct property_entry *prop, const char *name) return NULL; } -static void -property_set_pointer(struct property_entry *prop, const void *pointer) -{ - switch (prop->type) { - case DEV_PROP_U8: - if (prop->is_array) - prop->pointer.u8_data = pointer; - else - prop->value.u8_data = *((u8 *)pointer); - break; - case DEV_PROP_U16: - if (prop->is_array) - prop->pointer.u16_data = pointer; - else - prop->value.u16_data = *((u16 *)pointer); - break; - case DEV_PROP_U32: - if (prop->is_array) - prop->pointer.u32_data = pointer; - else - prop->value.u32_data = *((u32 *)pointer); - break; - case DEV_PROP_U64: - if (prop->is_array) - prop->pointer.u64_data = pointer; - else - prop->value.u64_data = *((u64 *)pointer); - break; - case DEV_PROP_STRING: - if (prop->is_array) - prop->pointer.str = pointer; - else - prop->value.str = pointer; - break; - default: - break; - } -} - static const void *property_get_pointer(const struct property_entry *prop) { switch (prop->type) { @@ -380,20 +341,21 @@ static int property_entry_copy_data(struct property_entry *dst, if (!new) return -ENOMEM; } + + dst->is_array = true; + dst->pointer.raw = new; } else if (src->type == DEV_PROP_STRING) { new = kstrdup(src->value.str, GFP_KERNEL); if (!new && src->value.str) return -ENOMEM; + + dst->value.str = new; } else { - new = pointer; + dst->value = src->value; } dst->length = src->length; - dst->is_array = src->is_array; dst->type = src->type; - - property_set_pointer(dst, new); - dst->name = kstrdup(src->name, GFP_KERNEL); if (!dst->name) goto out_free_data; diff --git a/include/linux/property.h b/include/linux/property.h index 44c1704f7163..4943b40d3536 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -239,6 +239,7 @@ struct property_entry { const u32 *u32_data; const u64 *u64_data; const char * const *str; + const void *raw; } pointer; union { u8 u8_data;
Instead of explicitly setting values of integer types when copying property entries lets just copy entire value union when processing non-array values. When handling array values assign the pointer there using the newly introduced "raw" pointer union member. This allows us to remove property_set_pointer(). Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/base/swnode.c | 50 +++++----------------------------------- include/linux/property.h | 1 + 2 files changed, 7 insertions(+), 44 deletions(-)