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

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

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