Re: [PATCH 1/1] cpuidle: add per-CPU latency_limit_ns sysfs attribute

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] cpuidle: redundant sysfs UAPI bypasses PM QoS framework
- [Medium] cpuidle: TOCTOU data race in latency_limit_ns evaluation
- [Medium] cpuidle: deferred enforcement of latency_limit_ns due to missing wakeup
--

commit 392083e97e8c2f4fec9d7c53da59689554ffca38
Author: Anthony Harivel <aharivel@redhat.com>
cpuidle: add per-CPU latency_limit_ns sysfs attribute

This commit introduces a new per-CPU sysfs attribute, latency_limit_ns, to
set an upper bound on exit latency for idle states. It modifies
cpuidle_governor_latency_req() to cap the latency constraint using this new
value.

> diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
> --- a/drivers/cpuidle/governor.c
> +++ b/drivers/cpuidle/governor.c
[ ... ]
> @@ -119,5 +121,11 @@ s64 cpuidle_governor_latency_req(unsigned int cpu)
>  	if (device_req > global_req)
>  		device_req = global_req;
>  
> -	return (s64)device_req * NSEC_PER_USEC;
> +	result = (s64)device_req * NSEC_PER_USEC;
> +
> +	dev = per_cpu(cpuidle_devices, cpu);
> +	if (dev && dev->latency_limit_ns && dev->latency_limit_ns < result)
> +		result = dev->latency_limit_ns;

[Severity: Medium]
Is it possible that this introduces a duplicate per-CPU sysfs attribute for
capping exit latency?

The exact capability seems to already exist via the PM QoS framework
(/sys/devices/system/cpu/cpuN/power/pm_qos_resume_latency_us).

By creating an uncoordinated interface, could this bypass PM QoS tracking and
hide the constraint from other subsystems that query PM QoS?

[Severity: Medium]
Could this result in TOCTOU data races or torn reads since
dev->latency_limit_ns is evaluated locklessly three times without READ_ONCE()?

If a concurrent sysfs write increases the value between the bounds check and
the assignment, might it bypass the PM QoS limit constraint, allowing the CPU
to incorrectly enter a deeper idle state?

Also, on 32-bit architectures, could these unprotected 64-bit reads and writes
be torn?

> +
> +	return result;
>  }
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
[ ... ]
> @@ -207,8 +207,44 @@ static void cpuidle_sysfs_release(struct kobject *kobj)
>  	complete(&kdev->kobj_unregister);
>  }
>  
> +static ssize_t show_latency_limit_ns(struct cpuidle_device *dev, char *buf)
> +{
> +	return sysfs_emit(buf, "%llu\n", dev->latency_limit_ns);
> +}
> +
> +static ssize_t store_latency_limit_ns(struct cpuidle_device *dev,
> +				       const char *buf, size_t count)
> +{
> +	u64 value;
> +	int err;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	err = kstrtou64(buf, 0, &value);
> +	if (err)
> +		return err;
> +
> +	dev->latency_limit_ns = value;
> +
> +	return count;

[Severity: Medium]
When this limit is updated, should we be sending an IPI or wakeup to the
target CPU?

If the CPU is currently in a deep idle state with an exit latency greater than
the new limit, it might not exit the C-state until the next hardware interrupt.

This seems to defer the enforcement of the latency constraint, whereas PM QoS
normally handles this by calling wake_up_all_idle_cpus().

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915131310.1053834-2-aharivel@redhat.com?part=1