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
