Patchwork [07/31] dt: add helper for phandle allocation

login
register
mail settings
Submitter Alexander Graf
Date June 5, 2012, 11:52 p.m.
Message ID <1338940402-28502-8-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/163219/
State New
Headers show

Comments

Alexander Graf - June 5, 2012, 11:52 p.m.
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(-)
Peter A. G. Crosthwaite - June 6, 2012, 5:18 a.m.
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);
>
Alexander Graf - June 6, 2012, 4 p.m.
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
Scott Wood - June 6, 2012, 4:55 p.m.
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
Peter A. G. Crosthwaite - June 7, 2012, 12:15 a.m.
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
>
Scott Wood - June 7, 2012, 12:31 a.m.
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

Patch

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);