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

[Hans de Goede <hdegoede@redhat.com>]

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