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

[Chanwoo Choi <cw00.choi@samsung.com>]

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@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()
			mutex_lock()	
		}

	}


> 
> Other suggestions?
> 
> - Enric
> 
>> Thanks
>>  Enric.
>>
>>
>>> 2018-06-15 19:04 GMT+09:00 Enric Balletbo i Serra
>>> <enric.balletbo@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.balletbo@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