Re: [PATCH 2/5] x86/mwait-idle: add SnowRidge C-state table

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jan 18, 2022 at 03:05:54PM +0100, Jan Beulich wrote:
> On 18.01.2022 11:17, Roger Pau Monné wrote:
> > On Mon, Sep 06, 2021 at 03:00:46PM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/cpu/mwait-idle.c
> >> +++ b/xen/arch/x86/cpu/mwait-idle.c
> >> @@ -742,6 +742,32 @@ static const struct cpuidle_state dnv_cs
> >>    {}
> >>  };
> >>  
> >> +/*
> >> + * Note, depending on HW and FW revision, SnowRidge SoC may or may not 
> >> support
> >> + * C6, and this is indicated in the CPUID mwait leaf.
> >> + */
> >> +static const struct cpuidle_state snr_cstates[] = {
> >> +  {
> >> +          .name = "C1",
> > 
> > We usually use names like "C1-SNR" or similar in other cpuidle_state
> > structs. Is using plain "C1" intentional here?
> 
> I don't know. We're simply following the Linux side change. If we're
> unhappy with their naming (it indeed looks inconsistent), then we
> ought to make a separate patch on top (and maybe submit that also to
> Linux).

Looks like at some point Linux dropped the '-SNR' and similar suffixes
from the state names, so we should likely import that patch as a
pre-req for consistency? It's commit:

de09cdd09fa1 intel_idle: stop exposing platform acronyms in sysfs

> 
> >> @@ -954,6 +980,11 @@ static const struct idle_cpu idle_cpu_dn
> >>    .disable_promotion_to_c1e = 1,
> >>  };
> >>  
> >> +static const struct idle_cpu idle_cpu_snr = {
> >> +  .state_table = snr_cstates,
> >> +  .disable_promotion_to_c1e = true,
> > 
> > This likely wants to be 1 because the type is bool_t.
> 
> bool_t is just an alias of bool, so "true" ought to be fine. We may
> want to follow Linux in switching to bool altogether - iirc we didn't
> have bool yet at the time the driver (or the first commit needing it)
> was ported. I guess I'll make a patch ...

OK, thanks!

I guess for the patch itself then:

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Would be nice to get those things fixed for consistency.

Roger.