On Tue, Oct 30, 2018 at 11:24:49AM +0100, Ulf Hansson wrote:
> On 29 October 2018 at 18:45, Lorenzo Pieralisi
> <lorenzo.pieralisi@xxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >>
> >> 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
