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

[Liang He]

At 2022-06-16 13:12:21, "Christophe JAILLET" <christophe.jaillet@xxxxxxxxxx> wrote:
>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);


Sorry for my troubles. I will check my patch more carefully.