[PATCH v2 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[PATCH v2 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[Myron Stowe]

The following three item patch series fixes an issue with the introduction
of > 256 processor declaration support: "Allow processor to be declared with
the Device() instead of Processor()" (git SHA 11bf04c4).

This version, v2, has no functional changes from the original
(gmane.linux.acpi.devel/34815).  The changes, base on reviewer comments, are:
  . a change to the printk verbage,
  . some additional comments.

Please queue as .29 material.

Thanks,
Myron

---

Myron Stowe (3):
      ACPI: 80 column adherence and spelling fix (no functional change)
      ACPI: Behave uniquely based on processor declaration definition type
      ACPI: Disambiguate processor declaration type


 drivers/acpi/processor_core.c |   83 +++++++++++++++++++++++------------------
 drivers/acpi/scan.c           |    2 -
 include/acpi/acpi_drivers.h   |    1 
 3 files changed, 49 insertions(+), 37 deletions(-)

[PATCH v2 1/3] ACPI: Disambiguate processor declaration type

[Myron Stowe]

Declaring processors in ACPI namespace can be done using either a
"Processor" definition or a "Device" definition (see section 8.4 -
Declaring Processors; "Advanced Configuration and Power Interface
Specification", Revision 3.0b).  Currently the two processor
declaration types are conflated.

This patch disambiguates the processor declaration's definition type
enabling subsequent code to behave uniquely based explicitly on the
declaration's type.

Signed-off-by: Myron Stowe <myron.stowe@hp.com>
Cc: Alexey Starikovskiy <aystarik@gmail.com>
Cc: Zhao Yakui <yakui.zhao@intel.com>
---

 drivers/acpi/processor_core.c |    1 +
 drivers/acpi/scan.c           |    2 +-
 include/acpi/acpi_drivers.h   |    1 +
 3 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 24a362f..0c670dd 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -89,6 +89,7 @@ static int acpi_processor_handle_eject(struct acpi_processor *pr);
 
 
 static const struct acpi_device_id processor_device_ids[] = {
+	{ACPI_PROCESSOR_OBJECT_HID, 0},
 	{ACPI_PROCESSOR_HID, 0},
 	{"", 0},
 };
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index a9dda8e..3fb6e2d 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1043,7 +1043,7 @@ static void acpi_device_set_id(struct acpi_device *device,
 		hid = ACPI_POWER_HID;
 		break;
 	case ACPI_BUS_TYPE_PROCESSOR:
-		hid = ACPI_PROCESSOR_HID;
+		hid = ACPI_PROCESSOR_OBJECT_HID;
 		break;
 	case ACPI_BUS_TYPE_SYSTEM:
 		hid = ACPI_SYSTEM_HID;
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index cf04c60..7469ff3 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -41,6 +41,7 @@
  */
 
 #define ACPI_POWER_HID			"LNXPOWER"
+#define ACPI_PROCESSOR_OBJECT_HID	"ACPI_CPU"
 #define ACPI_PROCESSOR_HID		"ACPI0007"
 #define ACPI_SYSTEM_HID			"LNXSYSTM"
 #define ACPI_THERMAL_HID		"LNXTHERM"

[PATCH v2 2/3] ACPI: Behave uniquely based on processor declaration definition type

[Myron Stowe]

Associating a Local SAPIC with a processor object is dependent upon the
processor object's definition type.  CPUs declared as "Processor" should
use the Local SAPIC's 'processor_id', and CPUs declared as "Device"
should use the 'uid'.  Note that for "Processor" declarations, even if a
'_UID' child object exists, it has no bearing with respect to mapping
Local SAPICs (see section 5.2.11.13 - Local SAPIC Structure; "Advanced
Configuration and Power Interface Specification", Revision 3.0b).

This patch changes the lsapic mapping logic to rely on the distinction of
how the processor object was declared - the mapping can't just try both
types of matches irregardless of declaration type and rely on one failing
as is currently being done.

Signed-off-by: Myron Stowe <myron.stowe@hp.com>
Cc: Alexey Starikovskiy <aystarik@gmail.com>
Cc: Zhao Yakui <yakui.zhao@intel.com>
---

 drivers/acpi/processor_core.c |   75 +++++++++++++++++++++++------------------
 1 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 0c670dd..35d33e8 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(struct acpi_device *device)
 /* Use the acpiid in MADT to map cpus in case of SMP */
 
 #ifndef CONFIG_SMP
-static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
+static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
 #else
 
 static struct acpi_table_madt *madt;
@@ -429,27 +429,35 @@ static int map_lapic_id(struct acpi_subtable_header *entry,
 }
 
 static int map_lsapic_id(struct acpi_subtable_header *entry,
-		  u32 acpi_id, int *apic_id)
+		int device_declaration, u32 acpi_id, int *apic_id)
 {
 	struct acpi_madt_local_sapic *lsapic =
 		(struct acpi_madt_local_sapic *)entry;
+	u32 tmp = (lsapic->id << 8) | lsapic->eid;
+
 	/* Only check enabled APICs*/
-	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
-		/* First check against id */
-		if (lsapic->processor_id == acpi_id) {
-			*apic_id = (lsapic->id << 8) | lsapic->eid;
-			return 1;
-		/* Check against optional uid */
-		} else if (entry->length >= 16 &&
-			lsapic->uid == acpi_id) {
-			*apic_id = lsapic->uid;
-			return 1;
-		}
-	}
+	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
+		return 0;
+
+	/* Device statement declaration type */
+	if (device_declaration) {
+		if (entry->length < 16)
+			printk(KERN_ERR PREFIX
+			    "Invalid LSAPIC with Device type processor (SAPIC ID %#x)\n",
+			    tmp);
+		else if (lsapic->uid == acpi_id)
+			goto found;
+	/* Processor statement declaration type */
+	} else if (lsapic->processor_id == acpi_id)
+		goto found;
+
 	return 0;
+found:
+	*apic_id = tmp;
+	return 1;
 }
 
-static int map_madt_entry(u32 acpi_id)
+static int map_madt_entry(int type, u32 acpi_id)
 {
 	unsigned long madt_end, entry;
 	int apic_id = -1;
@@ -470,7 +478,7 @@ static int map_madt_entry(u32 acpi_id)
 			if (map_lapic_id(header, acpi_id, &apic_id))
 				break;
 		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
-			if (map_lsapic_id(header, acpi_id, &apic_id))
+			if (map_lsapic_id(header, type, acpi_id, &apic_id))
 				break;
 		}
 		entry += header->length;
@@ -478,7 +486,7 @@ static int map_madt_entry(u32 acpi_id)
 	return apic_id;
 }
 
-static int map_mat_entry(acpi_handle handle, u32 acpi_id)
+static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
 {
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *obj;
@@ -501,7 +509,7 @@ static int map_mat_entry(acpi_handle handle, u32 acpi_id)
 	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
 		map_lapic_id(header, acpi_id, &apic_id);
 	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
-		map_lsapic_id(header, acpi_id, &apic_id);
+		map_lsapic_id(header, type, acpi_id, &apic_id);
 	}
 
 exit:
@@ -510,14 +518,14 @@ exit:
 	return apic_id;
 }
 
-static int get_cpu_id(acpi_handle handle, u32 acpi_id)
+static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
 {
 	int i;
 	int apic_id = -1;
 
-	apic_id = map_mat_entry(handle, acpi_id);
+	apic_id = map_mat_entry(handle, type, acpi_id);
 	if (apic_id == -1)
-		apic_id = map_madt_entry(acpi_id);
+		apic_id = map_madt_entry(type, acpi_id);
 	if (apic_id == -1)
 		return apic_id;
 
@@ -533,15 +541,16 @@ static int get_cpu_id(acpi_handle handle, u32 acpi_id)
                                  Driver Interface
    -------------------------------------------------------------------------- */
 
-static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
+static int acpi_processor_get_info(struct acpi_device *device)
 {
 	acpi_status status = 0;
 	union acpi_object object = { 0 };
 	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
-	int cpu_index;
+	struct acpi_processor *pr;
+	int cpu_index, device_declaration = 0;
 	static int cpu0_initialized;
 
-
+	pr = acpi_driver_data(device);
 	if (!pr)
 		return -EINVAL;
 
@@ -562,8 +571,11 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "No bus mastering arbitration control\n"));
 
-	/* Check if it is a Device with HID and UID */
-	if (has_uid) {
+	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
+		/*
+		 * Declared with "Device" statement; match _UID.
+		 * Note that we don't handle string _UIDs yet.
+		 */
 		unsigned long long value;
 		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
 						NULL, &value);
@@ -571,13 +583,10 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
 			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
 			return -ENODEV;
 		}
+		device_declaration = 1;
 		pr->acpi_id = value;
 	} else {
-		/*
-		* Evalute the processor object.  Note that it is common on SMP to
-		* have the first (boot) processor with a valid PBLK address while
-		* all others have a NULL address.
-		*/
+		/* Declared with "Processor" statement; match ProcessorID */
 		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
 		if (ACPI_FAILURE(status)) {
 			printk(KERN_ERR PREFIX "Evaluating processor object\n");
@@ -590,7 +599,7 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
 		*/
 		pr->acpi_id = object.processor.proc_id;
 	}
-	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
+	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
 
 	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
 	if (!cpu0_initialized && (cpu_index == -1) &&
@@ -662,7 +671,7 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device)
 
 	pr = acpi_driver_data(device);
 
-	result = acpi_processor_get_info(pr, device->flags.unique_id);
+	result = acpi_processor_get_info(device);
 	if (result) {
 		/* Processor is physically not present */
 		return 0;

Re: [PATCH v2 2/3] ACPI: Behave uniquely based on processor declaration definition type

[Zhao Yakui]

On Fri, 2008-10-31 at 06:13 +0800, Myron Stowe wrote:
> Associating a Local SAPIC with a processor object is dependent upon the
> processor object's definition type.  CPUs declared as "Processor" should
> use the Local SAPIC's 'processor_id', and CPUs declared as "Device"
> should use the 'uid'.  Note that for "Processor" declarations, even if a
> '_UID' child object exists, it has no bearing with respect to mapping
> Local SAPICs (see section 5.2.11.13 - Local SAPIC Structure; "Advanced
> Configuration and Power Interface Specification", Revision 3.0b).
> 
> This patch changes the lsapic mapping logic to rely on the distinction of
> how the processor object was declared - the mapping can't just try both
> types of matches irregardless of declaration type and rely on one failing
> as is currently being done.
> 
> Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> Cc: Alexey Starikovskiy <aystarik@gmail.com>
> Cc: Zhao Yakui <yakui.zhao@intel.com>
> ---
> 
>  drivers/acpi/processor_core.c |   75 +++++++++++++++++++++++------------------
>  1 files changed, 42 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> index 0c670dd..35d33e8 100644
> --- a/drivers/acpi/processor_core.c
> +++ b/drivers/acpi/processor_core.c
> @@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(struct acpi_device *device)
>  /* Use the acpiid in MADT to map cpus in case of SMP */
>  
>  #ifndef CONFIG_SMP
> -static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
> +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
>  #else
>  
>  static struct acpi_table_madt *madt;
> @@ -429,27 +429,35 @@ static int map_lapic_id(struct acpi_subtable_header *entry,
>  }
>  
>  static int map_lsapic_id(struct acpi_subtable_header *entry,
> -		  u32 acpi_id, int *apic_id)
> +		int device_declaration, u32 acpi_id, int *apic_id)
>  {
>  	struct acpi_madt_local_sapic *lsapic =
>  		(struct acpi_madt_local_sapic *)entry;
> +	u32 tmp = (lsapic->id << 8) | lsapic->eid;
> +
>  	/* Only check enabled APICs*/
> -	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
> -		/* First check against id */
> -		if (lsapic->processor_id == acpi_id) {
> -			*apic_id = (lsapic->id << 8) | lsapic->eid;
> -			return 1;
> -		/* Check against optional uid */
> -		} else if (entry->length >= 16 &&
> -			lsapic->uid == acpi_id) {
> -			*apic_id = lsapic->uid;
> -			return 1;
> -		}
> -	}
> +	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
> +		return 0;
> +
> +	/* Device statement declaration type */
> +	if (device_declaration) {
> +		if (entry->length < 16)
> +			printk(KERN_ERR PREFIX
> +			    "Invalid LSAPIC with Device type processor (SAPIC ID %#x)\n",
> +			    tmp);
> +		else if (lsapic->uid == acpi_id)
> +			goto found;
> +	/* Processor statement declaration type */
> +	} else if (lsapic->processor_id == acpi_id)
> +		goto found;
> +
>  	return 0;
> +found:
> +	*apic_id = tmp;
> +	return 1;
>  }
>  
> -static int map_madt_entry(u32 acpi_id)
> +static int map_madt_entry(int type, u32 acpi_id)
>  {
>  	unsigned long madt_end, entry;
>  	int apic_id = -1;
> @@ -470,7 +478,7 @@ static int map_madt_entry(u32 acpi_id)
>  			if (map_lapic_id(header, acpi_id, &apic_id))
>  				break;
>  		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> -			if (map_lsapic_id(header, acpi_id, &apic_id))
> +			if (map_lsapic_id(header, type, acpi_id, &apic_id))
>  				break;
>  		}
>  		entry += header->length;
> @@ -478,7 +486,7 @@ static int map_madt_entry(u32 acpi_id)
>  	return apic_id;
>  }
>  
> -static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> +static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
>  {
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>  	union acpi_object *obj;
> @@ -501,7 +509,7 @@ static int map_mat_entry(acpi_handle handle, u32 acpi_id)
>  	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
>  		map_lapic_id(header, acpi_id, &apic_id);
>  	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> -		map_lsapic_id(header, acpi_id, &apic_id);
> +		map_lsapic_id(header, type, acpi_id, &apic_id);
>  	}
>  
>  exit:
> @@ -510,14 +518,14 @@ exit:
>  	return apic_id;
>  }
>  
> -static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
>  {
>  	int i;
>  	int apic_id = -1;
>  
> -	apic_id = map_mat_entry(handle, acpi_id);
> +	apic_id = map_mat_entry(handle, type, acpi_id);
>  	if (apic_id == -1)
> -		apic_id = map_madt_entry(acpi_id);
> +		apic_id = map_madt_entry(type, acpi_id);
>  	if (apic_id == -1)
>  		return apic_id;
>  
> @@ -533,15 +541,16 @@ static int get_cpu_id(acpi_handle handle, u32 acpi_id)
>                                   Driver Interface
>     -------------------------------------------------------------------------- */
>  
> -static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> +static int acpi_processor_get_info(struct acpi_device *device)
>  {
>  	acpi_status status = 0;
>  	union acpi_object object = { 0 };
>  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> -	int cpu_index;
> +	struct acpi_processor *pr;
> +	int cpu_index, device_declaration = 0;
>  	static int cpu0_initialized;
>  
> -
> +	pr = acpi_driver_data(device);
>  	if (!pr)
>  		return -EINVAL;
>  
> @@ -562,8 +571,11 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  				  "No bus mastering arbitration control\n"));
>  
> -	/* Check if it is a Device with HID and UID */
> -	if (has_uid) {
> +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> +		/*
> +		 * Declared with "Device" statement; match _UID.
> +		 * Note that we don't handle string _UIDs yet.
Looks very good. 
Can you add the check whether the device.flags.unique_id exists before
evaluating the _UID object? 
If not exist, it indicates that the processor definition is incorrect.

Thanks.
> +		 */
>  		unsigned long long value;
>  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
>  						NULL, &value);
> @@ -571,13 +583,10 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
>  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
>  			return -ENODEV;
>  		}
> +		device_declaration = 1;
>  		pr->acpi_id = value;
>  	} else {
> -		/*
> -		* Evalute the processor object.  Note that it is common on SMP to
> -		* have the first (boot) processor with a valid PBLK address while
> -		* all others have a NULL address.
> -		*/
> +		/* Declared with "Processor" statement; match ProcessorID */
>  		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
>  		if (ACPI_FAILURE(status)) {
>  			printk(KERN_ERR PREFIX "Evaluating processor object\n");
> @@ -590,7 +599,7 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
>  		*/
>  		pr->acpi_id = object.processor.proc_id;
>  	}
> -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
>  
>  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
>  	if (!cpu0_initialized && (cpu_index == -1) &&
> @@ -662,7 +671,7 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device)
>  
>  	pr = acpi_driver_data(device);
>  
> -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> +	result = acpi_processor_get_info(device);
>  	if (result) {
>  		/* Processor is physically not present */
>  		return 0;
> 

Re: [PATCH v2 2/3] ACPI: Behave uniquely based on processor declaration definition type

[Myron Stowe]

On Fri, 2008-10-31 at 09:19 +0800, Zhao Yakui wrote:
> On Fri, 2008-10-31 at 06:13 +0800, Myron Stowe wrote:
> > Associating a Local SAPIC with a processor object is dependent upon the
> > processor object's definition type.  CPUs declared as "Processor" should
> > use the Local SAPIC's 'processor_id', and CPUs declared as "Device"
> > should use the 'uid'.  Note that for "Processor" declarations, even if a
> > '_UID' child object exists, it has no bearing with respect to mapping
> > Local SAPICs (see section 5.2.11.13 - Local SAPIC Structure; "Advanced
> > Configuration and Power Interface Specification", Revision 3.0b).
> > 
> > This patch changes the lsapic mapping logic to rely on the distinction of
> > how the processor object was declared - the mapping can't just try both
> > types of matches irregardless of declaration type and rely on one failing
> > as is currently being done.
> > 
> > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > Cc: Alexey Starikovskiy <aystarik@gmail.com>
> > Cc: Zhao Yakui <yakui.zhao@intel.com>
> > ---
> > 
> >  drivers/acpi/processor_core.c |   75 +++++++++++++++++++++++------------------
> >  1 files changed, 42 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> > index 0c670dd..35d33e8 100644
> > --- a/drivers/acpi/processor_core.c
> > +++ b/drivers/acpi/processor_core.c
> > @@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(struct acpi_device *device)
> >  /* Use the acpiid in MADT to map cpus in case of SMP */
> >  
> >  #ifndef CONFIG_SMP
> > -static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
> > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
> >  #else
> >  
> >  static struct acpi_table_madt *madt;
> > @@ -429,27 +429,35 @@ static int map_lapic_id(struct acpi_subtable_header *entry,
> >  }
> >  
> >  static int map_lsapic_id(struct acpi_subtable_header *entry,
> > -		  u32 acpi_id, int *apic_id)
> > +		int device_declaration, u32 acpi_id, int *apic_id)
> >  {
> >  	struct acpi_madt_local_sapic *lsapic =
> >  		(struct acpi_madt_local_sapic *)entry;
> > +	u32 tmp = (lsapic->id << 8) | lsapic->eid;
> > +
> >  	/* Only check enabled APICs*/
> > -	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
> > -		/* First check against id */
> > -		if (lsapic->processor_id == acpi_id) {
> > -			*apic_id = (lsapic->id << 8) | lsapic->eid;
> > -			return 1;
> > -		/* Check against optional uid */
> > -		} else if (entry->length >= 16 &&
> > -			lsapic->uid == acpi_id) {
> > -			*apic_id = lsapic->uid;
> > -			return 1;
> > -		}
> > -	}
> > +	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
> > +		return 0;
> > +
> > +	/* Device statement declaration type */
> > +	if (device_declaration) {
> > +		if (entry->length < 16)
> > +			printk(KERN_ERR PREFIX
> > +			    "Invalid LSAPIC with Device type processor (SAPIC ID %#x)\n",
> > +			    tmp);
> > +		else if (lsapic->uid == acpi_id)
> > +			goto found;
> > +	/* Processor statement declaration type */
> > +	} else if (lsapic->processor_id == acpi_id)
> > +		goto found;
> > +
> >  	return 0;
> > +found:
> > +	*apic_id = tmp;
> > +	return 1;
> >  }
> >  
> > -static int map_madt_entry(u32 acpi_id)
> > +static int map_madt_entry(int type, u32 acpi_id)
> >  {
> >  	unsigned long madt_end, entry;
> >  	int apic_id = -1;
> > @@ -470,7 +478,7 @@ static int map_madt_entry(u32 acpi_id)
> >  			if (map_lapic_id(header, acpi_id, &apic_id))
> >  				break;
> >  		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > -			if (map_lsapic_id(header, acpi_id, &apic_id))
> > +			if (map_lsapic_id(header, type, acpi_id, &apic_id))
> >  				break;
> >  		}
> >  		entry += header->length;
> > @@ -478,7 +486,7 @@ static int map_madt_entry(u32 acpi_id)
> >  	return apic_id;
> >  }
> >  
> > -static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> > +static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
> >  {
> >  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> >  	union acpi_object *obj;
> > @@ -501,7 +509,7 @@ static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> >  	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
> >  		map_lapic_id(header, acpi_id, &apic_id);
> >  	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > -		map_lsapic_id(header, acpi_id, &apic_id);
> > +		map_lsapic_id(header, type, acpi_id, &apic_id);
> >  	}
> >  
> >  exit:
> > @@ -510,14 +518,14 @@ exit:
> >  	return apic_id;
> >  }
> >  
> > -static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
> >  {
> >  	int i;
> >  	int apic_id = -1;
> >  
> > -	apic_id = map_mat_entry(handle, acpi_id);
> > +	apic_id = map_mat_entry(handle, type, acpi_id);
> >  	if (apic_id == -1)
> > -		apic_id = map_madt_entry(acpi_id);
> > +		apic_id = map_madt_entry(type, acpi_id);
> >  	if (apic_id == -1)
> >  		return apic_id;
> >  
> > @@ -533,15 +541,16 @@ static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> >                                   Driver Interface
> >     -------------------------------------------------------------------------- */
> >  
> > -static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > +static int acpi_processor_get_info(struct acpi_device *device)
> >  {
> >  	acpi_status status = 0;
> >  	union acpi_object object = { 0 };
> >  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> > -	int cpu_index;
> > +	struct acpi_processor *pr;
> > +	int cpu_index, device_declaration = 0;
> >  	static int cpu0_initialized;
> >  
> > -
> > +	pr = acpi_driver_data(device);
> >  	if (!pr)
> >  		return -EINVAL;
> >  
> > @@ -562,8 +571,11 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >  				  "No bus mastering arbitration control\n"));
> >  
> > -	/* Check if it is a Device with HID and UID */
> > -	if (has_uid) {
> > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> > +		/*
> > +		 * Declared with "Device" statement; match _UID.
> > +		 * Note that we don't handle string _UIDs yet.
> Looks very good. 
> Can you add the check whether the device.flags.unique_id exists before
> evaluating the _UID object? 
> If not exist, it indicates that the processor definition is incorrect.

The additional check would create a relationship with
'device.flags.unique_id' which seems redundant and would introduce
unnecessary complexity going forward.  While such an additional check
would possibly short circuit the call to 'acpi_evaluate_integer()' -
when FW is in error and a _UID child object does not exist; a case that
is already caught - this code is not in a performance path and thus
seems to yield no benefit.

Was there some other aspect that you were thinking of?

Myron

> Thanks.
> > +		 */
> >  		unsigned long long value;
> >  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> >  						NULL, &value);
> > @@ -571,13 +583,10 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> >  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
> >  			return -ENODEV;
> >  		}
> > +		device_declaration = 1;
> >  		pr->acpi_id = value;
> >  	} else {
> > -		/*
> > -		* Evalute the processor object.  Note that it is common on SMP to
> > -		* have the first (boot) processor with a valid PBLK address while
> > -		* all others have a NULL address.
> > -		*/
> > +		/* Declared with "Processor" statement; match ProcessorID */
> >  		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
> >  		if (ACPI_FAILURE(status)) {
> >  			printk(KERN_ERR PREFIX "Evaluating processor object\n");
> > @@ -590,7 +599,7 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> >  		*/
> >  		pr->acpi_id = object.processor.proc_id;
> >  	}
> > -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> > +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
> >  
> >  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
> >  	if (!cpu0_initialized && (cpu_index == -1) &&
> > @@ -662,7 +671,7 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device)
> >  
> >  	pr = acpi_driver_data(device);
> >  
> > -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> > +	result = acpi_processor_get_info(device);
> >  	if (result) {
> >  		/* Processor is physically not present */
> >  		return 0;
> > 
> 
-- 
Myron Stowe                             HP Open Source & Linux Org

Re: [PATCH v2 2/3] ACPI: Behave uniquely based on processor declaration definition type

[Zhao Yakui]

On Mon, 2008-11-03 at 08:10 +0800, Myron Stowe wrote:
> On Fri, 2008-10-31 at 09:19 +0800, Zhao Yakui wrote:
> > On Fri, 2008-10-31 at 06:13 +0800, Myron Stowe wrote:
> > > Associating a Local SAPIC with a processor object is dependent upon the
> > > processor object's definition type.  CPUs declared as "Processor" should
> > > use the Local SAPIC's 'processor_id', and CPUs declared as "Device"
> > > should use the 'uid'.  Note that for "Processor" declarations, even if a
> > > '_UID' child object exists, it has no bearing with respect to mapping
> > > Local SAPICs (see section 5.2.11.13 - Local SAPIC Structure; "Advanced
> > > Configuration and Power Interface Specification", Revision 3.0b).
> > > 
> > > This patch changes the lsapic mapping logic to rely on the distinction of
> > > how the processor object was declared - the mapping can't just try both
> > > types of matches irregardless of declaration type and rely on one failing
> > > as is currently being done.
> > > 
> > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > Cc: Alexey Starikovskiy <aystarik@gmail.com>
> > > Cc: Zhao Yakui <yakui.zhao@intel.com>
> > > ---
> > > 
> > >  drivers/acpi/processor_core.c |   75 +++++++++++++++++++++++------------------
> > >  1 files changed, 42 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> > > index 0c670dd..35d33e8 100644
> > > --- a/drivers/acpi/processor_core.c
> > > +++ b/drivers/acpi/processor_core.c
> > > @@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(struct acpi_device *device)
> > >  /* Use the acpiid in MADT to map cpus in case of SMP */
> > >  
> > >  #ifndef CONFIG_SMP
> > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
> > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
> > >  #else
> > >  
> > >  static struct acpi_table_madt *madt;
> > > @@ -429,27 +429,35 @@ static int map_lapic_id(struct acpi_subtable_header *entry,
> > >  }
> > >  
> > >  static int map_lsapic_id(struct acpi_subtable_header *entry,
> > > -		  u32 acpi_id, int *apic_id)
> > > +		int device_declaration, u32 acpi_id, int *apic_id)
> > >  {
> > >  	struct acpi_madt_local_sapic *lsapic =
> > >  		(struct acpi_madt_local_sapic *)entry;
> > > +	u32 tmp = (lsapic->id << 8) | lsapic->eid;
> > > +
> > >  	/* Only check enabled APICs*/
> > > -	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
> > > -		/* First check against id */
> > > -		if (lsapic->processor_id == acpi_id) {
> > > -			*apic_id = (lsapic->id << 8) | lsapic->eid;
> > > -			return 1;
> > > -		/* Check against optional uid */
> > > -		} else if (entry->length >= 16 &&
> > > -			lsapic->uid == acpi_id) {
> > > -			*apic_id = lsapic->uid;
> > > -			return 1;
> > > -		}
> > > -	}
> > > +	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
> > > +		return 0;
> > > +
> > > +	/* Device statement declaration type */
> > > +	if (device_declaration) {
> > > +		if (entry->length < 16)
> > > +			printk(KERN_ERR PREFIX
> > > +			    "Invalid LSAPIC with Device type processor (SAPIC ID %#x)\n",
> > > +			    tmp);
> > > +		else if (lsapic->uid == acpi_id)
> > > +			goto found;
> > > +	/* Processor statement declaration type */
> > > +	} else if (lsapic->processor_id == acpi_id)
> > > +		goto found;
> > > +
> > >  	return 0;
> > > +found:
> > > +	*apic_id = tmp;
> > > +	return 1;
> > >  }
> > >  
> > > -static int map_madt_entry(u32 acpi_id)
> > > +static int map_madt_entry(int type, u32 acpi_id)
> > >  {
> > >  	unsigned long madt_end, entry;
> > >  	int apic_id = -1;
> > > @@ -470,7 +478,7 @@ static int map_madt_entry(u32 acpi_id)
> > >  			if (map_lapic_id(header, acpi_id, &apic_id))
> > >  				break;
> > >  		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > -			if (map_lsapic_id(header, acpi_id, &apic_id))
> > > +			if (map_lsapic_id(header, type, acpi_id, &apic_id))
> > >  				break;
> > >  		}
> > >  		entry += header->length;
> > > @@ -478,7 +486,7 @@ static int map_madt_entry(u32 acpi_id)
> > >  	return apic_id;
> > >  }
> > >  
> > > -static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> > > +static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
> > >  {
> > >  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > >  	union acpi_object *obj;
> > > @@ -501,7 +509,7 @@ static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> > >  	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
> > >  		map_lapic_id(header, acpi_id, &apic_id);
> > >  	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > -		map_lsapic_id(header, acpi_id, &apic_id);
> > > +		map_lsapic_id(header, type, acpi_id, &apic_id);
> > >  	}
> > >  
> > >  exit:
> > > @@ -510,14 +518,14 @@ exit:
> > >  	return apic_id;
> > >  }
> > >  
> > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
> > >  {
> > >  	int i;
> > >  	int apic_id = -1;
> > >  
> > > -	apic_id = map_mat_entry(handle, acpi_id);
> > > +	apic_id = map_mat_entry(handle, type, acpi_id);
> > >  	if (apic_id == -1)
> > > -		apic_id = map_madt_entry(acpi_id);
> > > +		apic_id = map_madt_entry(type, acpi_id);
> > >  	if (apic_id == -1)
> > >  		return apic_id;
> > >  
> > > @@ -533,15 +541,16 @@ static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> > >                                   Driver Interface
> > >     -------------------------------------------------------------------------- */
> > >  
> > > -static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > +static int acpi_processor_get_info(struct acpi_device *device)
> > >  {
> > >  	acpi_status status = 0;
> > >  	union acpi_object object = { 0 };
> > >  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> > > -	int cpu_index;
> > > +	struct acpi_processor *pr;
> > > +	int cpu_index, device_declaration = 0;
> > >  	static int cpu0_initialized;
> > >  
> > > -
> > > +	pr = acpi_driver_data(device);
> > >  	if (!pr)
> > >  		return -EINVAL;
> > >  
> > > @@ -562,8 +571,11 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > >  				  "No bus mastering arbitration control\n"));
> > >  
> > > -	/* Check if it is a Device with HID and UID */
> > > -	if (has_uid) {
> > > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> > > +		/*
> > > +		 * Declared with "Device" statement; match _UID.
> > > +		 * Note that we don't handle string _UIDs yet.
> > Looks very good. 
> > Can you add the check whether the device.flags.unique_id exists before
> > evaluating the _UID object? 
> > If not exist, it indicates that the processor definition is incorrect.
> 
> The additional check would create a relationship with
> 'device.flags.unique_id' which seems redundant and would introduce
> unnecessary complexity going forward.  While such an additional check
> would possibly short circuit the call to 'acpi_evaluate_integer()' -
> when FW is in error and a _UID child object does not exist; a case that
> is already caught - this code is not in a performance path and thus
> seems to yield no benefit.
In your patch the device.flags.unique_id is not used. Maybe on some
systems the processor is defined by Device. But there is no _UID
object.This is incorrect. 
   IMO in such case we should catch such error.
  
Best regards.
   Yakui
> Was there some other aspect that you were thinking of?
> 
> Myron
> 
> > Thanks.
> > > +		 */
> > >  		unsigned long long value;
> > >  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> > >  						NULL, &value);
> > > @@ -571,13 +583,10 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > >  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
> > >  			return -ENODEV;
> > >  		}
> > > +		device_declaration = 1;
> > >  		pr->acpi_id = value;
> > >  	} else {
> > > -		/*
> > > -		* Evalute the processor object.  Note that it is common on SMP to
> > > -		* have the first (boot) processor with a valid PBLK address while
> > > -		* all others have a NULL address.
> > > -		*/
> > > +		/* Declared with "Processor" statement; match ProcessorID */
> > >  		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
> > >  		if (ACPI_FAILURE(status)) {
> > >  			printk(KERN_ERR PREFIX "Evaluating processor object\n");
> > > @@ -590,7 +599,7 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > >  		*/
> > >  		pr->acpi_id = object.processor.proc_id;
> > >  	}
> > > -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> > > +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
> > >  
> > >  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
> > >  	if (!cpu0_initialized && (cpu_index == -1) &&
> > > @@ -662,7 +671,7 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device)
> > >  
> > >  	pr = acpi_driver_data(device);
> > >  
> > > -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> > > +	result = acpi_processor_get_info(device);
> > >  	if (result) {
> > >  		/* Processor is physically not present */
> > >  		return 0;
> > > 
> > 

Re: [PATCH v2 2/3] ACPI: Behave uniquely based on processor declaration definition type

[Bjorn Helgaas]

On Sunday 02 November 2008 6:15:17 pm Zhao Yakui wrote:
> On Mon, 2008-11-03 at 08:10 +0800, Myron Stowe wrote:
> > On Fri, 2008-10-31 at 09:19 +0800, Zhao Yakui wrote:
> > > On Fri, 2008-10-31 at 06:13 +0800, Myron Stowe wrote:
> > > > Associating a Local SAPIC with a processor object is dependent upon
> > > > the processor object's definition type.  CPUs declared as "Processor"
> > > > should use the Local SAPIC's 'processor_id', and CPUs declared as
> > > > "Device" should use the 'uid'.  Note that for "Processor"
> > > > declarations, even if a '_UID' child object exists, it has no bearing
> > > > with respect to mapping Local SAPICs (see section 5.2.11.13 - Local
> > > > SAPIC Structure; "Advanced Configuration and Power Interface
> > > > Specification", Revision 3.0b).
> > > >
> > > > This patch changes the lsapic mapping logic to rely on the
> > > > distinction of how the processor object was declared - the mapping
> > > > can't just try both types of matches irregardless of declaration type
> > > > and rely on one failing as is currently being done.
> > > >
> > > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > > Cc: Alexey Starikovskiy <aystarik@gmail.com>
> > > > Cc: Zhao Yakui <yakui.zhao@intel.com>
> > > > ---
> > > >
> > > >  drivers/acpi/processor_core.c |   75
> > > > +++++++++++++++++++++++------------------ 1 files changed, 42
> > > > insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/processor_core.c
> > > > b/drivers/acpi/processor_core.c index 0c670dd..35d33e8 100644
> > > > --- a/drivers/acpi/processor_core.c
> > > > +++ b/drivers/acpi/processor_core.c
> > > > @@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(struct
> > > > acpi_device *device) /* Use the acpiid in MADT to map cpus in case of
> > > > SMP */
> > > >
> > > >  #ifndef CONFIG_SMP
> > > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
> > > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) {
> > > > return -1; } #else
> > > >
> > > >  static struct acpi_table_madt *madt;
> > > > @@ -429,27 +429,35 @@ static int map_lapic_id(struct
> > > > acpi_subtable_header *entry, }
> > > >
> > > >  static int map_lsapic_id(struct acpi_subtable_header *entry,
> > > > -		  u32 acpi_id, int *apic_id)
> > > > +		int device_declaration, u32 acpi_id, int *apic_id)
> > > >  {
> > > >  	struct acpi_madt_local_sapic *lsapic =
> > > >  		(struct acpi_madt_local_sapic *)entry;
> > > > +	u32 tmp = (lsapic->id << 8) | lsapic->eid;
> > > > +
> > > >  	/* Only check enabled APICs*/
> > > > -	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
> > > > -		/* First check against id */
> > > > -		if (lsapic->processor_id == acpi_id) {
> > > > -			*apic_id = (lsapic->id << 8) | lsapic->eid;
> > > > -			return 1;
> > > > -		/* Check against optional uid */
> > > > -		} else if (entry->length >= 16 &&
> > > > -			lsapic->uid == acpi_id) {
> > > > -			*apic_id = lsapic->uid;
> > > > -			return 1;
> > > > -		}
> > > > -	}
> > > > +	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
> > > > +		return 0;
> > > > +
> > > > +	/* Device statement declaration type */
> > > > +	if (device_declaration) {
> > > > +		if (entry->length < 16)
> > > > +			printk(KERN_ERR PREFIX
> > > > +			    "Invalid LSAPIC with Device type processor (SAPIC ID %#x)\n",
> > > > +			    tmp);
> > > > +		else if (lsapic->uid == acpi_id)
> > > > +			goto found;
> > > > +	/* Processor statement declaration type */
> > > > +	} else if (lsapic->processor_id == acpi_id)
> > > > +		goto found;
> > > > +
> > > >  	return 0;
> > > > +found:
> > > > +	*apic_id = tmp;
> > > > +	return 1;
> > > >  }
> > > >
> > > > -static int map_madt_entry(u32 acpi_id)
> > > > +static int map_madt_entry(int type, u32 acpi_id)
> > > >  {
> > > >  	unsigned long madt_end, entry;
> > > >  	int apic_id = -1;
> > > > @@ -470,7 +478,7 @@ static int map_madt_entry(u32 acpi_id)
> > > >  			if (map_lapic_id(header, acpi_id, &apic_id))
> > > >  				break;
> > > >  		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > > -			if (map_lsapic_id(header, acpi_id, &apic_id))
> > > > +			if (map_lsapic_id(header, type, acpi_id, &apic_id))
> > > >  				break;
> > > >  		}
> > > >  		entry += header->length;
> > > > @@ -478,7 +486,7 @@ static int map_madt_entry(u32 acpi_id)
> > > >  	return apic_id;
> > > >  }
> > > >
> > > > -static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> > > > +static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
> > > >  {
> > > >  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > > >  	union acpi_object *obj;
> > > > @@ -501,7 +509,7 @@ static int map_mat_entry(acpi_handle handle, u32
> > > > acpi_id) if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
> > > >  		map_lapic_id(header, acpi_id, &apic_id);
> > > >  	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > > -		map_lsapic_id(header, acpi_id, &apic_id);
> > > > +		map_lsapic_id(header, type, acpi_id, &apic_id);
> > > >  	}
> > > >
> > > >  exit:
> > > > @@ -510,14 +518,14 @@ exit:
> > > >  	return apic_id;
> > > >  }
> > > >
> > > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> > > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
> > > >  {
> > > >  	int i;
> > > >  	int apic_id = -1;
> > > >
> > > > -	apic_id = map_mat_entry(handle, acpi_id);
> > > > +	apic_id = map_mat_entry(handle, type, acpi_id);
> > > >  	if (apic_id == -1)
> > > > -		apic_id = map_madt_entry(acpi_id);
> > > > +		apic_id = map_madt_entry(type, acpi_id);
> > > >  	if (apic_id == -1)
> > > >  		return apic_id;
> > > >
> > > > @@ -533,15 +541,16 @@ static int get_cpu_id(acpi_handle handle, u32
> > > > acpi_id) Driver Interface
> > > >    
> > > > ---------------------------------------------------------------------
> > > >----- */
> > > >
> > > > -static int acpi_processor_get_info(struct acpi_processor *pr,
> > > > unsigned has_uid) +static int acpi_processor_get_info(struct
> > > > acpi_device *device) {
> > > >  	acpi_status status = 0;
> > > >  	union acpi_object object = { 0 };
> > > >  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> > > > -	int cpu_index;
> > > > +	struct acpi_processor *pr;
> > > > +	int cpu_index, device_declaration = 0;
> > > >  	static int cpu0_initialized;
> > > >
> > > > -
> > > > +	pr = acpi_driver_data(device);
> > > >  	if (!pr)
> > > >  		return -EINVAL;
> > > >
> > > > @@ -562,8 +571,11 @@ static int acpi_processor_get_info(struct
> > > > acpi_processor *pr, unsigned has_uid) ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > > >  				  "No bus mastering arbitration control\n"));
> > > >
> > > > -	/* Check if it is a Device with HID and UID */
> > > > -	if (has_uid) {
> > > > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> > > > +		/*
> > > > +		 * Declared with "Device" statement; match _UID.
> > > > +		 * Note that we don't handle string _UIDs yet.
> > >
> > > Looks very good.
> > > Can you add the check whether the device.flags.unique_id exists before
> > > evaluating the _UID object?
> > > If not exist, it indicates that the processor definition is incorrect.
> >
> > The additional check would create a relationship with
> > 'device.flags.unique_id' which seems redundant and would introduce
> > unnecessary complexity going forward.  While such an additional check
> > would possibly short circuit the call to 'acpi_evaluate_integer()' -
> > when FW is in error and a _UID child object does not exist; a case that
> > is already caught - this code is not in a performance path and thus
> > seems to yield no benefit.
>
> In your patch the device.flags.unique_id is not used. Maybe on some
> systems the processor is defined by Device. But there is no _UID
> object.This is incorrect.
>    IMO in such case we should catch such error.

If defective firmware uses a Device declaration for a processor, but
does not supply a _UID method, Myron's patch will attempt to evaluate
_UID, the evaluation will fail because _UID doesn't exist, we'll print
a message, and return -ENODEV.

That's the same way other errors in acpi_processor_get_info() are
handled.  Are you proposing that this one (a Device declaration
without _UID) should be handled differently?  How would you
suggest that it be handled?

Bjorn

> > > > +		 */
> > > >  		unsigned long long value;
> > > >  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> > > >  						NULL, &value);
> > > > @@ -571,13 +583,10 @@ static int acpi_processor_get_info(struct
> > > > acpi_processor *pr, unsigned has_uid) printk(KERN_ERR PREFIX
> > > > "Evaluating processor _UID\n");
> > > >  			return -ENODEV;
> > > >  		}
> > > > +		device_declaration = 1;
> > > >  		pr->acpi_id = value;
> > > >  	} else {
> > > > -		/*
> > > > -		* Evalute the processor object.  Note that it is common on SMP to
> > > > -		* have the first (boot) processor with a valid PBLK address while
> > > > -		* all others have a NULL address.
> > > > -		*/
> > > > +		/* Declared with "Processor" statement; match ProcessorID */
> > > >  		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
> > > >  		if (ACPI_FAILURE(status)) {
> > > >  			printk(KERN_ERR PREFIX "Evaluating processor object\n");
> > > > @@ -590,7 +599,7 @@ static int acpi_processor_get_info(struct
> > > > acpi_processor *pr, unsigned has_uid) */
> > > >  		pr->acpi_id = object.processor.proc_id;
> > > >  	}
> > > > -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> > > > +	cpu_index = get_cpu_id(pr->handle, device_declaration,
> > > > pr->acpi_id);
> > > >
> > > >  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
> > > >  	if (!cpu0_initialized && (cpu_index == -1) &&
> > > > @@ -662,7 +671,7 @@ static int __cpuinit acpi_processor_start(struct
> > > > acpi_device *device)
> > > >
> > > >  	pr = acpi_driver_data(device);
> > > >
> > > > -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> > > > +	result = acpi_processor_get_info(device);
> > > >  	if (result) {
> > > >  		/* Processor is physically not present */
> > > >  		return 0;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] ACPI: Behave uniquely based on processor declaration definition type

[Myron Stowe]

On Mon, 2008-11-03 at 09:15 +0800, Zhao Yakui wrote:
> On Mon, 2008-11-03 at 08:10 +0800, Myron Stowe wrote:
> > On Fri, 2008-10-31 at 09:19 +0800, Zhao Yakui wrote:
> > > On Fri, 2008-10-31 at 06:13 +0800, Myron Stowe wrote:
snip
> > > >  
> > > > @@ -562,8 +571,11 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > > >  				  "No bus mastering arbitration control\n"));
> > > >  
> > > > -	/* Check if it is a Device with HID and UID */
> > > > -	if (has_uid) {
> > > > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> > > > +		/*
> > > > +		 * Declared with "Device" statement; match _UID.
> > > > +		 * Note that we don't handle string _UIDs yet.
> > > Looks very good. 
> > > Can you add the check whether the device.flags.unique_id exists before
> > > evaluating the _UID object? 
> > > If not exist, it indicates that the processor definition is incorrect.
> > 
> > The additional check would create a relationship with
> > 'device.flags.unique_id' which seems redundant and would introduce
> > unnecessary complexity going forward.  While such an additional check
> > would possibly short circuit the call to 'acpi_evaluate_integer()' -
> > when FW is in error and a _UID child object does not exist; a case that
> > is already caught - this code is not in a performance path and thus
> > seems to yield no benefit.
> In your patch the device.flags.unique_id is not used.
Yes, instead the explicit indicator that [Patch 1/3] introduced was used
so one can explicitly destinguish between "Processor" declared CPU
devices and "Device" declared CPU devices.  This was mainly because it
is valid for both declaration types to have _UID child objects (but only
"Device" declared devices will use the _UID for mapping purposes as we
have already covered and agreed upon).
>  Maybe on some
> systems the processor is defined by Device. But there is no _UID
> object.This is incorrect.
Agreed, this would be incorrect - a platform FW error.
>    IMO in such case we should catch such error.
There are a number of reasons that 'acpi_processor_get_info()' can fail.
They all return some type of -ERRNO to 'acpi_processor_start()' which,
upon receiving a non-zero value, short circuits out due to "Processor is
physically not present".

Are you suggesting that this case is significantly different from any of
the other error cases and should be handled seperately (currently all
error cases are handled in the same fashion)?  If so, what specifically
were you thinking should be done?

Thanks,
Myron
>   
> Best regards.
>    Yakui
> > Was there some other aspect that you were thinking of?
> > 
> > Myron
> > 
> > > Thanks.
> > > > +		 */
> > > >  		unsigned long long value;
> > > >  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> > > >  						NULL, &value);
> > > > @@ -571,13 +583,10 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > >  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
> > > >  			return -ENODEV;
> > > >  		}
> > > > +		device_declaration = 1;
> > > >  		pr->acpi_id = value;
> > > >  	} else {
> > > > -		/*
> > > > -		* Evalute the processor object.  Note that it is common on SMP to
> > > > -		* have the first (boot) processor with a valid PBLK address while
> > > > -		* all others have a NULL address.
> > > > -		*/
> > > > +		/* Declared with "Processor" statement; match ProcessorID */
> > > >  		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
> > > >  		if (ACPI_FAILURE(status)) {
> > > >  			printk(KERN_ERR PREFIX "Evaluating processor object\n");
> > > > @@ -590,7 +599,7 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > >  		*/
> > > >  		pr->acpi_id = object.processor.proc_id;
> > > >  	}
> > > > -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> > > > +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
> > > >  
> > > >  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
> > > >  	if (!cpu0_initialized && (cpu_index == -1) &&
> > > > @@ -662,7 +671,7 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device)
> > > >  
> > > >  	pr = acpi_driver_data(device);
> > > >  
> > > > -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> > > > +	result = acpi_processor_get_info(device);
> > > >  	if (result) {
> > > >  		/* Processor is physically not present */
> > > >  		return 0;
> > > > 
> > > 
> 
-- 
Myron Stowe                             HP Open Source & Linux Org

Re: [PATCH v2 2/3] ACPI: Behave uniquely based on processor declaration definition type

[Zhao Yakui]

On Mon, 2008-11-03 at 10:51 +0800, Myron Stowe wrote:
> On Mon, 2008-11-03 at 09:15 +0800, Zhao Yakui wrote:
> > On Mon, 2008-11-03 at 08:10 +0800, Myron Stowe wrote:
> > > On Fri, 2008-10-31 at 09:19 +0800, Zhao Yakui wrote:
> > > > On Fri, 2008-10-31 at 06:13 +0800, Myron Stowe wrote:
> snip
> > > > >  
> > > > > @@ -562,8 +571,11 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > > >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > > > >  				  "No bus mastering arbitration control\n"));
> > > > >  
> > > > > -	/* Check if it is a Device with HID and UID */
> > > > > -	if (has_uid) {
> > > > > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> > > > > +		/*
> > > > > +		 * Declared with "Device" statement; match _UID.
> > > > > +		 * Note that we don't handle string _UIDs yet.
> > > > Looks very good. 
> > > > Can you add the check whether the device.flags.unique_id exists before
> > > > evaluating the _UID object? 
> > > > If not exist, it indicates that the processor definition is incorrect.
> > > 
> > > The additional check would create a relationship with
> > > 'device.flags.unique_id' which seems redundant and would introduce
> > > unnecessary complexity going forward.  While such an additional check
> > > would possibly short circuit the call to 'acpi_evaluate_integer()' -
> > > when FW is in error and a _UID child object does not exist; a case that
> > > is already caught - this code is not in a performance path and thus
> > > seems to yield no benefit.
> > In your patch the device.flags.unique_id is not used.
> Yes, instead the explicit indicator that [Patch 1/3] introduced was used
> so one can explicitly destinguish between "Processor" declared CPU
> devices and "Device" declared CPU devices.  This was mainly because it
> is valid for both declaration types to have _UID child objects (but only
> "Device" declared devices will use the _UID for mapping purposes as we
> have already covered and agreed upon).
> >  Maybe on some
> > systems the processor is defined by Device. But there is no _UID
> > object.This is incorrect.
> Agreed, this would be incorrect - a platform FW error.
When there is no _UID object for the processor definition using Device,
it is a FW error. And this error should be printed. 
Of course this error is detected by the acpi_evaluate_integer. But if a
string is returned by _UID object, the acpi_evaluate_integer will also
return failure. But in such case we can't know the exact error from the
dmesg.

IMO It is unnecessary to evaluate the _UID object when there is _UID
object(by checking the device.flags.unique_id). In such case the error
info is printed. (" BIOS bug : no _UID object for the processor
definition using device").

When there exists the _UID object, the acpi_evaluate_integer will be
called.  And the return value of _UID is regarded as the ACPI processor
ID. If AE_OK is not returned by acpi_evaluate_integer, maybe it is
caused by other error(For example: mismatch type). In such case the log
info is helpful to get the root cause.

Of course it is also OK that the error is detected by the
acpi_evaluate_integer.

Best regards.
   Yakui
> >    IMO in such case we should catch such error.
> There are a number of reasons that 'acpi_processor_get_info()' can fail.
> They all return some type of -ERRNO to 'acpi_processor_start()' which,
> upon receiving a non-zero value, short circuits out due to "Processor is
> physically not present".
> 
> Are you suggesting that this case is significantly different from any of
> the other error cases and should be handled seperately (currently all
> error cases are handled in the same fashion)?  If so, what specifically
> were you thinking should be done?
> 
> Thanks,
> Myron
> >   
> > Best regards.
> >    Yakui
> > > Was there some other aspect that you were thinking of?
> > > 
> > > Myron
> > > 
> > > > Thanks.
> > > > > +		 */
> > > > >  		unsigned long long value;
> > > > >  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> > > > >  						NULL, &value);
> > > > > @@ -571,13 +583,10 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > > >  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
> > > > >  			return -ENODEV;
> > > > >  		}
> > > > > +		device_declaration = 1;
> > > > >  		pr->acpi_id = value;
> > > > >  	} else {
> > > > > -		/*
> > > > > -		* Evalute the processor object.  Note that it is common on SMP to
> > > > > -		* have the first (boot) processor with a valid PBLK address while
> > > > > -		* all others have a NULL address.
> > > > > -		*/
> > > > > +		/* Declared with "Processor" statement; match ProcessorID */
> > > > >  		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
> > > > >  		if (ACPI_FAILURE(status)) {
> > > > >  			printk(KERN_ERR PREFIX "Evaluating processor object\n");
> > > > > @@ -590,7 +599,7 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > > >  		*/
> > > > >  		pr->acpi_id = object.processor.proc_id;
> > > > >  	}
> > > > > -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> > > > > +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
> > > > >  
> > > > >  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
> > > > >  	if (!cpu0_initialized && (cpu_index == -1) &&
> > > > > @@ -662,7 +671,7 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device)
> > > > >  
> > > > >  	pr = acpi_driver_data(device);
> > > > >  
> > > > > -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> > > > > +	result = acpi_processor_get_info(device);
> > > > >  	if (result) {
> > > > >  		/* Processor is physically not present */
> > > > >  		return 0;
> > > > > 
> > > > 
> > 

Re: [PATCH v2 2/3] ACPI: Behave uniquely based on processor declaration definition type

[Bjorn Helgaas]

On Sunday 02 November 2008 08:59:02 pm Zhao Yakui wrote:
> On Mon, 2008-11-03 at 10:51 +0800, Myron Stowe wrote:
> > On Mon, 2008-11-03 at 09:15 +0800, Zhao Yakui wrote:
> > > On Mon, 2008-11-03 at 08:10 +0800, Myron Stowe wrote:
> > > > On Fri, 2008-10-31 at 09:19 +0800, Zhao Yakui wrote:
> > > > > On Fri, 2008-10-31 at 06:13 +0800, Myron Stowe wrote:
> > snip
> > > > > >  
> > > > > > @@ -562,8 +571,11 @@ static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > > > >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > > > > >  				  "No bus mastering arbitration control\n"));
> > > > > >  
> > > > > > -	/* Check if it is a Device with HID and UID */
> > > > > > -	if (has_uid) {
> > > > > > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> > > > > > +		/*
> > > > > > +		 * Declared with "Device" statement; match _UID.
> > > > > > +		 * Note that we don't handle string _UIDs yet.
> > > > > Looks very good. 
> > > > > Can you add the check whether the device.flags.unique_id exists before
> > > > > evaluating the _UID object? 
> > > > > If not exist, it indicates that the processor definition is incorrect.
> > > > 
> > > > The additional check would create a relationship with
> > > > 'device.flags.unique_id' which seems redundant and would introduce
> > > > unnecessary complexity going forward.  While such an additional check
> > > > would possibly short circuit the call to 'acpi_evaluate_integer()' -
> > > > when FW is in error and a _UID child object does not exist; a case that
> > > > is already caught - this code is not in a performance path and thus
> > > > seems to yield no benefit.
> > > In your patch the device.flags.unique_id is not used.
> > Yes, instead the explicit indicator that [Patch 1/3] introduced was used
> > so one can explicitly destinguish between "Processor" declared CPU
> > devices and "Device" declared CPU devices.  This was mainly because it
> > is valid for both declaration types to have _UID child objects (but only
> > "Device" declared devices will use the _UID for mapping purposes as we
> > have already covered and agreed upon).
> > >  Maybe on some
> > > systems the processor is defined by Device. But there is no _UID
> > > object.This is incorrect.
> > Agreed, this would be incorrect - a platform FW error.
> When there is no _UID object for the processor definition using Device,
> it is a FW error. And this error should be printed. 
> Of course this error is detected by the acpi_evaluate_integer. But if a
> string is returned by _UID object, the acpi_evaluate_integer will also
> return failure. But in such case we can't know the exact error from the
> dmesg.
> 
> IMO It is unnecessary to evaluate the _UID object when there is _UID
> object(by checking the device.flags.unique_id). In such case the error
> info is printed. (" BIOS bug : no _UID object for the processor
> definition using device").
> 
> When there exists the _UID object, the acpi_evaluate_integer will be
> called.  And the return value of _UID is regarded as the ACPI processor
> ID. If AE_OK is not returned by acpi_evaluate_integer, maybe it is
> caused by other error(For example: mismatch type). In such case the log
> info is helpful to get the root cause.
> 
> Of course it is also OK that the error is detected by the
> acpi_evaluate_integer.

I think you're proposing this (this is only pseudo-code):

	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
		/* Declared with "Device" statement; match _UID. */
		if (!device->flags.unique_id) {
			printk(KERN_ERR PREFIX "BIOS bug: no _UID object for processor Device definition\n");
			return -ENODEV;
		}
                status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID, NULL, &value);
		if (ACPI_FAILURE(status)) {
			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
			return -ENODEV;
		}
	} else {
		/* Declared with "Processor" statement; match ProcessorID */
		...
	}

I think it is pointless to add code to check device->flags.unique_id.

The only benefit is that we get a different error message for a
specific kind of firmware defect.  For someone bringing up a platform
with new firmware that uses a string _UID or is defective, this might
save five minutes.  But the extra code takes up space in *everybody's*
kernel all the time.

And it makes the code more complicated because it adds a
dependency on "device->flags.unique_id", so code readers now have
to figure out what that means and whether it is set correctly.

Maybe it would be worthwhile to change the second error message to
something like this:

	printk(KERN_ERR PREFIX "Evaluating processor _UID (%x)\n", status);

Then one could probably distinguish the "_UID didn't exist" error from
the "_UID is not an integer" error.

Would that address your concern?

Bjorn

[PATCH v2 3/3] ACPI: 80 column adherence and spelling fix (no functional change)

[Myron Stowe]

Signed-off-by: Myron Stowe <myron.stowe@hp.com>
---

 drivers/acpi/processor_core.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 35d33e8..10088ae 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -594,9 +594,10 @@ static int acpi_processor_get_info(struct acpi_device *device)
 		}
 
 		/*
-		* TBD: Synch processor ID (via LAPIC/LSAPIC structures) on SMP.
-		*      >>> 'acpi_get_processor_id(acpi_id, &id)' in arch/xxx/acpi.c
-		*/
+		 * TBD: Synch processor ID (via LAPIC/LSAPIC structures) on SMP.
+		 *      >>> 'acpi_get_processor_id(acpi_id, &id)' in
+		 *      arch/xxx/acpi.c
+		 */
 		pr->acpi_id = object.processor.proc_id;
 	}
 	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);