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

[Yaxiong Tian]

在 2026/5/18 16:39, Jie Zhan 写道:
>
> 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 at 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
Okay, please ignore my suggestion.
>>>>        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;