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

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

[Yang Yingliang]

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")
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

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

[Yang Yingliang]

Add missing free_irq() before return error from sifive_ccache_init().

Fixes: a967a289f169 ("RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/soc/sifive/sifive_ccache.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/sifive/sifive_ccache.c b/drivers/soc/sifive/sifive_ccache.c
index 25019c16d8ae..98269d056728 100644
--- a/drivers/soc/sifive/sifive_ccache.c
+++ b/drivers/soc/sifive/sifive_ccache.c
@@ -240,7 +240,7 @@ static int __init sifive_ccache_init(void)
 				 NULL);
 		if (rc) {
 			pr_err("Could not request IRQ %d\n", g_irq[i]);
-			goto err_unmap;
+			goto err_free_irq;
 		}
 	}
 
@@ -254,6 +254,9 @@ static int __init sifive_ccache_init(void)
 #endif
 	return 0;
 
+err_free_irq:
+	while (--i >= 0)
+		free_irq(g_irq[i], NULL);
 err_unmap:
 	iounmap(ccache_base);
 	return rc;
-- 
2.25.1


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

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

[Conor Dooley]

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

On Mon, Oct 17, 2022 at 04:44:11PM +0800, Yang Yingliang wrote:
> Add missing free_irq() before return error from sifive_ccache_init().
> 
> Fixes: a967a289f169 ("RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  drivers/soc/sifive/sifive_ccache.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/sifive/sifive_ccache.c b/drivers/soc/sifive/sifive_ccache.c
> index 25019c16d8ae..98269d056728 100644
> --- a/drivers/soc/sifive/sifive_ccache.c
> +++ b/drivers/soc/sifive/sifive_ccache.c
> @@ -240,7 +240,7 @@ static int __init sifive_ccache_init(void)
>  				 NULL);
>  		if (rc) {
>  			pr_err("Could not request IRQ %d\n", g_irq[i]);
> -			goto err_unmap;
> +			goto err_free_irq;
>  		}
>  	}
>  
> @@ -254,6 +254,9 @@ static int __init sifive_ccache_init(void)
>  #endif
>  	return 0;
>  
> +err_free_irq:
> +	while (--i >= 0)
> +		free_irq(g_irq[i], NULL);
>  err_unmap:
>  	iounmap(ccache_base);
>  	return rc;
> -- 
> 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

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

[Conor Dooley]

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()?

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

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

[Yang Yingliang]

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