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 <[email protected]>
---
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 ++++++++++++++++++++++++++++++++-------
1 file changed, 54 insertions(+), 11 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 0b5b3abe054e..e059c431a558 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.
+ *
+ * Return: The matched governor's pointer.
+ */
+static struct devfreq_governor *try_then_request_governor(const char *name)
+{
+ struct devfreq_governor *governor;
+ int err = 0;
+
+ mutex_lock(&devfreq_list_lock);
+
+ 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);
+ }
+
+ mutex_unlock(&devfreq_list_lock);
+
+ return governor;
+}
+
static int devfreq_notify_transition(struct devfreq *devfreq,
struct devfreq_freqs *freqs, unsigned int state)
{
@@ -644,17 +685,16 @@ struct devfreq *devfreq_add_device(struct device *dev,
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__);
err = PTR_ERR(governor);
- goto err_init;
+ goto err_unregister;
}
+ mutex_lock(&devfreq_list_lock);
+
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
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 <[email protected]>
> ---
>
> 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.
> 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
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 <[email protected]>
>> ---
>>
>> 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.
>
>> 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?
-Akhil.
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 <[email protected]>
>>> ---
>>>
>>> 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.
>
On 6/22/2018 1:52 PM, Enric Balletbo i Serra wrote:
> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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.
>>
I agree.
-Akhil.
Hey Akhil,
On Fri, 2018-06-22 at 12:33 +0530, 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 <[email protected]
> > > om>
> > > ---
> > >
> > > 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.
> >
> > > 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?
>
Not sure why we should do that. devfreq->lock should be used to
protect the struct devfreq state, while the devfreq_list_lock
is apparently protecting the two lists (which seem unrelated
lists).
So, the two locks are not correlated.
Regards,
Eze
On 2018-06-22 22:43, Ezequiel Garcia wrote:
> Hey Akhil,
>
> On Fri, 2018-06-22 at 12:33 +0530, 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 <[email protected]
>> > > om>
>> > > ---
>> > >
>> > > 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.
>> >
>> > > 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?
>>
>
> Not sure why we should do that. devfreq->lock should be used to
> protect the struct devfreq state, while the devfreq_list_lock
> is apparently protecting the two lists (which seem unrelated
> lists).
>
> So, the two locks are not correlated.
>
> Regards,
> Eze
In governor_store(), we do 'df->governor = governor;' without taking
df->lock. So it is possible to switch governor while update_devfreq() is
in progress. I smell trouble there. Don't you think so?
I am assuming df->lock protects 'struct devfreq' and devfreq_list_lock
protects both device and governor lists.
-Akhil.
Hi Chanwoo,
Any comments?
Just a gentle ping to make sure the parallel conversation regarding
the mutex didn't distract you :)
Missatge de l'adreça <[email protected]> del dia dv., 22 de juny
2018 a les 23:22:
>
> On 2018-06-22 22:43, Ezequiel Garcia wrote:
> > Hey Akhil,
> >
> > On Fri, 2018-06-22 at 12:33 +0530, 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 <[email protected]
> >> > > om>
> >> > > ---
> >> > >
> >> > > 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.
> >> >
> >> > > 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?
> >>
> >
> > Not sure why we should do that. devfreq->lock should be used to
> > protect the struct devfreq state, while the devfreq_list_lock
> > is apparently protecting the two lists (which seem unrelated
> > lists).
> >
> > So, the two locks are not correlated.
> >
> > Regards,
> > Eze
> In governor_store(), we do 'df->governor = governor;' without taking
> df->lock. So it is possible to switch governor while update_devfreq() is
> in progress. I smell trouble there. Don't you think so?
> I am assuming df->lock protects 'struct devfreq' and devfreq_list_lock
> protects both device and governor lists.
>
> -Akhil.
> >> 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?
> >>
> >
> > Not sure why we should do that. devfreq->lock should be used to
> > protect the struct devfreq state, while the devfreq_list_lock
> > is apparently protecting the two lists (which seem unrelated
> > lists).
Correct.
devfreq->lock protects an instance of devfreq.
devfreq_list_lock protects the global devfreq data (list of devfreq / governors)
> >
> > So, the two locks are not correlated.
> >
> > Regards,
> > Eze
> In governor_store(), we do 'df->governor = governor;' without taking
> df->lock. So it is possible to switch governor while update_devfreq() is
> in progress.
Yup. that's possible.
> I smell trouble there. Don't you think so?
> I am assuming df->lock protects 'struct devfreq' and devfreq_list_lock
> protects both device and governor lists.
devfreq_list_lock is not supposed to protect a device.
Assuming a memory read of a word is atomic (I'm not aware of one that's not
unless in a case where the address is unaligned in some archtectures),
update_devfreq won't cause such issues because it reads "devfreq->governor"
only one in its execution except for the null check.
Thus, if there could be an error, it'd be a case where someone else is
doing "devfreq->governor = NULL" without devfreq->lock.
And, find_devfreq_governor() does not return NULL.
Cheers,
MyungJoo
>
> -Akhil.
>
>@@ -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;
>--
The possibility that the result of try_then_request_governor is
not kept protected until the line of
"if (df->governor == governor)" is itching.
Can you make it kept "locked" from the return of
find_devfreq_governor() (either in try_then... or in this function)
to the unlock of governor_store()?
Cheers,
MyungJoo
Hi MyungJoo
On 03/07/18 12:57, MyungJoo Ham wrote:
>> @@ -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;
>> --
>
> The possibility that the result of try_then_request_governor is
> not kept protected until the line of
> "if (df->governor == governor)" is itching.
>
> Can you make it kept "locked" from the return of
> find_devfreq_governor() (either in try_then... or in this function)
> to the unlock of governor_store()?
>
Sure, I'll send v4 in a minute.
Thanks,
Enric
>
> Cheers,
> MyungJoo
>