[RFC] [PATCH]: ACPI: Rename ACPI processor device bus ID

[RFC] [PATCH]: ACPI: Rename ACPI processor device bus ID

[yakui_zhao]

On some boxes several processors use the same processor bus id.
But they are located in different scope. For example:
	\_SB.SCK0.CPU0
	\_SB.SCK1.CPU0
This follows the ACPI spec.

	As they use the same bus id, it can't be registered when
registering proc I/F. It causes that the ACPI processor driver can't
be bound with the processor device.
	
Rename the processor device bus id. And the new bus id will be 
generated as the following format:
	CPU+ CPU ID
	
For example: If the cpu ID is 5, then the bus ID will be "CPU5".
	If the CPU ID is 10, then the bus ID will be "CPUA".


Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
 drivers/acpi/processor_core.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/processor_core.c
===================================================================
--- linux-2.6.orig/drivers/acpi/processor_core.c	2009-05-21 14:44:18.000000000 +0800
+++ linux-2.6/drivers/acpi/processor_core.c	2009-05-21 15:55:13.000000000 +0800
@@ -649,7 +649,16 @@
 			return -ENODEV;
 		}
 	}
-
+	/*
+	 * On some boxes several processors use the same processor bus id.
+	 * But they are located in different scope. For example:
+	 * \_SB.SCK0.CPU0
+	 * \_SB.SCK1.CPU0
+	 * Rename the processor device bus id. And the new bus id will be
+	 * generated as the following format:
+	 * CPU+CPU ID.
+	 */
+	sprintf(acpi_device_bid(device), "CPU%X", pr->id);
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Processor [%d:%d]\n", pr->id,
 			  pr->acpi_id));
 

Re: [RFC] [PATCH]: ACPI: Rename ACPI processor device bus ID

[Henrique de Moraes Holschuh]

On Thu, 21 May 2009, yakui_zhao wrote:
> +	sprintf(acpi_device_bid(device), "CPU%X", pr->id);

Is this safe against overflows, i.e. is pr->id something *we* set?  Because
if it is in any way read from the ACPI firmware, you have to either use
snprintf, or use the format string to limit the %X to a safe lenght...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [RFC] [PATCH]: ACPI: Rename ACPI processor device bus ID

[yakui_zhao]

On Sat, 2009-05-23 at 11:18 +0800, Henrique de Moraes Holschuh wrote:
> On Thu, 21 May 2009, yakui_zhao wrote:
> > +	sprintf(acpi_device_bid(device), "CPU%X", pr->id);
> 
> Is this safe against overflows, i.e. is pr->id something *we* set?  Because
> if it is in any way read from the ACPI firmware, you have to either use
> snprintf, or use the format string to limit the %X to a safe lenght...
Thanks for pointing out this issue.
Now the array size of acpi_bus_id is 5. And when the cpu number is above
256, the overflow will happen. But it is very luck that the following
three bytes are not used by other variable because of align. And this
still can work.
Of course I already sent a patch, in which the array size is changed
from 5 to 8.

At the same time if the cpu number is less than or equal to 256, the
length of format string is safe.

thanks.

Best regards.
	Yakui 
> 

Re: [RFC] [PATCH]: ACPI: Rename ACPI processor device bus ID

[Henrique de Moraes Holschuh]

On Sat, 23 May 2009, yakui_zhao wrote:
> On Sat, 2009-05-23 at 11:18 +0800, Henrique de Moraes Holschuh wrote:
> > On Thu, 21 May 2009, yakui_zhao wrote:
> > > +	sprintf(acpi_device_bid(device), "CPU%X", pr->id);
> > 
> > Is this safe against overflows, i.e. is pr->id something *we* set?  Because
> > if it is in any way read from the ACPI firmware, you have to either use
> > snprintf, or use the format string to limit the %X to a safe lenght...
> Thanks for pointing out this issue.
> Now the array size of acpi_bus_id is 5. And when the cpu number is above
> 256, the overflow will happen. But it is very luck that the following
> three bytes are not used by other variable because of align. And this
> still can work.
> Of course I already sent a patch, in which the array size is changed
> from 5 to 8.
> 
> At the same time if the cpu number is less than or equal to 256, the
> length of format string is safe.

Yeah, but I was really asking if, even with space for 8 chars, isn't there a
risk of pr->id being, say, 0xfffffffe due to some wierdness...

If there is such a risk, you should use snprintf, or a a length limit in the
format...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [RFC] [PATCH]: ACPI: Rename ACPI processor device bus ID

[yakui_zhao]

On Sun, 2009-05-24 at 05:01 +0800, Henrique de Moraes Holschuh wrote:
> On Sat, 23 May 2009, yakui_zhao wrote:
> > On Sat, 2009-05-23 at 11:18 +0800, Henrique de Moraes Holschuh wrote:
> > > On Thu, 21 May 2009, yakui_zhao wrote:
> > > > +	sprintf(acpi_device_bid(device), "CPU%X", pr->id);
> > > 
> > > Is this safe against overflows, i.e. is pr->id something *we* set?  Because
> > > if it is in any way read from the ACPI firmware, you have to either use
> > > snprintf, or use the format string to limit the %X to a safe lenght...
> > Thanks for pointing out this issue.
> > Now the array size of acpi_bus_id is 5. And when the cpu number is above
> > 256, the overflow will happen. But it is very luck that the following
> > three bytes are not used by other variable because of align. And this
> > still can work.
> > Of course I already sent a patch, in which the array size is changed
> > from 5 to 8.
> > 
> > At the same time if the cpu number is less than or equal to 256, the
> > length of format string is safe.
> 
> Yeah, but I was really asking if, even with space for 8 chars, isn't there a
> risk of pr->id being, say, 0xfffffffe due to some wierdness...

If the pr->id is set to 0xfffffffe, it is a risk.
But in fact OS will get the correct cpu id by using the function of
get_cpu_id. The result value is -1 or other correct cpu id.

So it is unnecessary to worry that the incorrect value is assigned to
pr->id.

Of course it will be OK to use the snprintf instead of sprintf.
thanks.
> If there is such a risk, you should use snprintf, or a a length limit in the
> format...

>