| Message ID | 20240409150536.9933-5-miguel.luis@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: processor: refactor acpi_processor_{get_info|remove} | expand |
On Tue, 9 Apr 2024 15:05:33 +0000 Miguel Luis <miguel.luis@oracle.com> 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@oracle.com> 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. 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. > --- > 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, > },
> On 10 Apr 2024, at 13:31, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > On Tue, 9 Apr 2024 15:05:33 +0000 > Miguel Luis <miguel.luis@oracle.com> 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@oracle.com> > 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 ? 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, >> }, > >
On Thu, 11 Apr 2024 11:02:37 +0000 Miguel Luis <miguel.luis@oracle.com> wrote: > > On 10 Apr 2024, at 13:31, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > > On Tue, 9 Apr 2024 15:05:33 +0000 > > Miguel Luis <miguel.luis@oracle.com> 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@oracle.com> > > 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, > >> }, > > > > >
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, },
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@oracle.com> --- drivers/acpi/acpi_processor.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)