From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
To: Enric Balletbo Serra <eballetbo@gmail.com>, cwchoi00@gmail.com
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
kernel@collabora.com, Chanwoo Choi <cw00.choi@samsung.com>,
Kyungmin Park <kyungmin.park@samsung.com>,
MyungJoo Ham <myungjoo.ham@samsung.com>,
Linux PM list <linux-pm@vger.kernel.org>
Subject: Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
Date: Tue, 19 Jun 2018 10:22:52 +0200 [thread overview]
Message-ID: <7eff02c1-3d78-de08-19ab-a4a45c491a86@collabora.com> (raw)
In-Reply-To: <CAFqH_5344NDHup+u0QtimTJjKaVVj46YB6UDMKtdMr70JJmkng@mail.gmail.com>
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) {
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
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
>
next prev parent reply other threads:[~2018-06-19 8:23 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-15 10:04 [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules Enric Balletbo i Serra
2018-06-17 3:50 ` Chanwoo Choi
2018-06-18 9:02 ` Enric Balletbo Serra
2018-06-19 8:22 ` Enric Balletbo i Serra [this message]
2018-06-20 0:47 ` Chanwoo Choi
2018-06-20 10:32 ` Enric Balletbo i Serra
2018-06-21 7:58 ` Chanwoo Choi
2018-06-21 8:04 ` Enric Balletbo Serra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7eff02c1-3d78-de08-19ab-a4a45c491a86@collabora.com \
--to=enric.balletbo@collabora.com \
--cc=cw00.choi@samsung.com \
--cc=cwchoi00@gmail.com \
--cc=eballetbo@gmail.com \
--cc=kernel@collabora.com \
--cc=kyungmin.park@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=myungjoo.ham@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.