On Mon, Sep 15, 2025 at 9:29 AM Shawn Guo shawnguo2@yeah.net wrote:
On Sun, Sep 14, 2025 at 06:43:26PM +0100, Qais Yousef wrote:
Why do you want to address the issue in the cpufreq core instead of doing that in the cpufreq-dt driver?
My intuition was to fix the regression at where the regression was introduced by recovering the code behavior.
Isn't the right fix here is at the driver level still? We can only give drivers what they ask for. If they ask for something wrong and result in something wrong, it is still their fault, no?
I'm not sure. The cpufreq-dt driver is following suggestion to use CPUFREQ_ETERNAL,
Fair enough.
Actually, there are a few other drivers that fall back to CPUFREQ_ETERNAL if they cannot determine transition_latency.
which has the implication that core will figure out a reasonable default value for platforms where the latency is unknown.
Is this expectation realistic, though? I'm not sure.
The core can only use a hard-coded default fallback number, but would that number be really suitable for all of the platforms in question?
And that was exactly the situation before the regression. How does it become the fault of cpufreq-dt driver?
The question is not about who's fault it is, but what's the best place to address this issue.
I think that addressing it in cpufreq_policy_transition_delay_us() is a bit confusing because it is related to initialization and the new branch becomes pure overhead for the drivers that don't set cpuinfo.transition_latency to CPUFREQ_ETERNAL.
However, addressing it at the initialization time would effectively mean that the core would do something like:
if (policy->cpuinfo.transition_latency == CPUFREQ_ETERNAL) policy->cpuinfo.transition_latency = CPUFREQ_DEFAULT_TANSITION_LATENCY_NS;
but then it would be kind of more straightforward to update everybody using CPUFREQ_ETERNAL to set cpuinfo.transition_latency to CPUFREQ_DEFAULT_TANSITION_LATENCY_NS directly (and then get rid of CPUFREQ_ETERNAL entirely).
Alternatively maybe we can add special handling for CPUFREQ_ETERNAL value, though I'd suggest to return 1ms (similar to the case of value being 0). Maybe we can redefine CPUFREQ_ETERNAL to be 0, but not sure if this can have side effects.
Changing CPUFREQ_ETERNAL to 0 looks so risky to me. What about adding an explicit check for CPUFREQ_ETERNAL?
---8<---
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index fc7eace8b65b..053f3a0288bc 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -549,11 +549,15 @@ unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy) if (policy->transition_delay_us) return policy->transition_delay_us;
if (policy->cpuinfo.transition_latency == CPUFREQ_ETERNAL)goto default_delay;
Can't USEC_PER_MSEC be just returned directly from here?
latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC; if (latency) /* Give a 50% breathing room between updates */ return latency + (latency >> 1);
Side note for self: The computation above can be done once at the policy initialization time and transition_latency can be stored in us (and only converted to ns when the corresponding sysfs attribute is read). It can be even set to USEC_PER_MSEC if zero.
+default_delay: return USEC_PER_MSEC; } EXPORT_SYMBOL_GPL(cpufreq_policy_transition_delay_us);
--->8---