Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1590299

Re: [PATCH v2 3/4] acpi: Fix the check handle in case of declaring processors using the Device operator

Show all headers | View raw

On Mon, 20 Feb 2017, Dou Liyang wrote:

> In ACPI spec, we can declare processors using both Processor and
> Device operator. And before we use the ACPI table, we should check
> the correctness for all processors in ACPI namespace.
> 
> But, Currently, the check handle is just include only the processors
> which are declared by Processor operator. It misses the processors
> declared by Device operator.
> 
> The patch adds the case of Device operator.

See the comments in the previous mails. They apply here as well.

Though this changelog is actively confusing. The subject line says:

  acpi: Fix the check handle in case of declaring processors using the Device
  	operator

Aside of being a way too long subject, it suggests that there is just a
missing check for the case where a processor is declared via the Device
operator. But that's not what the patch is doing.

It implements the distinction between Device and Processor operator, which
is missing in acpi_processor_ids_walk() right now.

So the proper changelog (if I understand the patch correctly) would be:

Subject: acpi/processor: Implement DEVICE operator for processor enumeration

  ACPI allows to declare processors either with the PROCESSOR or with the
  DEVICE operator. The current implementation handles only the PROCESSOR
  operator.

  On a system which uses the DEVICE operator for processor enumeration the
  evaluation fails.

  Check for the ACPI type of the ACPI handle and evaluate PROCESSOR and
  DEVICE types seperately.

Hmm?

>  {
>  	acpi_status status;
> +	acpi_object_type acpi_type;
> +	unsigned long long uid;
>  	union acpi_object object = { 0 };
>  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
>  
> -	status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> -	if (ACPI_FAILURE(status))
> -		acpi_handle_info(handle, "Not get the processor object\n");
> -	else
> -		processor_validated_ids_update(object.processor.proc_id);
> +	status = acpi_get_type(handle, &acpi_type);

Shouldn't the status be checked here?

> +	switch (acpi_type) {
> +	case ACPI_TYPE_PROCESSOR:
> +		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> +		if (ACPI_FAILURE(status))
> +			acpi_handle_info(handle, "Not get the processor object\n");
> +		else
> +			processor_validated_ids_update(
> +						object.processor.proc_id);
> +		break;
> +	case ACPI_TYPE_DEVICE:
> +		status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
> +		if (ACPI_FAILURE(status))
> +			return false;
> +		processor_validated_ids_update(uid);
> +		break;
> +	default:
> +		return false;

This is inconsistent vs. the failure handling in the PROCESSOR and DEVICE
case and the default case does not give any information either.

What about this:

	switch (acpi_type) {
	case ACPI_TYPE_PROCESSOR:
		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
		if (ACPI_FAILURE(status))
			goto err;
		uid = object.processor.proc_id;
		break;
		
	case ACPI_TYPE_DEVICE:
		status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
		if (ACPI_FAILURE(status))
			goto err;
		break;
	default:
		goto err;
	}

	processor_validated_ids_update(uid);
	return true;

err:
	acpi_handle_info(handle, "Invalid processor object\n");
	return false;
}

Thanks,

	tglx

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-02-20 09:50 +0100
  [PATCH v2 3/4] acpi: Fix the check handle in case of declaring processors using the Device operator Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-02-20 09:50 +0100
    Re: [PATCH v2 3/4] acpi: Fix the check handle in case of declaring  processors using the Device operator Thomas Gleixner <tglx@linutronix.de> - 2017-03-01 13:40 +0100
      Re: [PATCH v2 3/4] acpi: Fix the check handle in case of declaring  processors using the Device operator Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-03-02 09:20 +0100
  [PATCH v2 1/4] Revert"x86/acpi: Set persistent cpuid <-> nodeid mapping when booting" Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-02-20 09:50 +0100
    Re: [PATCH v2 1/4] Revert"x86/acpi: Set persistent cpuid <-> nodeid  mapping when booting" Thomas Gleixner <tglx@linutronix.de> - 2017-03-01 12:00 +0100
      Re: [PATCH v2 1/4] Revert"x86/acpi: Set persistent cpuid <-> nodeid  mapping when booting" Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-03-02 09:20 +0100
  [PATCH v2 2/4] Revert"x86/acpi: Enable MADT APIs to return disabled apicids" Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-02-20 09:50 +0100
    Re: [PATCH v2 2/4] Revert"x86/acpi: Enable MADT APIs to return  disabled apicids" Thomas Gleixner <tglx@linutronix.de> - 2017-03-01 12:00 +0100
      Re: [PATCH v2 2/4] Revert"x86/acpi: Enable MADT APIs to return  disabled apicids" Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-03-02 09:20 +0100
  Re: [PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid Ye Xiaolong <xiaolong.ye@intel.com> - 2017-02-21 02:10 +0100
    Re: [PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid Ye Xiaolong <xiaolong.ye@intel.com> - 2017-02-21 08:20 +0100
      Re: [PATCH v2 0/4] Revert works for the mapping of cpuid <-> nodeid Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-02-22 03:00 +0100

csiph-web