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?
[...]
Kind regards
Uffe
