Re: [PATCH 9/9] x86/mwait-idle: Add C-states validation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.05.2026 09:38, Roger Pau Monné wrote:
> On Mon, May 04, 2026 at 11:34:40AM +0200, Jan Beulich wrote:
>> On 24.04.2026 21:15, Roger Pau Monné wrote:
>>> On Thu, Mar 12, 2026 at 05:58:21PM +0100, Jan Beulich wrote:
>>>> @@ -1589,6 +1594,41 @@ static char *__init get_cmdline_field(ch
>>>>  }
>>>>  
>>>>  /**
>>>> + * validate_cmdline_cstate - Validate a C-state from cmdline.
>>>> + * @state: The C-state to validate.
>>>> + * @prev_state: The previous C-state in the table or NULL.
>>>> + *
>>>> + * Return: 0 if the C-state is valid or -EINVAL otherwise.
>>>
>>> Hm, I know we picked this up from upstream, but this function would
>>> better return a boolean, rather than 0 or -EINVAL.
>>
>> I agree, but I didn't want to deviate from their code purely for cosmetic
>> reasons.
>>
>>>> +static int __init validate_cmdline_cstate(struct cpuidle_state *state,
>>>> +                                    struct cpuidle_state *prev_state)
>>>> +{
>>>> +  if (state->exit_latency == 0)
>>>> +          /* Exit latency 0 can only be used for the POLL state */
>>>> +          return -EINVAL;
>>>> +
>>>> +  if (state->exit_latency > MAX_CMDLINE_LATENCY_US)
>>>> +          return -EINVAL;
>>>> +
>>>> +  if (state->target_residency > MAX_CMDLINE_RESIDENCY_US)
>>>> +          return -EINVAL;
>>>> +
>>>> +  if (state->target_residency < state->exit_latency)
>>>> +          return -EINVAL;
>>>> +
>>>> +  if (!prev_state)
>>>> +          return 0;
>>>> +
>>>> +  if (state->exit_latency <= prev_state->exit_latency)
>>>> +          return -EINVAL;
>>>> +
>>>> +  if (state->target_residency <= prev_state->target_residency)
>>>> +          return -EINVAL;
>>>
>>> I'm not an expert on C-states, but isn't this checking against the
>>> previous value kind of defeating part of the purpose of the command
>>> line?
>>
>> I don't know. The question would need raising to the author.
>>
>>> Also, it might help to also write down those limits in the command
>>> line documentation.
>>
>> What do you mean there? Some of the values are universal, but some
>> checks are against model-specific values. I don't think you mean to
>> enumerate them all?
> 
> Maybe it's indeed not very useful.  What I referring to was something
> along the lines of: "the command line provided residency and latency
> values must be smaller than the default ones".  As noted above it
> seems weird to me than higher than current values cannot be set,
> albeit I have no idea what's the expected usage of this interface.

Hmm, while meaning to make this change I came to wonder: What exactly do
you refer to by "current values" and "default ones"? prev_state here
isn't "previous state" as in "before this option was parsed", but as in
"next lower C-state", as per

                prev_state = i ? &cmdline_states[i - 1] : NULL;

ahead of the call site.

Instead what I'm inclined to do (despite deviating from the original) is
to constify the function's parameters.

Jan