Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Jonathan Cameron]

On Thu, 11 Apr 2024 11:02:37 +0000
Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:

> > On 10 Apr 2024, at 13:31, Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> > 
> > On Tue,  9 Apr 2024 15:05:33 +0000
> > Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
> >   
> >> acpi_unmap_cpu is architecture dependent. Isolate it.
> >> The pre-processor guard for detach may now be restricted to
> >> cpu unmap.
> >> 
> >> Signed-off-by: Miguel Luis <miguel.luis@xxxxxxxxxx>  
> > Again the why question isn't answered by the patch description.
> > 
> > I assume this is to try and resolve the remove question of releasing
> > resources that was outstanding on vCPU HP v4 series Russell posted.
> > 
> > I've not looked as closely at the remove path as the add one yet, but
> > my gut feeling is same issue applies.
> > This code that runs in here should not be dependent on whether
> > CONFIG_ACPI_HOTPLUG_CPU is enabled or not.  
> 
> I agree.
> 
> >  What we do for the
> > make disabled flow should not run a few of the steps in 
> > acpi_processor_remove() we should make that clear by calling
> > a different function that doesn't have those steps.
> >   
> 
> Perhaps this got answered already elsewhere but is it OK for the detach handler
> to be out of CONFIG_ACPI_HOTPLUG_CPU ?

There is code that is again specific to CONFIG_ACPI_HOTPLUG_CPU and
code that is specific to the disabling only case.  So I think the conditions
will end up looking pretty similar to the attach path.
> 
> Miguel
> 
> >> ---
> >> drivers/acpi/acpi_processor.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> >> index c6e2f64a056b..edcd6a8d4735 100644
> >> --- a/drivers/acpi/acpi_processor.c
> >> +++ b/drivers/acpi/acpi_processor.c
> >> @@ -492,6 +492,14 @@ static int acpi_processor_add(struct acpi_device *device,
> >> }
> >> 
> >> #ifdef CONFIG_ACPI_HOTPLUG_CPU
> >> +static void acpi_processor_hotunplug_unmap_cpu(struct acpi_processor *pr)
> >> +{
> >> + acpi_unmap_cpu(pr->id);
> >> +}
> >> +#else
> >> +static void acpi_processor_hotunplug_unmap_cpu(struct acpi_processor *pr) {}
> >> +#endif /* CONFIG_ACPI_HOTPLUG_CPU */
> >> +
> >> /* Removal */
> >> static void acpi_processor_remove(struct acpi_device *device)
> >> {
> >> @@ -524,7 +532,7 @@ static void acpi_processor_remove(struct acpi_device *device)
> >> 
> >> /* Remove the CPU. */
> >> arch_unregister_cpu(pr->id);
> >> - acpi_unmap_cpu(pr->id);
> >> + acpi_processor_hotunplug_unmap_cpu(pr);
> >> 
> >> cpus_write_unlock();
> >> cpu_maps_update_done();
> >> @@ -535,7 +543,6 @@ static void acpi_processor_remove(struct acpi_device *device)
> >> free_cpumask_var(pr->throttling.shared_cpu_map);
> >> kfree(pr);
> >> }
> >> -#endif /* CONFIG_ACPI_HOTPLUG_CPU */
> >> 
> >> #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
> >> bool __init processor_physically_present(acpi_handle handle)
> >> @@ -660,9 +667,7 @@ static const struct acpi_device_id processor_device_ids[] = {
> >> static struct acpi_scan_handler processor_handler = {
> >> .ids = processor_device_ids,
> >> .attach = acpi_processor_add,
> >> -#ifdef CONFIG_ACPI_HOTPLUG_CPU
> >> .detach = acpi_processor_remove,
> >> -#endif
> >> .hotplug = {
> >> .enabled = true,
> >> },  
> > 
> >   
>