linux-kernel - Re: [PATCH] drivers: soc: sifive: Add missing of_node_put() in sifive_l2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a5f6f76d-efc1-b421-7623-ad46996f5b94@wanadoo.fr>
Date:   Thu, 16 Jun 2022 07:12:21 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Liang He <windhl@....com>, palmer@...belt.com,
        paul.walmsley@...ive.com, aou@...s.berkeley.edu
Cc:     linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers: soc: sifive: Add missing of_node_put() in
 sifive_l2_cache.c

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@....com>
> ---
>   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);