[PATCH] mmc: core: Cleanup unused OF nodes while parsing for child nodes

[PATCH] mmc: core: Cleanup unused OF nodes while parsing for child nodes

[Ulf Hansson]

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c296bc0..e6b0bdb 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1291,6 +1291,8 @@ struct device_node *mmc_of_find_child_device(struct mmc_host *host,
 	for_each_child_of_node(host->parent->of_node, node) {
 		if (mmc_of_get_func_num(node) == func_num)
 			return node;
+		else
+			of_node_put(node);
 	}
 
 	return NULL;
-- 
1.9.1

Re: [PATCH] mmc: core: Cleanup unused OF nodes while parsing for child nodes

[Hans de Goede]

Hi,

On 30-03-15 11:19, Ulf Hansson wrote:
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>   drivers/mmc/core/core.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c296bc0..e6b0bdb 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1291,6 +1291,8 @@ struct device_node *mmc_of_find_child_device(struct mmc_host *host,
>   	for_each_child_of_node(host->parent->of_node, node) {
>   		if (mmc_of_get_func_num(node) == func_num)
>   			return node;
> +		else
> +			of_node_put(node);
>   	}
>
>   	return NULL;
>

I don't think this is right, non of the other users of for_each_child_of_node
do this, I think that rather then doing this we should be changing the callers
of mmc_of_find_child_device to do: of_node_get(), except for the call which
my "mmc: Add support for marking hpi as broken through devicetree" patch adds,
as that is intended to only take a temporary reference.

Regards,

Hans

Re: [PATCH] mmc: core: Cleanup unused OF nodes while parsing for child nodes

[Ulf Hansson]

On 30 March 2015 at 15:37, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 30-03-15 11:19, Ulf Hansson wrote:
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>   drivers/mmc/core/core.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index c296bc0..e6b0bdb 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1291,6 +1291,8 @@ struct device_node *mmc_of_find_child_device(struct
>> mmc_host *host,
>>         for_each_child_of_node(host->parent->of_node, node) {
>>                 if (mmc_of_get_func_num(node) == func_num)
>>                         return node;
>> +               else
>> +                       of_node_put(node);
>>         }
>>
>>         return NULL;
>>
>
> I don't think this is right, non of the other users of
> for_each_child_of_node
> do this, I think that rather then doing this we should be changing the
> callers

So, everybody don't follow the API. Cool. :-)

> of mmc_of_find_child_device to do: of_node_get(), except for the call which
> my "mmc: Add support for marking hpi as broken through devicetree" patch
> adds,
> as that is intended to only take a temporary reference.
>

In principle you are saying that the implementation of
for_each_child_of_node() API needs to be adopted for how users
actually use it, which means leave the of_node_get|put() to be done
entirely by the caller, right?

Kind regards
Uffe

Re: [PATCH] mmc: core: Cleanup unused OF nodes while parsing for child nodes

[Hans de Goede]

Hi,

On 30-03-15 17:09, Ulf Hansson wrote:
> On 30 March 2015 at 15:37, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 30-03-15 11:19, Ulf Hansson wrote:
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>    drivers/mmc/core/core.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index c296bc0..e6b0bdb 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1291,6 +1291,8 @@ struct device_node *mmc_of_find_child_device(struct
>>> mmc_host *host,
>>>          for_each_child_of_node(host->parent->of_node, node) {
>>>                  if (mmc_of_get_func_num(node) == func_num)
>>>                          return node;
>>> +               else
>>> +                       of_node_put(node);
>>>          }
>>>
>>>          return NULL;
>>>
>>
>> I don't think this is right, non of the other users of
>> for_each_child_of_node
>> do this, I think that rather then doing this we should be changing the
>> callers
>
> So, everybody don't follow the API. Cool. :-)

Hmm, I don't know where (if anywhere) the API is specified, but if I look at
the actual implementation in include/linux/of.h

#define for_each_child_of_node(parent, child) \
         for (child = of_get_next_child(parent, NULL); child != NULL; \
              child = of_get_next_child(parent, child))

And in drivers/of/base.c:

static struct device_node *__of_get_next_child(const struct device_node *node,
                                                 struct device_node *prev)
{
         struct device_node *next;

         if (!node)
                 return NULL;

         next = prev ? prev->sibling : node->child;
         for (; next; next = next->sibling)
                 if (of_node_get(next))
                         break;
         of_node_put(prev);
         return next;
}

(the non __ prefixed version takes a lock then calls into this one)

Note the "of_node_put(prev);" in the of_get_next_child implementation,
so yes we've a ref while going through the loop, but its gets freed
on the "increment" part of the for.

Also see e.g. include/linux/of.h :

static inline int of_get_child_count(const struct device_node *np)
{
         struct device_node *child;
         int num = 0;

         for_each_child_of_node(np, child)
                 num++;

         return num;
}

Which I would expect to get things right.

>> of mmc_of_find_child_device to do: of_node_get(), except for the call which
>> my "mmc: Add support for marking hpi as broken through devicetree" patch
>> adds,
>> as that is intended to only take a temporary reference.
>>
>
> In principle you are saying that the implementation of
> for_each_child_of_node() API needs to be adopted for how users
> actually use it, which means leave the of_node_get|put() to be done
> entirely by the caller, right?

What I'm saying is that, if I'm not reading the code the wrong way, that
is already how the for_each_child_of_node() API works.

As for the mmc subsys it seems that means that no changes are necessary,
since we do:

         for_each_child_of_node(host->parent->of_node, node) {
                 if (mmc_of_get_func_num(node) == func_num)
                         return node;
         }

So when we've found the right node, we jump out of the loop, returning
the reference we have while in the loop.

This does mean that my: "mmc: Add support for marking hpi as broken through devicetree"
patch needs to be changed as I ended up calling mmc_of_find_child_device twice
in there, since in that patch I need the of_node before mmc_add_card() gets called,
so I'm leaking a reference there. I'll do a v2 fixing this.

Regards,

Hans

Re: [PATCH] mmc: core: Cleanup unused OF nodes while parsing for child nodes

[Ulf Hansson]

On 31 March 2015 at 09:06, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 30-03-15 17:09, Ulf Hansson wrote:
>>
>> On 30 March 2015 at 15:37, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 30-03-15 11:19, Ulf Hansson wrote:
>>>>
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> ---
>>>>    drivers/mmc/core/core.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index c296bc0..e6b0bdb 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -1291,6 +1291,8 @@ struct device_node
>>>> *mmc_of_find_child_device(struct
>>>> mmc_host *host,
>>>>          for_each_child_of_node(host->parent->of_node, node) {
>>>>                  if (mmc_of_get_func_num(node) == func_num)
>>>>                          return node;
>>>> +               else
>>>> +                       of_node_put(node);
>>>>          }
>>>>
>>>>          return NULL;
>>>>
>>>
>>> I don't think this is right, non of the other users of
>>> for_each_child_of_node
>>> do this, I think that rather then doing this we should be changing the
>>> callers
>>
>>
>> So, everybody don't follow the API. Cool. :-)
>
>
> Hmm, I don't know where (if anywhere) the API is specified, but if I look at
> the actual implementation in include/linux/of.h
>
> #define for_each_child_of_node(parent, child) \
>         for (child = of_get_next_child(parent, NULL); child != NULL; \
>              child = of_get_next_child(parent, child))
>
> And in drivers/of/base.c:
>
> static struct device_node *__of_get_next_child(const struct device_node
> *node,
>                                                 struct device_node *prev)
> {
>         struct device_node *next;
>
>         if (!node)
>                 return NULL;
>
>         next = prev ? prev->sibling : node->child;
>         for (; next; next = next->sibling)
>                 if (of_node_get(next))
>                         break;
>         of_node_put(prev);
>         return next;
> }
>
> (the non __ prefixed version takes a lock then calls into this one)
>
> Note the "of_node_put(prev);" in the of_get_next_child implementation,
> so yes we've a ref while going through the loop, but its gets freed
> on the "increment" part of the for.
>
> Also see e.g. include/linux/of.h :
>
> static inline int of_get_child_count(const struct device_node *np)
> {
>         struct device_node *child;
>         int num = 0;
>
>         for_each_child_of_node(np, child)
>                 num++;
>
>         return num;
> }
>
> Which I would expect to get things right.

Agree. I didn't look at the code carefully enough. My patch is wrong!

>
>>> of mmc_of_find_child_device to do: of_node_get(), except for the call
>>> which
>>> my "mmc: Add support for marking hpi as broken through devicetree" patch
>>> adds,
>>> as that is intended to only take a temporary reference.
>>>
>>
>> In principle you are saying that the implementation of
>> for_each_child_of_node() API needs to be adopted for how users
>> actually use it, which means leave the of_node_get|put() to be done
>> entirely by the caller, right?
>
>
> What I'm saying is that, if I'm not reading the code the wrong way, that
> is already how the for_each_child_of_node() API works.
>
> As for the mmc subsys it seems that means that no changes are necessary,
> since we do:
>
>         for_each_child_of_node(host->parent->of_node, node) {
>                 if (mmc_of_get_func_num(node) == func_num)
>                         return node;
>         }
>
> So when we've found the right node, we jump out of the loop, returning
> the reference we have while in the loop.
>
> This does mean that my: "mmc: Add support for marking hpi as broken through
> devicetree"
> patch needs to be changed as I ended up calling mmc_of_find_child_device
> twice
> in there, since in that patch I need the of_node before mmc_add_card() gets
> called,
> so I'm leaking a reference there. I'll do a v2 fixing this.

So this discussion around $subject patch, turned out to have some
valuable outcome anyway. :-)

Kind regards
Uffe