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

[Jan Beulich <jbeulich@suse.com>]

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?

Jan