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

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

[Enric Balletbo i Serra]

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

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

[Ezequiel Garcia]

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.

>  	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

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

[Akhil P Oommen]

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

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

[Enric Balletbo i Serra]

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

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

[Akhil P Oommen]

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 <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.
>>
I agree.
-Akhil.

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

[Ezequiel Garcia]

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 <enric.balletbo@collabora.c
> > > 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

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

[akhilpo]

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 <enric.balletbo@collabora.c
>> > > 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.

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

[Enric Balletbo Serra]

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 <akhilpo@codeaurora.org> 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 <enric.balletbo@collabora.c
> >> > > 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.

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

[MyungJoo Ham]

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

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

[MyungJoo Ham]

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

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

[Enric Balletbo i Serra]

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
>