Re: [PATCHv3 1/2] powerpc/of: split out new_property() for reusing
On Sat, Mar 7, 2020 at 3:59 AM Nathan Lynch wrote:
>
> Hi,
>
> Pingfan Liu writes:
> > Splitting out new_property() for coming reusing and moving it to
> > of_helpers.c.
>
> [...]
>
> > +struct property *new_property(const char *name, const int length,
> > + const unsigned char *value, struct property *last)
> > +{
> > + struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
> > +
> > + if (!new)
> > + return NULL;
> > +
> > + new->name = kstrdup(name, GFP_KERNEL);
> > + if (!new->name)
> > + goto cleanup;
> > + new->value = kmalloc(length + 1, GFP_KERNEL);
> > + if (!new->value)
> > + goto cleanup;
> > +
> > + memcpy(new->value, value, length);
> > + *(((char *)new->value) + length) = 0;
> > + new->length = length;
> > + new->next = last;
> > + return new;
> > +
> > +cleanup:
> > + kfree(new->name);
> > + kfree(new->value);
> > + kfree(new);
> > + return NULL;
> > +}
>
> This function in its current form isn't suitable for more general use:
>
> * It appears to be tailored to string properties - note the char * value
> parameter, the length + 1 allocation and nul termination.
>
> * Most code shouldn't need the 'last' argument. The code where this
> currently resides builds a list of properties and attaches it to a new
> node, bypassing of_add_property().
>
> Let's look at the call site you add in your next patch:
>
> + big = cpu_to_be64(p->bound_addr);
> + property = new_property("bound-addr", sizeof(u64), (const
> unsigned char *)&big,
> + NULL);
> + of_add_property(dn, property);
>
> So you have to use a cast, and this is going to allocate (sizeof(u64) + 1)
> for the value, is that what you want?
>
> I think you should leave that legacy pseries reconfig code undisturbed
> (frankly that stuff should get deprecated and removed) and if you want a
> generic helper it should look more like:
>
> struct property *of_property_new(const char *name, size_t length,
> const void *value, gfp_t allocflags)
>
> __of_prop_dup() looks like a good model/guide here.
Thanks for your good suggestion.
I will re-code based on your suggestion, if [2/2] turns out acceptable.
Regards,
Pingfan
Re: [PATCHv3 1/2] powerpc/of: split out new_property() for reusing
Hi,
Pingfan Liu writes:
> Splitting out new_property() for coming reusing and moving it to
> of_helpers.c.
[...]
> +struct property *new_property(const char *name, const int length,
> + const unsigned char *value, struct property *last)
> +{
> + struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
> +
> + if (!new)
> + return NULL;
> +
> + new->name = kstrdup(name, GFP_KERNEL);
> + if (!new->name)
> + goto cleanup;
> + new->value = kmalloc(length + 1, GFP_KERNEL);
> + if (!new->value)
> + goto cleanup;
> +
> + memcpy(new->value, value, length);
> + *(((char *)new->value) + length) = 0;
> + new->length = length;
> + new->next = last;
> + return new;
> +
> +cleanup:
> + kfree(new->name);
> + kfree(new->value);
> + kfree(new);
> + return NULL;
> +}
This function in its current form isn't suitable for more general use:
* It appears to be tailored to string properties - note the char * value
parameter, the length + 1 allocation and nul termination.
* Most code shouldn't need the 'last' argument. The code where this
currently resides builds a list of properties and attaches it to a new
node, bypassing of_add_property().
Let's look at the call site you add in your next patch:
+ big = cpu_to_be64(p->bound_addr);
+ property = new_property("bound-addr", sizeof(u64), (const
unsigned char *)&big,
+ NULL);
+ of_add_property(dn, property);
So you have to use a cast, and this is going to allocate (sizeof(u64) + 1)
for the value, is that what you want?
I think you should leave that legacy pseries reconfig code undisturbed
(frankly that stuff should get deprecated and removed) and if you want a
generic helper it should look more like:
struct property *of_property_new(const char *name, size_t length,
const void *value, gfp_t allocflags)
__of_prop_dup() looks like a good model/guide here.
Re: [PATCHv3 1/2] powerpc/of: split out new_property() for reusing
On 4/3/20 7:47 pm, Pingfan Liu wrote: Splitting out new_property() for coming reusing and moving it to of_helpers.c. Also do some coding style cleanup. Signed-off-by: Pingfan Liu To: linuxppc-dev@lists.ozlabs.org Cc: Benjamin Herrenschmidt Cc: Paul Mackerras Cc: Michael Ellerman Cc: Hari Bathini Cc: Aneesh Kumar K.V Cc: Oliver O'Halloran Cc: Dan Williams Cc: Andrew Donnellan Cc: Christophe Leroy Cc: Rob Herring Cc: Frank Rowand Cc: ke...@lists.infradead.org Would this be useful to move into generic OF code? Also if more functions are moving into of_helpers.c perhaps there should be a common function name prefix. Otherwise your style cleanup looks good. Reviewed-by: Andrew Donnellan -- Andrew Donnellan OzLabs, ADL Canberra a...@linux.ibm.com IBM Australia Limited
[PATCHv3 1/2] powerpc/of: split out new_property() for reusing
Splitting out new_property() for coming reusing and moving it to
of_helpers.c.
Also do some coding style cleanup.
Signed-off-by: Pingfan Liu
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: Hari Bathini
Cc: Aneesh Kumar K.V
Cc: Oliver O'Halloran
Cc: Dan Williams
Cc: Andrew Donnellan
Cc: Christophe Leroy
Cc: Rob Herring
Cc: Frank Rowand
Cc: ke...@lists.infradead.org
---
arch/powerpc/platforms/pseries/of_helpers.c | 28
arch/powerpc/platforms/pseries/of_helpers.h | 3 +++
arch/powerpc/platforms/pseries/reconfig.c | 26 --
3 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/of_helpers.c
b/arch/powerpc/platforms/pseries/of_helpers.c
index 66dfd82..1022e0f 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -7,6 +7,34 @@
#include "of_helpers.h"
+struct property *new_property(const char *name, const int length,
+ const unsigned char *value, struct property *last)
+{
+ struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
+
+ if (!new)
+ return NULL;
+
+ new->name = kstrdup(name, GFP_KERNEL);
+ if (!new->name)
+ goto cleanup;
+ new->value = kmalloc(length + 1, GFP_KERNEL);
+ if (!new->value)
+ goto cleanup;
+
+ memcpy(new->value, value, length);
+ *(((char *)new->value) + length) = 0;
+ new->length = length;
+ new->next = last;
+ return new;
+
+cleanup:
+ kfree(new->name);
+ kfree(new->value);
+ kfree(new);
+ return NULL;
+}
+
/**
* pseries_of_derive_parent - basically like dirname(1)
* @path: the full_name of a node to be added to the tree
diff --git a/arch/powerpc/platforms/pseries/of_helpers.h
b/arch/powerpc/platforms/pseries/of_helpers.h
index decad65..34add82 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.h
+++ b/arch/powerpc/platforms/pseries/of_helpers.h
@@ -4,6 +4,9 @@
#include
+struct property *new_property(const char *name, const int length,
+ const unsigned char *value, struct property *last);
+
struct device_node *pseries_of_derive_parent(const char *path);
#endif /* _PSERIES_OF_HELPERS_H */
diff --git a/arch/powerpc/platforms/pseries/reconfig.c
b/arch/powerpc/platforms/pseries/reconfig.c
index 7f7369f..8e5a2ba 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -165,32 +165,6 @@ static char * parse_next_property(char *buf, char *end,
char **name, int *length
return tmp;
}
-static struct property *new_property(const char *name, const int length,
-const unsigned char *value, struct
property *last)
-{
- struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
-
- if (!new)
- return NULL;
-
- if (!(new->name = kstrdup(name, GFP_KERNEL)))
- goto cleanup;
- if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
- goto cleanup;
-
- memcpy(new->value, value, length);
- *(((char *)new->value) + length) = 0;
- new->length = length;
- new->next = last;
- return new;
-
-cleanup:
- kfree(new->name);
- kfree(new->value);
- kfree(new);
- return NULL;
-}
-
static int do_add_node(char *buf, size_t bufsize)
{
char *path, *end, *name;
--
2.7.5
