| Message ID | 20210422192442.706906-1-sathyanarayanan.kuppuswamy@linux.intel.com |
|---|---|
| Headers | show |
| Series | Add multiprocessor wake-up support | expand |
On 4/22/21 12:37 PM, Dave Hansen wrote: > On 4/22/21 12:24 PM, Kuppuswamy Sathyanarayanan wrote: >> ACPICA commit f1ee04207a212f6c519441e7e25397649ebc4cea >> >> Add Multiprocessor Wakeup Mailbox Structure definition. It is useful >> in parsing MADT Wake table. >> >> Link: https://github.com/acpica/acpica/commit/f1ee0420 >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> Signed-off-by: Bob Moore <robert.moore@intel.com> >> Signed-off-by: Erik Kaneda <erik.kaneda@intel.com> > > This SoB chain doesn't look right. This is what it would have been if > You sent it to Bob who sent it to Erik, who submitted it. Internally, its submitted to Bob and Erik for ACPICA merge. I think Sign-off is added to track it. >
> -----Original Message----- > From: Borislav Petkov <bp@alien8.de> > Sent: Thursday, April 22, 2021 12:56 PM > To: Kuppuswamy, Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> > Cc: Hansen, Dave <dave.hansen@intel.com>; Rafael J Wysocki > <rjw@rjwysocki.net>; Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar > <mingo@redhat.com>; H . Peter Anvin <hpa@zytor.com>; Peter Zijlstra > <peterz@infradead.org>; Len Brown <lenb@kernel.org>; Moore, Robert > <robert.moore@intel.com>; Kaneda, Erik <erik.kaneda@intel.com>; linux- > acpi@vger.kernel.org; devel@acpica.org; linux-kernel@vger.kernel.org; > x86@kernel.org > Subject: Re: [PATCH 1/3] ACPICA: ACPI 6.4: MADT: add Multiprocessor > Wakeup Mailbox Structure > > On Thu, Apr 22, 2021 at 12:46:13PM -0700, Kuppuswamy, Sathyanarayanan > wrote: > > > This SoB chain doesn't look right. This is what it would have been if > > > You sent it to Bob who sent it to Erik, who submitted it. > > Internally, its submitted to Bob and Erik for ACPICA merge. > > I think Sign-off is added to track it. > > This is not how this works - when Erik/Bob merge it, *then* they add > their SOB. Right now it should have only your SOB. Sorry about that. The script to format the ACPICA upstream to Linux ACPICA automatically adds signed off-by tags from me and Bob to the patch. This would work if we go through the normal process of running the Linux script after we do an ACPICA release. We decided to submit this earlier to meet Sathya's deadline. Sathya, Please drop these lines from this patch and the SVKL patch. Erik > > Documentation/process/submitting-patches.rst, section "Sign your work - > the Developer's Certificate of Origin" > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On 4/22/21 4:04 PM, Thomas Gleixner wrote: > Kuppuswamy! > > On Thu, Apr 22 2021 at 12:24, Kuppuswamy Sathyanarayanan wrote: >> +static int acpi_wakeup_cpu(int apicid, unsigned long start_ip) >> +{ >> + acpi_mp_wake_mailbox_init(); >> + >> + if (!acpi_mp_wake_mailbox) >> + return -EINVAL; >> + >> + WRITE_ONCE(acpi_mp_wake_mailbox->apic_id, apicid); >> + WRITE_ONCE(acpi_mp_wake_mailbox->wakeup_vector, start_ip); >> + WRITE_ONCE(acpi_mp_wake_mailbox->command, ACPI_MP_WAKE_COMMAND_WAKEUP); > > What's the point of using WRITE_ONCE() here? Where is the required > READ_ONCE() counterpart and the required documentation in form of a > comment? mailbox memory is shared between firmware and OS. We use WRITE_ONCE to notify compiler about it, and also to preserve the order of writes to these addresses. As per MADT Wake protocol, once OS update the mailbox command address with ACPI_MP_*_WAKEUP command, firmware will be bring the AP out sleep state and trigger the boot process. Since its a one way communication, we don't need to read the mailbox address again. So there is no corresponding READ_ONCE() call. I will add some comments about it in next version. > >> +static int __init acpi_parse_mp_wake(union acpi_subtable_headers *header, >> + const unsigned long end) >> +{ > ... >> + acpi_wake_cpu_handler_update(acpi_wakeup_cpu); > ... > >> +++ b/arch/x86/kernel/apic/probe_32.c >> @@ -207,3 +207,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id) >> } >> return 0; >> } >> + >> +void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler) >> +{ >> + struct apic **drv; >> + >> + for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) >> + (*drv)->wakeup_secondary_cpu = handler; >> +} >> diff --git a/arch/x86/kernel/apic/probe_64.c b/arch/x86/kernel/apic/probe_64.c >> index c46720f185c0..986dbb68d3c4 100644 >> --- a/arch/x86/kernel/apic/probe_64.c >> +++ b/arch/x86/kernel/apic/probe_64.c >> @@ -50,3 +50,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id) >> } >> return 0; >> } >> + >> +void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler) >> +{ >> + struct apic **drv; >> + >> + for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) >> + (*drv)->wakeup_secondary_cpu = handler; >> +} > > What's the reason for having two verbatim copies of the same function > which has no dependency on CONFIG_*_32/64 at all? Initially I thought all ACPI related functions are grouped in probe_*.c. After some review, now I know its incorrect. I will move the function definition to apic.c > > Thanks, > > tglx >