Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

From: Jeremy Linton
Date: Fri Jun 28 2019 - 11:21:53 EST


On 6/19/19 4:15 AM, John Garry wrote:
On 18/06/2019 22:28, Jeremy Linton wrote:

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
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
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

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?