Hi Enric,
On 2018년 06월 20일 19:32, Enric Balletbo i Serra wrote:
> 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 <cwcho...@gmail.com> 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?
You're right. Sorry, my wrong opinion steals your time.
>
> 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.
Also, I checked the cpufreq's case. We better to make the separate function
like cpufreq_parse_governor() in cpufreq subsystem.
>
> 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.balle...@collabora.com>:
>>>>>> 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.balle...@collabora.com>
>>>>>> ---
>>>>>>
>>>>>> 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
>>>>
>>>
>>>
>>>
>>
>>
>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
