Re: [PATCH v2 2/3] ACPI: Behave uniquely based on processordeclaration 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/