From patchwork Thu May 2 17:31:49 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eduardo Valentin X-Patchwork-Id: 2513311 Return-Path: X-Original-To: patchwork-linux-acpi@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 1D11EDFB76 for ; Thu, 2 May 2013 17:32:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757497Ab3EBRcG (ORCPT ); Thu, 2 May 2013 13:32:06 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:33418 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753432Ab3EBRcE (ORCPT ); Thu, 2 May 2013 13:32:04 -0400 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by comal.ext.ti.com (8.13.7/8.13.7) with ESMTP id r42HW3JX024763; Thu, 2 May 2013 12:32:03 -0500 Received: from DFLE72.ent.ti.com (dfle72.ent.ti.com [128.247.5.109]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id r42HW2RQ024346; Thu, 2 May 2013 12:32:03 -0500 Received: from dlelxv22.itg.ti.com (172.17.1.197) by DFLE72.ent.ti.com (128.247.5.109) with Microsoft SMTP Server id 14.2.342.3; Thu, 2 May 2013 12:32:02 -0500 Received: from [172.24.68.12] (h68-12.vpn.ti.com [172.24.68.12]) by dlelxv22.itg.ti.com (8.13.8/8.13.8) with ESMTP id r42HVxv9006014; Thu, 2 May 2013 12:32:00 -0500 Message-ID: <5182A305.6000504@ti.com> Date: Thu, 2 May 2013 13:31:49 -0400 From: Eduardo Valentin User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130308 Thunderbird/17.0.4 MIME-Version: 1.0 To: Srinivas Pandruvada CC: Eduardo Valentin , , , Subject: Re: [PATCH] thermal: Can't set policy References: <1367008557-12443-1-git-send-email-srinivas.pandruvada@linux.intel.com> <517AEFC0.2010301@ti.com> <517B0E9D.6000904@linux.intel.com> <5181F0A3.90005@linux.intel.com> In-Reply-To: <5181F0A3.90005@linux.intel.com> X-Enigmail-Version: 1.5.1 Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org 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: >> - 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 >>>> >>>> --- >>>> 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 >>> 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 >>> >>> 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; >>>> >>>> >>> >>> >> >> > > > 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++; }