diff mbox series

[v5,06/14] software node: get rid of property_set_pointer()

Message ID 20191011230721.206646-7-dmitry.torokhov@gmail.com (mailing list archive)
State Deferred, archived
Headers show
Series software node: add support for reference properties | expand

Commit Message

Dmitry Torokhov Oct. 11, 2019, 11:07 p.m. UTC
Instead of explicitly setting values of integer types when copying
property entries lets just copy entire value union when processing
non-array values.

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

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

We are not losing anything from removing typed pointer union because the
upper layers do their accesses through void pointers anyway, and we
trust the "type" of the property when interpret the data. We rely on
users of property entries on using PROPERTY_ENTRY_XXX() macros to
properly initialize entries instead of poking in the instances directly.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 90 +++++++++-------------------------------
 include/linux/property.h | 12 ++----
 2 files changed, 22 insertions(+), 80 deletions(-)

Comments

Andy Shevchenko Oct. 15, 2019, 12:09 p.m. UTC | #1
On Fri, Oct 11, 2019 at 04:07:13PM -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.
> 
> For value arrays we no longer use union of pointers, but rather a single
> void pointer, which allows us to remove property_set_pointer().
> 
> In property_get_pointer() we do not need to handle each data type
> separately, we can simply return either the pointer or pointer to values
> union.
> 
> We are not losing anything from removing typed pointer union because the
> upper layers do their accesses through void pointers anyway, and we
> trust the "type" of the property when interpret the data. We rely on
> users of property entries on using PROPERTY_ENTRY_XXX() macros to
> properly initialize entries instead of poking in the instances directly.

I'm not sure about this change since the struct definition is still available
to use. If we would change it to be opaque pointer, it will be possible to get
rid of the type differentiation in cleaner way.

Anyway, perhaps somebody else can look at this.
Dmitry Torokhov Oct. 15, 2019, 6:14 p.m. UTC | #2
On Tue, Oct 15, 2019 at 03:09:49PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:13PM -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.
> > 
> > For value arrays we no longer use union of pointers, but rather a single
> > void pointer, which allows us to remove property_set_pointer().
> > 
> > In property_get_pointer() we do not need to handle each data type
> > separately, we can simply return either the pointer or pointer to values
> > union.
> > 
> > We are not losing anything from removing typed pointer union because the
> > upper layers do their accesses through void pointers anyway, and we
> > trust the "type" of the property when interpret the data. We rely on
> > users of property entries on using PROPERTY_ENTRY_XXX() macros to
> > properly initialize entries instead of poking in the instances directly.
> 
> I'm not sure about this change since the struct definition is still available
> to use. If we would change it to be opaque pointer, it will be possible to get
> rid of the type differentiation in cleaner way.

We expose type and other fields that should not be manipulated directly,
for the benefit of being able to use static initializers. Having value
structure allows for not too ugly non-array initializers.

Thanks.
diff mbox series

Patch

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 2f2248c9003c..26e56dd66436 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -103,71 +103,15 @@  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) {
-	case DEV_PROP_U8:
-		if (prop->is_array)
-			return prop->pointer.u8_data;
-		return &prop->value.u8_data;
-	case DEV_PROP_U16:
-		if (prop->is_array)
-			return prop->pointer.u16_data;
-		return &prop->value.u16_data;
-	case DEV_PROP_U32:
-		if (prop->is_array)
-			return prop->pointer.u32_data;
-		return &prop->value.u32_data;
-	case DEV_PROP_U64:
-		if (prop->is_array)
-			return prop->pointer.u64_data;
-		return &prop->value.u64_data;
-	case DEV_PROP_STRING:
-		if (prop->is_array)
-			return prop->pointer.str;
-		return &prop->value.str;
-	default:
+	if (!prop->length)
 		return NULL;
-	}
+
+	if (prop->is_array)
+		return prop->pointer;
+
+	return &prop->value;
 }
 
 static const void *property_entry_find(const struct property_entry *props,
@@ -322,13 +266,15 @@  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_array) {
-		if (p->type == DEV_PROP_STRING && p->pointer.str) {
+		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(p->pointer.str[i]);
+				kfree(src_str[i]);
 		}
 		kfree(pointer);
 	} else if (p->type == DEV_PROP_STRING) {
@@ -341,6 +287,7 @@  static const char * const *
 property_copy_string_array(const struct property_entry *src)
 {
 	const char **d;
+	const char * const *src_str = src->pointer;
 	size_t nval = src->length / sizeof(*d);
 	int i;
 
@@ -349,8 +296,8 @@  property_copy_string_array(const struct property_entry *src)
 		return NULL;
 
 	for (i = 0; i < nval; i++) {
-		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
-		if (!d[i] && src->pointer.str[i]) {
+		d[i] = kstrdup(src_str[i], GFP_KERNEL);
+		if (!d[i] && src_str[i]) {
 			while (--i >= 0)
 				kfree(d[i]);
 			kfree(d);
@@ -380,20 +327,21 @@  static int property_entry_copy_data(struct property_entry *dst,
 			if (!new)
 				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->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 2c9d4d209296..ec8f84d564a8 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -233,13 +233,7 @@  struct property_entry {
 	bool is_array;
 	enum dev_prop_type type;
 	union {
-		union {
-			const u8 *u8_data;
-			const u16 *u16_data;
-			const u32 *u32_data;
-			const u64 *u64_data;
-			const char * const *str;
-		} pointer;
+		const void *pointer;
 		union {
 			u8 u8_data;
 			u16 u16_data;
@@ -262,7 +256,7 @@  struct property_entry {
 	.length = (_len_) * sizeof(_type_),				\
 	.is_array = true,						\
 	.type = DEV_PROP_##_Type_,					\
-	{ .pointer = { ._type_##_data = _val_ } },			\
+	{ .pointer = _val_ },						\
 }
 
 #define PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, _len_)		\
@@ -280,7 +274,7 @@  struct property_entry {
 	.length = (_len_) * sizeof(const char *),			\
 	.is_array = true,						\
 	.type = DEV_PROP_STRING,					\
-	{ .pointer = { .str = _val_ } },				\
+	{ .pointer = _val_ },						\
 }
 
 #define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)				\