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

[Mykola Kostenok]

> -----Original Message-----
> From: Andrew Jeffery [mailto:andrew at aj.id.au]
> Sent: Thursday, July 27, 2017 9:01 AM
> To: Mykola Kostenok <c_mykolak at mellanox.com>; Joel Stanley
> <joel at jms.id.au>
> Cc: linux-hwmon at vger.kernel.org; Jaghathiswari Rankappagounder
> Natarajan <jaghu at google.com>; Jean Delvare <jdelvare at suse.com>; Ohad
> Oz <ohado at mellanox.com>; Vadim Pasternak <vadimp at mellanox.com>;
> Patrick Venture <venture at google.com>; OpenBMC Maillist
> <openbmc at lists.ozlabs.org>; Rob Herring <robh+dt at kernel.org>; Guenter
> Roeck <linux at roeck-us.net>
> Subject: Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
> 
> 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

Ok, I agree that we don’t need of_node_put after loop. 
We must do of_node_put only in case of error.

So I will send next patch v6 with this:
           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;
 +               }
          }

Without it and with CONFIG_OF_UNITTEST we see this:
[    3.000000] [<80010080>] (unwind_backtrace) from [<8000d934>] (show_stack+0x20/0x24)
[    3.000000] [<8000d934>] (show_stack) from [<801dbf8c>] (dump_stack+0x20/0x28)
[    3.000000] [<801dbf8c>] (dump_stack) from [<8030ad14>] (of_node_release+0x98/0xa0)
[    3.000000] [<8030ad14>] (of_node_release) from [<801de0ec>] (kobject_put+0xf8/0x1ec)
[    3.000000] [<801de0ec>] (kobject_put) from [<8030a340>] (of_node_put+0x24/0x28)
[    3.000000] [<8030a340>] (of_node_put) from [<80305fe4>] (__of_get_next_child+0x58/0x70)
[    3.000000] [<80305fe4>] (__of_get_next_child) from [<8030668c>] (of_get_next_child+0x20/0x28)
[    3.000000] [<8030668c>] (of_get_next_child) from [<802e39ac>] (aspeed_pwm_tacho_probe+0x490/0x574)
[    3.000000] [<802e39ac>] (aspeed_pwm_tacho_probe) from [<80244090>] (platform_drv_probe+0x60/0xc0)
[    3.000000] [<80244090>] (platform_drv_probe) from [<80242408>] (driver_probe_device+0x280/0x44c)
[    3.000000] [<80242408>] (driver_probe_device) from [<802426c4>] (__driver_attach+0xf0/0x104)
[    3.000000] [<802426c4>] (__driver_attach) from [<802403d8>] (bus_for_each_dev+0x7c/0xb0)
[    3.000000] [<802403d8>] (bus_for_each_dev) from [<8024286c>] (driver_attach+0x28/0x30)
[    3.000000] [<8024286c>] (driver_attach) from [<80240e38>] (bus_add_driver+0x14c/0x268)
[    3.000000] [<80240e38>] (bus_add_driver) from [<802432f8>] (driver_register+0x88/0x104)
[    3.000000] [<802432f8>] (driver_register) from [<80244cd0>] (__platform_driver_register+0x40/0x54)
[    3.000000] [<80244cd0>] (__platform_driver_register) from [<805bfc64>] (aspeed_pwm_tacho_driver_init+0x18/0x20)
[    3.000000] [<805bfc64>] (aspeed_pwm_tacho_driver_init) from [<8059de70>] (do_one_initcall+0xac/0x168)
[    3.000000] [<8059de70>] (do_one_initcall) from [<8059e040>] (kernel_init_freeable+0x114/0x1cc)
[    3.000000] [<8059e040>] (kernel_init_freeable) from [<804072a0>] (kernel_init+0x18/0x104)
[    3.000000] [<804072a0>] (kernel_init) from [<8000a5e8>] (ret_from_fork+0x14/0x2c)
[    3.200000] OF: ERROR: Bad of_node_put() on /ahb/apb/pwm-tacho-controller at 1e786000/fan at 1
And kernel panic at the end.

Best regards. Mykola Kostenok.