diff mbox series

[V7,1/3] of: dynamic: Add interfaces for creating device node dynamically

Message ID 1674183732-5157-2-git-send-email-lizhi.hou@amd.com
State New
Headers
Series Generate device tree node for pci devices |

Commit Message

Lizhi Hou Jan. 20, 2023, 3:02 a.m. UTC 
  of_create_node() creates device node dynamically. The parent device node
and full name are required for creating the node. It optionally creates
an OF changeset and attaches the newly created node to the changeset. The
device node pointer and the changeset pointer can be used to add
properties to the device node and apply the node to the base tree.

of_destroy_node() frees the device node created by of_create_node(). If
an OF changeset was also created for this node, it will destroy the
changeset before freeing the device node.

Expand of_changeset APIs to handle specific types of properties.
    of_changeset_add_prop_string()
    of_changeset_add_prop_string_array()
    of_changeset_add_prop_u32_array()

Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Signed-off-by: Sonal Santan <sonal.santan@amd.com>
Signed-off-by: Max Zhen <max.zhen@amd.com>
Reviewed-by: Brian Xu <brian.xu@amd.com>
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/of/dynamic.c | 197 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   |  24 ++++++
 2 files changed, 221 insertions(+)

Comments

Rob Herring March 23, 2023, 10:40 p.m. UTC | #1 
On Thu, Jan 19, 2023 at 9:02 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
>
> of_create_node() creates device node dynamically. The parent device node
> and full name are required for creating the node. It optionally creates
> an OF changeset and attaches the newly created node to the changeset. The
> device node pointer and the changeset pointer can be used to add
> properties to the device node and apply the node to the base tree.
>
> of_destroy_node() frees the device node created by of_create_node(). If
> an OF changeset was also created for this node, it will destroy the
> changeset before freeing the device node.
>
> Expand of_changeset APIs to handle specific types of properties.
>     of_changeset_add_prop_string()
>     of_changeset_add_prop_string_array()
>     of_changeset_add_prop_u32_array()
>
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>

Your Sob should be last because you sent this patch. The order of Sob
is roughly the order of possession of the patch.

> Signed-off-by: Sonal Santan <sonal.santan@amd.com>
> Signed-off-by: Max Zhen <max.zhen@amd.com>

So Sonal and Max modified this patch?

> Reviewed-by: Brian Xu <brian.xu@amd.com>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>

Why does this have Clément's Sob?

> ---
>  drivers/of/dynamic.c | 197 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h   |  24 ++++++
>  2 files changed, 221 insertions(+)
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index cd3821a6444f..4e211a1d039f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -461,6 +461,71 @@ struct device_node *__of_node_dup(const struct device_node *np,
>         return NULL;
>  }
>
> +/**
> + * of_create_node - Dynamically create a device node

For consistency, I think this should be of_changeset_create_node().

> + *
> + * @parent: Pointer to parent device node
> + * @full_name: Node full name
> + * @cset: Pointer to returning changeset
> + *
> + * Return: Pointer to the created device node or NULL in case of an error.
> + */
> +struct device_node *of_create_node(struct device_node *parent,
> +                                  const char *full_name,
> +                                  struct of_changeset **cset)
> +{
> +       struct of_changeset *ocs;
> +       struct device_node *np;
> +       int ret;
> +
> +       np = __of_node_dup(NULL, full_name);
> +       if (!np)
> +               return NULL;
> +       np->parent = parent;
> +
> +       if (!cset)
> +               return np;
> +
> +       ocs = kmalloc(sizeof(*ocs), GFP_KERNEL);
> +       if (!ocs) {
> +               of_node_put(np);
> +               return NULL;
> +       }
> +
> +       of_changeset_init(ocs);
> +       ret = of_changeset_attach_node(ocs, np);
> +       if (ret) {
> +               of_changeset_destroy(ocs);
> +               of_node_put(np);
> +               kfree(ocs);
> +               return NULL;
> +       }
> +
> +       np->data = ocs;
> +       *cset = ocs;
> +
> +       return np;
> +}
> +EXPORT_SYMBOL(of_create_node);
> +
> +/**
> + * of_destroy_node - Destroy a dynamically created device node
> + *
> + * @np: Pointer to dynamically created device node
> + *
> + */
> +void of_destroy_node(struct device_node *np)
> +{
> +       struct of_changeset *ocs;
> +
> +       if (np->data) {
> +               ocs = (struct of_changeset *)np->data;
> +               of_changeset_destroy(ocs);
> +       }
> +       of_node_put(np);

A sequence like this would be broken:

np  = of_create_node()
of_node_get(np)
of_destroy_node(np)

The put here won't free the node because it still has a ref, but we
just freed the changeset. For this to work correctly, we would need
the release function to handle np->data instead. However, all users of
data aren't a changeset.

I'm failing to remember why we're storing the changeset in 'data', but
there doesn't seem to be a reason now so I think that can just be
dropped. Then if you want to free the node, you'd just do an
of_node_put(). (And maybe after the node is attached you do a put too,
because the attach does a get. Not completely sure.)

A unittest for all these functions would be helpful.

Rob
Lizhi Hou March 24, 2023, 2:11 a.m. UTC | #2 
On 3/23/23 15:40, Rob Herring wrote:
> On Thu, Jan 19, 2023 at 9:02 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
>> of_create_node() creates device node dynamically. The parent device node
>> and full name are required for creating the node. It optionally creates
>> an OF changeset and attaches the newly created node to the changeset. The
>> device node pointer and the changeset pointer can be used to add
>> properties to the device node and apply the node to the base tree.
>>
>> of_destroy_node() frees the device node created by of_create_node(). If
>> an OF changeset was also created for this node, it will destroy the
>> changeset before freeing the device node.
>>
>> Expand of_changeset APIs to handle specific types of properties.
>>      of_changeset_add_prop_string()
>>      of_changeset_add_prop_string_array()
>>      of_changeset_add_prop_u32_array()
>>
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> Your Sob should be last because you sent this patch. The order of Sob
> is roughly the order of possession of the patch.
Got it.
>
>> Signed-off-by: Sonal Santan <sonal.santan@amd.com>
>> Signed-off-by: Max Zhen <max.zhen@amd.com>
> So Sonal and Max modified this patch?
They did not directly modify the code. And we discussed the design 
together.  They also reviewed the patch before I sent it out. Please let 
me know if other keyword should be used in this case.
>
>> Reviewed-by: Brian Xu <brian.xu@amd.com>
>> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> Why does this have Clément's Sob?
I referenced Clément 's code and used one portion in my first patch 
series. And I re-implemented it later to address the code review 
comments/requests.
>
>> ---
>>   drivers/of/dynamic.c | 197 +++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/of.h   |  24 ++++++
>>   2 files changed, 221 insertions(+)
>>
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index cd3821a6444f..4e211a1d039f 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -461,6 +461,71 @@ struct device_node *__of_node_dup(const struct device_node *np,
>>          return NULL;
>>   }
>>
>> +/**
>> + * of_create_node - Dynamically create a device node
> For consistency, I think this should be of_changeset_create_node().
Sure.
>
>> + *
>> + * @parent: Pointer to parent device node
>> + * @full_name: Node full name
>> + * @cset: Pointer to returning changeset
>> + *
>> + * Return: Pointer to the created device node or NULL in case of an error.
>> + */
>> +struct device_node *of_create_node(struct device_node *parent,
>> +                                  const char *full_name,
>> +                                  struct of_changeset **cset)
>> +{
>> +       struct of_changeset *ocs;
>> +       struct device_node *np;
>> +       int ret;
>> +
>> +       np = __of_node_dup(NULL, full_name);
>> +       if (!np)
>> +               return NULL;
>> +       np->parent = parent;
>> +
>> +       if (!cset)
>> +               return np;
>> +
>> +       ocs = kmalloc(sizeof(*ocs), GFP_KERNEL);
>> +       if (!ocs) {
>> +               of_node_put(np);
>> +               return NULL;
>> +       }
>> +
>> +       of_changeset_init(ocs);
>> +       ret = of_changeset_attach_node(ocs, np);
>> +       if (ret) {
>> +               of_changeset_destroy(ocs);
>> +               of_node_put(np);
>> +               kfree(ocs);
>> +               return NULL;
>> +       }
>> +
>> +       np->data = ocs;
>> +       *cset = ocs;
>> +
>> +       return np;
>> +}
>> +EXPORT_SYMBOL(of_create_node);
>> +
>> +/**
>> + * of_destroy_node - Destroy a dynamically created device node
>> + *
>> + * @np: Pointer to dynamically created device node
>> + *
>> + */
>> +void of_destroy_node(struct device_node *np)
>> +{
>> +       struct of_changeset *ocs;
>> +
>> +       if (np->data) {
>> +               ocs = (struct of_changeset *)np->data;
>> +               of_changeset_destroy(ocs);
>> +       }
>> +       of_node_put(np);
> A sequence like this would be broken:
>
> np  = of_create_node()
> of_node_get(np)
> of_destroy_node(np)
>
> The put here won't free the node because it still has a ref, but we
> just freed the changeset. For this to work correctly, we would need
> the release function to handle np->data instead. However, all users of
> data aren't a changeset.
>
> I'm failing to remember why we're storing the changeset in 'data', but
> there doesn't seem to be a reason now so I think that can just be
> dropped. Then if you want to free the node, you'd just do an
> of_node_put(). (And maybe after the node is attached you do a put too,
> because the attach does a get. Not completely sure.)

The question is how to save changeset and free it later. I used global 
link list to track the changeset been created.

Storing the changeset in 'data' can avoid using the global link list.

To use of_node_put() to free both node and changeset, I think we can

   1) add a new flag, then in of_node_release() we can know np->data is 
changeset by checking the flag.

   2) When creating node, allocate extra memory for changeset and set 
np->data to a global function of_free_dynamic_node().

       In of_node_release(), check if np->data == of_free_dynamic_node, 
call of_free_dynamic_node(np).

       in of_free_dynamic_node(), free changeset by 
of_changeset_destroy(np+1)

Does this make sense to you? If yes, 1) or 2) sounds better?

>
> A unittest for all these functions would be helpful.

Ok, I will create unittest for the new added functions.


Thanks,

Lizhi

>
> Rob
Rob Herring March 24, 2023, 2:14 p.m. UTC | #3 
On Thu, Mar 23, 2023 at 9:12 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
>
>
> On 3/23/23 15:40, Rob Herring wrote:
> > On Thu, Jan 19, 2023 at 9:02 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
> >> of_create_node() creates device node dynamically. The parent device node
> >> and full name are required for creating the node. It optionally creates
> >> an OF changeset and attaches the newly created node to the changeset. The
> >> device node pointer and the changeset pointer can be used to add
> >> properties to the device node and apply the node to the base tree.
> >>
> >> of_destroy_node() frees the device node created by of_create_node(). If
> >> an OF changeset was also created for this node, it will destroy the
> >> changeset before freeing the device node.
> >>
> >> Expand of_changeset APIs to handle specific types of properties.
> >>      of_changeset_add_prop_string()
> >>      of_changeset_add_prop_string_array()
> >>      of_changeset_add_prop_u32_array()
> >>
> >> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> > Your Sob should be last because you sent this patch. The order of Sob
> > is roughly the order of possession of the patch.
> Got it.
> >
> >> Signed-off-by: Sonal Santan <sonal.santan@amd.com>
> >> Signed-off-by: Max Zhen <max.zhen@amd.com>
> > So Sonal and Max modified this patch?
> They did not directly modify the code. And we discussed the design
> together.  They also reviewed the patch before I sent it out. Please let
> me know if other keyword should be used in this case.

Reviewed-by or nothing. Some feel that only reviews on public lists
should get that tag and internal, private reviews don't matter.

> >
> >> Reviewed-by: Brian Xu <brian.xu@amd.com>
> >> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > Why does this have Clément's Sob?
> I referenced Clément 's code and used one portion in my first patch
> series. And I re-implemented it later to address the code review
> comments/requests.

Then it goes first or you can use the 'Co-developed-by' tag.

> >
> >> ---
> >>   drivers/of/dynamic.c | 197 +++++++++++++++++++++++++++++++++++++++++++
> >>   include/linux/of.h   |  24 ++++++
> >>   2 files changed, 221 insertions(+)
> >>
> >> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> >> index cd3821a6444f..4e211a1d039f 100644
> >> --- a/drivers/of/dynamic.c
> >> +++ b/drivers/of/dynamic.c
> >> @@ -461,6 +461,71 @@ struct device_node *__of_node_dup(const struct device_node *np,
> >>          return NULL;
> >>   }
> >>
> >> +/**
> >> + * of_create_node - Dynamically create a device node
> > For consistency, I think this should be of_changeset_create_node().
> Sure.
> >
> >> + *
> >> + * @parent: Pointer to parent device node
> >> + * @full_name: Node full name
> >> + * @cset: Pointer to returning changeset
> >> + *
> >> + * Return: Pointer to the created device node or NULL in case of an error.
> >> + */
> >> +struct device_node *of_create_node(struct device_node *parent,
> >> +                                  const char *full_name,
> >> +                                  struct of_changeset **cset)
> >> +{
> >> +       struct of_changeset *ocs;
> >> +       struct device_node *np;
> >> +       int ret;
> >> +
> >> +       np = __of_node_dup(NULL, full_name);
> >> +       if (!np)
> >> +               return NULL;
> >> +       np->parent = parent;
> >> +
> >> +       if (!cset)
> >> +               return np;
> >> +
> >> +       ocs = kmalloc(sizeof(*ocs), GFP_KERNEL);
> >> +       if (!ocs) {
> >> +               of_node_put(np);
> >> +               return NULL;
> >> +       }
> >> +
> >> +       of_changeset_init(ocs);
> >> +       ret = of_changeset_attach_node(ocs, np);
> >> +       if (ret) {
> >> +               of_changeset_destroy(ocs);
> >> +               of_node_put(np);
> >> +               kfree(ocs);
> >> +               return NULL;
> >> +       }
> >> +
> >> +       np->data = ocs;
> >> +       *cset = ocs;
> >> +
> >> +       return np;
> >> +}
> >> +EXPORT_SYMBOL(of_create_node);
> >> +
> >> +/**
> >> + * of_destroy_node - Destroy a dynamically created device node
> >> + *
> >> + * @np: Pointer to dynamically created device node
> >> + *
> >> + */
> >> +void of_destroy_node(struct device_node *np)
> >> +{
> >> +       struct of_changeset *ocs;
> >> +
> >> +       if (np->data) {
> >> +               ocs = (struct of_changeset *)np->data;
> >> +               of_changeset_destroy(ocs);
> >> +       }
> >> +       of_node_put(np);
> > A sequence like this would be broken:
> >
> > np  = of_create_node()
> > of_node_get(np)
> > of_destroy_node(np)
> >
> > The put here won't free the node because it still has a ref, but we
> > just freed the changeset. For this to work correctly, we would need
> > the release function to handle np->data instead. However, all users of
> > data aren't a changeset.
> >
> > I'm failing to remember why we're storing the changeset in 'data', but
> > there doesn't seem to be a reason now so I think that can just be
> > dropped. Then if you want to free the node, you'd just do an
> > of_node_put(). (And maybe after the node is attached you do a put too,
> > because the attach does a get. Not completely sure.)
>
> The question is how to save changeset and free it later. I used global
> link list to track the changeset been created.
>
> Storing the changeset in 'data' can avoid using the global link list.
>
> To use of_node_put() to free both node and changeset, I think we can
>
>    1) add a new flag, then in of_node_release() we can know np->data is
> changeset by checking the flag.
>
>    2) When creating node, allocate extra memory for changeset and set
> np->data to a global function of_free_dynamic_node().
>
>        In of_node_release(), check if np->data == of_free_dynamic_node,
> call of_free_dynamic_node(np).
>
>        in of_free_dynamic_node(), free changeset by
> of_changeset_destroy(np+1)
>
> Does this make sense to you? If yes, 1) or 2) sounds better?

Neither works. Changesets and nodes are not 1:1 in general though they
are in your use. So you can use the data ptr, but the caller has to
decide that, not the DT core code.

Rob
Lizhi Hou March 24, 2023, 9:26 p.m. UTC | #4 
On 3/24/23 07:14, Rob Herring wrote:
> On Thu, Mar 23, 2023 at 9:12 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
>>
>> On 3/23/23 15:40, Rob Herring wrote:
>>> On Thu, Jan 19, 2023 at 9:02 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
>>>> of_create_node() creates device node dynamically. The parent device node
>>>> and full name are required for creating the node. It optionally creates
>>>> an OF changeset and attaches the newly created node to the changeset. The
>>>> device node pointer and the changeset pointer can be used to add
>>>> properties to the device node and apply the node to the base tree.
>>>>
>>>> of_destroy_node() frees the device node created by of_create_node(). If
>>>> an OF changeset was also created for this node, it will destroy the
>>>> changeset before freeing the device node.
>>>>
>>>> Expand of_changeset APIs to handle specific types of properties.
>>>>       of_changeset_add_prop_string()
>>>>       of_changeset_add_prop_string_array()
>>>>       of_changeset_add_prop_u32_array()
>>>>
>>>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>>> Your Sob should be last because you sent this patch. The order of Sob
>>> is roughly the order of possession of the patch.
>> Got it.
>>>> Signed-off-by: Sonal Santan <sonal.santan@amd.com>
>>>> Signed-off-by: Max Zhen <max.zhen@amd.com>
>>> So Sonal and Max modified this patch?
>> They did not directly modify the code. And we discussed the design
>> together.  They also reviewed the patch before I sent it out. Please let
>> me know if other keyword should be used in this case.
> Reviewed-by or nothing. Some feel that only reviews on public lists
> should get that tag and internal, private reviews don't matter.
>
>>>> Reviewed-by: Brian Xu <brian.xu@amd.com>
>>>> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
>>> Why does this have Clément's Sob?
>> I referenced Clément 's code and used one portion in my first patch
>> series. And I re-implemented it later to address the code review
>> comments/requests.
> Then it goes first or you can use the 'Co-developed-by' tag.
>
>>>> ---
>>>>    drivers/of/dynamic.c | 197 +++++++++++++++++++++++++++++++++++++++++++
>>>>    include/linux/of.h   |  24 ++++++
>>>>    2 files changed, 221 insertions(+)
>>>>
>>>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>>>> index cd3821a6444f..4e211a1d039f 100644
>>>> --- a/drivers/of/dynamic.c
>>>> +++ b/drivers/of/dynamic.c
>>>> @@ -461,6 +461,71 @@ struct device_node *__of_node_dup(const struct device_node *np,
>>>>           return NULL;
>>>>    }
>>>>
>>>> +/**
>>>> + * of_create_node - Dynamically create a device node
>>> For consistency, I think this should be of_changeset_create_node().
>> Sure.
>>>> + *
>>>> + * @parent: Pointer to parent device node
>>>> + * @full_name: Node full name
>>>> + * @cset: Pointer to returning changeset
>>>> + *
>>>> + * Return: Pointer to the created device node or NULL in case of an error.
>>>> + */
>>>> +struct device_node *of_create_node(struct device_node *parent,
>>>> +                                  const char *full_name,
>>>> +                                  struct of_changeset **cset)
>>>> +{
>>>> +       struct of_changeset *ocs;
>>>> +       struct device_node *np;
>>>> +       int ret;
>>>> +
>>>> +       np = __of_node_dup(NULL, full_name);
>>>> +       if (!np)
>>>> +               return NULL;
>>>> +       np->parent = parent;
>>>> +
>>>> +       if (!cset)
>>>> +               return np;
>>>> +
>>>> +       ocs = kmalloc(sizeof(*ocs), GFP_KERNEL);
>>>> +       if (!ocs) {
>>>> +               of_node_put(np);
>>>> +               return NULL;
>>>> +       }
>>>> +
>>>> +       of_changeset_init(ocs);
>>>> +       ret = of_changeset_attach_node(ocs, np);
>>>> +       if (ret) {
>>>> +               of_changeset_destroy(ocs);
>>>> +               of_node_put(np);
>>>> +               kfree(ocs);
>>>> +               return NULL;
>>>> +       }
>>>> +
>>>> +       np->data = ocs;
>>>> +       *cset = ocs;
>>>> +
>>>> +       return np;
>>>> +}
>>>> +EXPORT_SYMBOL(of_create_node);
>>>> +
>>>> +/**
>>>> + * of_destroy_node - Destroy a dynamically created device node
>>>> + *
>>>> + * @np: Pointer to dynamically created device node
>>>> + *
>>>> + */
>>>> +void of_destroy_node(struct device_node *np)
>>>> +{
>>>> +       struct of_changeset *ocs;
>>>> +
>>>> +       if (np->data) {
>>>> +               ocs = (struct of_changeset *)np->data;
>>>> +               of_changeset_destroy(ocs);
>>>> +       }
>>>> +       of_node_put(np);
>>> A sequence like this would be broken:
>>>
>>> np  = of_create_node()
>>> of_node_get(np)
>>> of_destroy_node(np)
>>>
>>> The put here won't free the node because it still has a ref, but we
>>> just freed the changeset. For this to work correctly, we would need
>>> the release function to handle np->data instead. However, all users of
>>> data aren't a changeset.
>>>
>>> I'm failing to remember why we're storing the changeset in 'data', but
>>> there doesn't seem to be a reason now so I think that can just be
>>> dropped. Then if you want to free the node, you'd just do an
>>> of_node_put(). (And maybe after the node is attached you do a put too,
>>> because the attach does a get. Not completely sure.)
>> The question is how to save changeset and free it later. I used global
>> link list to track the changeset been created.
>>
>> Storing the changeset in 'data' can avoid using the global link list.
>>
>> To use of_node_put() to free both node and changeset, I think we can
>>
>>     1) add a new flag, then in of_node_release() we can know np->data is
>> changeset by checking the flag.
>>
>>     2) When creating node, allocate extra memory for changeset and set
>> np->data to a global function of_free_dynamic_node().
>>
>>         In of_node_release(), check if np->data == of_free_dynamic_node,
>> call of_free_dynamic_node(np).
>>
>>         in of_free_dynamic_node(), free changeset by
>> of_changeset_destroy(np+1)
>>
>> Does this make sense to you? If yes, 1) or 2) sounds better?
> Neither works. Changesets and nodes are not 1:1 in general though they
> are in your use. So you can use the data ptr, but the caller has to
> decide that, not the DT core code.

Ok. In of_pci_make_dev_node(), I can do

      ocs = kmalloc(*ocs);

      of_changeset_init(ocs);

      np = of_changeset_create_node(ocs, name);

      np->data = ocs;

Then in of_pci_remove_node(), I can do

      if (!np || !of_node_check_flag(np, OF_DYNAMIC)) return;

     of_changeset_destroy(np->data);

     of_node_put(np);


Does this sound reasonable?


Thanks,

Lizhi

>
> Rob
Rob Herring April 5, 2023, 1:45 a.m. UTC | #5 
On Fri, Mar 24, 2023 at 4:26 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
>
>
> On 3/24/23 07:14, Rob Herring wrote:
> > On Thu, Mar 23, 2023 at 9:12 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
> >>
> >> On 3/23/23 15:40, Rob Herring wrote:
> >>> On Thu, Jan 19, 2023 at 9:02 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
> >>>> of_create_node() creates device node dynamically. The parent device node
> >>>> and full name are required for creating the node. It optionally creates
> >>>> an OF changeset and attaches the newly created node to the changeset. The
> >>>> device node pointer and the changeset pointer can be used to add
> >>>> properties to the device node and apply the node to the base tree.
> >>>>
> >>>> of_destroy_node() frees the device node created by of_create_node(). If
> >>>> an OF changeset was also created for this node, it will destroy the
> >>>> changeset before freeing the device node.
> >>>>
> >>>> Expand of_changeset APIs to handle specific types of properties.
> >>>>       of_changeset_add_prop_string()
> >>>>       of_changeset_add_prop_string_array()
> >>>>       of_changeset_add_prop_u32_array()
> >>>>
> >>>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> >>> Your Sob should be last because you sent this patch. The order of Sob
> >>> is roughly the order of possession of the patch.
> >> Got it.
> >>>> Signed-off-by: Sonal Santan <sonal.santan@amd.com>
> >>>> Signed-off-by: Max Zhen <max.zhen@amd.com>
> >>> So Sonal and Max modified this patch?
> >> They did not directly modify the code. And we discussed the design
> >> together.  They also reviewed the patch before I sent it out. Please let
> >> me know if other keyword should be used in this case.
> > Reviewed-by or nothing. Some feel that only reviews on public lists
> > should get that tag and internal, private reviews don't matter.
> >
> >>>> Reviewed-by: Brian Xu <brian.xu@amd.com>
> >>>> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> >>> Why does this have Clément's Sob?
> >> I referenced Clément 's code and used one portion in my first patch
> >> series. And I re-implemented it later to address the code review
> >> comments/requests.
> > Then it goes first or you can use the 'Co-developed-by' tag.
> >
> >>>> ---
> >>>>    drivers/of/dynamic.c | 197 +++++++++++++++++++++++++++++++++++++++++++
> >>>>    include/linux/of.h   |  24 ++++++
> >>>>    2 files changed, 221 insertions(+)
> >>>>
> >>>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> >>>> index cd3821a6444f..4e211a1d039f 100644
> >>>> --- a/drivers/of/dynamic.c
> >>>> +++ b/drivers/of/dynamic.c
> >>>> @@ -461,6 +461,71 @@ struct device_node *__of_node_dup(const struct device_node *np,
> >>>>           return NULL;
> >>>>    }
> >>>>
> >>>> +/**
> >>>> + * of_create_node - Dynamically create a device node
> >>> For consistency, I think this should be of_changeset_create_node().
> >> Sure.
> >>>> + *
> >>>> + * @parent: Pointer to parent device node
> >>>> + * @full_name: Node full name
> >>>> + * @cset: Pointer to returning changeset
> >>>> + *
> >>>> + * Return: Pointer to the created device node or NULL in case of an error.
> >>>> + */
> >>>> +struct device_node *of_create_node(struct device_node *parent,
> >>>> +                                  const char *full_name,
> >>>> +                                  struct of_changeset **cset)
> >>>> +{
> >>>> +       struct of_changeset *ocs;
> >>>> +       struct device_node *np;
> >>>> +       int ret;
> >>>> +
> >>>> +       np = __of_node_dup(NULL, full_name);
> >>>> +       if (!np)
> >>>> +               return NULL;
> >>>> +       np->parent = parent;
> >>>> +
> >>>> +       if (!cset)
> >>>> +               return np;
> >>>> +
> >>>> +       ocs = kmalloc(sizeof(*ocs), GFP_KERNEL);
> >>>> +       if (!ocs) {
> >>>> +               of_node_put(np);
> >>>> +               return NULL;
> >>>> +       }
> >>>> +
> >>>> +       of_changeset_init(ocs);
> >>>> +       ret = of_changeset_attach_node(ocs, np);
> >>>> +       if (ret) {
> >>>> +               of_changeset_destroy(ocs);
> >>>> +               of_node_put(np);
> >>>> +               kfree(ocs);
> >>>> +               return NULL;
> >>>> +       }
> >>>> +
> >>>> +       np->data = ocs;
> >>>> +       *cset = ocs;
> >>>> +
> >>>> +       return np;
> >>>> +}
> >>>> +EXPORT_SYMBOL(of_create_node);
> >>>> +
> >>>> +/**
> >>>> + * of_destroy_node - Destroy a dynamically created device node
> >>>> + *
> >>>> + * @np: Pointer to dynamically created device node
> >>>> + *
> >>>> + */
> >>>> +void of_destroy_node(struct device_node *np)
> >>>> +{
> >>>> +       struct of_changeset *ocs;
> >>>> +
> >>>> +       if (np->data) {
> >>>> +               ocs = (struct of_changeset *)np->data;
> >>>> +               of_changeset_destroy(ocs);
> >>>> +       }
> >>>> +       of_node_put(np);
> >>> A sequence like this would be broken:
> >>>
> >>> np  = of_create_node()
> >>> of_node_get(np)
> >>> of_destroy_node(np)
> >>>
> >>> The put here won't free the node because it still has a ref, but we
> >>> just freed the changeset. For this to work correctly, we would need
> >>> the release function to handle np->data instead. However, all users of
> >>> data aren't a changeset.
> >>>
> >>> I'm failing to remember why we're storing the changeset in 'data', but
> >>> there doesn't seem to be a reason now so I think that can just be
> >>> dropped. Then if you want to free the node, you'd just do an
> >>> of_node_put(). (And maybe after the node is attached you do a put too,
> >>> because the attach does a get. Not completely sure.)
> >> The question is how to save changeset and free it later. I used global
> >> link list to track the changeset been created.
> >>
> >> Storing the changeset in 'data' can avoid using the global link list.
> >>
> >> To use of_node_put() to free both node and changeset, I think we can
> >>
> >>     1) add a new flag, then in of_node_release() we can know np->data is
> >> changeset by checking the flag.
> >>
> >>     2) When creating node, allocate extra memory for changeset and set
> >> np->data to a global function of_free_dynamic_node().
> >>
> >>         In of_node_release(), check if np->data == of_free_dynamic_node,
> >> call of_free_dynamic_node(np).
> >>
> >>         in of_free_dynamic_node(), free changeset by
> >> of_changeset_destroy(np+1)
> >>
> >> Does this make sense to you? If yes, 1) or 2) sounds better?
> > Neither works. Changesets and nodes are not 1:1 in general though they
> > are in your use. So you can use the data ptr, but the caller has to
> > decide that, not the DT core code.
>
> Ok. In of_pci_make_dev_node(), I can do
>
>       ocs = kmalloc(*ocs);
>
>       of_changeset_init(ocs);
>
>       np = of_changeset_create_node(ocs, name);
>
>       np->data = ocs;
>
> Then in of_pci_remove_node(), I can do
>
>       if (!np || !of_node_check_flag(np, OF_DYNAMIC)) return;
>
>      of_changeset_destroy(np->data);
>
>      of_node_put(np);
>
>
> Does this sound reasonable?

Yes, I think that should work.

Rob
diff mbox series

Patch

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cd3821a6444f..4e211a1d039f 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -461,6 +461,71 @@  struct device_node *__of_node_dup(const struct device_node *np,
 	return NULL;
 }
 
+/**
+ * of_create_node - Dynamically create a device node
+ *
+ * @parent: Pointer to parent device node
+ * @full_name: Node full name
+ * @cset: Pointer to returning changeset
+ *
+ * Return: Pointer to the created device node or NULL in case of an error.
+ */
+struct device_node *of_create_node(struct device_node *parent,
+				   const char *full_name,
+				   struct of_changeset **cset)
+{
+	struct of_changeset *ocs;
+	struct device_node *np;
+	int ret;
+
+	np = __of_node_dup(NULL, full_name);
+	if (!np)
+		return NULL;
+	np->parent = parent;
+
+	if (!cset)
+		return np;
+
+	ocs = kmalloc(sizeof(*ocs), GFP_KERNEL);
+	if (!ocs) {
+		of_node_put(np);
+		return NULL;
+	}
+
+	of_changeset_init(ocs);
+	ret = of_changeset_attach_node(ocs, np);
+	if (ret) {
+		of_changeset_destroy(ocs);
+		of_node_put(np);
+		kfree(ocs);
+		return NULL;
+	}
+
+	np->data = ocs;
+	*cset = ocs;
+
+	return np;
+}
+EXPORT_SYMBOL(of_create_node);
+
+/**
+ * of_destroy_node - Destroy a dynamically created device node
+ *
+ * @np: Pointer to dynamically created device node
+ *
+ */
+void of_destroy_node(struct device_node *np)
+{
+	struct of_changeset *ocs;
+
+	if (np->data) {
+		ocs = (struct of_changeset *)np->data;
+		of_changeset_destroy(ocs);
+	}
+	of_node_put(np);
+}
+EXPORT_SYMBOL(of_destroy_node);
+
 static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
 {
 	if (ce->action == OF_RECONFIG_ATTACH_NODE &&
@@ -934,3 +999,135 @@  int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_changeset_action);
+
+static int of_changeset_add_prop_helper(struct of_changeset *ocs,
+					struct device_node *np,
+					const struct property *pp)
+{
+	struct property *new_pp;
+	int ret;
+
+	new_pp = __of_prop_dup(pp, GFP_KERNEL);
+	if (!new_pp)
+		return -ENOMEM;
+
+	ret = of_changeset_add_property(ocs, np, new_pp);
+	if (ret) {
+		kfree(new_pp->name);
+		kfree(new_pp->value);
+		kfree(new_pp);
+	}
+
+	return ret;
+}
+
+/**
+ * of_changeset_add_prop_string - Add a string property to a changeset
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @prop_name:	name of the property to be added
+ * @str:	pointer to null terminated string
+ *
+ * Create a string property and add it to a changeset.
+ *
+ * Return: 0 on success, a negative error value in case of an error.
+ */
+int of_changeset_add_prop_string(struct of_changeset *ocs,
+				 struct device_node *np,
+				 const char *prop_name, const char *str)
+{
+	struct property prop;
+
+	prop.name = (char *)prop_name;
+	prop.length = strlen(str) + 1;
+	prop.value = (void *)str;
+
+	return of_changeset_add_prop_helper(ocs, np, &prop);
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_prop_string);
+
+/**
+ * of_changeset_add_prop_string_array - Add a string list property to
+ * a changeset
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @prop_name:	name of the property to be added
+ * @str_array:	pointer to an array of null terminated strings
+ * @sz:		number of string array elements
+ *
+ * Create a string list property and add it to a changeset.
+ *
+ * Return: 0 on success, a negative error value in case of an error.
+ */
+int of_changeset_add_prop_string_array(struct of_changeset *ocs,
+				       struct device_node *np,
+				       const char *prop_name,
+				       const char **str_array, size_t sz)
+{
+	struct property prop;
+	int i, ret;
+	char *vp;
+
+	prop.name = (char *)prop_name;
+
+	prop.length = 0;
+	for (i = 0; i < sz; i++)
+		prop.length += strlen(str_array[i]) + 1;
+
+	prop.value = kmalloc(prop.length, GFP_KERNEL);
+	if (!prop.value)
+		return -ENOMEM;
+
+	vp = prop.value;
+	for (i = 0; i < sz; i++) {
+		vp += snprintf(vp, (char *)prop.value + prop.length - vp, "%s",
+			       str_array[i]) + 1;
+	}
+	ret = of_changeset_add_prop_helper(ocs, np, &prop);
+	kfree(prop.value);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_prop_string_array);
+
+/**
+ * of_changeset_add_prop_u32_array - Add a property of 32 bit integers
+ * property to a changeset
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @prop_name:	name of the property to be added
+ * @array:	pointer to an array of 32 bit integers
+ * @sz:		number of array elements
+ *
+ * Create a property of 32 bit integers and add it to a changeset.
+ *
+ * Return: 0 on success, a negative error value in case of an error.
+ */
+int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
+				    struct device_node *np,
+				    const char *prop_name,
+				    const u32 *array, size_t sz)
+{
+	struct property prop;
+	__be32 *val;
+	int i, ret;
+
+	val = kcalloc(sz, sizeof(__be32), GFP_KERNEL);
+	if (!val)
+		return -ENOMEM;
+
+	for (i = 0; i < sz; i++)
+		val[i] = cpu_to_be32(array[i]);
+	prop.name = (char *)prop_name;
+	prop.length = sizeof(u32) * sz;
+	prop.value = (void *)val;
+
+	ret = of_changeset_add_prop_helper(ocs, np, &prop);
+	kfree(val);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_prop_u32_array);
diff --git a/include/linux/of.h b/include/linux/of.h
index 8b9f94386dc3..ebd276d4c4aa 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1505,6 +1505,30 @@  static inline int of_changeset_update_property(struct of_changeset *ocs,
 {
 	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
 }
+
+struct device_node *of_create_node(struct device_node *parent,
+				   const char *full_name,
+				   struct of_changeset **cset);
+void of_destroy_node(struct device_node *np);
+int of_changeset_add_prop_string(struct of_changeset *ocs,
+				 struct device_node *np,
+				 const char *prop_name, const char *str);
+int of_changeset_add_prop_string_array(struct of_changeset *ocs,
+				       struct device_node *np,
+				       const char *prop_name,
+				       const char **str_array, size_t sz);
+int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
+				    struct device_node *np,
+				    const char *prop_name,
+				    const u32 *array, size_t sz);
+static inline int of_changeset_add_prop_u32(struct of_changeset *ocs,
+					    struct device_node *np,
+					    const char *prop_name,
+					    const u32 val)
+{
+	return of_changeset_add_prop_u32_array(ocs, np, prop_name, &val, 1);
+}
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {