| Message ID | 20181026144843.3028-1-ulf.hansson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] ARM: cpuidle: Don't register the driver when back-end init returns -ENXIO | expand |
On 29 October 2018 at 19:10, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 26/10/2018 16:48, Ulf Hansson wrote: >> There's no point to register the cpuidle driver for the current CPU, when >> the initialization of the arch specific back-end data fails by returning >> -ENXIO. >> >> Instead, let's re-order the sequence to its original flow, by first trying >> to initialize the back-end part and then act accordingly on the returned >> error code. Additionally, let's print the error message, no matter of what >> error code that was returned. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> >> Note, as far as I can tell it's only qcom-spm that may return -ENXIO, which also >> was original reason to why this error path was invented. >> >> Unfurtunate I don't have a qcom spm platfrom at hand, however I manually tried >> this by hacking the PSCI driver to return -ENXIO for some CPUs. So I assume it >> should then also work for qcom spm. >> >> --- >> drivers/cpuidle/cpuidle-arm.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c >> index 073557f433eb..df564d783216 100644 >> --- a/drivers/cpuidle/cpuidle-arm.c >> +++ b/drivers/cpuidle/cpuidle-arm.c >> @@ -103,13 +103,6 @@ static int __init arm_idle_init_cpu(int cpu) >> goto out_kfree_drv; >> } >> >> - ret = cpuidle_register_driver(drv); >> - if (ret) { >> - if (ret != -EBUSY) >> - pr_err("Failed to register cpuidle driver\n"); >> - goto out_kfree_drv; >> - } >> - > > Why not move the following block at the beginning of the function ? That's kind of what is happening in patch 2/2. However, I wanted to take small steps, isolating one change per patch. Does it make sense to keep $subject patch as is, when you look at patch2/2? > > ---> > > /* > * Call arch CPU operations in order to initialize > * idle states suspend back-end specific data > */ > ret = arm_cpuidle_init(cpu); > > /* > * Skip the cpuidle device initialization if the reported > * failure is a HW misconfiguration/breakage (-ENXIO). > */ > if (ret == -ENXIO) > return 0; > > if (ret) { > pr_err("CPU %d failed to init idle CPU ops\n", cpu); > s/goto out_unregister_drv;/return ret;/ > } > > <--- [...] Kind regards Uffe
On Tue, Oct 30, 2018 at 11:24:49AM +0100, Ulf Hansson wrote: > On 29 October 2018 at 18:45, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > On Fri, Oct 26, 2018 at 04:48:43PM +0200, Ulf Hansson wrote: > >> There's no point to register the cpuidle driver for the current CPU, when > >> the initialization of the arch specific back-end data fails by returning > >> -ENXIO. > >> > >> Instead, let's re-order the sequence to its original flow, by first trying > >> to initialize the back-end part and then act accordingly on the returned > >> error code. Additionally, let's print the error message, no matter of what > >> error code that was returned. > >> > >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > >> --- > >> > >> Note, as far as I can tell it's only qcom-spm that may return -ENXIO, which also > >> was original reason to why this error path was invented. > >> > >> Unfurtunate I don't have a qcom spm platfrom at hand, however I manually tried > >> this by hacking the PSCI driver to return -ENXIO for some CPUs. So I assume it > >> should then also work for qcom spm. > >> > >> --- > >> drivers/cpuidle/cpuidle-arm.c | 22 ++++++++++------------ > >> 1 file changed, 10 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c > >> index 073557f433eb..df564d783216 100644 > >> --- a/drivers/cpuidle/cpuidle-arm.c > >> +++ b/drivers/cpuidle/cpuidle-arm.c > >> @@ -103,13 +103,6 @@ static int __init arm_idle_init_cpu(int cpu) > >> goto out_kfree_drv; > >> } > >> > >> - ret = cpuidle_register_driver(drv); > >> - if (ret) { > >> - if (ret != -EBUSY) > >> - pr_err("Failed to register cpuidle driver\n"); > >> - goto out_kfree_drv; > >> - } > >> - > >> /* > >> * Call arch CPU operations in order to initialize > >> * idle states suspend back-end specific data > >> @@ -117,15 +110,20 @@ static int __init arm_idle_init_cpu(int cpu) > >> ret = arm_cpuidle_init(cpu); > >> > >> /* > >> - * Skip the cpuidle device initialization if the reported > >> + * Allow the initialization to continue for other CPUs, if the reported > >> * failure is a HW misconfiguration/breakage (-ENXIO). > >> */ > >> - if (ret == -ENXIO) > >> - return 0; > > > > I wonder whether moving the drv allocation and dt initialization after > > arm_cpuidle_init(cpu) would simplify the error path; the patch makes sense > > the error path is getting a bit hard to read - maybe we can simplify it. > > Yep, exactly that is happening in patch 2/2. But as I also told > Daniel, I wanted to take small steps and keep one change per patch. > > Does it make sense to keep this as is, when you have looked at patch 2/2? > > > > > I think, if you can single it out, it would be good to add a Fixes: > > tag too. > > I can do that, but I am not sure it actually solves a problem, besides > improving the error path. > > Or what did you have in mind? I am not sure it counts as a bug but leaving the driver registered even if the following set-up fails is certainly not the expected behaviour. Hardly urgent but still a fix. Thanks, Lorenzo
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c index 073557f433eb..df564d783216 100644 --- a/drivers/cpuidle/cpuidle-arm.c +++ b/drivers/cpuidle/cpuidle-arm.c @@ -103,13 +103,6 @@ static int __init arm_idle_init_cpu(int cpu) goto out_kfree_drv; } - ret = cpuidle_register_driver(drv); - if (ret) { - if (ret != -EBUSY) - pr_err("Failed to register cpuidle driver\n"); - goto out_kfree_drv; - } - /* * Call arch CPU operations in order to initialize * idle states suspend back-end specific data @@ -117,15 +110,20 @@ static int __init arm_idle_init_cpu(int cpu) ret = arm_cpuidle_init(cpu); /* - * Skip the cpuidle device initialization if the reported + * Allow the initialization to continue for other CPUs, if the reported * failure is a HW misconfiguration/breakage (-ENXIO). */ - if (ret == -ENXIO) - return 0; - if (ret) { pr_err("CPU %d failed to init idle CPU ops\n", cpu); - goto out_unregister_drv; + ret = ret == -ENXIO ? 0 : ret; + goto out_kfree_drv; + } + + ret = cpuidle_register_driver(drv); + if (ret) { + if (ret != -EBUSY) + pr_err("Failed to register cpuidle driver\n"); + goto out_kfree_drv; } dev = kzalloc(sizeof(*dev), GFP_KERNEL);
There's no point to register the cpuidle driver for the current CPU, when the initialization of the arch specific back-end data fails by returning -ENXIO. Instead, let's re-order the sequence to its original flow, by first trying to initialize the back-end part and then act accordingly on the returned error code. Additionally, let's print the error message, no matter of what error code that was returned. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- Note, as far as I can tell it's only qcom-spm that may return -ENXIO, which also was original reason to why this error path was invented. Unfurtunate I don't have a qcom spm platfrom at hand, however I manually tried this by hacking the PSCI driver to return -ENXIO for some CPUs. So I assume it should then also work for qcom spm. --- drivers/cpuidle/cpuidle-arm.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) -- 2.17.1