Re: [PATCH v3 4/4] devfreq: Refcount governor modules while in use

On 6/1/2026 10:03 AM, Jie Zhan wrote:
> 
> 
> On 5/21/2026 4:13 PM, zhenglifeng (A) wrote:
>> On 5/19/2026 7:32 PM, Jie Zhan wrote:
>>> A governor module can be inserted or removed dynamically when built as a
>>> kernel module.  'devfreq->governor' would become NULL if the governor
>>> module is removed when it's in use.
>>>
>>> Add a refcount mechanism for governor modules to prevent the governor
>>> module from being removed (except for force unload):
>>> 1. Add an optional 'owner' member to struct devfreq_governor so the devfreq
>>>    core can identify the module that holds the governor code.
>>> 2. Get and put a refcount of the governor module when starting and stopping
>>>    the governor.
>>>
>>> The new 'owner' field is optional:
>>> - Common governor modules (performance, powersave, simple_ondemand,
>>>   userspace, passive) set 'owner' to THIS_MODULE.
>>> - Governors that are bundled into a device driver module must leave 'owner'
>>>   NULL.  The device's lifetime already pins that module, and setting
>>>   'owner' would create a self-reference that blocks the driver from being
>>>   unloaded.
>>>
>>> As a result, a non-forced rmmod of an in-use stand-alone governor now
>>> fails with -EBUSY, e.g.:
>>>
>>>   $ cat governor
>>>   performance
>>>   $ rmmod governor_performance
>>>   rmmod: ERROR: Module governor_performance is in use
>>>
>>> Force unloads (rmmod -f, if configured) can't be blocked.
>>>
>>> Signed-off-by: Jie Zhan <[email protected]>
>>> ---
>>>  drivers/devfreq/devfreq.c                 | 17 ++++++++++++++++-
>>>  drivers/devfreq/governor_passive.c        |  1 +
>>>  drivers/devfreq/governor_performance.c    |  1 +
>>>  drivers/devfreq/governor_powersave.c      |  1 +
>>>  drivers/devfreq/governor_simpleondemand.c |  1 +
>>>  drivers/devfreq/governor_userspace.c      |  1 +
>>>  include/linux/devfreq-governor.h          | 11 +++++++++++
>>>  7 files changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 9e3e6a7348f8..1cee43636ded 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -345,24 +345,37 @@ static int devfreq_set_governor(struct devfreq *df,
>>>  				 __func__, df->governor->name, ret);
>>>  			return ret;
>>>  		}
>>> +		module_put(old_gov->owner);
>>>  	}
>>>  
>>>  	/* Start the new governor */
>>> +	if (!try_module_get(new_gov->owner)) {
>>> +		df->governor = NULL;
>>
>> Shouldn't the old governor be restored here?
>>
> Yeah.  Perhaps we just move try_module_get() before stopping the old
> governor so as to avoid another hunk of restoring governor.

Follow up (after a long while though):
I found it quite hard to restore the old governor here while keep the code
clear.  I'd suggest that leaving it NULL for this rare error path.  The
current devfreq-next code is able to handle a NULL df->governor.

Thanks!
Jie