Re: [PATCH 1/3] of: overlay_adjust_phandles() - do not modify const field

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Mon, Apr 24, 2017 at 12:20 AM,  <frowand.list@xxxxxxxxx> wrote:
> From: Frank Rowand <frank.rowand@xxxxxxxx>
>
> When adjusting overlay phandles to apply to the live device tree, can
> not modify the property value because it is type const.
>
> This is to resolve the issue found by Stephen Boyd [1] when he changed
> the type of struct property.value from void * to const void *.  As
> a result of the type change, the overlay code had compile errors
> where the resolver updates phandle values.

Conceptually, I prefer your first version. phandles are special and
there's little reason to expose them except to generate a dts or dtb
from /proc/device-tree. We could still generate the phandle file in
that case, but I don't know if special casing phandle is worth it.

>
>   [1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html
>
> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
> ---
>  drivers/of/base.c       |  4 ++--
>  drivers/of/dynamic.c    | 28 +++++++++++++++++++++------
>  drivers/of/of_private.h |  3 +++
>  drivers/of/resolver.c   | 51 ++++++++++++++++++++++++++++++-------------------
>  4 files changed, 58 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d7c4629a3a2d..b41650fd0fcf 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -220,8 +220,8 @@ void __init of_core_init(void)
>                 proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>  }
>
> -static struct property *__of_find_property(const struct device_node *np,
> -                                          const char *name, int *lenp)
> +struct property *__of_find_property(const struct device_node *np,
> +                                   const char *name, int *lenp)
>  {
>         struct property *pp;
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 888fdbc09992..44963b4e7235 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -354,17 +354,17 @@ void of_node_release(struct kobject *kobj)
>  }
>
>  /**
> - * __of_prop_dup - Copy a property dynamically.
> + * __of_prop_alloc - Create a property dynamically.
>   * @prop:      Property to copy
>   * @allocflags:        Allocation flags (typically pass GFP_KERNEL)
>   *
> - * Copy a property by dynamically allocating the memory of both the
> + * Create a property by dynamically allocating the memory of both the
>   * property structure and the property name & contents. The property's
>   * flags have the OF_DYNAMIC bit set so that we can differentiate between
>   * dynamically allocated properties and not.
>   * Returns the newly allocated property or NULL on out of memory error.
>   */
> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +struct property *__of_prop_alloc(char *name, void *value, int len, gfp_t allocflags)
>  {
>         struct property *new;
>
> @@ -378,9 +378,9 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
>          * of zero bytes. We do this to work around the use
>          * of of_get_property() calls on boolean values.
>          */
> -       new->name = kstrdup(prop->name, allocflags);
> -       new->value = kmemdup(prop->value, prop->length, allocflags);
> -       new->length = prop->length;
> +       new->name = kstrdup(name, allocflags);
> +       new->value = kmemdup(value, len, allocflags);
> +       new->length = len;
>         if (!new->name || !new->value)
>                 goto err_free;
>
> @@ -397,6 +397,22 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
>  }
>
>  /**
> + * __of_prop_dup - Copy a property dynamically.
> + * @prop:      Property to copy
> + * @allocflags:        Allocation flags (typically pass GFP_KERNEL)
> + *
> + * Copy a property by dynamically allocating the memory of both the
> + * property structure and the property name & contents. The property's
> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
> + * dynamically allocated properties and not.
> + * Returns the newly allocated property or NULL on out of memory error.
> + */
> +struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +{
> +       return __of_prop_alloc(prop->name, prop->value, prop->length, allocflags);
> +}
> +
> +/**
>   * __of_node_dup() - Duplicate or create an empty device node dynamically.
>   * @fmt: Format string (plus vargs) for new full name of the device node
>   *
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 18bbb4517e25..554394c96569 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -62,6 +62,7 @@ static inline int of_property_notify(int action, struct device_node *np,
>   * without taking node references, so you either have to
>   * own the devtree lock or work on detached trees only.
>   */
> +struct property *__of_prop_alloc(char *name, void *value, int len, gfp_t allocflags);
>  struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
>  __printf(2, 3) struct device_node *__of_node_dup(const struct device_node *np, const char *fmt, ...);
>
> @@ -70,6 +71,8 @@ extern const void *__of_get_property(const struct device_node *np,
>  extern int __of_add_property(struct device_node *np, struct property *prop);
>  extern int __of_add_property_sysfs(struct device_node *np,
>                 struct property *prop);
> +extern struct property *__of_find_property(const struct device_node *np,
> +                                          const char *name, int *lenp);
>  extern int __of_remove_property(struct device_node *np, struct property *prop);
>  extern void __of_remove_property_sysfs(struct device_node *np,
>                 struct property *prop);
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 7ae9863cb0a4..a2d5b8f0b7bf 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c
> @@ -20,6 +20,8 @@
>  #include <linux/errno.h>
>  #include <linux/slab.h>
>
> +#include "of_private.h"
> +
>  /* illegal phandle value (set when unresolved) */
>  #define OF_PHANDLE_ILLEGAL     0xdeadbeef
>
> @@ -67,36 +69,43 @@ static phandle live_tree_max_phandle(void)
>         return phandle;
>  }
>
> -static void adjust_overlay_phandles(struct device_node *overlay,
> +static int adjust_overlay_phandles(struct device_node *overlay,
>                 int phandle_delta)
>  {
>         struct device_node *child;
> +       struct property *newprop;
>         struct property *prop;
>         phandle phandle;

Some of these can move into the if statement. That will save some
stack space on recursion (or maybe the compiler is smart enough).

> +       int ret;
>
> -       /* adjust node's phandle in node */
> -       if (overlay->phandle != 0 && overlay->phandle != OF_PHANDLE_ILLEGAL)
> -               overlay->phandle += phandle_delta;
> -
> -       /* copy adjusted phandle into *phandle properties */
> -       for_each_property_of_node(overlay, prop) {
> +       if (overlay->phandle != 0 && overlay->phandle != OF_PHANDLE_ILLEGAL) {
>
> -               if (of_prop_cmp(prop->name, "phandle") &&
> -                   of_prop_cmp(prop->name, "linux,phandle"))
> -                       continue;
> -
> -               if (prop->length < 4)
> -                       continue;
> +               overlay->phandle += phandle_delta;
>
> -               phandle = be32_to_cpup(prop->value);
> -               if (phandle == OF_PHANDLE_ILLEGAL)
> -                       continue;
> +               phandle = cpu_to_be32(overlay->phandle);
> +
> +               prop = __of_find_property(overlay, "phandle", NULL);
> +               newprop = __of_prop_alloc(prop->name, &phandle, sizeof(phandle),
> +                                         GFP_KERNEL);
> +               if (!newprop)
> +                       return -ENOMEM;
> +               __of_update_property(overlay, newprop, &prop);
> +
> +               prop = __of_find_property(overlay, "linux,phandle", NULL);
> +               newprop = __of_prop_alloc(prop->name, &phandle, sizeof(phandle),
> +                                         GFP_KERNEL);

There is no reason to support "linux,phandle" for overlays. That is
legacy (pre ePAPR) which predates any overlays by a long time.

Also, dtc still defaults to generating both phandle and linux,phandle
properties which maybe we should switch off now. If anything, we're
wasting a bit of memory storing both. I think we should only store
"phandle" and convert any cases of only a "linux,phandle" property to
"phandle".

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux