[PATCH v2 00/13] The panic notifiers refactor strikes back - fixes/clean-ups

[PATCH v2 00/13] The panic notifiers refactor strikes back - fixes/clean-ups

[Guilherme G. Piccoli]

Hi folks, this the second iteration of the panic notifiers refactor work,
but limited to the fixes/clean-ups in the first moment. The (full) V1 is
available at:
https://lore.kernel.org/lkml/20220427224924.592546-1-gpiccoli@igalia.com/

The idea of splitting the series is that, originally we had a bunch of fixes
followed by the notifiers refactor, but this second part (the effective
refactor) is a bit "polemic", with reviews having antagonistic goals and some
complexities  - it might be hard to achieve consensus.
For the curious, here is a good summary of the conflicting views and some
strategies we might take in the refactor V2:
https://lore.kernel.org/lkml/0d084eed-4781-c815-29c7-ac62c498e216@igalia.com/

So splitting and sending only the simple fixes/clean-ups in a first moment
makes sense, this way we don't prevent them to be discussed/merged/reworked
while the more complex part is subject to scrutiny in a different (future)
email thread.


I've tried to test this series building for all affected architecture/drivers
and also through some boot/runtime tests; below the test "matrix" used:

Build tests (using cross-compilers): alpha, arm, arm64, parisc, um, x86_64.
Boot/Runtime tests: x86_64 (Hyper-V and QEMU guests).

Here is the link with the .config files used:
https://people.igalia.com/gpiccoli/panic_notifiers_configs/5.19-rc7/
(tried my best to build all the affected code).


The series is based on 5.19-rc7; I'd like to ask that, if possible, maintainers
take the patches here in their trees, since there is no need to merge the series
as whole, patches are independent from each other.

Regarding the CC strategy, I've tried to reduce a bit the list of CCed emails,
given that it was huge in the first iteration. Hopefully I didn't forget
anybody interested in the topic (my apologies if so).

As usual, reviews / comments are always welcome, thanks in advance for them!
Cheers,


Guilherme




Guilherme G. Piccoli (13):
  ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths
  notifier: Add panic notifiers info and purge trailing whitespaces
  firmware: google: Test spinlock on panic path to avoid lockups
  soc: bcm: brcmstb: Document panic notifier action and remove useless header
  alpha: Clean-up the panic notifier code
  um: Improve panic notifiers consistency and ordering
  parisc: Replace regular spinlock with spin_trylock on panic path
  tracing: Improve panic/die notifiers
  notifier: Show function names on notifier routines if DEBUG_NOTIFIERS is set
  EDAC/altera: Skip the panic notifier if kdump is loaded
  video/hyperv_fb: Avoid taking busy spinlock on panic path
  drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers
  panic: Fixes the panic_print NMI backtrace setting

 arch/alpha/kernel/setup.c           |  36 ++++-----
 arch/arm/kernel/machine_kexec.c     |   2 +
 arch/arm/kernel/smp.c               |   5 +-
 arch/parisc/include/asm/pdc.h       |   1 +
 arch/parisc/kernel/firmware.c       |  27 ++++++-
 arch/um/drivers/mconsole_kern.c     |   7 +-
 arch/um/kernel/um_arch.c            |   8 +-
 drivers/edac/altera_edac.c          |  16 +++-
 drivers/firmware/google/gsmi.c      |   8 ++
 drivers/hv/ring_buffer.c            |  16 ++++
 drivers/hv/vmbus_drv.c              | 109 +++++++++++++++++-----------
 drivers/parisc/power.c              |  17 +++--
 drivers/soc/bcm/brcmstb/pm/pm-arm.c |  16 +++-
 drivers/video/fbdev/hyperv_fb.c     |  16 +++-
 include/linux/hyperv.h              |   2 +
 include/linux/notifier.h            |   8 +-
 kernel/notifier.c                   |  22 ++++--
 kernel/panic.c                      |  47 +++++++-----
 kernel/trace/trace.c                |  55 +++++++-------
 19 files changed, 268 insertions(+), 150 deletions(-)

-- 
2.37.1

[PATCH v2 03/13] firmware: google: Test spinlock on panic path to avoid lockups

[Guilherme G. Piccoli]

Currently the gsmi driver registers a panic notifier as well as
reboot and die notifiers. The callbacks registered are called in
atomic and very limited context - for instance, panic disables
preemption and local IRQs, also all secondary CPUs (not executing
the panic path) are shutdown.

With that said, taking a spinlock in this scenario is a dangerous
invitation for lockup scenarios. So, fix that by checking if the
spinlock is free to acquire in the panic notifier callback - if not,
bail-out and avoid a potential hang.

Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: David Gow <davidgow@google.com>
Cc: Evan Green <evgreen@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

---

V2:
- do not use spin_trylock anymore, to avoid messing with
non-panic paths; now we just check the spinlock state in
the panic notifier before taking it. Thanks Evan for the
review/idea!

 drivers/firmware/google/gsmi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index adaa492c3d2d..3ef5f3c0b4e4 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -681,6 +681,14 @@ static struct notifier_block gsmi_die_notifier = {
 static int gsmi_panic_callback(struct notifier_block *nb,
 			       unsigned long reason, void *arg)
 {
+	/*
+	 * Perform the lock check before effectively trying
+	 * to acquire it on gsmi_shutdown_reason() to avoid
+	 * potential lockups in atomic context.
+	 */
+	if (spin_is_locked(&gsmi_dev.lock))
+		return NOTIFY_DONE;
+
 	gsmi_shutdown_reason(GSMI_SHUTDOWN_PANIC);
 	return NOTIFY_DONE;
 }
-- 
2.37.1

Re: [PATCH v2 03/13] firmware: google: Test spinlock on panic path to avoid lockups

[Guilherme G. Piccoli]

On 19/07/2022 16:53, Guilherme G. Piccoli wrote:
> Currently the gsmi driver registers a panic notifier as well as
> reboot and die notifiers. The callbacks registered are called in
> atomic and very limited context - for instance, panic disables
> preemption and local IRQs, also all secondary CPUs (not executing
> the panic path) are shutdown.
> 
> With that said, taking a spinlock in this scenario is a dangerous
> invitation for lockup scenarios. So, fix that by checking if the
> spinlock is free to acquire in the panic notifier callback - if not,
> bail-out and avoid a potential hang.
> 
> Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: David Gow <davidgow@google.com>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> ---
> 
> V2:
> - do not use spin_trylock anymore, to avoid messing with
> non-panic paths; now we just check the spinlock state in
> the panic notifier before taking it. Thanks Evan for the
> review/idea!
> 
>  drivers/firmware/google/gsmi.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> [...]

Hi Evan, do you think this one is good now, based on your previous review?

Appreciate any feedback!
Cheers,


Guilherme

Re: [PATCH v2 03/13] firmware: google: Test spinlock on panic path to avoid lockups

[Evan Green]

On Tue, Jul 19, 2022 at 12:55 PM Guilherme G. Piccoli
<gpiccoli@igalia.com> wrote:
>
> Currently the gsmi driver registers a panic notifier as well as
> reboot and die notifiers. The callbacks registered are called in
> atomic and very limited context - for instance, panic disables
> preemption and local IRQs, also all secondary CPUs (not executing
> the panic path) are shutdown.
>
> With that said, taking a spinlock in this scenario is a dangerous
> invitation for lockup scenarios. So, fix that by checking if the
> spinlock is free to acquire in the panic notifier callback - if not,
> bail-out and avoid a potential hang.
>
> Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: David Gow <davidgow@google.com>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH v2 03/13] firmware: google: Test spinlock on panic path to avoid lockups

[Guilherme G. Piccoli]

On 08/08/2022 02:07, Evan Green wrote:
> On Tue, Jul 19, 2022 at 12:55 PM Guilherme G. Piccoli
> <gpiccoli@igalia.com> wrote:
>>
>> Currently the gsmi driver registers a panic notifier as well as
>> reboot and die notifiers. The callbacks registered are called in
>> atomic and very limited context - for instance, panic disables
>> preemption and local IRQs, also all secondary CPUs (not executing
>> the panic path) are shutdown.
>>
>> With that said, taking a spinlock in this scenario is a dangerous
>> invitation for lockup scenarios. So, fix that by checking if the
>> spinlock is free to acquire in the panic notifier callback - if not,
>> bail-out and avoid a potential hang.
>>
>> Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
>> Cc: Ard Biesheuvel <ardb@kernel.org>
>> Cc: David Gow <davidgow@google.com>
>> Cc: Evan Green <evgreen@chromium.org>
>> Cc: Julius Werner <jwerner@chromium.org>
>> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> Reviewed-by: Evan Green <evgreen@chromium.org>

Thanks a bunch Evan!

Ard / Greg, do you think you could get this patch through your -next (or
-fixes) trees? Not sure which tree is the most common for picking GSMI
stuff.

I'm trying to get these fixes merged individually in their trees to not
stall the whole series and increase the burden of re-submitting.
Thanks in advance,


Guilherme

Re: [PATCH v2 03/13] firmware: google: Test spinlock on panic path to avoid lockups

[Greg Kroah-Hartman]

On Mon, Aug 08, 2022 at 12:14:30PM -0300, Guilherme G. Piccoli wrote:
> On 08/08/2022 02:07, Evan Green wrote:
> > On Tue, Jul 19, 2022 at 12:55 PM Guilherme G. Piccoli
> > <gpiccoli@igalia.com> wrote:
> >>
> >> Currently the gsmi driver registers a panic notifier as well as
> >> reboot and die notifiers. The callbacks registered are called in
> >> atomic and very limited context - for instance, panic disables
> >> preemption and local IRQs, also all secondary CPUs (not executing
> >> the panic path) are shutdown.
> >>
> >> With that said, taking a spinlock in this scenario is a dangerous
> >> invitation for lockup scenarios. So, fix that by checking if the
> >> spinlock is free to acquire in the panic notifier callback - if not,
> >> bail-out and avoid a potential hang.
> >>
> >> Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
> >> Cc: Ard Biesheuvel <ardb@kernel.org>
> >> Cc: David Gow <davidgow@google.com>
> >> Cc: Evan Green <evgreen@chromium.org>
> >> Cc: Julius Werner <jwerner@chromium.org>
> >> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> > 
> > Reviewed-by: Evan Green <evgreen@chromium.org>
> 
> Thanks a bunch Evan!
> 
> Ard / Greg, do you think you could get this patch through your -next (or
> -fixes) trees? Not sure which tree is the most common for picking GSMI
> stuff.

Picking out an individual patch from a series with as many responses and
threads like this one is quite difficult.

Just resend this as a stand-alone patch if you want it applied
stand-alone as our tools want to apply a whole patch series at once.

> I'm trying to get these fixes merged individually in their trees to not
> stall the whole series and increase the burden of re-submitting.

The burden is on the submitter, not the maintainer as we have more
submitters than reviewers/maintainers.

thanks,

greg k-h

Re: [PATCH v2 03/13] firmware: google: Test spinlock on panic path to avoid lockups

[Guilherme G. Piccoli]

On 08/08/2022 12:26, Greg Kroah-Hartman wrote:
> [...]
>>
>> Ard / Greg, do you think you could get this patch through your -next (or
>> -fixes) trees? Not sure which tree is the most common for picking GSMI
>> stuff.
> 
> Picking out an individual patch from a series with as many responses and
> threads like this one is quite difficult.
> 
> Just resend this as a stand-alone patch if you want it applied
> stand-alone as our tools want to apply a whole patch series at once.
> 
>> I'm trying to get these fixes merged individually in their trees to not
>> stall the whole series and increase the burden of re-submitting.
> 
> The burden is on the submitter, not the maintainer as we have more
> submitters than reviewers/maintainers.
> 

I understand, thanks for letting me know!

Let me clarify / ask something: this series, for example, is composed as
a bunch of patches "centered" around the same idea, panic notifiers
improvements/fixes. But its patches belong to completely different
subsystems, like EFI/misc, architectures (alpha, parisc, arm), core
kernel code, etc.

What is the best way of getting this merged?
(a) Re-send individual patches with the respective Review/ACK tags to
the proper subsystem, or;

(b) Wait until the whole series is ACKed/Reviewed, and a single
maintainer (like you or Andrew, for example) would pick the whole series
and apply at once, even if it spans across multiple parts of the kernel?

Let me know what is the general preference of the kernel maintainers,
and I'll gladly follow that =)

Thanks,


Guilherme

Re: [PATCH v2 03/13] firmware: google: Test spinlock on panic path to avoid lockups

[Greg Kroah-Hartman]

On Mon, Aug 08, 2022 at 12:37:46PM -0300, Guilherme G. Piccoli wrote:
> Let me clarify / ask something: this series, for example, is composed as
> a bunch of patches "centered" around the same idea, panic notifiers
> improvements/fixes. But its patches belong to completely different
> subsystems, like EFI/misc, architectures (alpha, parisc, arm), core
> kernel code, etc.
> 
> What is the best way of getting this merged?
> (a) Re-send individual patches with the respective Review/ACK tags to
> the proper subsystem, or;

Yes.

> (b) Wait until the whole series is ACKed/Reviewed, and a single
> maintainer (like you or Andrew, for example) would pick the whole series
> and apply at once, even if it spans across multiple parts of the kernel?

No, only do this after a kernel release cycle happens and there are
straggler patches that did not get picked up by the relevant subsystem
maintainers.

thanks,

greg k-h