* [PATCH] thermal: Can't set policy @ 2013-04-26 20:35 Srinivas Pandruvada 2013-04-26 21:21 ` Eduardo Valentin 0 siblings, 1 reply; 5+ messages in thread From: Srinivas Pandruvada @ 2013-04-26 20:35 UTC (permalink / raw) To: linux-acpi; +Cc: rui.zhang, linux-pm, Srinivas Pandruvada Setting policy results in invalid value error. >echo "step_wise" > policy >echo: write error: Invalid argument Need clean up of the buffer which "echo" may add based on the arguments, before comparing aganist list of governor names. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- drivers/thermal/thermal_core.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 4cdc3e3..ed6904f 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -696,16 +696,27 @@ static ssize_t policy_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { - int ret = -EINVAL; + int ret; struct thermal_zone_device *tz = to_thermal_zone(dev); struct thermal_governor *gov; + char str_gov[THERMAL_NAME_LENGTH]; + char format[6]; /* enough for 3 digit format width */ + + ret = snprintf(format, sizeof(format), "%%%ds", + THERMAL_NAME_LENGTH - 1); + if (ret < 0) + return ret; + ret = sscanf(buf, format, str_gov); + if (ret <= 0) + return -EINVAL; mutex_lock(&thermal_governor_lock); - gov = __find_governor(buf); - if (!gov) + gov = __find_governor(str_gov); + if (!gov) { + ret = -EINVAL; goto exit; - + } tz->governor = gov; ret = count; -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] thermal: Can't set policy 2013-04-26 20:35 [PATCH] thermal: Can't set policy Srinivas Pandruvada @ 2013-04-26 21:21 ` Eduardo Valentin [not found] ` <517B0E9D.6000904@linux.intel.com> 0 siblings, 1 reply; 5+ messages in thread From: Eduardo Valentin @ 2013-04-26 21:21 UTC (permalink / raw) To: Srinivas Pandruvada; +Cc: linux-acpi, rui.zhang, linux-pm, eduardo.valentin Hello Srinivas, On 26-04-2013 16:35, Srinivas Pandruvada wrote: > Setting policy results in invalid value error. > >echo "step_wise" > policy > >echo: write error: Invalid argument > > Need clean up of the buffer which "echo" may add based on the > arguments, before comparing aganist list of governor names. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > drivers/thermal/thermal_core.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 4cdc3e3..ed6904f 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -696,16 +696,27 @@ static ssize_t > policy_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t count) > { > - int ret = -EINVAL; > + int ret; > struct thermal_zone_device *tz = to_thermal_zone(dev); > struct thermal_governor *gov; > + char str_gov[THERMAL_NAME_LENGTH]; > + char format[6]; /* enough for 3 digit format width */ > + > + ret = snprintf(format, sizeof(format), "%%%ds", > + THERMAL_NAME_LENGTH - 1); > + if (ret < 0) > + return ret; > + ret = sscanf(buf, format, str_gov); > + if (ret <= 0) > + return -EINVAL; Is this due to trainling \n? Why not using sysfs_streq()? I believe it is better approach. Can you please check if the following solves the issue? https://gitorious.org/thermal-framework/thermal-framework/commit/810a33a629b40adfa92a164883281bbdfad80516 commit 810a33a629b40adfa92a164883281bbdfad80516 Author: Eduardo Valentin <eduardo.valentin@ti.com> Date: Fri Apr 26 17:12:30 2013 -0400 thermal: better string treatment while finding governors To avoid returning error value just because of trailing \n, this patch changes the function to find governors by name to use sysfs_streq(). Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index f36cd44..08ea62c 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -58,7 +58,7 @@ static struct thermal_governor *__find_governor(const char *name) struct thermal_governor *pos; list_for_each_entry(pos, &thermal_governor_list, governor_list) - if (!strnicmp(name, pos->name, THERMAL_NAME_LENGTH)) + if (sysfs_streq(name, pos->name)) return pos; return NULL; > > mutex_lock(&thermal_governor_lock); > > - gov = __find_governor(buf); > - if (!gov) > + gov = __find_governor(str_gov); > + if (!gov) { > + ret = -EINVAL; > goto exit; > - > + } > tz->governor = gov; > ret = count; > > ^ permalink raw reply related [flat|nested] 5+ messages in thread
[parent not found: <517B0E9D.6000904@linux.intel.com>]
* Re: [PATCH] thermal: Can't set policy [not found] ` <517B0E9D.6000904@linux.intel.com> @ 2013-05-02 4:50 ` Srinivas Pandruvada 2013-05-02 17:31 ` Eduardo Valentin 0 siblings, 1 reply; 5+ messages in thread From: Srinivas Pandruvada @ 2013-05-02 4:50 UTC (permalink / raw) To: Eduardo Valentin; +Cc: linux-acpi, rui.zhang, linux-pm Hi Valentin, Any further thoughts? Thanks, Srinivas On 04/26/2013 04:32 PM, Srinivas Pandruvada wrote: > Hi Valentin, > > I planned to use that before , but it will change the semantics here. > > - Here comparison is using a case in-sensitive version, sysfs_streq is > case sensitive. > - I am not sure if there is any protection for length. If we pass a > more than THERM_NAME_LENGTH string, then whether sysfs_streq can > handle. so we need to pre-check for length. > - some stupid echo command with CRLF, will still fail > I see that some other syfs string write (eg. cpufreq.c) decided not to > use sysfs_streq, may be historical. > > Thanks, > Srinivas > > > On 04/26/2013 02:21 PM, Eduardo Valentin wrote: >> Hello Srinivas, >> >> On 26-04-2013 16:35, Srinivas Pandruvada wrote: >>> Setting policy results in invalid value error. >>> >echo "step_wise" > policy >>> >echo: write error: Invalid argument >>> >>> Need clean up of the buffer which "echo" may add based on the >>> arguments, before comparing aganist list of governor names. >>> >>> Signed-off-by: Srinivas Pandruvada >>> <srinivas.pandruvada@linux.intel.com> >>> --- >>> drivers/thermal/thermal_core.c | 19 +++++++++++++++---- >>> 1 file changed, 15 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/thermal/thermal_core.c >>> b/drivers/thermal/thermal_core.c >>> index 4cdc3e3..ed6904f 100644 >>> --- a/drivers/thermal/thermal_core.c >>> +++ b/drivers/thermal/thermal_core.c >>> @@ -696,16 +696,27 @@ static ssize_t >>> policy_store(struct device *dev, struct device_attribute *attr, >>> const char *buf, size_t count) >>> { >>> - int ret = -EINVAL; >>> + int ret; >>> struct thermal_zone_device *tz = to_thermal_zone(dev); >>> struct thermal_governor *gov; >>> + char str_gov[THERMAL_NAME_LENGTH]; >>> + char format[6]; /* enough for 3 digit format width */ >>> + >>> + ret = snprintf(format, sizeof(format), "%%%ds", >>> + THERMAL_NAME_LENGTH - 1); >>> + if (ret < 0) >>> + return ret; >>> + ret = sscanf(buf, format, str_gov); >>> + if (ret <= 0) >>> + return -EINVAL; >> >> >> Is this due to trainling \n? Why not using sysfs_streq()? I believe >> it is better approach. Can you please check if the following solves >> the issue? >> >> https://gitorious.org/thermal-framework/thermal-framework/commit/810a33a629b40adfa92a164883281bbdfad80516 >> >> >> commit 810a33a629b40adfa92a164883281bbdfad80516 >> Author: Eduardo Valentin <eduardo.valentin@ti.com> >> Date: Fri Apr 26 17:12:30 2013 -0400 >> >> thermal: better string treatment while finding governors >> >> To avoid returning error value just because of trailing >> \n, this patch changes the function to find governors >> by name to use sysfs_streq(). >> >> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com> >> >> diff --git a/drivers/thermal/thermal_core.c >> b/drivers/thermal/thermal_core.c >> index f36cd44..08ea62c 100644 >> --- a/drivers/thermal/thermal_core.c >> +++ b/drivers/thermal/thermal_core.c >> @@ -58,7 +58,7 @@ static struct thermal_governor >> *__find_governor(const char *name) >> struct thermal_governor *pos; >> >> list_for_each_entry(pos, &thermal_governor_list, governor_list) >> - if (!strnicmp(name, pos->name, THERMAL_NAME_LENGTH)) >> + if (sysfs_streq(name, pos->name)) >> return pos; >> >> return NULL; >> >>> >>> mutex_lock(&thermal_governor_lock); >>> >>> - gov = __find_governor(buf); >>> - if (!gov) >>> + gov = __find_governor(str_gov); >>> + if (!gov) { >>> + ret = -EINVAL; >>> goto exit; >>> - >>> + } >>> tz->governor = gov; >>> ret = count; >>> >>> >> >> > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] thermal: Can't set policy 2013-05-02 4:50 ` Srinivas Pandruvada @ 2013-05-02 17:31 ` Eduardo Valentin 0 siblings, 0 replies; 5+ messages in thread From: Eduardo Valentin @ 2013-05-02 17:31 UTC (permalink / raw) To: Srinivas Pandruvada; +Cc: Eduardo Valentin, linux-acpi, rui.zhang, linux-pm [-- Attachment #1: Type: text/plain, Size: 5567 bytes --] Srivinas, On 02-05-2013 00:50, Srinivas Pandruvada wrote: > Hi Valentin, > > Any further thoughts? Yeah, > > Thanks, > Srinivas > > On 04/26/2013 04:32 PM, Srinivas Pandruvada wrote: >> Hi Valentin, >> >> I planned to use that before , but it will change the semantics here. >> >> - Here comparison is using a case in-sensitive version, sysfs_streq is >> case sensitive. Indeed. That changes the semantics. On the other hand, I do not see why we need to be case in-sensitive. Depending on how you see this, it might infringe the sysfs rules. Besides, I am favor to have 1 to 1 relation between array of chars and their meaning. On the other hand, changing the semantics might break userland. I personally do not have any userland application that uses this sysfs nodes. Neither I am aware of any. But I request Rui and Durga s opinions here. >> - I am not sure if there is any protection for length. If we pass a >> more than THERM_NAME_LENGTH string, then whether sysfs_streq can >> handle. so we need to pre-check for length. sysfs_streq does not check for length, only for string ending char and CRLF. In fact we would need to check for string ending before using it. Side note, I believe we might consider the following: diff --git a/lib/string.c b/lib/string.c index e5878de..b83eee7 100644 --- a/lib/string.c +++ b/lib/string.c @@ -522,7 +522,7 @@ EXPORT_SYMBOL(strsep); */ bool sysfs_streq(const char *s1, const char *s2) { - while (*s1 && *s1 == *s2) { + while (*s1 && *s2 && *s1 == *s2) { s1++; s2++; } >> - some stupid echo command with CRLF, will still fail sure about that? sysfs_streq() was actually introduced to avoid it. If yes, then we have a bug on it. >> I see that some other syfs string write (eg. cpufreq.c) decided not to >> use sysfs_streq, may be historical. >> yeah, maybe. But have you git greped for sysfs_streq()? Obviously, your patch provides more protection. On the other hand, I believe we need to check if we fix this locally or if we need to fix sysfs_streq() too. Providing a sysfs_strneq() would also be a good thing. >> Thanks, >> Srinivas >> >> >> On 04/26/2013 02:21 PM, Eduardo Valentin wrote: >>> Hello Srinivas, >>> >>> On 26-04-2013 16:35, Srinivas Pandruvada wrote: >>>> Setting policy results in invalid value error. >>>> >echo "step_wise" > policy >>>> >echo: write error: Invalid argument >>>> >>>> Need clean up of the buffer which "echo" may add based on the >>>> arguments, before comparing aganist list of governor names. >>>> >>>> Signed-off-by: Srinivas Pandruvada >>>> <srinivas.pandruvada@linux.intel.com> >>>> --- >>>> drivers/thermal/thermal_core.c | 19 +++++++++++++++---- >>>> 1 file changed, 15 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/thermal/thermal_core.c >>>> b/drivers/thermal/thermal_core.c >>>> index 4cdc3e3..ed6904f 100644 >>>> --- a/drivers/thermal/thermal_core.c >>>> +++ b/drivers/thermal/thermal_core.c >>>> @@ -696,16 +696,27 @@ static ssize_t >>>> policy_store(struct device *dev, struct device_attribute *attr, >>>> const char *buf, size_t count) >>>> { >>>> - int ret = -EINVAL; >>>> + int ret; >>>> struct thermal_zone_device *tz = to_thermal_zone(dev); >>>> struct thermal_governor *gov; >>>> + char str_gov[THERMAL_NAME_LENGTH]; >>>> + char format[6]; /* enough for 3 digit format width */ >>>> + >>>> + ret = snprintf(format, sizeof(format), "%%%ds", >>>> + THERMAL_NAME_LENGTH - 1); >>>> + if (ret < 0) >>>> + return ret; >>>> + ret = sscanf(buf, format, str_gov); >>>> + if (ret <= 0) >>>> + return -EINVAL; >>> >>> >>> Is this due to trainling \n? Why not using sysfs_streq()? I believe >>> it is better approach. Can you please check if the following solves >>> the issue? >>> >>> https://gitorious.org/thermal-framework/thermal-framework/commit/810a33a629b40adfa92a164883281bbdfad80516 >>> >>> >>> commit 810a33a629b40adfa92a164883281bbdfad80516 >>> Author: Eduardo Valentin <eduardo.valentin@ti.com> >>> Date: Fri Apr 26 17:12:30 2013 -0400 >>> >>> thermal: better string treatment while finding governors >>> >>> To avoid returning error value just because of trailing >>> \n, this patch changes the function to find governors >>> by name to use sysfs_streq(). >>> >>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com> >>> >>> diff --git a/drivers/thermal/thermal_core.c >>> b/drivers/thermal/thermal_core.c >>> index f36cd44..08ea62c 100644 >>> --- a/drivers/thermal/thermal_core.c >>> +++ b/drivers/thermal/thermal_core.c >>> @@ -58,7 +58,7 @@ static struct thermal_governor >>> *__find_governor(const char *name) >>> struct thermal_governor *pos; >>> >>> list_for_each_entry(pos, &thermal_governor_list, governor_list) >>> - if (!strnicmp(name, pos->name, THERMAL_NAME_LENGTH)) >>> + if (sysfs_streq(name, pos->name)) >>> return pos; >>> >>> return NULL; >>> >>>> >>>> mutex_lock(&thermal_governor_lock); >>>> >>>> - gov = __find_governor(buf); >>>> - if (!gov) >>>> + gov = __find_governor(str_gov); >>>> + if (!gov) { >>>> + ret = -EINVAL; >>>> goto exit; >>>> - >>>> + } >>>> tz->governor = gov; >>>> ret = count; >>>> >>>> >>> >>> >> >> > > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 295 bytes --] ^ permalink raw reply related [flat|nested] 5+ messages in thread
[parent not found: <1366989432-4825-1-git-send-email-srinivas.pandruvada@linux.intel.com>]
* Re: [PATCH] thermal: Can't set policy [not found] <1366989432-4825-1-git-send-email-srinivas.pandruvada@linux.intel.com> @ 2013-04-26 20:33 ` Rafael J. Wysocki 0 siblings, 0 replies; 5+ messages in thread From: Rafael J. Wysocki @ 2013-04-26 20:33 UTC (permalink / raw) To: Srinivas Pandruvada; +Cc: linux-acpi, rui.zhang, Linux PM list On Friday, April 26, 2013 08:17:12 AM Srinivas Pandruvada wrote: > Setting policy results in invalid value error. > >echo "step_wise" > policy > >echo: write error: Invalid argument > > Need clean up of the buffer which "echo" may add based on the > arguments, before comparing aganist list of governor names. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> This needs to go to linux-pm@vger.kernel.org, so that the people who work on thermal have chance to comment. Please resend. Thanks, Rafael > --- > drivers/thermal/thermal_core.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 4cdc3e3..ed6904f 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -696,16 +696,27 @@ static ssize_t > policy_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t count) > { > - int ret = -EINVAL; > + int ret; > struct thermal_zone_device *tz = to_thermal_zone(dev); > struct thermal_governor *gov; > + char str_gov[THERMAL_NAME_LENGTH]; > + char format[6]; /* enough for 3 digit format width */ > + > + ret = snprintf(format, sizeof(format), "%%%ds", > + THERMAL_NAME_LENGTH - 1); > + if (ret < 0) > + return ret; > + ret = sscanf(buf, format, str_gov); > + if (ret <= 0) > + return -EINVAL; > > mutex_lock(&thermal_governor_lock); > > - gov = __find_governor(buf); > - if (!gov) > + gov = __find_governor(str_gov); > + if (!gov) { > + ret = -EINVAL; > goto exit; > - > + } > tz->governor = gov; > ret = count; > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-05-02 17:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-26 20:35 [PATCH] thermal: Can't set policy Srinivas Pandruvada
2013-04-26 21:21 ` Eduardo Valentin
[not found] ` <517B0E9D.6000904@linux.intel.com>
2013-05-02 4:50 ` Srinivas Pandruvada
2013-05-02 17:31 ` Eduardo Valentin
[not found] <1366989432-4825-1-git-send-email-srinivas.pandruvada@linux.intel.com>
2013-04-26 20:33 ` Rafael J. Wysocki
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).