linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
@ 2016-10-26 19:17 ` Saravana Kannan
  2016-10-27  0:06   ` Chanwoo Choi
  0 siblings, 1 reply; 16+ messages in thread
From: Saravana Kannan @ 2016-10-26 19:17 UTC (permalink / raw)
  To: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel
  Cc: Saravana Kannan, cw00.choi

If the new governor fails to start, switch back to old governor so that the
devfreq state is not left in some weird limbo.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
* Fix NULL deref for real this time.
* Addressed some style preferences.

 drivers/devfreq/devfreq.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index bf3ea76..77c41a5 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	struct devfreq *df = to_devfreq(dev);
 	int ret;
 	char str_governor[DEVFREQ_NAME_LEN + 1];
-	struct devfreq_governor *governor;
+	const struct devfreq_governor *governor, *prev_governor;
 
 	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
 	if (ret != 1)
@@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 			goto out;
 		}
 	}
+	prev_governor = df->governor;
 	df->governor = governor;
 	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
 	ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
-	if (ret)
+	if (ret) {
 		dev_warn(dev, "%s: Governor %s not started(%d)\n",
 			 __func__, df->governor->name, ret);
+		if (prev_governor) {
+			df->governor = prev_governor;
+			strncpy(df->governor_name, prev_governor->name,
+				DEVFREQ_NAME_LEN);
+			df->governor->event_handler(df, DEVFREQ_GOV_START,
+						    NULL);
+		}
+	}
 out:
 	mutex_unlock(&devfreq_list_lock);
 
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-10-26 19:17 ` [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start Saravana Kannan
@ 2016-10-27  0:06   ` Chanwoo Choi
  2016-11-07 18:57     ` Saravana Kannan
  0 siblings, 1 reply; 16+ messages in thread
From: Chanwoo Choi @ 2016-10-27  0:06 UTC (permalink / raw)
  To: Saravana Kannan, Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park,
	linux-pm, linux-kernel

Hi,

On 2016년 10월 27일 04:17, Saravana Kannan wrote:
> If the new governor fails to start, switch back to old governor so that the
> devfreq state is not left in some weird limbo.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
> * Fix NULL deref for real this time.
> * Addressed some style preferences.
> 
>  drivers/devfreq/devfreq.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index bf3ea76..77c41a5 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	struct devfreq *df = to_devfreq(dev);
>  	int ret;
>  	char str_governor[DEVFREQ_NAME_LEN + 1];
> -	struct devfreq_governor *governor;
> +	const struct devfreq_governor *governor, *prev_governor;
>  
>  	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>  	if (ret != 1)
> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  			goto out;
>  		}
>  	}
> +	prev_governor = df->governor;
>  	df->governor = governor;
>  	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>  	ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> -	if (ret)
> +	if (ret) {
>  		dev_warn(dev, "%s: Governor %s not started(%d)\n",
>  			 __func__, df->governor->name, ret);
> +		if (prev_governor) {

I think that prev_governor is always set. You don't need to check it.
Why do check it?

> +			df->governor = prev_governor;
> +			strncpy(df->governor_name, prev_governor->name,
> +				DEVFREQ_NAME_LEN);
> +			df->governor->event_handler(df, DEVFREQ_GOV_START,
> +						    NULL);
> +		}
> +	}
>  out:
>  	mutex_unlock(&devfreq_list_lock);
>  
> 

-- 
Best Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-10-27  0:06   ` Chanwoo Choi
@ 2016-11-07 18:57     ` Saravana Kannan
  2016-11-08  9:02       ` Chanwoo Choi
  0 siblings, 1 reply; 16+ messages in thread
From: Saravana Kannan @ 2016-11-07 18:57 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
> Hi,
>
> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>> If the new governor fails to start, switch back to old governor so that the
>> devfreq state is not left in some weird limbo.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>> * Fix NULL deref for real this time.
>> * Addressed some style preferences.
>>
>>   drivers/devfreq/devfreq.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index bf3ea76..77c41a5 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>   	struct devfreq *df = to_devfreq(dev);
>>   	int ret;
>>   	char str_governor[DEVFREQ_NAME_LEN + 1];
>> -	struct devfreq_governor *governor;
>> +	const struct devfreq_governor *governor, *prev_governor;
>>
>>   	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>   	if (ret != 1)
>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>   			goto out;
>>   		}
>>   	}
>> +	prev_governor = df->governor;
>>   	df->governor = governor;
>>   	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>   	ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>> -	if (ret)
>> +	if (ret) {
>>   		dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>   			 __func__, df->governor->name, ret);
>> +		if (prev_governor) {
>
> I think that prev_governor is always set. You don't need to check it.
> Why do check it?

Not true. Even higher up in this function, we check if df->governor != 
NULL. Simple example would be that the default governor passed in while 
adding the device isn't compiled in.


Thanks,
Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-07 18:57     ` Saravana Kannan
@ 2016-11-08  9:02       ` Chanwoo Choi
  2016-11-08 20:52         ` Saravana Kannan
  0 siblings, 1 reply; 16+ messages in thread
From: Chanwoo Choi @ 2016-11-08  9:02 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

Hi,

On 2016년 11월 08일 03:57, Saravana Kannan wrote:
> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>> If the new governor fails to start, switch back to old governor so that the
>>> devfreq state is not left in some weird limbo.
>>>
>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>> ---
>>> * Fix NULL deref for real this time.
>>> * Addressed some style preferences.
>>>
>>>   drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index bf3ea76..77c41a5 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>       struct devfreq *df = to_devfreq(dev);
>>>       int ret;
>>>       char str_governor[DEVFREQ_NAME_LEN + 1];
>>> -    struct devfreq_governor *governor;
>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>
>>>       ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>       if (ret != 1)
>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>               goto out;
>>>           }
>>>       }
>>> +    prev_governor = df->governor;
>>>       df->governor = governor;
>>>       strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>       ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>> -    if (ret)
>>> +    if (ret) {
>>>           dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>                __func__, df->governor->name, ret);
>>> +        if (prev_governor) {
>>
>> I think that prev_governor is always set. You don't need to check it.
>> Why do check it?
> 
> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.

I don't understand. If device is not registered by devfreq_add_device(),
you don't change the governor by using governor_store().

If you can change the governor through governor_store(),
it means that the devfreq instance was added without any problem.
The added devfreq instance must have the sepecific governor
on df->governor variable.

So, I think that df->governor is always set and then prev_governor is always set.

Best Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-08  9:02       ` Chanwoo Choi
@ 2016-11-08 20:52         ` Saravana Kannan
  2016-11-09  1:33           ` Chanwoo Choi
  0 siblings, 1 reply; 16+ messages in thread
From: Saravana Kannan @ 2016-11-08 20:52 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
> Hi,
>
> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>> If the new governor fails to start, switch back to old governor so that the
>>>> devfreq state is not left in some weird limbo.
>>>>
>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>> ---
>>>> * Fix NULL deref for real this time.
>>>> * Addressed some style preferences.
>>>>
>>>>    drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index bf3ea76..77c41a5 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>        struct devfreq *df = to_devfreq(dev);
>>>>        int ret;
>>>>        char str_governor[DEVFREQ_NAME_LEN + 1];
>>>> -    struct devfreq_governor *governor;
>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>
>>>>        ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>        if (ret != 1)
>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>                goto out;
>>>>            }
>>>>        }
>>>> +    prev_governor = df->governor;
>>>>        df->governor = governor;
>>>>        strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>>        ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>> -    if (ret)
>>>> +    if (ret) {
>>>>            dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>                 __func__, df->governor->name, ret);
>>>> +        if (prev_governor) {
>>>
>>> I think that prev_governor is always set. You don't need to check it.
>>> Why do check it?
>>
>> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.
>
> I don't understand. If device is not registered by devfreq_add_device(),
> you don't change the governor by using governor_store().
>
> If you can change the governor through governor_store(),
> it means that the devfreq instance was added without any problem.
> The added devfreq instance must have the sepecific governor
> on df->governor variable.
>
> So, I think that df->governor is always set and then prev_governor is always set.

Read the code more closely. add_device doesn't (and shouldn't) fail if 
the governor isn't present. After that, the governor could be changed 
from user space.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-08 20:52         ` Saravana Kannan
@ 2016-11-09  1:33           ` Chanwoo Choi
  2016-11-09  2:36             ` Chanwoo Choi
  0 siblings, 1 reply; 16+ messages in thread
From: Chanwoo Choi @ 2016-11-09  1:33 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

On 2016년 11월 09일 05:52, Saravana Kannan wrote:
> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>> If the new governor fails to start, switch back to old governor so that the
>>>>> devfreq state is not left in some weird limbo.
>>>>>
>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>> ---
>>>>> * Fix NULL deref for real this time.
>>>>> * Addressed some style preferences.
>>>>>
>>>>>    drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index bf3ea76..77c41a5 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>        struct devfreq *df = to_devfreq(dev);
>>>>>        int ret;
>>>>>        char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>> -    struct devfreq_governor *governor;
>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>
>>>>>        ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>>        if (ret != 1)
>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>                goto out;
>>>>>            }
>>>>>        }
>>>>> +    prev_governor = df->governor;
>>>>>        df->governor = governor;
>>>>>        strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>>>        ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>>> -    if (ret)
>>>>> +    if (ret) {
>>>>>            dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>                 __func__, df->governor->name, ret);
>>>>> +        if (prev_governor) {
>>>>
>>>> I think that prev_governor is always set. You don't need to check it.
>>>> Why do check it?
>>>
>>> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.
>>
>> I don't understand. If device is not registered by devfreq_add_device(),
>> you don't change the governor by using governor_store().
>>
>> If you can change the governor through governor_store(),
>> it means that the devfreq instance was added without any problem.
>> The added devfreq instance must have the sepecific governor
>> on df->governor variable.
>>
>> So, I think that df->governor is always set and then prev_governor is always set.
> 
> Read the code more closely. add_device doesn't (and shouldn't) fail if the governor isn't present. After that, the governor could be changed from user space.

If governor is not present during devfreq_add_device(),
the devfreq instance is not able to register the devfreq framework.
So, the user never touch the sysfs[1] to change the governor
because the sysfs[1] is not created by devfreq framework.
[1] /sys/class/devfreq/[device name]/governor

In summary, if governor is not present during devfreq_add_device(),
the devfreq framework doesn't create tye sysfs[1] for governor.

The user-space never change the governor through sysfs[1]
because there is no any sysfs entry[1].

Best Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-09  1:33           ` Chanwoo Choi
@ 2016-11-09  2:36             ` Chanwoo Choi
  2016-11-09  2:38               ` Chanwoo Choi
  0 siblings, 1 reply; 16+ messages in thread
From: Chanwoo Choi @ 2016-11-09  2:36 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

Hi,

On 2016년 11월 09일 10:33, Chanwoo Choi wrote:
> On 2016년 11월 09일 05:52, Saravana Kannan wrote:
>> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>>> Hi,
>>>>>
>>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>>> If the new governor fails to start, switch back to old governor so that the
>>>>>> devfreq state is not left in some weird limbo.
>>>>>>
>>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>>> ---
>>>>>> * Fix NULL deref for real this time.
>>>>>> * Addressed some style preferences.
>>>>>>
>>>>>>    drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>> index bf3ea76..77c41a5 100644
>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>        struct devfreq *df = to_devfreq(dev);
>>>>>>        int ret;
>>>>>>        char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>> -    struct devfreq_governor *governor;
>>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>>
>>>>>>        ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>>>        if (ret != 1)
>>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>                goto out;
>>>>>>            }
>>>>>>        }
>>>>>> +    prev_governor = df->governor;
>>>>>>        df->governor = governor;
>>>>>>        strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>>>>        ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>>>> -    if (ret)
>>>>>> +    if (ret) {
>>>>>>            dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>>                 __func__, df->governor->name, ret);
>>>>>> +        if (prev_governor) {
>>>>>
>>>>> I think that prev_governor is always set. You don't need to check it.
>>>>> Why do check it?
>>>>
>>>> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.
>>>
>>> I don't understand. If device is not registered by devfreq_add_device(),
>>> you don't change the governor by using governor_store().
>>>
>>> If you can change the governor through governor_store(),
>>> it means that the devfreq instance was added without any problem.
>>> The added devfreq instance must have the sepecific governor
>>> on df->governor variable.
>>>
>>> So, I think that df->governor is always set and then prev_governor is always set.
>>
>> Read the code more closely. add_device doesn't (and shouldn't) fail if the governor isn't present. After that, the governor could be changed from user space.
> 
> If governor is not present during devfreq_add_device(),
> the devfreq instance is not able to register the devfreq framework.
> So, the user never touch the sysfs[1] to change the governor
> because the sysfs[1] is not created by devfreq framework.
> [1] /sys/class/devfreq/[device name]/governor
> 
> In summary, if governor is not present during devfreq_add_device(),
> the devfreq framework doesn't create tye sysfs[1] for governor.
> 
> The user-space never change the governor through sysfs[1]
> because there is no any sysfs entry[1].

I checked the code of devfreq_add_device().
As you mentioned, if there is no governor,
devfreq_add_device() don't pass wihtout calling the devfreq->governor->even_handler().

I discussed this issue with Myungjoo Ham (devfreq maintainer).
It is bug of devfreq_add_device(). I'll fix this issue.

Best Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-09  2:36             ` Chanwoo Choi
@ 2016-11-09  2:38               ` Chanwoo Choi
  2016-11-09 20:34                 ` Saravana Kannan
  0 siblings, 1 reply; 16+ messages in thread
From: Chanwoo Choi @ 2016-11-09  2:38 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

On 2016년 11월 09일 11:36, Chanwoo Choi wrote:
> Hi,
> 
> On 2016년 11월 09일 10:33, Chanwoo Choi wrote:
>> On 2016년 11월 09일 05:52, Saravana Kannan wrote:
>>> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>>>> If the new governor fails to start, switch back to old governor so that the
>>>>>>> devfreq state is not left in some weird limbo.
>>>>>>>
>>>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>>>> ---
>>>>>>> * Fix NULL deref for real this time.
>>>>>>> * Addressed some style preferences.
>>>>>>>
>>>>>>>    drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>>> index bf3ea76..77c41a5 100644
>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>        struct devfreq *df = to_devfreq(dev);
>>>>>>>        int ret;
>>>>>>>        char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>>> -    struct devfreq_governor *governor;
>>>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>>>
>>>>>>>        ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>>>>        if (ret != 1)
>>>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>                goto out;
>>>>>>>            }
>>>>>>>        }
>>>>>>> +    prev_governor = df->governor;
>>>>>>>        df->governor = governor;
>>>>>>>        strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>>>>>        ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>>>>> -    if (ret)
>>>>>>> +    if (ret) {
>>>>>>>            dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>>>                 __func__, df->governor->name, ret);
>>>>>>> +        if (prev_governor) {
>>>>>>
>>>>>> I think that prev_governor is always set. You don't need to check it.
>>>>>> Why do check it?
>>>>>
>>>>> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.
>>>>
>>>> I don't understand. If device is not registered by devfreq_add_device(),
>>>> you don't change the governor by using governor_store().
>>>>
>>>> If you can change the governor through governor_store(),
>>>> it means that the devfreq instance was added without any problem.
>>>> The added devfreq instance must have the sepecific governor
>>>> on df->governor variable.
>>>>
>>>> So, I think that df->governor is always set and then prev_governor is always set.
>>>
>>> Read the code more closely. add_device doesn't (and shouldn't) fail if the governor isn't present. After that, the governor could be changed from user space.
>>
>> If governor is not present during devfreq_add_device(),
>> the devfreq instance is not able to register the devfreq framework.
>> So, the user never touch the sysfs[1] to change the governor
>> because the sysfs[1] is not created by devfreq framework.
>> [1] /sys/class/devfreq/[device name]/governor
>>
>> In summary, if governor is not present during devfreq_add_device(),
>> the devfreq framework doesn't create tye sysfs[1] for governor.
>>
>> The user-space never change the governor through sysfs[1]
>> because there is no any sysfs entry[1].
> 
> I checked the code of devfreq_add_device().
> As you mentioned, if there is no governor,
> devfreq_add_device() don't pass wihtout calling the devfreq->governor->even_handler().

Wrong expression / don't pass -> pass

Regards,
Chanwoo Choi

> 
> I discussed this issue with Myungjoo Ham (devfreq maintainer).
> It is bug of devfreq_add_device(). I'll fix this issue.
> 
> Best Regards,
> Chanwoo Choi
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-09  2:38               ` Chanwoo Choi
@ 2016-11-09 20:34                 ` Saravana Kannan
  2016-11-10  0:10                   ` Chanwoo Choi
  0 siblings, 1 reply; 16+ messages in thread
From: Saravana Kannan @ 2016-11-09 20:34 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

On 11/08/2016 06:38 PM, Chanwoo Choi wrote:
> On 2016년 11월 09일 11:36, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2016년 11월 09일 10:33, Chanwoo Choi wrote:
>>> On 2016년 11월 09일 05:52, Saravana Kannan wrote:
>>>> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>>>>> Hi,
>>>>>
>>>>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>>>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>>>>> If the new governor fails to start, switch back to old governor so that the
>>>>>>>> devfreq state is not left in some weird limbo.
>>>>>>>>
>>>>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>>>>> ---
>>>>>>>> * Fix NULL deref for real this time.
>>>>>>>> * Addressed some style preferences.
>>>>>>>>
>>>>>>>>     drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>>>>     1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>>>> index bf3ea76..77c41a5 100644
>>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>         struct devfreq *df = to_devfreq(dev);
>>>>>>>>         int ret;
>>>>>>>>         char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>>>> -    struct devfreq_governor *governor;
>>>>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>>>>
>>>>>>>>         ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>>>>>         if (ret != 1)
>>>>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>                 goto out;
>>>>>>>>             }
>>>>>>>>         }
>>>>>>>> +    prev_governor = df->governor;
>>>>>>>>         df->governor = governor;
>>>>>>>>         strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>>>>>>         ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>>>>>> -    if (ret)
>>>>>>>> +    if (ret) {
>>>>>>>>             dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>>>>                  __func__, df->governor->name, ret);
>>>>>>>> +        if (prev_governor) {
>>>>>>>
>>>>>>> I think that prev_governor is always set. You don't need to check it.
>>>>>>> Why do check it?
>>>>>>
>>>>>> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.
>>>>>
>>>>> I don't understand. If device is not registered by devfreq_add_device(),
>>>>> you don't change the governor by using governor_store().
>>>>>
>>>>> If you can change the governor through governor_store(),
>>>>> it means that the devfreq instance was added without any problem.
>>>>> The added devfreq instance must have the sepecific governor
>>>>> on df->governor variable.
>>>>>
>>>>> So, I think that df->governor is always set and then prev_governor is always set.
>>>>
>>>> Read the code more closely. add_device doesn't (and shouldn't) fail if the governor isn't present. After that, the governor could be changed from user space.
>>>
>>> If governor is not present during devfreq_add_device(),
>>> the devfreq instance is not able to register the devfreq framework.
>>> So, the user never touch the sysfs[1] to change the governor
>>> because the sysfs[1] is not created by devfreq framework.
>>> [1] /sys/class/devfreq/[device name]/governor
>>>
>>> In summary, if governor is not present during devfreq_add_device(),
>>> the devfreq framework doesn't create tye sysfs[1] for governor.
>>>
>>> The user-space never change the governor through sysfs[1]
>>> because there is no any sysfs entry[1].
>>
>> I checked the code of devfreq_add_device().
>> As you mentioned, if there is no governor,
>> devfreq_add_device() don't pass wihtout calling the devfreq->governor->even_handler().
>
> Wrong expression / don't pass -> pass
>

I think it's correct as is. Just because the governor isn't there (or 
hasn't registered yet) doesn't mean the device add should fail.

That aside, do you care to ack this patch for the way the code is currently?

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-09 20:34                 ` Saravana Kannan
@ 2016-11-10  0:10                   ` Chanwoo Choi
  2016-11-11 23:25                     ` Saravana Kannan
  2018-12-07 20:29                     ` Saravana Kannan
  0 siblings, 2 replies; 16+ messages in thread
From: Chanwoo Choi @ 2016-11-10  0:10 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

Hi,

On 2016년 11월 10일 05:34, Saravana Kannan wrote:
> On 11/08/2016 06:38 PM, Chanwoo Choi wrote:
>> On 2016년 11월 09일 11:36, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 2016년 11월 09일 10:33, Chanwoo Choi wrote:
>>>> On 2016년 11월 09일 05:52, Saravana Kannan wrote:
>>>>> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>>>>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>>>>>> If the new governor fails to start, switch back to old governor so that the
>>>>>>>>> devfreq state is not left in some weird limbo.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>>>>>> ---
>>>>>>>>> * Fix NULL deref for real this time.
>>>>>>>>> * Addressed some style preferences.
>>>>>>>>>
>>>>>>>>>     drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>>>>>     1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>>>>> index bf3ea76..77c41a5 100644
>>>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>         struct devfreq *df = to_devfreq(dev);
>>>>>>>>>         int ret;
>>>>>>>>>         char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>>>>> -    struct devfreq_governor *governor;
>>>>>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>>>>>
>>>>>>>>>         ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>>>>>>         if (ret != 1)
>>>>>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>                 goto out;
>>>>>>>>>             }
>>>>>>>>>         }
>>>>>>>>> +    prev_governor = df->governor;
>>>>>>>>>         df->governor = governor;
>>>>>>>>>         strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>>>>>>>         ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>>>>>>> -    if (ret)
>>>>>>>>> +    if (ret) {
>>>>>>>>>             dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>>>>>                  __func__, df->governor->name, ret);
>>>>>>>>> +        if (prev_governor) {
>>>>>>>>
>>>>>>>> I think that prev_governor is always set. You don't need to check it.
>>>>>>>> Why do check it?
>>>>>>>
>>>>>>> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.
>>>>>>
>>>>>> I don't understand. If device is not registered by devfreq_add_device(),
>>>>>> you don't change the governor by using governor_store().
>>>>>>
>>>>>> If you can change the governor through governor_store(),
>>>>>> it means that the devfreq instance was added without any problem.
>>>>>> The added devfreq instance must have the sepecific governor
>>>>>> on df->governor variable.
>>>>>>
>>>>>> So, I think that df->governor is always set and then prev_governor is always set.
>>>>>
>>>>> Read the code more closely. add_device doesn't (and shouldn't) fail if the governor isn't present. After that, the governor could be changed from user space.
>>>>
>>>> If governor is not present during devfreq_add_device(),
>>>> the devfreq instance is not able to register the devfreq framework.
>>>> So, the user never touch the sysfs[1] to change the governor
>>>> because the sysfs[1] is not created by devfreq framework.
>>>> [1] /sys/class/devfreq/[device name]/governor
>>>>
>>>> In summary, if governor is not present during devfreq_add_device(),
>>>> the devfreq framework doesn't create tye sysfs[1] for governor.
>>>>
>>>> The user-space never change the governor through sysfs[1]
>>>> because there is no any sysfs entry[1].
>>>
>>> I checked the code of devfreq_add_device().
>>> As you mentioned, if there is no governor,
>>> devfreq_add_device() don't pass wihtout calling the devfreq->governor->even_handler().
>>
>> Wrong expression / don't pass -> pass
>>
> 
> I think it's correct as is. Just because the governor isn't there (or hasn't registered yet) doesn't mean the device add should fail.
> 
> That aside, do you care to ack this patch for the way the code is currently?

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

After fixing the bug of devfreq_add_device(),
I'll remove the unneeded 'if statement' of prev_governor in governor_store. 

Best Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-10  0:10                   ` Chanwoo Choi
@ 2016-11-11 23:25                     ` Saravana Kannan
  2016-11-12  2:17                       ` Chanwoo Choi
  2018-12-07 20:29                     ` Saravana Kannan
  1 sibling, 1 reply; 16+ messages in thread
From: Saravana Kannan @ 2016-11-11 23:25 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

On 11/09/2016 04:10 PM, Chanwoo Choi wrote:
> Hi,
>
> On 2016년 11월 10일 05:34, Saravana Kannan wrote:
>> On 11/08/2016 06:38 PM, Chanwoo Choi wrote:
>>> On 2016년 11월 09일 11:36, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> On 2016년 11월 09일 10:33, Chanwoo Choi wrote:
>>>>> On 2016년 11월 09일 05:52, Saravana Kannan wrote:
>>>>>> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>>>>>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>>>>>>> If the new governor fails to start, switch back to old governor so that the
>>>>>>>>>> devfreq state is not left in some weird limbo.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>>>>>>> ---
>>>>>>>>>> * Fix NULL deref for real this time.
>>>>>>>>>> * Addressed some style preferences.
>>>>>>>>>>
>>>>>>>>>>      drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>>>>>>      1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>>>>>> index bf3ea76..77c41a5 100644
>>>>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>          struct devfreq *df = to_devfreq(dev);
>>>>>>>>>>          int ret;
>>>>>>>>>>          char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>>>>>> -    struct devfreq_governor *governor;
>>>>>>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>>>>>>
>>>>>>>>>>          ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>>>>>>>          if (ret != 1)
>>>>>>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>                  goto out;
>>>>>>>>>>              }
>>>>>>>>>>          }
>>>>>>>>>> +    prev_governor = df->governor;
>>>>>>>>>>          df->governor = governor;
>>>>>>>>>>          strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>>>>>>>>          ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>>>>>>>> -    if (ret)
>>>>>>>>>> +    if (ret) {
>>>>>>>>>>              dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>>>>>>                   __func__, df->governor->name, ret);
>>>>>>>>>> +        if (prev_governor) {
>>>>>>>>>
>>>>>>>>> I think that prev_governor is always set. You don't need to check it.
>>>>>>>>> Why do check it?
>>>>>>>>
>>>>>>>> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.
>>>>>>>
>>>>>>> I don't understand. If device is not registered by devfreq_add_device(),
>>>>>>> you don't change the governor by using governor_store().
>>>>>>>
>>>>>>> If you can change the governor through governor_store(),
>>>>>>> it means that the devfreq instance was added without any problem.
>>>>>>> The added devfreq instance must have the sepecific governor
>>>>>>> on df->governor variable.
>>>>>>>
>>>>>>> So, I think that df->governor is always set and then prev_governor is always set.
>>>>>>
>>>>>> Read the code more closely. add_device doesn't (and shouldn't) fail if the governor isn't present. After that, the governor could be changed from user space.
>>>>>
>>>>> If governor is not present during devfreq_add_device(),
>>>>> the devfreq instance is not able to register the devfreq framework.
>>>>> So, the user never touch the sysfs[1] to change the governor
>>>>> because the sysfs[1] is not created by devfreq framework.
>>>>> [1] /sys/class/devfreq/[device name]/governor
>>>>>
>>>>> In summary, if governor is not present during devfreq_add_device(),
>>>>> the devfreq framework doesn't create tye sysfs[1] for governor.
>>>>>
>>>>> The user-space never change the governor through sysfs[1]
>>>>> because there is no any sysfs entry[1].
>>>>
>>>> I checked the code of devfreq_add_device().
>>>> As you mentioned, if there is no governor,
>>>> devfreq_add_device() don't pass wihtout calling the devfreq->governor->even_handler().
>>>
>>> Wrong expression / don't pass -> pass
>>>
>>
>> I think it's correct as is. Just because the governor isn't there (or hasn't registered yet) doesn't mean the device add should fail.
>>
>> That aside, do you care to ack this patch for the way the code is currently?
>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Thanks

> After fixing the bug of devfreq_add_device(),
> I'll remove the unneeded 'if statement' of prev_governor in governor_store.
>

As mentioned earlier, I don't think it's a bug. It's fine as is.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-11 23:25                     ` Saravana Kannan
@ 2016-11-12  2:17                       ` Chanwoo Choi
  2016-11-12  2:18                         ` Chanwoo Choi
  0 siblings, 1 reply; 16+ messages in thread
From: Chanwoo Choi @ 2016-11-12  2:17 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Chanwoo Choi, Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park,
	linux-pm, linux-kernel

2016-11-12 8:25 GMT+09:00 Saravana Kannan <skannan@codeaurora.org>:
> On 11/09/2016 04:10 PM, Chanwoo Choi wrote:
>>
>> Hi,
>>
>> On 2016년 11월 10일 05:34, Saravana Kannan wrote:
>>>
>>> On 11/08/2016 06:38 PM, Chanwoo Choi wrote:
>>>>
>>>> On 2016년 11월 09일 11:36, Chanwoo Choi wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 2016년 11월 09일 10:33, Chanwoo Choi wrote:
>>>>>>
>>>>>> On 2016년 11월 09일 05:52, Saravana Kannan wrote:
>>>>>>>
>>>>>>> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>>>>>>>>
>>>>>>>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>>>>>>>>
>>>>>>>>>>> If the new governor fails to start, switch back to old governor
>>>>>>>>>>> so that the
>>>>>>>>>>> devfreq state is not left in some weird limbo.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>>>>>>>> ---
>>>>>>>>>>> * Fix NULL deref for real this time.
>>>>>>>>>>> * Addressed some style preferences.
>>>>>>>>>>>
>>>>>>>>>>>      drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>>>>>>>      1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/devfreq/devfreq.c
>>>>>>>>>>> b/drivers/devfreq/devfreq.c
>>>>>>>>>>> index bf3ea76..77c41a5 100644
>>>>>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device
>>>>>>>>>>> *dev, struct device_attribute *attr,
>>>>>>>>>>>          struct devfreq *df = to_devfreq(dev);
>>>>>>>>>>>          int ret;
>>>>>>>>>>>          char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>>>>>>> -    struct devfreq_governor *governor;
>>>>>>>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>>>>>>>
>>>>>>>>>>>          ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s",
>>>>>>>>>>> str_governor);
>>>>>>>>>>>          if (ret != 1)
>>>>>>>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device
>>>>>>>>>>> *dev, struct device_attribute *attr,
>>>>>>>>>>>                  goto out;
>>>>>>>>>>>              }
>>>>>>>>>>>          }
>>>>>>>>>>> +    prev_governor = df->governor;
>>>>>>>>>>>          df->governor = governor;
>>>>>>>>>>>          strncpy(df->governor_name, governor->name,
>>>>>>>>>>> DEVFREQ_NAME_LEN);
>>>>>>>>>>>          ret = df->governor->event_handler(df, DEVFREQ_GOV_START,
>>>>>>>>>>> NULL);
>>>>>>>>>>> -    if (ret)
>>>>>>>>>>> +    if (ret) {
>>>>>>>>>>>              dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>>>>>>>                   __func__, df->governor->name, ret);
>>>>>>>>>>> +        if (prev_governor) {
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I think that prev_governor is always set. You don't need to check
>>>>>>>>>> it.
>>>>>>>>>> Why do check it?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Not true. Even higher up in this function, we check if df->governor
>>>>>>>>> != NULL. Simple example would be that the default governor passed in while
>>>>>>>>> adding the device isn't compiled in.
>>>>>>>>
>>>>>>>>
>>>>>>>> I don't understand. If device is not registered by
>>>>>>>> devfreq_add_device(),
>>>>>>>> you don't change the governor by using governor_store().
>>>>>>>>
>>>>>>>> If you can change the governor through governor_store(),
>>>>>>>> it means that the devfreq instance was added without any problem.
>>>>>>>> The added devfreq instance must have the sepecific governor
>>>>>>>> on df->governor variable.
>>>>>>>>
>>>>>>>> So, I think that df->governor is always set and then prev_governor
>>>>>>>> is always set.
>>>>>>>
>>>>>>>
>>>>>>> Read the code more closely. add_device doesn't (and shouldn't) fail
>>>>>>> if the governor isn't present. After that, the governor could be changed
>>>>>>> from user space.
>>>>>>
>>>>>>
>>>>>> If governor is not present during devfreq_add_device(),
>>>>>> the devfreq instance is not able to register the devfreq framework.
>>>>>> So, the user never touch the sysfs[1] to change the governor
>>>>>> because the sysfs[1] is not created by devfreq framework.
>>>>>> [1] /sys/class/devfreq/[device name]/governor
>>>>>>
>>>>>> In summary, if governor is not present during devfreq_add_device(),
>>>>>> the devfreq framework doesn't create tye sysfs[1] for governor.
>>>>>>
>>>>>> The user-space never change the governor through sysfs[1]
>>>>>> because there is no any sysfs entry[1].
>>>>>
>>>>>
>>>>> I checked the code of devfreq_add_device().
>>>>> As you mentioned, if there is no governor,
>>>>> devfreq_add_device() don't pass wihtout calling the
>>>>> devfreq->governor->even_handler().
>>>>
>>>>
>>>> Wrong expression / don't pass -> pass
>>>>
>>>
>>> I think it's correct as is. Just because the governor isn't there (or
>>> hasn't registered yet) doesn't mean the device add should fail.
>>>
>>> That aside, do you care to ack this patch for the way the code is
>>> currently?
>>
>>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
>
> Thanks
>
>> After fixing the bug of devfreq_add_device(),
>> I'll remove the unneeded 'if statement' of prev_governor in
>> governor_store.
>>
>
> As mentioned earlier, I don't think it's a bug. It's fine as is.

No. I think it is a bug. The goal of devfreq should handle the frequency/voltage
with governor. If governor is not necessary of the specific device, the device
driver don't need to use the devfreq.

The default governor is always available and necessary of the device driver.

-- 
Best Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-12  2:17                       ` Chanwoo Choi
@ 2016-11-12  2:18                         ` Chanwoo Choi
  0 siblings, 0 replies; 16+ messages in thread
From: Chanwoo Choi @ 2016-11-12  2:18 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Chanwoo Choi, Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park,
	linux-pm, linux-kernel

2016-11-12 11:17 GMT+09:00 Chanwoo Choi <cwchoi00@gmail.com>:
> 2016-11-12 8:25 GMT+09:00 Saravana Kannan <skannan@codeaurora.org>:
>> On 11/09/2016 04:10 PM, Chanwoo Choi wrote:
>>>
>>> Hi,
>>>
>>> On 2016년 11월 10일 05:34, Saravana Kannan wrote:
>>>>
>>>> On 11/08/2016 06:38 PM, Chanwoo Choi wrote:
>>>>>
>>>>> On 2016년 11월 09일 11:36, Chanwoo Choi wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 2016년 11월 09일 10:33, Chanwoo Choi wrote:
>>>>>>>
>>>>>>> On 2016년 11월 09일 05:52, Saravana Kannan wrote:
>>>>>>>>
>>>>>>>> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>>>>>>>>>
>>>>>>>>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> If the new governor fails to start, switch back to old governor
>>>>>>>>>>>> so that the
>>>>>>>>>>>> devfreq state is not left in some weird limbo.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>>>>>>>>> ---
>>>>>>>>>>>> * Fix NULL deref for real this time.
>>>>>>>>>>>> * Addressed some style preferences.
>>>>>>>>>>>>
>>>>>>>>>>>>      drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>>>>>>>>      1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/devfreq/devfreq.c
>>>>>>>>>>>> b/drivers/devfreq/devfreq.c
>>>>>>>>>>>> index bf3ea76..77c41a5 100644
>>>>>>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>>>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device
>>>>>>>>>>>> *dev, struct device_attribute *attr,
>>>>>>>>>>>>          struct devfreq *df = to_devfreq(dev);
>>>>>>>>>>>>          int ret;
>>>>>>>>>>>>          char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>>>>>>>> -    struct devfreq_governor *governor;
>>>>>>>>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>>>>>>>>
>>>>>>>>>>>>          ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s",
>>>>>>>>>>>> str_governor);
>>>>>>>>>>>>          if (ret != 1)
>>>>>>>>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device
>>>>>>>>>>>> *dev, struct device_attribute *attr,
>>>>>>>>>>>>                  goto out;
>>>>>>>>>>>>              }
>>>>>>>>>>>>          }
>>>>>>>>>>>> +    prev_governor = df->governor;
>>>>>>>>>>>>          df->governor = governor;
>>>>>>>>>>>>          strncpy(df->governor_name, governor->name,
>>>>>>>>>>>> DEVFREQ_NAME_LEN);
>>>>>>>>>>>>          ret = df->governor->event_handler(df, DEVFREQ_GOV_START,
>>>>>>>>>>>> NULL);
>>>>>>>>>>>> -    if (ret)
>>>>>>>>>>>> +    if (ret) {
>>>>>>>>>>>>              dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>>>>>>>>                   __func__, df->governor->name, ret);
>>>>>>>>>>>> +        if (prev_governor) {
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I think that prev_governor is always set. You don't need to check
>>>>>>>>>>> it.
>>>>>>>>>>> Why do check it?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Not true. Even higher up in this function, we check if df->governor
>>>>>>>>>> != NULL. Simple example would be that the default governor passed in while
>>>>>>>>>> adding the device isn't compiled in.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I don't understand. If device is not registered by
>>>>>>>>> devfreq_add_device(),
>>>>>>>>> you don't change the governor by using governor_store().
>>>>>>>>>
>>>>>>>>> If you can change the governor through governor_store(),
>>>>>>>>> it means that the devfreq instance was added without any problem.
>>>>>>>>> The added devfreq instance must have the sepecific governor
>>>>>>>>> on df->governor variable.
>>>>>>>>>
>>>>>>>>> So, I think that df->governor is always set and then prev_governor
>>>>>>>>> is always set.
>>>>>>>>
>>>>>>>>
>>>>>>>> Read the code more closely. add_device doesn't (and shouldn't) fail
>>>>>>>> if the governor isn't present. After that, the governor could be changed
>>>>>>>> from user space.
>>>>>>>
>>>>>>>
>>>>>>> If governor is not present during devfreq_add_device(),
>>>>>>> the devfreq instance is not able to register the devfreq framework.
>>>>>>> So, the user never touch the sysfs[1] to change the governor
>>>>>>> because the sysfs[1] is not created by devfreq framework.
>>>>>>> [1] /sys/class/devfreq/[device name]/governor
>>>>>>>
>>>>>>> In summary, if governor is not present during devfreq_add_device(),
>>>>>>> the devfreq framework doesn't create tye sysfs[1] for governor.
>>>>>>>
>>>>>>> The user-space never change the governor through sysfs[1]
>>>>>>> because there is no any sysfs entry[1].
>>>>>>
>>>>>>
>>>>>> I checked the code of devfreq_add_device().
>>>>>> As you mentioned, if there is no governor,
>>>>>> devfreq_add_device() don't pass wihtout calling the
>>>>>> devfreq->governor->even_handler().
>>>>>
>>>>>
>>>>> Wrong expression / don't pass -> pass
>>>>>
>>>>
>>>> I think it's correct as is. Just because the governor isn't there (or
>>>> hasn't registered yet) doesn't mean the device add should fail.
>>>>
>>>> That aside, do you care to ack this patch for the way the code is
>>>> currently?
>>>
>>>
>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>>
>> Thanks
>>
>>> After fixing the bug of devfreq_add_device(),
>>> I'll remove the unneeded 'if statement' of prev_governor in
>>> governor_store.
>>>
>>
>> As mentioned earlier, I don't think it's a bug. It's fine as is.
>
> No. I think it is a bug. The goal of devfreq should handle the frequency/voltage
> with governor. If governor is not necessary of the specific device, the device
> driver don't need to use the devfreq.
>
> The default governor is always available and necessary of the device driver.

The default governor have to be always available and necessary of the
device driver.


-- 
Best Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2016-11-10  0:10                   ` Chanwoo Choi
  2016-11-11 23:25                     ` Saravana Kannan
@ 2018-12-07 20:29                     ` Saravana Kannan
  2018-12-11  5:35                       ` Chanwoo Choi
       [not found]                       ` <CGME20161026191715epcas1p311c2c626563a736e14c83fb0504c099e@epcms1p3>
  1 sibling, 2 replies; 16+ messages in thread
From: Saravana Kannan @ 2018-12-07 20:29 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel


On 11/9/16 4:10 PM, Chanwoo Choi wrote:
> Hi,
>
> On 2016년 11월 10일 05:34, Saravana Kannan wrote:
>> On 11/08/2016 06:38 PM, Chanwoo Choi wrote:
>>> On 2016년 11월 09일 11:36, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> On 2016년 11월 09일 10:33, Chanwoo Choi wrote:
>>>>> On 2016년 11월 09일 05:52, Saravana Kannan wrote:
>>>>>> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>>>>>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>>>>>>> If the new governor fails to start, switch back to old governor so that the
>>>>>>>>>> devfreq state is not left in some weird limbo.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>>>>>>> ---
>>>>>>>>>> * Fix NULL deref for real this time.
>>>>>>>>>> * Addressed some style preferences.
>>>>>>>>>>
>>>>>>>>>>      drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>>>>>>      1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>>>>>> index bf3ea76..77c41a5 100644
>>>>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>          struct devfreq *df = to_devfreq(dev);
>>>>>>>>>>          int ret;
>>>>>>>>>>          char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>>>>>> -    struct devfreq_governor *governor;
>>>>>>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>>>>>>
>>>>>>>>>>          ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>>>>>>>          if (ret != 1)
>>>>>>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>                  goto out;
>>>>>>>>>>              }
>>>>>>>>>>          }
>>>>>>>>>> +    prev_governor = df->governor;
>>>>>>>>>>          df->governor = governor;
>>>>>>>>>>          strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>>>>>>>>          ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>>>>>>>> -    if (ret)
>>>>>>>>>> +    if (ret) {
>>>>>>>>>>              dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>>>>>>                   __func__, df->governor->name, ret);
>>>>>>>>>> +        if (prev_governor) {
>>>>>>>>> I think that prev_governor is always set. You don't need to check it.
>>>>>>>>> Why do check it?
>>>>>>>> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.
>>>>>>> I don't understand. If device is not registered by devfreq_add_device(),
>>>>>>> you don't change the governor by using governor_store().
>>>>>>>
>>>>>>> If you can change the governor through governor_store(),
>>>>>>> it means that the devfreq instance was added without any problem.
>>>>>>> The added devfreq instance must have the sepecific governor
>>>>>>> on df->governor variable.
>>>>>>>
>>>>>>> So, I think that df->governor is always set and then prev_governor is always set.
>>>>>> Read the code more closely. add_device doesn't (and shouldn't) fail if the governor isn't present. After that, the governor could be changed from user space.
>>>>> If governor is not present during devfreq_add_device(),
>>>>> the devfreq instance is not able to register the devfreq framework.
>>>>> So, the user never touch the sysfs[1] to change the governor
>>>>> because the sysfs[1] is not created by devfreq framework.
>>>>> [1] /sys/class/devfreq/[device name]/governor
>>>>>
>>>>> In summary, if governor is not present during devfreq_add_device(),
>>>>> the devfreq framework doesn't create tye sysfs[1] for governor.
>>>>>
>>>>> The user-space never change the governor through sysfs[1]
>>>>> because there is no any sysfs entry[1].
>>>> I checked the code of devfreq_add_device().
>>>> As you mentioned, if there is no governor,
>>>> devfreq_add_device() don't pass wihtout calling the devfreq->governor->even_handler().
>>> Wrong expression / don't pass -> pass
>>>
>> I think it's correct as is. Just because the governor isn't there (or hasn't registered yet) doesn't mean the device add should fail.
>>
>> That aside, do you care to ack this patch for the way the code is currently?
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> After fixing the bug of devfreq_add_device(),
> I'll remove the unneeded 'if statement' of prev_governor in governor_store.

It's been 2 years and this patch still hasn't been picked up. Can we 
please pick this up and get this into the next kernel release?

Thanks,

Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
  2018-12-07 20:29                     ` Saravana Kannan
@ 2018-12-11  5:35                       ` Chanwoo Choi
       [not found]                       ` <CGME20161026191715epcas1p311c2c626563a736e14c83fb0504c099e@epcms1p3>
  1 sibling, 0 replies; 16+ messages in thread
From: Chanwoo Choi @ 2018-12-11  5:35 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel

Hi Saravana,

The devfreq git repo is maintained by Myungjoo Ham.
you can check it on MAINTAINERS file.

I think that you better to resend mail to mainline
with my reviewed tag because the devfreq core could be modified
and then merge conflict might be happen when apply this patch.

Regards,
Chanwoo Choi

On 2018년 12월 08일 05:29, Saravana Kannan wrote:
> 
> On 11/9/16 4:10 PM, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2016년 11월 10일 05:34, Saravana Kannan wrote:
>>> On 11/08/2016 06:38 PM, Chanwoo Choi wrote:
>>>> On 2016년 11월 09일 11:36, Chanwoo Choi wrote:
>>>>> Hi,
>>>>>
>>>>> On 2016년 11월 09일 10:33, Chanwoo Choi wrote:
>>>>>> On 2016년 11월 09일 05:52, Saravana Kannan wrote:
>>>>>>> On 11/08/2016 01:02 AM, Chanwoo Choi wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 2016년 11월 08일 03:57, Saravana Kannan wrote:
>>>>>>>>> On 10/26/2016 05:06 PM, Chanwoo Choi wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 2016년 10월 27일 04:17, Saravana Kannan wrote:
>>>>>>>>>>> If the new governor fails to start, switch back to old governor so that the
>>>>>>>>>>> devfreq state is not left in some weird limbo.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>>>>>>>>>> ---
>>>>>>>>>>> * Fix NULL deref for real this time.
>>>>>>>>>>> * Addressed some style preferences.
>>>>>>>>>>>
>>>>>>>>>>>      drivers/devfreq/devfreq.c | 13 +++++++++++--
>>>>>>>>>>>      1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>>>>>>> index bf3ea76..77c41a5 100644
>>>>>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>>>>>> @@ -919,7 +919,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>>          struct devfreq *df = to_devfreq(dev);
>>>>>>>>>>>          int ret;
>>>>>>>>>>>          char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>>>>>>> -    struct devfreq_governor *governor;
>>>>>>>>>>> +    const struct devfreq_governor *governor, *prev_governor;
>>>>>>>>>>>
>>>>>>>>>>>          ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>>>>>>>>          if (ret != 1)
>>>>>>>>>>> @@ -944,12 +944,21 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>>                  goto out;
>>>>>>>>>>>              }
>>>>>>>>>>>          }
>>>>>>>>>>> +    prev_governor = df->governor;
>>>>>>>>>>>          df->governor = governor;
>>>>>>>>>>>          strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>>>>>>>>>>          ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>>>>>>>>> -    if (ret)
>>>>>>>>>>> +    if (ret) {
>>>>>>>>>>>              dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>>>>>>>>                   __func__, df->governor->name, ret);
>>>>>>>>>>> +        if (prev_governor) {
>>>>>>>>>> I think that prev_governor is always set. You don't need to check it.
>>>>>>>>>> Why do check it?
>>>>>>>>> Not true. Even higher up in this function, we check if df->governor != NULL. Simple example would be that the default governor passed in while adding the device isn't compiled in.
>>>>>>>> I don't understand. If device is not registered by devfreq_add_device(),
>>>>>>>> you don't change the governor by using governor_store().
>>>>>>>>
>>>>>>>> If you can change the governor through governor_store(),
>>>>>>>> it means that the devfreq instance was added without any problem.
>>>>>>>> The added devfreq instance must have the sepecific governor
>>>>>>>> on df->governor variable.
>>>>>>>>
>>>>>>>> So, I think that df->governor is always set and then prev_governor is always set.
>>>>>>> Read the code more closely. add_device doesn't (and shouldn't) fail if the governor isn't present. After that, the governor could be changed from user space.
>>>>>> If governor is not present during devfreq_add_device(),
>>>>>> the devfreq instance is not able to register the devfreq framework.
>>>>>> So, the user never touch the sysfs[1] to change the governor
>>>>>> because the sysfs[1] is not created by devfreq framework.
>>>>>> [1] /sys/class/devfreq/[device name]/governor
>>>>>>
>>>>>> In summary, if governor is not present during devfreq_add_device(),
>>>>>> the devfreq framework doesn't create tye sysfs[1] for governor.
>>>>>>
>>>>>> The user-space never change the governor through sysfs[1]
>>>>>> because there is no any sysfs entry[1].
>>>>> I checked the code of devfreq_add_device().
>>>>> As you mentioned, if there is no governor,
>>>>> devfreq_add_device() don't pass wihtout calling the devfreq->governor->even_handler().
>>>> Wrong expression / don't pass -> pass
>>>>
>>> I think it's correct as is. Just because the governor isn't there (or hasn't registered yet) doesn't mean the device add should fail.
>>>
>>> That aside, do you care to ack this patch for the way the code is currently?
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>> After fixing the bug of devfreq_add_device(),
>> I'll remove the unneeded 'if statement' of prev_governor in governor_store.
> 
> It's been 2 years and this patch still hasn't been picked up. Can we please pick this up and get this into the next kernel release?
> 
> Thanks,
> 
> Saravana
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start
       [not found]                       ` <CGME20161026191715epcas1p311c2c626563a736e14c83fb0504c099e@epcms1p3>
@ 2018-12-11  7:18                         ` MyungJoo Ham
  0 siblings, 0 replies; 16+ messages in thread
From: MyungJoo Ham @ 2018-12-11  7:18 UTC (permalink / raw)
  To: Chanwoo Choi, Saravana Kannan
  Cc: Rafael J. Wysocki, Kyungmin Park, linux-pm, linux-kernel

> Hi Saravana,
> 
> The devfreq git repo is maintained by Myungjoo Ham.
> you can check it on MAINTAINERS file.
> 
> I think that you better to resend mail to mainline
> with my reviewed tag because the devfreq core could be modified
> and then merge conflict might be happen when apply this patch.
> 
> Regards,
> Chanwoo Choi
> 
> On 2018년 12월 08일 05:29, Saravana Kannan wrote:\r
> > \r
> > On 11/9/16 4:10 PM, Chanwoo Choi wrote:\r
> >> After fixing the bug of devfreq_add_device(),\r
> >> I'll remove the unneeded 'if statement' of prev_governor in governor_store.\r
> > \r
> > It's been 2 years and this patch still hasn't been picked up. Can we please pick this up and get this into the next kernel release?\r
> > \r
> > Thanks,\r
> > \r
> > Saravana\r
> > \r
\r
If that's already 2 years old, please rebase, test to see if that's\r
still valid with today's kernel, and resubmit.\r
\r
Sorry for missing it, right now, after 2 years,\r
I just can't remember this one.\r
\r
Cheers,\r
MyungJoo\r
\r


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-12-11  7:18 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20161026191715epcas1p311c2c626563a736e14c83fb0504c099e@epcas1p3.samsung.com>
2016-10-26 19:17 ` [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start Saravana Kannan
2016-10-27  0:06   ` Chanwoo Choi
2016-11-07 18:57     ` Saravana Kannan
2016-11-08  9:02       ` Chanwoo Choi
2016-11-08 20:52         ` Saravana Kannan
2016-11-09  1:33           ` Chanwoo Choi
2016-11-09  2:36             ` Chanwoo Choi
2016-11-09  2:38               ` Chanwoo Choi
2016-11-09 20:34                 ` Saravana Kannan
2016-11-10  0:10                   ` Chanwoo Choi
2016-11-11 23:25                     ` Saravana Kannan
2016-11-12  2:17                       ` Chanwoo Choi
2016-11-12  2:18                         ` Chanwoo Choi
2018-12-07 20:29                     ` Saravana Kannan
2018-12-11  5:35                       ` Chanwoo Choi
     [not found]                       ` <CGME20161026191715epcas1p311c2c626563a736e14c83fb0504c099e@epcms1p3>
2018-12-11  7:18                         ` MyungJoo Ham

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).