| Message ID | 20221017084411.3557098-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] soc: sifive: ccache: fix missing iounmap() in error path in sifive_ccache_init() | expand |
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
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 > .
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);
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(-)