| Message ID | 1338940402-28502-8-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote: > Phandle references work by having 2 pieces: > > - a "phandle" 1-cell property in the device tree node > - a reference to the same value in a property we want to point > to the other node > > To generate the 1-cell property, we need an allocation mechanism that > gives us a unique number space. This patch adds an allocator for these > properties. > > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > device_tree.c | 7 +++++++ > device_tree.h | 1 + > 2 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/device_tree.c b/device_tree.c > index d4f1f0a..317bdd0 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -220,6 +220,13 @@ int qemu_devtree_setprop_phandle(void *fdt, const char *node_path, > return qemu_devtree_setprop_cell(fdt, node_path, property, phandle); > } > > +uint32_t qemu_devtree_alloc_phandle(void *fdt) > +{ > + static int phandle = 0x8000; can easily double check for duplicates. Would also allow you to start from 1 rather than magic number 0x8000? while (fdt_node_offset_by_phandle(fdt, phandle) != FDT_ERR_NOTFOUND) phandle++; > + > + return phandle++; > +} > + > int qemu_devtree_nop_node(void *fdt, const char *node_path) > { > int r; > diff --git a/device_tree.h b/device_tree.h > index 5464dc7..f37a4da 100644 > --- a/device_tree.h > +++ b/device_tree.h > @@ -35,6 +35,7 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path, > int qemu_devtree_setprop_phandle(void *fdt, const char *node_path, > const char *property, const char *string); > uint32_t qemu_devtree_get_phandle(void *fdt, const char *path); > +uint32_t qemu_devtree_alloc_phandle(void *fdt); > int qemu_devtree_nop_node(void *fdt, const char *node_path); > int qemu_devtree_add_subnode(void *fdt, const char *name); >
On 06/06/2012 07:18 AM, Peter Crosthwaite wrote: > On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote: >> Phandle references work by having 2 pieces: >> >> - a "phandle" 1-cell property in the device tree node >> - a reference to the same value in a property we want to point >> to the other node >> >> To generate the 1-cell property, we need an allocation mechanism that >> gives us a unique number space. This patch adds an allocator for these >> properties. >> >> Signed-off-by: Alexander Graf<agraf@suse.de> >> --- >> device_tree.c | 7 +++++++ >> device_tree.h | 1 + >> 2 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/device_tree.c b/device_tree.c >> index d4f1f0a..317bdd0 100644 >> --- a/device_tree.c >> +++ b/device_tree.c >> @@ -220,6 +220,13 @@ int qemu_devtree_setprop_phandle(void *fdt, const char *node_path, >> return qemu_devtree_setprop_cell(fdt, node_path, property, phandle); >> } >> >> +uint32_t qemu_devtree_alloc_phandle(void *fdt) >> +{ >> + static int phandle = 0x8000; > can easily double check for duplicates. Would also allow you to start > from 1 rather than magic number 0x8000? > > while (fdt_node_offset_by_phandle(fdt, phandle) != FDT_ERR_NOTFOUND) > phandle++; That's a nice one. It'd even allow us to drop the external phandle starting id patch. Scott, would this work for you? Alex
On 06/06/2012 11:00 AM, Alexander Graf wrote: > On 06/06/2012 07:18 AM, Peter Crosthwaite wrote: >> On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote: >>> Phandle references work by having 2 pieces: >>> >>> - a "phandle" 1-cell property in the device tree node >>> - a reference to the same value in a property we want to point >>> to the other node >>> >>> To generate the 1-cell property, we need an allocation mechanism that >>> gives us a unique number space. This patch adds an allocator for these >>> properties. >>> >>> Signed-off-by: Alexander Graf<agraf@suse.de> >>> --- >>> device_tree.c | 7 +++++++ >>> device_tree.h | 1 + >>> 2 files changed, 8 insertions(+), 0 deletions(-) >>> >>> diff --git a/device_tree.c b/device_tree.c >>> index d4f1f0a..317bdd0 100644 >>> --- a/device_tree.c >>> +++ b/device_tree.c >>> @@ -220,6 +220,13 @@ int qemu_devtree_setprop_phandle(void *fdt, >>> const char *node_path, >>> return qemu_devtree_setprop_cell(fdt, node_path, property, >>> phandle); >>> } >>> >>> +uint32_t qemu_devtree_alloc_phandle(void *fdt) >>> +{ >>> + static int phandle = 0x8000; >> can easily double check for duplicates. Would also allow you to start >> from 1 rather than magic number 0x8000? You can't check for duplicates, because the tree fragments you'll be conflicting with haven't been added to the tree yet. That's done by a tool operating on the tree output by the first pass of qemu, and is fed back into the second pass of qemu. -Scott
On Thu, Jun 7, 2012 at 2:55 AM, Scott Wood <scottwood@freescale.com> wrote: > On 06/06/2012 11:00 AM, Alexander Graf wrote: >> On 06/06/2012 07:18 AM, Peter Crosthwaite wrote: >>> On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote: >>>> Phandle references work by having 2 pieces: >>>> >>>> - a "phandle" 1-cell property in the device tree node >>>> - a reference to the same value in a property we want to point >>>> to the other node >>>> >>>> To generate the 1-cell property, we need an allocation mechanism that >>>> gives us a unique number space. This patch adds an allocator for these >>>> properties. >>>> >>>> Signed-off-by: Alexander Graf<agraf@suse.de> >>>> --- >>>> device_tree.c | 7 +++++++ >>>> device_tree.h | 1 + >>>> 2 files changed, 8 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/device_tree.c b/device_tree.c >>>> index d4f1f0a..317bdd0 100644 >>>> --- a/device_tree.c >>>> +++ b/device_tree.c >>>> @@ -220,6 +220,13 @@ int qemu_devtree_setprop_phandle(void *fdt, >>>> const char *node_path, >>>> return qemu_devtree_setprop_cell(fdt, node_path, property, >>>> phandle); >>>> } >>>> >>>> +uint32_t qemu_devtree_alloc_phandle(void *fdt) >>>> +{ >>>> + static int phandle = 0x8000; >>> can easily double check for duplicates. Would also allow you to start >>> from 1 rather than magic number 0x8000? > > You can't check for duplicates, because the tree fragments you'll be > conflicting with haven't been added to the tree yet. That's done by a > tool operating on the tree output by the first pass of qemu, and is fed > back into the second pass of qemu. > Im not sure what you mean by "fragments [which] havent been added to the tree yet"? Im thinking here of the use model where you use the DTB API to modify and existing DTB (probably pc-bios/foo.dtb). Yes, for Alex's series im not sure it wins anything as you are starting a DTB from scratch (which will have no pre-existing phandles), but it guards against a potentially very obscure and hard to find bug in some other potential uses of this API. Regards, Peter > -Scott >
On 06/06/2012 07:15 PM, Peter Crosthwaite wrote: > On Thu, Jun 7, 2012 at 2:55 AM, Scott Wood <scottwood@freescale.com> wrote: >> On 06/06/2012 11:00 AM, Alexander Graf wrote: >>> On 06/06/2012 07:18 AM, Peter Crosthwaite wrote: >>>> On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote: >>>>> +uint32_t qemu_devtree_alloc_phandle(void *fdt) >>>>> +{ >>>>> + static int phandle = 0x8000; >>>> can easily double check for duplicates. Would also allow you to start >>>> from 1 rather than magic number 0x8000? >> >> You can't check for duplicates, because the tree fragments you'll be >> conflicting with haven't been added to the tree yet. That's done by a >> tool operating on the tree output by the first pass of qemu, and is fed >> back into the second pass of qemu. >> > > Im not sure what you mean by "fragments [which] havent been added to > the tree yet"? The use case I have in mind is running QEMU once in dumpdtb mode, merging in fragments from the host device tree for directly assigned devices, and passing the result back to a second invocation of QEMU. For the first pass of QEMU, the managing tool would look at the host device tree to determine a suitable phandle range to pass to QEMU. > Im thinking here of the use model where you use the DTB > API to modify and existing DTB (probably pc-bios/foo.dtb). Yes, for > Alex's series im not sure it wins anything as you are starting a DTB > from scratch (which will have no pre-existing phandles), but it guards > against a potentially very obscure and hard to find bug in some other > potential uses of this API. I'm not sure the API misuse it would protect against is particularly likely -- you'd have to have some phandle creators ignore the API, and others use the API, and the misbehaving phandle generator has to do its thing before the conflicting API-compliant phandle generation. Doing a phandle search is a fairly heavy operation, which may not be the biggest concern here, but still it shouldn't be called trivially. It would be different if QEMU were adding nodes to an existing tree, rather than creating a tree from scratch. In any case, it doesn't eliminate the need for passing in a starting point. -Scott
diff --git a/device_tree.c b/device_tree.c index d4f1f0a..317bdd0 100644 --- a/device_tree.c +++ b/device_tree.c @@ -220,6 +220,13 @@ int qemu_devtree_setprop_phandle(void *fdt, const char *node_path, return qemu_devtree_setprop_cell(fdt, node_path, property, phandle); } +uint32_t qemu_devtree_alloc_phandle(void *fdt) +{ + static int phandle = 0x8000; + + return phandle++; +} + int qemu_devtree_nop_node(void *fdt, const char *node_path) { int r; diff --git a/device_tree.h b/device_tree.h index 5464dc7..f37a4da 100644 --- a/device_tree.h +++ b/device_tree.h @@ -35,6 +35,7 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path, int qemu_devtree_setprop_phandle(void *fdt, const char *node_path, const char *property, const char *string); uint32_t qemu_devtree_get_phandle(void *fdt, const char *path); +uint32_t qemu_devtree_alloc_phandle(void *fdt); int qemu_devtree_nop_node(void *fdt, const char *node_path); int qemu_devtree_add_subnode(void *fdt, const char *name);
Phandle references work by having 2 pieces: - a "phandle" 1-cell property in the device tree node - a reference to the same value in a property we want to point to the other node To generate the 1-cell property, we need an allocation mechanism that gives us a unique number space. This patch adds an allocator for these properties. Signed-off-by: Alexander Graf <agraf@suse.de> --- device_tree.c | 7 +++++++ device_tree.h | 1 + 2 files changed, 8 insertions(+), 0 deletions(-)