Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2791 bytes --]

Hi Mykola,

I know you've sent out subsequent versions, but I wanted to address one
of your arguments here:

On Wed, 2017-07-26 at 11:08 +0000, Mykola Kostenok wrote:
> > > @@ -834,10 +946,12 @@ static int aspeed_pwm_tacho_probe(struct
> > > platform_device *pdev)
> > > 
> > >          for_each_child_of_node(np, child) {
> > >                  ret = aspeed_create_fan(dev, child, priv);
> > > -               of_node_put(child);
> > > -               if (ret)
> > > +               if (ret) {
> > > +                       of_node_put(child);
> > >                          return ret;
> > > +               }
> > >          }
> > > +       of_node_put(child);
> > 
> > I'm not sure about these.
> > 
> > Cheers,
> > 
> > Joel
> 
> If CONFIG_OF_UNITTEST is set, system initialization  fails on this of_node_put.
> I checked and found that for_each_child_of_node is macro witch use __of_get_next_child
>  in cycle. __of_get_next_child do of_node_put previous child but not last.
> 
> 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;
> }
> #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))
> 
> So inside cycle we should not use of_node_put on each child. We must use it only on last child in cycle.

I was just looking at this myself for a different driver, and I don't
think this argument is right. The natural terminating condition of the
loop is child == NULL. child == NULL occurs if we have zero-or-more-
children; the child is always initialised as part of the loop header
and would be NULL if there are no children. If we have more than one
child, the reference to the last valid child is passed to of_node_put()
in __of_get_next_child() in order to return the terminating NULL. Given
__of_get_next_child() is passed the last node and the result is NULL,
the call to of_node_put() after the loop is always invoked on NULL,
which performs an early return.

As such I think it is unnecessary.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]