Re: [PATCH v3] leds: max77650: Add of_node_put() before return

[Dan Murphy <dmurphy@xxxxxx>]

On 7/11/19 2:54 AM, Nishka Dasgupta wrote:
> Each iteration of for_each_child_of_node puts the previous node, but in
> the case of a return from the middle of the loop, there is no put, thus
> causing a memory leak.
> Hence create a new label, err_node_put, which puts the previous node and
> returns variable rv. Modify the mid-loop return statements to instead
> store the return value in rv and jump to err_node_put.
> Issue found with Coccinelle.
>
> Signed-off-by: Nishka Dasgupta <nishkadg.linux@xxxxxxxxx>

Reviewed-by: Dan Murphy <dmurphy@xxxxxx>


> ---
> Changes in v3:
> - Add change log.
> Changes in v2:
> - Change subject line to match previous patches on the same file.
> - Merge the of_node_put calls into a single call in a label at the end
>    of the function instead of calling it separately for each return
>    statement.
>
>   drivers/leds/leds-max77650.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/leds/leds-max77650.c b/drivers/leds/leds-max77650.c
> index 6b74ce9cac12..1eb9998899e4 100644
> --- a/drivers/leds/leds-max77650.c
> +++ b/drivers/leds/leds-max77650.c
> @@ -93,8 +93,10 @@ static int max77650_led_probe(struct platform_device *pdev)
>   
>   	for_each_child_of_node(of_node, child) {
>   		rv = of_property_read_u32(child, "reg", &reg);
> -		if (rv || reg >= MAX77650_LED_NUM_LEDS)
> -			return -EINVAL;
> +		if (rv || reg >= MAX77650_LED_NUM_LEDS) {
> +			rv = -EINVAL;
> +			goto err_node_put;
> +		}
>   
>   		led = &leds[reg];
>   		led->map = map;
> @@ -109,8 +111,10 @@ static int max77650_led_probe(struct platform_device *pdev)
>   		} else {
>   			led->cdev.name = devm_kasprintf(dev, GFP_KERNEL,
>   							"max77650:%s", label);
> -			if (!led->cdev.name)
> -				return -ENOMEM;
> +			if (!led->cdev.name) {
> +				rv = -ENOMEM;
> +				goto err_node_put;
> +			}
>   		}
>   
>   		of_property_read_string(child, "linux,default-trigger",
> @@ -118,20 +122,23 @@ static int max77650_led_probe(struct platform_device *pdev)
>   
>   		rv = devm_of_led_classdev_register(dev, child, &led->cdev);
>   		if (rv)
> -			return rv;
> +			goto err_node_put;
>   
>   		rv = regmap_write(map, led->regA, MAX77650_LED_A_DEFAULT);
>   		if (rv)
> -			return rv;
> +			goto err_node_put;
>   
>   		rv = regmap_write(map, led->regB, MAX77650_LED_B_DEFAULT);
>   		if (rv)
> -			return rv;
> +			goto err_node_put;
>   	}
>   
>   	return regmap_write(map,
>   			    MAX77650_REG_CNFG_LED_TOP,
>   			    MAX77650_LED_TOP_DEFAULT);
> +err_node_put:
> +	of_node_put(child);
> +	return rv;
>   }
>   
>   static struct platform_driver max77650_led_driver = {