* [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
@ 2018-06-15 10:04 Enric Balletbo i Serra
2018-06-17 3:50 ` Chanwoo Choi
0 siblings, 1 reply; 8+ messages in thread
From: Enric Balletbo i Serra @ 2018-06-15 10:04 UTC (permalink / raw)
To: linux-kernel; +Cc: kernel, Chanwoo Choi, Kyungmin Park, MyungJoo Ham, linux-pm
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
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
0 siblings, 1 reply; 8+ messages in thread
From: Chanwoo Choi @ 2018-06-17 3:50 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: linux-kernel, kernel, Chanwoo Choi, Kyungmin Park, MyungJoo Ham,
Linux PM list
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().
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
2018-06-17 3:50 ` Chanwoo Choi
@ 2018-06-18 9:02 ` Enric Balletbo Serra
2018-06-19 8:22 ` Enric Balletbo i Serra
0 siblings, 1 reply; 8+ messages in thread
From: Enric Balletbo Serra @ 2018-06-18 9:02 UTC (permalink / raw)
To: cwchoi00
Cc: Enric Balletbo i Serra, linux-kernel, kernel, Chanwoo Choi,
Kyungmin Park, MyungJoo Ham, Linux PM list
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.
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
2018-06-18 9:02 ` Enric Balletbo Serra
@ 2018-06-19 8:22 ` Enric Balletbo i Serra
2018-06-20 0:47 ` Chanwoo Choi
0 siblings, 1 reply; 8+ messages in thread
From: Enric Balletbo i Serra @ 2018-06-19 8:22 UTC (permalink / raw)
To: Enric Balletbo Serra, cwchoi00
Cc: linux-kernel, kernel, Chanwoo Choi, Kyungmin Park, MyungJoo Ham,
Linux PM list
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
2018-06-19 8:22 ` Enric Balletbo i Serra
@ 2018-06-20 0:47 ` Chanwoo Choi
2018-06-20 10:32 ` Enric Balletbo i Serra
0 siblings, 1 reply; 8+ messages in thread
From: Chanwoo Choi @ 2018-06-20 0:47 UTC (permalink / raw)
To: Enric Balletbo i Serra, Enric Balletbo Serra, cwchoi00
Cc: linux-kernel, kernel, Kyungmin Park, MyungJoo Ham, Linux PM list
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
2018-06-20 0:47 ` Chanwoo Choi
@ 2018-06-20 10:32 ` Enric Balletbo i Serra
2018-06-21 7:58 ` Chanwoo Choi
0 siblings, 1 reply; 8+ messages in thread
From: Enric Balletbo i Serra @ 2018-06-20 10:32 UTC (permalink / raw)
To: Chanwoo Choi, Enric Balletbo Serra, cwchoi00
Cc: linux-kernel, kernel, Kyungmin Park, MyungJoo Ham, Linux PM list
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@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?
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@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
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
2018-06-20 10:32 ` Enric Balletbo i Serra
@ 2018-06-21 7:58 ` Chanwoo Choi
2018-06-21 8:04 ` Enric Balletbo Serra
0 siblings, 1 reply; 8+ messages in thread
From: Chanwoo Choi @ 2018-06-21 7:58 UTC (permalink / raw)
To: Enric Balletbo i Serra, Enric Balletbo Serra, cwchoi00
Cc: linux-kernel, kernel, Kyungmin Park, MyungJoo Ham, Linux PM list
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 <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()
>
> 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.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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
2018-06-21 7:58 ` Chanwoo Choi
@ 2018-06-21 8:04 ` Enric Balletbo Serra
0 siblings, 0 replies; 8+ messages in thread
From: Enric Balletbo Serra @ 2018-06-21 8:04 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Enric Balletbo i Serra, cwchoi00, linux-kernel, kernel,
Kyungmin Park, MyungJoo Ham, Linux PM list
Hi Chanwoo,
Missatge de Chanwoo Choi <cw00.choi@samsung.com> del dia dj., 21 de
juny 2018 a les 9:58:
>
> 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 <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()
> >
> > 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.
>
Not a problem :) I learned while we discussed regarding the different
options. I will send a v2 then
Thanks,
Enric
> >
> > 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.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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-06-21 8:04 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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.