Re: [PATCH v2 5/6] devfreq: Get and put module refcount when switching governor

[Jie Zhan <zhanjie9@hisilicon.com>]

On 5/14/2026 2:50 PM, Yaxiong Tian wrote:
> 
> 在 2026/5/13 17:38, Jie Zhan 写道:
>> A governor module can be dynamically inserted or removed if compiled as a
>> kernel module.  'devfreq->governor' would become NULL if the governor
>> module is removed when it's in use.
>>
>> To prevent the governor module from being removed (except for force
>> unload) when it's in use, get and put a refcount of the governor module
>> when starting and stopping the governor.
>>
>> As a result, unloading a governor module in use returns an error, e.g.:
>>    $ cat governor
>>    performance
>>    $ rmmod governor_performance
>>    rmmod: ERROR: Module governor_performance is in use
>>
>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
>> ---
>>   drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index e1363ab69173..2ea42325d030 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;
>> +        return -EINVAL;
>> +    }
>> +
> 
> Here, new_gov has already been checked in try_then_request_governor() for whether the module can be obtained.
> 
> I think it might be more reasonable to merge try_then_request_governor() and devfreq_set_governor(), so that when new_gov cannot be obtained, the operation of the old governor is not affected.
> 
Well yeah, indeed! I'll take a look at how to implement this.
Thanks for the suggestion.

Jie
>>       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);
>> +        module_put(new_gov->owner);
>>             /* Restore previous governor */
>>           df->governor = old_gov;
>>           if (!df->governor)
>>               return ret;
>>   +        if (!try_module_get(old_gov->owner)) {
>> +            df->governor = NULL;
>> +            return -EINVAL;
>> +        }
>> +
>>           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);
>> +            module_put(old_gov->owner);
>>               df->governor = NULL;
>>               return ret;
>>           }
>> @@ -1040,9 +1053,11 @@ int devfreq_remove_device(struct devfreq *devfreq)
>>         devfreq_cooling_unregister(devfreq->cdev);
>>   -    if (devfreq->governor)
>> +    if (devfreq->governor) {
>>           devfreq->governor->event_handler(devfreq,
>>                            DEVFREQ_GOV_STOP, NULL);
>> +        module_put(devfreq->governor->owner);
>> +    }
>>       device_unregister(&devfreq->dev);
>>         return 0;