Re: [PATCH] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.
From: Enric Balletbo Serra
Date: Thu Jun 21 2018 - 04:04:48 EST
Hi Chanwoo,
Missatge de Chanwoo Choi <cw00.choi@xxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxx>:
> >>>>>> 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@xxxxxxxxxxxxx>
> >>>>>> ---
> >>>>>>
> >>>>>> 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