| Message ID | 20240409150536.9933-4-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:32 +0000 Miguel Luis <miguel.luis@oracle.com> wrote: > mapping and unmaping a cpu at the stage of extra cpu enumeration is > architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's > isolate that functionality from architecture independent one. Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu() to make the arch specific nature of that call more obvious? I think that has caused more confusion in the discussion than whether it is hotplug specific or not. As mentioned in patch 2, fairly sure this needs to go before that patch. Jonathan > > Signed-off-by: Miguel Luis <miguel.luis@oracle.com> > --- > drivers/acpi/acpi_processor.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > index 9ea58b61d741..c6e2f64a056b 100644 > --- a/drivers/acpi/acpi_processor.c > +++ b/drivers/acpi/acpi_processor.c > @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) > pr_info("CPU%d has been hot-added\n", pr->id); > pr->flags.need_hotplug_init = 1; > } > +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) > +{ > + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); > +} > +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) > +{ > + acpi_unmap_cpu(pr->id); > +} > #else > static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {} > +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) > +{ > + return 0; > +} > +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {} > #endif /* CONFIG_ACPI_HOTPLUG_CPU */ > > /* Enumerate extra CPUs */ > @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr) > cpu_maps_update_begin(); > cpus_write_lock(); > > - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); > + ret = acpi_processor_hotplug_map_cpu(pr); > if (ret) > goto out; > > ret = arch_register_cpu(pr->id); > if (ret) { > - acpi_unmap_cpu(pr->id); > + acpi_processor_hotplug_unmap_cpu(pr); > goto out; > } >
> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > On Tue, 9 Apr 2024 15:05:32 +0000 > Miguel Luis <miguel.luis@oracle.com> wrote: > >> mapping and unmaping a cpu at the stage of extra cpu enumeration is >> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's >> isolate that functionality from architecture independent one. > > Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu() > to make the arch specific nature of that call more obvious? Not sure about the pattern to use here but that seems fine to me. Current usage is architectures export acpi_map_cpu from the acpi interface and do their thing. Question is what to do when there’s a use-case which dismisses acpi_map_cpu and it gets called on the code path? 1) export it and do nothing - it would be creating unnecessary dependency. 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped into CONFIG_ACPI_HOTPLUG_CPU. Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely used for CPU HP and the same applies to acpi_unmap_cpu. > I think that has caused more confusion in the discussion than > whether it is hotplug specific or not. Indeed. Within the CPU HP path there are these arch specific intricacies. > > As mentioned in patch 2, fairly sure this needs to go before that > patch. 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late CPU initialisation I think. Miguel > > Jonathan > >> >> Signed-off-by: Miguel Luis <miguel.luis@oracle.com> >> --- >> drivers/acpi/acpi_processor.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c >> index 9ea58b61d741..c6e2f64a056b 100644 >> --- a/drivers/acpi/acpi_processor.c >> +++ b/drivers/acpi/acpi_processor.c >> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) >> pr_info("CPU%d has been hot-added\n", pr->id); >> pr->flags.need_hotplug_init = 1; >> } >> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) >> +{ >> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); >> +} >> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) >> +{ >> + acpi_unmap_cpu(pr->id); >> +} >> #else >> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {} >> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) >> +{ >> + return 0; >> +} >> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {} >> #endif /* CONFIG_ACPI_HOTPLUG_CPU */ >> >> /* Enumerate extra CPUs */ >> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr) >> cpu_maps_update_begin(); >> cpus_write_lock(); >> >> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); >> + ret = acpi_processor_hotplug_map_cpu(pr); >> if (ret) >> goto out; >> >> ret = arch_register_cpu(pr->id); >> if (ret) { >> - acpi_unmap_cpu(pr->id); >> + acpi_processor_hotplug_unmap_cpu(pr); >> goto out; >> } >> >
On Wed, 10 Apr 2024 18:29:34 +0000 Miguel Luis <miguel.luis@oracle.com> wrote: > > On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > > On Tue, 9 Apr 2024 15:05:32 +0000 > > Miguel Luis <miguel.luis@oracle.com> wrote: > > > >> mapping and unmaping a cpu at the stage of extra cpu enumeration is > >> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's > >> isolate that functionality from architecture independent one. > > > > Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu() > > to make the arch specific nature of that call more obvious? > > Not sure about the pattern to use here but that seems fine to me. Current usage > is architectures export acpi_map_cpu from the acpi interface and do their > thing. > > Question is what to do when there’s a use-case which dismisses acpi_map_cpu and > it gets called on the code path? I'm not sure what you mean by dismisses? Is missing perhaps? If that is what you mean, I think it's a mistake to allow that code to be called from a path that isn't dependent on CONFIG_ACPI_HOTPLUG_CPU. It makes no sense to do so and stubbing it out to give the impression that the calling it does make sense (when looking at the caller) is misleading. Jonathan > > 1) export it and do nothing - it would be creating unnecessary dependency. > > 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped > into CONFIG_ACPI_HOTPLUG_CPU. > > Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely > used for CPU HP and the same applies to acpi_unmap_cpu. > > > I think that has caused more confusion in the discussion than > > whether it is hotplug specific or not. > > Indeed. Within the CPU HP path there are these arch specific intricacies. > > > > > As mentioned in patch 2, fairly sure this needs to go before that > > patch. > > 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late > CPU initialisation I think. > > Miguel > > > > > Jonathan > > > >> > >> Signed-off-by: Miguel Luis <miguel.luis@oracle.com> > >> --- > >> drivers/acpi/acpi_processor.c | 17 +++++++++++++++-- > >> 1 file changed, 15 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > >> index 9ea58b61d741..c6e2f64a056b 100644 > >> --- a/drivers/acpi/acpi_processor.c > >> +++ b/drivers/acpi/acpi_processor.c > >> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) > >> pr_info("CPU%d has been hot-added\n", pr->id); > >> pr->flags.need_hotplug_init = 1; > >> } > >> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) > >> +{ > >> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); > >> +} > >> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) > >> +{ > >> + acpi_unmap_cpu(pr->id); > >> +} > >> #else > >> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {} > >> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) > >> +{ > >> + return 0; > >> +} > >> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {} > >> #endif /* CONFIG_ACPI_HOTPLUG_CPU */ > >> > >> /* Enumerate extra CPUs */ > >> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr) > >> cpu_maps_update_begin(); > >> cpus_write_lock(); > >> > >> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); > >> + ret = acpi_processor_hotplug_map_cpu(pr); > >> if (ret) > >> goto out; > >> > >> ret = arch_register_cpu(pr->id); > >> if (ret) { > >> - acpi_unmap_cpu(pr->id); > >> + acpi_processor_hotplug_unmap_cpu(pr); > >> goto out; > >> } > >> > > >
> On 10 Apr 2024, at 19:44, Jonathan Cameron <jonathan.cameron@huawei.com> wrote: > > On Wed, 10 Apr 2024 18:29:34 +0000 > Miguel Luis <miguel.luis@oracle.com> wrote: > >>> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: >>> >>> On Tue, 9 Apr 2024 15:05:32 +0000 >>> Miguel Luis <miguel.luis@oracle.com> wrote: >>> >>>> mapping and unmaping a cpu at the stage of extra cpu enumeration is >>>> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's >>>> isolate that functionality from architecture independent one. >>> >>> Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu() >>> to make the arch specific nature of that call more obvious? >> >> Not sure about the pattern to use here but that seems fine to me. Current usage >> is architectures export acpi_map_cpu from the acpi interface and do their >> thing. >> >> Question is what to do when there’s a use-case which dismisses acpi_map_cpu and >> it gets called on the code path? > > I'm not sure what you mean by dismisses? > I mean when acpi_map_cpu is not needed. > Is missing perhaps? Yes. > If that is what you mean, I think it's a mistake to allow > that code to be called from a path that isn't dependent on > CONFIG_ACPI_HOTPLUG_CPU. > It makes no sense to do so and stubbing it out to give > the impression that the calling it does make sense (when looking at the caller) > is misleading. OK, that would be what not to do. acpi_processor_enumerate_extra could deal with make_present and make_enabled while a stub would still be needed for make_present since it depends on CONFIG_ACPI_HOTPLUG_CPU? Miguel > > Jonathan > > >> >> 1) export it and do nothing - it would be creating unnecessary dependency. >> >> 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped >> into CONFIG_ACPI_HOTPLUG_CPU. >> >> Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely >> used for CPU HP and the same applies to acpi_unmap_cpu. >> >>> I think that has caused more confusion in the discussion than >>> whether it is hotplug specific or not. >> >> Indeed. Within the CPU HP path there are these arch specific intricacies. >> >>> >>> As mentioned in patch 2, fairly sure this needs to go before that >>> patch. >> >> 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late >> CPU initialisation I think. >> >> Miguel >> >>> >>> Jonathan >>> >>>> >>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com> >>>> --- >>>> drivers/acpi/acpi_processor.c | 17 +++++++++++++++-- >>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c >>>> index 9ea58b61d741..c6e2f64a056b 100644 >>>> --- a/drivers/acpi/acpi_processor.c >>>> +++ b/drivers/acpi/acpi_processor.c >>>> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) >>>> pr_info("CPU%d has been hot-added\n", pr->id); >>>> pr->flags.need_hotplug_init = 1; >>>> } >>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) >>>> +{ >>>> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); >>>> +} >>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) >>>> +{ >>>> + acpi_unmap_cpu(pr->id); >>>> +} >>>> #else >>>> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {} >>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) >>>> +{ >>>> + return 0; >>>> +} >>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {} >>>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */ >>>> >>>> /* Enumerate extra CPUs */ >>>> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr) >>>> cpu_maps_update_begin(); >>>> cpus_write_lock(); >>>> >>>> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); >>>> + ret = acpi_processor_hotplug_map_cpu(pr); >>>> if (ret) >>>> goto out; >>>> >>>> ret = arch_register_cpu(pr->id); >>>> if (ret) { >>>> - acpi_unmap_cpu(pr->id); >>>> + acpi_processor_hotplug_unmap_cpu(pr); >>>> goto out; >>>> } >>>> >>> >> >
On Thu, 11 Apr 2024 10:52:13 +0000 Miguel Luis <miguel.luis@oracle.com> wrote: > > On 10 Apr 2024, at 19:44, Jonathan Cameron <jonathan.cameron@huawei.com> wrote: > > > > On Wed, 10 Apr 2024 18:29:34 +0000 > > Miguel Luis <miguel.luis@oracle.com> wrote: > > > >>> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > >>> > >>> On Tue, 9 Apr 2024 15:05:32 +0000 > >>> Miguel Luis <miguel.luis@oracle.com> wrote: > >>> > >>>> mapping and unmaping a cpu at the stage of extra cpu enumeration is > >>>> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's > >>>> isolate that functionality from architecture independent one. > >>> > >>> Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu() > >>> to make the arch specific nature of that call more obvious? > >> > >> Not sure about the pattern to use here but that seems fine to me. Current usage > >> is architectures export acpi_map_cpu from the acpi interface and do their > >> thing. > >> > >> Question is what to do when there’s a use-case which dismisses acpi_map_cpu and > >> it gets called on the code path? > > > > I'm not sure what you mean by dismisses? > > > > I mean when acpi_map_cpu is not needed. > > > Is missing perhaps? > > Yes. > > > If that is what you mean, I think it's a mistake to allow > > that code to be called from a path that isn't dependent on > > CONFIG_ACPI_HOTPLUG_CPU. > > It makes no sense to do so and stubbing it out to give > > the impression that the calling it does make sense (when looking at the caller) > > is misleading. > > OK, that would be what not to do. > > acpi_processor_enumerate_extra could deal with make_present and make_enabled while > a stub would still be needed for make_present since it depends on > CONFIG_ACPI_HOTPLUG_CPU? Sure, you could make it do that with a bunch of checks on the config being enabled, but currently I don't see the overlap in shared code as being sufficient for that to make sense. The discussion before was assuming that things like the acpi_map_cpu calls might do stuff that is wanted in the make_enabled() case. Given they don't do anything that we want there I don't see sharing the code as useful. I am however in favor of renaming those hotplug only calls to something more meaningful so no one 'thinks' they may be relevant in the enabling only case! Jonathan p.s. I'm smashing the outputs of the thread with Rafael into a coherent patch set at the moment, perhaps seeing that will make it clearer what is going on. I got distracted by fixing numa node handling this morning but that's now pushed out for a follow on series. > > Miguel > > > > > Jonathan > > > > > >> > >> 1) export it and do nothing - it would be creating unnecessary dependency. > >> > >> 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped > >> into CONFIG_ACPI_HOTPLUG_CPU. > >> > >> Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely > >> used for CPU HP and the same applies to acpi_unmap_cpu. > >> > >>> I think that has caused more confusion in the discussion than > >>> whether it is hotplug specific or not. > >> > >> Indeed. Within the CPU HP path there are these arch specific intricacies. > >> > >>> > >>> As mentioned in patch 2, fairly sure this needs to go before that > >>> patch. > >> > >> 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late > >> CPU initialisation I think. > >> > >> Miguel > >> > >>> > >>> Jonathan > >>> > >>>> > >>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com> > >>>> --- > >>>> drivers/acpi/acpi_processor.c | 17 +++++++++++++++-- > >>>> 1 file changed, 15 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > >>>> index 9ea58b61d741..c6e2f64a056b 100644 > >>>> --- a/drivers/acpi/acpi_processor.c > >>>> +++ b/drivers/acpi/acpi_processor.c > >>>> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) > >>>> pr_info("CPU%d has been hot-added\n", pr->id); > >>>> pr->flags.need_hotplug_init = 1; > >>>> } > >>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) > >>>> +{ > >>>> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); > >>>> +} > >>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) > >>>> +{ > >>>> + acpi_unmap_cpu(pr->id); > >>>> +} > >>>> #else > >>>> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {} > >>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) > >>>> +{ > >>>> + return 0; > >>>> +} > >>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {} > >>>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */ > >>>> > >>>> /* Enumerate extra CPUs */ > >>>> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr) > >>>> cpu_maps_update_begin(); > >>>> cpus_write_lock(); > >>>> > >>>> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); > >>>> + ret = acpi_processor_hotplug_map_cpu(pr); > >>>> if (ret) > >>>> goto out; > >>>> > >>>> ret = arch_register_cpu(pr->id); > >>>> if (ret) { > >>>> - acpi_unmap_cpu(pr->id); > >>>> + acpi_processor_hotplug_unmap_cpu(pr); > >>>> goto out; > >>>> } > >>>> > >>> > >> > > >
> On 11 Apr 2024, at 13:57, Jonathan Cameron <jonathan.cameron@huawei.com> wrote: > > On Thu, 11 Apr 2024 10:52:13 +0000 > Miguel Luis <miguel.luis@oracle.com> wrote: > >>> On 10 Apr 2024, at 19:44, Jonathan Cameron <jonathan.cameron@huawei.com> wrote: >>> >>> On Wed, 10 Apr 2024 18:29:34 +0000 >>> Miguel Luis <miguel.luis@oracle.com> wrote: >>> >>>>> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: >>>>> >>>>> On Tue, 9 Apr 2024 15:05:32 +0000 >>>>> Miguel Luis <miguel.luis@oracle.com> wrote: >>>>> >>>>>> mapping and unmaping a cpu at the stage of extra cpu enumeration is >>>>>> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's >>>>>> isolate that functionality from architecture independent one. >>>>> >>>>> Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu() >>>>> to make the arch specific nature of that call more obvious? >>>> >>>> Not sure about the pattern to use here but that seems fine to me. Current usage >>>> is architectures export acpi_map_cpu from the acpi interface and do their >>>> thing. >>>> >>>> Question is what to do when there’s a use-case which dismisses acpi_map_cpu and >>>> it gets called on the code path? >>> >>> I'm not sure what you mean by dismisses? >>> >> >> I mean when acpi_map_cpu is not needed. >> >>> Is missing perhaps? >> >> Yes. >> >>> If that is what you mean, I think it's a mistake to allow >>> that code to be called from a path that isn't dependent on >>> CONFIG_ACPI_HOTPLUG_CPU. >>> It makes no sense to do so and stubbing it out to give >>> the impression that the calling it does make sense (when looking at the caller) >>> is misleading. >> >> OK, that would be what not to do. >> >> acpi_processor_enumerate_extra could deal with make_present and make_enabled while >> a stub would still be needed for make_present since it depends on >> CONFIG_ACPI_HOTPLUG_CPU? > > Sure, you could make it do that with a bunch of checks on the > config being enabled, but currently I don't see the overlap in > shared code as being sufficient for that to make sense. > > The discussion before was assuming that things like the acpi_map_cpu > calls might do stuff that is wanted in the make_enabled() case. > > Given they don't do anything that we want there I don't see sharing > the code as useful. > > I am however in favor of renaming those hotplug only calls to something > more meaningful so no one 'thinks' they may be relevant in the > enabling only case! > > Jonathan > > p.s. I'm smashing the outputs of the thread with Rafael into a coherent > patch set at the moment, perhaps seeing that will make it clearer what > is going on. I got distracted by fixing numa node handling this morning > but that's now pushed out for a follow on series. > Thanks! Looking forward to see v5. Miguel > >> >> Miguel >> >>> >>> Jonathan >>> >>> >>>> >>>> 1) export it and do nothing - it would be creating unnecessary dependency. >>>> >>>> 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped >>>> into CONFIG_ACPI_HOTPLUG_CPU. >>>> >>>> Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely >>>> used for CPU HP and the same applies to acpi_unmap_cpu. >>>> >>>>> I think that has caused more confusion in the discussion than >>>>> whether it is hotplug specific or not. >>>> >>>> Indeed. Within the CPU HP path there are these arch specific intricacies. >>>> >>>>> >>>>> As mentioned in patch 2, fairly sure this needs to go before that >>>>> patch. >>>> >>>> 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late >>>> CPU initialisation I think. >>>> >>>> Miguel >>>> >>>>> >>>>> Jonathan >>>>> >>>>>> >>>>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com> >>>>>> --- >>>>>> drivers/acpi/acpi_processor.c | 17 +++++++++++++++-- >>>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c >>>>>> index 9ea58b61d741..c6e2f64a056b 100644 >>>>>> --- a/drivers/acpi/acpi_processor.c >>>>>> +++ b/drivers/acpi/acpi_processor.c >>>>>> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) >>>>>> pr_info("CPU%d has been hot-added\n", pr->id); >>>>>> pr->flags.need_hotplug_init = 1; >>>>>> } >>>>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) >>>>>> +{ >>>>>> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); >>>>>> +} >>>>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) >>>>>> +{ >>>>>> + acpi_unmap_cpu(pr->id); >>>>>> +} >>>>>> #else >>>>>> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {} >>>>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) >>>>>> +{ >>>>>> + return 0; >>>>>> +} >>>>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {} >>>>>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */ >>>>>> >>>>>> /* Enumerate extra CPUs */ >>>>>> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr) >>>>>> cpu_maps_update_begin(); >>>>>> cpus_write_lock(); >>>>>> >>>>>> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); >>>>>> + ret = acpi_processor_hotplug_map_cpu(pr); >>>>>> if (ret) >>>>>> goto out; >>>>>> >>>>>> ret = arch_register_cpu(pr->id); >>>>>> if (ret) { >>>>>> - acpi_unmap_cpu(pr->id); >>>>>> + acpi_processor_hotplug_unmap_cpu(pr); >>>>>> goto out; >>>>>> }
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 9ea58b61d741..c6e2f64a056b 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) pr_info("CPU%d has been hot-added\n", pr->id); pr->flags.need_hotplug_init = 1; } +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) +{ + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); +} +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) +{ + acpi_unmap_cpu(pr->id); +} #else static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {} +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr) +{ + return 0; +} +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {} #endif /* CONFIG_ACPI_HOTPLUG_CPU */ /* Enumerate extra CPUs */ @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr) cpu_maps_update_begin(); cpus_write_lock(); - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id); + ret = acpi_processor_hotplug_map_cpu(pr); if (ret) goto out; ret = arch_register_cpu(pr->id); if (ret) { - acpi_unmap_cpu(pr->id); + acpi_processor_hotplug_unmap_cpu(pr); goto out; }
mapping and unmaping a cpu at the stage of extra cpu enumeration is architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's isolate that functionality from architecture independent one. Signed-off-by: Miguel Luis <miguel.luis@oracle.com> --- drivers/acpi/acpi_processor.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)