Re: [PATCH v3 4/5] s390/idle: Introduce cpuidle for s390

[Christian Loehle]

On 6/18/26 15:47, Mete Durlu wrote:
> On 6/18/26 4:41 PM, Christian Loehle wrote:
>> On 6/18/26 13:00, Mete Durlu wrote:
>>> Introduce generic cpuidle driver on s390. Use a two stage approach to
>>> handle idle scenarios and use idle governor for idle stage selection.
>>> Two stages are, from shallow to deep, idle polling and enabled wait.
>>>
>>> Suggested-by: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>
>>> Suggested-by: Heiko Carstens <hca@xxxxxxxxxxxxx>
>>> Signed-off-by: Mete Durlu <meted@xxxxxxxxxxxxx>
>>> ---
> 
> [..snip..]
> 
> 
>>> +static int s390_cpuidle_cpu_online(unsigned int cpu)
>>> +{
>>> +    struct cpuidle_device *dev = &per_cpu(cpuidle_dev, cpu);
>>> +    int rc;
>>> +
>>> +    if (dev->registered) {
>>> +        cpuidle_pause_and_lock();
>>> +        rc = cpuidle_enable_device(dev);
>>> +        cpuidle_resume_and_unlock();
>>> +        if (rc)
>>> +            pr_err("Failed to enable cpuidle device on cpu %u\n", cpu);
>>> +    } else {
>>> +        dev->cpu = cpu;
>>> +        rc = cpuidle_register_device(dev);
>>> +        if (rc)
>>> +            pr_err("Failed to register cpuidle driver on cpu %u\n", cpu);
>>> +    }
>>> +    return rc;
>>
>> Most other drivers allow for hotplug cpu_online to succeed even if cpuidle doesn't, is
>> this intentionally done otherwise here?
> 
> Yes it is by design. Users would not be aware that something went wrong
> if we don't fail here (unless they check dmesg regularly which we don't
> expect). By failing here driver communicates that there is a problem
> and allows users to resolve that instead of letting a cpu run with
> degraded performance.
> 

Okay, thanks!
So FWIW this looks reasonable to me now, can't really comment much
on the s390 specifics, in particular as a guest, but from a purely
cpuidle perspective this looks okay, so good to see that this seems
to be working for you!

Reviewed-by: Christian Loehle <christian.loehle@xxxxxxx>