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

Hi Chanwoo,
Missatge de Chanwoo Choi [off-list ref] del dia dj., 21 de
juny 2018 a les 9:58:

Hi Enric,

On 2018년 06월 20일 19:32, Enric Balletbo i Serra wrote:

quoted

Hi Chanwoo,

On 20/06/18 02:47, Chanwoo Choi wrote:

quoted

Hi Enric,

On 2018년 06월 19일 17:22, Enric Balletbo i Serra wrote:

quoted

Hi Chanwoo,

On 18/06/18 11:02, Enric Balletbo Serra wrote:

quoted

Hi Chanwoo,
Missatge de Chanwoo Choi [off-list ref] del dia dg., 17 de juny
2018 a les 5:50:

quoted

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.

quoted

     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

quoted

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.

quoted

store_scaling_governor
  -> cpu_parse_governor
     -> request_module

cpufreq_add_dev_interface
  -> cpu_freq_init_policy
     -> cpu_parse_governor
        -> request_module

Thanks,
- Enric

quoted

                     mutex_lock()
             }

     }

quoted

Other suggestions?

- Enric

quoted

Thanks
 Enric.

quoted

2018-06-15 19:04 GMT+09:00 Enric Balletbo i Serra
[off-list ref]:

quoted

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 <redacted>
---

 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