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

[Dou Liyang]

Hi tglx,

At 03/01/2017 07:12 PM, Thomas Gleixner wrote:

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?



Yes, you are right. I didn't explain clearly.
I will modify in my next version.


 {
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),  };

-   status = acpi_evaluate_object(handle, NULL, NULL, );
-   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, _type);


Shouldn't the status be checked here?


oops, Yes. Need to be checked.




+   switch (acpi_type) {
+   case ACPI_TYPE_PROCESSOR:
+   status = acpi_evaluate_object(handle, NULL, NULL, );
+   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, );
+   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, );
if (ACPI_FAILURE(status))
goto err;
uid = object.processor.proc_id;
break;

case ACPI_TYPE_DEVICE:
status = acpi_evaluate_integer(handle, "_UID", NULL, );
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;
}



Looks good than mine.

Thanks,
Liyang.


Thanks,

tglx

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

[Dou Liyang]

Hi tglx,

At 03/01/2017 07:12 PM, Thomas Gleixner wrote:

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?



Yes, you are right. I didn't explain clearly.
I will modify in my next version.


 {
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),  };

-   status = acpi_evaluate_object(handle, NULL, NULL, );
-   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, _type);


Shouldn't the status be checked here?


oops, Yes. Need to be checked.




+   switch (acpi_type) {
+   case ACPI_TYPE_PROCESSOR:
+   status = acpi_evaluate_object(handle, NULL, NULL, );
+   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, );
+   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, );
if (ACPI_FAILURE(status))
goto err;
uid = object.processor.proc_id;
break;

case ACPI_TYPE_DEVICE:
status = acpi_evaluate_integer(handle, "_UID", NULL, );
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;
}



Looks good than mine.

Thanks,
Liyang.


Thanks,

tglx

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

[Thomas Gleixner]

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),  };
>  
> - status = acpi_evaluate_object(handle, NULL, NULL, );
> - 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, _type);

Shouldn't the status be checked here?

> + switch (acpi_type) {
> + case ACPI_TYPE_PROCESSOR:
> + status = acpi_evaluate_object(handle, NULL, NULL, );
> + 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, );
> + 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, );
if (ACPI_FAILURE(status))
goto err;
uid = object.processor.proc_id;
break;

case ACPI_TYPE_DEVICE:
status = acpi_evaluate_integer(handle, "_UID", NULL, );
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

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

[Thomas Gleixner]

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),  };
>  
> - status = acpi_evaluate_object(handle, NULL, NULL, );
> - 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, _type);

Shouldn't the status be checked here?

> + switch (acpi_type) {
> + case ACPI_TYPE_PROCESSOR:
> + status = acpi_evaluate_object(handle, NULL, NULL, );
> + 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, );
> + 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, );
if (ACPI_FAILURE(status))
goto err;
uid = object.processor.proc_id;
break;

case ACPI_TYPE_DEVICE:
status = acpi_evaluate_integer(handle, "_UID", NULL, );
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

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

[Dou Liyang]

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.

Signed-off-by: Dou Liyang 
---
 drivers/acpi/acpi_processor.c | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index f43a586..eb500e1 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -633,25 +633,43 @@ static acpi_status __init 
acpi_processor_ids_walk(acpi_handle handle,
  void **rv)
 {
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),  };
 
-   status = acpi_evaluate_object(handle, NULL, NULL, );
-   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, _type);
+   switch (acpi_type) {
+   case ACPI_TYPE_PROCESSOR:
+   status = acpi_evaluate_object(handle, NULL, NULL, );
+   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, );
+   if (ACPI_FAILURE(status))
+   return false;
+   processor_validated_ids_update(uid);
+   break;
+   default:
+   return false;
+   }
 
return AE_OK;
 }
 
-static void __init acpi_processor_check_duplicates(void)
+void __init acpi_processor_check_duplicates(void)
 {
-   /* Search all processor nodes in ACPI namespace */
+   /* check the correctness for all processors in ACPI namespace */
acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
ACPI_UINT32_MAX,
acpi_processor_ids_walk,
NULL, NULL, NULL);
+   acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, acpi_processor_ids_walk,
+   NULL, NULL);
 }
 
 bool __init acpi_processor_validate_proc_id(int proc_id)
-- 
2.5.5

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

[Dou Liyang]

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.

Signed-off-by: Dou Liyang 
---
 drivers/acpi/acpi_processor.c | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index f43a586..eb500e1 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -633,25 +633,43 @@ static acpi_status __init 
acpi_processor_ids_walk(acpi_handle handle,
  void **rv)
 {
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),  };
 
-   status = acpi_evaluate_object(handle, NULL, NULL, );
-   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, _type);
+   switch (acpi_type) {
+   case ACPI_TYPE_PROCESSOR:
+   status = acpi_evaluate_object(handle, NULL, NULL, );
+   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, );
+   if (ACPI_FAILURE(status))
+   return false;
+   processor_validated_ids_update(uid);
+   break;
+   default:
+   return false;
+   }
 
return AE_OK;
 }
 
-static void __init acpi_processor_check_duplicates(void)
+void __init acpi_processor_check_duplicates(void)
 {
-   /* Search all processor nodes in ACPI namespace */
+   /* check the correctness for all processors in ACPI namespace */
acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
ACPI_UINT32_MAX,
acpi_processor_ids_walk,
NULL, NULL, NULL);
+   acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, acpi_processor_ids_walk,
+   NULL, NULL);
 }
 
 bool __init acpi_processor_validate_proc_id(int proc_id)
-- 
2.5.5