Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

From: Enric Balletbo i Serra
Date: Wed Jun 20 2018 - 06:32:47 EST

Next message: Alexandre Belloni: "Re: [PATCH v5 2/6] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks"
Previous message: Thomas Gleixner: "Re: [PATCH v10 7/7] x86/tsc: use tsc early"
In reply to: Chanwoo Choi: "Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules."
Next in thread: Chanwoo Choi: "Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Chanwoo,

On 20/06/18 02:47, Chanwoo Choi wrote:
> Hi Enric,
>
> On 2018ë 06ì 19ì 17:22, Enric Balletbo i Serra wrote:
>> Hi Chanwoo,
>>
>> On 18/06/18 11:02, Enric Balletbo Serra wrote:
>>> Hi Chanwoo,
>>> Missatge de Chanwoo Choi <cwchoi00@xxxxxxxxx> del dia dg., 17 de juny
>>> 2018 a les 5:50:
>>>>
>>>> Hi Enric,
>>>>
>>>> This issue will happen on the position to use find_devfreq_governor()
>>>> as following:
>>>> - devfreq_add_governora() and governor_store()
>>>>
>>>> If device driver with module type after loaded want to change the
>>>> scaling governor,
>>>> new governor might be not yet loaded. So, devfreq bettero to consider this case
>>>> in the find_devfreq_governor().
>>>>
>>> Ok, I'll move there and send a v2.
>>>
>>
>> I tried your suggestion but I found one problem, if I move the code in
>> find_devfreq_governor it end up with a deadlock. The reason is the following calls.
>>
>> devfreq_add_device
>> find_devfreq_governor (!!!)
>> request_module
>> devfreq_simple_ondemand_init
>> devfreq_add_governor
>> find_devfreq_governor (DEADLOCK)
>>
>> So I am wondering if shouldn't be more easy fix the issue in both places,
>> devfreq_add_device and governor_store.
>>
>> To devfreq_add_device
>>
>> devfreq_add_device
>> governor = find_devfreq_governor
>> if (IS_ERR(governor) {
>
> In this error case, you have to unlock the mutex
> before calling the request_module(). I added the pseudo code
> of my opinion.
>
>> request_module
>> governor = find_devfreq_governor
>> if (IS_ERR(governor)
>> return ERR_PTR(governor)
>> }
>>
>> And the same for governor_store
>>
>> governor_store
>> governor = find_devfreq_governor
>> if (IS_ERR(governor) {
>> request_module
>> governor = find_devfreq_governor
>> if (IS_ERR(governor)
>> return ERR_PTR(governor)
>> }
>>
>> Maybe all can go in a new function try_find_devfreq_governor_then_request
>
> How about modify the find_devfreq_governor() as following?
> I think that it is possible because previous find_devfreq_governor()
> always check whether mutex is locked or not.
>
> find_devfreq_governor() {
>
> // check whether mutex is locked or not
> if (!mutex_is_lock(&devfreq_list_lock)) {
> WARN(...)
> return -EINVAL
> }
>
> // find the registered governor with list_for_each_entry
>
> if (governor is not loaded) {
> mutex_unlock()
> request_module()

Then the problem is that the find_devfreq_governor is reentrant because the init
function of the governor calls devfreq_add_governor and find_devfreq_governor
again. E.g for simpleondemand governor you will get this loop.

find_devfreq_governor
-> request_module
-> devfreq_simple_ondemand_init
-> devfreq_add_governor
-> find_devfreq_governor
-> request_module
-> devfreq_simple_ondemand_init
-> devfreq_add_governor
-> find_devfreq_governor
-> request_module
...

Makes sense or I am missing something and there is a way to quit from this loop?

FWIW I checked how the cpufreq driver does this as it should have the same
problem. The find_governor function is just a simple search and instead of
integrating the request_module inside the find_governor function they have a
cpu_parse_governor that calls request module from the userspace call and from
the init call.

store_scaling_governor
-> cpu_parse_governor
-> request_module

cpufreq_add_dev_interface
-> cpu_freq_init_policy
-> cpu_parse_governor
-> request_module

Thanks,
- Enric

> mutex_lock()
> }
>
> }
>
>
>>
>> Other suggestions?
>>
>> - Enric
>>
>>> Thanks
>>> Enric.
>>>
>>>
>>>> 2018-06-15 19:04 GMT+09:00 Enric Balletbo i Serra
>>>> <enric.balletbo@xxxxxxxxxxxxx>:
>>>>> When the devfreq driver and the governor driver are built as modules,
>>>>> the call to devfreq_add_device() fails because the governor driver is
>>>>> not loaded at the time the devfreq driver loads. The devfreq driver has
>>>>> a build dependency on the governor but also should have a runtime
>>>>> dependency. We need to make sure that the governor driver is loaded
>>>>> before the devfreq driver.
>>>>>
>>>>> This patch fixes this bug in devfreq_add_device(). First tries to find
>>>>> the governor, and then, if was not found, it requests the module and
>>>>> tries again.
>>>>>
>>>>> Fixes: 1b5c1be2c88e (PM / devfreq: map devfreq drivers to governor using name)
>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>> drivers/devfreq/devfreq.c | 36 +++++++++++++++++++++++++++++++-----
>>>>> 1 file changed, 31 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index fe2af6aa88fc..1d917f043e30 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -11,6 +11,7 @@
>>>>> */
>>>>>
>>>>> #include <linux/kernel.h>
>>>>> +#include <linux/kmod.h>
>>>>> #include <linux/sched.h>
>>>>> #include <linux/errno.h>
>>>>> #include <linux/err.h>
>>>>> @@ -648,10 +649,35 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>>>>
>>>>> governor = find_devfreq_governor(devfreq->governor_name);
>>>>> if (IS_ERR(governor)) {
>>>>> - dev_err(dev, "%s: Unable to find governor for the device\n",
>>>>> - __func__);
>>>>> - err = PTR_ERR(governor);
>>>>> - goto err_init;
>>>>> + list_del(&devfreq->node);
>>>>> + mutex_unlock(&devfreq_list_lock);
>>>>> +
>>>>> + /*
>>>>> + * If the governor is not found, then request the module and
>>>>> + * try again. This can happen when both drivers (the governor
>>>>> + * driver and the driver that calls devfreq_add_device) are
>>>>> + * built as modules.
>>>>> + */
>>>>> + if (!strncmp(devfreq->governor_name,
>>>>> + DEVFREQ_GOV_SIMPLE_ONDEMAND, DEVFREQ_NAME_LEN))
>>>>> + err = request_module("governor_%s", "simpleondemand");
>>>>> + else
>>>>> + err = request_module("governor_%s",
>>>>> + devfreq->governor_name);
>>>>> + if (err)
>>>>> + goto err_unregister;
>>>>> +
>>>>> + mutex_lock(&devfreq_list_lock);
>>>>> + list_add(&devfreq->node, &devfreq_list);
>>>>> +
>>>>> + governor = find_devfreq_governor(devfreq->governor_name);
>>>>> + if (IS_ERR(governor)) {
>>>>> + dev_err(dev,
>>>>> + "%s: Unable to find governor for the device\n",
>>>>> + __func__);
>>>>> + err = PTR_ERR(governor);
>>>>> + goto err_init;
>>>>> + }
>>>>> }
>>>>>
>>>>> devfreq->governor = governor;
>>>>> @@ -669,7 +695,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>>>> err_init:
>>>>> list_del(&devfreq->node);
>>>>> mutex_unlock(&devfreq_list_lock);
>>>>> -
>>>>> +err_unregister:
>>>>> device_unregister(&devfreq->dev);
>>>>> err_dev:
>>>>> if (devfreq)
>>>>> --
>>>>> 2.17.1
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Best Regards,
>>>> Chanwoo Choi
>>>> Samsung Electronics
>>>
>>
>>
>>
>
>

Next message: Alexandre Belloni: "Re: [PATCH v5 2/6] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks"
Previous message: Thomas Gleixner: "Re: [PATCH v10 7/7] x86/tsc: use tsc early"
In reply to: Chanwoo Choi: "Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules."
Next in thread: Chanwoo Choi: "Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]