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

From: Liang He
Date: Thu Jun 16 2022 - 01:34:12 EST





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.