| Message ID | 20241010-input_automate_of_node_put-v1-0-ebc62138fbf8@gmail.com |
|---|---|
| Headers | show |
| Series | input: automate of_node_put() calls for device_node | expand |
On Thu, 10 Oct 2024 23:25:53 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: Hi, > Use the scoped variant of the macro to simplify the code and error > handling. This makes the error handling more robust by ensuring that > the child node is always freed. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> Looks good to me: Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre > --- > drivers/input/keyboard/sun4i-lradc-keys.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c > index f304cab0ebdb..f1e269605f05 100644 > --- a/drivers/input/keyboard/sun4i-lradc-keys.c > +++ b/drivers/input/keyboard/sun4i-lradc-keys.c > @@ -202,7 +202,7 @@ static void sun4i_lradc_close(struct input_dev *dev) > static int sun4i_lradc_load_dt_keymap(struct device *dev, > struct sun4i_lradc_data *lradc) > { > - struct device_node *np, *pp; > + struct device_node *np; > int i; > int error; > > @@ -223,28 +223,25 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev, > return -ENOMEM; > > i = 0; > - for_each_child_of_node(np, pp) { > + for_each_child_of_node_scoped(np, pp) { > struct sun4i_lradc_keymap *map = &lradc->chan0_map[i]; > u32 channel; > > error = of_property_read_u32(pp, "channel", &channel); > if (error || channel != 0) { > dev_err(dev, "%pOFn: Inval channel prop\n", pp); > - of_node_put(pp); > return -EINVAL; > } > > error = of_property_read_u32(pp, "voltage", &map->voltage); > if (error) { > dev_err(dev, "%pOFn: Inval voltage prop\n", pp); > - of_node_put(pp); > return -EINVAL; > } > > error = of_property_read_u32(pp, "linux,code", &map->keycode); > if (error) { > dev_err(dev, "%pOFn: Inval linux,code prop\n", pp); > - of_node_put(pp); > return -EINVAL; > } > >
This series removes the explicit calls to 'of_node_put()' from the input subsystem, either by switching from 'for_each_child_of_node()' to its scoped variant 'for_each_child_of_node_scoped()', or by adding the cleanup attribute to the device_node by means of the '__free()' macro. This series simplifies the code in some cases, and it makes it in general more robust, as it will avoid memory leaks if early returns are added without the required call to 'of_node_put()', which is a rather common issue. The following drivers unconditionally release the device node after using it: - misc/twl4030-vibra.c ('of_node_put()' under an if, but only if the node received a valid value). - misc/sparcspkr.c - serio/i8042-sparcio.h - touchscreen/raspberrypi-ts.c - touchscreen/ts4800-ts.c The usage of the cleanup faciliy for these drivers offers no real gain at the moment, but as soon as an error path is added to them, things can go wrong, as it has happened multiple times with such nodes. I intended to remove this error-prone pattern from the subsystem, so it is not "borrowed" by new users. But if someone has strong feelings about the automatic cleanup for those drives, I will not complain if they are left as they are (at least until a new buggy error path is introduced ;)). The approach for the variable declaration is the one that has been followed in previous clean ups: as near as possible to its usage, instead of at the top. I have no strong feelings about that either, but I would prefer it that way for consistency and to have a common pattern for future additions. A single call to 'of_node_put()' has been left behind in rmi4/rmi_bus.c, as it is used to release a node passed as a parameter, which would make the use of the cleanup attribute too cumbersome for no real gain. It is called unconditionally, and it will probably not be used as a common pattern for new users of a device_node. There has been some previous work from Dmitry to use the cleanup facilities for 'fwnode_handle' and mutexes[1][2], which this series aims to complement for 'device_node'. Link: https://lore.kernel.org/linux-input/20240904044244.1042174-1-dmitry.torokhov@gmail.com/ [1] Link: https://lore.kernel.org/linux-input/20240825051627.2848495-1-dmitry.torokhov@gmail.com/ [2] Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- Javier Carrasco (10): Input: cap11xx - switch to for_each_child_of_node_scoped Input: mtk-pmic-keys - switch to for_each_child_of_node_scoped Input: sun4i-lradc-keys - switch to for_each_child_of_node_scoped Input: twl6040-vibra - use cleanup facility for device_node Input: twl4030-vibra - use cleanup facility for device_node Input: sparcspkr - use cleanup facility for device_node Input: 88pm860x - use cleanup facility for device_node Input: i8042 - use cleanup facility for device_node Input: raspberrypi-ts - use cleanup facility for device_node Input: ts4800-ts - use cleanup facility for device_node drivers/input/keyboard/cap11xx.c | 12 ++++-------- drivers/input/keyboard/mtk-pmic-keys.c | 17 +++++------------ drivers/input/keyboard/sun4i-lradc-keys.c | 7 ++----- drivers/input/misc/sparcspkr.c | 4 +--- drivers/input/misc/twl4030-vibra.c | 11 +++-------- drivers/input/misc/twl6040-vibra.c | 8 ++------ drivers/input/serio/i8042-sparcio.h | 6 +----- drivers/input/touchscreen/88pm860x-ts.c | 20 +++++++------------- drivers/input/touchscreen/raspberrypi-ts.c | 4 +--- drivers/input/touchscreen/ts4800-ts.c | 5 ++--- 10 files changed, 28 insertions(+), 66 deletions(-) --- base-commit: 515ef92b4939fa51f9f1ee278618e2d419b0b8b0 change-id: 20241009-input_automate_of_node_put-1bae9f5c02d9 Best regards,