* 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;
as well as URLs for NNTP newsgroup(s).