[PATCH v3 3/4] devfreq: Factor out devfreq_set_governor()

[Jie Zhan]

On 5/21/2026 4:06 PM, zhenglifeng (A) wrote:
> On 5/19/2026 7:32 PM, Jie Zhan wrote:
>> governor_store() and devfreq_add_device() contain similar logic for setting
>> a governor when devfreq->governor is NULL.  Merge this into a common
>> function, devfreq_set_governor(), to reduce code duplication and unify the
>> entry of setting governors.
>>
>> This also prepares for further changes that get / put a module refcount of
>> the active governor and prevent the governor module from being unloaded
>> while it's in use.
>>
>> Signed-off-by: Jie Zhan <zhanjie9 at hisilicon.com>
>> ---
>>  drivers/devfreq/devfreq.c | 117 ++++++++++++++++++--------------------
>>  1 file changed, 56 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 53c40d795a13..9e3e6a7348f8 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -318,6 +318,59 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>>  	return governor;
>>  }
>>  
>> +static int devfreq_set_governor(struct devfreq *df,
>> +				const struct devfreq_governor *new_gov)
>> +{
>> +	const struct devfreq_governor *old_gov;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	lockdep_assert_held(&devfreq_list_lock);
>> +
>> +	old_gov = df->governor;
>> +	dev = &df->dev;
>> +
>> +	if (old_gov) {
>> +		if (old_gov == new_gov)
>> +			return 0;
>> +
>> +		if (IS_SUPPORTED_FLAG(old_gov->flags, IMMUTABLE) ||
>> +		    IS_SUPPORTED_FLAG(new_gov->flags, IMMUTABLE))
>> +			return -EINVAL;
>> +
>> +		/* Stop the current governor */
>> +		ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
>> +		if (ret) {
>> +			dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
>> +				 __func__, df->governor->name, ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/* Start the new governor */
>> +	df->governor = new_gov;
>> +	ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>> +	if (ret) {
>> +		dev_warn(dev, "%s: Governor %s not started(%d)\n",
>> +			 __func__, df->governor->name, ret);
>> +
>> +		/* Restore previous governor */
>> +		df->governor = old_gov;
>> +		if (!df->governor)
>> +			return ret;
>> +
>> +		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>> +		if (ret) {
>> +			dev_err(dev, "%s: restore Governor %s failed (%d)\n",
>> +				__func__, df->governor->name, ret);
>> +			df->governor = NULL;
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return sysfs_update_group(&df->dev.kobj, &gov_attr_group);
> 
> Here is a little problem which is not introduced by this patch. When
> starting new governor fails but restoring old governor succeeds, this
> function will return 0. This might lead users to believe that the governor
> has been successfully modified.
> 
Yes, good catch.  This suppresses the error code.

I think, from a quick glance, the path of restoring the old governor should
discard the return value of event_handler() and skip updating sysfs.