Messages in this thread | | | Subject | Re: [PATCH v3] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules. | From | Enric Balletbo i Serra <> | Date | Fri, 22 Jun 2018 10:22:34 +0200 |
| |
Hi Ezequiel and Akhil,
On 22/06/18 09:03, Akhil P Oommen wrote: > > On 6/22/2018 6:41 AM, Ezequiel Garcia wrote: >> Hey Enric, >> >> On Fri, 2018-06-22 at 00:04 +0200, 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 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 | 65 ++++++++++++++++++++++++++++++++----- >>> -- >> [snip snip] >>> - 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__); >>> err = PTR_ERR(governor); >>> - goto err_init; >>> + goto err_unregister; >>> } >>> + mutex_lock(&devfreq_list_lock); >>> + >> I know it's not something we are introducing in this patch, >> but still... calling a hook with a mutex held looks >> fishy to me. >> >> This lock should only protect the list, unless I am missing >> something. >>
I think so too.
>>> devfreq->governor = governor; >>> err = devfreq->governor->event_handler(devfreq, >>> DEVFREQ_GOV_START, >>> NULL); >>> @@ -663,14 +703,16 @@ 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); >>> - >>> +err_unregister: >>> device_unregister(&devfreq->dev); >>> err_dev: >>> if (devfreq) >>> @@ -988,12 +1030,13 @@ static ssize_t governor_store(struct device >>> *dev, struct device_attribute *attr, >>> if (ret != 1) >>> 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; >>> + return PTR_ERR(governor); >>> } >>> + >>> + mutex_lock(&devfreq_list_lock); >>> + >>> if (df->governor == governor) { >>> ret = 0; >>> goto out; >>> -- >>> 2.17.1 >>> >>> >> >> Regards, >> Eze > > Adding to Ezequiel's point, shouldn't we take more granular lock (devfreq->lock) > first and then call devfreq_list_lock at the time of adding to the list? >
Yes, I think so. I think, though, that this should be a separate patch, not sure if a pre or post patch to this one, but for sure it's another topic. Current patch tries to solve different problem an only tries to follow the current locking/unlocking. Anyway this is a maintainer decision I guess.
Thanks, Enric
> -Akhil. >
| |