From: Miguel Luis <miguel.luis@oracle.com>
To: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
Len Brown <lenb@kernel.org>,
"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"rmk+kernel@armlinux.org.uk" <rmk+kernel@armlinux.org.uk>
Subject: Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU
Date: Thu, 11 Apr 2024 15:55:57 +0000 [thread overview]
Message-ID: <B5D6188D-97F8-4EF4-9EC3-9D4B8AC57830@oracle.com> (raw)
In-Reply-To: <20240411145720.000056f6@Huawei.com>
> 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;
>>>>>> }
next prev parent reply other threads:[~2024-04-11 15:56 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-09 15:05 [RFC PATCH 0/4] ACPI: processor: refactor acpi_processor_{get_info|remove} Miguel Luis
2024-04-09 15:05 ` [RFC PATCH 1/4] ACPI: processor: refactor acpi_processor_get_info: evaluation of processor declaration Miguel Luis
2024-04-10 13:13 ` Jonathan Cameron
2024-04-10 15:35 ` Miguel Luis
2024-04-09 15:05 ` [RFC PATCH 2/4] ACPI: processor: refactor acpi_processor_get_info: isolate cpu hotpug init delay Miguel Luis
2024-04-10 13:20 ` Jonathan Cameron
2024-04-10 17:20 ` Miguel Luis
2024-04-10 19:40 ` Jonathan Cameron
2024-04-09 15:05 ` [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU Miguel Luis
2024-04-10 13:23 ` Jonathan Cameron
2024-04-10 18:29 ` Miguel Luis
2024-04-10 19:44 ` Jonathan Cameron
2024-04-11 10:52 ` Miguel Luis
2024-04-11 13:57 ` Jonathan Cameron
2024-04-11 15:55 ` Miguel Luis [this message]
2024-04-09 15:05 ` [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu " Miguel Luis
2024-04-10 13:31 ` Jonathan Cameron
2024-04-11 11:02 ` Miguel Luis
2024-04-11 14:02 ` Jonathan Cameron
2024-04-10 13:35 ` [RFC PATCH 0/4] ACPI: processor: refactor acpi_processor_{get_info|remove} Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B5D6188D-97F8-4EF4-9EC3-9D4B8AC57830@oracle.com \
--to=miguel.luis@oracle.com \
--cc=jonathan.cameron@huawei.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rmk+kernel@armlinux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).