| Message ID | 20240522-fwnode_for_each_available_child_node_scoped-v1-2-1188b0da12dc@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | device property: introduce fwnode_for_each_available_child_node_scoped() | expand |
On Wed, May 22, 2024 at 11:18:08AM +0200, Javier Carrasco wrote: > The error path from a zero value of the "shunt-resistor-micro-ohms" > property does not decrement the refcount of the child node. > > Instead of adding the missing fwnode_handle_put(), a safer fix for > future modifications is using the _scoped version of the macro, > which removes the need for fwnode_handle_put() in all error paths. Not sure if the fix for easier backporting is better, up to Guenter and Greg. If the above is the case, you will need 1) backportable fix; 2) the patch 1 as of this series; 3) conversion patch. > The macro defines the child node internally, which removes the need for > the current child node declaration as well. FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> in case it will go the current way.
On 5/22/24 02:18, Javier Carrasco wrote: > The error path from a zero value of the "shunt-resistor-micro-ohms" > property does not decrement the refcount of the child node. > > Instead of adding the missing fwnode_handle_put(), a safer fix for > future modifications is using the _scoped version of the macro, > which removes the need for fwnode_handle_put() in all error paths. > > The macro defines the child node internally, which removes the need for > the current child node declaration as well. > > Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> I really don't like fixes which depend on an API change. Guenter > --- > drivers/hwmon/ltc2992.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c > index 229aed15d5ca..3feee400ecf8 100644 > --- a/drivers/hwmon/ltc2992.c > +++ b/drivers/hwmon/ltc2992.c > @@ -855,24 +855,19 @@ 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) { >
On 22/05/2024 21:08, Guenter Roeck wrote: > On 5/22/24 02:18, Javier Carrasco wrote: >> The error path from a zero value of the "shunt-resistor-micro-ohms" >> property does not decrement the refcount of the child node. >> >> Instead of adding the missing fwnode_handle_put(), a safer fix for >> future modifications is using the _scoped version of the macro, >> which removes the need for fwnode_handle_put() in all error paths. >> >> The macro defines the child node internally, which removes the need for >> the current child node declaration as well. >> >> Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero") >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > I really don't like fixes which depend on an API change. > > Guenter > We could split the fix into two steps as suggested by Andy, where the fix would simply add the missing fwnode_handle_put(), adding the corresponding tag to include it in the stable kernels (the bug was introduced with v6.7). By doing that, the new macro would not need to be backported, which is probably a safer approach. The patch with the new macro would be left as it is, and another patch would convert the the loop to use the new macro to avoid that a similar bug arises again. That would be this 2/2 + dropping the additional fwnode_handle_put(). Best regards, Javier Carrasco
On 5/22/24 12:46, Javier Carrasco wrote: > On 22/05/2024 21:08, Guenter Roeck wrote: >> On 5/22/24 02:18, Javier Carrasco wrote: >>> The error path from a zero value of the "shunt-resistor-micro-ohms" >>> property does not decrement the refcount of the child node. >>> >>> Instead of adding the missing fwnode_handle_put(), a safer fix for >>> future modifications is using the _scoped version of the macro, >>> which removes the need for fwnode_handle_put() in all error paths. >>> >>> The macro defines the child node internally, which removes the need for >>> the current child node declaration as well. >>> >>> Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero") >>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >> >> I really don't like fixes which depend on an API change. >> >> Guenter >> > > We could split the fix into two steps as suggested by Andy, where the > fix would simply add the missing fwnode_handle_put(), adding the > corresponding tag to include it in the stable kernels (the bug was > introduced with v6.7). By doing that, the new macro would not need to be > backported, which is probably a safer approach. > > The patch with the new macro would be left as it is, and another patch > would convert the the loop to use the new macro to avoid that a similar > bug arises again. That would be this 2/2 + dropping the additional > fwnode_handle_put(). > Please do. Thanks, Guenter > Best regards, > Javier Carrasco >
diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c index 229aed15d5ca..3feee400ecf8 100644 --- a/drivers/hwmon/ltc2992.c +++ b/drivers/hwmon/ltc2992.c @@ -855,24 +855,19 @@ 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) {
The error path from a zero value of the "shunt-resistor-micro-ohms" property does not decrement the refcount of the child node. Instead of adding the missing fwnode_handle_put(), a safer fix for future modifications is using the _scoped version of the macro, which removes the need for fwnode_handle_put() in all error paths. The macro defines the child node internally, which removes the need for the current child node declaration as well. Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero") Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/hwmon/ltc2992.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)