diff mbox series

[v3,1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

Message ID 20190628181457.6609-2-jeremy.linton@arm.com
State Superseded
Headers show
Series arm64/PPTT ACPI 6.3 thread flag support | expand

Commit Message

Jeremy Linton June 28, 2019, 6:14 p.m. UTC
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  | 62 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/acpi.h |  5 ++++
 2 files changed, 66 insertions(+), 1 deletion(-)

-- 
2.21.0

Comments

Rafael J. Wysocki July 3, 2019, 9:24 a.m. UTC | #1
On Fri, Jun 28, 2019 at 8:15 PM Jeremy Linton <jeremy.linton@arm.com> 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@arm.com>

> ---

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

> +{

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

> +       if (saved_pptt_rev > -1 && saved_pptt_rev < rev)

> +               return -ENOENT;


It should be fine to return 'false' here, shouldn't it?

> +

> +       status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);

> +       if (ACPI_FAILURE(status)) {

> +               saved_pptt_rev = 0;

> +               acpi_pptt_warn_missing();

> +               return ret;


And here?

> +       }

> +

> +       saved_pptt_rev = table->revision;

> +

> +       if (saved_pptt_rev >= rev)

> +               cpu_node = acpi_find_processor_node(table, acpi_cpu_id);

> +

> +       if (cpu_node)

> +               ret = (cpu_node->flags & flag) != 0;


And this can be

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 +627,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 +702,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

>
Jeremy Linton July 3, 2019, 3:11 p.m. UTC | #2
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@arm.com> 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@arm.com>

>> ---

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

> 

>> +{

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

> 

>> +       if (saved_pptt_rev > -1 && saved_pptt_rev < rev)

>> +               return -ENOENT;

> 

> It should be fine to return 'false' here, shouldn't it?


See bool, The alternative of course is a dedicated function to export 
this "error" but I think we want to avoid exporting what is effectively 
a version check into the reset of the kernel.

> 

>> +

>> +       status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);

>> +       if (ACPI_FAILURE(status)) {

>> +               saved_pptt_rev = 0;

>> +               acpi_pptt_warn_missing();

>> +               return ret;

> 

> And here?

> 

>> +       }

>> +

>> +       saved_pptt_rev = table->revision;

>> +

>> +       if (saved_pptt_rev >= rev)

>> +               cpu_node = acpi_find_processor_node(table, acpi_cpu_id);

>> +

>> +       if (cpu_node)

>> +               ret = (cpu_node->flags & flag) != 0;

> 

> And this can be

> 

> ret = !!(cpu_node->flags & flag);


Ok, sure.


> 

>> +

>> +       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 +627,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 +702,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

>>
Rafael J. Wysocki July 3, 2019, 9:57 p.m. UTC | #3
On Wed, Jul 3, 2019 at 5:11 PM Jeremy Linton <jeremy.linton@arm.com> 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@arm.com> 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@arm.com>

> >> ---

> >>   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?
Jeremy Linton July 4, 2019, 2:41 a.m. UTC | #4
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@arm.com> 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@arm.com> 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@arm.com>

>>>> ---

>>>>    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.
Rafael J. Wysocki July 9, 2019, 7:19 a.m. UTC | #5
On Thu, Jul 4, 2019 at 4:41 AM Jeremy Linton <jeremy.linton@arm.com> 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@arm.com> 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@arm.com> 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@arm.com>

> >>>> ---

> >>>>    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.
John Garry July 12, 2019, 7:21 a.m. UTC | #6
在 03/07/2019 17:24, Rafael J. Wysocki 写道:
> On Fri, Jun 28, 2019 at 8:15 PM Jeremy Linton <jeremy.linton@arm.com> 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@arm.com>

>> ---

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

> 

>> +{

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


I would have made saved_pptt_rev a global, but, indeed, caching the 
value is not so important.

> 

>> +       if (saved_pptt_rev > -1 && saved_pptt_rev < rev)

>> +               return -ENOENT;

> 

> It should be fine to return 'false' here, shouldn't it?

> 

>> +

>> +       status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);

>> +       if (ACPI_FAILURE(status)) {

>> +               saved_pptt_rev = 0;

>> +               acpi_pptt_warn_missing();

>> +               return ret;

> 

> And here?

> 

>> +       }

>> +

>> +       saved_pptt_rev = table->revision;

>> +

>> +       if (saved_pptt_rev >= rev)

>> +               cpu_node = acpi_find_processor_node(table, acpi_cpu_id);

>> +

>> +       if (cpu_node)

>> +               ret = (cpu_node->flags & flag) != 0;

> 

> And this can be

> 

> 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 +627,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);

>> +}


You could also consider the following to avoid the tri-state return value:

bool acpi_pptt_cpu_find_threaded(unsigned int cpu, bool *is_threaded)

>> +

>>   /**

>>    * find_acpi_cpu_topology() - Determine a unique topology value for a given CPU

>>    * @cpu: Kernel logical CPU number

>> @@ -641,7 +702,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

>>

> 

> .

>
diff mbox series

Patch

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)
+{
+	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 */
+	if (saved_pptt_rev > -1 && saved_pptt_rev < rev)
+		return -ENOENT;
+
+	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+	if (ACPI_FAILURE(status)) {
+		saved_pptt_rev = 0;
+		acpi_pptt_warn_missing();
+		return ret;
+	}
+
+	saved_pptt_rev = table->revision;
+
+	if (saved_pptt_rev >= rev)
+		cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
+
+	if (cpu_node)
+		ret = (cpu_node->flags & flag) != 0;
+
+	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 +627,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 +702,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;