Skip to contentSkip to conversation

ACPI: pr->id is unsigned

PATCH In review roel kluin 3 participants 5 messages Sep 15, 2008 – Sep 17, 2008 ACPI

lore
Share & tools
lore

roel kluin

author
#0 lore raw

PATCH 1/1 ACPI: pr->id is unsigned

since pr->id is unsigned, shouldn't something like the patch below be applied?

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

---

drivers/acpi/processor_core.c +1 -1
index e36422a..75c0f76 100644
@@ -667,7 +667,7 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device)
 		return 0;
 	}
 
-	BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
+	BUG_ON((pr->id >= nr_cpu_ids) || ((unsigned long)pr->id < 0));
 
 	/*
 	 * Buggy BIOS check
#1 lore raw

drivers/acpi/processor_core.c static int __cpuinit acpi_processor_start(struct acpi_device *device) · line 670

Quoted 4 lines · roel kluin

since pr->id is unsigned, shouldn't something like the patch below be applied?

drivers/acpi/processor_core.c +1 -0
+	BUG_ON((pr->id >= nr_cpu_ids) || ((unsigned long)pr->id < 0));

Under what conditions will the clause "(unsigned long)pr->id < 0)" be true, and when will it be false? What will any sane optimizing compiler do?

And *sometimes*, the *real* bug is that pr->id should be a signed quantity, not an unsigned one, and the cast is just papering over the issue.

In other words, the original line is almost certainly buggy. However, this isn't the right fix. Somebody who actually understands the code will have to decide what *should* be happening here (that's beyond my understanding of that code)...

#2 lore raw
Quoted 9 lines · On Monday 15 September 2008 22:26:45 [email protected] wrote:

drivers/acpi/processor_core.c static int __cpuinit acpi_processor_start(struct acpi_device *device) · line 670

Quoted 4 lines · roel kluin

since pr->id is unsigned, shouldn't something like the patch below be applied?

drivers/acpi/processor_core.c +1 -0
+	BUG_ON((pr->id >= nr_cpu_ids) || ((unsigned long)pr->id < 0));

Under what conditions will the clause "(unsigned long)pr->id < 0)" be true, and when will it be false? What will any sane optimizing compiler do?

Quoted 7 lines

And *sometimes*, the *real* bug is that pr->id should be a signed quantity, not an unsigned one, and the cast is just papering over the issue.

In other words, the original line is almost certainly buggy. However, this isn't the right fix. Somebody who actually understands the code will have to decide what *should* be happening here (that's beyond my understanding of that code)...

Just removing "pr->id < 0" condition should be the right fix.

For such an easy patch in the ACPI subsystem it's enough to only post to the linux-acpi mailing list, no need to bother the whole world.

Thanks for finding this, do you mind to repost a new version?

Thomas

author
#3 lore raw
Quoted 8 lines · Thomas Renninger
Quoted 7 lines · On Monday 15 September 2008 22:26:45 [email protected] wrote:

Review

Quoted 4 lines · roel kluin

since pr->id is unsigned, shouldn't something like the patch below be applied?

(quoted diff) +1 -0
+	BUG_ON((pr->id >= nr_cpu_ids) || ((unsigned long)pr->id < 0));

Under what conditions will the clause "(unsigned long)pr->id < 0)" be true, and when will it be false? What will any sane optimizing compiler do?

ah, yes, remove it, I wasn't thinking. :-/

Quoted 7 lines
Quoted 7 lines

And *sometimes*, the *real* bug is that pr->id should be a signed quantity, not an unsigned one, and the cast is just papering over the issue.

In other words, the original line is almost certainly buggy. However, this isn't the right fix. Somebody who actually understands the code will have to decide what *should* be happening here (that's beyond my understanding of that code)...

Thanks for correcting me, I should have verified that.

A prior mail, http://lkml.org/lkml/2008/9/9/136
brought me on a wrong line of thought:

[Quote (with ret as an unsigned long)]
if ((unsigned long)ret >= -ERESTART_RESTARTBLOCK)

This and similar ones are not wrong. The -constant is converted to unsigned (which is a well-defined operation) after which an >=u (greater-or-equal unsigned) is performed.
[End Quote]

Quoted 4 lines

Just removing "pr->id < 0" condition should be the right fix.

For such an easy patch in the ACPI subsystem it's enough to only post to the linux-acpi mailing list, no need to bother the whole world.

Ok, removed the rest of the world :-)

Quoted 4 lines

Thanks for finding this, do you mind to repost a new version?

Thomas

pr->id is unsigned

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

---

drivers/acpi/processor_core.c +1 -1
index ee68ac5..9f203d3 100644
@@ -667,7 +667,7 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device)
 		return 0;
 	}
 
-	BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
+	BUG_ON(pr->id >= nr_cpu_ids);
 
 	/*
 	 * Buggy BIOS check
#4 lore raw

Thanks!

Len can you take this one, pls.

Quoted 3 lines · On Wednesday 17 September 2008 04:48:19 roel kluin wrote:
Quoted 2 lines · Thomas Renninger
Quoted 1 line · On Monday 15 September 2008 22:26:45 [email protected] wrote:

On Mon, 15 Sep 2008 21:32:20 EDT, roel kluin said:

...

Quoted 4 lines

pr->id is unsigned

Signed-off-by Roel Kluin <[email protected]>
Signed-off-by Thomas Renninger <[email protected]>
Quoted 14 lines

---

drivers/acpi/processor_core.c +1 -1
index ee68ac5..9f203d3 100644
@@ -667,7 +667,7 @@ static int __cpuinit acpi_processor_start(struct
 acpi_device *device) return 0;
 	}
 
-	BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
+	BUG_ON(pr->id >= nr_cpu_ids);
 
 	/*
 	 * Buggy BIOS check