On Thu, 2017-07-27 at 08:36 +0000, Mykola Kostenok wrote: > > -----Original Message----- > > > > From: Andrew Jeffery [mailto:and...@aj.id.au] > > Sent: Thursday, July 27, 2017 9:01 AM > > > > To: Mykola Kostenok <c_myko...@mellanox.com>; Joel Stanley > > > > <j...@jms.id.au> > > > > Cc: linux-hwmon@vger.kernel.org; Jaghathiswari Rankappagounder > > > > > > Natarajan <ja...@google.com>; Jean Delvare <jdelv...@suse.com>; Ohad > > > > > > Oz <oh...@mellanox.com>; Vadim Pasternak <vad...@mellanox.com>; > > > > Patrick Venture <vent...@google.com>; OpenBMC Maillist > > > > > > <open...@lists.ozlabs.org>; Rob Herring <robh...@kernel.org>; > > > > > > Guenter > > > > Roeck <li...@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.
Yep, this is the right approach as far as I can see. Thanks, Andrew > > 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@1e786000/fan@1 > And kernel panic at the end. > > Best regards. Mykola Kostenok.
signature.asc
Description: This is a digitally signed message part