| Message ID | 20180703132931.14389-1-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
>+ if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, >+ DEVFREQ_NAME_LEN)) >+ err = request_module("governor_%s", "simpleondemand"); >+ else >+ err = request_module("governor_%s", name); >+ if (err) >+ return NULL; You are returning without the lock acquired..
Hi Enric, On 2018년 07월 03일 22:29, Enric Balletbo i Serra wrote: > When the devfreq driver and the governor driver are built as modules, > the call to devfreq_add_device() or governor_store() 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 by adding a try_then_request_governor() > function. First tries to find the governor, and then, if it is 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> > --- > > Changes in v4: > - Kept "locked" devfreq_list from the return of find_devfreq_governor() to > the unlock of governor_store(). Requested by MyungJoo Ham. > > Changes in v3: > - Remove unneded change in dev_err message. > - Fix err returned value in case to not find the governor. > > Changes in v2: > - Add a new function to request the module and call that function from > devfreq_add_device and governor_store. > > drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 47 insertions(+), 6 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 0b5b3abe054e..4ea6b19879a1 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> > @@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) > return ERR_PTR(-ENODEV); > } > > +/** > + * try_then_request_governor() - Try to find the governor and request the > + * module if is not found. > + * @name: name of the governor Usually, devfreq used 'governor_name' indicating the name of governor. you better to use 'governor_name' instead of 'name' for more readability. > + * > + * Search the list of devfreq governors and request the module and try again > + * if is not found. This can happen when both drivers (the governor driver > + * and the driver that call devfreq_add_device) are built as modules. > + * devfreq_list_lock should be held by the caller. > + * > + * Return: The matched governor's pointer. Usually, devfreq.c didn;t use the 'Return: ...'. So, you better to explain what is returned from this function with function description. > + */ > +static struct devfreq_governor *try_then_request_governor(const char *name) ditto. (name -> governor_name) > +{ > + struct devfreq_governor *governor; > + int err = 0; You have to check whether governor name is NULL or not. if (IS_ERR_OR_NULL(name)) { pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); return ERR_PTR(-EINVAL); } > + > + WARN(!mutex_is_locked(&devfreq_list_lock), > + "devfreq_list_lock must be locked."); > + > + governor = find_devfreq_governor(name); > + if (IS_ERR(governor)) { > + mutex_unlock(&devfreq_list_lock); > + > + if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, > + DEVFREQ_NAME_LEN)) > + err = request_module("governor_%s", "simpleondemand"); > + else > + err = request_module("governor_%s", name); > + if (err) > + return NULL; When error happen, you unlock the mutex. If failed to request module, you should restore the previous state. Please mutex_lock(&devfreq_list_lock) before return. > + > + mutex_lock(&devfreq_list_lock); > + > + governor = find_devfreq_governor(name); > + } > + > + return governor; > +} > + > static int devfreq_notify_transition(struct devfreq *devfreq, > struct devfreq_freqs *freqs, unsigned int state) > { > @@ -643,11 +684,9 @@ struct devfreq *devfreq_add_device(struct device *dev, > srcu_init_notifier_head(&devfreq->transition_notifier_list); > > mutex_unlock(&devfreq->lock); > - This change is not related to this patch. > mutex_lock(&devfreq_list_lock); > - list_add(&devfreq->node, &devfreq_list); > > - governor = find_devfreq_governor(devfreq->governor_name); > + governor = try_then_request_governor(devfreq->governor_name); > if (IS_ERR(governor)) { > dev_err(dev, "%s: Unable to find governor for the device\n", > __func__); > @@ -663,14 +702,15 @@ struct devfreq *devfreq_add_device(struct device *dev, > __func__); > goto err_init; > } > + > + list_add(&devfreq->node, &devfreq_list); > + > mutex_unlock(&devfreq_list_lock); > > return devfreq; > > err_init: > - list_del(&devfreq->node); > mutex_unlock(&devfreq_list_lock); > - This change is not related to this patch. > device_unregister(&devfreq->dev); > err_dev: > if (devfreq) > @@ -989,7 +1029,8 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr, > return -EINVAL; > > mutex_lock(&devfreq_list_lock); > - governor = find_devfreq_governor(str_governor); > + Don't need to add the blank line. It is enough to change the function from find_devfreq_governor to try_then_request_governor. > + governor = try_then_request_governor(str_governor); > if (IS_ERR(governor)) { > ret = PTR_ERR(governor); > goto out; >
Hi Chanwoo, On 04/07/18 03:06, Chanwoo Choi wrote: > Hi Enric, > > On 2018년 07월 03일 22:29, Enric Balletbo i Serra wrote: >> When the devfreq driver and the governor driver are built as modules, >> the call to devfreq_add_device() or governor_store() 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 by adding a try_then_request_governor() >> function. First tries to find the governor, and then, if it is 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> >> --- >> >> Changes in v4: >> - Kept "locked" devfreq_list from the return of find_devfreq_governor() to >> the unlock of governor_store(). Requested by MyungJoo Ham. >> >> Changes in v3: >> - Remove unneded change in dev_err message. >> - Fix err returned value in case to not find the governor. >> >> Changes in v2: >> - Add a new function to request the module and call that function from >> devfreq_add_device and governor_store. >> >> drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++----- >> 1 file changed, 47 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 0b5b3abe054e..4ea6b19879a1 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> >> @@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) >> return ERR_PTR(-ENODEV); >> } >> >> +/** >> + * try_then_request_governor() - Try to find the governor and request the >> + * module if is not found. >> + * @name: name of the governor > > Usually, devfreq used 'governor_name' indicating the name of governor. > you better to use 'governor_name' instead of 'name' for more readability. > I don't mind to change if you want. But let me try to convince you first. I used name for two reasons: 1. I saw that you are using governor_name sometimes, but find_devfreq_governor uses name not governor_name. IMHO the function name in these two specific cases 'try_then_request_governor(name)' is enough readable. 2. If we want to use governor_name and then do not have the line exceeding the 80 characters I need to split the function in two lines. For me the readability is better when you have all in one line. If I did not convince you, just let me now and I'll change for governor_name :) >> + * >> + * Search the list of devfreq governors and request the module and try again >> + * if is not found. This can happen when both drivers (the governor driver >> + * and the driver that call devfreq_add_device) are built as modules. >> + * devfreq_list_lock should be held by the caller. >> + * >> + * Return: The matched governor's pointer. > > Usually, devfreq.c didn;t use the 'Return: ...'. So, you better to explain > what is returned from this function with function description. > OK. >> + */ >> +static struct devfreq_governor *try_then_request_governor(const char *name) > > ditto. (name -> governor_name) > I convinced you? ;) >> +{ >> + struct devfreq_governor *governor; >> + int err = 0; > > You have to check whether governor name is NULL or not. > > if (IS_ERR_OR_NULL(name)) { > pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); > return ERR_PTR(-EINVAL); > } > Right. >> + >> + WARN(!mutex_is_locked(&devfreq_list_lock), >> + "devfreq_list_lock must be locked."); >> + >> + governor = find_devfreq_governor(name); >> + if (IS_ERR(governor)) { >> + mutex_unlock(&devfreq_list_lock); >> + >> + if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, >> + DEVFREQ_NAME_LEN)) >> + err = request_module("governor_%s", "simpleondemand"); >> + else >> + err = request_module("governor_%s", name); >> + if (err) >> + return NULL; > > When error happen, you unlock the mutex. If failed to request module, > you should restore the previous state. Please mutex_lock(&devfreq_list_lock) > before return. > Oh right, my bad. >> + >> + mutex_lock(&devfreq_list_lock); >> + >> + governor = find_devfreq_governor(name); >> + } >> + >> + return governor; >> +} >> + >> static int devfreq_notify_transition(struct devfreq *devfreq, >> struct devfreq_freqs *freqs, unsigned int state) >> { >> @@ -643,11 +684,9 @@ struct devfreq *devfreq_add_device(struct device *dev, >> srcu_init_notifier_head(&devfreq->transition_notifier_list); >> >> mutex_unlock(&devfreq->lock); >> - > > This change is not related to this patch. > Ack. >> mutex_lock(&devfreq_list_lock); >> - list_add(&devfreq->node, &devfreq_list); >> >> - governor = find_devfreq_governor(devfreq->governor_name); >> + governor = try_then_request_governor(devfreq->governor_name); >> if (IS_ERR(governor)) { >> dev_err(dev, "%s: Unable to find governor for the device\n", >> __func__); >> @@ -663,14 +702,15 @@ struct devfreq *devfreq_add_device(struct device *dev, >> __func__); >> goto err_init; >> } >> + >> + list_add(&devfreq->node, &devfreq_list); >> + >> mutex_unlock(&devfreq_list_lock); >> >> return devfreq; >> >> err_init: >> - list_del(&devfreq->node); >> mutex_unlock(&devfreq_list_lock); >> - > > This change is not related to this patch. > Ack. >> device_unregister(&devfreq->dev); >> err_dev: >> if (devfreq) >> @@ -989,7 +1029,8 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr, >> return -EINVAL; >> >> mutex_lock(&devfreq_list_lock); >> - governor = find_devfreq_governor(str_governor); >> + > > Don't need to add the blank line. It is enough to change the function > from find_devfreq_governor to try_then_request_governor. > >> + governor = try_then_request_governor(str_governor); >> if (IS_ERR(governor)) { >> ret = PTR_ERR(governor); >> goto out; >> > > Preparing next version, many thanks for the review. Enric
Hi Enric, On 2018년 07월 04일 17:16, Enric Balletbo i Serra wrote: > Hi Chanwoo, > > On 04/07/18 03:06, Chanwoo Choi wrote: >> Hi Enric, >> >> On 2018년 07월 03일 22:29, Enric Balletbo i Serra wrote: >>> When the devfreq driver and the governor driver are built as modules, >>> the call to devfreq_add_device() or governor_store() 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 by adding a try_then_request_governor() >>> function. First tries to find the governor, and then, if it is 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> >>> --- >>> >>> Changes in v4: >>> - Kept "locked" devfreq_list from the return of find_devfreq_governor() to >>> the unlock of governor_store(). Requested by MyungJoo Ham. >>> >>> Changes in v3: >>> - Remove unneded change in dev_err message. >>> - Fix err returned value in case to not find the governor. >>> >>> Changes in v2: >>> - Add a new function to request the module and call that function from >>> devfreq_add_device and governor_store. >>> >>> drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++----- >>> 1 file changed, 47 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 0b5b3abe054e..4ea6b19879a1 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> >>> @@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) >>> return ERR_PTR(-ENODEV); >>> } >>> >>> +/** >>> + * try_then_request_governor() - Try to find the governor and request the >>> + * module if is not found. >>> + * @name: name of the governor >> >> Usually, devfreq used 'governor_name' indicating the name of governor. >> you better to use 'governor_name' instead of 'name' for more readability. >> > > I don't mind to change if you want. But let me try to convince you first. I used > name for two reasons: > 1. I saw that you are using governor_name sometimes, but find_devfreq_governor > uses name not governor_name. IMHO the function name in these two specific cases > 'try_then_request_governor(name)' is enough readable. OK. skip the my comment of changing the variable name. Thanks. > 2. If we want to use governor_name and then do not have the line exceeding the > 80 characters I need to split the function in two lines. For me the readability > is better when you have all in one line. > > If I did not convince you, just let me now and I'll change for governor_name :) > (snip)
Hi Enric, Please send this patch to stable-kernel mailing list. Regards, Chanwoo Choi On 2018년 07월 04일 17:26, Chanwoo Choi wrote: > Hi Enric, > > On 2018년 07월 04일 17:16, Enric Balletbo i Serra wrote: >> Hi Chanwoo, >> >> On 04/07/18 03:06, Chanwoo Choi wrote: >>> Hi Enric, >>> >>> On 2018년 07월 03일 22:29, Enric Balletbo i Serra wrote: >>>> When the devfreq driver and the governor driver are built as modules, >>>> the call to devfreq_add_device() or governor_store() 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 by adding a try_then_request_governor() >>>> function. First tries to find the governor, and then, if it is 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> >>>> --- >>>> >>>> Changes in v4: >>>> - Kept "locked" devfreq_list from the return of find_devfreq_governor() to >>>> the unlock of governor_store(). Requested by MyungJoo Ham. >>>> >>>> Changes in v3: >>>> - Remove unneded change in dev_err message. >>>> - Fix err returned value in case to not find the governor. >>>> >>>> Changes in v2: >>>> - Add a new function to request the module and call that function from >>>> devfreq_add_device and governor_store. >>>> >>>> drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++----- >>>> 1 file changed, 47 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index 0b5b3abe054e..4ea6b19879a1 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> >>>> @@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) >>>> return ERR_PTR(-ENODEV); >>>> } >>>> >>>> +/** >>>> + * try_then_request_governor() - Try to find the governor and request the >>>> + * module if is not found. >>>> + * @name: name of the governor >>> >>> Usually, devfreq used 'governor_name' indicating the name of governor. >>> you better to use 'governor_name' instead of 'name' for more readability. >>> >> >> I don't mind to change if you want. But let me try to convince you first. I used >> name for two reasons: >> 1. I saw that you are using governor_name sometimes, but find_devfreq_governor >> uses name not governor_name. IMHO the function name in these two specific cases >> 'try_then_request_governor(name)' is enough readable. > > OK. skip the my comment of changing the variable name. Thanks. > >> 2. If we want to use governor_name and then do not have the line exceeding the >> 80 characters I need to split the function in two lines. For me the readability >> is better when you have all in one line. >> >> If I did not convince you, just let me now and I'll change for governor_name :) >> > > (snip) >
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 0b5b3abe054e..4ea6b19879a1 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> @@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) return ERR_PTR(-ENODEV); } +/** + * try_then_request_governor() - Try to find the governor and request the + * module if is not found. + * @name: name of the governor + * + * Search the list of devfreq governors and request the module and try again + * if is not found. This can happen when both drivers (the governor driver + * and the driver that call devfreq_add_device) are built as modules. + * devfreq_list_lock should be held by the caller. + * + * Return: The matched governor's pointer. + */ +static struct devfreq_governor *try_then_request_governor(const char *name) +{ + struct devfreq_governor *governor; + int err = 0; + + WARN(!mutex_is_locked(&devfreq_list_lock), + "devfreq_list_lock must be locked."); + + governor = find_devfreq_governor(name); + if (IS_ERR(governor)) { + mutex_unlock(&devfreq_list_lock); + + if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, + DEVFREQ_NAME_LEN)) + err = request_module("governor_%s", "simpleondemand"); + else + err = request_module("governor_%s", name); + if (err) + return NULL; + + mutex_lock(&devfreq_list_lock); + + governor = find_devfreq_governor(name); + } + + return governor; +} + static int devfreq_notify_transition(struct devfreq *devfreq, struct devfreq_freqs *freqs, unsigned int state) { @@ -643,11 +684,9 @@ struct devfreq *devfreq_add_device(struct device *dev, srcu_init_notifier_head(&devfreq->transition_notifier_list); mutex_unlock(&devfreq->lock); - mutex_lock(&devfreq_list_lock); - list_add(&devfreq->node, &devfreq_list); - governor = find_devfreq_governor(devfreq->governor_name); + governor = try_then_request_governor(devfreq->governor_name); if (IS_ERR(governor)) { dev_err(dev, "%s: Unable to find governor for the device\n", __func__); @@ -663,14 +702,15 @@ struct devfreq *devfreq_add_device(struct device *dev, __func__); goto err_init; } + + list_add(&devfreq->node, &devfreq_list); + mutex_unlock(&devfreq_list_lock); return devfreq; err_init: - list_del(&devfreq->node); mutex_unlock(&devfreq_list_lock); - device_unregister(&devfreq->dev); err_dev: if (devfreq) @@ -989,7 +1029,8 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr, return -EINVAL; mutex_lock(&devfreq_list_lock); - governor = find_devfreq_governor(str_governor); + + governor = try_then_request_governor(str_governor); if (IS_ERR(governor)) { ret = PTR_ERR(governor); goto out;
When the devfreq driver and the governor driver are built as modules, the call to devfreq_add_device() or governor_store() 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 by adding a try_then_request_governor() function. First tries to find the governor, and then, if it is 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> --- Changes in v4: - Kept "locked" devfreq_list from the return of find_devfreq_governor() to the unlock of governor_store(). Requested by MyungJoo Ham. Changes in v3: - Remove unneded change in dev_err message. - Fix err returned value in case to not find the governor. Changes in v2: - Add a new function to request the module and call that function from devfreq_add_device and governor_store. drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 6 deletions(-)