Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1508742 > unrolled thread
| Started by | Saravana Kannan <skannan@codeaurora.org> |
|---|---|
| First post | 2016-10-26 03:30 +0200 |
| Last post | 2016-10-26 21:20 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v2] PM / devfreq: Restart previous governor if new governor fails to start Saravana Kannan <skannan@codeaurora.org> - 2016-10-26 03:30 +0200
Re: [PATCH v2] PM / devfreq: Restart previous governor if new governor fails to start Chanwoo Choi <cw00.choi@samsung.com> - 2016-10-26 09:30 +0200
Re: [PATCH v2] PM / devfreq: Restart previous governor if new governor fails to start Saravana Kannan <skannan@codeaurora.org> - 2016-10-26 21:20 +0200
| From | Saravana Kannan <skannan@codeaurora.org> |
|---|---|
| Date | 2016-10-26 03:30 +0200 |
| Subject | [PATCH v2] PM / devfreq: Restart previous governor if new governor fails to start |
| Message-ID | <swlcu-54I-11@gated-at.bofh.it> |
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>
---
* Fixed typo in commit text
* Fixed potential NULL deref
drivers/devfreq/devfreq.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index bf3ea76..243253e 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_gov;
ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
if (ret != 1)
@@ -944,12 +944,17 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
goto out;
}
}
+ prev_gov = 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);
+ df->governor = prev_gov;
+ strncpy(df->governor_name, prev_gov->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
[toc] | [next] | [standalone]
| From | Chanwoo Choi <cw00.choi@samsung.com> |
|---|---|
| Date | 2016-10-26 09:30 +0200 |
| Subject | Re: [PATCH v2] PM / devfreq: Restart previous governor if new governor fails to start |
| Message-ID | <swqOS-pQ-11@gated-at.bofh.it> |
| In reply to | #1508742 |
Hi,
On 2016년 10월 26일 10:25, 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>
> ---
> * Fixed typo in commit text
> * Fixed potential NULL deref
>
> drivers/devfreq/devfreq.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index bf3ea76..243253e 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_gov;
>
> ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
> if (ret != 1)
> @@ -944,12 +944,17 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
> goto out;
> }
> }
> + prev_gov = 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);
> + df->governor = prev_gov;
> + strncpy(df->governor_name, prev_gov->name, DEVFREQ_NAME_LEN);
> + df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> + }
> out:
> mutex_unlock(&devfreq_list_lock);
>
>
Looks good to me.
But, how about using the 'prev_governor' instead of 'prev_gov'?
because the variable name of current governor is 'governor' instead of 'gov'.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
[toc] | [prev] | [next] | [standalone]
| From | Saravana Kannan <skannan@codeaurora.org> |
|---|---|
| Date | 2016-10-26 21:20 +0200 |
| Subject | Re: [PATCH v2] PM / devfreq: Restart previous governor if new governor fails to start |
| Message-ID | <swBTY-83B-31@gated-at.bofh.it> |
| In reply to | #1508871 |
On 10/26/2016 12:22 AM, Chanwoo Choi wrote:
> Hi,
>
> On 2016년 10월 26일 10:25, 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>
>> ---
>> * Fixed typo in commit text
>> * Fixed potential NULL deref
>>
>> drivers/devfreq/devfreq.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index bf3ea76..243253e 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_gov;
>>
>> ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>> if (ret != 1)
>> @@ -944,12 +944,17 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>> goto out;
>> }
>> }
>> + prev_gov = 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);
>> + df->governor = prev_gov;
>> + strncpy(df->governor_name, prev_gov->name, DEVFREQ_NAME_LEN);
>> + df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>> + }
Sigh... looks like I added the changes to the index but forgot to commit
it before I sent the patch. Will send v3.
>> out:
>> mutex_unlock(&devfreq_list_lock);
>>
>>
>
> Looks good to me.
> But, how about using the 'prev_governor' instead of 'prev_gov'?
> because the variable name of current governor is 'governor' instead of 'gov'.
I was trying to avoid wrapping lines too much. But this doesn't cause
any additional wraps. Done.
>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
Can you please review v3?
Thanks,
Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web