Re: [PATCH v3 3/4] cpufreq: Remove driver default policy->min/max init

[Pierre Gondois <pierre.gondois@arm.com>]

Hello Rafael,

On 6/8/26 18:50, Rafael J. Wysocki wrote:
> Hi,
>
> On Wed, Jun 3, 2026 at 9:49 AM Pierre Gondois <pierre.gondois@arm.com> wrote:
>> Hello Rafael,
>>
>> On 6/1/26 20:08, Rafael J. Wysocki wrote:
>>> On Thu, May 28, 2026 at 11:10 AM Pierre Gondois <pierre.gondois@arm.com> wrote:
>>>> Prior to [1], drivers were setting policy->min/max and
>>>> the value was used as a QoS constraint. After that change,
>>>> the values were only temporarily used: cpufreq_set_policy()
>>>> ultimately overriding them through:
>>>> cpufreq_policy_online()
>>>> \-cpufreq_init_policy()
>>>>     \-cpufreq_set_policy()
>>>>       \-/* Set policy->min/max */
>>>>
>>>> This patch reinstate the initial behaviour. This will allow
>>>> drivers to request min/max QoS frequencies if desired.
>>>> For instance, the cppc driver advertises a lowest non-linear
>>>> frequency, which should be used as a min QoS value.
>>>>
>>>> To avoid having drivers setting policy->min/max to default
>>>> values which are considered as QoS values (i.e. the reason
>>>> why [1] was introduced), remove the initialization of
>>>> policy->min/max in .init() callbacks wherever the
>>>> policy->min/max values are identical to the
>>>> policy->cpuinfo.min/max_freq.
>>>>
>>>> Indeed, the previous patch ("cpufreq: Set default
>>>> policy->min/max values for all drivers") makes this initialization
>>>> redundant.
>>>>
>>>> The only drivers where these values are different are:
>>>> - gx-suspmod.c (min)
>>>> - cppc-cpufreq.c (min)
>>>> - longrun.c
>>>>
>>>> [1]
>>>> commit 521223d8b3ec ("cpufreq: Fix initialization of min and
>>>> max frequency QoS requests")
>>>>
>>>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>>>> Acked-by: Jie Zhan <zhanjie9@hisilicon.com>
>>> sashiko.dev has some feedback on this patch and appears to have a point:
>>>
>>> https://sashiko.dev/#/patchset/20260528090913.2759118-1-pierre.gondois%40arm.com
>>>
>>> Can you have a look at it please?
>>>
>> [sashiko]
>>
>>   > Does removing the policy->max = max_freq assignment here break UAPI
>>   > expectations by exposing the unlisted boost frequency in
>> scaling_max_freq?
>>   >
>>   > Commit 538b0188da4653 intentionally allowed drivers like acpi-cpufreq
>> to set
>>   > policy->cpuinfo.max_freq to a higher boost frequency while relying on
>>   > cpufreq_frequency_table_cpuinfo() to clamp policy->max to the frequency
>>   > table's nominal maximum (max_freq). This ensured that user-space
>> tools saw
>>   > the nominal maximum in scaling_max_freq.
>>   >
>>   > Although commit 521223d8b3ec temporarily disrupted this by defaulting
>> the QoS
>>   > max to -1, a subsequent patch in this series changes the core to
>> initialize
>>   > the QoS request using policy->max.
>>
>> Effectively PATCH [4/4] cpufreq: Use policy->min/max init as QoS request
>> now uses the policy->max value set by the .init() callback to set
>> the max_freq_req QoS constraint.
>>
>>   >
>>   > If the policy->max = max_freq assignment were preserved, the subsequent
>>   > patch would successfully use the nominal frequency as the QoS max
>> request,
>>   > restoring the correct clamping behavior.
>>
>> IIUC this suggests to use the nominal freq. as the QoS max request.
>> This was behaving like that prior to 521223d8b3ec. However doing
>> that would mean that if boost is enabled and the max_freq_req sysfs
>> is not updated, then the frequency would still be clamped by
>> the max_freq_req. 521223d8b3ec intended to correct that.
>>
>> Sashiko seems to suggest modifications to come back to the
>> pre-521223d8b3ec behaviour, but I think 521223d8b3ec is correct
>> and we should conserve this behaviour.
> So there is some confusion in the patch changelogs of this series, but
> not in the code, regarding the role of the last argument of
> freq_qos_add_request().  Namely, that argument is the initial request
> value for the given request object which is subsequently managed by
> user space.  User space may in fact change it to whatever value it
> wants (either lower or higher) and it is only taken into account along
> with the other requests in the given chain.  IMV it is better to
> clarify that, so I have updated the changelogs when applying the
> patches.
>
> Please see
>
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=8c83947c5dbbd49b36d08bb99e344327c6278781
>
> and its ancestors and let me know if there's anything missing in the
> changelogs thereof.

The new commit message is indeed clearer.
Thanks for the update.

Regards,

Pierre


> Thanks!