input: automate of_node_put() calls for device_node
Subsystems (6)
cover input: automate of_node_put() calls for device_node
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'.
---
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,
Signature · 1 line
Javier Carrasco <[email protected]>
PATCH 1/10 Input: cap11xx - switch to for_each_child_of_node_scoped
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.
--- drivers/input/keyboard/cap11xx.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
drivers/input/keyboard/cap11xx.c +4 -8
@@ -416,7 +416,7 @@ static int cap11xx_led_set(struct led_classdev *cdev,static int cap11xx_init_leds (struct device *dev,struct cap11xx_priv *priv,int num_leds) { -struct device_node *node = dev->of_node, *child; +struct device_node *node = dev->of_node;struct cap11xx_led *led;int cnt =of_get_child_count (node);int error;
@@ -445,7 +445,7 @@ static int cap11xx_init_leds(struct device *dev,if (error)return error; -for_each_child_of_node (node, child) { +for_each_child_of_node_scoped (node, child) {u32 reg; led->cdev.name =
@@ -458,19 +458,15 @@ static int cap11xx_init_leds(struct device *dev, led->cdev.brightness =LED_OFF ; error =of_property_read_u32 (child,"reg" , ®); -if (error !=0 || reg >= num_leds) { -of_node_put (child); +if (error !=0 || reg >= num_leds)return -EINVAL ; - } led->reg = reg; led->priv = priv; error =devm_led_classdev_register (dev, &led->cdev); -if (error) { -of_node_put (child); +if (error)return error; - } priv->num_leds++; led++;
Signature · 1 line
2.43.0
Re: [PATCH 01/10] Input: cap11xx - switch to for_each_child_of_node_scoped
Quoted 5 lines · Javier Carrasco
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 <[email protected]>
Applied, thank you.
Signature · 1 line
Dmitry
PATCH 2/10 Input: mtk-pmic-keys - switch to for_each_child_of_node_scoped
In reply to Javier CarrascoUse 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.
--- drivers/input/keyboard/mtk-pmic-keys.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
drivers/input/keyboard/mtk-pmic-keys.c +5 -12
@@ -307,7 +307,7 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)int error, index =0 ;unsigned int keycount;struct mt6397_chip *pmic_chip =dev_get_drvdata (pdev->dev.parent); -struct device_node *node = pdev->dev.of_node, *child; +struct device_node *node = pdev->dev.of_node;static const char *const irqnames[] = {"powerkey" ,"homekey" };static const char *const irqnames_r[] = {"powerkey_r" ,"homekey_r" };struct mtk_pmic_keys *keys;
@@ -343,24 +343,20 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)return -EINVAL ; } -for_each_child_of_node (node, child) { +for_each_child_of_node_scoped (node, child) { keys->keys[index].regs = &mtk_pmic_regs->keys_regs[index]; keys->keys[index].irq =platform_get_irq_byname (pdev, irqnames[index]); -if (keys->keys[index].irq <0 ) { -of_node_put (child); +if (keys->keys[index].irq <0 )return keys->keys[index].irq; - }if (of_device_is_compatible (node,"mediatek,mt6358-keys" )) { keys->keys[index].irq_r =platform_get_irq_byname (pdev, irqnames_r[index]); -if (keys->keys[index].irq_r <0 ) { -of_node_put (child); +if (keys->keys[index].irq_r <0 )return keys->keys[index].irq_r; - } } error = of_property_read_u32(child,
@@ -369,7 +365,6 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)dev_err (keys->dev,"failed to read key:%d linux,keycode property: %d\n" , index, error); -of_node_put (child);return error; }
@@ -377,10 +372,8 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev) keys->keys[index].wakeup = true; error =mtk_pmic_key_setup (keys, &keys->keys[index]); -if (error) { -of_node_put (child); +if (error)return error; - } index++; }
Signature · 1 line
2.43.0
Re: [PATCH 02/10] Input: mtk-pmic-keys - switch to for_each_child_of_node_scoped
Quoted 5 lines · Javier Carrasco
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 <[email protected]>
Applied, thank you.
Signature · 1 line
Dmitry
PATCH 3/10 Input: sun4i-lradc-keys - switch to for_each_child_of_node_scoped
In reply to Javier CarrascoUse 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.
--- drivers/input/keyboard/sun4i-lradc-keys.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/input/keyboard/sun4i-lradc-keys.c +2 -5
@@ -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 ; }
Signature · 1 line
2.43.0
Re: [PATCH 03/10] Input: sun4i-lradc-keys - switch to for_each_child_of_node_scoped
On Thu, 10 Oct 2024 23:25:53 +0200
Javier Carrasco <[email protected]> wrote:
Hi,
Quoted 5 lines
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 <[email protected]>
Looks good to me:
Cheers,
Andre
Quoted 48 lines
--- drivers/input/keyboard/sun4i-lradc-keys.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/input/keyboard/sun4i-lradc-keys.c +2
-5@@ -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 ; }
Re: [PATCH 03/10] Input: sun4i-lradc-keys - switch to for_each_child_of_node_scoped
In reply to Javier CarrascoQuoted 33 lines · Javier Carrasco
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 <[email protected]>--- drivers/input/keyboard/sun4i-lradc-keys.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/input/keyboard/sun4i-lradc-keys.c +2
-2@@ -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 ) {
drivers/input/keyboard/sun4i-lradc-keys.c static int sun4i_lradc_load_dt_keymap(struct device *dev, · line 239
Quoted 10 lines
drivers/input/keyboard/sun4i-lradc-keys.c +0
-2dev_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 ;
I wonder if it would not be better to return the real error rather than clobber it with -EINVAL, but I guess this should be a separate patch.
Applied, thank you.
Signature · 1 line
Dmitry
PATCH 4/10 Input: twl6040-vibra - use cleanup facility for device_node
In reply to Javier CarrascoUse the '__free(device_node)' macro to simplify the code and error handling. This makes the error handling more robust by ensuring that the device node is always freed.
--- drivers/input/misc/twl6040-vibra.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/input/misc/twl6040-vibra.c +2 -6
@@ -229,14 +229,13 @@ static DEFINE_SIMPLE_DEV_PM_OPS(twl6040_vibra_pm_ops,static int twl6040_vibra_probe (struct platform_device *pdev) {struct device *twl6040_core_dev = pdev->dev.parent; -struct device_node *twl6040_core_node;struct vibra_info *info;int vddvibl_uV =0 ;int vddvibr_uV =0 ;int error; - twl6040_core_node =of_get_child_by_name (twl6040_core_dev->of_node, -"vibra" ); + struct device_node *twl6040_core_node __free(device_node) = +of_get_child_by_name (twl6040_core_dev->of_node,"vibra" );if (!twl6040_core_node) {dev_err (&pdev->dev,"parent of node is missing?\n" );return -EINVAL ;
@@ -244,7 +243,6 @@ static int twl6040_vibra_probe(struct platform_device *pdev) info =devm_kzalloc (&pdev->dev,sizeof (*info),GFP_KERNEL );if (!info) { -of_node_put (twl6040_core_node);dev_err (&pdev->dev,"couldn't allocate memory\n" );return -ENOMEM ; }
@@ -264,8 +262,6 @@ static int twl6040_vibra_probe(struct platform_device *pdev)of_property_read_u32 (twl6040_core_node,"ti,vddvibl-uV" , &vddvibl_uV);of_property_read_u32 (twl6040_core_node,"ti,vddvibr-uV" , &vddvibr_uV); -of_node_put (twl6040_core_node); -if ((!info->vibldrv_res && !info->viblmotor_res) || (!info->vibrdrv_res && !info->vibrmotor_res)) {dev_err (info->dev,"invalid vibra driver/motor resistance\n" );
Signature · 1 line
2.43.0
Re: [PATCH 04/10] Input: twl6040-vibra - use cleanup facility for device_node
Quoted 5 lines · Javier Carrasco
Use the '
__free(device_node)' macro to simplify the code and error handling. This makes the error handling more robust by ensuring that the device node is always freed.Signed-off-by Javier Carrasco <[email protected]>
Applied, thank you.
Signature · 1 line
Dmitry
PATCH 5/10 Input: twl4030-vibra - use cleanup facility for device_node
In reply to Javier CarrascoUse the '__free(device_node)' macro to simplify the code by automatically freeing the device node.
--- drivers/input/misc/twl4030-vibra.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
drivers/input/misc/twl4030-vibra.c +3 -8
@@ -165,15 +165,10 @@ static DEFINE_SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,static bool twl4030_vibra_check_coexist (struct device_node *parent) { -struct device_node *node; + struct device_node *node __free(device_node) = +of_get_child_by_name (parent,"codec" ); - node =of_get_child_by_name (parent,"codec" ); -if (node) { -of_node_put (node); -return true; - } - -return false; +return node ? true : false; }static int twl4030_vibra_probe (struct platform_device *pdev)
Signature · 1 line
2.43.0
Dmitry Torokhov
maintainerRe: [PATCH 05/10] Input: twl4030-vibra - use cleanup facility for device_node
Quoted 18 lines · Javier Carrasco
Use the '
__free(device_node)' macro to simplify the code by automatically freeing the device node.Signed-off-by Javier Carrasco <[email protected]>--- drivers/input/misc/twl4030-vibra.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)drivers/input/misc/twl4030-vibra.c +1
-1@@ -165,15 +165,10 @@ static DEFINE_SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,static bool twl4030_vibra_check_coexist (struct device_node *parent) { -struct device_node *node; + struct device_node *node __free(device_node) =
drivers/input/misc/twl4030-vibra.c static DEFINE_SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops, · line 171
Quoted 10 lines
drivers/input/misc/twl4030-vibra.c +2
-7+of_get_child_by_name (parent,"codec" ); - node =of_get_child_by_name (parent,"codec" ); -if (node) { -of_node_put (node); -return true; - } - -return false; +return node ? true : false;
This better expressed as
return node != NULL;
I made the adjustment and applied, thank you.
Signature · 1 line
Dmitry
PATCH 6/10 Input: sparcspkr - use cleanup facility for device_node
In reply to Javier CarrascoUse the 'free(device_node)' macro to simplify the code by automatically freeing the device node, which removes the need for explicit calls to 'of_node_put()'.
--- drivers/input/misc/sparcspkr.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/input/misc/sparcspkr.c +1 -3
@@ -188,7 +188,6 @@ static int bbc_beep_probe(struct platform_device *op) {struct sparcspkr_state *state;struct bbc_beep_info *info; -struct device_node *dp;int err = -ENOMEM ; state =kzalloc (sizeof (*state),GFP_KERNEL );
@@ -199,14 +198,13 @@ static int bbc_beep_probe(struct platform_device *op) state->event = bbc_spkr_event;spin_lock_init (&state->lock); - dp =of_find_node_by_path ("/" ); err = -ENODEV ; + struct device_node *dp __free(device_node) = of_find_node_by_path("/");if (!dp) goto out_free; info = &state->u.bbc; info->clock_freq =of_getintprop_default (dp,"clock-frequency" ,0 ); -of_node_put (dp);if (!info->clock_freq) goto out_free;
Signature · 1 line
2.43.0
Re: [PATCH 06/10] Input: sparcspkr - use cleanup facility for device_node
Quoted 14 lines · Javier Carrasco
drivers/input/misc/sparcspkr.c +0
-1@@ -188,7 +188,6 @@ static int bbc_beep_probe(struct platform_device *op) {struct sparcspkr_state *state;struct bbc_beep_info *info; -struct device_node *dp;int err = -ENOMEM ; state =kzalloc (sizeof (*state),GFP_KERNEL );
drivers/input/misc/sparcspkr.c static int bbc_beep_probe(struct platform_device *op) · line 204
Quoted 8 lines
drivers/input/misc/sparcspkr.c +1
-1@@ -199,14 +198,13 @@ static int bbc_beep_probe(struct platform_device *op) state->event = bbc_spkr_event;spin_lock_init (&state->lock); - dp =of_find_node_by_path ("/" ); err = -ENODEV ; + struct device_node *dp __free(device_node) = of_find_node_by_path("/");if (!dp) goto out_free;
Sigh... See that
state = kzalloc(sizeof(*state), GFP_KERNEL); if (!state) goto out_err;
above?
IOW, this will quietly generate broken code if built with gcc (and refuse to compile with clang). Yeah, this one is trivially fixed (return -ENOMEM instead of a goto), but...
__cleanup() can be useful, but it's really *not* safe for blind use; you need to watch out for changed scopes (harmless in case of device_node) and for gotos (broken here).
Re: [PATCH 06/10] Input: sparcspkr - use cleanup facility for device_node
Quoted 30 lines · Al Viro
Quoted 14 lines · Javier Carrasco
drivers/input/misc/sparcspkr.c +0
-1@@ -188,7 +188,6 @@ static int bbc_beep_probe(struct platform_device *op) {struct sparcspkr_state *state;struct bbc_beep_info *info; -struct device_node *dp;int err = -ENOMEM ; state =kzalloc (sizeof (*state),GFP_KERNEL );drivers/input/misc/sparcspkr.c static int bbc_beep_probe(struct platform_device *op) · line 204
Quoted 8 lines
drivers/input/misc/sparcspkr.c +1
-1@@ -199,14 +198,13 @@ static int bbc_beep_probe(struct platform_device *op) state->event = bbc_spkr_event;spin_lock_init (&state->lock); - dp =of_find_node_by_path ("/" ); err = -ENODEV ; + struct device_node *dp __free(device_node) = of_find_node_by_path("/");if (!dp) goto out_free;Sigh... See that
state = kzalloc(sizeof(*state), GFP_KERNEL); if (!state) goto out_err;above?
Quoted 7 lines
IOW, this will quietly generate broken code if built with gcc (and refuse to compile with clang). Yeah, this one is trivially fixed (return
-ENOMEMinstead of a goto), but...
__cleanup()can be useful, but it's really *not* safe for blind use; you need to watch out for changed scopes (harmless in case ofdevice_node) and for gotos (broken here).
Hi Al Viro,
sorry, but I think I don't get you. First, I don't see sparc64 as a supported architecture for clang in the Linux documentation. Maybe the documentation is not up-to-date, but I tried to compile with clang and it seems to be true that it is not supported. Anyway, that is not the issue here.
Second, I might be missing something about the scopes you are mentioning. 'state' gets allocated before the device_node is declared, and when the device_node is declared and its initialization fails, it should jump to 'out_free' to free 'state', shouldn't it? Sorry if I have overlooked something here.
Thank your for your feedback and best regards,
Javier Carrasco
Re: [PATCH 06/10] Input: sparcspkr - use cleanup facility for device_node
Quoted 47 lines · Javier Carrasco
Quoted 30 lines · Al Viro
Quoted 14 lines · Javier Carrasco
drivers/input/misc/sparcspkr.c +0
-1@@ -188,7 +188,6 @@ static int bbc_beep_probe(struct platform_device *op) {struct sparcspkr_state *state;struct bbc_beep_info *info; -struct device_node *dp;int err = -ENOMEM ; state =kzalloc (sizeof (*state),GFP_KERNEL );drivers/input/misc/sparcspkr.c static int bbc_beep_probe(struct platform_device *op) · line 204
Quoted 8 lines
drivers/input/misc/sparcspkr.c +1
-1@@ -199,14 +198,13 @@ static int bbc_beep_probe(struct platform_device *op) state->event = bbc_spkr_event;spin_lock_init (&state->lock); - dp =of_find_node_by_path ("/" ); err = -ENODEV ; + struct device_node *dp __free(device_node) = of_find_node_by_path("/");if (!dp) goto out_free;Sigh... See that
state = kzalloc(sizeof(*state), GFP_KERNEL); if (!state) goto out_err;above?
Quoted 7 lines
IOW, this will quietly generate broken code if built with gcc (and refuse to compile with clang). Yeah, this one is trivially fixed (return
-ENOMEMinstead of a goto), but...
__cleanup()can be useful, but it's really *not* safe for blind use; you need to watch out for changed scopes (harmless in case ofdevice_node) and for gotos (broken here).Hi Al Viro,
sorry, but I think I don't get you. First, I don't see sparc64 as a supported architecture for clang in the Linux documentation. Maybe the documentation is not up-to-date, but I tried to compile with clang and it seems to be true that it is not supported. Anyway, that is not the issue here.
Quoted 9 lines
Second, I might be missing something about the scopes you are mentioning. 'state' gets allocated before the
device_nodeis declared, and when thedevice_nodeis declared and its initialization fails, it should jump to 'out_free' to free 'state', shouldn't it? Sorry if I have overlooked something here.Thank your for your feedback and best regards,
Javier Carrasco
I think that the issue you are talking about is that the goto will trigger the cleanup function of the device_node, which will not be initialized at that point.
Yes, the goto before the device_node declaration hurts, and a return as you said would be better.
Thanks and best regards,
Javier Carrasco
Re: [PATCH 06/10] Input: sparcspkr - use cleanup facility for device_node
Quoted 3 lines · Javier Carrasco
I think that the issue you are talking about is that the goto will trigger the cleanup function of the
device_node, which will not be initialized at that point.
... and gcc will compile that without an error. Clang will not, but you need to watch out for build coverage in arch-specific code - clang doesn't cover every architecture (and won't cover some of them, no matter what - alpha, for example).
As for the scope changes... note that you've delayed the moment of of_node_put() in some of those. It's harmless for device_node, but try something of that sort with e.g.
mutex_lock(&lock); something(); mutex_unlock(&lock); foo(); return 0;
where foo() itself grabs the same lock and it's not harmless at all -
guard(mutex)(&lock); something(); foo(); return 0;
is equivalent to moving mutex_unlock() to the end of scope, i.e. past the call of foo(), resulting in
mutex_lock(&lock); something(); foo(); // deadlock mutex_unlock(&lock); return 0;
__cleanup *is* a useful tool, when used carefully, but you really have to watch out for crap of that sort.
Re: [PATCH 06/10] Input: sparcspkr - use cleanup facility for device_node
Quoted 31 lines · Al Viro
Quoted 3 lines · Javier Carrasco
I think that the issue you are talking about is that the goto will trigger the cleanup function of the
device_node, which will not be initialized at that point.... and gcc will compile that without an error. Clang will not, but you need to watch out for build coverage in arch-specific code - clang doesn't cover every architecture (and won't cover some of them, no matter what - alpha, for example).
As for the scope changes... note that you've delayed the moment of
of_node_put()in some of those. It's harmless fordevice_node, but try something of that sort with e.g.mutex_lock(&lock); something(); mutex_unlock(&lock); foo(); return 0;where
foo()itself grabs the same lock and it's not harmless at all -guard(mutex)(&lock); something(); foo(); return 0;is equivalent to moving
mutex_unlock()to the end of scope, i.e. past the call offoo(), resulting in
Quoted 8 lines
mutex_lock(&lock); something(); foo(); // deadlock mutex_unlock(&lock); return 0;
__cleanup*is* a useful tool, when used carefully, but you really have to watch out for crap of that sort.
For cases like the one you are mentioning, scoped_guard() is the real one to be used, but I get your point.
I just overlooked the goto as it just goes to a return, and I processed in my mind as a direct return, which is not! I have even reviewed such issues in the past... karma.
The goto in that case is meaningless anyway, and a direct return would be more readable anyway.
Thanks again.
PATCH 7/10 Input: 88pm860x - use cleanup facility for device_node
In reply to Javier CarrascoUse the '__free(device_node)' macro to simplify the code and error handling. This makes the code more robust by ensuring that the device node is always freed.
Drop the first assignment to 'pdev->dev.parent->of_node', as it was only a shortcut, and tie 'np' to its usage as a child node.
--- drivers/input/touchscreen/88pm860x-ts.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
drivers/input/touchscreen/88pm860x-ts.c +7 -13
@@ -117,13 +117,14 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,struct pm860x_chip *chip, int *res_x) { - struct device_node *np = pdev->dev.parent->of_node; struct i2c_client *i2c = (chip->id == CHIP_PM8607) ? chip->client \ : chip->companion;int data, n, ret; -if (!np) +if (!pdev->dev.parent->of_node)return -ENODEV ; - np =of_get_child_by_name (np,"touch" ); + + struct device_node *np __free(device_node) = +of_get_child_by_name (pdev->dev.parent->of_node,"touch" );if (!np) {dev_err (&pdev->dev,"Can't find touch node\n" );return -EINVAL ;
@@ -141,13 +142,13 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,if (data) { ret =pm860x_reg_write (i2c,PM8607_GPADC_MISC1 , data);if (ret <0 ) - goto err_put_node; +return -EINVAL ; }/* set tsi prebias time */ if (!of_property_read_u32 (np,"marvell,88pm860x-tsi-prebias" , &data)) { ret =pm860x_reg_write (i2c,PM8607_TSI_PREBIAS , data);if (ret <0 ) - goto err_put_node; +return -EINVAL ; }/* set prebias & prechg time of pen detect */ data =0 ;
@@ -158,18 +159,11 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,if (data) { ret =pm860x_reg_write (i2c,PM8607_PD_PREBIAS , data);if (ret <0 ) - goto err_put_node; +return -EINVAL ; }of_property_read_u32 (np,"marvell,88pm860x-resistor-X" , res_x); -of_node_put (np); - return 0; - -err_put_node: -of_node_put (np); - - return -EINVAL; }#else #define pm860x_touch_dt_init (x, y, z) (-1)
Signature · 1 line
2.43.0
Re: [PATCH 07/10] Input: 88pm860x - use cleanup facility for device_node
Quoted 8 lines · Javier Carrasco
Use the '
__free(device_node)' macro to simplify the code and error handling. This makes the code more robust by ensuring that the device node is always freed.Drop the first assignment to '
pdev->dev.parent->of_node', as it was only a shortcut, and tie 'np' to its usage as a child node.Signed-off-by Javier Carrasco <[email protected]>
Applied, thank you.
Signature · 1 line
Dmitry
PATCH 8/10 Input: i8042 - use cleanup facility for device_node
In reply to Javier CarrascoUse the '__free(device_node)' macro to automatically free the device node, removing the need for explicit calls to 'of_node_put()' to decrement its refcount.
--- drivers/input/serio/i8042-sparcio.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/input/serio/i8042-sparcio.h +1 -5
@@ -106,17 +106,13 @@ static struct platform_driver sparc_i8042_driver = {static bool i8042_is_mr_coffee (void ) { -struct device_node *root; + struct device_node *root __free(device_node) = of_find_node_by_path("/");const char *name;bool is_mr_coffee; - root =of_find_node_by_path ("/" ); - name =of_get_property (root,"name" ,NULL ); is_mr_coffee = name && !strcmp (name,"SUNW,JavaStation-1" ); -of_node_put (root); -return is_mr_coffee; }
Signature · 1 line
2.43.0
Dmitry Torokhov
maintainerRe: [PATCH 08/10] Input: i8042 - use cleanup facility for device_node
Quoted 14 lines · Javier Carrasco
Use the '
__free(device_node)' macro to automatically free the device node, removing the need for explicit calls to 'of_node_put()' to decrement its refcount.Signed-off-by Javier Carrasco <[email protected]>--- drivers/input/serio/i8042-sparcio.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)drivers/input/serio/i8042-sparcio.h +0
-0
drivers/input/serio/i8042-sparcio.h static struct platform_driver sparc_i8042_driver = { · line 111
Quoted 7 lines
drivers/input/serio/i8042-sparcio.h +1
-1@@ -106,17 +106,13 @@ static struct platform_driver sparc_i8042_driver = {static bool i8042_is_mr_coffee (void ) { -struct device_node *root; + struct device_node *root __free(device_node) = of_find_node_by_path("/");const char *name;bool is_mr_coffee;
This temporary is no longer needed. I removed it and applied, thanks.
Quoted 15 lines
drivers/input/serio/i8042-sparcio.h +0
-4- root =of_find_node_by_path ("/" ); - name =of_get_property (root,"name" ,NULL ); is_mr_coffee = name && !strcmp (name,"SUNW,JavaStation-1" ); -of_node_put (root); -return is_mr_coffee; }Signature · 1 line
2.43.0
Signature · 1 line
Dmitry
PATCH 9/10 Input: raspberrypi-ts - use cleanup facility for device_node
In reply to Javier CarrascoUse the '__free(device_node)' macro to automatically free the device node, removing the need for explicit calls to 'of_node_put()' to decrement its refcount.
--- drivers/input/touchscreen/raspberrypi-ts.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/input/touchscreen/raspberrypi-ts.c +1 -3
@@ -122,20 +122,18 @@ static int rpi_ts_probe(struct platform_device *pdev)struct device *dev = &pdev->dev;struct device_node *np = dev->of_node;struct input_dev *input; -struct device_node *fw_node;struct rpi_firmware *fw;struct rpi_ts *ts;u32 touchbuf;int error; - fw_node =of_get_parent (np); + struct device_node *fw_node __free(device_node) = of_get_parent(np);if (!fw_node) {dev_err (dev,"Missing firmware node\n" );return -ENOENT ; } fw =devm_rpi_firmware_get (&pdev->dev, fw_node); -of_node_put (fw_node);if (!fw)return -EPROBE_DEFER ;
Signature · 1 line
2.43.0
Re: [PATCH 09/10] Input: raspberrypi-ts - use cleanup facility for device_node
Quoted 5 lines · Javier Carrasco
Use the '
__free(device_node)' macro to automatically free the device node, removing the need for explicit calls to 'of_node_put()' to decrement its refcount.Signed-off-by Javier Carrasco <[email protected]>
Applied, thank you.
Signature · 1 line
Dmitry
PATCH 10/10 Input: ts4800-ts - use cleanup facility for device_node
In reply to Javier CarrascoUse the '__free(device_node)' macro to automatically free the device node, removing the need for explicit calls to 'of_node_put()' to decrement its refcount.
--- drivers/input/touchscreen/ts4800-ts.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/input/touchscreen/ts4800-ts.c +2 -3
@@ -110,18 +110,17 @@ static int ts4800_parse_dt(struct platform_device *pdev, {struct device *dev = &pdev->dev;struct device_node *np = dev->of_node; -struct device_node *syscon_np;u32 reg, bit;int error; - syscon_np =of_parse_phandle (np,"syscon" ,0 ); + struct device_node *syscon_np __free(device_node) = +of_parse_phandle (np,"syscon" ,0 );if (!syscon_np) {dev_err (dev,"no syscon property\n" );return -ENODEV ; } ts->regmap =syscon_node_to_regmap (syscon_np); -of_node_put (syscon_np);if (IS_ERR (ts->regmap)) {dev_err (dev,"cannot get parent's regmap\n" );return PTR_ERR (ts->regmap);
Signature · 1 line
2.43.0
Re: [PATCH 10/10] Input: ts4800-ts - use cleanup facility for device_node
Quoted 5 lines · Javier Carrasco
Use the '
__free(device_node)' macro to automatically free the device node, removing the need for explicit calls to 'of_node_put()' to decrement its refcount.Signed-off-by Javier Carrasco <[email protected]>
Applied, thank you.
Signature · 1 line
Dmitry