Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Rob Herring]

+dt list

On Fri, May 1, 2015 at 1:03 AM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> The requirement is raised when developing the PCI hotplug feature
> for PowerPC PowerNV platform, which runs on top of skiboot firmware.
> When plugging PCI adapter to one PCI slot, the firmware rescans the
> slot and build FDT (Flat Device Tree) blob, which is sent to the
> PowerNV PCI hotplug driver for processing. The new constructed device
> nodes from the FDT blob are expected to be attached to the device
> node of the PCI slot. Unfortunately, it seems we don't have a API
> to support the scenario. The patch intends to support it by newly
> introduced function of_fdt_add_subtree(), the design behind it is
> shown as below:
>
>    * When the sub-tree FDT blob, which is owned by firmware, is
>      received by kernel. It's copied over to the blob, which is
>      dynamically allocated. Since then, the FDT blob owned by
>      firmware isn't touched.
>    * Rework unflatten_dt_node() so that the device nodes in current
>      and deeper depth have been constructed from the FDT blob. All
>      device nodes are marked with flag OF_DYNAMIC_HYBIRD, which is

Perhaps you meant HYBRID?

>      similar to OF_DYNAMIC. However, device node with the flag set
>      can be free'd, but in the way other than that for OF_DYNAMIC
>      device nodes.

The difference seems to be whether you allocate space or just point to
the FDT for various strings/data. Is that right?

>    * of_fdt_add_subtree() is the introduced API to do the work.

Have you looked at overlays and if so why do they not work for your purposes?

Why do you need to do this with the flattened tree?

Rob

>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  drivers/of/dynamic.c   |  19 +++++--
>  drivers/of/fdt.c       | 133 ++++++++++++++++++++++++++++++++++++++++---------
>  include/linux/of.h     |   2 +
>  include/linux/of_fdt.h |   1 +
>  4 files changed, 127 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 3351ef4..f562080 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -330,13 +330,22 @@ void of_node_release(struct kobject *kobj)
>                 return;
>         }
>
> -       if (!of_node_check_flag(node, OF_DYNAMIC))
> +       /* Release the subtree */
> +       if (node->subtree) {
> +               kfree(node->subtree);
> +               node->subtree = NULL;
> +       }
> +
> +       if (!of_node_check_flag(node, OF_DYNAMIC) &&
> +           !of_node_check_flag(node, OF_DYNAMIC_HYBIRD))
>                 return;
>
>         while (prop) {
>                 struct property *next = prop->next;
> -               kfree(prop->name);
> -               kfree(prop->value);
> +               if (of_node_check_flag(node, OF_DYNAMIC)) {
> +                       kfree(prop->name);
> +                       kfree(prop->value);
> +               }
>                 kfree(prop);
>                 prop = next;
>
> @@ -345,7 +354,9 @@ void of_node_release(struct kobject *kobj)
>                         node->deadprops = NULL;
>                 }
>         }
> -       kfree(node->full_name);
> +
> +       if (of_node_check_flag(node, OF_DYNAMIC))
> +               kfree(node->full_name);
>         kfree(node->data);
>         kfree(node);
>  }
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index cde35c5d01..7659560 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -28,6 +28,10 @@
>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>  #include <asm/page.h>
>
> +#include "of_private.h"
> +
> +static int cur_node_depth;
> +
>  /*
>   * of_fdt_limit_memory - limit the number of regions in the /memory node
>   * @limit: maximum entries
> @@ -168,20 +172,20 @@ static void *unflatten_dt_alloc(void **mem, unsigned long size,
>   * @dad: Parent struct device_node
>   * @fpsize: Size of the node path up at the current depth.
>   */
> -static void * unflatten_dt_node(void *blob,
> -                               void *mem,
> -                               int *poffset,
> -                               struct device_node *dad,
> -                               struct device_node **nodepp,
> -                               unsigned long fpsize,
> -                               bool dryrun)
> +static void *unflatten_dt_node(void *blob,
> +                              void *mem,
> +                              int *poffset,
> +                              struct device_node *dad,
> +                              struct device_node **nodepp,
> +                              unsigned long fpsize,
> +                              bool dryrun,
> +                              bool dynamic)
>  {
>         const __be32 *p;
>         struct device_node *np;
>         struct property *pp, **prev_pp = NULL;
>         const char *pathp;
>         unsigned int l, allocl;
> -       static int depth = 0;
>         int old_depth;
>         int offset;
>         int has_name = 0;
> @@ -219,12 +223,18 @@ static void * unflatten_dt_node(void *blob,
>                 }
>         }
>
> -       np = unflatten_dt_alloc(&mem, sizeof(struct device_node) + allocl,
> +       if (dynamic)
> +               np = kzalloc(sizeof(struct device_node) + allocl, GFP_KERNEL);
> +       else
> +               np = unflatten_dt_alloc(&mem,
> +                               sizeof(struct device_node) + allocl,
>                                 __alignof__(struct device_node));
>         if (!dryrun) {
>                 char *fn;
>                 of_node_init(np);
>                 np->full_name = fn = ((char *)np) + sizeof(*np);
> +               if (dynamic)
> +                       of_node_set_flag(np, OF_DYNAMIC_HYBIRD);
>                 if (new_format) {
>                         /* rebuild full path for new format */
>                         if (dad && dad->parent) {
> @@ -267,8 +277,12 @@ static void * unflatten_dt_node(void *blob,
>                 }
>                 if (strcmp(pname, "name") == 0)
>                         has_name = 1;
> -               pp = unflatten_dt_alloc(&mem, sizeof(struct property),
> -                                       __alignof__(struct property));
> +
> +               if (dynamic)
> +                       pp = kzalloc(sizeof(struct property), GFP_KERNEL);
> +               else
> +                       pp = unflatten_dt_alloc(&mem, sizeof(struct property),
> +                                               __alignof__(struct property));
>                 if (!dryrun) {
>                         /* We accept flattened tree phandles either in
>                          * ePAPR-style "phandle" properties, or the
> @@ -309,8 +323,13 @@ static void * unflatten_dt_node(void *blob,
>                 if (pa < ps)
>                         pa = p1;
>                 sz = (pa - ps) + 1;
> -               pp = unflatten_dt_alloc(&mem, sizeof(struct property) + sz,
> -                                       __alignof__(struct property));
> +
> +               if (dynamic)
> +                       pp = kzalloc(sizeof(struct property) + sz, GFP_KERNEL);
> +               else
> +                       pp = unflatten_dt_alloc(&mem,
> +                                               sizeof(struct property) + sz,
> +                                               __alignof__(struct property));
>                 if (!dryrun) {
>                         pp->name = "name";
>                         pp->length = sz;
> @@ -334,13 +353,21 @@ static void * unflatten_dt_node(void *blob,
>                         np->type = "<NULL>";
>         }
>
> -       old_depth = depth;
> -       *poffset = fdt_next_node(blob, *poffset, &depth);
> -       if (depth < 0)
> -               depth = 0;
> -       while (*poffset > 0 && depth > old_depth)
> -               mem = unflatten_dt_node(blob, mem, poffset, np, NULL,
> -                                       fpsize, dryrun);
> +       old_depth = cur_node_depth;
> +       *poffset = fdt_next_node(blob, *poffset, &cur_node_depth);
> +       while (*poffset > 0) {
> +               if (cur_node_depth < old_depth)
> +                       break;
> +
> +               if (cur_node_depth == old_depth)
> +                       mem = unflatten_dt_node(blob, mem, poffset,
> +                                               dad, NULL, fpsize,
> +                                               dryrun, dynamic);
> +               else if (cur_node_depth > old_depth)
> +                       mem = unflatten_dt_node(blob, mem, poffset,
> +                                               np, NULL, fpsize,
> +                                               dryrun, dynamic);
> +       }
>
>         if (*poffset < 0 && *poffset != -FDT_ERR_NOTFOUND)
>                 pr_err("unflatten: error %d processing FDT\n", *poffset);
> @@ -379,8 +406,8 @@ static void * unflatten_dt_node(void *blob,
>   * for the resulting tree
>   */
>  static void __unflatten_device_tree(void *blob,
> -                            struct device_node **mynodes,
> -                            void * (*dt_alloc)(u64 size, u64 align))
> +                               struct device_node **mynodes,
> +                               void * (*dt_alloc)(u64 size, u64 align))
>  {
>         unsigned long size;
>         int start;
> @@ -405,7 +432,9 @@ static void __unflatten_device_tree(void *blob,
>
>         /* First pass, scan for size */
>         start = 0;
> -       size = (unsigned long)unflatten_dt_node(blob, NULL, &start, NULL, NULL, 0, true);
> +       cur_node_depth = 1;
> +       size = (unsigned long)unflatten_dt_node(blob, NULL, &start, NULL,
> +                                               NULL, 0, true, false);
>         size = ALIGN(size, 4);
>
>         pr_debug("  size is %lx, allocating...\n", size);
> @@ -420,7 +449,8 @@ static void __unflatten_device_tree(void *blob,
>
>         /* Second pass, do actual unflattening */
>         start = 0;
> -       unflatten_dt_node(blob, mem, &start, NULL, mynodes, 0, false);
> +       cur_node_depth = 1;
> +       unflatten_dt_node(blob, mem, &start, NULL, mynodes, 0, false, false);
>         if (be32_to_cpup(mem + size) != 0xdeadbeef)
>                 pr_warning("End of tree marker overwritten: %08x\n",
>                            be32_to_cpup(mem + size));
> @@ -448,6 +478,61 @@ void of_fdt_unflatten_tree(unsigned long *blob,
>  }
>  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>
> +static void populate_sysfs_for_child_nodes(struct device_node *parent)
> +{
> +       struct device_node *child;
> +
> +       for_each_child_of_node(parent, child) {
> +               __of_attach_node_sysfs(child);
> +               populate_sysfs_for_child_nodes(child);
> +       }
> +}
> +
> +/**
> + * of_fdt_add_substree - Create sub-tree of device nodes
> + * @parent: parent device node to which the sub-tree will attach
> + * @blob: flat device tree blob representing the sub-tree
> + *
> + * Copy over the FDT blob, which passed from firmware, and then
> + * unflatten the sub-tree.
> + */
> +void of_fdt_add_subtree(struct device_node *parent, void *blob)
> +{
> +       int start = 0;
> +
> +       /* Validate the header */
> +       if (!blob || fdt_check_header(blob)) {
> +               pr_err("%s: Invalid device-tree blob header at 0x%p\n",
> +                      __func__, blob);
> +               return;
> +       }
> +
> +       /* Free the flat blob for last time lazily */
> +       if (parent->subtree) {
> +               kfree(parent->subtree);
> +               parent->subtree = NULL;
> +       }
> +
> +       /* Copy over the flat blob */
> +       parent->subtree = kzalloc(fdt_totalsize(blob), GFP_KERNEL);
> +       if (!parent->subtree) {
> +               pr_err("%s: Cannot copy over device-tree blob\n",
> +                      __func__);
> +               return;
> +       }
> +
> +       memcpy(parent->subtree, blob, fdt_totalsize(blob));
> +
> +       /* Unflatten it */
> +       mutex_lock(&of_mutex);
> +       cur_node_depth = 1;
> +       unflatten_dt_node(parent->subtree, NULL, &start, parent, NULL,
> +                         strlen(parent->full_name), false, true);
> +       populate_sysfs_for_child_nodes(parent);
> +       mutex_unlock(&of_mutex);
> +}
> +EXPORT_SYMBOL(of_fdt_add_subtree);
> +
>  /* Everything below here references initial_boot_params directly. */
>  int __initdata dt_root_addr_cells;
>  int __initdata dt_root_size_cells;
> diff --git a/include/linux/of.h b/include/linux/of.h
> index ddeaae6..ac50b02 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -60,6 +60,7 @@ struct device_node {
>         struct  device_node *sibling;
>         struct  kobject kobj;
>         unsigned long _flags;
> +       void    *subtree;
>         void    *data;
>  #if defined(CONFIG_SPARC)
>         const char *path_component_name;
> @@ -222,6 +223,7 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
>  #define OF_DETACHED    2 /* node has been detached from the device tree */
>  #define OF_POPULATED   3 /* device already created for the node */
>  #define OF_POPULATED_BUS       4 /* of_platform_populate recursed to children of this node */
> +#define OF_DYNAMIC_HYBIRD      5 /* similar to OF_DYNAMIC, but partially */
>
>  #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags)
>  #define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags)
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index 587ee50..1fb47d7 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -39,6 +39,7 @@ extern int of_fdt_match(const void *blob, unsigned long node,
>                         const char *const *compat);
>  extern void of_fdt_unflatten_tree(unsigned long *blob,
>                                struct device_node **mynodes);
> +extern void of_fdt_add_subtree(struct device_node *parent, void *blob);
>
>  /* TBD: Temporary export of fdt globals - remove when code fully merged */
>  extern int __initdata dt_root_addr_cells;
> --
> 2.1.0
>

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Fri, 2015-05-01 at 07:54 -0500, Rob Herring wrote:

> The difference seems to be whether you allocate space or just point to
> the FDT for various strings/data. Is that right?
> 
> >    * of_fdt_add_subtree() is the introduced API to do the work.
> 
> Have you looked at overlays and if so why do they not work for your purposes?
> 
> Why do you need to do this with the flattened tree?

The basic idea I asked Gavin to implement is that since the FW needs to
provide a bunch of DT updates to Linux at runtime in the form of new
nodes below an existing one, rather than doing it via some new/custom
format, instead, have it send a bit of FDT blob to expand under an
existing node.

As for the details of Gavin implementation, I haven't looked at it in
details yet so there might be issues there, however I don't know what
you mean by "overlays", any pointer ?

Cheers,
Ben.

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Rob Herring]

On Fri, May 1, 2015 at 10:22 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Fri, 2015-05-01 at 07:54 -0500, Rob Herring wrote:
>
>> The difference seems to be whether you allocate space or just point to
>> the FDT for various strings/data. Is that right?
>>
>> >    * of_fdt_add_subtree() is the introduced API to do the work.
>>
>> Have you looked at overlays and if so why do they not work for your purposes?
>>
>> Why do you need to do this with the flattened tree?
>
> The basic idea I asked Gavin to implement is that since the FW needs to
> provide a bunch of DT updates to Linux at runtime in the form of new
> nodes below an existing one, rather than doing it via some new/custom
> format, instead, have it send a bit of FDT blob to expand under an
> existing node.

Overlay = an FDT blob to graft into a live running system. Sounds like
the same thing.

> As for the details of Gavin implementation, I haven't looked at it in
> details yet so there might be issues there, however I don't know what
> you mean by "overlays", any pointer ?

CONFIG_OF_OVERLAY

http://events.linuxfoundation.org/sites/events/files/slides/dynamic-dt-keynote-v3.pdf

Rob

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Fri, 2015-05-01 at 13:46 -0500, Rob Herring wrote:
> On Fri, May 1, 2015 at 10:22 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Fri, 2015-05-01 at 07:54 -0500, Rob Herring wrote:
> >
> >> The difference seems to be whether you allocate space or just point to
> >> the FDT for various strings/data. Is that right?
> >>
> >> >    * of_fdt_add_subtree() is the introduced API to do the work.
> >>
> >> Have you looked at overlays and if so why do they not work for your purposes?
> >>
> >> Why do you need to do this with the flattened tree?
> >
> > The basic idea I asked Gavin to implement is that since the FW needs to
> > provide a bunch of DT updates to Linux at runtime in the form of new
> > nodes below an existing one, rather than doing it via some new/custom
> > format, instead, have it send a bit of FDT blob to expand under an
> > existing node.
> 
> Overlay = an FDT blob to graft into a live running system. Sounds like
> the same thing.
> 
> > As for the details of Gavin implementation, I haven't looked at it in
> > details yet so there might be issues there, however I don't know what
> > you mean by "overlays", any pointer ?
> 
> CONFIG_OF_OVERLAY
> 
> http://events.linuxfoundation.org/sites/events/files/slides/dynamic-dt-keynote-v3.pdf

Well, that looks horrendously complicated, poorly documented and totally
unused in-tree outside of the unittest stuff, yay ! It has all sort of
"features" that I don't really care about.

I still don't see what it buys me other than making my FW a lot more
complex having to generate all that additional fixup etc... crap that I
don't totally get yet.

What's wrong with just unflattening the nodes in place ? The DT comes
from the FW in the first place so all the phandles are already good in
the new added blob. Internally, the FW created new nodes in its internal
representation and flattened the subtree and sends that subtree to
Linux.

I don't plan to play "revert" either, if you unplug, I do need to remove
what's under the slot but that's true of boot time devices, not just
"new" ones, so the overlay stuff won't do the trick and I certainly
don't want to keep track...

Ben.

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Sat, 2015-05-02 at 08:57 +1000, Benjamin Herrenschmidt wrote:

> > Overlay = an FDT blob to graft into a live running system. Sounds like
> > the same thing.
> > 
> > > As for the details of Gavin implementation, I haven't looked at it in
> > > details yet so there might be issues there, however I don't know what
> > > you mean by "overlays", any pointer ?
> > 
> > CONFIG_OF_OVERLAY
> > 
> > http://events.linuxfoundation.org/sites/events/files/slides/dynamic-dt-keynote-v3.pdf
> 
> Well, that looks horrendously complicated, poorly documented and totally
> unused in-tree outside of the unittest stuff, yay ! It has all sort of
> "features" that I don't really care about.

Looking a bit more at it, I don't quite see how I can attach a subtree
using that stuff.

Instead, each node in the overlay seems to need extra nodes and
properties to refer to the original.

So the FW would essentially have to create something a lot more complex
than just reflattening a bit of its internal tree. For each internal
node, it will need to add all those __overlay__ nodes and properties.

That is not going to fly for me at all. It's order of magnitudes more
complex than the solution we are pursuing.

So I think for our use case, we should continue in the direction of
having a helper to unflatten a piece of FDT underneath an existing
node. I don't like the "HYBRID" stuff though, we should not refer to
the original FDT, we should just make them normal dynamic nodes.

Ben.

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Sat, 2015-05-02 at 09:29 +1000, Benjamin Herrenschmidt wrote:

> Looking a bit more at it, I don't quite see how I can attach a subtree
> using that stuff.
> 
> Instead, each node in the overlay seems to need extra nodes and
> properties to refer to the original.
> 
> So the FW would essentially have to create something a lot more complex
> than just reflattening a bit of its internal tree. For each internal
> node, it will need to add all those __overlay__ nodes and properties.
> 
> That is not going to fly for me at all. It's order of magnitudes more
> complex than the solution we are pursuing.
> 
> So I think for our use case, we should continue in the direction of
> having a helper to unflatten a piece of FDT underneath an existing
> node. I don't like the "HYBRID" stuff though, we should not refer to
> the original FDT, we should just make them normal dynamic nodes.

A bit more thought... if we were to use the overlay stuff, Gavin, what
we *could* do is add to OPAL FW internal representation a generation
count to every node and property.

That way we could essentially know whenever something's changed from
what we flattened originally for the kernel.

We can then create a generic (not PCI specific) call that generates
an overlay tree for every node and property that has a generation
count that is newer than what was flattened (or passed by the OS).

It's still a LOT more complex than what we need though...

Cheers,
Ben.

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Gavin Shan]

On Sat, May 02, 2015 at 12:48:26PM +1000, Benjamin Herrenschmidt wrote:
>On Sat, 2015-05-02 at 09:29 +1000, Benjamin Herrenschmidt wrote:
>
>> Looking a bit more at it, I don't quite see how I can attach a subtree
>> using that stuff.
>> 
>> Instead, each node in the overlay seems to need extra nodes and
>> properties to refer to the original.
>> 
>> So the FW would essentially have to create something a lot more complex
>> than just reflattening a bit of its internal tree. For each internal
>> node, it will need to add all those __overlay__ nodes and properties.
>> 
>> That is not going to fly for me at all. It's order of magnitudes more
>> complex than the solution we are pursuing.
>> 
>> So I think for our use case, we should continue in the direction of
>> having a helper to unflatten a piece of FDT underneath an existing
>> node. I don't like the "HYBRID" stuff though, we should not refer to
>> the original FDT, we should just make them normal dynamic nodes.

Just took a close look on the overlay code. Hopefully I understand
how it works completely. Yeah, there is one questions according to my
understanding. The "overlay" device node should have been in child list
of the device node, who also has the indicator to "target" node. That
means some one else has to create "overlay" node and figure out the
"target" node in advance, then invokes overlay module to apply the
changes. From this perspective, the mechanism is something used to
apply the changes to device-tree, not parsing and create device nodes
from input. It does gurantee all the changes will be applied or none
of them. So I agree on what Ben suggested: to continue the direction
of having a helper to unflatten FDT blobk underneath the existing node,
and "HYBRID" should be replaced with "OF_DYNAMIC".

>
>A bit more thought... if we were to use the overlay stuff, Gavin, what
>we *could* do is add to OPAL FW internal representation a generation
>count to every node and property.
>
>That way we could essentially know whenever something's changed from
>what we flattened originally for the kernel.
>
>We can then create a generic (not PCI specific) call that generates
>an overlay tree for every node and property that has a generation
>count that is newer than what was flattened (or passed by the OS).
>
>It's still a LOT more complex than what we need though...
>

Thanks, Ben. If we really need utilize overlay to support our case,
we need some one to parse the input (device-tree changes) from firmware
and create "overlay" device node and "target" node as I mentioned above.
It's not simpler than the way we had to support our case. I'm not sure
if we really need utilize overlay for our case.

Thanks,
Gavin

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Mon, 2015-05-04 at 11:30 +1000, Gavin Shan wrote:
> Thanks, Ben. If we really need utilize overlay to support our case,
> we need some one to parse the input (device-tree changes) from
> firmware
> and create "overlay" device node and "target" node as I mentioned
> above.
> It's not simpler than the way we had to support our case. I'm not sure
> if we really need utilize overlay for our case.

No, if we decide to go down that path, then the FW needs to create
the overlay.

This could be done by having some kind of versioning to all nodes and
properties using a global generation count.

Ie, if we "know" what we passed to Linux, we can generate an overlay
that contains everything that changed since then using the version
numbers.

However, we should probably encode the version in the tree itself and
have specific APIs to retrieve "from" a given version to properly deal
with kexec'ing a kernel since in that case, the new kernel will have
something that isn't version 0 but version N where N is the latest
applied overlay.

Also I don't know how removing nodes works with overlay. IE the overlay
system is designed around the idea of removing the overlay to retrieve
the original tree.

In our case, our overlays are meant to be fully committed, and I don't
know whether there's a way to keep track. On unplug, we will just remove
all the nodes below the slot.

Cheers,
Ben.

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Gavin Shan]

On Sat, May 02, 2015 at 09:29:36AM +1000, Benjamin Herrenschmidt wrote:
>On Sat, 2015-05-02 at 08:57 +1000, Benjamin Herrenschmidt wrote:
>
>> > Overlay = an FDT blob to graft into a live running system. Sounds like
>> > the same thing.
>> > 
>> > > As for the details of Gavin implementation, I haven't looked at it in
>> > > details yet so there might be issues there, however I don't know what
>> > > you mean by "overlays", any pointer ?
>> > 
>> > CONFIG_OF_OVERLAY
>> > 
>> > http://events.linuxfoundation.org/sites/events/files/slides/dynamic-dt-keynote-v3.pdf
>> 
>> Well, that looks horrendously complicated, poorly documented and totally
>> unused in-tree outside of the unittest stuff, yay ! It has all sort of
>> "features" that I don't really care about.
>
>Looking a bit more at it, I don't quite see how I can attach a subtree
>using that stuff.
>
>Instead, each node in the overlay seems to need extra nodes and
>properties to refer to the original.
>
>So the FW would essentially have to create something a lot more complex
>than just reflattening a bit of its internal tree. For each internal
>node, it will need to add all those __overlay__ nodes and properties.
>
>That is not going to fly for me at all. It's order of magnitudes more
>complex than the solution we are pursuing.
>
>So I think for our use case, we should continue in the direction of
>having a helper to unflatten a piece of FDT underneath an existing
>node. I don't like the "HYBRID" stuff though, we should not refer to
>the original FDT, we should just make them normal dynamic nodes.
>

The original FDT from firmware is copied over to the memory chunk
allocated from slab by kernel. So we refer to the copy of the FDT,
not original one. Yeah, "HYBRID" wouldn't be a good idea. If we
want make all device nodes and properties of the sub-tree "DYNAMIC",
the FDT hasn't to be copied over from skiboot to kernel, indicating
those dynamic device nodes and properties in the subtree can be
figured out directly from the FDT blob, which is owned by firmware.

Also, it just need small changes to code what we have. Not too much
changes needed.

Thanks,
Gavin

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Pantelis Antoniou]

Hi Ben,

> On May 2, 2015, at 01:57 , Benjamin Herrenschmidt <benh-XVmvHMARGAQRh2imMr4xaA@public.gmane.orgng.org> wrote:
> 
> On Fri, 2015-05-01 at 13:46 -0500, Rob Herring wrote:
>> On Fri, May 1, 2015 at 10:22 AM, Benjamin Herrenschmidt
>> <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
>>> On Fri, 2015-05-01 at 07:54 -0500, Rob Herring wrote:
>>> 
>>>> The difference seems to be whether you allocate space or just point to
>>>> the FDT for various strings/data. Is that right?
>>>> 
>>>>>   * of_fdt_add_subtree() is the introduced API to do the work.
>>>> 
>>>> Have you looked at overlays and if so why do they not work for your purposes?
>>>> 
>>>> Why do you need to do this with the flattened tree?
>>> 
>>> The basic idea I asked Gavin to implement is that since the FW needs to
>>> provide a bunch of DT updates to Linux at runtime in the form of new
>>> nodes below an existing one, rather than doing it via some new/custom
>>> format, instead, have it send a bit of FDT blob to expand under an
>>> existing node.
>> 
>> Overlay = an FDT blob to graft into a live running system. Sounds like
>> the same thing.
>> 
>>> As for the details of Gavin implementation, I haven't looked at it in
>>> details yet so there might be issues there, however I don't know what
>>> you mean by "overlays", any pointer ?
>> 
>> CONFIG_OF_OVERLAY
>> 
>> http://events.linuxfoundation.org/sites/events/files/slides/dynamic-dt-keynote-v3.pdf
> 
> Well, that looks horrendously complicated, poorly documented and totally
> unused in-tree outside of the unittest stuff, yay ! It has all sort of
> "features" that I don't really care about.
> 

If it was easy to get stuff in, it would get more of the real-use drivers
in.

> I still don't see what it buys me other than making my FW a lot more
> complex having to generate all that additional fixup etc... crap that I
> don't totally get yet.
> 

You don’t generate any additional fixups. You just compile with the option
that generates all the fixups for you.

> What's wrong with just unflattening the nodes in place ? The DT comes
> from the FW in the first place so all the phandles are already good in
> the new added blob. Internally, the FW created new nodes in its internal
> representation and flattened the subtree and sends that subtree to
> Linux.
> 
> I don't plan to play "revert" either, if you unplug, I do need to remove
> what's under the slot but that's true of boot time devices, not just
> "new" ones, so the overlay stuff won't do the trick and I certainly
> don't want to keep track…
> 

You get all of the corner cases handled for free. Perhaps it works for your
case too.

Perhaps you can educate me on what you need supported and we can make sure
it’s included.

> Ben.
> 
> 

Regards

— Pantelis

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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Mon, 2015-05-04 at 19:41 +0300, Pantelis Antoniou wrote:
> 
> You get all of the corner cases handled for free. Perhaps it works for
> your case too.
> 
> Perhaps you can educate me on what you need supported and we can make
> sure it’s included.

Which corner cases ?

IE, what I want is simply "update" the device-tree below a PCIe slot on
PCI hotplug.

The DT isn't "compiled" from a dts (it's amazing how many people seem to
believe this is the only way you get fdt's nowadays). It's dynamically
(ie programatically) generated by firmware at boot time and contains
whatever PCIe devices happen to be plugged during boot.

When doing PCIe hotplug operations, the kernel does various FW calls
(among others to control slot power), and during these, the FW re-probes
underneath the slot and refreshes its internal representation. So the
phandles remain fully consistent, there is no fixup needed.

We want the kernel to also update his copy as wee in order to avoid
keeping stale nodes that don't match what's there anymore. Also, when
plugging specific kind of IO drawers, the FW can provide additional node
and properties that will be used to control slots inside the drawers.

So what we need is:

  - On PCIe unplug, remove all old nodes below the slot
  - On PCIe plug, get all the new nodes from FW

Note that there is no need to do anything like platform device probing
etc... the PCI layer takes care of that, we will remove the old nodes
after the pci_dev are gone and create the new ones before Linux
re-probes the PCIe bus subtree.

So what we need is very simple: The removal can be handled without FW
help, and the plug case is a matter of just transferring all those new
nodes to Linux to re-expand.

Since the phandle etc... are all consistent with the original tree,
there is no fixups required.

So the "trivial" way to do it (and the way we have implemented the FW
side so far) is to have the FW simply "flatten" the subtree below the
slot and pass it to Linux, with the intent of expanding it back below
the slot node.

This is what Gavin proposed patches do.

The overlay mechanism adds all sorts of features that we don't seen to
need and would make the above more complex.

Cheers,
Ben.

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Tue, 2015-05-05 at 07:14 +1000, Benjamin Herrenschmidt wrote:
> So the "trivial" way to do it (and the way we have implemented the FW
> side so far) is to have the FW simply "flatten" the subtree below the
> slot and pass it to Linux, with the intent of expanding it back below
> the slot node.
> 
> This is what Gavin proposed patches do.
> 
> The overlay mechanism adds all sorts of features that we don't seen to
> need and would make the above more complex.

Guys, I never got a final answer from you on this. Are we ok with adding
the way to just expand a subtree or are you insistent we need to use the
overlap mechanism ?

Cheers,
Ben.

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Rob Herring]

On Wed, May 13, 2015 at 6:35 PM, Benjamin Herrenschmidt
<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> On Tue, 2015-05-05 at 07:14 +1000, Benjamin Herrenschmidt wrote:
>> So the "trivial" way to do it (and the way we have implemented the FW
>> side so far) is to have the FW simply "flatten" the subtree below the
>> slot and pass it to Linux, with the intent of expanding it back below
>> the slot node.
>>
>> This is what Gavin proposed patches do.
>>
>> The overlay mechanism adds all sorts of features that we don't seen to
>> need and would make the above more complex.
>
> Guys, I never got a final answer from you on this. Are we ok with adding
> the way to just expand a subtree or are you insistent we need to use the
> overlap mechanism ?

I haven't decided really.

The main thing with the current patch is I don't really like the added
complexity to unflatten_dt_node. It is already a fairly complex
function. Perhaps removing of "hybrid" as discussed will help?

If there are things we can do to make overlays easier to use in your
use case, I'd like to hear ideas. I don't really buy that being more
complex than needed is an obstacle. That is very often the case to
have common, scale-able solutions. I want to see a simple case be
simple to support.

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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Wed, 2015-05-13 at 19:18 -0500, Rob Herring wrote:

> I haven't decided really.
> 
> The main thing with the current patch is I don't really like the added
> complexity to unflatten_dt_node. It is already a fairly complex
> function. Perhaps removing of "hybrid" as discussed will help?

I agree, we should be able to make that much simpler, I was planning on
sorting that out with Gavin.

> If there are things we can do to make overlays easier to use in your
> use case, I'd like to hear ideas. I don't really buy that being more
> complex than needed is an obstacle. That is very often the case to
> have common, scale-able solutions. I want to see a simple case be
> simple to support.

Well, it's a LOT more complex from the FW perspective for a bunch of
features we don't really need, in a way because the DT update in our
case is just purely informational to avoid keeping wrong/outdated DT
bits, it has little functional impact (it might have a bit for interrupt
routing through bridges though).

However, I am also pursuing an approach on FW side using a generation
count in our nodes and properties which we could use to generate
arbitrary overlays if we know what generation linux has.

There might actual be a usage scenario for a generic way for our
firwmare to convey DT updates to Linux for other reasons.

A few things that I don't find in the overlay code (but maybe I haven't
looked at it hard enough):

 - Can it remove nodes/properties ?

 - Can it "commit" a changeset so it's permanently part of the main DT ?
We will never have a concept of "revertable" changesets, if we need a
subsequent update, we will get a new overlay from FW that will remove
what needs to be removed and add what needs to be added.

IE, our current mechanism without overlay is fairly simple:

  - On PCI unplug, we remove all nodes below the slot (from linux),
the FW does the equivalent internally.

  - On PCI re-plug, the FW internally builds new nodes and sends a
new subtree as an FDT that we can expand/attach.

Now we could consider that subtree as a changeset that can be undone,
but that wouldn't work for boot time. And subsequent updates wouldn't
have that concept of "undoing" anyway.

IE. conceptually, what overlays do today is quite rooted around the idea
of having a fixed "base" DT and some pre-compiled DTB overlays that
get added/removed. The design completely ignore the idea of a FW that
maintains a "live" tree which we want to keep in sync, which is what we
want to do here, or what we could do with a "live" open firmware
implementation.

Now we might be able to reconcile them, but it feels to me that the
overlay/changeset stuff is too rooted in the first concept...

Ben.


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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Pantelis Antoniou]

Hi Ben,

Sorry for taking this long to respond, but I am working on the same problem right
now. I thought I might have something to show, but not yet :)

My PCI overlay case is different. In my case there is no firmware and there
is the blob is provided as an overlay.

The idea is that for a given PCI bus, when a PCI device with a matching
device id, vendor id is probed a matching overlay should be applied.

The trickiness lies in the way that the way that the target is different
each time and how to handle generational issues (i.e. what happens if the pci device
is removed before the application of the overlay occurs, what happens when multiple
applications should happen in parallel, etc.)


> On May 14, 2015, at 03:54 , Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> On Wed, 2015-05-13 at 19:18 -0500, Rob Herring wrote:
> 
>> I haven't decided really.
>> 
>> The main thing with the current patch is I don't really like the added
>> complexity to unflatten_dt_node. It is already a fairly complex
>> function. Perhaps removing of "hybrid" as discussed will help?
> 
> I agree, we should be able to make that much simpler, I was planning on
> sorting that out with Gavin.
> 

I think using overlays should cover your case without any issues.
I don’t like messing with the unflatten method TBH.

>> If there are things we can do to make overlays easier to use in your
>> use case, I'd like to hear ideas. I don't really buy that being more
>> complex than needed is an obstacle. That is very often the case to
>> have common, scale-able solutions. I want to see a simple case be
>> simple to support.
> 
> Well, it's a LOT more complex from the FW perspective for a bunch of
> features we don't really need, in a way because the DT update in our
> case is just purely informational to avoid keeping wrong/outdated DT
> bits, it has little functional impact (it might have a bit for interrupt
> routing through bridges though).
> 
> However, I am also pursuing an approach on FW side using a generation
> count in our nodes and properties which we could use to generate
> arbitrary overlays if we know what generation linux has.
> 
> There might actual be a usage scenario for a generic way for our
> firwmare to convey DT updates to Linux for other reasons.
> 
> A few things that I don't find in the overlay code (but maybe I haven't
> looked at it hard enough):
> 
> - Can it remove nodes/properties ?
> 

Yes.

> - Can it "commit" a changeset so it's permanently part of the main DT ?
> We will never have a concept of "revertable" changesets, if we need a
> subsequent update, we will get a new overlay from FW that will remove
> what needs to be removed and add what needs to be added.
> 

The overlay when applied is a part of the kernel DT tree.
It is trivial to add a mechanism that simply commits everything and
tosses away the revert information.

Note that in that case you have to make provisions for the unflatten
blob to not be freed or for the device tree nodes/properties to be
dynamically allocated.

> IE, our current mechanism without overlay is fairly simple:
> 
>  - On PCI unplug, we remove all nodes below the slot (from linux),
> the FW does the equivalent internally.
> 

If you use an overlay, you just revert it and everything would
be as it was before, without anything hanging below the slot node.

Note that the ‘remove all nodes below the slot’ does not work for my case.

That is because there are devices being instantiated under the slot
(i2c busses, i2c devices, FPGAs etc) that need to be removed from the
system.

>  - On PCI re-plug, the FW internally builds new nodes and sends a
> new subtree as an FDT that we can expand/attach.
> 

You can easily send a DT blob containing an overlay from firmware.

It can be even easy, since you might not have to recreate the full blob
each time, but instead using flat device tree methods to populate the
few properties that change each time.

> Now we could consider that subtree as a changeset that can be undone,
> but that wouldn't work for boot time. And subsequent updates wouldn't
> have that concept of "undoing" anyway.
> 

I have posted another patch that does boot-time DT quirk which are
non-revertable.

https://lkml.org/lkml/2015/2/18/258

> IE. conceptually, what overlays do today is quite rooted around the idea
> of having a fixed "base" DT and some pre-compiled DTB overlays that
> get added/removed. The design completely ignore the idea of a FW that
> maintains a "live" tree which we want to keep in sync, which is what we
> want to do here, or what we could do with a "live" open firmware
> implementation.
> 
> Now we might be able to reconcile them, but it feels to me that the
> overlay/changeset stuff is too rooted in the first concept…
> 

The first DT overlays use case (beaglebone capes) is what got the concept
started.

Right now is a generic mechanism to apply modifications to the kernel
live tree, with the possibility to revert them.

> Ben.
> 
> 

Regards

— Pantelis

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Thu, 2015-05-14 at 09:23 +0300, Pantelis Antoniou wrote:

> > A few things that I don't find in the overlay code (but maybe I haven't
> > looked at it hard enough):
> > 
> > - Can it remove nodes/properties ?
> > 
> 
> Yes.

Ok, I've missed that when looking at the overlay code then, I'll have to
give it a closer look.

> > - Can it "commit" a changeset so it's permanently part of the main DT ?
> > We will never have a concept of "revertable" changesets, if we need a
> > subsequent update, we will get a new overlay from FW that will remove
> > what needs to be removed and add what needs to be added.
> > 
> 
> The overlay when applied is a part of the kernel DT tree.
> It is trivial to add a mechanism that simply commits everything and
> tosses away the revert information.
> 
> Note that in that case you have to make provisions for the unflatten
> blob to not be freed or for the device tree nodes/properties to be
> dynamically allocated.

I think it makes sense to do the dynamic thing anyway...

> > IE, our current mechanism without overlay is fairly simple:
> > 
> >  - On PCI unplug, we remove all nodes below the slot (from linux),
> > the FW does the equivalent internally.
> > 
> 
> If you use an overlay, you just revert it and everything would
> be as it was before, without anything hanging below the slot node.

Except that doesn't work for the boot time content which we get
from the firmware as part of the initial FDT (and we can't change that
without breaking backward compatibility).

> Note that the ‘remove all nodes below the slot’ does not work for my case.
> 
> That is because there are devices being instantiated under the slot
> (i2c busses, i2c devices, FPGAs etc) that need to be removed from the
> system.

Right while in my case, there isn't, it's just the standard OF PCI
representation generated by FW, the main thing is that it might have
some enriched properties for some known cable cards of external drawers
that are good to have.

> >  - On PCI re-plug, the FW internally builds new nodes and sends a
> > new subtree as an FDT that we can expand/attach.
> > 
> 
> You can easily send a DT blob containing an overlay from firmware.

Sending one is easy. Building it is not :-)

> It can be even easy, since you might not have to recreate the full blob
> each time, but instead using flat device tree methods to populate the
> few properties that change each time.

No, we basically have our internal tree in the firmware in a format
similar to Linux, ie, a pointer based tree. We can "flatten" it of
course, but generating an overlay is trickier. We can, it's just more
work and we are running out of time (I basically have to cut that FW in
the next few days, then we'll be stuck with whatever interfaces we
created, I have a big of time to fix bugs after that but that's about
it).

> > Now we could consider that subtree as a changeset that can be undone,
> > but that wouldn't work for boot time. And subsequent updates wouldn't
> > have that concept of "undoing" anyway.
> > 
> 
> I have posted another patch that does boot-time DT quirk which are
> non-revertable.
> 
> https://lkml.org/lkml/2015/2/18/258

Not sure how that applies in my case ... I can't change the
representation of the PCI subtree, this is standard OFW representation,
I can't change the FW to make it an overlay-like thing at boot time,
that would break existing kernels.

> > IE. conceptually, what overlays do today is quite rooted around the idea
> > of having a fixed "base" DT and some pre-compiled DTB overlays that
> > get added/removed. The design completely ignore the idea of a FW that
> > maintains a "live" tree which we want to keep in sync, which is what we
> > want to do here, or what we could do with a "live" open firmware
> > implementation.
> > 
> > Now we might be able to reconcile them, but it feels to me that the
> > overlay/changeset stuff is too rooted in the first concept…
> > 
> 
> The first DT overlays use case (beaglebone capes) is what got the concept
> started.
> 
> Right now is a generic mechanism to apply modifications to the kernel
> live tree, with the possibility to revert them.

Yes but as I said it's not really thought in term of keeping the kernel
tree in sync with an external dynamically generated tree. Maybe we can
fix it, but it's more complex...

Ben.

> > Ben.
> > 
> > 
> 
> Regards
> 
> — Pantelis

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Pantelis Antoniou]

Hi Ben,

> On May 14, 2015, at 09:46 , Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> On Thu, 2015-05-14 at 09:23 +0300, Pantelis Antoniou wrote:
> 
>>> A few things that I don't find in the overlay code (but maybe I haven't
>>> looked at it hard enough):
>>> 
>>> - Can it remove nodes/properties ?
>>> 
>> 
>> Yes.
> 
> Ok, I've missed that when looking at the overlay code then, I'll have to
> give it a closer look.
> 

Ok, let me be more specific. It used to be able to do it ;)
The problem was the format used (a ‘-‘ prefix to the name).

Since I didn’t have clear use for it, I was asked to drop it by Grant.

The removal capability is of-course there for the revert case.

>>> - Can it "commit" a changeset so it's permanently part of the main DT ?
>>> We will never have a concept of "revertable" changesets, if we need a
>>> subsequent update, we will get a new overlay from FW that will remove
>>> what needs to be removed and add what needs to be added.
>>> 
>> 
>> The overlay when applied is a part of the kernel DT tree.
>> It is trivial to add a mechanism that simply commits everything and
>> tosses away the revert information.
>> 
>> Note that in that case you have to make provisions for the unflatten
>> blob to not be freed or for the device tree nodes/properties to be
>> dynamically allocated.
> 
> I think it makes sense to do the dynamic thing anyway...
> 
>>> IE, our current mechanism without overlay is fairly simple:
>>> 
>>> - On PCI unplug, we remove all nodes below the slot (from linux),
>>> the FW does the equivalent internally.
>>> 
>> 
>> If you use an overlay, you just revert it and everything would
>> be as it was before, without anything hanging below the slot node.
> 
> Except that doesn't work for the boot time content which we get
> from the firmware as part of the initial FDT (and we can't change that
> without breaking backward compatibility).
> 

OK

>> Note that the ‘remove all nodes below the slot’ does not work for my case.
>> 
>> That is because there are devices being instantiated under the slot
>> (i2c busses, i2c devices, FPGAs etc) that need to be removed from the
>> system.
> 
> Right while in my case, there isn't, it's just the standard OF PCI
> representation generated by FW, the main thing is that it might have
> some enriched properties for some known cable cards of external drawers
> that are good to have.
> 

I see.

>>> - On PCI re-plug, the FW internally builds new nodes and sends a
>>> new subtree as an FDT that we can expand/attach.
>>> 
>> 
>> You can easily send a DT blob containing an overlay from firmware.
> 
> Sending one is easy. Building it is not :-)
> 

Heh, true ;)

>> It can be even easy, since you might not have to recreate the full blob
>> each time, but instead using flat device tree methods to populate the
>> few properties that change each time.
> 
> No, we basically have our internal tree in the firmware in a format
> similar to Linux, ie, a pointer based tree. We can "flatten" it of
> course, but generating an overlay is trickier. We can, it's just more
> work and we are running out of time (I basically have to cut that FW in
> the next few days, then we'll be stuck with whatever interfaces we
> created, I have a big of time to fix bugs after that but that's about
> it).
> 

Hmm, since you just want to transmit a whole subtree things are a bit
simpler.

You don’t need any of the fixups, and your target node is known.

So your overlay is simply:

/ {
	fragment@0 {
		target-path = “/foo”;
		__overlay__ {
			/* contents of the slot */
		};
	}; 
};

I think it’s possible to just bit-mangle a blob (in pseudo code).

	const u8 template_overlay_blob[] = { <compiled blob of the above> };

	flatten_slot(slot_blob);

	overlay_blob = allocate_new_blob(template_overlay_blob, slot_blob);

	overlay_node = find_node(overlay_blob, “/fragment@0/__overlay__);
	target_prop = find_prop(overlay_blob, “/fragment@0/target-path”);

	inject_slot_blob(overlay_blob, overlay_node, slot_blob);
	modify_slot_target(overlay_blob, target_prop, slot_target);
	
I don’t think you need to re-flatten anything, shuffling bits around with
memmove should work.

>>> Now we could consider that subtree as a changeset that can be undone,
>>> but that wouldn't work for boot time. And subsequent updates wouldn't
>>> have that concept of "undoing" anyway.
>>> 
>> 
>> I have posted another patch that does boot-time DT quirk which are
>> non-revertable.
>> 
>> https://lkml.org/lkml/2015/2/18/258
> 
> Not sure how that applies in my case ... I can't change the
> representation of the PCI subtree, this is standard OFW representation,
> I can't change the FW to make it an overlay-like thing at boot time,
> that would break existing kernels.
> 

The idea is to append the ‘quirk’ to the already booting device tree blob.

Another idea floating around was to simple concatenate the booting blob with
any overlay blobs you want applied at boot time.

>>> IE. conceptually, what overlays do today is quite rooted around the idea
>>> of having a fixed "base" DT and some pre-compiled DTB overlays that
>>> get added/removed. The design completely ignore the idea of a FW that
>>> maintains a "live" tree which we want to keep in sync, which is what we
>>> want to do here, or what we could do with a "live" open firmware
>>> implementation.
>>> 
>>> Now we might be able to reconcile them, but it feels to me that the
>>> overlay/changeset stuff is too rooted in the first concept…
>>> 
>> 
>> The first DT overlays use case (beaglebone capes) is what got the concept
>> started.
>> 
>> Right now is a generic mechanism to apply modifications to the kernel
>> live tree, with the possibility to revert them.
> 
> Yes but as I said it's not really thought in term of keeping the kernel
> tree in sync with an external dynamically generated tree. Maybe we can
> fix it, but it's more complex…
> 

Yes it is, unfortunately.

> Ben.
> 
>>> Ben.
>>> 
>>> 
>> 
>> Regards
>> 
>> — Pantelis
> 
> 

Regards

— Pantelis

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Thu, 2015-05-14 at 10:04 +0300, Pantelis Antoniou wrote:

> Hmm, since you just want to transmit a whole subtree things are a bit
> simpler.
> 
> You don’t need any of the fixups, and your target node is known.
> 
> So your overlay is simply:
> 
> / {
> 	fragment@0 {
> 		target-path = “/foo”;
> 		__overlay__ {
> 			/* contents of the slot */
> 		};
> 	}; 
> };
>
> I think it’s possible to just bit-mangle a blob (in pseudo code).
> 
> 	const u8 template_overlay_blob[] = { <compiled blob of the above> };
> 
> 	flatten_slot(slot_blob);
> 
> 	overlay_blob = allocate_new_blob(template_overlay_blob, slot_blob);
> 
> 	overlay_node = find_node(overlay_blob, “/fragment@0/__overlay__);
> 	target_prop = find_prop(overlay_blob, “/fragment@0/target-path”);
> 
> 	inject_slot_blob(overlay_blob, overlay_node, slot_blob);
> 	modify_slot_target(overlay_blob, target_prop, slot_target);
> 	
> I don’t think you need to re-flatten anything, shuffling bits around with
> memmove should work.

Fairly gross :-)

But yeah generating the overlay doesn't necessarily scare me, I can
generate a temp tree that is the overlay in which I "copy" the subtree
(or in my internal ptr-based representation I could have a concept of
alias which I follow while flattening).

That leaves me with these problems:

 - No support for removing of nodes, so that needs to be added back to
the format and to Linux unless I continue removing by hand in the PCI
hotplug code itself

 - No support for "committing" the overlay which needs to be added as
well.

> >>> Now we could consider that subtree as a changeset that can be undone,
> >>> but that wouldn't work for boot time. And subsequent updates wouldn't
> >>> have that concept of "undoing" anyway.
> >>> 
> >> 
> >> I have posted another patch that does boot-time DT quirk which are
> >> non-revertable.
> >> 
> >> https://lkml.org/lkml/2015/2/18/258
> > 
> > Not sure how that applies in my case ... I can't change the
> > representation of the PCI subtree, this is standard OFW representation,
> > I can't change the FW to make it an overlay-like thing at boot time,
> > that would break existing kernels.
> > 
> 
> The idea is to append the ‘quirk’ to the already booting device tree blob.

I know but that's not how things work for me. At boot time the FW passes
me one tree that contains all the PCI stuff it has probed.

> Another idea floating around was to simple concatenate the booting blob with
> any overlay blobs you want applied at boot time.

Sure but I don't get overlay blobs at boot time.

> >>> IE. conceptually, what overlays do today is quite rooted around the idea
> >>> of having a fixed "base" DT and some pre-compiled DTB overlays that
> >>> get added/removed. The design completely ignore the idea of a FW that
> >>> maintains a "live" tree which we want to keep in sync, which is what we
> >>> want to do here, or what we could do with a "live" open firmware
> >>> implementation.
> >>> 
> >>> Now we might be able to reconcile them, but it feels to me that the
> >>> overlay/changeset stuff is too rooted in the first concept…
> >>> 
> >> 
> >> The first DT overlays use case (beaglebone capes) is what got the concept
> >> started.
> >> 
> >> Right now is a generic mechanism to apply modifications to the kernel
> >> live tree, with the possibility to revert them.
> > 
> > Yes but as I said it's not really thought in term of keeping the kernel
> > tree in sync with an external dynamically generated tree. Maybe we can
> > fix it, but it's more complex…
> > 
> 
> Yes it is, unfortunately.

Right. Which makes the solution of just passing my bit of tree as a blob
which I expand in Linux where I want it rather than an overlay tempting
if we can make Gavin patch more palatable (removing the hybrid stuff
etc...).

Cheers,
Ben.

> > Ben.
> > 
> >>> Ben.
> >>> 
> >>> 
> >> 
> >> Regards
> >> 
> >> — Pantelis
> > 
> > 
> 
> Regards
> 
> — Pantelis


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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Pantelis Antoniou]

Hi Ben,

> On May 14, 2015, at 10:14 , Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> On Thu, 2015-05-14 at 10:04 +0300, Pantelis Antoniou wrote:
> 
>> Hmm, since you just want to transmit a whole subtree things are a bit
>> simpler.
>> 
>> You don’t need any of the fixups, and your target node is known.
>> 
>> So your overlay is simply:
>> 
>> / {
>> 	fragment@0 {
>> 		target-path = “/foo”;
>> 		__overlay__ {
>> 			/* contents of the slot */
>> 		};
>> 	}; 
>> };
>> 
>> I think it’s possible to just bit-mangle a blob (in pseudo code).
>> 
>> 	const u8 template_overlay_blob[] = { <compiled blob of the above> };
>> 
>> 	flatten_slot(slot_blob);
>> 
>> 	overlay_blob = allocate_new_blob(template_overlay_blob, slot_blob);
>> 
>> 	overlay_node = find_node(overlay_blob, “/fragment@0/__overlay__);
>> 	target_prop = find_prop(overlay_blob, “/fragment@0/target-path”);
>> 
>> 	inject_slot_blob(overlay_blob, overlay_node, slot_blob);
>> 	modify_slot_target(overlay_blob, target_prop, slot_target);
>> 	
>> I don’t think you need to re-flatten anything, shuffling bits around with
>> memmove should work.
> 
> Fairly gross :-)
> 

You don’t want to know how sausages are made, but they are delicious :)

> But yeah generating the overlay doesn't necessarily scare me, I can
> generate a temp tree that is the overlay in which I "copy" the subtree
> (or in my internal ptr-based representation I could have a concept of
> alias which I follow while flattening).
> 
> That leaves me with these problems:
> 
> - No support for removing of nodes, so that needs to be added back to
> the format and to Linux unless I continue removing by hand in the PCI
> hotplug code itself
> 

What kind of nodes/properties you need to remove at _application_ time?

What you describe is inserting a bunch of properties and nodes under
a slot’s device node. Reverting the overlay removes them all just fine.

> - No support for "committing" the overlay which needs to be added as
> well.
> 

That’s the easiest part.

>>>>> Now we could consider that subtree as a changeset that can be undone,
>>>>> but that wouldn't work for boot time. And subsequent updates wouldn't
>>>>> have that concept of "undoing" anyway.
>>>>> 
>>>> 
>>>> I have posted another patch that does boot-time DT quirk which are
>>>> non-revertable.
>>>> 
>>>> https://lkml.org/lkml/2015/2/18/258
>>> 
>>> Not sure how that applies in my case ... I can't change the
>>> representation of the PCI subtree, this is standard OFW representation,
>>> I can't change the FW to make it an overlay-like thing at boot time,
>>> that would break existing kernels.
>>> 
>> 
>> The idea is to append the ‘quirk’ to the already booting device tree blob.
> 
> I know but that's not how things work for me. At boot time the FW passes
> me one tree that contains all the PCI stuff it has probed.
> 
>> Another idea floating around was to simple concatenate the booting blob with
>> any overlay blobs you want applied at boot time.
> 
> Sure but I don't get overlay blobs at boot time.
> 
>>>>> IE. conceptually, what overlays do today is quite rooted around the idea
>>>>> of having a fixed "base" DT and some pre-compiled DTB overlays that
>>>>> get added/removed. The design completely ignore the idea of a FW that
>>>>> maintains a "live" tree which we want to keep in sync, which is what we
>>>>> want to do here, or what we could do with a "live" open firmware
>>>>> implementation.
>>>>> 
>>>>> Now we might be able to reconcile them, but it feels to me that the
>>>>> overlay/changeset stuff is too rooted in the first concept…
>>>>> 
>>>> 
>>>> The first DT overlays use case (beaglebone capes) is what got the concept
>>>> started.
>>>> 
>>>> Right now is a generic mechanism to apply modifications to the kernel
>>>> live tree, with the possibility to revert them.
>>> 
>>> Yes but as I said it's not really thought in term of keeping the kernel
>>> tree in sync with an external dynamically generated tree. Maybe we can
>>> fix it, but it's more complex…
>>> 
>> 
>> Yes it is, unfortunately.
> 
> Right. Which makes the solution of just passing my bit of tree as a blob
> which I expand in Linux where I want it rather than an overlay tempting
> if we can make Gavin patch more palatable (removing the hybrid stuff
> etc…)
> .
> 

I see. Well, how about this?

Who said you have to do the whole blob dance in the firmware?

You can just as easily pass the blob as it is to the linux kernel and
the kernel there can convert it to an overlay and apply it.

> Cheers,
> Ben.
> 
>>> Ben.
>>> 
>>>>> Ben.
>>>>> 
>>>>> 
>>>> 
>>>> Regards
>>>> 
>>>> — Pantelis
>>> 
>>> 
>> 
>> Regards
>> 
>> — Pantelis
> 
> 

Regards

— Pantelis

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Thu, 2015-05-14 at 10:19 +0300, Pantelis Antoniou wrote:

> 
> You don’t want to know how sausages are made, but they are delicious :)

 ... most of the time :)

> > But yeah generating the overlay doesn't necessarily scare me, I can
> > generate a temp tree that is the overlay in which I "copy" the subtree
> > (or in my internal ptr-based representation I could have a concept of
> > alias which I follow while flattening).
> > 
> > That leaves me with these problems:
> > 
> > - No support for removing of nodes, so that needs to be added back to
> > the format and to Linux unless I continue removing by hand in the PCI
> > hotplug code itself
> > 
> 
> What kind of nodes/properties you need to remove at _application_ time?

Well, if we stick to removing by hand in Linux for the unplug case, then
none.

> What you describe is inserting a bunch of properties and nodes under
> a slot’s device node. Reverting the overlay removes them all just fine.

Except that still doesn't work for boot time :-)

So I would have to do a special case on unplug:

	if (slot->dt_is_overlay) /* set to false at boot */
		remove_subtree_myself();
	else
		undo_overlay(slot->overlay);

> > - No support for "committing" the overlay which needs to be added as
> > well.
> > 
> 
> That’s the easiest part.

Yeah, I will need to get my head around the code a bit more but it
doesn't seem too scary.

> I see. Well, how about this?
> 
> Who said you have to do the whole blob dance in the firmware?
> 
> You can just as easily pass the blob as it is to the linux kernel and
> the kernel there can convert it to an overlay and apply it.

That's not that pretty but we can do that too which solve the problem of
fixing the FW interface.

There is however an argument to be made in having the FW be able to
generate arbitrary overlays. If we ever want to pass more "property"
updates or node updates to Linux at runtime.

A few cases have crept up on the radar, like updating the pstate tables
or VPD informations ...

If we go down that path, then I would implement a concept of generation
count in the firmware, so I can generate an overlay that include all the
changes since the last "generation" given to Linux.

However that requires supporting removal of nodes/properties. So I'm
tempted to keep that feature on the back burner and go with an ad-hoc
interface for PCI for now.

Ben.


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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

> So I would have to do a special case on unplug:
> 
> 	if (slot->dt_is_overlay) /* set to false at boot */
> 		remove_subtree_myself();
> 	else
> 		undo_overlay(slot->overlay);

Of course I just inverted the polarity of the if () in the example :-)

But you get the idea...

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Pantelis Antoniou]

Hi Ben,

> On May 14, 2015, at 10:25 , Benjamin Herrenschmidt <benh-XVmvHMARGARBNKmDvNhNkA@public.gmane.orging.org> wrote:
> 
> On Thu, 2015-05-14 at 10:19 +0300, Pantelis Antoniou wrote:
> 
>> 
>> You don’t want to know how sausages are made, but they are delicious :)
> 
> ... most of the time :)
> 
>>> But yeah generating the overlay doesn't necessarily scare me, I can
>>> generate a temp tree that is the overlay in which I "copy" the subtree
>>> (or in my internal ptr-based representation I could have a concept of
>>> alias which I follow while flattening).
>>> 
>>> That leaves me with these problems:
>>> 
>>> - No support for removing of nodes, so that needs to be added back to
>>> the format and to Linux unless I continue removing by hand in the PCI
>>> hotplug code itself
>>> 
>> 
>> What kind of nodes/properties you need to remove at _application_ time?
> 
> Well, if we stick to removing by hand in Linux for the unplug case, then
> none.
> 

OK

>> What you describe is inserting a bunch of properties and nodes under
>> a slot’s device node. Reverting the overlay removes them all just fine.
> 
> Except that still doesn't work for boot time :-)
> 
> So I would have to do a special case on unplug:
> 
> 	if (slot->dt_is_overlay) /* set to false at boot */
> 		remove_subtree_myself();
> 	else
> 		undo_overlay(slot->overlay);
> 

OK, in that case you do require removal. But in any case it’s the ‘negative’
of an already applied one, either at boot time or not.

Modifying the overlay code to apply a ‘negative’ property should do the trick.

Is that correct?

>>> - No support for "committing" the overlay which needs to be added as
>>> well.
>>> 
>> 
>> That’s the easiest part.
> 
> Yeah, I will need to get my head around the code a bit more but it
> doesn't seem too scary.
> 
>> I see. Well, how about this?
>> 
>> Who said you have to do the whole blob dance in the firmware?
>> 
>> You can just as easily pass the blob as it is to the linux kernel and
>> the kernel there can convert it to an overlay and apply it.
> 
> That's not that pretty but we can do that too which solve the problem of
> fixing the FW interface.
> 
> There is however an argument to be made in having the FW be able to
> generate arbitrary overlays. If we ever want to pass more "property"
> updates or node updates to Linux at runtime.
> 
> A few cases have crept up on the radar, like updating the pstate tables
> or VPD informations ...
> 
> If we go down that path, then I would implement a concept of generation
> count in the firmware, so I can generate an overlay that include all the
> changes since the last "generation" given to Linux.
> 

I will probably need that generation count myself for my PCI use case.

> However that requires supporting removal of nodes/properties. So I'm
> tempted to keep that feature on the back burner and go with an ad-hoc
> interface for PCI for now.
> 

I see. Bonne chance :)

> Ben.
> 
> 

Regards

— Pantelis

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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Thu, 2015-05-14 at 10:34 +0300, Pantelis Antoniou wrote:

> >> What you describe is inserting a bunch of properties and nodes under
> >> a slot’s device node. Reverting the overlay removes them all just fine.
> > 
> > Except that still doesn't work for boot time :-)
> > 
> > So I would have to do a special case on unplug:
> > 
> > 	if (slot->dt_is_overlay) /* set to false at boot */
> > 		remove_subtree_myself();
> > 	else
> > 		undo_overlay(slot->overlay);
> > 
> 
> OK, in that case you do require removal. But in any case it’s the ‘negative’
> of an already applied one, either at boot time or not.

Sort-of, unless we have a way in the overlay to simply specify node
removal statements so we don't have to explicitly remove all properties
(or even all children).

> Modifying the overlay code to apply a ‘negative’ property should do the trick.
> 
> Is that correct?

I would do negatives node and let Linux imply the properties (or even
children).

But yes, that would probably do.

 .../...

> I will probably need that generation count myself for my PCI use case.
> 
> > However that requires supporting removal of nodes/properties. So I'm
> > tempted to keep that feature on the back burner and go with an ad-hoc
> > interface for PCI for now.
> > 
> 
> I see. Bonne chance :)

Merci :)

Cheers,
Ben.


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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Pantelis Antoniou]

Hi Ben,

> On May 14, 2015, at 10:47 , Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 

[snip]

So I spend some time thinking about your use case and I think it boils down
to this:

I have a live tree in the firmware, I have made changes and I need to reflect
those changes to the live tree in the kernel.

Sounds like ‘how do I generate a patch for getting those two in sync'. No?

I can see where this might be useful for others as all.

I think we really need to create a liblivedt like we have libfdt since
we have a number of projects going about using/manipulating DT at runtime.

1. The linux kernel, with it’s own live tree implementation.
2. The device tree compiler (it has a live tree) custom implemented.
3. Your weird and wonderful (or wacky) firmware.
4. u-boot does use DT now, but it does with libfdt. I believe this is suboptimal.
5. barebox does DT as well.

Most of what we want to do with DT can be abstracted in a library I think that
all of those projects can use.

What are your thoughts?

Regards

— Pantelis

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Thu, 2015-05-14 at 14:02 +0300, Pantelis Antoniou wrote:
> Hi Ben,
> 
> > On May 14, 2015, at 10:47 , Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > 
> 
> [snip]
> 
> So I spend some time thinking about your use case and I think it boils down
> to this:
> 
> I have a live tree in the firmware, I have made changes and I need to reflect
> those changes to the live tree in the kernel.
> 
> Sounds like ‘how do I generate a patch for getting those two in sync'. No?

More or less.

> I can see where this might be useful for others as all.
> 
> I think we really need to create a liblivedt like we have libfdt since
> we have a number of projects going about using/manipulating DT at runtime.
> 
> 1. The linux kernel, with it’s own live tree implementation.
> 2. The device tree compiler (it has a live tree) custom implemented.
> 3. Your weird and wonderful (or wacky) firmware.
> 4. u-boot does use DT now, but it does with libfdt. I believe this is suboptimal.
> 5. barebox does DT as well.
> 
> Most of what we want to do with DT can be abstracted in a library I think that
> all of those projects can use.
> 
> What are your thoughts?

Well, we have at least two implementations, the kernel one and the one
in our OPAL firmware:

https://github.com/open-power/skiboot/blob/master/include/device.h
https://github.com/open-power/skiboot/blob/master/core/device.c

The latter uses some nice Rusty tricks (tm) for multiple argument
functions.

It would make sense to do a library somewhere yes. However, I need to
cut my firmware API pretty much today so I think for now I'll stick
to something Ad-Hoc for the PCI hotplug code that just passes the
bit of FDT with the new devices and leave the "grand project" of live
sync of the tree for later.

There are other implementations of live DT in various Open Firmware
variants out there, most are in Forth which I suggest you don't bother
with unless you enjoy pain, but I think at least one of these is
actually in C.

Cheers,
Ben.

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Grant Likely]

Sorry for not weighing in earlier, I've had other work keeping me away.

My short answer: don't use overlays. They're not what you need. Generic
CONFIG_OF_DYNAMIC should be all that is required to make changes in your
use case.

Overlays are a specific api for being able to apply a set of changes in
a revertable way, but as you say, it is a lot more complicated. However,
overlays are built on top of the of_changeset API which is a lot
simpler. It doesn't do any phandle resolution, and it doesn't do any
tracking. It takes a set of changes (attach node, detach node, add
property, remove property), an applies them to the live tree. At that
point the changes are permenant*.

It is documented in Documentation/devicetree/changesets.txt

Ideally, I want all DT changes to go through the changeset API so that
the lifecycle issues are delt with in one place. It also defers firing
notifiers until after the entire changeset is applied. With
of_attach_node/of_detach_node the notifiers are sent immediately after
each change when the tree may be in an inconsistent state. For example,
a driver expecting child nodes, but the child nodes haven't been added
yet.

Comments below...

* There is an API for reverting a changeset, which simply applies the
  changes backwards and in reverse. The overlay code uses it, but you
  won't need it.

On Thu, 14 May 2015 10:54:31 +1000
, Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
 wrote:
> On Wed, 2015-05-13 at 19:18 -0500, Rob Herring wrote:
> 
> > I haven't decided really.
> > 
> > The main thing with the current patch is I don't really like the added
> > complexity to unflatten_dt_node. It is already a fairly complex
> > function. Perhaps removing of "hybrid" as discussed will help?
> 
> I agree, we should be able to make that much simpler, I was planning on
> sorting that out with Gavin.

Ditto here. I don't want to have any new kinds of nodes created either.
They are either OF_DYNAMIC, and therefore freeable, or they are not and
cannot be freed.

> > If there are things we can do to make overlays easier to use in your
> > use case, I'd like to hear ideas. I don't really buy that being more
> > complex than needed is an obstacle. That is very often the case to
> > have common, scale-able solutions. I want to see a simple case be
> > simple to support.
> 
> Well, it's a LOT more complex from the FW perspective for a bunch of
> features we don't really need, in a way because the DT update in our
> case is just purely informational to avoid keeping wrong/outdated DT
> bits, it has little functional impact (it might have a bit for interrupt
> routing through bridges though).
> 
> However, I am also pursuing an approach on FW side using a generation
> count in our nodes and properties which we could use to generate
> arbitrary overlays if we know what generation linux has.
> 
> There might actual be a usage scenario for a generic way for our
> firwmare to convey DT updates to Linux for other reasons.
> 
> A few things that I don't find in the overlay code (but maybe I haven't
> looked at it hard enough):
> 
>  - Can it remove nodes/properties ?

Overlays: No, because I asked Pantelis to drop them.
Changeset: yes, absolutely

>  - Can it "commit" a changeset so it's permanently part of the main DT ?
> We will never have a concept of "revertable" changesets, if we need a
> subsequent update, we will get a new overlay from FW that will remove
> what needs to be removed and add what needs to be added.

Yes

> IE, our current mechanism without overlay is fairly simple:
> 
>   - On PCI unplug, we remove all nodes below the slot (from linux),
> the FW does the equivalent internally.
> 
>   - On PCI re-plug, the FW internally builds new nodes and sends a
> new subtree as an FDT that we can expand/attach.
> 
> Now we could consider that subtree as a changeset that can be undone,
> but that wouldn't work for boot time. And subsequent updates wouldn't
> have that concept of "undoing" anyway.
> 
> IE. conceptually, what overlays do today is quite rooted around the idea
> of having a fixed "base" DT and some pre-compiled DTB overlays that
> get added/removed. The design completely ignore the idea of a FW that
> maintains a "live" tree which we want to keep in sync, which is what we
> want to do here, or what we could do with a "live" open firmware
> implementation.

Right, which is exactly the reason for the changeset/overlay split.
Overlays assume a fixed base, and that overlays are kind of like plug-in
modules. changeset makes no such assumption.

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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt]

On Sun, 2015-06-07 at 08:54 +0100, Grant Likely wrote:
> > IE. conceptually, what overlays do today is quite rooted around the idea
> > of having a fixed "base" DT and some pre-compiled DTB overlays that
> > get added/removed. The design completely ignore the idea of a FW that
> > maintains a "live" tree which we want to keep in sync, which is what we
> > want to do here, or what we could do with a "live" open firmware
> > implementation.
> 
> Right, which is exactly the reason for the changeset/overlay split.
> Overlays assume a fixed base, and that overlays are kind of like plug-in
> modules. changeset makes no such assumption.

So you suggest we create a function that takes an fdt and an "anchor" as input,
and expands that FDT below that anchor, but does so by using the changeset API
under the hood ?

Even that looks somewhat tricky (turn that bit of FDT into a pile of changeset
actions), however, I can see how we could create a new function inside changeset
to attach a subtree.

Ie. of_attach_subtree() (which could have it's own reconfig action but we
don't care that much yet), which takes an expanded subtree and an anchor, and
calls of_attach_node() in effect for all nodes in there.

We could then have a two pass mechanism in our hotplug code:

 - Expand the bit of fdt into a separate tree
 - Use of_attach_subtree to "add" that subtree to the main tree

What do you think ?

Ben.


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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Grant Likely]

On Mon, Jun 8, 2015 at 9:57 PM, Benjamin Herrenschmidt
<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> On Sun, 2015-06-07 at 08:54 +0100, Grant Likely wrote:
>> > IE. conceptually, what overlays do today is quite rooted around the idea
>> > of having a fixed "base" DT and some pre-compiled DTB overlays that
>> > get added/removed. The design completely ignore the idea of a FW that
>> > maintains a "live" tree which we want to keep in sync, which is what we
>> > want to do here, or what we could do with a "live" open firmware
>> > implementation.
>>
>> Right, which is exactly the reason for the changeset/overlay split.
>> Overlays assume a fixed base, and that overlays are kind of like plug-in
>> modules. changeset makes no such assumption.
>
> So you suggest we create a function that takes an fdt and an "anchor" as input,
> and expands that FDT below that anchor, but does so by using the changeset API
> under the hood ?
>
> Even that looks somewhat tricky (turn that bit of FDT into a pile of changeset
> actions), however, I can see how we could create a new function inside changeset
> to attach a subtree.
>
> Ie. of_attach_subtree() (which could have it's own reconfig action but we
> don't care that much yet), which takes an expanded subtree and an anchor, and
> calls of_attach_node() in effect for all nodes in there.
>
> We could then have a two pass mechanism in our hotplug code:
>
>  - Expand the bit of fdt into a separate tree
>  - Use of_attach_subtree to "add" that subtree to the main tree
>
> What do you think ?

I like that.

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

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Gavin Shan]

On Mon, Jun 08, 2015 at 10:34:13PM +0100, Grant Likely wrote:
>On Mon, Jun 8, 2015 at 9:57 PM, Benjamin Herrenschmidt
><benh@kernel.crashing.org> wrote:
>> On Sun, 2015-06-07 at 08:54 +0100, Grant Likely wrote:
>>> > IE. conceptually, what overlays do today is quite rooted around the idea
>>> > of having a fixed "base" DT and some pre-compiled DTB overlays that
>>> > get added/removed. The design completely ignore the idea of a FW that
>>> > maintains a "live" tree which we want to keep in sync, which is what we
>>> > want to do here, or what we could do with a "live" open firmware
>>> > implementation.
>>>
>>> Right, which is exactly the reason for the changeset/overlay split.
>>> Overlays assume a fixed base, and that overlays are kind of like plug-in
>>> modules. changeset makes no such assumption.
>>
>> So you suggest we create a function that takes an fdt and an "anchor" as input,
>> and expands that FDT below that anchor, but does so by using the changeset API
>> under the hood ?
>>
>> Even that looks somewhat tricky (turn that bit of FDT into a pile of changeset
>> actions), however, I can see how we could create a new function inside changeset
>> to attach a subtree.
>>
>> Ie. of_attach_subtree() (which could have it's own reconfig action but we
>> don't care that much yet), which takes an expanded subtree and an anchor, and
>> calls of_attach_node() in effect for all nodes in there.
>>
>> We could then have a two pass mechanism in our hotplug code:
>>
>>  - Expand the bit of fdt into a separate tree
>>  - Use of_attach_subtree to "add" that subtree to the main tree
>>
>> What do you think ?
>
>I like that.
>

Thanks, Grant and Ben. Currently, I'm collecting more feedbacks for v5. So
it's something I will address in v6.

Thanks,
Gavin

>g.
>

Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Gavin Shan]

On Fri, May 01, 2015 at 07:54:03AM -0500, Rob Herring wrote:
>+dt list
>
>On Fri, May 1, 2015 at 1:03 AM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>> The requirement is raised when developing the PCI hotplug feature
>> for PowerPC PowerNV platform, which runs on top of skiboot firmware.
>> When plugging PCI adapter to one PCI slot, the firmware rescans the
>> slot and build FDT (Flat Device Tree) blob, which is sent to the
>> PowerNV PCI hotplug driver for processing. The new constructed device
>> nodes from the FDT blob are expected to be attached to the device
>> node of the PCI slot. Unfortunately, it seems we don't have a API
>> to support the scenario. The patch intends to support it by newly
>> introduced function of_fdt_add_subtree(), the design behind it is
>> shown as below:
>>
>>    * When the sub-tree FDT blob, which is owned by firmware, is
>>      received by kernel. It's copied over to the blob, which is
>>      dynamically allocated. Since then, the FDT blob owned by
>>      firmware isn't touched.
>>    * Rework unflatten_dt_node() so that the device nodes in current
>>      and deeper depth have been constructed from the FDT blob. All
>>      device nodes are marked with flag OF_DYNAMIC_HYBIRD, which is
>
>Perhaps you meant HYBRID?
>

Yeah, It should be "HYBRID".

>>      similar to OF_DYNAMIC. However, device node with the flag set
>>      can be free'd, but in the way other than that for OF_DYNAMIC
>>      device nodes.
>
>The difference seems to be whether you allocate space or just point to
>the FDT for various strings/data. Is that right?
>

It's correct. The FDT blob passed from firmware is copied by kernel to
the memory chunk, which is allocated from slab. That means the FDT blob
managed by firmware can be released in time. In kernel, the instances of
"struct device_node" and "struct property" are allocated from slab
dynamically, but some of their fields are points to the (copied) FDT
blob. It indicates the (copied) FDT can only be released when the sub-tree
is cut off completely.


>>    * of_fdt_add_subtree() is the introduced API to do the work.
>
>Have you looked at overlays and if so why do they not work for your purposes?
>
>Why do you need to do this with the flattened tree?
>

It seems that Ben already helped answering the questions. I'll reply
in other threads if necessary. Rob, thanks for review.

Thanks,
Gavin

>Rob
>
>>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  drivers/of/dynamic.c   |  19 +++++--
>>  drivers/of/fdt.c       | 133 ++++++++++++++++++++++++++++++++++++++++---------
>>  include/linux/of.h     |   2 +
>>  include/linux/of_fdt.h |   1 +
>>  4 files changed, 127 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index 3351ef4..f562080 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -330,13 +330,22 @@ void of_node_release(struct kobject *kobj)
>>                 return;
>>         }
>>
>> -       if (!of_node_check_flag(node, OF_DYNAMIC))
>> +       /* Release the subtree */
>> +       if (node->subtree) {
>> +               kfree(node->subtree);
>> +               node->subtree = NULL;
>> +       }
>> +
>> +       if (!of_node_check_flag(node, OF_DYNAMIC) &&
>> +           !of_node_check_flag(node, OF_DYNAMIC_HYBIRD))
>>                 return;
>>
>>         while (prop) {
>>                 struct property *next = prop->next;
>> -               kfree(prop->name);
>> -               kfree(prop->value);
>> +               if (of_node_check_flag(node, OF_DYNAMIC)) {
>> +                       kfree(prop->name);
>> +                       kfree(prop->value);
>> +               }
>>                 kfree(prop);
>>                 prop = next;
>>
>> @@ -345,7 +354,9 @@ void of_node_release(struct kobject *kobj)
>>                         node->deadprops = NULL;
>>                 }
>>         }
>> -       kfree(node->full_name);
>> +
>> +       if (of_node_check_flag(node, OF_DYNAMIC))
>> +               kfree(node->full_name);
>>         kfree(node->data);
>>         kfree(node);
>>  }
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index cde35c5d01..7659560 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -28,6 +28,10 @@
>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>  #include <asm/page.h>
>>
>> +#include "of_private.h"
>> +
>> +static int cur_node_depth;
>> +
>>  /*
>>   * of_fdt_limit_memory - limit the number of regions in the /memory node
>>   * @limit: maximum entries
>> @@ -168,20 +172,20 @@ static void *unflatten_dt_alloc(void **mem, unsigned long size,
>>   * @dad: Parent struct device_node
>>   * @fpsize: Size of the node path up at the current depth.
>>   */
>> -static void * unflatten_dt_node(void *blob,
>> -                               void *mem,
>> -                               int *poffset,
>> -                               struct device_node *dad,
>> -                               struct device_node **nodepp,
>> -                               unsigned long fpsize,
>> -                               bool dryrun)
>> +static void *unflatten_dt_node(void *blob,
>> +                              void *mem,
>> +                              int *poffset,
>> +                              struct device_node *dad,
>> +                              struct device_node **nodepp,
>> +                              unsigned long fpsize,
>> +                              bool dryrun,
>> +                              bool dynamic)
>>  {
>>         const __be32 *p;
>>         struct device_node *np;
>>         struct property *pp, **prev_pp = NULL;
>>         const char *pathp;
>>         unsigned int l, allocl;
>> -       static int depth = 0;
>>         int old_depth;
>>         int offset;
>>         int has_name = 0;
>> @@ -219,12 +223,18 @@ static void * unflatten_dt_node(void *blob,
>>                 }
>>         }
>>
>> -       np = unflatten_dt_alloc(&mem, sizeof(struct device_node) + allocl,
>> +       if (dynamic)
>> +               np = kzalloc(sizeof(struct device_node) + allocl, GFP_KERNEL);
>> +       else
>> +               np = unflatten_dt_alloc(&mem,
>> +                               sizeof(struct device_node) + allocl,
>>                                 __alignof__(struct device_node));
>>         if (!dryrun) {
>>                 char *fn;
>>                 of_node_init(np);
>>                 np->full_name = fn = ((char *)np) + sizeof(*np);
>> +               if (dynamic)
>> +                       of_node_set_flag(np, OF_DYNAMIC_HYBIRD);
>>                 if (new_format) {
>>                         /* rebuild full path for new format */
>>                         if (dad && dad->parent) {
>> @@ -267,8 +277,12 @@ static void * unflatten_dt_node(void *blob,
>>                 }
>>                 if (strcmp(pname, "name") == 0)
>>                         has_name = 1;
>> -               pp = unflatten_dt_alloc(&mem, sizeof(struct property),
>> -                                       __alignof__(struct property));
>> +
>> +               if (dynamic)
>> +                       pp = kzalloc(sizeof(struct property), GFP_KERNEL);
>> +               else
>> +                       pp = unflatten_dt_alloc(&mem, sizeof(struct property),
>> +                                               __alignof__(struct property));
>>                 if (!dryrun) {
>>                         /* We accept flattened tree phandles either in
>>                          * ePAPR-style "phandle" properties, or the
>> @@ -309,8 +323,13 @@ static void * unflatten_dt_node(void *blob,
>>                 if (pa < ps)
>>                         pa = p1;
>>                 sz = (pa - ps) + 1;
>> -               pp = unflatten_dt_alloc(&mem, sizeof(struct property) + sz,
>> -                                       __alignof__(struct property));
>> +
>> +               if (dynamic)
>> +                       pp = kzalloc(sizeof(struct property) + sz, GFP_KERNEL);
>> +               else
>> +                       pp = unflatten_dt_alloc(&mem,
>> +                                               sizeof(struct property) + sz,
>> +                                               __alignof__(struct property));
>>                 if (!dryrun) {
>>                         pp->name = "name";
>>                         pp->length = sz;
>> @@ -334,13 +353,21 @@ static void * unflatten_dt_node(void *blob,
>>                         np->type = "<NULL>";
>>         }
>>
>> -       old_depth = depth;
>> -       *poffset = fdt_next_node(blob, *poffset, &depth);
>> -       if (depth < 0)
>> -               depth = 0;
>> -       while (*poffset > 0 && depth > old_depth)
>> -               mem = unflatten_dt_node(blob, mem, poffset, np, NULL,
>> -                                       fpsize, dryrun);
>> +       old_depth = cur_node_depth;
>> +       *poffset = fdt_next_node(blob, *poffset, &cur_node_depth);
>> +       while (*poffset > 0) {
>> +               if (cur_node_depth < old_depth)
>> +                       break;
>> +
>> +               if (cur_node_depth == old_depth)
>> +                       mem = unflatten_dt_node(blob, mem, poffset,
>> +                                               dad, NULL, fpsize,
>> +                                               dryrun, dynamic);
>> +               else if (cur_node_depth > old_depth)
>> +                       mem = unflatten_dt_node(blob, mem, poffset,
>> +                                               np, NULL, fpsize,
>> +                                               dryrun, dynamic);
>> +       }
>>
>>         if (*poffset < 0 && *poffset != -FDT_ERR_NOTFOUND)
>>                 pr_err("unflatten: error %d processing FDT\n", *poffset);
>> @@ -379,8 +406,8 @@ static void * unflatten_dt_node(void *blob,
>>   * for the resulting tree
>>   */
>>  static void __unflatten_device_tree(void *blob,
>> -                            struct device_node **mynodes,
>> -                            void * (*dt_alloc)(u64 size, u64 align))
>> +                               struct device_node **mynodes,
>> +                               void * (*dt_alloc)(u64 size, u64 align))
>>  {
>>         unsigned long size;
>>         int start;
>> @@ -405,7 +432,9 @@ static void __unflatten_device_tree(void *blob,
>>
>>         /* First pass, scan for size */
>>         start = 0;
>> -       size = (unsigned long)unflatten_dt_node(blob, NULL, &start, NULL, NULL, 0, true);
>> +       cur_node_depth = 1;
>> +       size = (unsigned long)unflatten_dt_node(blob, NULL, &start, NULL,
>> +                                               NULL, 0, true, false);
>>         size = ALIGN(size, 4);
>>
>>         pr_debug("  size is %lx, allocating...\n", size);
>> @@ -420,7 +449,8 @@ static void __unflatten_device_tree(void *blob,
>>
>>         /* Second pass, do actual unflattening */
>>         start = 0;
>> -       unflatten_dt_node(blob, mem, &start, NULL, mynodes, 0, false);
>> +       cur_node_depth = 1;
>> +       unflatten_dt_node(blob, mem, &start, NULL, mynodes, 0, false, false);
>>         if (be32_to_cpup(mem + size) != 0xdeadbeef)
>>                 pr_warning("End of tree marker overwritten: %08x\n",
>>                            be32_to_cpup(mem + size));
>> @@ -448,6 +478,61 @@ void of_fdt_unflatten_tree(unsigned long *blob,
>>  }
>>  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>>
>> +static void populate_sysfs_for_child_nodes(struct device_node *parent)
>> +{
>> +       struct device_node *child;
>> +
>> +       for_each_child_of_node(parent, child) {
>> +               __of_attach_node_sysfs(child);
>> +               populate_sysfs_for_child_nodes(child);
>> +       }
>> +}
>> +
>> +/**
>> + * of_fdt_add_substree - Create sub-tree of device nodes
>> + * @parent: parent device node to which the sub-tree will attach
>> + * @blob: flat device tree blob representing the sub-tree
>> + *
>> + * Copy over the FDT blob, which passed from firmware, and then
>> + * unflatten the sub-tree.
>> + */
>> +void of_fdt_add_subtree(struct device_node *parent, void *blob)
>> +{
>> +       int start = 0;
>> +
>> +       /* Validate the header */
>> +       if (!blob || fdt_check_header(blob)) {
>> +               pr_err("%s: Invalid device-tree blob header at 0x%p\n",
>> +                      __func__, blob);
>> +               return;
>> +       }
>> +
>> +       /* Free the flat blob for last time lazily */
>> +       if (parent->subtree) {
>> +               kfree(parent->subtree);
>> +               parent->subtree = NULL;
>> +       }
>> +
>> +       /* Copy over the flat blob */
>> +       parent->subtree = kzalloc(fdt_totalsize(blob), GFP_KERNEL);
>> +       if (!parent->subtree) {
>> +               pr_err("%s: Cannot copy over device-tree blob\n",
>> +                      __func__);
>> +               return;
>> +       }
>> +
>> +       memcpy(parent->subtree, blob, fdt_totalsize(blob));
>> +
>> +       /* Unflatten it */
>> +       mutex_lock(&of_mutex);
>> +       cur_node_depth = 1;
>> +       unflatten_dt_node(parent->subtree, NULL, &start, parent, NULL,
>> +                         strlen(parent->full_name), false, true);
>> +       populate_sysfs_for_child_nodes(parent);
>> +       mutex_unlock(&of_mutex);
>> +}
>> +EXPORT_SYMBOL(of_fdt_add_subtree);
>> +
>>  /* Everything below here references initial_boot_params directly. */
>>  int __initdata dt_root_addr_cells;
>>  int __initdata dt_root_size_cells;
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index ddeaae6..ac50b02 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -60,6 +60,7 @@ struct device_node {
>>         struct  device_node *sibling;
>>         struct  kobject kobj;
>>         unsigned long _flags;
>> +       void    *subtree;
>>         void    *data;
>>  #if defined(CONFIG_SPARC)
>>         const char *path_component_name;
>> @@ -222,6 +223,7 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
>>  #define OF_DETACHED    2 /* node has been detached from the device tree */
>>  #define OF_POPULATED   3 /* device already created for the node */
>>  #define OF_POPULATED_BUS       4 /* of_platform_populate recursed to children of this node */
>> +#define OF_DYNAMIC_HYBIRD      5 /* similar to OF_DYNAMIC, but partially */
>>
>>  #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags)
>>  #define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags)
>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>> index 587ee50..1fb47d7 100644
>> --- a/include/linux/of_fdt.h
>> +++ b/include/linux/of_fdt.h
>> @@ -39,6 +39,7 @@ extern int of_fdt_match(const void *blob, unsigned long node,
>>                         const char *const *compat);
>>  extern void of_fdt_unflatten_tree(unsigned long *blob,
>>                                struct device_node **mynodes);
>> +extern void of_fdt_add_subtree(struct device_node *parent, void *blob);
>>
>>  /* TBD: Temporary export of fdt globals - remove when code fully merged */
>>  extern int __initdata dt_root_addr_cells;
>> --
>> 2.1.0
>>
>