diff mbox series

[v7,03/16] ACPI: processor: Drop duplicated check on _STA (enabled + present)

Message ID 20240418135412.14730-4-Jonathan.Cameron@huawei.com
State Superseded
Headers show
Series ACPI/arm64: add support for virtual cpu hotplug | expand

Commit Message

Jonathan Cameron April 18, 2024, 1:53 p.m. UTC
The ACPI bus scan will only result in acpi_processor_add() being called
if _STA has already been checked and the result is that the
processor is enabled and present.  Hence drop this additional check.

Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v7: No change
v6: New patch to drop this unnecessary code. Now I think we only
    need to explicitly read STA to print a warning in the ARM64
    arch_unregister_cpu() path where we want to know if the
    present bit has been unset as well.
---
 drivers/acpi/acpi_processor.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Rafael J. Wysocki April 22, 2024, 6:48 p.m. UTC | #1
On Thu, Apr 18, 2024 at 3:55 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> The ACPI bus scan will only result in acpi_processor_add() being called
> if _STA has already been checked and the result is that the
> processor is enabled and present.  Hence drop this additional check.
>
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

LGTM, so

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
> v7: No change
> v6: New patch to drop this unnecessary code. Now I think we only
>     need to explicitly read STA to print a warning in the ARM64
>     arch_unregister_cpu() path where we want to know if the
>     present bit has been unset as well.
> ---
>  drivers/acpi/acpi_processor.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 7fc924aeeed0..ba0a6f0ac841 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -186,17 +186,11 @@ static void __init acpi_pcc_cpufreq_init(void) {}
>  #ifdef CONFIG_ACPI_HOTPLUG_CPU
>  static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>  {
> -       unsigned long long sta;
> -       acpi_status status;
>         int ret;
>
>         if (invalid_phys_cpuid(pr->phys_id))
>                 return -ENODEV;
>
> -       status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
> -       if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
> -               return -ENODEV;
> -
>         cpu_maps_update_begin();
>         cpus_write_lock();
>
> --
Hanjun Guo April 23, 2024, 6:49 a.m. UTC | #2
On 2024/4/18 21:53, Jonathan Cameron wrote:
> The ACPI bus scan will only result in acpi_processor_add() being called
> if _STA has already been checked and the result is that the
> processor is enabled and present.  Hence drop this additional check.
> 
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> v7: No change
> v6: New patch to drop this unnecessary code. Now I think we only
>      need to explicitly read STA to print a warning in the ARM64
>      arch_unregister_cpu() path where we want to know if the
>      present bit has been unset as well.
> ---
>   drivers/acpi/acpi_processor.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 7fc924aeeed0..ba0a6f0ac841 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -186,17 +186,11 @@ static void __init acpi_pcc_cpufreq_init(void) {}
>   #ifdef CONFIG_ACPI_HOTPLUG_CPU
>   static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>   {
> -	unsigned long long sta;
> -	acpi_status status;
>   	int ret;
>   
>   	if (invalid_phys_cpuid(pr->phys_id))
>   		return -ENODEV;
>   
> -	status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
> -	if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
> -		return -ENODEV;
> -
>   	cpu_maps_update_begin();
>   	cpus_write_lock();

Since the status bits were checked before acpi_processor_add() being
called, do we need to remove the if (!acpi_device_is_enabled(device))
check in acpi_processor_add() as well?

Thanks
Hanjun
Rafael J. Wysocki April 23, 2024, 9:31 a.m. UTC | #3
On Tue, Apr 23, 2024 at 8:49 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>
> On 2024/4/18 21:53, Jonathan Cameron wrote:
> > The ACPI bus scan will only result in acpi_processor_add() being called
> > if _STA has already been checked and the result is that the
> > processor is enabled and present.  Hence drop this additional check.
> >
> > Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > ---
> > v7: No change
> > v6: New patch to drop this unnecessary code. Now I think we only
> >      need to explicitly read STA to print a warning in the ARM64
> >      arch_unregister_cpu() path where we want to know if the
> >      present bit has been unset as well.
> > ---
> >   drivers/acpi/acpi_processor.c | 6 ------
> >   1 file changed, 6 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > index 7fc924aeeed0..ba0a6f0ac841 100644
> > --- a/drivers/acpi/acpi_processor.c
> > +++ b/drivers/acpi/acpi_processor.c
> > @@ -186,17 +186,11 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> >   #ifdef CONFIG_ACPI_HOTPLUG_CPU
> >   static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> >   {
> > -     unsigned long long sta;
> > -     acpi_status status;
> >       int ret;
> >
> >       if (invalid_phys_cpuid(pr->phys_id))
> >               return -ENODEV;
> >
> > -     status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
> > -     if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
> > -             return -ENODEV;
> > -
> >       cpu_maps_update_begin();
> >       cpus_write_lock();
>
> Since the status bits were checked before acpi_processor_add() being
> called, do we need to remove the if (!acpi_device_is_enabled(device))
> check in acpi_processor_add() as well?

No, because its caller only checks the present bit.  The function
itself checks the enabled bit.
Hanjun Guo April 23, 2024, 11:13 a.m. UTC | #4
On 2024/4/23 17:31, Rafael J. Wysocki wrote:
> On Tue, Apr 23, 2024 at 8:49 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>>
>> On 2024/4/18 21:53, Jonathan Cameron wrote:
>>> The ACPI bus scan will only result in acpi_processor_add() being called
>>> if _STA has already been checked and the result is that the
>>> processor is enabled and present.  Hence drop this additional check.
>>>
>>> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>
>>> ---
>>> v7: No change
>>> v6: New patch to drop this unnecessary code. Now I think we only
>>>       need to explicitly read STA to print a warning in the ARM64
>>>       arch_unregister_cpu() path where we want to know if the
>>>       present bit has been unset as well.
>>> ---
>>>    drivers/acpi/acpi_processor.c | 6 ------
>>>    1 file changed, 6 deletions(-)
>>>
>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>>> index 7fc924aeeed0..ba0a6f0ac841 100644
>>> --- a/drivers/acpi/acpi_processor.c
>>> +++ b/drivers/acpi/acpi_processor.c
>>> @@ -186,17 +186,11 @@ static void __init acpi_pcc_cpufreq_init(void) {}
>>>    #ifdef CONFIG_ACPI_HOTPLUG_CPU
>>>    static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>>>    {
>>> -     unsigned long long sta;
>>> -     acpi_status status;
>>>        int ret;
>>>
>>>        if (invalid_phys_cpuid(pr->phys_id))
>>>                return -ENODEV;
>>>
>>> -     status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
>>> -     if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
>>> -             return -ENODEV;
>>> -
>>>        cpu_maps_update_begin();
>>>        cpus_write_lock();
>>
>> Since the status bits were checked before acpi_processor_add() being
>> called, do we need to remove the if (!acpi_device_is_enabled(device))
>> check in acpi_processor_add() as well?
> 
> No, because its caller only checks the present bit.  The function
> itself checks the enabled bit.

Thanks for the pointer, I can see the detail in the acpi_bus_attach()
now,

Reviewed-by: Hanjun Guo <guohanjun@huawei.com>

Thanks
Hanjun
Gavin Shan April 26, 2024, 9:24 a.m. UTC | #5
On 4/18/24 23:53, Jonathan Cameron wrote:
> The ACPI bus scan will only result in acpi_processor_add() being called
> if _STA has already been checked and the result is that the
> processor is enabled and present.  Hence drop this additional check.
> 
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> v7: No change
> v6: New patch to drop this unnecessary code. Now I think we only
>      need to explicitly read STA to print a warning in the ARM64
>      arch_unregister_cpu() path where we want to know if the
>      present bit has been unset as well.
> ---
>   drivers/acpi/acpi_processor.c | 6 ------
>   1 file changed, 6 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>
diff mbox series

Patch

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 7fc924aeeed0..ba0a6f0ac841 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -186,17 +186,11 @@  static void __init acpi_pcc_cpufreq_init(void) {}
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
 static int acpi_processor_hotadd_init(struct acpi_processor *pr)
 {
-	unsigned long long sta;
-	acpi_status status;
 	int ret;
 
 	if (invalid_phys_cpuid(pr->phys_id))
 		return -ENODEV;
 
-	status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
-	if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
-		return -ENODEV;
-
 	cpu_maps_update_begin();
 	cpus_write_lock();