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

[Jie Zhan <zhanjie9@hisilicon.com>]

On 5/15/2026 6:39 PM, Jie Zhan wrote:
> 
> 
> 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
Hi Yaxiong,

A follow-up after digging deeper.

1. To be clear, the try_module_get() / module_put() pairs in
try_then_request_governor() and devfreq_set_governor() protect different
ranges.  Assuming no FORCE_UNLOAD, the former protects the governor module
from the time it's queried until it's set as the working governor, while
the latter protects when the governor is in use.

However, I realized the former protection is no longer needed since I
removed the governor list lock in v2.  devfreq_remove_governor() and the
'request and set' governor sequence are already synchronized by
'devfreq_list_lock'. Thus, patch 6 should be dropped.

2. devfreq_add_device() needs error prints because it's a probe path.
governor_store() doesn't want error prints since it's not a good practice
to print on a sysfs read/write.  Merging the two functions will violate
either of them.

Thanks,
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;