Re: [PATCH] thermal: Can't set policy

Re: [PATCH] thermal: Can't set policy

[Rafael J. Wysocki]

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.

[PATCH] thermal: Can't set policy

[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

Re: [PATCH] thermal: Can't set policy

[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;
>
>

Re: [PATCH] thermal: Can't set policy

[Srinivas Pandruvada]

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

Re: [PATCH] thermal: Can't set policy

[Eduardo Valentin]

[-- 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 --]