Re: [PATCH v5 1/5] cpufreq/amd-pstate: Add dynamic energy performance preference

[Gautham R. Shenoy]

Hello Mario,


On Mon, Jan 05, 2026 at 11:14:37PM -0600, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@xxxxxxx>
> 
> Dynamic energy performance preference will change the EPP profile
> based on whether the machine is running on AC or DC power.
> 
> A notification chain from the power supply core is used to adjust
> EPP values on plug in or plug out events.
> 
> For non-server systems:
>     * the default EPP for AC mode is `performance`.
>     * the default EPP for DC mode is `balance_performance`.

In addition to these, the commit also adds a sysfs toggle, blocks
manual EPP, forces PERFORMANCE policy.

Might be good to mention these in the commit log.

> 
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>

[..snip..]


> +static int amd_pstate_set_dynamic_epp(struct cpufreq_policy *policy)
> +{
> +	struct amd_cpudata *cpudata = policy->driver_data;
> +	int ret;
> +	u8 epp;
> +
> +	epp = amd_pstate_get_balanced_epp(policy);
> +	ret = amd_pstate_set_epp(policy, epp);
> +	if (ret)
> +		return ret;
> +
> +	/* only enable notifier if things will actually change */
> +	if (cpudata->epp_default_ac != cpudata->epp_default_dc) {
> +		ret = power_supply_reg_notifier(&cpudata->power_nb);
> +		if (ret)
> +			goto cleanup;
> +		cpudata->power_nb.notifier_call = amd_pstate_power_supply_notifier;
> +	}
> +
> +	cpudata->dynamic_epp = true;
> +
> +	return 0;
> +
> +cleanup:
> +	amd_pstate_clear_dynamic_epp(policy);
> +
> +	return ret;
> +}


Can the notifier_call assignment and registration be reordered?

The cpudata struct is allocated with kzalloc, so
power_nb.notifier_call is NULL when power_supply_reg_notifier() is
called.  power_supply_reg_notifier() calls
blocking_notifier_chain_register(), which adds the notifier block to
the chain and then releases the rwsem.

If power_supply_changed_work() fires between the registration and the
assignment, the notification path reaches notifier_call_chain():

kernel/notifier.c:notifier_call_chain() {
    ...
    ret = nb->notifier_call(nb, val, v);
    ...
}

At this point notifier_call is still NULL, resulting in a NULL pointer
dereference.  Setting notifier_call before calling
power_supply_reg_notifier() would close this window.



> +
>  /* Sysfs attributes */
>  

[..snip..]

>  
>  static struct freq_attr *amd_pstate_attr[] = {
>  	&amd_pstate_max_freq,
> @@ -1421,6 +1528,7 @@ static struct freq_attr *amd_pstate_epp_attr[] = {
>  static struct attribute *pstate_global_attributes[] = {
>  	&dev_attr_status.attr,
>  	&dev_attr_prefcore.attr,
> +	&dev_attr_dynamic_epp.attr,
>  	NULL
>  };
>  
> @@ -1512,15 +1620,20 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	if (amd_pstate_acpi_pm_profile_server() ||
>  	    amd_pstate_acpi_pm_profile_undefined()) {
>  		policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> -		cpudata->epp_default = amd_pstate_get_epp(cpudata);
> +		cpudata->epp_default_ac = cpudata->epp_default_dc = amd_pstate_get_epp(cpudata);

I wonder if we can set dynamic_epp to false for Server platforms.

The reason being that there are Server users who switch to the
powersave governor via a systemd startup script to set the right EPP.

With dynamic_epp = true, this is no longer possible. The user has to
first disable dynamic_epp and then switch to the powersave govenor.

Or perhaps you have a commandline override to disable dynamic_epp at boot time ?


-- 
Thanks and Regards
gautham.