Re: [PATCH] drivers: soc: sifive: Add missing of_node_put() in sifive_l2_cache.c

[Christophe JAILLET]

Le 15/06/2022 à 14:23, Liang He a écrit :
> In sifive_l2_init(), of_find_matching_node() will return a node pointer
> with refcount incremented. We should use of_node_put() in each fail path
> or when it is not used anymore.
>
> Signed-off-by: Liang He <windhl@xxxxxxx>
> ---
>   drivers/soc/sifive/sifive_l2_cache.c | 29 ++++++++++++++++++++++------
>   1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/soc/sifive/sifive_l2_cache.c b/drivers/soc/sifive/sifive_l2_cache.c
> index 59640a1d0b28..2b9c9522ef21 100644
> --- a/drivers/soc/sifive/sifive_l2_cache.c
> +++ b/drivers/soc/sifive/sifive_l2_cache.c
> @@ -198,29 +198,41 @@ static int __init sifive_l2_init(void)
>   	struct resource res;
>   	int i, rc, intr_num;
>   

Hi,
this empty line is not needed.

> +	int ret;
> +
>   	np = of_find_matching_node(NULL, sifive_l2_ids);
>   	if (!np)
>   		return -ENODEV;
>   
>   	if (of_address_to_resource(np, 0, &res))
> -		return -ENODEV;
> +	{

this should be at the end of the previous line.

> +		ret = -ENODEV;
> +		goto out_put;
> +	}
>   
>   	l2_base = ioremap(res.start, resource_size(&res));
>   	if (!l2_base)
> -		return -ENOMEM;
> +	{

Same here.

 +		ret = -ENOMEM;
> +		goto out_put;
> +	}
>   
>   	intr_num = of_property_count_u32_elems(np, "interrupts");
> -	if (!intr_num) {
> +	if (!intr_num) {		
>   		pr_err("L2CACHE: no interrupts property\n");
> -		return -ENODEV;
> +		ret = -ENODEV

Missing ";" as reported by the bot.

> +		goto out_put;
>   	}
>   
>   	for (i = 0; i < intr_num; i++) {
>   		g_irq[i] = irq_of_parse_and_map(np, i);
>   		rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL);
> +		
>   		if (rc) {
> +			

Why a new empty line here?

>   			pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]);
> -			return rc;
> +			ret = rc;
> +			goto out_put;
>   		}
>   	}
>   
> @@ -232,6 +244,11 @@ static int __init sifive_l2_init(void)
>   #ifdef CONFIG_DEBUG_FS
>   	setup_sifive_debug();
>   #endif
> -	return 0;
> +	ret = 0;
> +	
> +	

No need for 2 empty lines here.


There are also some trailing white spaces on some lines.

"./scripts/checkpatch <name_of_the_patch>" catches some of these tiny 
issues. Using --strict catches even more of these issues.

You should also always at least compile test your patches, even if they 
look obvious,

CJ


> +out_put:
> +	of_node_put(np);
> +	return ret;
>   }
>   device_initcall(sifive_l2_init);