Re: [PATCH 1/2] soc: sifive: ccache: fix missing iounmap() in error path in sifive_ccache_init()

[Yang Yingliang <yangyingliang@huawei.com>]

Hi Conor,

On 2022/10/17 23:46, Conor Dooley wrote:
> On Mon, Oct 17, 2022 at 04:44:10PM +0800, Yang Yingliang wrote:
>> Add missing iounmap() before return error from sifive_ccache_init().
>>
>> Fixes: a967a289f169 ("RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs")
> Hey Yang Yangliang,
>
> Both of these patches look good to me.
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> One question - of_find_matching_node() increments the refcount of np
> right? It seems to me like we need an of_node_put() here, at the very
> least in the error paths (do we need it in the success path too?).
> If we are going to add cleanup to this driver, we may as well go the
> whole hog I think.
>
> In terms of the success paths, I assume the first place we can safely
> let the reference go is just before the call to ccache_config_read()?
Thanks for reviewing!

Yes, I think you are right, the refcount of np need be put in error paths,
and it's OK to put refcount before ccache_config_read() in normal path.
I can send another patch to fix this.

Thanks,
Yang
>
> Please lmk if I am misunderstanding anything here...
> Thanks,
> Conor.
>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>>   drivers/soc/sifive/sifive_ccache.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/soc/sifive/sifive_ccache.c b/drivers/soc/sifive/sifive_ccache.c
>> index 1c171150e878..25019c16d8ae 100644
>> --- a/drivers/soc/sifive/sifive_ccache.c
>> +++ b/drivers/soc/sifive/sifive_ccache.c
>> @@ -222,13 +222,16 @@ static int __init sifive_ccache_init(void)
>>   	if (!ccache_base)
>>   		return -ENOMEM;
>>   
>> -	if (of_property_read_u32(np, "cache-level", &level))
>> -		return -ENOENT;
>> +	if (of_property_read_u32(np, "cache-level", &level)) {
>> +		rc = -ENOENT;
>> +		goto err_unmap;
>> +	}
>>   
>>   	intr_num = of_property_count_u32_elems(np, "interrupts");
>>   	if (!intr_num) {
>>   		pr_err("No interrupts property\n");
>> -		return -ENODEV;
>> +		rc = -ENODEV;
>> +		goto err_unmap;
>>   	}
>>   
>>   	for (i = 0; i < intr_num; i++) {
>> @@ -237,7 +240,7 @@ static int __init sifive_ccache_init(void)
>>   				 NULL);
>>   		if (rc) {
>>   			pr_err("Could not request IRQ %d\n", g_irq[i]);
>> -			return rc;
>> +			goto err_unmap;
>>   		}
>>   	}
>>   
>> @@ -250,6 +253,10 @@ static int __init sifive_ccache_init(void)
>>   	setup_sifive_debug();
>>   #endif
>>   	return 0;
>> +
>> +err_unmap:
>> +	iounmap(ccache_base);
>> +	return rc;
>>   }
>>   
>>   device_initcall(sifive_ccache_init);
>> -- 
>> 2.25.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> .

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv