[PATCH 00/10] input: automate of_node_put() calls for device_node

[PATCH 00/10] input: automate of_node_put() calls for device_node

[Javier Carrasco]

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,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 01/10] Input: cap11xx - switch to for_each_child_of_node_scoped

[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 <javier.carrasco.cruz@gmail.com>
---
 drivers/input/keyboard/cap11xx.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
index b21ef9d6ff9d..0c17cbaa3d27 100644
--- a/drivers/input/keyboard/cap11xx.c
+++ b/drivers/input/keyboard/cap11xx.c
@@ -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", &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++;

-- 
2.43.0

Re: [PATCH 01/10] Input: cap11xx - switch to for_each_child_of_node_scoped

[Dmitry Torokhov]

On Thu, Oct 10, 2024 at 11:25:51PM +0200, Javier Carrasco wrote:
> 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>

Applied, thank you.

-- 
Dmitry

[PATCH 02/10] Input: mtk-pmic-keys - switch to for_each_child_of_node_scoped

[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 <javier.carrasco.cruz@gmail.com>
---
 drivers/input/keyboard/mtk-pmic-keys.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
index 4364c3401ff1..5ad6be914160 100644
--- a/drivers/input/keyboard/mtk-pmic-keys.c
+++ b/drivers/input/keyboard/mtk-pmic-keys.c
@@ -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++;
 	}

-- 
2.43.0

Re: [PATCH 02/10] Input: mtk-pmic-keys - switch to for_each_child_of_node_scoped

[Dmitry Torokhov]

On Thu, Oct 10, 2024 at 11:25:52PM +0200, Javier Carrasco wrote:
> 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>

Applied, thank you.

-- 
Dmitry

[PATCH 03/10] Input: sun4i-lradc-keys - switch to for_each_child_of_node_scoped

[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 <javier.carrasco.cruz@gmail.com>
---
 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;
 		}
 

-- 
2.43.0

Re: [PATCH 03/10] Input: sun4i-lradc-keys - switch to for_each_child_of_node_scoped

[Andre Przywara]

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;
>  		}
>  
> 

Re: [PATCH 03/10] Input: sun4i-lradc-keys - switch to for_each_child_of_node_scoped

[Dmitry Torokhov]

On Thu, Oct 10, 2024 at 11:25:53PM +0200, Javier Carrasco wrote:
> 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>
> ---
>  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;

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.

-- 
Dmitry

[PATCH 04/10] Input: twl6040-vibra - use cleanup facility for device_node

[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 <javier.carrasco.cruz@gmail.com>
---
 drivers/input/misc/twl6040-vibra.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c
index 78f0b63e5c20..afed9af65bf9 100644
--- a/drivers/input/misc/twl6040-vibra.c
+++ b/drivers/input/misc/twl6040-vibra.c
@@ -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");

-- 
2.43.0

Re: [PATCH 04/10] Input: twl6040-vibra - use cleanup facility for device_node

[Dmitry Torokhov]

On Thu, Oct 10, 2024 at 11:25:54PM +0200, Javier Carrasco wrote:
> 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 <javier.carrasco.cruz@gmail.com>

Applied, thank you.

-- 
Dmitry

[PATCH 05/10] Input: twl4030-vibra - use cleanup facility for device_node

[Javier Carrasco]

Use the '__free(device_node)' macro to simplify the code by
automatically freeing the device node.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/input/misc/twl4030-vibra.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
index 101548b35ee3..b8ec5c7c5bce 100644
--- a/drivers/input/misc/twl4030-vibra.c
+++ b/drivers/input/misc/twl4030-vibra.c
@@ -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)

-- 
2.43.0

Re: [PATCH 05/10] Input: twl4030-vibra - use cleanup facility for device_node

[Dmitry Torokhov]

On Thu, Oct 10, 2024 at 11:25:55PM +0200, Javier Carrasco wrote:
> Use the '__free(device_node)' macro to simplify the code by
> automatically freeing the device node.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/input/misc/twl4030-vibra.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
> index 101548b35ee3..b8ec5c7c5bce 100644
> --- a/drivers/input/misc/twl4030-vibra.c
> +++ b/drivers/input/misc/twl4030-vibra.c
> @@ -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;

This better expressed as

	return node != NULL;

I made the adjustment and applied, thank you.

-- 
Dmitry

[PATCH 06/10] Input: sparcspkr - use cleanup facility for device_node

[Javier Carrasco]

Use 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()'.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/input/misc/sparcspkr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/input/misc/sparcspkr.c b/drivers/input/misc/sparcspkr.c
index 20020cbc0752..bb1c732c8f95 100644
--- a/drivers/input/misc/sparcspkr.c
+++ b/drivers/input/misc/sparcspkr.c
@@ -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;
 

-- 
2.43.0

Re: [PATCH 06/10] Input: sparcspkr - use cleanup facility for device_node

[Al Viro]

On Thu, Oct 10, 2024 at 11:25:56PM +0200, Javier Carrasco wrote:
> 
> diff --git a/drivers/input/misc/sparcspkr.c b/drivers/input/misc/sparcspkr.c
> index 20020cbc0752..bb1c732c8f95 100644
> --- a/drivers/input/misc/sparcspkr.c
> +++ b/drivers/input/misc/sparcspkr.c
> @@ -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;

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

[Javier Carrasco]

On 10/10/2024 23:43, Al Viro wrote:
> On Thu, Oct 10, 2024 at 11:25:56PM +0200, Javier Carrasco wrote:
>>
>> diff --git a/drivers/input/misc/sparcspkr.c b/drivers/input/misc/sparcspkr.c
>> index 20020cbc0752..bb1c732c8f95 100644
>> --- a/drivers/input/misc/sparcspkr.c
>> +++ b/drivers/input/misc/sparcspkr.c
>> @@ -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;
> 
> 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).

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

[Javier Carrasco]

On 11/10/2024 00:01, Javier Carrasco wrote:
> On 10/10/2024 23:43, Al Viro wrote:
>> On Thu, Oct 10, 2024 at 11:25:56PM +0200, Javier Carrasco wrote:
>>>
>>> diff --git a/drivers/input/misc/sparcspkr.c b/drivers/input/misc/sparcspkr.c
>>> index 20020cbc0752..bb1c732c8f95 100644
>>> --- a/drivers/input/misc/sparcspkr.c
>>> +++ b/drivers/input/misc/sparcspkr.c
>>> @@ -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;
>>
>> 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).
> 
> 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
> 


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

[Al Viro]

On Fri, Oct 11, 2024 at 12:09:01AM +0200, Javier Carrasco wrote:

> 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

[Javier Carrasco]

On 11/10/2024 00:22, Al Viro wrote:
> On Fri, Oct 11, 2024 at 12:09:01AM +0200, Javier Carrasco wrote:
> 
>> 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.

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 07/10] Input: 88pm860x - use cleanup facility for device_node

[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 <javier.carrasco.cruz@gmail.com>
---
 drivers/input/touchscreen/88pm860x-ts.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/input/touchscreen/88pm860x-ts.c b/drivers/input/touchscreen/88pm860x-ts.c
index 81a3ea4b9a3d..0468ce2b216f 100644
--- a/drivers/input/touchscreen/88pm860x-ts.c
+++ b/drivers/input/touchscreen/88pm860x-ts.c
@@ -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)

-- 
2.43.0

Re: [PATCH 07/10] Input: 88pm860x - use cleanup facility for device_node

[Dmitry Torokhov]

On Thu, Oct 10, 2024 at 11:25:57PM +0200, Javier Carrasco wrote:
> 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 <javier.carrasco.cruz@gmail.com>

Applied, thank you.

-- 
Dmitry

[PATCH 08/10] Input: i8042 - use cleanup facility for device_node

[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 <javier.carrasco.cruz@gmail.com>
---
 drivers/input/serio/i8042-sparcio.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/input/serio/i8042-sparcio.h b/drivers/input/serio/i8042-sparcio.h
index c2fda54dc384..8f38b6f4ae77 100644
--- a/drivers/input/serio/i8042-sparcio.h
+++ b/drivers/input/serio/i8042-sparcio.h
@@ -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;
 }
 

-- 
2.43.0

Re: [PATCH 08/10] Input: i8042 - use cleanup facility for device_node

[Dmitry Torokhov]

On Thu, Oct 10, 2024 at 11:25:58PM +0200, Javier Carrasco wrote:
> 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 <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/input/serio/i8042-sparcio.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/input/serio/i8042-sparcio.h b/drivers/input/serio/i8042-sparcio.h
> index c2fda54dc384..8f38b6f4ae77 100644
> --- a/drivers/input/serio/i8042-sparcio.h
> +++ b/drivers/input/serio/i8042-sparcio.h
> @@ -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.

>  
> -	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;
>  }
>  
> 
> -- 
> 2.43.0
> 

-- 
Dmitry

[PATCH 09/10] Input: raspberrypi-ts - use cleanup facility for device_node

[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 <javier.carrasco.cruz@gmail.com>
---
 drivers/input/touchscreen/raspberrypi-ts.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/raspberrypi-ts.c b/drivers/input/touchscreen/raspberrypi-ts.c
index 45c575df994e..841d39a449b3 100644
--- a/drivers/input/touchscreen/raspberrypi-ts.c
+++ b/drivers/input/touchscreen/raspberrypi-ts.c
@@ -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;
 

-- 
2.43.0

Re: [PATCH 09/10] Input: raspberrypi-ts - use cleanup facility for device_node

[Dmitry Torokhov]

On Thu, Oct 10, 2024 at 11:25:59PM +0200, Javier Carrasco wrote:
> 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 <javier.carrasco.cruz@gmail.com>

Applied, thank you.

-- 
Dmitry

[PATCH 10/10] Input: ts4800-ts - use cleanup facility for device_node

[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 <javier.carrasco.cruz@gmail.com>
---
 drivers/input/touchscreen/ts4800-ts.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ts4800-ts.c b/drivers/input/touchscreen/ts4800-ts.c
index 6cf66aadc10e..98422d1e80d6 100644
--- a/drivers/input/touchscreen/ts4800-ts.c
+++ b/drivers/input/touchscreen/ts4800-ts.c
@@ -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);

-- 
2.43.0

Re: [PATCH 10/10] Input: ts4800-ts - use cleanup facility for device_node

[Dmitry Torokhov]

On Thu, Oct 10, 2024 at 11:26:00PM +0200, Javier Carrasco wrote:
> 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 <javier.carrasco.cruz@gmail.com>

Applied, thank you.

-- 
Dmitry