* drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret'
@ 2022-11-22 7:11 Dan Carpenter
2022-11-22 8:13 ` Anup Patel
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-11-22 7:11 UTC (permalink / raw)
To: oe-kbuild, Randy Dunlap
Cc: lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt, Anup Patel
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: eb7081409f94a9a8608593d0fb63a1aa3d6f95d8
commit: f81f7861ee2aaa6f652f18e8f622547bdd379724 cpuidle: riscv: support non-SMP config
date: 7 months ago
config: riscv-randconfig-m031-20221121
compiler: riscv64-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
smatch warnings:
drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret'
vim +/ret +506 drivers/cpuidle/cpuidle-riscv-sbi.c
6abf32f1d9c500 Anup Patel 2022-02-10 481 static int sbi_genpd_probe(struct device_node *np)
6abf32f1d9c500 Anup Patel 2022-02-10 482 {
6abf32f1d9c500 Anup Patel 2022-02-10 483 struct device_node *node;
6abf32f1d9c500 Anup Patel 2022-02-10 484 int ret = 0, pd_count = 0;
6abf32f1d9c500 Anup Patel 2022-02-10 485
6abf32f1d9c500 Anup Patel 2022-02-10 486 if (!np)
6abf32f1d9c500 Anup Patel 2022-02-10 487 return -ENODEV;
6abf32f1d9c500 Anup Patel 2022-02-10 488
6abf32f1d9c500 Anup Patel 2022-02-10 489 /*
6abf32f1d9c500 Anup Patel 2022-02-10 490 * Parse child nodes for the "#power-domain-cells" property and
6abf32f1d9c500 Anup Patel 2022-02-10 491 * initialize a genpd/genpd-of-provider pair when it's found.
6abf32f1d9c500 Anup Patel 2022-02-10 492 */
6abf32f1d9c500 Anup Patel 2022-02-10 493 for_each_child_of_node(np, node) {
6abf32f1d9c500 Anup Patel 2022-02-10 494 if (!of_find_property(node, "#power-domain-cells", NULL))
6abf32f1d9c500 Anup Patel 2022-02-10 495 continue;
6abf32f1d9c500 Anup Patel 2022-02-10 496
6abf32f1d9c500 Anup Patel 2022-02-10 497 ret = sbi_pd_init(node);
6abf32f1d9c500 Anup Patel 2022-02-10 498 if (ret)
6abf32f1d9c500 Anup Patel 2022-02-10 499 goto put_node;
6abf32f1d9c500 Anup Patel 2022-02-10 500
6abf32f1d9c500 Anup Patel 2022-02-10 501 pd_count++;
6abf32f1d9c500 Anup Patel 2022-02-10 502 }
6abf32f1d9c500 Anup Patel 2022-02-10 503
6abf32f1d9c500 Anup Patel 2022-02-10 504 /* Bail out if not using the hierarchical CPU topology. */
6abf32f1d9c500 Anup Patel 2022-02-10 505 if (!pd_count)
6abf32f1d9c500 Anup Patel 2022-02-10 @506 goto no_pd;
Error code?
6abf32f1d9c500 Anup Patel 2022-02-10 507
6abf32f1d9c500 Anup Patel 2022-02-10 508 /* Link genpd masters/subdomains to model the CPU topology. */
6abf32f1d9c500 Anup Patel 2022-02-10 509 ret = dt_idle_pd_init_topology(np);
6abf32f1d9c500 Anup Patel 2022-02-10 510 if (ret)
6abf32f1d9c500 Anup Patel 2022-02-10 511 goto remove_pd;
6abf32f1d9c500 Anup Patel 2022-02-10 512
6abf32f1d9c500 Anup Patel 2022-02-10 513 return 0;
6abf32f1d9c500 Anup Patel 2022-02-10 514
6abf32f1d9c500 Anup Patel 2022-02-10 515 put_node:
6abf32f1d9c500 Anup Patel 2022-02-10 516 of_node_put(node);
6abf32f1d9c500 Anup Patel 2022-02-10 517 remove_pd:
6abf32f1d9c500 Anup Patel 2022-02-10 518 sbi_pd_remove();
6abf32f1d9c500 Anup Patel 2022-02-10 519 pr_err("failed to create CPU PM domains ret=%d\n", ret);
6abf32f1d9c500 Anup Patel 2022-02-10 520 no_pd:
6abf32f1d9c500 Anup Patel 2022-02-10 521 return ret;
6abf32f1d9c500 Anup Patel 2022-02-10 522 }
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret' 2022-11-22 7:11 drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret' Dan Carpenter @ 2022-11-22 8:13 ` Anup Patel 2022-11-24 13:35 ` Conor Dooley 0 siblings, 1 reply; 4+ messages in thread From: Anup Patel @ 2022-11-22 8:13 UTC (permalink / raw) To: Dan Carpenter Cc: oe-kbuild, Randy Dunlap, lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt On Tue, Nov 22, 2022 at 12:41 PM Dan Carpenter <error27@gmail.com> wrote: > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > head: eb7081409f94a9a8608593d0fb63a1aa3d6f95d8 > commit: f81f7861ee2aaa6f652f18e8f622547bdd379724 cpuidle: riscv: support non-SMP config > date: 7 months ago > config: riscv-randconfig-m031-20221121 > compiler: riscv64-linux-gcc (GCC) 12.1.0 > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <error27@gmail.com> > > smatch warnings: > drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret' > > vim +/ret +506 drivers/cpuidle/cpuidle-riscv-sbi.c > > 6abf32f1d9c500 Anup Patel 2022-02-10 481 static int sbi_genpd_probe(struct device_node *np) > 6abf32f1d9c500 Anup Patel 2022-02-10 482 { > 6abf32f1d9c500 Anup Patel 2022-02-10 483 struct device_node *node; > 6abf32f1d9c500 Anup Patel 2022-02-10 484 int ret = 0, pd_count = 0; > 6abf32f1d9c500 Anup Patel 2022-02-10 485 > 6abf32f1d9c500 Anup Patel 2022-02-10 486 if (!np) > 6abf32f1d9c500 Anup Patel 2022-02-10 487 return -ENODEV; > 6abf32f1d9c500 Anup Patel 2022-02-10 488 > 6abf32f1d9c500 Anup Patel 2022-02-10 489 /* > 6abf32f1d9c500 Anup Patel 2022-02-10 490 * Parse child nodes for the "#power-domain-cells" property and > 6abf32f1d9c500 Anup Patel 2022-02-10 491 * initialize a genpd/genpd-of-provider pair when it's found. > 6abf32f1d9c500 Anup Patel 2022-02-10 492 */ > 6abf32f1d9c500 Anup Patel 2022-02-10 493 for_each_child_of_node(np, node) { > 6abf32f1d9c500 Anup Patel 2022-02-10 494 if (!of_find_property(node, "#power-domain-cells", NULL)) > 6abf32f1d9c500 Anup Patel 2022-02-10 495 continue; > 6abf32f1d9c500 Anup Patel 2022-02-10 496 > 6abf32f1d9c500 Anup Patel 2022-02-10 497 ret = sbi_pd_init(node); > 6abf32f1d9c500 Anup Patel 2022-02-10 498 if (ret) > 6abf32f1d9c500 Anup Patel 2022-02-10 499 goto put_node; > 6abf32f1d9c500 Anup Patel 2022-02-10 500 > 6abf32f1d9c500 Anup Patel 2022-02-10 501 pd_count++; > 6abf32f1d9c500 Anup Patel 2022-02-10 502 } > 6abf32f1d9c500 Anup Patel 2022-02-10 503 > 6abf32f1d9c500 Anup Patel 2022-02-10 504 /* Bail out if not using the hierarchical CPU topology. */ > 6abf32f1d9c500 Anup Patel 2022-02-10 505 if (!pd_count) > 6abf32f1d9c500 Anup Patel 2022-02-10 @506 goto no_pd; > > Error code? Yes, we intentionally "return 0" when there are no generic power-domains defined for the CPUs, the sbi_cpuidle_probe() continue further and try traditional DT cpuidle states. Regards, Anup > > 6abf32f1d9c500 Anup Patel 2022-02-10 507 > 6abf32f1d9c500 Anup Patel 2022-02-10 508 /* Link genpd masters/subdomains to model the CPU topology. */ > 6abf32f1d9c500 Anup Patel 2022-02-10 509 ret = dt_idle_pd_init_topology(np); > 6abf32f1d9c500 Anup Patel 2022-02-10 510 if (ret) > 6abf32f1d9c500 Anup Patel 2022-02-10 511 goto remove_pd; > 6abf32f1d9c500 Anup Patel 2022-02-10 512 > 6abf32f1d9c500 Anup Patel 2022-02-10 513 return 0; > 6abf32f1d9c500 Anup Patel 2022-02-10 514 > 6abf32f1d9c500 Anup Patel 2022-02-10 515 put_node: > 6abf32f1d9c500 Anup Patel 2022-02-10 516 of_node_put(node); > 6abf32f1d9c500 Anup Patel 2022-02-10 517 remove_pd: > 6abf32f1d9c500 Anup Patel 2022-02-10 518 sbi_pd_remove(); > 6abf32f1d9c500 Anup Patel 2022-02-10 519 pr_err("failed to create CPU PM domains ret=%d\n", ret); > 6abf32f1d9c500 Anup Patel 2022-02-10 520 no_pd: > 6abf32f1d9c500 Anup Patel 2022-02-10 521 return ret; > 6abf32f1d9c500 Anup Patel 2022-02-10 522 } > > -- > 0-DAY CI Kernel Test Service > https://01.org/lkp > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret' 2022-11-22 8:13 ` Anup Patel @ 2022-11-24 13:35 ` Conor Dooley 2022-11-29 5:19 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Conor Dooley @ 2022-11-24 13:35 UTC (permalink / raw) To: Anup Patel Cc: Dan Carpenter, oe-kbuild, Randy Dunlap, lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt Hey Anup, On Tue, Nov 22, 2022 at 01:43:38PM +0530, Anup Patel wrote: > On Tue, Nov 22, 2022 at 12:41 PM Dan Carpenter <error27@gmail.com> wrote: > > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > head: eb7081409f94a9a8608593d0fb63a1aa3d6f95d8 > > commit: f81f7861ee2aaa6f652f18e8f622547bdd379724 cpuidle: riscv: support non-SMP config > > date: 7 months ago > > config: riscv-randconfig-m031-20221121 > > compiler: riscv64-linux-gcc (GCC) 12.1.0 > > > > If you fix the issue, kindly add following tag where applicable > > | Reported-by: kernel test robot <lkp@intel.com> > > | Reported-by: Dan Carpenter <error27@gmail.com> > > > > smatch warnings: > > drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret' > > > > vim +/ret +506 drivers/cpuidle/cpuidle-riscv-sbi.c > > > > 6abf32f1d9c500 Anup Patel 2022-02-10 481 static int sbi_genpd_probe(struct device_node *np) > > 6abf32f1d9c500 Anup Patel 2022-02-10 482 { > > 6abf32f1d9c500 Anup Patel 2022-02-10 483 struct device_node *node; > > 6abf32f1d9c500 Anup Patel 2022-02-10 484 int ret = 0, pd_count = 0; > > 6abf32f1d9c500 Anup Patel 2022-02-10 485 > > 6abf32f1d9c500 Anup Patel 2022-02-10 486 if (!np) > > 6abf32f1d9c500 Anup Patel 2022-02-10 487 return -ENODEV; > > 6abf32f1d9c500 Anup Patel 2022-02-10 488 > > 6abf32f1d9c500 Anup Patel 2022-02-10 489 /* > > 6abf32f1d9c500 Anup Patel 2022-02-10 490 * Parse child nodes for the "#power-domain-cells" property and > > 6abf32f1d9c500 Anup Patel 2022-02-10 491 * initialize a genpd/genpd-of-provider pair when it's found. > > 6abf32f1d9c500 Anup Patel 2022-02-10 492 */ > > 6abf32f1d9c500 Anup Patel 2022-02-10 493 for_each_child_of_node(np, node) { > > 6abf32f1d9c500 Anup Patel 2022-02-10 494 if (!of_find_property(node, "#power-domain-cells", NULL)) > > 6abf32f1d9c500 Anup Patel 2022-02-10 495 continue; > > 6abf32f1d9c500 Anup Patel 2022-02-10 496 > > 6abf32f1d9c500 Anup Patel 2022-02-10 497 ret = sbi_pd_init(node); > > 6abf32f1d9c500 Anup Patel 2022-02-10 498 if (ret) > > 6abf32f1d9c500 Anup Patel 2022-02-10 499 goto put_node; > > 6abf32f1d9c500 Anup Patel 2022-02-10 500 > > 6abf32f1d9c500 Anup Patel 2022-02-10 501 pd_count++; > > 6abf32f1d9c500 Anup Patel 2022-02-10 502 } > > 6abf32f1d9c500 Anup Patel 2022-02-10 503 > > 6abf32f1d9c500 Anup Patel 2022-02-10 504 /* Bail out if not using the hierarchical CPU topology. */ > > 6abf32f1d9c500 Anup Patel 2022-02-10 505 if (!pd_count) > > 6abf32f1d9c500 Anup Patel 2022-02-10 @506 goto no_pd; > > > > Error code? > > Yes, we intentionally "return 0" when there are no > generic power-domains defined for the CPUs, the > sbi_cpuidle_probe() continue further and try traditional > DT cpuidle states. Happened upon this when looking for our other cpuidle conversation on lore earlier, would it not make more sense from a readability PoV to just return zero here? ret has to be zero at this point since: ret = sbi_pd_init(node); if (ret) goto put_node; and the `goto no_pd` does not do any cleanup. Certainly, it'd look more intentional that way, no? > > 6abf32f1d9c500 Anup Patel 2022-02-10 507 > > 6abf32f1d9c500 Anup Patel 2022-02-10 508 /* Link genpd masters/subdomains to model the CPU topology. */ > > 6abf32f1d9c500 Anup Patel 2022-02-10 509 ret = dt_idle_pd_init_topology(np); > > 6abf32f1d9c500 Anup Patel 2022-02-10 510 if (ret) > > 6abf32f1d9c500 Anup Patel 2022-02-10 511 goto remove_pd; > > 6abf32f1d9c500 Anup Patel 2022-02-10 512 > > 6abf32f1d9c500 Anup Patel 2022-02-10 513 return 0; > > 6abf32f1d9c500 Anup Patel 2022-02-10 514 > > 6abf32f1d9c500 Anup Patel 2022-02-10 515 put_node: > > 6abf32f1d9c500 Anup Patel 2022-02-10 516 of_node_put(node); > > 6abf32f1d9c500 Anup Patel 2022-02-10 517 remove_pd: > > 6abf32f1d9c500 Anup Patel 2022-02-10 518 sbi_pd_remove(); > > 6abf32f1d9c500 Anup Patel 2022-02-10 519 pr_err("failed to create CPU PM domains ret=%d\n", ret); I do find this cleanup a bit confusing though. It's probably just me not following, but I'd usually expect the teardown to get more complicated the later you leave the function, not simpler. How come, if sbi_od_init() fails, you need to call sbi_pd_remove()? Does it not clean up after itself, or is calling sbi_pd_remove() always a safe thing to do, even if, say, dt_idle_pd_alloc() failed? Similarly, how come the of_node_put() in the later failure cases? IIUC for_each_child_of_node() will leave us with a dangling of_node reference except if sbi_pd_init() fails. I'm probably missing something here, but it was at least non-obvious from the code. If I have missing something, and I probably have, please let me know what it is, I'd appreciate it! Thanks, Conor. > > 6abf32f1d9c500 Anup Patel 2022-02-10 520 no_pd: > > 6abf32f1d9c500 Anup Patel 2022-02-10 521 return ret; > > 6abf32f1d9c500 Anup Patel 2022-02-10 522 } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret' 2022-11-24 13:35 ` Conor Dooley @ 2022-11-29 5:19 ` Dan Carpenter 0 siblings, 0 replies; 4+ messages in thread From: Dan Carpenter @ 2022-11-29 5:19 UTC (permalink / raw) To: Conor Dooley Cc: Anup Patel, oe-kbuild, Randy Dunlap, lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt On Thu, Nov 24, 2022 at 01:35:33PM +0000, Conor Dooley wrote: > Hey Anup, > > On Tue, Nov 22, 2022 at 01:43:38PM +0530, Anup Patel wrote: > > On Tue, Nov 22, 2022 at 12:41 PM Dan Carpenter <error27@gmail.com> wrote: > > > > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > > head: eb7081409f94a9a8608593d0fb63a1aa3d6f95d8 > > > commit: f81f7861ee2aaa6f652f18e8f622547bdd379724 cpuidle: riscv: support non-SMP config > > > date: 7 months ago > > > config: riscv-randconfig-m031-20221121 > > > compiler: riscv64-linux-gcc (GCC) 12.1.0 > > > > > > If you fix the issue, kindly add following tag where applicable > > > | Reported-by: kernel test robot <lkp@intel.com> > > > | Reported-by: Dan Carpenter <error27@gmail.com> > > > > > > smatch warnings: > > > drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret' > > > > > > vim +/ret +506 drivers/cpuidle/cpuidle-riscv-sbi.c > > > > > > 6abf32f1d9c500 Anup Patel 2022-02-10 481 static int sbi_genpd_probe(struct device_node *np) > > > 6abf32f1d9c500 Anup Patel 2022-02-10 482 { > > > 6abf32f1d9c500 Anup Patel 2022-02-10 483 struct device_node *node; > > > 6abf32f1d9c500 Anup Patel 2022-02-10 484 int ret = 0, pd_count = 0; > > > 6abf32f1d9c500 Anup Patel 2022-02-10 485 > > > 6abf32f1d9c500 Anup Patel 2022-02-10 486 if (!np) > > > 6abf32f1d9c500 Anup Patel 2022-02-10 487 return -ENODEV; > > > 6abf32f1d9c500 Anup Patel 2022-02-10 488 > > > 6abf32f1d9c500 Anup Patel 2022-02-10 489 /* > > > 6abf32f1d9c500 Anup Patel 2022-02-10 490 * Parse child nodes for the "#power-domain-cells" property and > > > 6abf32f1d9c500 Anup Patel 2022-02-10 491 * initialize a genpd/genpd-of-provider pair when it's found. > > > 6abf32f1d9c500 Anup Patel 2022-02-10 492 */ > > > 6abf32f1d9c500 Anup Patel 2022-02-10 493 for_each_child_of_node(np, node) { > > > 6abf32f1d9c500 Anup Patel 2022-02-10 494 if (!of_find_property(node, "#power-domain-cells", NULL)) > > > 6abf32f1d9c500 Anup Patel 2022-02-10 495 continue; > > > 6abf32f1d9c500 Anup Patel 2022-02-10 496 > > > 6abf32f1d9c500 Anup Patel 2022-02-10 497 ret = sbi_pd_init(node); > > > 6abf32f1d9c500 Anup Patel 2022-02-10 498 if (ret) > > > 6abf32f1d9c500 Anup Patel 2022-02-10 499 goto put_node; > > > 6abf32f1d9c500 Anup Patel 2022-02-10 500 > > > 6abf32f1d9c500 Anup Patel 2022-02-10 501 pd_count++; > > > 6abf32f1d9c500 Anup Patel 2022-02-10 502 } > > > 6abf32f1d9c500 Anup Patel 2022-02-10 503 > > > 6abf32f1d9c500 Anup Patel 2022-02-10 504 /* Bail out if not using the hierarchical CPU topology. */ > > > 6abf32f1d9c500 Anup Patel 2022-02-10 505 if (!pd_count) > > > 6abf32f1d9c500 Anup Patel 2022-02-10 @506 goto no_pd; > > > > > > Error code? > > > > Yes, we intentionally "return 0" when there are no > > generic power-domains defined for the CPUs, the > > sbi_cpuidle_probe() continue further and try traditional > > DT cpuidle states. > > Happened upon this when looking for our other cpuidle conversation on > lore earlier, would it not make more sense from a readability PoV to > just return zero here? I am always in favor of direct returns over a do nothing return because of the ambiguity about error codes. Also I just published a new Smatch check where "return ret;" and "return 0;" are equivalent. ret = frob(); if (ret) return ret; if (something else) return ret; I have a different unpublished check for: ret = frob(); if (!ret) return ret; The bug I'm looking for here is that once or twice a year the ! character is unintentional. regards, dan carpenter ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-29 5:20 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-22 7:11 drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret' Dan Carpenter 2022-11-22 8:13 ` Anup Patel 2022-11-24 13:35 ` Conor Dooley 2022-11-29 5:19 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox