| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 20 Jun 2018 09:47:58 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: Enric Balletbo i Serra <enric.balletbo@...labora.com>,
Enric Balletbo Serra <eballetbo@...il.com>,
cwchoi00@...il.com
Cc: linux-kernel <linux-kernel@...r.kernel.org>, kernel@...labora.com,
Kyungmin Park <kyungmin.park@...sung.com>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Linux PM list <linux-pm@...r.kernel.org>
Subject: Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are
built as modules.
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@...il.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@...labora.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@...labora.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
Powered by blists - more mailing lists