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....@citrix.com>
Would be nice to get those things fixed for consistency.
Roger.
