devfreq: replace sscanf with kstrtol

PATCH In review [email protected] 3 participants 6 messages Aug 4, 2017 – Aug 7, 2017 Device frequency (devfreq)

lore

Share & tools

lore

[email protected]

author

2017-08-04 03:57 UTC #0 lore raw

PATCH 1/1 devfreq: replace sscanf with kstrtol

Hi,

Adding error checks to devfreq userspace governor, the current implementation results in setting wrong frequency when sscanf returns error.

From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
From: Santosh Mardi <[email protected]>
Date: Tue, 25 Jul 2017 18:47:11 +0530
Subject: [PATCH] devfreq: replace sscanf with kstrtol

store_freq function of devfreq userspace governor executes further, even if error is returned from sscanf, this will result in setting up wrong frequency value.

The usage for the sscanf is only for single variable so replace sscanf with kstrtol along with error check to bail out if any error is returned.

Signed-off-by Santosh Mardi <[email protected]>

---
 drivers/devfreq/governor_userspace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/devfreq/governor_userspace.c +4 -1

index 77028c2..a84796d 100644
@@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, struct device_attribute *attr,
 	mutex_lock(&devfreq->lock);
 	data = devfreq->data;
 
-	sscanf(buf, "%lu", &wanted);
+	err = kstrtol(buf, 0, &wanted);
+	if (err < 0)
+		goto out;
 	data->user_frequency = wanted;
 	data->valid = true;
 	err = update_devfreq(devfreq);
 	if (err == 0)
 		err = count;
+out:
 	mutex_unlock(&devfreq->lock);
 	return err;
 }

Signature · 3 lines

Regards,
Santosh M G.
Qualcomm Innovation Center

[email protected]

author

2017-08-04 04:23 UTC #1 lore raw

PATCH 1/1 devfreq: replace sscanf with kstrtol

Hi,

Adding error checks to devfreq userspace governor, the current implementation results in setting wrong frequency when sscanf returns error.

store_freq function of devfreq userspace governor executes further, even if error is returned from sscanf, this will result in setting up wrong frequency value.

The usage for the sscanf is only for single variable so replace sscanf with kstrtol along with error check to bail out if any error is returned.

Signed-off-by Santosh Mardi <[email protected]>

---
 drivers/devfreq/governor_userspace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/devfreq/governor_userspace.c +4 -1

index 77028c2..a84796d 100644
@@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, struct device_attribute *attr,
     mutex_lock(&devfreq->lock);
     data = devfreq->data;
 
-    sscanf(buf, "%lu", &wanted);
+    err = kstrtol(buf, 0, &wanted);
+    if (err < 0)
+        goto out;
     data->user_frequency = wanted;
     data->valid = true;
     err = update_devfreq(devfreq);
     if (err == 0)
         err = count;
+out:
     mutex_unlock(&devfreq->lock);
     return err;
 }

Signature · 3 lines

Regards,
Santosh M G.
Qualcomm Innovation Center

Chanwoo Choi

2017-08-04 15:13 UTC #2 lore raw

In reply to [email protected]

Hi,

Quoted 31 lines · On Fri, Aug 4, 2017 at 12:57 PM, <[email protected]> wrote:

Hi,
Adding error checks to devfreq userspace governor, the current implementation results in setting wrong
frequency when sscanf returns error.
From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
From: Santosh Mardi <[email protected]>
Date: Tue, 25 Jul 2017 18:47:11 +0530
Subject: [PATCH] devfreq: replace sscanf with kstrtol
store_freq function of devfreq userspace governor executes further, even if error is returned from sscanf, this will result in setting up wrong frequency value.
The usage for the sscanf is only for single variable so replace sscanf with kstrtol along with error check to bail out if any error is returned.
Signed-off-by Santosh Mardi <[email protected]>
---
 drivers/devfreq/governor_userspace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/devfreq/governor_userspace.c +0 -0
index 77028c2..a84796d 100644

drivers/devfreq/governor_userspace.c static ssize_t store_freq(struct device *dev, struct · line 59

Quoted 8 lines

drivers/devfreq/governor_userspace.c +3 -1

@@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, struct
 device_attribute *attr,
        mutex_lock(&devfreq->lock);
        data = devfreq->data;
 
-       sscanf(buf, "%lu", &wanted);
+       err = kstrtol(buf, 0, &wanted);
+       if (err < 0)
+               goto out;

I think that just you can check the return value as following:
The other point of devfreq already uses the following style to check the return value of sscanf. I think kstrtol is not necessary.

     err = sscanf(buf, "%lu", &wanted);
     if (err != 1)
          goto out;

And please use the scripts/get_maintainer.pl
in order to prevent the missing of the reviewer.

Quoted 14 lines

drivers/devfreq/governor_userspace.c +1 -0

        data->user_frequency = wanted;
        data->valid = true;
        err = update_devfreq(devfreq);
        if (err == 0)
                err = count;
+out:
        mutex_unlock(&devfreq->lock);
        return err;

}
--

Regards,
Santosh M G.
Qualcomm Innovation Center

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

[email protected]

author

2017-08-07 04:47 UTC #3 lore raw

Quoted 48 lines · Chanwoo Choi

Hi,
Quoted 36 lines · On Fri, Aug 4, 2017 at 12:57 PM, <[email protected]> wrote:
Hi,
Adding error checks to devfreq userspace governor, the current implementation results in setting wrong
frequency when sscanf returns error.
From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
From: Santosh Mardi <[email protected]>
Date: Tue, 25 Jul 2017 18:47:11 +0530
Subject: [PATCH] devfreq: replace sscanf with kstrtol
store_freq function of devfreq userspace governor executes further, even if error is returned from sscanf, this will result in setting up wrong frequency value.
The usage for the sscanf is only for single variable so replace sscanf with kstrtol along with error check to bail out if any error is returned.
Signed-off-by Santosh Mardi <[email protected]>
---
 drivers/devfreq/governor_userspace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/devfreq/governor_userspace.c +0 -0
index 77028c2..a84796d 100644
@@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, 
struct
device_attribute *attr,
        mutex_lock(&devfreq->lock);
        data = devfreq->data;
drivers/devfreq/governor_userspace.c static ssize_t store_freq(struct device *dev, struct device_attribute *attr, · line 58
Quoted 4 lines
drivers/devfreq/governor_userspace.c +3 -1
-       sscanf(buf, "%lu", &wanted);
+       err = kstrtol(buf, 0, &wanted);
+       if (err < 0)
+               goto out;
I think that just you can check the return value as following:
The other point of devfreq already uses the following style to check the return value of sscanf. I think kstrtol is not necessary.

Quoted 4 lines

     err = sscanf(buf, "%lu", &wanted);
     if (err != 1)
          goto out;

[Santosh] - I Agree we need to have this error check as mentioned by you if we are scanning an arrary from the sscanf,
but in the above code we are only scanning one variable and there is a rule in the checkpatch scripts, not to use sscanf if it is a single variable, So I need to replace sscanf to strtol

I have added all the mails I got as output from scripts/get_maintainer.pl scripts in this mail.

Quoted 17 lines

And please use the scripts/get_maintainer.pl
in order to prevent the missing of the reviewer.
Quoted 14 lines
        data->user_frequency = wanted;
        data->valid = true;
        err = update_devfreq(devfreq);
        if (err == 0)
                err = count;
+out:
        mutex_unlock(&devfreq->lock);
        return err;
}
--
Regards,
Santosh M G.
Qualcomm Innovation Center

Chanwoo Choi

maintainer

2017-08-07 05:25 UTC #4 lore raw

Hi,

Quoted 48 lines · On 2017년 08월 07일 13:47, [email protected] wrote:

Quoted 47 lines · Chanwoo Choi
Hi,
Quoted 31 lines · On Fri, Aug 4, 2017 at 12:57 PM, <[email protected]> wrote:
Hi,
Adding error checks to devfreq userspace governor, the current implementation results in setting wrong
frequency when sscanf returns error.
From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
From: Santosh Mardi <[email protected]>
Date: Tue, 25 Jul 2017 18:47:11 +0530
Subject: [PATCH] devfreq: replace sscanf with kstrtol
store_freq function of devfreq userspace governor executes further, even if error is returned from sscanf, this will result in setting up wrong frequency value.
The usage for the sscanf is only for single variable so replace sscanf with kstrtol along with error check to bail out if any error is returned.
Signed-off-by Santosh Mardi <[email protected]>
---
 drivers/devfreq/governor_userspace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/devfreq/governor_userspace.c +0 -0
index 77028c2..a84796d 100644
drivers/devfreq/governor_userspace.c static ssize_t store_freq(struct device *dev, struct · line 59
Quoted 8 lines
drivers/devfreq/governor_userspace.c +3 -1
@@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, struct
 device_attribute *attr,
        mutex_lock(&devfreq->lock);
        data = devfreq->data;
 
-       sscanf(buf, "%lu", &wanted);
+       err = kstrtol(buf, 0, &wanted);
+       if (err < 0)
+               goto out;
I think that just you can check the return value as following:
The other point of devfreq already uses the following style to check the return value of sscanf. I think kstrtol is not necessary.

Quoted 7 lines

Quoted 4 lines
     err = sscanf(buf, "%lu", &wanted);
     if (err != 1)
          goto out;
[Santosh] - I Agree we need to have this error check as mentioned by you if we are scanning an arrary from the sscanf,
but in the above code we are only scanning one variable and there is a rule in the checkpatch scripts, not to use sscanf if it is a single variable, So I need to replace sscanf to strtol

IMHO, even if checkpatch shows the warning about sscanf, I'd like you to use 'sscanf' in order to maintain the consistency and readability when handling the sscanf.

For example, drivers/devfreq/devfreq.c and drivers/cpufreq/cpufreq.c have the same warnings on many points.

Quoted 2 lines

I have added all the mails I got as output from scripts/get_maintainer.pl scripts in this mail.

Maybe, you missed including me (reviewer) to cc list.

MyungJoo Ham <[email protected]> (maintainer:DEVICE FREQUENCY (DEVFREQ)) Kyungmin Park <[email protected]> (maintainer:DEVICE FREQUENCY (DEVFREQ)) Chanwoo Choi <[email protected]> (reviewer:DEVICE FREQUENCY (DEVFREQ)) [email protected] (open list:DEVICE FREQUENCY (DEVFREQ)) [email protected] (open list)

Quoted 22 lines

Quoted 19 lines

And please use the scripts/get_maintainer.pl
in order to prevent the missing of the reviewer.

Quoted 14 lines

drivers/devfreq/governor_userspace.c +1 -0

        data->user_frequency = wanted;
        data->valid = true;
        err = update_devfreq(devfreq);
        if (err == 0)
                err = count;
+out:
        mutex_unlock(&devfreq->lock);
        return err;

}

Signature · 3 lines

Regards,
Santosh M G.
Qualcomm Innovation Center

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

[email protected]

author

2017-08-07 12:26 UTC #5 lore raw

Quoted 60 lines · Chanwoo Choi

Hi,
Quoted 49 lines · On 2017년 08월 07일 13:47, [email protected] wrote:
Quoted 48 lines · Chanwoo Choi
Hi,
Quoted 36 lines · On Fri, Aug 4, 2017 at 12:57 PM, <[email protected]> wrote:
Hi,
Adding error checks to devfreq userspace governor, the current implementation results in setting wrong
frequency when sscanf returns error.
From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
From: Santosh Mardi <[email protected]>
Date: Tue, 25 Jul 2017 18:47:11 +0530
Subject: [PATCH] devfreq: replace sscanf with kstrtol
store_freq function of devfreq userspace governor executes further, even if error is returned from sscanf, this will result in setting up wrong frequency value.
The usage for the sscanf is only for single variable so replace sscanf with kstrtol along with error check to bail out if any error is returned.
Signed-off-by Santosh Mardi <[email protected]>
---
 drivers/devfreq/governor_userspace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/devfreq/governor_userspace.c +0 -0
index 77028c2..a84796d 100644
@@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, 
struct
device_attribute *attr,
        mutex_lock(&devfreq->lock);
        data = devfreq->data;
drivers/devfreq/governor_userspace.c static ssize_t store_freq(struct device *dev, struct device_attribute *attr, · line 58
Quoted 4 lines
drivers/devfreq/governor_userspace.c +3 -1
-       sscanf(buf, "%lu", &wanted);
+       err = kstrtol(buf, 0, &wanted);
+       if (err < 0)
+               goto out;
I think that just you can check the return value as following:
The other point of devfreq already uses the following style to check the return value of sscanf. I think kstrtol is not necessary.
Quoted 7 lines
Quoted 4 lines
     err = sscanf(buf, "%lu", &wanted);
     if (err != 1)
          goto out;
[Santosh] - I Agree we need to have this error check as mentioned by you if we are scanning an arrary from the sscanf,
but in the above code we are only scanning one variable and there is a rule in the checkpatch scripts, not to use sscanf if it is a single variable, So I need to replace sscanf to strtol

Quoted 6 lines

IMHO, even if checkpatch shows the warning about sscanf, I'd like you to use 'sscanf' in order to maintain the consistency and readability when handling the sscanf.
For example, drivers/devfreq/devfreq.c and drivers/cpufreq/cpufreq.c have the same warnings on many points.

[Santosh] - Thanks, will change the patch to add the error check for sscanf and bail out.

Quoted 4 lines

Quoted 3 lines
I have added all the mails I got as output from scripts/get_maintainer.pl scripts in this mail.

Quoted 9 lines

Maybe, you missed including me (reviewer) to cc list.
MyungJoo Ham <[email protected]> (maintainer:DEVICE FREQUENCY (DEVFREQ)) Kyungmin Park <[email protected]> (maintainer:DEVICE FREQUENCY (DEVFREQ))
Chanwoo Choi <[email protected]> (reviewer:DEVICE FREQUENCY (DEVFREQ)) [email protected] (open list:DEVICE FREQUENCY (DEVFREQ)) [email protected] (open list)

[Santosh] - Sorry, May be I am using bit older script file, will cross check this in future.

Quoted 22 lines

Quoted 22 lines
Quoted 19 lines
And please use the scripts/get_maintainer.pl
in order to prevent the missing of the reviewer.
Quoted 14 lines
        data->user_frequency = wanted;
        data->valid = true;
        err = update_devfreq(devfreq);
        if (err == 0)
                err = count;
+out:
        mutex_unlock(&devfreq->lock);
        return err;
}
--
Regards,
Santosh M G.
Qualcomm Innovation Center