diff mbox series

ACPI/PPTT: use ACPI ID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set

Message ID 1530289077-2444-1-git-send-email-sudeep.holla@arm.com
State New
Headers show
Series ACPI/PPTT: use ACPI ID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set | expand

Commit Message

Sudeep Holla June 29, 2018, 4:17 p.m. UTC 
Currently we use the ACPI processor ID only for the leaf/processor nodes
as the specification states it must match the value of ACPI processor ID
field in the processor’s entry in the MADT.

However, if a PPTT structure represents processors group, it match a
processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
flag describe whether the ACPI processor ID is valid.

Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
consistent instead of using table offset as it's currently done for non
leaf nodes.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/pptt.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Hi,

There's ongoing discussion on assigning ID based in OS using simple
counters. It can never be consistent with firmware's view. So if the
firmware provides valid UID for non-processors node, we must use it.

Regards,
Sudeep

Comments

Jeremy Linton June 29, 2018, 6:18 p.m. UTC | #1 
Hi,

On 06/29/2018 11:17 AM, Sudeep Holla wrote:
> Currently we use the ACPI processor ID only for the leaf/processor nodes
> as the specification states it must match the value of ACPI processor ID
> field in the processor’s entry in the MADT.
> 
> However, if a PPTT structure represents processors group, it match a
> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
> flag describe whether the ACPI processor ID is valid.
> 
> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
> consistent instead of using table offset as it's currently done for non
> leaf nodes.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/acpi/pptt.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Hi,
> 
> There's ongoing discussion on assigning ID based in OS using simple
> counters. It can never be consistent with firmware's view. So if the
> firmware provides valid UID for non-processors node, we must use it.
> 
> Regards,
> Sudeep
> 
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index e5ea1974d1e3..d1e26cb599bf 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
>   	if (cpu_node) {
>   		cpu_node = acpi_find_processor_package_id(table, cpu_node,
>   							  level, flag);
> -		/* Only the first level has a guaranteed id */
> -		if (level == 0)
> +		/*
> +		 * As per specification if the processor structure represents
> +		 * an actual processor, then ACPI processor ID must be valid.
> +		 * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
> +		 * should be set if the UID is valid
> +		 */
> +		if (level == 0 ||
> +		    cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>   			return cpu_node->acpi_processor_id;

While, for some machines this likely helps create more human readable 
ID's... What happens when the ID namespaces conflict with each other?

AKA, I'm a little shy of this change because your going from something 
we can guarantee is unique to depending on an portion of the PPTT 
definition that has a couple different ways that it can be interpreted.

OTOH the change is probably safe at the moment because i don't think 
anyone has partially marked nodes at a given PPTT "level" valid, or put 
structures that aren't part of the PE/cache's in the tree (outside of my 
juno test tree with the GPU's/etc).
Andrew Jones June 30, 2018, 7:16 a.m. UTC | #2 
On Fri, Jun 29, 2018 at 05:17:57PM +0100, Sudeep Holla wrote:
> Currently we use the ACPI processor ID only for the leaf/processor nodes
> as the specification states it must match the value of ACPI processor ID
> field in the processor’s entry in the MADT.
> 
> However, if a PPTT structure represents processors group, it match a
> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
> flag describe whether the ACPI processor ID is valid.
> 
> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
> consistent instead of using table offset as it's currently done for non
> leaf nodes.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/acpi/pptt.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Hi,
> 
> There's ongoing discussion on assigning ID based in OS using simple
> counters. It can never be consistent with firmware's view. So if the
> firmware provides valid UID for non-processors node, we must use it.

I agree with this. I've been so focused on the fact that the ACPI offsets
are arbitrary, and thus counters can't be worse, that I nearly forgot how
these IDs are actually defined:

From Documentation/cputopology.txt

"""
1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:

        physical package id of cpuX. Typically corresponds to a physical
        socket number, but the actual value is architecture and platform
        dependent.

2) /sys/devices/system/cpu/cpuX/topology/core_id:

        the CPU core ID of cpuX. Typically it is the hardware platform's
        identifier (rather than the kernel's).  The actual value is
        architecture and platform dependent.
"""

So all my consistency arguments are moot, as no user should expect
consistency from architecture and platform dependent IDs.

A comment on the patch below

> 
> Regards,
> Sudeep
> 
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index e5ea1974d1e3..d1e26cb599bf 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
>  	if (cpu_node) {
>  		cpu_node = acpi_find_processor_package_id(table, cpu_node,
>  							  level, flag);
> -		/* Only the first level has a guaranteed id */
> -		if (level == 0)
> +		/*
> +		 * As per specification if the processor structure represents
> +		 * an actual processor, then ACPI processor ID must be valid.
> +		 * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
> +		 * should be set if the UID is valid
> +		 */
> +		if (level == 0 ||
> +		    cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>  			return cpu_node->acpi_processor_id;
>  		return ACPI_PTR_DIFF(cpu_node, table);
>  	}
> -- 
> 2.7.4
>

When the valid flag is set we'll now return a [hopefully] correct platform
dependent ID, but when it's not we'll return an ACPI table offset. How
will users of the ID know? Also, it's possible to return -ENOENT for the
ID when calling find_acpi_cpu_topology(). How can we distinguish that from
an arbitrary platform dependent ID?

Thanks,
drew
Rafael J. Wysocki July 2, 2018, 9:06 a.m. UTC | #3 
On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Currently we use the ACPI processor ID only for the leaf/processor nodes
> as the specification states it must match the value of ACPI processor ID
> field in the processor’s entry in the MADT.
>
> However, if a PPTT structure represents processors group, it match a
> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
> flag describe whether the ACPI processor ID is valid.
>
> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
> consistent instead of using table offset as it's currently done for non
> leaf nodes.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/acpi/pptt.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> Hi,
>
> There's ongoing discussion on assigning ID based in OS using simple
> counters. It can never be consistent with firmware's view. So if the
> firmware provides valid UID for non-processors node, we must use it.

OK

Do you regard this as a fix for the recently merged PPTT material?  If
so, I should apply it as a fix for 4.18.

Thanks,
Rafael
Sudeep Holla July 2, 2018, 9:57 a.m. UTC | #4 
On 02/07/18 10:06, Rafael J. Wysocki wrote:
> On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> Currently we use the ACPI processor ID only for the leaf/processor nodes
>> as the specification states it must match the value of ACPI processor ID
>> field in the processor’s entry in the MADT.
>>
>> However, if a PPTT structure represents processors group, it match a
>> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
>> flag describe whether the ACPI processor ID is valid.
>>
>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
>> consistent instead of using table offset as it's currently done for non
>> leaf nodes.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/acpi/pptt.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> Hi,
>>
>> There's ongoing discussion on assigning ID based in OS using simple
>> counters. It can never be consistent with firmware's view. So if the
>> firmware provides valid UID for non-processors node, we must use it.
> 
> OK
> 
> Do you regard this as a fix for the recently merged PPTT material?  If
> so, I should apply it as a fix for 4.18.
> 

Yes, it should be considered as fix IMO.
Sudeep Holla July 2, 2018, 10:01 a.m. UTC | #5 
On 30/06/18 08:16, Andrew Jones wrote:
> On Fri, Jun 29, 2018 at 05:17:57PM +0100, Sudeep Holla wrote:
>> Currently we use the ACPI processor ID only for the leaf/processor nodes
>> as the specification states it must match the value of ACPI processor ID
>> field in the processor’s entry in the MADT.
>>
>> However, if a PPTT structure represents processors group, it match a
>> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
>> flag describe whether the ACPI processor ID is valid.
>>
>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
>> consistent instead of using table offset as it's currently done for non
>> leaf nodes.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/acpi/pptt.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> Hi,
>>
>> There's ongoing discussion on assigning ID based in OS using simple
>> counters. It can never be consistent with firmware's view. So if the
>> firmware provides valid UID for non-processors node, we must use it.
> 
> I agree with this. I've been so focused on the fact that the ACPI offsets
> are arbitrary, and thus counters can't be worse, that I nearly forgot how
> these IDs are actually defined:
> 

Yes, it's platform dependent and I now realize that I never explicitly
mentioned that, let alone emphasize on that. I was for UID for the same
reason.

[...]

> 
> When the valid flag is set we'll now return a [hopefully] correct platform
> dependent ID, but when it's not we'll return an ACPI table offset. How
> will users of the ID know? Also, it's possible to return -ENOENT for the
> ID when calling find_acpi_cpu_topology(). How can we distinguish that from
> an arbitrary platform dependent ID?
> 

But why should one need to distinguish that ? Even offset is kind of
sparsely distributed UID.
Sudeep Holla July 2, 2018, 10:08 a.m. UTC | #6 
On 29/06/18 19:18, Jeremy Linton wrote:
> Hi,
> 
> On 06/29/2018 11:17 AM, Sudeep Holla wrote:
>> Currently we use the ACPI processor ID only for the leaf/processor nodes
>> as the specification states it must match the value of ACPI processor ID
>> field in the processor’s entry in the MADT.
>>
>> However, if a PPTT structure represents processors group, it match a
>> processor container UID in the namespace and
>> ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
>> flag describe whether the ACPI processor ID is valid.
>>
>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
>> consistent instead of using table offset as it's currently done for non
>> leaf nodes.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   drivers/acpi/pptt.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> Hi,
>>
>> There's ongoing discussion on assigning ID based in OS using simple
>> counters. It can never be consistent with firmware's view. So if the
>> firmware provides valid UID for non-processors node, we must use it.
>>
>> Regards,
>> Sudeep
>>
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index e5ea1974d1e3..d1e26cb599bf 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct
>> acpi_table_header *table,
>>       if (cpu_node) {
>>           cpu_node = acpi_find_processor_package_id(table, cpu_node,
>>                                 level, flag);
>> -        /* Only the first level has a guaranteed id */
>> -        if (level == 0)
>> +        /*
>> +         * As per specification if the processor structure represents
>> +         * an actual processor, then ACPI processor ID must be valid.
>> +         * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
>> +         * should be set if the UID is valid
>> +         */
>> +        if (level == 0 ||
>> +            cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>>               return cpu_node->acpi_processor_id;
> 
> While, for some machines this likely helps create more human readable
> ID's... What happens when the ID namespaces conflict with each other?
> 

That's entirely left to the platform firmware. It should help userspace
to identify the topology in a way firmware is describing and no more
than that. If users use them for anything more, it's at their own risk.

> AKA, I'm a little shy of this change because your going from something
> we can guarantee is unique to depending on an portion of the PPTT
> definition that has a couple different ways that it can be interpreted.
> 

No, I am not guaranteeing anything here. I am just passing valid UID if
present to the caller. Interpretation is left to the caller and in ARM64
we should just use(at least my preference) the value as is for sysfs
topology.

> OTOH the change is probably safe at the moment because i don't think
> anyone has partially marked nodes at a given PPTT "level" valid, or put
> structures that aren't part of the PE/cache's in the tree (outside of my
> juno test tree with the GPU's/etc).
> 

Even if they are present, I don't see issue. If that's how firmware
presents the CPU topology, that should be exactly the way we too need to.
Rafael J. Wysocki July 2, 2018, 11:01 a.m. UTC | #7 
On Mon, Jul 2, 2018 at 11:57 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 02/07/18 10:06, Rafael J. Wysocki wrote:
>> On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>> Currently we use the ACPI processor ID only for the leaf/processor nodes
>>> as the specification states it must match the value of ACPI processor ID
>>> field in the processor’s entry in the MADT.
>>>
>>> However, if a PPTT structure represents processors group, it match a
>>> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
>>> flag describe whether the ACPI processor ID is valid.
>>>
>>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
>>> consistent instead of using table offset as it's currently done for non
>>> leaf nodes.
>>>
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>>  drivers/acpi/pptt.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> Hi,
>>>
>>> There's ongoing discussion on assigning ID based in OS using simple
>>> counters. It can never be consistent with firmware's view. So if the
>>> firmware provides valid UID for non-processors node, we must use it.
>>
>> OK
>>
>> Do you regard this as a fix for the recently merged PPTT material?  If
>> so, I should apply it as a fix for 4.18.
>>
>
> Yes, it should be considered as fix IMO.

So any chance to provide a Fixes: tag?
Sudeep Holla July 2, 2018, 12:52 p.m. UTC | #8 
On 02/07/18 12:01, Rafael J. Wysocki wrote:
> On Mon, Jul 2, 2018 at 11:57 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 02/07/18 10:06, Rafael J. Wysocki wrote:
>>> On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>> Currently we use the ACPI processor ID only for the leaf/processor nodes
>>>> as the specification states it must match the value of ACPI processor ID
>>>> field in the processor’s entry in the MADT.
>>>>
>>>> However, if a PPTT structure represents processors group, it match a
>>>> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
>>>> flag describe whether the ACPI processor ID is valid.
>>>>
>>>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
>>>> consistent instead of using table offset as it's currently done for non
>>>> leaf nodes.
>>>>
>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>>  drivers/acpi/pptt.c | 10 ++++++++--
>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> Hi,
>>>>
>>>> There's ongoing discussion on assigning ID based in OS using simple
>>>> counters. It can never be consistent with firmware's view. So if the
>>>> firmware provides valid UID for non-processors node, we must use it.
>>>
>>> OK
>>>
>>> Do you regard this as a fix for the recently merged PPTT material?  If
>>> so, I should apply it as a fix for 4.18.
>>>
>>
>> Yes, it should be considered as fix IMO.
> 
> So any chance to provide a Fixes: tag?

Sure, since it was in the same release, I didn't add it.

Fixes: 2bd00bcd73e5 ("ACPI/PPTT: Add Processor Properties Topology Table
parsing")
Jeremy Linton July 2, 2018, 1:23 p.m. UTC | #9 
Hi,

On 06/29/2018 11:17 AM, Sudeep Holla wrote:
> Currently we use the ACPI processor ID only for the leaf/processor nodes
> as the specification states it must match the value of ACPI processor ID
> field in the processor’s entry in the MADT.
> 
> However, if a PPTT structure represents processors group, it match a
> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
> flag describe whether the ACPI processor ID is valid.
> 
> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
> consistent instead of using table offset as it's currently done for non
> leaf nodes.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/acpi/pptt.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Hi,
> 
> There's ongoing discussion on assigning ID based in OS using simple
> counters. It can never be consistent with firmware's view. So if the
> firmware provides valid UID for non-processors node, we must use it.
> 
> Regards,
> Sudeep
> 
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index e5ea1974d1e3..d1e26cb599bf 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
>   	if (cpu_node) {
>   		cpu_node = acpi_find_processor_package_id(table, cpu_node,
>   							  level, flag);
> -		/* Only the first level has a guaranteed id */
> -		if (level == 0)
> +		/*
> +		 * As per specification if the processor structure represents
> +		 * an actual processor, then ACPI processor ID must be valid.
> +		 * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
> +		 * should be set if the UID is valid
> +		 */
> +		if (level == 0 ||
> +		    cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>   			return cpu_node->acpi_processor_id;
>   		return ACPI_PTR_DIFF(cpu_node, table);
>   	}
> 

Ok sure,

Acked-by: Jeremy Linton <jeremy.linton@arm.com>


PS: To table implementers, the spec today mandates that setting the 
valid flag on a non leaf node means there is a matching _UID processor 
container in DSDT/SSDT.
Jeremy Linton July 2, 2018, 2:41 p.m. UTC | #10 
Hi,

On 07/02/2018 04:57 AM, Sudeep Holla wrote:
> 
> 
> On 02/07/18 10:06, Rafael J. Wysocki wrote:
>> On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>> Currently we use the ACPI processor ID only for the leaf/processor nodes
>>> as the specification states it must match the value of ACPI processor ID
>>> field in the processor’s entry in the MADT.
>>>
>>> However, if a PPTT structure represents processors group, it match a
>>> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
>>> flag describe whether the ACPI processor ID is valid.
>>>
>>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
>>> consistent instead of using table offset as it's currently done for non
>>> leaf nodes.
>>>
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>>   drivers/acpi/pptt.c | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> Hi,
>>>
>>> There's ongoing discussion on assigning ID based in OS using simple
>>> counters. It can never be consistent with firmware's view. So if the
>>> firmware provides valid UID for non-processors node, we must use it.
>>
>> OK
>>
>> Do you regard this as a fix for the recently merged PPTT material?  If
>> so, I should apply it as a fix for 4.18.
>>
> 
> Yes, it should be considered as fix IMO.

Do we know which machines are affected?

I'm mostly agnostic to this patch because I believe its effectively a 
NOP. But that said, it could have a functional change, which implies 
there is some risk.
Rafael J. Wysocki July 4, 2018, 10:38 a.m. UTC | #11 
On Monday, July 2, 2018 2:52:24 PM CEST Sudeep Holla wrote:
> 
> On 02/07/18 12:01, Rafael J. Wysocki wrote:
> > On Mon, Jul 2, 2018 at 11:57 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >>
> >>
> >> On 02/07/18 10:06, Rafael J. Wysocki wrote:
> >>> On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >>>> Currently we use the ACPI processor ID only for the leaf/processor nodes
> >>>> as the specification states it must match the value of ACPI processor ID
> >>>> field in the processor’s entry in the MADT.
> >>>>
> >>>> However, if a PPTT structure represents processors group, it match a
> >>>> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
> >>>> flag describe whether the ACPI processor ID is valid.
> >>>>
> >>>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
> >>>> consistent instead of using table offset as it's currently done for non
> >>>> leaf nodes.
> >>>>
> >>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>>> ---
> >>>>  drivers/acpi/pptt.c | 10 ++++++++--
> >>>>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>
> >>>> Hi,
> >>>>
> >>>> There's ongoing discussion on assigning ID based in OS using simple
> >>>> counters. It can never be consistent with firmware's view. So if the
> >>>> firmware provides valid UID for non-processors node, we must use it.
> >>>
> >>> OK
> >>>
> >>> Do you regard this as a fix for the recently merged PPTT material?  If
> >>> so, I should apply it as a fix for 4.18.
> >>>
> >>
> >> Yes, it should be considered as fix IMO.
> > 
> > So any chance to provide a Fixes: tag?
> 
> Sure, since it was in the same release, I didn't add it.
> 
> Fixes: 2bd00bcd73e5 ("ACPI/PPTT: Add Processor Properties Topology Table
> parsing")
> 
> 

Applied now, thanks!
diff mbox series

Patch

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index e5ea1974d1e3..d1e26cb599bf 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -481,8 +481,14 @@  static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
 	if (cpu_node) {
 		cpu_node = acpi_find_processor_package_id(table, cpu_node,
 							  level, flag);
-		/* Only the first level has a guaranteed id */
-		if (level == 0)
+		/*
+		 * As per specification if the processor structure represents
+		 * an actual processor, then ACPI processor ID must be valid.
+		 * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
+		 * should be set if the UID is valid
+		 */
+		if (level == 0 ||
+		    cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
 			return cpu_node->acpi_processor_id;
 		return ACPI_PTR_DIFF(cpu_node, table);
 	}