Re: drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret'

[Conor Dooley]

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@xxxxxxxxx> 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@xxxxxxxxx>
> > | Reported-by: Dan Carpenter <error27@xxxxxxxxx>
> >
> > 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  }