Skip to contentSkip to conversation

intel_menlo: max_state is unsigned, invalid test

PATCH In review roel kluin 4 participants 4 messages Oct 29, 2008 – Nov 5, 2008

Subsystems (2)
ACPI Char and misc drivers
lore
Share & tools
lore

roel kluin

author
#0 lore raw

PATCH 1/1 intel_menlo: max_state is unsigned, invalid test

max_state is unsigned, so the test is invalid.

Signed-off-by Roel Kluin <[email protected]>

---
I think max_state can only become -1, no? then probably a different patch is required.
I may not be able to respond for a few weeks.

drivers/misc/intel_menlow.c +1 -1
index e00a275..980171d 100644
@@ -121,7 +121,7 @@ static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev,
 	if (memory_get_int_max_bandwidth(cdev, &max_state))
 		return -EFAULT;
 
-	if (max_state < 0 || state > max_state)
+	if (max_state == -1 || state > max_state)
 		return -EINVAL;
 
 	arg_list.count = 1;
#1 lore raw
Quoted 14 lines · On Wed, 29 Oct 2008 17:06:47 -0400 roel kluin <[email protected]> wrote:

max_state is unsigned, so the test is invalid.

Signed-off-by Roel Kluin <[email protected]>

---
I think max_state can only become -1, no? then probably a different patch is required.
I may not be able to respond for a few weeks.

drivers/misc/intel_menlow.c +0 -0
index e00a275..980171d 100644

drivers/misc/intel_menlow.c static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev, · line 127

Quoted 8 lines
drivers/misc/intel_menlow.c +1 -1
@@ -121,7 +121,7 @@ static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev,
 	if (memory_get_int_max_bandwidth(cdev, &max_state))
 		return -EFAULT;
 
-	if (max_state < 0 || state > max_state)
+	if (max_state == -1 || state > max_state)
 		return -EINVAL;
 
 	arg_list.count = 1;

hm, maybe.

This can only happen if acpi_evaluate_integer(MEMORY_GET_BANDWIDTH) returned no-error and a bandwidth of zero (I assume).

Is this a special case which the driver really wanted to handle? If so, why is "0" the only bad value which we're checking for? Or is this all some big brainfart which should be removed?

reviewer
#2 lore raw
Quoted 28 lines · Andrew Morton
Quoted 14 lines · On Wed, 29 Oct 2008 17:06:47 -0400 roel kluin <[email protected]> wrote:

max_state is unsigned, so the test is invalid.

Signed-off-by Roel Kluin <[email protected]>

---
I think max_state can only become -1, no? then probably a different patch is required.
I may not be able to respond for a few weeks.

drivers/misc/intel_menlow.c +0 -0
index e00a275..980171d 100644

drivers/misc/intel_menlow.c static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev, · line 127

Quoted 8 lines
drivers/misc/intel_menlow.c +1 -1
@@ -121,7 +121,7 @@ static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev,
 	if (memory_get_int_max_bandwidth(cdev, &max_state))
 		return -EFAULT;
 
-	if (max_state < 0 || state > max_state)
+	if (max_state == -1 || state > max_state)
 		return -EINVAL;
 
 	arg_list.count = 1;

hm, maybe.

Quoted 6 lines

This can only happen if acpi_evaluate_integer(MEMORY_GET_BANDWIDTH) returned no-error and a bandwidth of zero (I assume).

Is this a special case which the driver really wanted to handle? If so, why is "0" the only bad value which we're checking for? Or is this all some big brainfart which should be removed?

Sujith,
Please send me a patch to intel_menlo.c that documents the legal return values from GTHS.

If 0 is illegal, that is fine, but the upstream driver doesn't check for it properl (I like Rui's 9/11 patch better than the above, so andrew, you can drop this patch in any case)

thanks,
-Len

ps. Sujith, shouldn't there be a MAINTAINERS for this driver with your name on it?

#3 lore raw
Quoted 19 lines

-----Original Message-----
From: Len Brown [mailto:[email protected]]
Sent: Tuesday, November 04, 2008 9:37 AM
To: Andrew Morton; Thomas, Sujith; Zhang, Rui
Cc: roel kluin; Brown, Len; Linux Kernel Mailing List; linux- [email protected]
Subject: Re: [PATCH] intel_menlo: max_state is unsigned, invalid test

Quoted 5 lines · Andrew Morton
Quoted 2 lines · On Wed, 29 Oct 2008 17:06:47 -0400 roel kluin <[email protected]> wrote:

max_state is unsigned, so the test is invalid.

Quoted 7 lines
Quoted 7 lines

Review

Quoted 7 lines
Signed-off-by Roel Kluin <[email protected]>

---
I think max_state can only become -1, no? then probably a different patch is required.
I may not be able to respond for a few weeks.

(quoted diff) +0 -0

b/drivers/misc/intel_menlow.c

Quoted 22 lines
Quoted 4 lines

drivers/misc/intel_menlow.c static int memory_set_cur_bandwidth(struct

Quoted 4 lines

index e00a275..980171d 100644

drivers/misc/intel_menlow.c +0 -0
@@ -121,7 +121,7 @@ static int memory_set_cur_bandwidth(struct

thermal_cooling_device *cdev,

Quoted 17 lines

drivers/misc/intel_menlow.c static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev, · line 127

Quoted 9 lines
drivers/misc/intel_menlow.c +1 -1
 	if (memory_get_int_max_bandwidth(cdev, &max_state))
 		return -EFAULT;
 
-	if (max_state < 0 || state > max_state)
+	if (max_state == -1 || state > max_state)
 		return -EINVAL;
 
 	arg_list.count = 1;

hm, maybe.

This can only happen if acpi_evaluate_integer(MEMORY_GET_BANDWIDTH) returned no-error and a bandwidth of zero (I assume).

Is this a special case which the driver really wanted to handle? If so, why is "0" the only bad value which we're checking for? Or is

this

Quoted 10 lines
Quoted 1 line

all some big brainfart which should be removed?

Sujith,
Please send me a patch to intel_menlo.c that documents the legal return values from GTHS.

If 0 is illegal, that is fine, but the upstream driver doesn't check for it properl (I like Rui's 9/11 patch better than the above, so andrew, you can drop this patch in any case)

Quoted 7 lines

thanks,
-Len

ps. Sujith, shouldn't there be a MAINTAINERS for this driver with your name on it?

Hi Len,
I am also in agreement that Rui's 9/11 patch is the solution. I'll be sending out patches you requested.
These can be applied over Rui's 9/11 patch.

Regards,
Sujith