The _scoped() version of the fwnode_for_each_available_child_node()
follows the approach recently taken for other loops that handle child
nodes like for_each_child_of_node_scoped() or
device_for_each_child_node_scoped(), which are based on the __free()
auto cleanup handler to remove the need for fwnode_handle_put() on
early loop exits.
This new variant has been tested with the LTC2992, which currently uses
the non-scoped variant. There is one error path that does not decrement
the refcount of the child node, which can be fixed by using the new
macro. The bug was introduced in a later modification of the loop, which
shows how useful an automatic cleanup solution can be in many uses of
the non-scoped version.
In order to provide a backportable patch, the conversion in the LTC2992
driver is carried out in two steps: first the missing
fwnode_handle_put() is added, and then the code is refactored to adopt
the new, safer approach.
@Andy Shevchenko: I kept your Reviewed-by in 3/3, that now also removes
the new fwnode_handle_put() and braces added with 1/3.
Signed-off-by: Javier Carrasco <[email protected]>
---
Changes in v2:
- Fix the memory leak in a backportable patch and tag it for stable.
- Refactor 1/3 with 3/3 as well.
- Link to v1: https://lore.kernel.org/r/20240522-fwnode_for_each_available_child_node_scoped-v1-0-1188b0da12dc@gmail.com
---
Javier Carrasco (3):
hwmon: (ltc2992) Fix memory leak in ltc2992_parse_dt()
device property: introduce fwnode_for_each_available_child_node_scoped()
hwmon: (ltc2992) Use fwnode_for_each_available_child_node_scoped()
drivers/hwmon/ltc2992.c | 11 +++--------
include/linux/property.h | 5 +++++
2 files changed, 8 insertions(+), 8 deletions(-)
---
base-commit: 124cfbcd6d185d4f50be02d5f5afe61578916773
change-id: 20240521-fwnode_for_each_available_child_node_scoped-8f1f09d3a10c
Best regards,
--
Javier Carrasco <[email protected]>
A new error path was added to the fwnode_for_each_available_node() loop
in ltc2992_parse_dt(), which leads to an early return that requires a
call to fwnode_handle_put() to avoid a memory leak in that case.
Add the missing fwnode_handle_put() in the error path from a zero value
shunt resistor.
Cc: [email protected]
Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero")
Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/hwmon/ltc2992.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
index 229aed15d5ca..d4a93223cd3b 100644
--- a/drivers/hwmon/ltc2992.c
+++ b/drivers/hwmon/ltc2992.c
@@ -876,9 +876,11 @@ static int ltc2992_parse_dt(struct ltc2992_state *st)
ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", &val);
if (!ret) {
- if (!val)
+ if (!val) {
+ fwnode_handle_put(child);
return dev_err_probe(&st->client->dev, -EINVAL,
"shunt resistor value cannot be zero\n");
+ }
st->r_sense_uohm[addr] = val;
}
}
--
2.40.1
The scoped version of the fwnode_for_each_available_child_node() macro
automates object recfount decrement, avoiding possible memory leaks
in new error paths inside the loop like it happened when
commit '10b029020487 ("hwmon: (ltc2992) Avoid division by zero")'
was added.
The new macro removes the need to manually call fwnode_handle_put() in
the existing error paths and in any future addition. It also removes the
need for the current child node declaration as well, as it is internally
declared.
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/hwmon/ltc2992.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
index d4a93223cd3b..3feee400ecf8 100644
--- a/drivers/hwmon/ltc2992.c
+++ b/drivers/hwmon/ltc2992.c
@@ -855,32 +855,25 @@ static const struct regmap_config ltc2992_regmap_config = {
static int ltc2992_parse_dt(struct ltc2992_state *st)
{
struct fwnode_handle *fwnode;
- struct fwnode_handle *child;
u32 addr;
u32 val;
int ret;
fwnode = dev_fwnode(&st->client->dev);
- fwnode_for_each_available_child_node(fwnode, child) {
+ fwnode_for_each_available_child_node_scoped(fwnode, child) {
ret = fwnode_property_read_u32(child, "reg", &addr);
- if (ret < 0) {
- fwnode_handle_put(child);
+ if (ret < 0)
return ret;
- }
- if (addr > 1) {
- fwnode_handle_put(child);
+ if (addr > 1)
return -EINVAL;
- }
ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", &val);
if (!ret) {
- if (!val) {
- fwnode_handle_put(child);
+ if (!val)
return dev_err_probe(&st->client->dev, -EINVAL,
"shunt resistor value cannot be zero\n");
- }
st->r_sense_uohm[addr] = val;
}
}
--
2.40.1
Add a scoped version of fwnode_for_each_available_child_node() following
the approach recently taken for other loops that handle child nodes like
for_each_child_of_node_scoped() or device_for_each_child_node_scoped(),
which are based on the __free() auto cleanup handler to remove the need
for fwnode_handle_put() on early loop exits.
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Signed-off-by: Javier Carrasco <[email protected]>
---
include/linux/property.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/property.h b/include/linux/property.h
index 61fc20e5f81f..bcc3dda5a9d8 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
child = fwnode_get_next_available_child_node(fwnode, child))
+#define fwnode_for_each_available_child_node_scoped(fwnode, child) \
+ for (struct fwnode_handle *child __free(fwnode_handle) = \
+ fwnode_get_next_available_child_node(fwnode, NULL); \
+ child; child = fwnode_get_next_available_child_node(fwnode, child))
+
struct fwnode_handle *device_get_next_child_node(const struct device *dev,
struct fwnode_handle *child);
--
2.40.1
On Thu, 23 May 2024 17:47:16 +0200
Javier Carrasco <[email protected]> wrote:
> The scoped version of the fwnode_for_each_available_child_node() macro
> automates object recfount decrement, avoiding possible memory leaks
> in new error paths inside the loop like it happened when
> commit '10b029020487 ("hwmon: (ltc2992) Avoid division by zero")'
> was added.
>
> The new macro removes the need to manually call fwnode_handle_put() in
> the existing error paths and in any future addition. It also removes the
> need for the current child node declaration as well, as it is internally
> declared.
>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Javier Carrasco <[email protected]>
This looks like another instances of the lack of clarify about
what device_for_each_child_node[_scoped]() guarantees about node availability.
On DT it guarantees the node is available as ultimately calls
of_get_next_available_child()
On ACPI it doesn't (I think).
For swnode, there isn't an obvious concept of available.
It would be much better if we reached some agreement on this and
hence could avoid using the fwnode variants just to get the _available_ form
as done here. Or just add the device_for_each_available_child_node[_scoped]()
and call that in almost all cases.
In generic code, do we ever want to walk unavailable child nodes?
Jonathan
> ---
> drivers/hwmon/ltc2992.c | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
> index d4a93223cd3b..3feee400ecf8 100644
> --- a/drivers/hwmon/ltc2992.c
> +++ b/drivers/hwmon/ltc2992.c
> @@ -855,32 +855,25 @@ static const struct regmap_config ltc2992_regmap_config = {
> static int ltc2992_parse_dt(struct ltc2992_state *st)
> {
> struct fwnode_handle *fwnode;
> - struct fwnode_handle *child;
> u32 addr;
> u32 val;
> int ret;
>
> fwnode = dev_fwnode(&st->client->dev);
>
> - fwnode_for_each_available_child_node(fwnode, child) {
> + fwnode_for_each_available_child_node_scoped(fwnode, child) {
> ret = fwnode_property_read_u32(child, "reg", &addr);
> - if (ret < 0) {
> - fwnode_handle_put(child);
> + if (ret < 0)
> return ret;
> - }
>
> - if (addr > 1) {
> - fwnode_handle_put(child);
> + if (addr > 1)
> return -EINVAL;
> - }
>
> ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", &val);
> if (!ret) {
> - if (!val) {
> - fwnode_handle_put(child);
> + if (!val)
> return dev_err_probe(&st->client->dev, -EINVAL,
> "shunt resistor value cannot be zero\n");
> - }
> st->r_sense_uohm[addr] = val;
> }
> }
>
Sun, May 26, 2024 at 02:48:51PM +0100, Jonathan Cameron kirjoitti:
> On Thu, 23 May 2024 17:47:16 +0200
> Javier Carrasco <[email protected]> wrote:
>
> > The scoped version of the fwnode_for_each_available_child_node() macro
> > automates object recfount decrement, avoiding possible memory leaks
> > in new error paths inside the loop like it happened when
> > commit '10b029020487 ("hwmon: (ltc2992) Avoid division by zero")'
> > was added.
> >
> > The new macro removes the need to manually call fwnode_handle_put() in
> > the existing error paths and in any future addition. It also removes the
> > need for the current child node declaration as well, as it is internally
> > declared.
> >
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > Signed-off-by: Javier Carrasco <[email protected]>
>
> This looks like another instances of the lack of clarify about
> what device_for_each_child_node[_scoped]() guarantees about node availability.
> On DT it guarantees the node is available as ultimately calls
> of_get_next_available_child()
>
> On ACPI it doesn't (I think).
> For swnode, there isn't an obvious concept of available.
>
> It would be much better if we reached some agreement on this and
> hence could avoid using the fwnode variants just to get the _available_ form
> as done here.
> Or just add the device_for_each_available_child_node[_scoped]()
> and call that in almost all cases.
device_for_each*() _implies_ availability. You need to talk to Rob about all
this. The design of the device_for_each*() was exactly done in accordance with
his suggestions...
> In generic code, do we ever want to walk unavailable child nodes?
..which are most likely like your question here, i.e. why we ever need to
traverse over unavailable nodes.
--
With Best Regards,
Andy Shevchenko
On Mon, 27 May 2024 17:30:10 +0300
Andy Shevchenko <[email protected]> wrote:
> Sun, May 26, 2024 at 02:48:51PM +0100, Jonathan Cameron kirjoitti:
> > On Thu, 23 May 2024 17:47:16 +0200
> > Javier Carrasco <[email protected]> wrote:
> >
> > > The scoped version of the fwnode_for_each_available_child_node() macro
> > > automates object recfount decrement, avoiding possible memory leaks
> > > in new error paths inside the loop like it happened when
> > > commit '10b029020487 ("hwmon: (ltc2992) Avoid division by zero")'
> > > was added.
> > >
> > > The new macro removes the need to manually call fwnode_handle_put() in
> > > the existing error paths and in any future addition. It also removes the
> > > need for the current child node declaration as well, as it is internally
> > > declared.
> > >
> > > Reviewed-by: Andy Shevchenko <[email protected]>
> > > Signed-off-by: Javier Carrasco <[email protected]>
> >
> > This looks like another instances of the lack of clarify about
> > what device_for_each_child_node[_scoped]() guarantees about node availability.
> > On DT it guarantees the node is available as ultimately calls
> > of_get_next_available_child()
> >
> > On ACPI it doesn't (I think).
> > For swnode, there isn't an obvious concept of available.
> >
> > It would be much better if we reached some agreement on this and
> > hence could avoid using the fwnode variants just to get the _available_ form
> > as done here.
>
> > Or just add the device_for_each_available_child_node[_scoped]()
> > and call that in almost all cases.
>
> device_for_each*() _implies_ availability. You need to talk to Rob about all
> this. The design of the device_for_each*() was exactly done in accordance with
> his suggestions...
>
Does it imply that for ACPI? I can't find a query of _STA in the callbacks
(which is there for the for fwnode_*available calls.
Mind you it wouldn't be the first time I've missed something in the ACPI parsing
code, so maybe it is there indirectly.
I know from previous discussions that the DT version was intentional, but
I'm nervous that the same assumptions don't apply to ACPI.
> > In generic code, do we ever want to walk unavailable child nodes?
>
> ...which are most likely like your question here, i.e. why we ever need to
> traverse over unavailable nodes.
>
Jonathan
On Mon, May 27, 2024 at 03:57:17PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 17:30:10 +0300
> Andy Shevchenko <[email protected]> wrote:
> > Sun, May 26, 2024 at 02:48:51PM +0100, Jonathan Cameron kirjoitti:
> > > On Thu, 23 May 2024 17:47:16 +0200
> > > Javier Carrasco <[email protected]> wrote:
..
> > > This looks like another instances of the lack of clarify about
> > > what device_for_each_child_node[_scoped]() guarantees about node availability.
> > > On DT it guarantees the node is available as ultimately calls
> > > of_get_next_available_child()
> > >
> > > On ACPI it doesn't (I think).
> > > For swnode, there isn't an obvious concept of available.
> > >
> > > It would be much better if we reached some agreement on this and
> > > hence could avoid using the fwnode variants just to get the _available_ form
> > > as done here.
> >
> > > Or just add the device_for_each_available_child_node[_scoped]()
> > > and call that in almost all cases.
> >
> > device_for_each*() _implies_ availability. You need to talk to Rob about all
> > this. The design of the device_for_each*() was exactly done in accordance with
> > his suggestions...
>
> Does it imply that for ACPI? I can't find a query of _STA in the callbacks
> (which is there for the for fwnode_*available calls.
IIRC for ACPI/swnode the availability is always "yes" as long as property can
be found. Basically it means the fwnode_*() == fwnode_*available() for these
back-ends.
AFAIU ACPI concept here is that once parsed and namespaced (in terms of putting
the respective part of description table into ACPI namespace) it's lways
available. Otherwise it's not, but at the same time the respective child node
(property) may not be found
> Mind you it wouldn't be the first time I've missed something in the ACPI parsing
> code, so maybe it is there indirectly.
I might have a weak memory, but see my understanding above.
> I know from previous discussions that the DT version was intentional, but
> I'm nervous that the same assumptions don't apply to ACPI.
>
> > > In generic code, do we ever want to walk unavailable child nodes?
> >
> > ...which are most likely like your question here, i.e. why we ever need to
> > traverse over unavailable nodes.
--
With Best Regards,
Andy Shevchenko
On Thu, May 23, 2024 at 05:47:14PM +0200, Javier Carrasco wrote:
> A new error path was added to the fwnode_for_each_available_node() loop
> in ltc2992_parse_dt(), which leads to an early return that requires a
> call to fwnode_handle_put() to avoid a memory leak in that case.
>
> Add the missing fwnode_handle_put() in the error path from a zero value
> shunt resistor.
>
> Cc: [email protected]
> Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero")
> Signed-off-by: Javier Carrasco <[email protected]>
Applied.
Thanks,
Guenter
> ---
> drivers/hwmon/ltc2992.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
> index 229aed15d5ca..d4a93223cd3b 100644
> --- a/drivers/hwmon/ltc2992.c
> +++ b/drivers/hwmon/ltc2992.c
> @@ -876,9 +876,11 @@ static int ltc2992_parse_dt(struct ltc2992_state *st)
>
> ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", &val);
> if (!ret) {
> - if (!val)
> + if (!val) {
> + fwnode_handle_put(child);
> return dev_err_probe(&st->client->dev, -EINVAL,
> "shunt resistor value cannot be zero\n");
> + }
> st->r_sense_uohm[addr] = val;
> }
> }