Re: [PATCH ?] ACPI: pr->id is unsigned

Re: [PATCH ?] ACPI: pr->id is unsigned

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

On Mon, 15 Sep 2008 21:32:20 EDT, roel kluin said:
> since pr->id is unsigned, shouldn't something like
> the patch below be applied?

> +	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)...


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

[PATCH ?] ACPI: pr->id is unsigned

[roel kluin]

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

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index e36422a..75c0f76 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -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

Re: [PATCH ?] ACPI: pr->id is unsigned

[Thomas Renninger]

On Monday 15 September 2008 22:26:45 Valdis.Kletnieks@vt.edu wrote:
> On Mon, 15 Sep 2008 21:32:20 EDT, roel kluin said:
> > since pr->id is unsigned, shouldn't something like
> > the patch below be applied?
> >
> > +	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)...

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

Re: [PATCH ?] ACPI: pr->id is unsigned

[roel kluin]

Thomas Renninger wrote:
> On Monday 15 September 2008 22:26:45 Valdis.Kletnieks@vt.edu wrote:
>> On Mon, 15 Sep 2008 21:32:20 EDT, roel kluin said:
>>> since pr->id is unsigned, shouldn't something like
>>> the patch below be applied?
>>>
>>> +	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. :-/

>> 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]

> 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 :-)

> Thanks for finding this, do you mind to repost a new version?
> 
>     Thomas
> 
pr->id is unsigned

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index ee68ac5..9f203d3 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -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

Re: [PATCH ?] ACPI: pr->id is unsigned

[Thomas Renninger]

Thanks!

Len can you take this one, pls.

On Wednesday 17 September 2008 04:48:19 roel kluin wrote:
> Thomas Renninger wrote:
> > On Monday 15 September 2008 22:26:45 Valdis.Kletnieks@vt.edu wrote:
> >> On Mon, 15 Sep 2008 21:32:20 EDT, roel kluin said:
...
>
> pr->id is unsigned
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
Signed-off-by: Thomas Renninger <trenn@suse.de>

> ---
> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> index ee68ac5..9f203d3 100644
> --- a/drivers/acpi/processor_core.c
> +++ b/drivers/acpi/processor_core.c
> @@ -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