| Message ID | 20240523-fwnode_for_each_available_child_node_scoped-v2-3-701f3a03f2fb@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | device property: introduce fwnode_for_each_available_child_node_scoped() | expand |
On Thu, 23 May 2024 17:47:16 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> 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 <andriy.shevchenko@linux.intel.com> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> 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 <javier.carrasco.cruz@gmail.com> 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 <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > 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.
On Mon, 27 May 2024 17:30:10 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > Sun, May 26, 2024 at 02:48:51PM +0100, Jonathan Cameron kirjoitti: > > On Thu, 23 May 2024 17:47:16 +0200 > > Javier Carrasco <javier.carrasco.cruz@gmail.com> 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 <andriy.shevchenko@linux.intel.com> > > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > > > 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 <andy.shevchenko@gmail.com> wrote: > > Sun, May 26, 2024 at 02:48:51PM +0100, Jonathan Cameron kirjoitti: > > > On Thu, 23 May 2024 17:47:16 +0200 > > > Javier Carrasco <javier.carrasco.cruz@gmail.com> 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.
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; } }
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 <andriy.shevchenko@linux.intel.com> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/hwmon/ltc2992.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)