* [patch] thermal: underflow in trip_point_temp_store()
@ 2015-11-03 22:14 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2015-11-03 22:14 UTC (permalink / raw
To: Zhang Rui; +Cc: Eduardo Valentin, linux-pm, kernel-janitors
This is to address a static checker warning about an underflow in
imx_set_trip_temp(). The checker is complaining that we have a user
supplied value for "temp" from kstrtoul() where we treat it as signed,
we cap the upper but we accept negative values.
This looks unintentional since the caller is using unsigned longs to
represent the temperature. Let's change it to int and reject negatives
in the caller.
Also I changed it to reject negative "trip" values as well.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Someday we will use super cooled CPUs and we will need to rethink this
code. :)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9e525c..151a630 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -664,7 +664,7 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
int trip, ret;
- unsigned long temperature;
+ int temperature;
if (!tz->ops->set_trip_temp)
return -EPERM;
@@ -672,7 +672,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
if (!sscanf(attr->attr.name, "trip_point_%d_temp", &trip))
return -EINVAL;
- if (kstrtoul(buf, 10, &temperature))
+ if (kstrtoint(buf, 10, &temperature))
+ return -EINVAL;
+ if (trip < 0 || temperature < 0)
return -EINVAL;
ret = tz->ops->set_trip_temp(tz, trip, temperature);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [patch] thermal: underflow in trip_point_temp_store()
@ 2015-11-03 22:14 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2015-11-03 22:14 UTC (permalink / raw
To: Zhang Rui; +Cc: Eduardo Valentin, linux-pm, kernel-janitors
This is to address a static checker warning about an underflow in
imx_set_trip_temp(). The checker is complaining that we have a user
supplied value for "temp" from kstrtoul() where we treat it as signed,
we cap the upper but we accept negative values.
This looks unintentional since the caller is using unsigned longs to
represent the temperature. Let's change it to int and reject negatives
in the caller.
Also I changed it to reject negative "trip" values as well.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Someday we will use super cooled CPUs and we will need to rethink this
code. :)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9e525c..151a630 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -664,7 +664,7 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
int trip, ret;
- unsigned long temperature;
+ int temperature;
if (!tz->ops->set_trip_temp)
return -EPERM;
@@ -672,7 +672,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
if (!sscanf(attr->attr.name, "trip_point_%d_temp", &trip))
return -EINVAL;
- if (kstrtoul(buf, 10, &temperature))
+ if (kstrtoint(buf, 10, &temperature))
+ return -EINVAL;
+ if (trip < 0 || temperature < 0)
return -EINVAL;
ret = tz->ops->set_trip_temp(tz, trip, temperature);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch] thermal: underflow in trip_point_temp_store()
2015-11-03 22:14 ` Dan Carpenter
@ 2015-11-04 5:57 ` Eduardo Valentin
-1 siblings, 0 replies; 10+ messages in thread
From: Eduardo Valentin @ 2015-11-04 5:57 UTC (permalink / raw
To: Dan Carpenter; +Cc: Zhang Rui, linux-pm, kernel-janitors
Hello Dan,
On Wed, Nov 04, 2015 at 01:14:34AM +0300, Dan Carpenter wrote:
> This is to address a static checker warning about an underflow in
> imx_set_trip_temp(). The checker is complaining that we have a user
> supplied value for "temp" from kstrtoul() where we treat it as signed,
> we cap the upper but we accept negative values.
>
> This looks unintentional since the caller is using unsigned longs to
> represent the temperature. Let's change it to int and reject negatives
> in the caller.
>
> Also I changed it to reject negative "trip" values as well.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Someday we will use super cooled CPUs and we will need to rethink this
> code. :)
>
I wish cpus would be the only type of devices covered here :-)
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9e525c..151a630 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -664,7 +664,7 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> {
> struct thermal_zone_device *tz = to_thermal_zone(dev);
> int trip, ret;
> - unsigned long temperature;
> + int temperature;
>
> if (!tz->ops->set_trip_temp)
> return -EPERM;
> @@ -672,7 +672,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> if (!sscanf(attr->attr.name, "trip_point_%d_temp", &trip))
> return -EINVAL;
>
> - if (kstrtoul(buf, 10, &temperature))
> + if (kstrtoint(buf, 10, &temperature))
> + return -EINVAL;
> + if (trip < 0 || temperature < 0)
While this maintains the original code semantics, I would prefer we
could accept negative values here. Main reason is to maintain the range
accepted by the current type in use for temperature. Second reason is
that I heard reports of people willing to use this code to control
thermal zones that would be at temperatures below 0.
Also, while here, could you please help cleaning emul_temp_store?
Thanks for sending this patch.
BR,
Eduardo
> return -EINVAL;
>
> ret = tz->ops->set_trip_temp(tz, trip, temperature);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] thermal: underflow in trip_point_temp_store()
@ 2015-11-04 5:57 ` Eduardo Valentin
0 siblings, 0 replies; 10+ messages in thread
From: Eduardo Valentin @ 2015-11-04 5:57 UTC (permalink / raw
To: Dan Carpenter; +Cc: Zhang Rui, linux-pm, kernel-janitors
Hello Dan,
On Wed, Nov 04, 2015 at 01:14:34AM +0300, Dan Carpenter wrote:
> This is to address a static checker warning about an underflow in
> imx_set_trip_temp(). The checker is complaining that we have a user
> supplied value for "temp" from kstrtoul() where we treat it as signed,
> we cap the upper but we accept negative values.
>
> This looks unintentional since the caller is using unsigned longs to
> represent the temperature. Let's change it to int and reject negatives
> in the caller.
>
> Also I changed it to reject negative "trip" values as well.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Someday we will use super cooled CPUs and we will need to rethink this
> code. :)
>
I wish cpus would be the only type of devices covered here :-)
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9e525c..151a630 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -664,7 +664,7 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> {
> struct thermal_zone_device *tz = to_thermal_zone(dev);
> int trip, ret;
> - unsigned long temperature;
> + int temperature;
>
> if (!tz->ops->set_trip_temp)
> return -EPERM;
> @@ -672,7 +672,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> if (!sscanf(attr->attr.name, "trip_point_%d_temp", &trip))
> return -EINVAL;
>
> - if (kstrtoul(buf, 10, &temperature))
> + if (kstrtoint(buf, 10, &temperature))
> + return -EINVAL;
> + if (trip < 0 || temperature < 0)
While this maintains the original code semantics, I would prefer we
could accept negative values here. Main reason is to maintain the range
accepted by the current type in use for temperature. Second reason is
that I heard reports of people willing to use this code to control
thermal zones that would be at temperatures below 0.
Also, while here, could you please help cleaning emul_temp_store?
Thanks for sending this patch.
BR,
Eduardo
> return -EINVAL;
>
> ret = tz->ops->set_trip_temp(tz, trip, temperature);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] thermal: underflow in trip_point_temp_store()
2015-11-04 5:57 ` Eduardo Valentin
@ 2015-11-04 11:32 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2015-11-04 11:32 UTC (permalink / raw
To: Eduardo Valentin; +Cc: Zhang Rui, linux-pm, kernel-janitors
Oh... Hm. I actually sent a totally different patch to fix this
warning and you just merged it. I had forgotten about that. As far as
I am concerned this issue is solved. :) Sorry for sending duplicates.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] thermal: underflow in trip_point_temp_store()
@ 2015-11-04 11:32 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2015-11-04 11:32 UTC (permalink / raw
To: Eduardo Valentin; +Cc: Zhang Rui, linux-pm, kernel-janitors
Oh... Hm. I actually sent a totally different patch to fix this
warning and you just merged it. I had forgotten about that. As far as
I am concerned this issue is solved. :) Sorry for sending duplicates.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] thermal: underflow in trip_point_temp_store()
2015-11-03 22:14 ` Dan Carpenter
@ 2015-11-04 12:26 ` walter harms
-1 siblings, 0 replies; 10+ messages in thread
From: walter harms @ 2015-11-04 12:26 UTC (permalink / raw
To: Dan Carpenter; +Cc: Zhang Rui, Eduardo Valentin, linux-pm, kernel-janitors
Am 03.11.2015 23:14, schrieb Dan Carpenter:
> This is to address a static checker warning about an underflow in
> imx_set_trip_temp(). The checker is complaining that we have a user
> supplied value for "temp" from kstrtoul() where we treat it as signed,
> we cap the upper but we accept negative values.
>
> This looks unintentional since the caller is using unsigned longs to
> represent the temperature. Let's change it to int and reject negatives
> in the caller.
>
> Also I changed it to reject negative "trip" values as well.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Someday we will use super cooled CPUs and we will need to rethink this
> code. :)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9e525c..151a630 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -664,7 +664,7 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> {
> struct thermal_zone_device *tz = to_thermal_zone(dev);
> int trip, ret;
> - unsigned long temperature;
> + int temperature;
>
> if (!tz->ops->set_trip_temp)
> return -EPERM;
> @@ -672,7 +672,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> if (!sscanf(attr->attr.name, "trip_point_%d_temp", &trip))
> return -EINVAL;
>
> - if (kstrtoul(buf, 10, &temperature))
> + if (kstrtoint(buf, 10, &temperature))
> + return -EINVAL;
> + if (trip < 0 || temperature < 0)
> return -EINVAL;
>
> ret = tz->ops->set_trip_temp(tz, trip, temperature);
IMHO the test should be near the point where the value is generated.
if (!sscanf(attr->attr.name, "trip_point_%d_temp", &trip))
return -EINVAL;
if (trip < 0)
return -EINVAL;
if (kstrtoint(buf, 10, &temperature))
return -EINVAL;
if (temperature < 0)
return -EINVAL;
That way it is easily visible under what condition -EINVAL is generated (to many).
hope that helps,
re,
wh
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] thermal: underflow in trip_point_temp_store()
@ 2015-11-04 12:26 ` walter harms
0 siblings, 0 replies; 10+ messages in thread
From: walter harms @ 2015-11-04 12:26 UTC (permalink / raw
To: Dan Carpenter; +Cc: Zhang Rui, Eduardo Valentin, linux-pm, kernel-janitors
Am 03.11.2015 23:14, schrieb Dan Carpenter:
> This is to address a static checker warning about an underflow in
> imx_set_trip_temp(). The checker is complaining that we have a user
> supplied value for "temp" from kstrtoul() where we treat it as signed,
> we cap the upper but we accept negative values.
>
> This looks unintentional since the caller is using unsigned longs to
> represent the temperature. Let's change it to int and reject negatives
> in the caller.
>
> Also I changed it to reject negative "trip" values as well.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Someday we will use super cooled CPUs and we will need to rethink this
> code. :)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9e525c..151a630 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -664,7 +664,7 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> {
> struct thermal_zone_device *tz = to_thermal_zone(dev);
> int trip, ret;
> - unsigned long temperature;
> + int temperature;
>
> if (!tz->ops->set_trip_temp)
> return -EPERM;
> @@ -672,7 +672,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> if (!sscanf(attr->attr.name, "trip_point_%d_temp", &trip))
> return -EINVAL;
>
> - if (kstrtoul(buf, 10, &temperature))
> + if (kstrtoint(buf, 10, &temperature))
> + return -EINVAL;
> + if (trip < 0 || temperature < 0)
> return -EINVAL;
>
> ret = tz->ops->set_trip_temp(tz, trip, temperature);
IMHO the test should be near the point where the value is generated.
if (!sscanf(attr->attr.name, "trip_point_%d_temp", &trip))
return -EINVAL;
if (trip < 0)
return -EINVAL;
if (kstrtoint(buf, 10, &temperature))
return -EINVAL;
if (temperature < 0)
return -EINVAL;
That way it is easily visible under what condition -EINVAL is generated (to many).
hope that helps,
re,
wh
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] thermal: underflow in trip_point_temp_store()
2015-11-04 11:32 ` Dan Carpenter
@ 2015-11-04 16:32 ` Eduardo Valentin
-1 siblings, 0 replies; 10+ messages in thread
From: Eduardo Valentin @ 2015-11-04 16:32 UTC (permalink / raw
To: Dan Carpenter; +Cc: Zhang Rui, linux-pm, kernel-janitors
On Wed, Nov 04, 2015 at 02:32:32PM +0300, Dan Carpenter wrote:
> Oh... Hm. I actually sent a totally different patch to fix this
> warning and you just merged it. I had forgotten about that. As far as
> I am concerned this issue is solved. :) Sorry for sending duplicates.
Ok. Thanks :-). I guess this one took more attention for being touching
the thermal core. On IMX driver, it might still make sense to remove the
negative values as we are dealing with CPU temperature over there.
But it is good as it brings to the radar the point of reviewing again
the possibility of supporting negative temperatures.
BR
Eduardo
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] thermal: underflow in trip_point_temp_store()
@ 2015-11-04 16:32 ` Eduardo Valentin
0 siblings, 0 replies; 10+ messages in thread
From: Eduardo Valentin @ 2015-11-04 16:32 UTC (permalink / raw
To: Dan Carpenter; +Cc: Zhang Rui, linux-pm, kernel-janitors
On Wed, Nov 04, 2015 at 02:32:32PM +0300, Dan Carpenter wrote:
> Oh... Hm. I actually sent a totally different patch to fix this
> warning and you just merged it. I had forgotten about that. As far as
> I am concerned this issue is solved. :) Sorry for sending duplicates.
Ok. Thanks :-). I guess this one took more attention for being touching
the thermal core. On IMX driver, it might still make sense to remove the
negative values as we are dealing with CPU temperature over there.
But it is good as it brings to the radar the point of reviewing again
the possibility of supporting negative temperatures.
BR
Eduardo
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-11-04 16:32 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-03 22:14 [patch] thermal: underflow in trip_point_temp_store() Dan Carpenter
2015-11-03 22:14 ` Dan Carpenter
2015-11-04 5:57 ` Eduardo Valentin
2015-11-04 5:57 ` Eduardo Valentin
2015-11-04 11:32 ` Dan Carpenter
2015-11-04 11:32 ` Dan Carpenter
2015-11-04 16:32 ` Eduardo Valentin
2015-11-04 16:32 ` Eduardo Valentin
2015-11-04 12:26 ` walter harms
2015-11-04 12:26 ` walter harms
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.