Message ID | 20220615151856.3970186-1-windhl@126.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | drivers: hwmon: Add missing of_node_put() in gsc-hwmon.c | expand |
On 6/15/22 08:18, Liang He wrote: > In gsc_hwmon_get_devtree_pdata(), of_find_compatible_node() will return > a node pointer with refcount incremented. We should use of_node_put() in > fail path or when it is not used anymore. > > Signed-off-by: Liang He <windhl@126.com> > --- Please use proper subject lines. Here it should have been hwmon: (gsc-hwmon) Add missing of_node_put() > drivers/hwmon/gsc-hwmon.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c > index 1fe37418ff46..34c20d13627a 100644 > --- a/drivers/hwmon/gsc-hwmon.c > +++ b/drivers/hwmon/gsc-hwmon.c > @@ -268,10 +268,14 @@ gsc_hwmon_get_devtree_pdata(struct device *dev) > > /* fan controller base address */ > fan = of_find_compatible_node(dev->parent->of_node, NULL, "gw,gsc-fan"); A single of_node_put(fan) here would have been be sufficient. > - if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) { > + if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) { > + of_node_put(fan); > dev_err(dev, "fan node without base\n"); > return ERR_PTR(-EINVAL); > } > + > + /* if fan&&!of_property_read_u32 fail */ This comment only adds confusion and does not add any value. Guenter > + of_node_put(fan); > > /* allocate structures for channels and count instances of each type */ > device_for_each_child_node(dev, child) {
At 2022-06-16 01:57:49, "Guenter Roeck" <linux@roeck-us.net> wrote: > >Please use proper subject lines. Here it should have been > >hwmon: (gsc-hwmon) Add missing of_node_put() Thanks, I will change it in my new patch. >>> drivers/hwmon/gsc-hwmon.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c >> index 1fe37418ff46..34c20d13627a 100644 >> --- a/drivers/hwmon/gsc-hwmon.c >> +++ b/drivers/hwmon/gsc-hwmon.c >> @@ -268,10 +268,14 @@ gsc_hwmon_get_devtree_pdata(struct device *dev) >> >> /* fan controller base address */ >> fan = of_find_compatible_node(dev->parent->of_node, NULL, "gw,gsc-fan"); > >A single of_node_put(fan) here would have been be sufficient. I think of_node_put after should come after its usage, right? >>> - if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) { >> + if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) { >> + of_node_put(fan); >> dev_err(dev, "fan node without base\n"); >> return ERR_PTR(-EINVAL); >> } >> + >> + /* if fan&&!of_property_read_u32 fail */ > >This comment only adds confusion and does not add any value. Sorry, I just want to say, if *fan* is not NULL, but of_property_read_u32() returns 0. In that case, we still need a of_node_put() to release fan, right? > >Guenter > >> + of_node_put(fan); >> >> /* allocate structures for channels and count instances of each type */ >> device_for_each_child_node(dev, child) { Hi, Guenter, I am preparing my new patch and I want to discuss your suggestions as above.
On 6/15/22 19:37, 和亮 wrote: > > > > At 2022-06-16 01:57:49, "Guenter Roeck" <linux@roeck-us.net> wrote: >> >> Please use proper subject lines. Here it should have been >> >> hwmon: (gsc-hwmon) Add missing of_node_put() > > > > Thanks, I will change it in my new patch. > > >>>> drivers/hwmon/gsc-hwmon.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c >>> index 1fe37418ff46..34c20d13627a 100644 >>> --- a/drivers/hwmon/gsc-hwmon.c >>> +++ b/drivers/hwmon/gsc-hwmon.c >>> @@ -268,10 +268,14 @@ gsc_hwmon_get_devtree_pdata(struct device *dev) >>> >>> /* fan controller base address */ >>> fan = of_find_compatible_node(dev->parent->of_node, NULL, "gw,gsc-fan"); >> >> A single of_node_put(fan) here would have been be sufficient. > > > > I think of_node_put after should come after its usage, right? > > Yes, you are correct. Sorry for the noise. >>>> - if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) { >>> + if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) { >>> + of_node_put(fan); >>> dev_err(dev, "fan node without base\n"); >>> return ERR_PTR(-EINVAL); >>> } >>> + >>> + /* if fan&&!of_property_read_u32 fail */ >> > >> This comment only adds confusion and does not add any value. > > > Sorry, I just want to say, if *fan* is not NULL, but of_property_read_u32() returns 0. > In that case, we still need a of_node_put() to release fan, right? > Yes, but that is obvious, and the comment is not needed. Thanks, Guenter >> >> Guenter >> >>> + of_node_put(fan); >>> >>> /* allocate structures for channels and count instances of each type */ > >>> device_for_each_child_node(dev, child) { > > > Hi, Guenter, I am preparing my new patch and I want to discuss your suggestions as above. >
diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c index 1fe37418ff46..34c20d13627a 100644 --- a/drivers/hwmon/gsc-hwmon.c +++ b/drivers/hwmon/gsc-hwmon.c @@ -268,10 +268,14 @@ gsc_hwmon_get_devtree_pdata(struct device *dev) /* fan controller base address */ fan = of_find_compatible_node(dev->parent->of_node, NULL, "gw,gsc-fan"); - if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) { + if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) { + of_node_put(fan); dev_err(dev, "fan node without base\n"); return ERR_PTR(-EINVAL); } + + /* if fan&&!of_property_read_u32 fail */ + of_node_put(fan); /* allocate structures for channels and count instances of each type */ device_for_each_child_node(dev, child) {
In gsc_hwmon_get_devtree_pdata(), of_find_compatible_node() will return a node pointer with refcount incremented. We should use of_node_put() in fail path or when it is not used anymore. Signed-off-by: Liang He <windhl@126.com> --- drivers/hwmon/gsc-hwmon.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)