Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/2] arm64/PPTT ACPI 6.3 thread flag support
@ 2019-06-14 22:31 Jeremy Linton
  2019-06-14 22:31 ` [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton
  2019-06-14 22:31 ` [PATCH v2 2/2] arm64: topology: Use PPTT to determine if PE is a thread Jeremy Linton
  0 siblings, 2 replies; 11+ messages in thread
From: Jeremy Linton @ 2019-06-14 22:31 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb, Jeremy Linton

ACPI 6.3 adds a flag to the CPU node to indicate whether
the given CPU is a thread. Add a function to return that
information for a given linux logical CPU and then utilize
it while building the arm64 topology.

v1->v2:
	Return ENOENT instead on ENONET.

Jeremy Linton (2):
  ACPI/PPTT: Add support for ACPI 6.3 thread flag
  arm64: topology: Use PPTT to determine if PE is a thread

 arch/arm64/kernel/topology.c |  8 ++++--
 drivers/acpi/pptt.c          | 53 +++++++++++++++++++++++++++++++++++-
 include/linux/acpi.h         |  5 ++++
 3 files changed, 62 insertions(+), 4 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
  2019-06-14 22:31 [PATCH v2 0/2] arm64/PPTT ACPI 6.3 thread flag support Jeremy Linton
@ 2019-06-14 22:31 ` Jeremy Linton
  2019-06-17 12:34   ` Valentin Schneider
  2019-06-14 22:31 ` [PATCH v2 2/2] arm64: topology: Use PPTT to determine if PE is a thread Jeremy Linton
  1 sibling, 1 reply; 11+ messages in thread
From: Jeremy Linton @ 2019-06-14 22:31 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb, Jeremy Linton

ACPI 6.3 adds a flag to the CPU node to indicate whether
the given PE is a thread. Add a function to return that
information for a given linux logical CPU.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/acpi/pptt.c  | 53 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/acpi.h |  5 +++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index b72e6afaa8fb..6f45f8c07b46 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -517,6 +517,43 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
 	return retval;
 }
 
+/**
+ * check_acpi_cpu_flag() - Determine if CPU node has a flag set
+ * @cpu: Kernel logical CPU number
+ * @rev: The PPTT revision defining the flag
+ * @flag: The flag itself
+ *
+ * Check the node representing a CPU for a given flag.
+ *
+ * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
+ *	   the table revision isn't new enough.
+ * Otherwise returns flag value
+ */
+static int check_acpi_cpu_flag(unsigned int cpu, int rev, u32 flag)
+{
+	struct acpi_table_header *table;
+	acpi_status status;
+	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
+	struct acpi_pptt_processor *cpu_node = NULL;
+	int ret = -ENOENT;
+
+	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+	if (ACPI_FAILURE(status)) {
+		acpi_pptt_warn_missing();
+		return ret;
+	}
+
+	if (table->revision >= rev)
+		cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
+
+	if (cpu_node)
+		ret = cpu_node->flags & flag;
+
+	acpi_put_table(table);
+
+	return ret;
+}
+
 /**
  * acpi_find_last_cache_level() - Determines the number of cache levels for a PE
  * @cpu: Kernel logical CPU number
@@ -581,6 +618,21 @@ int cache_setup_acpi(unsigned int cpu)
 	return status;
 }
 
+/**
+ * acpi_pptt_cpu_is_thread() - Determine if CPU is a thread
+ * @cpu: Kernel logical CPU number
+ *
+ *
+ * Return: 1, a thread
+ *         0, not a thread
+ *         -ENOENT ,if the PPTT doesn't exist, the CPU cannot be found or
+ *         the table revision isn't new enough.
+ */
+int acpi_pptt_cpu_is_thread(unsigned int cpu)
+{
+	return check_acpi_cpu_flag(cpu, 2, ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD);
+}
+
 /**
  * find_acpi_cpu_topology() - Determine a unique topology value for a given CPU
  * @cpu: Kernel logical CPU number
@@ -641,7 +693,6 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
 	return ret;
 }
 
-
 /**
  * find_acpi_cpu_topology_package() - Determine a unique CPU package value
  * @cpu: Kernel logical CPU number
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d315d86844e4..3e339375e213 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1301,10 +1301,15 @@ static inline int lpit_read_residency_count_address(u64 *address)
 #endif
 
 #ifdef CONFIG_ACPI_PPTT
+int acpi_pptt_cpu_is_thread(unsigned int cpu);
 int find_acpi_cpu_topology(unsigned int cpu, int level);
 int find_acpi_cpu_topology_package(unsigned int cpu);
 int find_acpi_cpu_cache_topology(unsigned int cpu, int level);
 #else
+static inline int acpi_pptt_cpu_is_thread(unsigned int cpu)
+{
+	return -EINVAL;
+}
 static inline int find_acpi_cpu_topology(unsigned int cpu, int level)
 {
 	return -EINVAL;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 2/2] arm64: topology: Use PPTT to determine if PE is a thread
  2019-06-14 22:31 [PATCH v2 0/2] arm64/PPTT ACPI 6.3 thread flag support Jeremy Linton
  2019-06-14 22:31 ` [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton
@ 2019-06-14 22:31 ` Jeremy Linton
  1 sibling, 0 replies; 11+ messages in thread
From: Jeremy Linton @ 2019-06-14 22:31 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb, Jeremy Linton

ACPI 6.3 adds a thread flag to represent if a CPU/PE is
actually a thread. Given that the MPIDR_MT bit may not
represent this information consistently on homogeneous machines
we should prefer the PPTT flag if its available.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/topology.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 0825c4a856e3..cbbedb53cf06 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -346,11 +346,9 @@ void remove_cpu_topology(unsigned int cpu)
  */
 static int __init parse_acpi_topology(void)
 {
-	bool is_threaded;
+	int is_threaded;
 	int cpu, topology_id;
 
-	is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
-
 	for_each_possible_cpu(cpu) {
 		int i, cache_id;
 
@@ -358,6 +356,10 @@ static int __init parse_acpi_topology(void)
 		if (topology_id < 0)
 			return topology_id;
 
+		is_threaded = acpi_pptt_cpu_is_thread(cpu);
+		if (is_threaded < 0)
+			is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
+
 		if (is_threaded) {
 			cpu_topology[cpu].thread_id = topology_id;
 			topology_id = find_acpi_cpu_topology(cpu, 1);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
  2019-06-14 22:31 ` [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton
@ 2019-06-17 12:34   ` Valentin Schneider
  2019-06-18 14:21     ` Jeremy Linton
  2019-06-25 15:20     ` Sudeep Holla
  0 siblings, 2 replies; 11+ messages in thread
From: Valentin Schneider @ 2019-06-17 12:34 UTC (permalink / raw)
  To: Jeremy Linton, linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb

Hi Jeremy,

Few nits below.

Also, I had a look at the other PPTT processor flags that were introduced
in 6.3, and the only other one being used is ACPI_LEAF_NODE in
acpi_pptt_leaf_node(). However that one already has a handle on the table
header, so the check_acpi_cpu_flag() isn't of much help there.

I don't believe the other existing flags will benefit from the helper since
they are more about describing the PPTT tree, but I think it doesn't hurt
to keep it around for potential future flags.

On 14/06/2019 23:31, Jeremy Linton wrote:
[...]
> @@ -517,6 +517,43 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
>  	return retval;
>  }
>  
> +/**
> + * check_acpi_cpu_flag() - Determine if CPU node has a flag set
> + * @cpu: Kernel logical CPU number
> + * @rev: The PPTT revision defining the flag
> + * @flag: The flag itself
> + *
> + * Check the node representing a CPU for a given flag.
> + *
> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
> + *	   the table revision isn't new enough.
> + * Otherwise returns flag value
> + */

Nit: strictly speaking we're not returning the flag value but its mask
applied to the flags field. I don't think anyone will care about getting
the actual flag value, but it should be made obvious in the doc:

-ENOENT if ...
0 if the flag isn't set
> 0 if it is set.

[...]
> @@ -581,6 +618,21 @@ int cache_setup_acpi(unsigned int cpu)
>  	return status;
>  }
>  
> +/**
> + * acpi_pptt_cpu_is_thread() - Determine if CPU is a thread
> + * @cpu: Kernel logical CPU number
> + *
> + *

Nit: extra newline

> + * Return: 1, a thread
> + *         0, not a thread
> + *         -ENOENT ,if the PPTT doesn't exist, the CPU cannot be found or
> + *         the table revision isn't new enough.
> + */
> +int acpi_pptt_cpu_is_thread(unsigned int cpu)
> +{
> +	return check_acpi_cpu_flag(cpu, 2, ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD);
> +}
> +
>  /**
>   * find_acpi_cpu_topology() - Determine a unique topology value for a given CPU
>   * @cpu: Kernel logical CPU number
> @@ -641,7 +693,6 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
[...]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
  2019-06-17 12:34   ` Valentin Schneider
@ 2019-06-18 14:21     ` Jeremy Linton
  2019-06-18 14:40       ` Valentin Schneider
  2019-06-25 15:20     ` Sudeep Holla
  1 sibling, 1 reply; 11+ messages in thread
From: Jeremy Linton @ 2019-06-18 14:21 UTC (permalink / raw)
  To: Valentin Schneider, linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb

On 6/17/19 7:34 AM, Valentin Schneider wrote:
> Hi Jeremy,
> 
> Few nits below.
> 
> Also, I had a look at the other PPTT processor flags that were introduced
> in 6.3, and the only other one being used is ACPI_LEAF_NODE in
> acpi_pptt_leaf_node(). However that one already has a handle on the table
> header, so the check_acpi_cpu_flag() isn't of much help there.
> 
> I don't believe the other existing flags will benefit from the helper since
> they are more about describing the PPTT tree, but I think it doesn't hurt
> to keep it around for potential future flags.

That was the thought process.

> 
> On 14/06/2019 23:31, Jeremy Linton wrote:
> [...]
>> @@ -517,6 +517,43 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
>>   	return retval;
>>   }
>>   
>> +/**
>> + * check_acpi_cpu_flag() - Determine if CPU node has a flag set
>> + * @cpu: Kernel logical CPU number
>> + * @rev: The PPTT revision defining the flag
>> + * @flag: The flag itself
>> + *
>> + * Check the node representing a CPU for a given flag.
>> + *
>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
>> + *	   the table revision isn't new enough.
>> + * Otherwise returns flag value
>> + */
> 
> Nit: strictly speaking we're not returning the flag value but its mask
> applied to the flags field. I don't think anyone will care about getting
> the actual flag value, but it should be made obvious in the doc:

Or I clarify the code to actually do what the comments says. Maybe that 
is what John G was also pointing out too?


> 
> -ENOENT if ...
> 0 if the flag isn't set
>> 0 if it is set.
> 
> [...]
>> @@ -581,6 +618,21 @@ int cache_setup_acpi(unsigned int cpu)
>>   	return status;
>>   }
>>   
>> +/**
>> + * acpi_pptt_cpu_is_thread() - Determine if CPU is a thread
>> + * @cpu: Kernel logical CPU number
>> + *
>> + *
> 
> Nit: extra newline
> 
>> + * Return: 1, a thread
>> + *         0, not a thread
>> + *         -ENOENT ,if the PPTT doesn't exist, the CPU cannot be found or
>> + *         the table revision isn't new enough.
>> + */
>> +int acpi_pptt_cpu_is_thread(unsigned int cpu)
>> +{
>> +	return check_acpi_cpu_flag(cpu, 2, ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD);
>> +}
>> +
>>   /**
>>    * find_acpi_cpu_topology() - Determine a unique topology value for a given CPU
>>    * @cpu: Kernel logical CPU number
>> @@ -641,7 +693,6 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
> [...]
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
  2019-06-18 14:21     ` Jeremy Linton
@ 2019-06-18 14:40       ` Valentin Schneider
  2019-06-18 17:23         ` John Garry
  0 siblings, 1 reply; 11+ messages in thread
From: Valentin Schneider @ 2019-06-18 14:40 UTC (permalink / raw)
  To: Jeremy Linton, linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb

On 18/06/2019 15:21, Jeremy Linton wrote:
[...]
>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
>>> + *       the table revision isn't new enough.
>>> + * Otherwise returns flag value
>>> + */
>>
>> Nit: strictly speaking we're not returning the flag value but its mask
>> applied to the flags field. I don't think anyone will care about getting
>> the actual flag value, but it should be made obvious in the doc:
> 
> Or I clarify the code to actually do what the comments says. Maybe that is what John G was also pointing out too?
> 

Mmm I didn't find any reply from John regarding this in v1, but I wouldn't
mind either way, as long as the doc & code are aligned.

[...]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
  2019-06-18 14:40       ` Valentin Schneider
@ 2019-06-18 17:23         ` John Garry
  2019-06-18 21:28           ` Jeremy Linton
  0 siblings, 1 reply; 11+ messages in thread
From: John Garry @ 2019-06-18 17:23 UTC (permalink / raw)
  To: Valentin Schneider, Jeremy Linton, linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb

On 18/06/2019 15:40, Valentin Schneider wrote:
> On 18/06/2019 15:21, Jeremy Linton wrote:
> [...]
>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
>>>> + *       the table revision isn't new enough.
>>>> + * Otherwise returns flag value
>>>> + */
>>>
>>> Nit: strictly speaking we're not returning the flag value but its mask
>>> applied to the flags field. I don't think anyone will care about getting
>>> the actual flag value, but it should be made obvious in the doc:
>>
>> Or I clarify the code to actually do what the comments says. Maybe that is what John G was also pointing out too?
>>

No, I was just saying that the kernel topology can be broken without 
this series.

>
> Mmm I didn't find any reply from John regarding this in v1, but I wouldn't
> mind either way, as long as the doc & code are aligned.
>

BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too 
much, i.e. check if the PPTT is new enough to support the thread flag 
and also check if it is set for a specific cpu. I'd consider separate 
functions here.

thanks,
John

> [...]
>
> .
>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
  2019-06-18 17:23         ` John Garry
@ 2019-06-18 21:28           ` Jeremy Linton
  2019-06-19  9:15             ` John Garry
  0 siblings, 1 reply; 11+ messages in thread
From: Jeremy Linton @ 2019-06-18 21:28 UTC (permalink / raw)
  To: John Garry, Valentin Schneider, linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb

Hi,

On 6/18/19 12:23 PM, John Garry wrote:
> On 18/06/2019 15:40, Valentin Schneider wrote:
>> On 18/06/2019 15:21, Jeremy Linton wrote:
>> [...]
>>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be 
>>>>> found or
>>>>> + *       the table revision isn't new enough.
>>>>> + * Otherwise returns flag value
>>>>> + */
>>>>
>>>> Nit: strictly speaking we're not returning the flag value but its mask
>>>> applied to the flags field. I don't think anyone will care about 
>>>> getting
>>>> the actual flag value, but it should be made obvious in the doc:
>>>
>>> Or I clarify the code to actually do what the comments says. Maybe 
>>> that is what John G was also pointing out too?
>>>
> 
> No, I was just saying that the kernel topology can be broken without 
> this series.
> 
>>
>> Mmm I didn't find any reply from John regarding this in v1, but I 
>> wouldn't
>> mind either way, as long as the doc & code are aligned.
>>
> 
> BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too 
> much, i.e. check if the PPTT is new enough to support the thread flag 
> and also check if it is set for a specific cpu. I'd consider separate 
> functions here.

? Your suggesting replacing the


if (table->revision >= rev)
	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);

check with

if (revision_check(table, rev))
	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);


and a function like

static int revision_check(acpixxxx *table, int rev)
{
	return (table->revision >= rev);
}

Although, frankly if one were to do this, it should probably be a macro 
with the table type, and used in the dozen or so other places I found 
doing similar checks (spcr, iort, etc).

Or something else?




> 
> thanks,
> John
> 
>> [...]
>>
>> .
>>
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
  2019-06-18 21:28           ` Jeremy Linton
@ 2019-06-19  9:15             ` John Garry
  2019-06-28 15:21               ` Jeremy Linton
  0 siblings, 1 reply; 11+ messages in thread
From: John Garry @ 2019-06-19  9:15 UTC (permalink / raw)
  To: Jeremy Linton, Valentin Schneider, linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb

On 18/06/2019 22:28, Jeremy Linton wrote:
> Hi,
>
> On 6/18/19 12:23 PM, John Garry wrote:
>> On 18/06/2019 15:40, Valentin Schneider wrote:
>>> On 18/06/2019 15:21, Jeremy Linton wrote:
>>> [...]
>>>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be
>>>>>> found or
>>>>>> + *       the table revision isn't new enough.
>>>>>> + * Otherwise returns flag value
>>>>>> + */
>>>>>
>>>>> Nit: strictly speaking we're not returning the flag value but its mask
>>>>> applied to the flags field. I don't think anyone will care about
>>>>> getting
>>>>> the actual flag value, but it should be made obvious in the doc:
>>>>
>>>> Or I clarify the code to actually do what the comments says. Maybe
>>>> that is what John G was also pointing out too?
>>>>
>>
>> No, I was just saying that the kernel topology can be broken without
>> this series.
>>
>>>
>>> Mmm I didn't find any reply from John regarding this in v1, but I
>>> wouldn't
>>> mind either way, as long as the doc & code are aligned.
>>>
>>
>> BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too
>> much, i.e. check if the PPTT is new enough to support the thread flag
>> and also check if it is set for a specific cpu. I'd consider separate
>> functions here.
>

Hi,

> ? Your suggesting replacing the
>

I am not saying definitely that this should be changed, it's just that 
acpi_pptt_cpu_is_thread() returning false, true, or "no entry" is not a 
typical API format.

How about acpi_pptt_support_thread_info(cpu) and 
acpi_pptt_cpu_is_threaded(cpu), both returning false/true only?

None of this is ideal.

BTW, Have you audited which arm64 systems have MT bit set legitimately?

>
> if (table->revision >= rev)

I know that checking the table revision is not on the fast path, but it 
seems unnecessarily inefficient to always read it this way, I mean 
calling acpi_table_get().

Can you have a static value for the table revision? Or is this just how 
other table info is accessed in ACPI code?

>     cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>
> check with
>
> if (revision_check(table, rev))
>     cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>
>
> and a function like
>
> static int revision_check(acpixxxx *table, int rev)
> {
>     return (table->revision >= rev);
> }
>
> Although, frankly if one were to do this, it should probably be a macro
> with the table type, and used in the dozen or so other places I found
> doing similar checks (spcr, iort, etc).
>
> Or something else?
>
>
>
>

thanks,
John

>>
>>> [...]
>>>
>>> .
>>>
>>
>>
>
>
> .
>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
  2019-06-17 12:34   ` Valentin Schneider
  2019-06-18 14:21     ` Jeremy Linton
@ 2019-06-25 15:20     ` Sudeep Holla
  1 sibling, 0 replies; 11+ messages in thread
From: Sudeep Holla @ 2019-06-25 15:20 UTC (permalink / raw)
  To: Valentin Schneider
  Cc: Jeremy Linton, linux-arm-kernel, linux-acpi, linux-kernel,
	catalin.marinas, will.deacon, rjw, lenb, Sudeep Holla

On Mon, Jun 17, 2019 at 01:34:51PM +0100, Valentin Schneider wrote:
> Hi Jeremy,
>
> Few nits below.
>
> Also, I had a look at the other PPTT processor flags that were introduced
> in 6.3, and the only other one being used is ACPI_LEAF_NODE in
> acpi_pptt_leaf_node(). However that one already has a handle on the table
> header, so the check_acpi_cpu_flag() isn't of much help there.
>
> I don't believe the other existing flags will benefit from the helper since
> they are more about describing the PPTT tree, but I think it doesn't hurt
> to keep it around for potential future flags.
>
> On 14/06/2019 23:31, Jeremy Linton wrote:
> [...]
> > @@ -517,6 +517,43 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
> >  	return retval;
> >  }
> >
> > +/**
> > + * check_acpi_cpu_flag() - Determine if CPU node has a flag set
> > + * @cpu: Kernel logical CPU number
> > + * @rev: The PPTT revision defining the flag
> > + * @flag: The flag itself

How about the "the processor structure flag being examined" ?

> > + *
> > + * Check the node representing a CPU for a given flag.
> > + *
> > + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
> > + *	   the table revision isn't new enough.
> > + * Otherwise returns flag value
> > + */
>
> Nit: strictly speaking we're not returning the flag value but its mask
> applied to the flags field. I don't think anyone will care about getting
> the actual flag value, but it should be made obvious in the doc:
>

I agree with that. I am also fine if you want to change the code to
return 0 or 1 based on the flag value. It then aligns well with comment
under acpi_pptt_cpu_is_thread. Either way, we just need consistency.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
  2019-06-19  9:15             ` John Garry
@ 2019-06-28 15:21               ` Jeremy Linton
  0 siblings, 0 replies; 11+ messages in thread
From: Jeremy Linton @ 2019-06-28 15:21 UTC (permalink / raw)
  To: John Garry, Valentin Schneider, linux-arm-kernel
  Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
	sudeep.holla, lenb

Hi,

On 6/19/19 4:15 AM, John Garry wrote:
> On 18/06/2019 22:28, Jeremy Linton wrote:
>> Hi,
>>
>> On 6/18/19 12:23 PM, John Garry wrote:
>>> On 18/06/2019 15:40, Valentin Schneider wrote:
>>>> On 18/06/2019 15:21, Jeremy Linton wrote:
>>>> [...]
>>>>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be
>>>>>>> found or
>>>>>>> + *       the table revision isn't new enough.
>>>>>>> + * Otherwise returns flag value
>>>>>>> + */
>>>>>>
>>>>>> Nit: strictly speaking we're not returning the flag value but its 
>>>>>> mask
>>>>>> applied to the flags field. I don't think anyone will care about
>>>>>> getting
>>>>>> the actual flag value, but it should be made obvious in the doc:
>>>>>
>>>>> Or I clarify the code to actually do what the comments says. Maybe
>>>>> that is what John G was also pointing out too?
>>>>>
>>>
>>> No, I was just saying that the kernel topology can be broken without
>>> this series.
>>>
>>>>
>>>> Mmm I didn't find any reply from John regarding this in v1, but I
>>>> wouldn't
>>>> mind either way, as long as the doc & code are aligned.
>>>>
>>>
>>> BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too
>>> much, i.e. check if the PPTT is new enough to support the thread flag
>>> and also check if it is set for a specific cpu. I'd consider separate
>>> functions here.
>>
> 
> Hi,
> 
>> ? Your suggesting replacing the
>>
> 
> I am not saying definitely that this should be changed, it's just that 
> acpi_pptt_cpu_is_thread() returning false, true, or "no entry" is not a 
> typical API format.
> 
> How about acpi_pptt_support_thread_info(cpu) and 
> acpi_pptt_cpu_is_threaded(cpu), both returning false/true only?

I'm not sure we want to be exporting what is effectively a version check 
into the rest of the code. Plus, AFAIK it doesn't really simplify 
anything except the case of ACPI machines with revision 1 PPTTs, because 
those would only be doing a single check and assuming the state of the 
MT bit. That MT check is suspect anyway, although AFAIK it gets the 
right answer on all machines that predate ACPI 6.3..


> 
> None of this is ideal.
> 
> BTW, Have you audited which arm64 systems have MT bit set legitimately?

Not formally, given I don't have access to everything available.

> 
>>
>> if (table->revision >= rev)
> 
> I know that checking the table revision is not on the fast path, but it 
> seems unnecessarily inefficient to always read it this way, I mean 
> calling acpi_table_get().
> 
> Can you have a static value for the table revision? Or is this just how 
> other table info is accessed in ACPI code?

Yes caching the revision internally would save a get/put per core, for 
older machines. I don't think its a big deal in normal operation but its 
a couple extra lines so...

I will post it with an internally cached saved_pptt_rev. That will save 
CPU count get/puts in the case where the revision isn't new enough.


> 
>>     cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>>
>> check with
>>
>> if (revision_check(table, rev))
>>     cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>>
>>
>> and a function like
>>
>> static int revision_check(acpixxxx *table, int rev)
>> {
>>     return (table->revision >= rev);
>> }
>>
>> Although, frankly if one were to do this, it should probably be a macro
>> with the table type, and used in the dozen or so other places I found
>> doing similar checks (spcr, iort, etc).
>>
>> Or something else?
>>
>>
>>
>>
> 
> thanks,
> John

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14 22:31 [PATCH v2 0/2] arm64/PPTT ACPI 6.3 thread flag support Jeremy Linton
2019-06-14 22:31 ` [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton
2019-06-17 12:34   ` Valentin Schneider
2019-06-18 14:21     ` Jeremy Linton
2019-06-18 14:40       ` Valentin Schneider
2019-06-18 17:23         ` John Garry
2019-06-18 21:28           ` Jeremy Linton
2019-06-19  9:15             ` John Garry
2019-06-28 15:21               ` Jeremy Linton
2019-06-25 15:20     ` Sudeep Holla
2019-06-14 22:31 ` [PATCH v2 2/2] arm64: topology: Use PPTT to determine if PE is a thread Jeremy Linton

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org linux-acpi@archiver.kernel.org
	public-inbox-index linux-acpi


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/ public-inbox