On Thu, Jul 4, 2019 at 4:41 AM Jeremy Linton <jeremy.linton@xxxxxxx> wrote:
>
> Hi,
>
> On 7/3/19 4:57 PM, Rafael J. Wysocki wrote:
> > On Wed, Jul 3, 2019 at 5:11 PM Jeremy Linton <jeremy.linton@xxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> Thanks for taking a look at this.
> >>
> >> On 7/3/19 4:24 AM, Rafael J. Wysocki wrote:
> >>> On Fri, Jun 28, 2019 at 8:15 PM Jeremy Linton <jeremy.linton@xxxxxxx> wrote:
> >>>>
> >>>> 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@xxxxxxx>
> >>>> ---
> >>>> drivers/acpi/pptt.c | 62 +++++++++++++++++++++++++++++++++++++++++++-
> >>>> include/linux/acpi.h | 5 ++++
> >>>> 2 files changed, 66 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> >>>> index b72e6afaa8fb..bb6196422fad 100644
> >>>> --- a/drivers/acpi/pptt.c
> >>>> +++ b/drivers/acpi/pptt.c
> >>>> @@ -517,6 +517,52 @@ 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.
> >>>> + * 1, any passed flag set
> >>>> + * 0, flag unset
> >>>> + */
> >>>> +static int check_acpi_cpu_flag(unsigned int cpu, int rev, u32 flag)
> >>>
> >>> Why not bool?
> >>
> >> At least for the thread flag we need the three states so that we can
> >> fall back to the CPU's description of itself on machines without ACPI
> >> 6.3 tables.
> >>
> >> The ThunderX2 is threaded and without a firmware update a change like
> >> this will break it.
> >
> > Fair enough.
> >
> >>>
> >>>> +{
> >>>> + 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;
> >>>> + static int saved_pptt_rev = -1;
> >>>> +
> >>>> + /* Cache the PPTT revision to avoid repeat table get/put on failure */
> >>>
> >>> This is a rather questionable optimization.
> >>>
> >>> Does the extra table get/put really matter?
> >>
> >> AFAIK, Probably not.
> >
> > Then why to optimize it?
>
> There was some discussion in the v2 review thread about all the get/put
> operations which only existed to return failure for each core in the
> machine.
>
> https://www.spinics.net/lists/arm-kernel/msg735948.html
>
> I guess I should drop it, until we have some proof that there is a problem.
Yes, please.
