| Message ID | 20220615122315.3965435-1-windhl@126.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: soc: sifive: Add missing of_node_put() in sifive_l2_cache.c | expand |
Hi Liang,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.19-rc2 next-20220615]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Liang-He/drivers-soc-sifive-Add-missing-of_node_put-in-sifive_l2_cache-c/20220615-202528
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 018ab4fabddd94f1c96f3b59e180691b9e88d5d8
config: riscv-allyesconfig (https://download.01.org/0day-ci/archive/20220616/202206160559.fVKJx0ST-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/49c4086768b5aff410a4a19ca740f8ae8e211844
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Liang-He/drivers-soc-sifive-Add-missing-of_node_put-in-sifive_l2_cache-c/20220615-202528
git checkout 49c4086768b5aff410a4a19ca740f8ae8e211844
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/soc/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/soc/sifive/sifive_l2_cache.c: In function 'sifive_l2_init':
>> drivers/soc/sifive/sifive_l2_cache.c:224:17: error: expected ';' before 'goto'
224 | goto out_put;
| ^~~~
vim +224 drivers/soc/sifive/sifive_l2_cache.c
194
195 static int __init sifive_l2_init(void)
196 {
197 struct device_node *np;
198 struct resource res;
199 int i, rc, intr_num;
200
201 int ret;
202
203 np = of_find_matching_node(NULL, sifive_l2_ids);
204 if (!np)
205 return -ENODEV;
206
207 if (of_address_to_resource(np, 0, &res))
208 {
209 ret = -ENODEV;
210 goto out_put;
211 }
212
213 l2_base = ioremap(res.start, resource_size(&res));
214 if (!l2_base)
215 {
216 ret = -ENOMEM;
217 goto out_put;
218 }
219
220 intr_num = of_property_count_u32_elems(np, "interrupts");
221 if (!intr_num) {
222 pr_err("L2CACHE: no interrupts property\n");
223 ret = -ENODEV
> 224 goto out_put;
225 }
226
227 for (i = 0; i < intr_num; i++) {
228 g_irq[i] = irq_of_parse_and_map(np, i);
229 rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL);
230
231 if (rc) {
232
233 pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]);
234 ret = rc;
235 goto out_put;
236 }
237 }
238
239 l2_config_read();
240
241 l2_cache_ops.get_priv_group = l2_get_priv_group;
242 riscv_set_cacheinfo_ops(&l2_cache_ops);
243
At 2022-06-16 05:58:05, "kernel test robot" <lkp@intel.com> wrote: >Hi Liang, > >Thank you for the patch! Yet something to improve: > >[auto build test ERROR on linus/master] >[also build test ERROR on v5.19-rc2 next-20220615] >[If your patch is applied to the wrong git tree, kindly drop us a note. >And when submitting patch, we suggest to use '--base' as documented in >https://git-scm.com/docs/git-format-patch] > When I use --base, there are too many prerequests-patch-id caused by my local commits. I don't know if it will cause other troubles. So I resent a new patch still without this '--base'. Is it Ok? Sorry, I forget to say in new patch that is based on v5.19-rc2 mainline git repo. >url: https://github.com/intel-lab-lkp/linux/commits/Liang-He/drivers-soc-sifive-Add-missing-of_node_put-in-sifive_l2_cache-c/20220615-202528 >base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 018ab4fabddd94f1c96f3b59e180691b9e88d5d8 >config: riscv-allyesconfig (https://download.01.org/0day-ci/archive/20220616/202206160559.fVKJx0ST-lkp@intel.com/config) >compiler: riscv64-linux-gcc (GCC) 11.3.0 >reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/intel-lab-lkp/linux/commit/49c4086768b5aff410a4a19ca740f8ae8e211844 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Liang-He/drivers-soc-sifive-Add-missing-of_node_put-in-sifive_l2_cache-c/20220615-202528 > git checkout 49c4086768b5aff410a4a19ca740f8ae8e211844 > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/soc/ > >If you fix the issue, kindly add following tag where applicable >Reported-by: kernel test robot <lkp@intel.com> > Thanks for this report, now I make a new patch and add "Reported-by: kernel test robot <lkp@intel.com>" >All errors (new ones prefixed by >>): > > drivers/soc/sifive/sifive_l2_cache.c: In function 'sifive_l2_init': >>> drivers/soc/sifive/sifive_l2_cache.c:224:17: error: expected ';' before 'goto' > 224 | goto out_put; > | ^~~~ > > Thanks for all your effort to improve the patch.
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@126.com> > --- > 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);
At 2022-06-16 13:12:21, "Christophe JAILLET" <christophe.jaillet@wanadoo.fr> 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@126.com> >> --- >> 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.
Le 16/06/2022 à 07:33, Liang He a écrit : > > Sorry for my troubles. I will check my patch more carefully. No problem, you seem to be knew on the lists so you have to learn the tricks, coding-style, tools... Happy patching :) CJ
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; + 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; + { + ret = -ENODEV; + goto out_put; + } l2_base = ioremap(res.start, resource_size(&res)); if (!l2_base) - return -ENOMEM; + { + 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 + 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) { + 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; + + +out_put: + of_node_put(np); + return ret; } device_initcall(sifive_l2_init);
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@126.com> --- drivers/soc/sifive/sifive_l2_cache.c | 29 ++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)