Re: [PATCH v4] notifiers: Add tracepoints to the notifiers infrastructure

[Guilherme G. Piccoli]

On 14/03/2023 18:18, Andrew Morton wrote:
> On Tue, 14 Mar 2023 18:08:37 -0300 "Guilherme G. Piccoli" 
>  wrote:
> [...]
>> Hi Andrew, thanks!
>>
>> Do you want me to re-submit? I see some emails of the patch getting
>> added to "mm-nonmm-unstable" (and also a checkpatch fixes you added on
>> top of that). Lemme know how should I proceed.
> 
> I changed it, thanks.

Tnx a bunch =)

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4] notifiers: Add tracepoints to the notifiers infrastructure

[Guilherme G. Piccoli]

On 14/03/2023 17:50, Andrew Morton wrote:
> On Tue, 14 Mar 2023 17:00:58 -0300 "Guilherme G. Piccoli" 
>  wrote:
> 
>>  include/trace/events/notifiers.h | 69 
>>  kernel/notifier.c|  6 +++
> 
> Perhaps the filenames should match, which means "notifier.h".

Hi Andrew, thanks!

Do you want me to re-submit? I see some emails of the patch getting
added to "mm-nonmm-unstable" (and also a checkpatch fixes you added on
top of that). Lemme know how should I proceed.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v4] notifiers: Add tracepoints to the notifiers infrastructure

[Guilherme G. Piccoli]

Currently there is no way to show the callback names for registered,
unregistered or executed notifiers. This is very useful for debug
purposes, hence add this functionality here in the form of notifiers'
tracepoints, one per operation.

Cc: Arjan van de Ven 
Cc: Michael Kelley 
Cc: Steven Rostedt 
Cc: Xiaoming Ni 
Signed-off-by: Guilherme G. Piccoli 
---

V4:
- Rebased and tested on top of v6.3-rc2.
- Sending as standalone patch, not part of any series.

V3:
- Yet another major change - thanks to Arjan's great suggestion,
refactored the code to make use of tracepoints instead of guarding
the output under a Kconfig debug setting.

V2:
- Major improvement thanks to the great idea from Xiaoming - changed
all the ksym wheel reinvention to printk %ps modifier;

- Instead of ifdefs, using IS_ENABLED() - thanks Steven.

- Removed an unlikely() hint on debug path.

V3: https://lore.kernel.org/lkml/20220819221731.480795-8-gpicc...@igalia.com/
V2: https://lore.kernel.org/lkml/20220719195325.402745-10-gpicc...@igalia.com/

Thanks in advance for reviews!
Cheers,

Guilherme


 include/trace/events/notifiers.h | 69 
 kernel/notifier.c|  6 +++
 2 files changed, 75 insertions(+)
 create mode 100644 include/trace/events/notifiers.h

diff --git a/include/trace/events/notifiers.h b/include/trace/events/notifiers.h
new file mode 100644
index ..e8f30631aef5
--- /dev/null
+++ b/include/trace/events/notifiers.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM notifiers
+
+#if !defined(_TRACE_NOTIFIERS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_NOTIFIERS_H
+
+#include 
+
+DECLARE_EVENT_CLASS(notifiers_info,
+
+   TP_PROTO(void *cb),
+
+   TP_ARGS(cb),
+
+   TP_STRUCT__entry(
+   __field(void *, cb)
+   ),
+
+   TP_fast_assign(
+   __entry->cb = cb;
+   ),
+
+   TP_printk("%ps", __entry->cb)
+);
+
+/*
+ * notifiers_register - called upon notifier callback registration
+ *
+ * @cb:callback pointer
+ *
+ */
+DEFINE_EVENT(notifiers_info, notifiers_register,
+
+   TP_PROTO(void *cb),
+
+   TP_ARGS(cb)
+);
+
+/*
+ * notifiers_unregister - called upon notifier callback unregistration
+ *
+ * @cb:callback pointer
+ *
+ */
+DEFINE_EVENT(notifiers_info, notifiers_unregister,
+
+   TP_PROTO(void *cb),
+
+   TP_ARGS(cb)
+);
+
+/*
+ * notifiers_run - called upon notifier callback execution
+ *
+ * @cb:callback pointer
+ *
+ */
+DEFINE_EVENT(notifiers_info, notifiers_run,
+
+   TP_PROTO(void *cb),
+
+   TP_ARGS(cb)
+);
+
+#endif /* _TRACE_NOTIFIERS_H */
+
+/* This part must be outside protection */
+#include 
diff --git a/kernel/notifier.c b/kernel/notifier.c
index d353e4b5402d..f57e54ddbb5f 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -7,6 +7,9 @@
 #include 
 #include 
 
+#define CREATE_TRACE_POINTS
+#include 
+
 /*
  * Notifier list for kernel code which wants to be called
  * at shutdown. This is used to stop any idling DMA operations
@@ -37,6 +40,7 @@ static int notifier_chain_register(struct notifier_block **nl,
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
+   trace_notifiers_register((void*)n->notifier_call);
return 0;
 }
 
@@ -46,6 +50,7 @@ static int notifier_chain_unregister(struct notifier_block 
**nl,
while ((*nl) != NULL) {
if ((*nl) == n) {
rcu_assign_pointer(*nl, n->next);
+   trace_notifiers_unregister((void*)n->notifier_call);
return 0;
}
nl = &((*nl)->next);
@@ -84,6 +89,7 @@ static int notifier_call_chain(struct notifier_block **nl,
continue;
}
 #endif
+   trace_notifiers_run((void*)nb->notifier_call);
ret = nb->notifier_call(nb, val, v);
 
if (nr_calls)
-- 
2.39.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4] panic: Fixes the panic_print NMI backtrace setting

[Guilherme G. Piccoli]

On 26/02/2023 02:44, Andrew Morton wrote:
> On Fri, 10 Feb 2023 17:35:10 -0300 "Guilherme G. Piccoli" 
>  wrote:
> [...] 
>> Notice that while at it, I got rid of the "crash_kexec_post_notifiers"
>> local copy in panic(). This was introduced by commit b26e27ddfd2a
>> ("kexec: use core_param for crash_kexec_post_notifiers boot option"),
>> but it is not clear from comments or commit message why this local copy
>> is required.
>>
>> My understanding is that it's a mechanism to prevent some concurrency,
>> in case some other CPU modify this variable while panic() is running.
>> I find it very unlikely, hence I removed it - but if people consider
>> this copy needed, I can respin this patch and keep it, even providing a
>> comment about that, in order to be explict about its need.
> 
> Only two sites change crash_kexec_post_notifiers, in
> arch/powerpc/kernel/fadump.c and drivers/hv/hv_common.c.  Yes, it's
> very unlikely that this will be altered while panic() is running and
> the consequences will be slight anyway.
> 
> But formally, we shouldn't do this, especially in a -stable
> backportable patch.  So please, let's have the minimal bugfix for now
> and we can look at removing that local at a later time?
> 

Thanks Andrew, I agree with you! I just sent a V5 with the bugfix alone,
not changing this local/global variable behavior.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5] panic: Fixes the panic_print NMI backtrace setting

[Guilherme G. Piccoli]

Commit 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
panic_print")
introduced a setting for the "panic_print" kernel parameter to allow
users to request a NMI backtrace on panic. Problem is that the panic_print
handling happens after the secondary CPUs are already disabled, hence
this option ended-up being kind of a no-op - kernel skips the NMI trace
in idling CPUs, which is the case of offline CPUs.

Fix it by checking the NMI backtrace bit in the panic_print prior to
the CPU disabling function.

Fixes: 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
panic_print")
Cc: sta...@vger.kernel.org
Signed-off-by: Guilherme G. Piccoli 
---

V5:
- Kept the local version of "crash_kexec_post_notifiers", since
this is standalone fix that should be backported to stable. Hence,
it's not a good idea to mess with it in this patch (thanks Andrew!).

V4:
- Sent as standalone patch, rebased against v6.2-rc7.
- Link: 
https://lore.kernel.org/lkml/20230210203510.1734835-1-gpicc...@igalia.com/


 kernel/panic.c | 44 ++--
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 463c9295bc28..e026191a0a07 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -211,9 +211,6 @@ static void panic_print_sys_info(bool console_flush)
return;
}
 
-   if (panic_print & PANIC_PRINT_ALL_CPU_BT)
-   trigger_all_cpu_backtrace();
-
if (panic_print & PANIC_PRINT_TASK_INFO)
show_state();
 
@@ -243,6 +240,30 @@ void check_panic_on_warn(const char *origin)
  origin, limit);
 }
 
+/*
+ * Helper that triggers the NMI backtrace (if set in panic_print)
+ * and then performs the secondary CPUs shutdown - we cannot have
+ * the NMI backtrace after the CPUs are off!
+ */
+static void panic_other_cpus_shutdown(bool crash_kexec)
+{
+   if (panic_print & PANIC_PRINT_ALL_CPU_BT)
+   trigger_all_cpu_backtrace();
+
+   /*
+* Note that smp_send_stop() is the usual SMP shutdown function,
+* which unfortunately may not be hardened to work in a panic
+* situation. If we want to do crash dump after notifier calls
+* and kmsg_dump, we will need architecture dependent extra
+* bits in addition to stopping other CPUs, hence we rely on
+* crash_smp_send_stop() for that.
+*/
+   if (!crash_kexec)
+   smp_send_stop();
+   else
+   crash_smp_send_stop();
+}
+
 /**
  * panic - halt the system
  * @fmt: The text string to print
@@ -333,23 +354,10 @@ void panic(const char *fmt, ...)
 *
 * Bypass the panic_cpu check and call __crash_kexec directly.
 */
-   if (!_crash_kexec_post_notifiers) {
+   if (!_crash_kexec_post_notifiers)
__crash_kexec(NULL);
 
-   /*
-* Note smp_send_stop is the usual smp shutdown function, which
-* unfortunately means it may not be hardened to work in a
-* panic situation.
-*/
-   smp_send_stop();
-   } else {
-   /*
-* If we want to do crash dump after notifier calls and
-* kmsg_dump, we will need architecture dependent extra
-* works in addition to stopping other CPUs.
-*/
-   crash_smp_send_stop();
-   }
+   panic_other_cpus_shutdown(_crash_kexec_post_notifiers);
 
/*
 * Run any panic handlers, including those that might need to
-- 
2.39.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4] panic: Fixes the panic_print NMI backtrace setting

[Guilherme G. Piccoli]

On 14/02/2023 11:46, Petr Mladek wrote:
> [...]
>> My understanding is that it's a mechanism to prevent some concurrency,
>> in case some other CPU modify this variable while panic() is running.
>> I find it very unlikely, hence I removed it - but if people consider
>> this copy needed, I can respin this patch and keep it, even providing a
>> comment about that, in order to be explict about its need.
> 
> Yes, I think that it makes the behavior consistent even when the
> global variable manipulated in parallel.
> 
> I would personally prefer to keep the local copy. Better safe
> than sorry.
> 

Hi Petr, thanks for your review!
OK, we could keep this local copy, makes sense...even adding a comment,
to make its purpose really clear.


>> [...]
>> @@ -211,9 +211,6 @@ static void panic_print_sys_info(bool console_flush)
>>  return;
>>  }
>>  
>> -if (panic_print & PANIC_PRINT_ALL_CPU_BT)
>> -trigger_all_cpu_backtrace();
>> -
> 
> Sigh, this is yet another PANIC_PRINT_ action that need special
> timing. We should handle both the same way.
> 
> What about the following? The parameter @mask says what
> actions are allowed at the given time.
> < ..code..> 

I think your approach is interesting, it's very "organized".

But I think it's a bit conflicting with that purpose we had on notifiers
refactor, to deprecate "bogus" usages of panic_print, as in
https://lore.kernel.org/lkml/20220427224924.592546-26-gpicc...@igalia.com/ .

So, the idea of my approach is to allow:

(a) Easy removal of panic_print_sys_info() of panic(), once we move it
to a panic notifier;

(b) Better separate and identify the "bogus" cases. The CPU backtrace
one is less a bogus case in my opinion, more a "complicated" one, since
it's related with the CPUs stop routines. But the console flush, as we
discussed, it's clearly something that calls for a new parameter (and
such param was added in the refactor patch).


In the end, I think your approach is interesting but it's kinda like
we're adding the fix to later, on refactor, entirely remove/rework it.
With my approach we wouldn't be calling panic_print_sys_info() again
(3rd time!) on panic(), and also would be more natural to move it later
to a new panic notifier.

What you / others think? If your approach is in the end preferred, it's
fine by me - I'd just ask you to submit as a full patch so we can get it
merged as a fix in 6.3, if possible (and backport it to the 6.1/6.2
stable). Now, if my approach is fine, I can resubmit as a V5 keeping the
local variable - lemme know.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v4] panic: Fixes the panic_print NMI backtrace setting

[Guilherme G. Piccoli]

Commit 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
panic_print")
introduced a setting for the "panic_print" kernel parameter to allow
users to request a NMI backtrace on panic. Problem is that the panic_print
handling happens after the secondary CPUs are already disabled, hence
this option ended-up being kind of a no-op - kernel skips the NMI trace
in idling CPUs, which is the case of offline CPUs.

Fix it by checking the NMI backtrace bit in the panic_print prior to
the CPU disabling function.

Fixes: 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
panic_print")
Cc: sta...@vger.kernel.org
Signed-off-by: Guilherme G. Piccoli 

---

V4:
- Sent as standalone patch, rebased against v6.2-rc7.

V2 / V3:
- New patch, there was no V1 of this one.
Link for V3: 
https://lore.kernel.org/lkml/20220819221731.480795-12-gpicc...@igalia.com/


Hi folks, thanks in advance for reviews/comments.

Notice that while at it, I got rid of the "crash_kexec_post_notifiers"
local copy in panic(). This was introduced by commit b26e27ddfd2a
("kexec: use core_param for crash_kexec_post_notifiers boot option"),
but it is not clear from comments or commit message why this local copy
is required.

My understanding is that it's a mechanism to prevent some concurrency,
in case some other CPU modify this variable while panic() is running.
I find it very unlikely, hence I removed it - but if people consider
this copy needed, I can respin this patch and keep it, even providing a
comment about that, in order to be explict about its need.

Let me know your thoughts!
Cheers,

Guilherme


 kernel/panic.c | 47 +++
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 463c9295bc28..f45ee88be8a2 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -211,9 +211,6 @@ static void panic_print_sys_info(bool console_flush)
return;
}
 
-   if (panic_print & PANIC_PRINT_ALL_CPU_BT)
-   trigger_all_cpu_backtrace();
-
if (panic_print & PANIC_PRINT_TASK_INFO)
show_state();
 
@@ -243,6 +240,30 @@ void check_panic_on_warn(const char *origin)
  origin, limit);
 }
 
+/*
+ * Helper that triggers the NMI backtrace (if set in panic_print)
+ * and then performs the secondary CPUs shutdown - we cannot have
+ * the NMI backtrace after the CPUs are off!
+ */
+static void panic_other_cpus_shutdown(void)
+{
+   if (panic_print & PANIC_PRINT_ALL_CPU_BT)
+   trigger_all_cpu_backtrace();
+
+   /*
+* Note that smp_send_stop() is the usual SMP shutdown function,
+* which unfortunately may not be hardened to work in a panic
+* situation. If we want to do crash dump after notifier calls
+* and kmsg_dump, we will need architecture dependent extra
+* bits in addition to stopping other CPUs, hence we rely on
+* crash_smp_send_stop() for that.
+*/
+   if (!crash_kexec_post_notifiers)
+   smp_send_stop();
+   else
+   crash_smp_send_stop();
+}
+
 /**
  * panic - halt the system
  * @fmt: The text string to print
@@ -258,7 +279,6 @@ void panic(const char *fmt, ...)
long i, i_next = 0, len;
int state = 0;
int old_cpu, this_cpu;
-   bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
 
if (panic_on_warn) {
/*
@@ -333,23 +353,10 @@ void panic(const char *fmt, ...)
 *
 * Bypass the panic_cpu check and call __crash_kexec directly.
 */
-   if (!_crash_kexec_post_notifiers) {
+   if (!crash_kexec_post_notifiers)
__crash_kexec(NULL);
 
-   /*
-* Note smp_send_stop is the usual smp shutdown function, which
-* unfortunately means it may not be hardened to work in a
-* panic situation.
-*/
-   smp_send_stop();
-   } else {
-   /*
-* If we want to do crash dump after notifier calls and
-* kmsg_dump, we will need architecture dependent extra
-* works in addition to stopping other CPUs.
-*/
-   crash_smp_send_stop();
-   }
+   panic_other_cpus_shutdown();
 
/*
 * Run any panic handlers, including those that might need to
@@ -370,7 +377,7 @@ void panic(const char *fmt, ...)
 *
 * Bypass the panic_cpu check and call __crash_kexec directly.
 */
-   if (_crash_kexec_post_notifiers)
+   if (crash_kexec_post_notifiers)
__crash_kexec(NULL);
 
console_unblank();
-- 
2.39.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 08/11] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

Hi Boris and Petr, first of all thanks for your great analysis and
really sorry for the huge delay in my response.

Below I'm pasting the 2 relevant responses from both Petr and Boris.


On 22/11/2022 12:06, Borislav Petkov wrote:
> On Tue, Nov 22, 2022 at 10:33:12AM -0300, Guilherme G. Piccoli wrote:
> 
> Leaving in the whole thing for newly added people.
> 
>> On 18/09/2022 11:10, Guilherme G. Piccoli wrote:
>>> On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
>>>> The altera_edac panic notifier performs some data collection with
>>>> regards errors detected; such code relies in the regmap layer to
>>>> perform reads/writes, so the code is abstracted and there is some
>>>> risk level to execute that, since the panic path runs in atomic
>>>> context, with interrupts/preemption and secondary CPUs disabled.
>>>>
>>>> Users want the information collected in this panic notifier though,
>>>> so in order to balance the risk/benefit, let's skip the altera panic
>>>> notifier if kdump is loaded. While at it, remove a useless header
>>>> and encompass a macro inside the sole ifdef block it is used.
>>>>
>>>> Cc: Borislav Petkov 
>>>> Cc: Petr Mladek 
>>>> Cc: Tony Luck 
>>>> Acked-by: Dinh Nguyen 
>>>> Signed-off-by: Guilherme G. Piccoli 
>>>>
>>>> ---
>>>>
>>>> V3:
>>>> - added the ack tag from Dinh - thanks!
>>>> - had a good discussion with Boris about that in V2 [0],
>>>> hopefully we can continue and reach a consensus in this V3.
>>>> [0] 
>>>> https://lore.kernel.org/lkml/46137c67-25b4-6657-33b7-cffdc7afc...@igalia.com/
>>>>
>>>> V2:
>>>> - new patch, based on the discussion in [1].
>>>> [1] 
>>>> https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/
>>>>
>>>> [...]
>>>
>>> Hi Dinh, Tony, Boris - sorry for the ping.
>>>
>>> Appreciate reviews on this one - Dinh already ACKed the patch but Boris
>>> raised some points in the past version [0], so any opinions or
>>> discussions are welcome!
>>
>>
>> Hi folks, monthly ping heheh
>> Apologies for the re-pings, please let me know if there is anything
>> required to move on this patch.
> 
> Looking at this again, I really don't like the sprinkling of
> 
>   if (kexec_crash_loaded())
> 
> in unrelated code. And I still think that the real fix here is to kill
> this
> 
>   edac->panic_notifier
> 
> thing. And replace it with simply logging the error from the double bit
> error interrupt handle. That DBERR IRQ thing altr_edac_a10_irq_handler().
> Because this is what this panic notifier does - dump double-bit errors.
> 
> Now, if Dinh doesn't move, I guess we can ask Tony and/or Rabara (he has
> sent a patch for this driver recently and Altera belongs to Intel now)
> to find someone who can test such a change and we (you could give it a
> try first :)) can do that change.
> 
> Thx.
> 

On 09/12/2022 13:03, Petr Mladek wrote:> [...]>
> I have read the discussion about v2 [1] and this looks like a bad
> approach from my POV.
>
> My understanding is that the information provided by this notifier
> could not be found in the crashdump. It means that people really
> want to run this before crashdump in principle.
>
> Of course, there is the question how much safe this code is. I mean
> if the panic() code path might get blocked here.
>
> I see two possibilities.
>
> The best solution would be if we know that this is "always" safe or if
> it can be done a safe way. Then we could keep it as it is or implement
> the safe way.
>
> Alternative solution would be to create a kernel parameter that
> would enable/disable this particular report when kdump is enabled.
> The question would be the default. It would depend on how risky
> the code is and how useful the information is.
>
> [1] https://lore.kernel.org/r/20220719195325.402745-11-gpicc...@igalia.com
>


So, for me Petr approach is the more straightforward, though we could
rethink the idea of this notifier being...a notifier, as suggest Boris heh

Anyway, what I plan to do is: I'll re-submit a simple clean-up for this
code (header / ifdef stuff), not functional-changing the code path.

After that, when re-submitting the V2 or the notifiers refactor (which
I'm pending for some good months =O ), I'll deal with this code
properly, factoring the ideas and proposing a meaningful change.

So, let's discard this patch for now.
Thanks again,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: kdump kernel randomly hang with tick_periodic call trace on bare metal system

[Guilherme G. Piccoli]

On 20/12/2022 02:51, Baoquan He wrote:
> On 12/20/22 at 01:41pm, Baoquan He wrote:
>> On one intel bare metal system, I can randomly reproduce the kdump hang
>> as below with tick_periodic call trace. Attach the kernel config for
>> reference.
> 
> Forgot mentioning this random hang is also caused by adding
> 'nr_cpus=2' into normal kernel's cmdline, then triggering crash will get
> kdump kernel hang as below kdump log shown.
> 

The weird thing is that you seem to be using "nr_cpus=1" instead - this
is the cmdline from the log:

"nr_cpus=2 irqpoll nr_cpus=1 reset_devices cgroup_disable=memory mce=off
numa=off udev.children-max=2 panic=10 acpi_no_memhotplug
transparent_hugepage=never nokaslr hest_disable novmcoredd cma=0
hugetlb_cma=0 disable_cpu_apicid=16 [...]"

You seems to pass twice the "nr_cpus" thing, and I guess kernel pick the
last one?

Also, what is "disable_cpu_apicid=16"? Could this be related?


Thanks for the report!
Cheers,


Guilherme




___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 3/3] kexec: Introduce paramters load_limit_reboot and load_limit_panic

[Guilherme G. Piccoli]

On 08/12/2022 13:38, Ricardo Ribalda wrote:
> Add two parameter to specify how many times a kexec kernel can be loaded.
> 
> The sysadmin can set different limits for kexec panic and kexec reboot
> kernels.
> 
> The value can be modified at runtime via sysfs, but only with a value
> smaller than the current one (except -1).
> 
> Signed-off-by: Ricardo Ribalda 
> ---

Thanks for your patches Ricardo!

Small nit in the subject: s/paramters/parameters. Just observed that
after Joel's review anyway, so kudos to him heh

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 06/11] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 13/12/2022 21:06, Steven Rostedt wrote:
> [...]
> Heh, yeah, I forgot about this one too.
> 
>>
>> So, do you think it's possible to pick it for 6.2? No dependency here,
>> it's a standalone patch.
> 
> I can pull it in.

Awesome, thanks a bunch!


> 
>>
>> Thanks again and sorry for the annoyance!
> 
> No, sorry for missing these.
> 
> -- Steve

No biggies =))

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 06/11] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 20/10/2022 18:29, Steven Rostedt wrote:
> On Fri, 19 Aug 2022 19:17:26 -0300
> "Guilherme G. Piccoli"  wrote:
> 
>> Currently the tracing dump_on_oops feature is implemented through
>> separate notifiers, one for die/oops and the other for panic;
>> given they have the same functionality, let's unify them.
>>
>> Also improve the function comment and change the priority of the
>> notifier to make it execute earlier, avoiding showing useless trace
>> data (like the callback names for the other notifiers); finally,
>> we also removed an unnecessary header inclusion.
>>
>> Cc: Petr Mladek 
>> Cc: Sergei Shtylyov 
>> Cc: Steven Rostedt 
>> Signed-off-by: Guilherme G. Piccoli 
> 
> Sorry for the late reply.
> 
> Acked-by: Steven Rostedt (Google) 
> 
> -- Steve

Hi Steve, since you were so kind in the other patch...decided to ping in
this one as well heheh

So, do you think it's possible to pick it for 6.2? No dependency here,
it's a standalone patch.

Thanks again and sorry for the annoyance!
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 11/11] panic: Fixes the panic_print NMI backtrace setting

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Commit 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
> panic_print")
> introduced a setting for the "panic_print" kernel parameter to allow
> users to request a NMI backtrace on panic. Problem is that the panic_print
> handling happens after the secondary CPUs are already disabled, hence
> this option ended-up being kind of a no-op - kernel skips the NMI trace
> in idling CPUs, which is the case of offline CPUs.
> 
> Fix it by checking the NMI backtrace bit in the panic_print prior to
> the CPU disabling function.
> 
> Fixes: 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
> panic_print")
> Cc: Feng Tang 
> Cc: Petr Mladek 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - No changes.
> 
> V2:
> - new patch, there was no V1 of this one.
> 
> Hi folks, thanks upfront for reviews. This is a new patch, fixing an issue
> I found in my tests, so I shoved it into this fixes series.
> 
> Notice that while at it, I got rid of the "crash_kexec_post_notifiers"
> local copy in panic(). This was introduced by commit b26e27ddfd2a
> ("kexec: use core_param for crash_kexec_post_notifiers boot option"),
> but it is not clear from comments or commit message why this local copy
> is required.
> 
> My understanding is that it's a mechanism to prevent some concurrency,
> in case some other CPU modify this variable while panic() is running.
> I find it very unlikely, hence I removed it - but if people consider
> this copy needed, I can respin this patch and keep it, even providing a
> comment about that, in order to be explict about its need.
> 
> Let me know your thoughts! Cheers,
> 
> Guilherme
> 
> 

Hi folks, bi-monthly ping - apologies for the noise heh

Is there anything suggested so we can get this fix merged in 6.2? Any
suggestions / reviews are much appreciated.

Tnx in advance,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 08/11] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 18/09/2022 11:10, Guilherme G. Piccoli wrote:
> On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
>> The altera_edac panic notifier performs some data collection with
>> regards errors detected; such code relies in the regmap layer to
>> perform reads/writes, so the code is abstracted and there is some
>> risk level to execute that, since the panic path runs in atomic
>> context, with interrupts/preemption and secondary CPUs disabled.
>>
>> Users want the information collected in this panic notifier though,
>> so in order to balance the risk/benefit, let's skip the altera panic
>> notifier if kdump is loaded. While at it, remove a useless header
>> and encompass a macro inside the sole ifdef block it is used.
>>
>> Cc: Borislav Petkov 
>> Cc: Petr Mladek 
>> Cc: Tony Luck 
>> Acked-by: Dinh Nguyen 
>> Signed-off-by: Guilherme G. Piccoli 
>>
>> ---
>>
>> V3:
>> - added the ack tag from Dinh - thanks!
>> - had a good discussion with Boris about that in V2 [0],
>> hopefully we can continue and reach a consensus in this V3.
>> [0] 
>> https://lore.kernel.org/lkml/46137c67-25b4-6657-33b7-cffdc7afc...@igalia.com/
>>
>> V2:
>> - new patch, based on the discussion in [1].
>> [1] 
>> https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/
>>
>> [...]
> 
> Hi Dinh, Tony, Boris - sorry for the ping.
> 
> Appreciate reviews on this one - Dinh already ACKed the patch but Boris
> raised some points in the past version [0], so any opinions or
> discussions are welcome!


Hi folks, monthly ping heheh
Apologies for the re-pings, please let me know if there is anything
required to move on this patch.

Cheers,


Guilherme


P.S. I've been trimming the huge CC list in the series, done it here as
well.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 07/11] notifiers: Add tracepoints to the notifiers infrastructure

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Currently there is no way to show the callback names for registered,
> unregistered or executed notifiers. This is very useful for debug
> purposes, hence add this functionality here in the form of notifiers'
> tracepoints, one per operation.
> 
> Cc: Arjan van de Ven 
> Cc: Cong Wang 
> Cc: Sebastian Andrzej Siewior 
> Cc: Steven Rostedt 
> Cc: Valentin Schneider 
> Cc: Xiaoming Ni 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - Yet another major change - thanks to Arjan's great suggestion,
> refactored the code to make use of tracepoints instead of guarding
> the output with a Kconfig debug setting.
> 
> V2:
> - Major improvement thanks to the great idea from Xiaoming - changed
> all the ksym wheel reinvention to printk %ps modifier;
> 
> - Instead of ifdefs, using IS_ENABLED() - thanks Steven.
> 
> - Removed an unlikely() hint on debug path.
> 
> 
>  include/trace/events/notifiers.h | 69 
>  kernel/notifier.c|  6 +++
>  2 files changed, 75 insertions(+)
>  create mode 100644 include/trace/events/notifiers.h
> 

Hi Arjan / Xiaoming / all, monthly ping heh

If there's any advice on how to move things here, I would appreciate!
Thanks in advance and apologies for the re-pings.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 06/11] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 20/10/2022 19:22, Steven Rostedt wrote:
> On Thu, 20 Oct 2022 18:53:43 -0300
> "Guilherme G. Piccoli"  wrote:
> 
>> Could you pick it in your tree? Or do you prefer that I re-send as a
>> solo patch, with your ACK?
> 
> I wasn't sure there were any dependencies on this. If not, I can take it.
> 
> -- Steve

Hi Steve, I'm really sorry for the re-ping. Just wanted to know what's
the status here, not sure if you picked this one (checked
linux-trace/for-next, didn't see it there) or planning to.

Thanks in advance,


Guilherme


P.S. Trimmed huge CC list...

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 03/11] alpha: Clean-up the panic notifier code

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> The alpha panic notifier has some code issues, not following
> the conventions of other notifiers. Also, it might halt the
> machine but still it is set to run as early as possible, which
> doesn't seem to be a good idea.
> 
> So, let's clean the code and set the notifier to run as the
> latest, following the same approach other architectures are
> doing - also, remove the unnecessary include of a header already
> included indirectly.
> 
> Cc: Ivan Kokshaysky 
> Cc: Matt Turner 
> Cc: Richard Henderson 
> Reviewed-by: Petr Mladek 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - No changes.
> 
> V2:
> - Fixed rth email address;
> - Added Petr's review tag - thanks!
> 

Hi Alpha maintainers, is there anything else to be done here? I'd really
appreciate any advice on how to get this merged.

I'm also adding here Richard's linaro email (and trimming huge CC list).

Thanks in advance!
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 02/11] notifier: Add panic notifiers info and purge trailing whitespaces

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Although many notifiers are mentioned in the comments, the panic
> notifiers infrastructure is not. Also, the file contains some
> trailing whitespaces. Fix both issues here.
> 
> Cc: Arjan van de Ven 
> Cc: Cong Wang 
> Cc: Sebastian Andrzej Siewior 
> Cc: Valentin Schneider 
> Cc: Xiaoming Ni 
> Reviewed-by: Baoquan He 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - Added Baoquan review tag - thanks!
> 
> V2:
> - No changes.
> 
> 

Hi Andrew, do you think it makes sense to merge this one for v6.2/next?
I don't see anything else required here, lemme know otherwise.

Thanks,


Guilherme


P.S. Trimmed a bit the huge CC list, but kept all MLs.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 10/11] drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers

[Guilherme G. Piccoli]

On 11/11/2022 20:16, Wei Liu wrote:
> On Fri, Nov 11, 2022 at 10:47:17PM +, Wei Liu wrote:
>> On Thu, Nov 10, 2022 at 06:32:54PM -0300, Guilherme G. Piccoli wrote:
>>> [Trimming long CC list]
>>>
>>> Hi Wei / Michael, just out of curiosity, did this (and patch 9) ended-up
>>> being queued in hyperv-next?
>>>
>>
>> No. They are not queued.
>>
>> I didn't notice Michael's email and thought they would be picked up by
>> someone else while dealing with the whole series.
>>
>> I will pick them up later.
> 
> They are now queued to hyperv-next. Thanks.

Thanks a lot Wei and Michael, much appreciated =)

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 10/11] drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers

[Guilherme G. Piccoli]

[Trimming long CC list]

Hi Wei / Michael, just out of curiosity, did this (and patch 9) ended-up
being queued in hyperv-next?

Thanks in advance,


Guilherme

On 17/10/2022 12:26, Michael Kelley (LINUX) wrote:
> From: Guilherme G. Piccoli  Sent: Tuesday, October 4, 
> 2022 10:20 AM
>>
>> On 04/10/2022 13:24, Michael Kelley (LINUX) wrote:
>>> [...]
>>>
>>> Tested this patch in combination with Patch 9 in this series.  Verified
>>> that both the panic and die paths work correctly with notification to
>>> Hyper-V via hyperv_report_panic() or via hv_kmsg_dump().  Hyper-V
>>> framebuffer is updated as expected, though I did not reproduce
>>> a case where the ring buffer lock is held.  vmbus_initiate_unload() runs
>>> as expected.
>>>
>>> Tested-by: Michael Kelley 
>>>
>>
>> Thanks a lot for the tests/review Michael!
>>
>> Do you think Hyper-V folks could add both patches in hv tree? If you
>> prefer, I can re-send them individually.
>>
> 
> Wei Liu:  Could you pick up Patch 9 and Patch 10 from this series in the
> hyperv-next tree?
> 
> Michael
> 

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 06/11] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 20/10/2022 18:29, Steven Rostedt wrote:
> [...]
> Sorry for the late reply.
> 
> Acked-by: Steven Rostedt (Google) 
> 
> -- Steve

No need for apologies! Appreciate your review/ack =)

Could you pick it in your tree? Or do you prefer that I re-send as a
solo patch, with your ACK?

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 06/11] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 20/10/2022 19:22, Steven Rostedt wrote:
> On Thu, 20 Oct 2022 18:53:43 -0300
> "Guilherme G. Piccoli"  wrote:
> 
>> Could you pick it in your tree? Or do you prefer that I re-send as a
>> solo patch, with your ACK?
> 
> I wasn't sure there were any dependencies on this. If not, I can take it.
> 
> -- Steve

Thank you! No dependency at all...I just piled a bunch of independent
panic notifiers fixes, so they can be reviewed by the maintainers and
picked individually.

Some maintainers though prefer them to be sent individually, as solo
patches...hence my question.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 01/11] ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths

[Guilherme G. Piccoli]

On 17/10/2022 14:47, Russell King (Oracle) wrote:
> On Mon, Oct 17, 2022 at 11:50:05AM -0300, Guilherme G. Piccoli wrote:
>> On 17/10/2022 11:17, Russell King (Oracle) wrote:
>>> [...]
>>>> Monthly ping - let me know if there's something I should improve in
>>>> order this fix is considered!
>>>
>>> Patches don't get applied unless they end up in the patch system.
>>> Thanks.
>>>
>>
>> Thanks Russell! Can you show me some documentation on how should I send
>> the patches to this patch system? My understanding based in the
>> MAINTAINERS file is that we should send the arm32 patches to you + arm ML.
> 
> Look below in my signature --.
>  |
>v


Thank you! It seems I was able to submit it properly now:
https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=9257/1

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 07/11] notifiers: Add tracepoints to the notifiers infrastructure

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Currently there is no way to show the callback names for registered,
> unregistered or executed notifiers. This is very useful for debug
> purposes, hence add this functionality here in the form of notifiers'
> tracepoints, one per operation.
> 
> Cc: Arjan van de Ven 
> Cc: Cong Wang 
> Cc: Sebastian Andrzej Siewior 
> Cc: Steven Rostedt 
> Cc: Valentin Schneider 
> Cc: Xiaoming Ni 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - Yet another major change - thanks to Arjan's great suggestion,
> refactored the code to make use of tracepoints instead of guarding
> the output with a Kconfig debug setting.
> 
> V2:
> - Major improvement thanks to the great idea from Xiaoming - changed
> all the ksym wheel reinvention to printk %ps modifier;
> 
> - Instead of ifdefs, using IS_ENABLED() - thanks Steven.
> 
> - Removed an unlikely() hint on debug path.
> 

Hi Arjan / all, apologies for the re-ping.
Did you have a chance to take a look in this one, is there anything else
I could improve?

Thanks in advance,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 01/11] ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths

[Guilherme G. Piccoli]

On 17/10/2022 11:17, Russell King (Oracle) wrote:
> [...]
>> Monthly ping - let me know if there's something I should improve in
>> order this fix is considered!
> 
> Patches don't get applied unless they end up in the patch system.
> Thanks.
> 

Thanks Russell! Can you show me some documentation on how should I send
the patches to this patch system? My understanding based in the
MAINTAINERS file is that we should send the arm32 patches to you + arm ML.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 06/11] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Currently the tracing dump_on_oops feature is implemented through
> separate notifiers, one for die/oops and the other for panic;
> given they have the same functionality, let's unify them.
> 
> Also improve the function comment and change the priority of the
> notifier to make it execute earlier, avoiding showing useless trace
> data (like the callback names for the other notifiers); finally,
> we also removed an unnecessary header inclusion.
> 
> Cc: Petr Mladek 
> Cc: Sergei Shtylyov 
> Cc: Steven Rostedt 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - Removed goto usage, as per Steven suggestion (thanks!).
> 
> V2:
> - Different approach; instead of using IDs to distinguish die and
> panic events, rely on address comparison like other notifiers do
> and as per Petr's suggestion;
> 
> - Removed ACK from Steven since the code changed.
> 

Hi Steven, apologies for the re-ping. Is there anything else to do in
this one, or do you think it's good enough?

Thanks,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 04/11] um: Improve panic notifiers consistency and ordering

[Guilherme G. Piccoli]

On 18/09/2022 18:19, Richard Weinberger wrote:
> - Ursprüngliche Mail -
>> Von: "Guilherme G. Piccoli" 
>> On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
>>> Currently the panic notifiers from user mode linux don't follow
>>> the convention for most of the other notifiers present in the
>>> kernel (indentation, priority setting, numeric return).
>>> More important, the priorities could be improved, since it's a
>>> special case (userspace), hence we could run the notifiers earlier;
>>> user mode linux shouldn't care much with other panic notifiers but
>>> the ordering among the mconsole and arch notifier is important,
>>> given that the arch one effectively triggers a core dump.
>>>
>>> Fix that by running the mconsole notifier as the first panic
>>> notifier, followed by the architecture one (that coredumps).
>>>
>>> Cc: Anton Ivanov 
>>> Cc: Johannes Berg 
>>> Cc: Richard Weinberger 
>>> Signed-off-by: Guilherme G. Piccoli 
>>>
>>> V3:
>>> - No changes.
>>> [...]
>>
>> Hi Johannes, sorry for the ping. Do you think you could pick this one?
>> Or if you prefer, I can resend it alone (not in the series) - let me
>> know your preference.
> 
> This patch is on my TODO list.
> Just had no chance to test it.
> 
> Thanks,
> //richard

Hi Richard / Johannes, is there any news on this one?
Thanks in advance,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 01/11] ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths

[Guilherme G. Piccoli]

On 18/09/2022 10:58, Guilherme G. Piccoli wrote:
> On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
>> Currently the regular CPU shutdown path for ARM disables IRQs/FIQs
>> in the secondary CPUs - smp_send_stop() calls ipi_cpu_stop(), which
>> is responsible for that. IRQs are architecturally masked when we
>> take an interrupt, but FIQs are high priority than IRQs, hence they
>> aren't masked. With that said, it makes sense to disable FIQs here,
>> but there's no need for (re-)disabling IRQs.
>>
>> More than that: there is an alternative path for disabling CPUs,
>> in the form of function crash_smp_send_stop(), which is used for
>> kexec/panic path. This function relies on a SMP call that also
>> triggers a busy-wait loop [at machine_crash_nonpanic_core()], but
>> without disabling FIQs. This might lead to odd scenarios, like
>> early interrupts in the boot of kexec'd kernel or even interrupts
>> in secondary "disabled" CPUs while the main one still works in the
>> panic path and assumes all secondary CPUs are (really!) off.
>>
>> So, let's disable FIQs in both paths and *not* disable IRQs a second
>> time, since they are already masked in both paths by the architecture.
>> This way, we keep both CPU quiesce paths consistent and safe.
>>
>> Cc: Marc Zyngier 
>> Cc: Michael Kelley 
>> Cc: Russell King 
>> Signed-off-by: Guilherme G. Piccoli 
>>

Monthly ping - let me know if there's something I should improve in
order this fix is considered!
Thanks,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 08/11] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 18/09/2022 11:10, Guilherme G. Piccoli wrote:
> On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
>> The altera_edac panic notifier performs some data collection with
>> regards errors detected; such code relies in the regmap layer to
>> perform reads/writes, so the code is abstracted and there is some
>> risk level to execute that, since the panic path runs in atomic
>> context, with interrupts/preemption and secondary CPUs disabled.
>>
>> Users want the information collected in this panic notifier though,
>> so in order to balance the risk/benefit, let's skip the altera panic
>> notifier if kdump is loaded. While at it, remove a useless header
>> and encompass a macro inside the sole ifdef block it is used.
>>
>> Cc: Borislav Petkov 
>> Cc: Petr Mladek 
>> Cc: Tony Luck 
>> Acked-by: Dinh Nguyen 
>> Signed-off-by: Guilherme G. Piccoli 
>>
>> ---
>>
>> V3:
>> - added the ack tag from Dinh - thanks!
>> - had a good discussion with Boris about that in V2 [0],
>> hopefully we can continue and reach a consensus in this V3.
>> [0] 
>> https://lore.kernel.org/lkml/46137c67-25b4-6657-33b7-cffdc7afc...@igalia.com/
>>
>> V2:
>> - new patch, based on the discussion in [1].
>> [1] 
>> https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/
>>
>> [...]
> 
> Hi Dinh, Tony, Boris - sorry for the ping.

Hey folks, apologies for the new ping.

Is there anything to improve here maybe? Reviews / opinions are very
appreciated!
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 04/11] um: Improve panic notifiers consistency and ordering

[Guilherme G. Piccoli]

On 17/10/2022 11:10, Richard Weinberger wrote:
> - Ursprüngliche Mail -
>> Von: "Guilherme G. Piccoli" 
>> Hi Richard / Johannes, is there any news on this one?
>> Thanks in advance,
> 
> It's upstream:
> git.kernel.org/linus/758dfdb9185cf94160f20e85bbe05583e3cd4ff4
> 
> Thanks,
> //richard

Wow, thanks! I am sorry, I didn't notice.
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 10/11] drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers

[Guilherme G. Piccoli]

On 04/10/2022 13:24, Michael Kelley (LINUX) wrote:
> [...]
> 
> Tested this patch in combination with Patch 9 in this series.  Verified
> that both the panic and die paths work correctly with notification to
> Hyper-V via hyperv_report_panic() or via hv_kmsg_dump().  Hyper-V
> framebuffer is updated as expected, though I did not reproduce
> a case where the ring buffer lock is held.  vmbus_initiate_unload() runs
> as expected.
> 
> Tested-by: Michael Kelley 
> 

Thanks a lot for the tests/review Michael!

Do you think Hyper-V folks could add both patches in hv tree? If you
prefer, I can re-send them individually.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 07/11] notifiers: Add tracepoints to the notifiers infrastructure

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Currently there is no way to show the callback names for registered,
> unregistered or executed notifiers. This is very useful for debug
> purposes, hence add this functionality here in the form of notifiers'
> tracepoints, one per operation.
> 
> Cc: Arjan van de Ven 
> Cc: Cong Wang 
> Cc: Sebastian Andrzej Siewior 
> Cc: Steven Rostedt 
> Cc: Valentin Schneider 
> Cc: Xiaoming Ni 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - Yet another major change - thanks to Arjan's great suggestion,
> refactored the code to make use of tracepoints instead of guarding
> the output with a Kconfig debug setting.
> 
> V2:
> - Major improvement thanks to the great idea from Xiaoming - changed
> all the ksym wheel reinvention to printk %ps modifier;
> 
> - Instead of ifdefs, using IS_ENABLED() - thanks Steven.
> 
> - Removed an unlikely() hint on debug path.
> 
> [...]

Hi Arjan / Xioming, apologies for the ping.
Do you think the patch is good enough now? I liked the tracepoint
approach, indeed it was a much better idea than guarding the prints with
the DEBUG Kconfig heheh

Appreciate your opinions!
Thanks in advance,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 08/11] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> The altera_edac panic notifier performs some data collection with
> regards errors detected; such code relies in the regmap layer to
> perform reads/writes, so the code is abstracted and there is some
> risk level to execute that, since the panic path runs in atomic
> context, with interrupts/preemption and secondary CPUs disabled.
> 
> Users want the information collected in this panic notifier though,
> so in order to balance the risk/benefit, let's skip the altera panic
> notifier if kdump is loaded. While at it, remove a useless header
> and encompass a macro inside the sole ifdef block it is used.
> 
> Cc: Borislav Petkov 
> Cc: Petr Mladek 
> Cc: Tony Luck 
> Acked-by: Dinh Nguyen 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - added the ack tag from Dinh - thanks!
> - had a good discussion with Boris about that in V2 [0],
> hopefully we can continue and reach a consensus in this V3.
> [0] 
> https://lore.kernel.org/lkml/46137c67-25b4-6657-33b7-cffdc7afc...@igalia.com/
> 
> V2:
> - new patch, based on the discussion in [1].
> [1] 
> https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/
> 
> [...]

Hi Dinh, Tony, Boris - sorry for the ping.

Appreciate reviews on this one - Dinh already ACKed the patch but Boris
raised some points in the past version [0], so any opinions or
discussions are welcome!

Thanks,


Guilherme


[0]
https://lore.kernel.org/lkml/46137c67-25b4-6657-33b7-cffdc7afc...@igalia.com/

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 11/11] panic: Fixes the panic_print NMI backtrace setting

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Commit 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
> panic_print")
> introduced a setting for the "panic_print" kernel parameter to allow
> users to request a NMI backtrace on panic. Problem is that the panic_print
> handling happens after the secondary CPUs are already disabled, hence
> this option ended-up being kind of a no-op - kernel skips the NMI trace
> in idling CPUs, which is the case of offline CPUs.
> 
> Fix it by checking the NMI backtrace bit in the panic_print prior to
> the CPU disabling function.
> 
> Fixes: 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
> panic_print")
> Cc: Feng Tang 
> Cc: Petr Mladek 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - No changes.
> 
> V2:
> - new patch, there was no V1 of this one.
> 
> Hi folks, thanks upfront for reviews. This is a new patch, fixing an issue
> I found in my tests, so I shoved it into this fixes series.
> 
> Notice that while at it, I got rid of the "crash_kexec_post_notifiers"
> local copy in panic(). This was introduced by commit b26e27ddfd2a
> ("kexec: use core_param for crash_kexec_post_notifiers boot option"),
> but it is not clear from comments or commit message why this local copy
> is required.
> 
> My understanding is that it's a mechanism to prevent some concurrency,
> in case some other CPU modify this variable while panic() is running.
> I find it very unlikely, hence I removed it - but if people consider
> this copy needed, I can respin this patch and keep it, even providing a
> comment about that, in order to be explict about its need.
> 
> Let me know your thoughts! Cheers,
> 
> Guilherme
> 
> 
>  kernel/panic.c | 47 +++
>  1 file changed, 27 insertions(+), 20 deletions(-)
> [...]

Hi Andrew, sorry for the ping.

Does the patch makes sense for you? Any comments are much appreciated!
Tnx in advance,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 04/11] um: Improve panic notifiers consistency and ordering

[Guilherme G. Piccoli]

On 18/09/2022 18:19, Richard Weinberger wrote:
> - Ursprüngliche Mail -
>> Von: "Guilherme G. Piccoli" 
>> On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
>>> Currently the panic notifiers from user mode linux don't follow
>>> the convention for most of the other notifiers present in the
>>> kernel (indentation, priority setting, numeric return).
>>> More important, the priorities could be improved, since it's a
>>> special case (userspace), hence we could run the notifiers earlier;
>>> user mode linux shouldn't care much with other panic notifiers but
>>> the ordering among the mconsole and arch notifier is important,
>>> given that the arch one effectively triggers a core dump.
>>>
>>> Fix that by running the mconsole notifier as the first panic
>>> notifier, followed by the architecture one (that coredumps).
>>>
>>> Cc: Anton Ivanov 
>>> Cc: Johannes Berg 
>>> Cc: Richard Weinberger 
>>> Signed-off-by: Guilherme G. Piccoli 
>>>
>>> V3:
>>> - No changes.
>>> [...]
>>
>> Hi Johannes, sorry for the ping. Do you think you could pick this one?
>> Or if you prefer, I can resend it alone (not in the series) - let me
>> know your preference.
> 
> This patch is on my TODO list.
> Just had no chance to test it.
> 
> Thanks,
> //richard

Thanks a lot Richard!

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 09/11] video/hyperv_fb: Avoid taking busy spinlock on panic path

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> The Hyper-V framebuffer code registers a panic notifier in order
> to try updating its fbdev if the kernel crashed. The notifier
> callback is straightforward, but it calls the vmbus_sendpacket()
> routine eventually, and such function takes a spinlock for the
> ring buffer operations.
> 
> Panic path runs in atomic context, with local interrupts and
> preemption disabled, and all secondary CPUs shutdown. That said,
> taking a spinlock might cause a lockup if a secondary CPU was
> disabled with such lock taken. Fix it here by checking if the
> ring buffer spinlock is busy on Hyper-V framebuffer panic notifier;
> if so, bail-out avoiding the potential lockup scenario.
> 
> Cc: Andrea Parri (Microsoft) 
> Cc: Dexuan Cui 
> Cc: Haiyang Zhang 
> Cc: "K. Y. Srinivasan" 
> Cc: Michael Kelley 
> Cc: Stephen Hemminger 
> Cc: Tianyu Lan 
> Cc: Wei Liu 
> Tested-by: Fabio A M Martins 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - simplified the code based on Michael's suggestion - thanks!
> 
> V2:
> - new patch, based on the discussion in [0].
> [0] 
> https://lore.kernel.org/lkml/2787b476-6366-1c83-db80-0393da417...@igalia.com/
> 
> 
>  drivers/hv/ring_buffer.c| 13 +
>  drivers/video/fbdev/hyperv_fb.c |  8 +++-
>  include/linux/hyperv.h  |  2 ++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> [...]

Hi Michael, apologies for the ping.
Any reviews/comments on this one are greatly appreciated!

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 06/11] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Currently the tracing dump_on_oops feature is implemented through
> separate notifiers, one for die/oops and the other for panic;
> given they have the same functionality, let's unify them.
> 
> Also improve the function comment and change the priority of the
> notifier to make it execute earlier, avoiding showing useless trace
> data (like the callback names for the other notifiers); finally,
> we also removed an unnecessary header inclusion.
> 
> Cc: Petr Mladek 
> Cc: Sergei Shtylyov 
> Cc: Steven Rostedt 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - Removed goto usage, as per Steven suggestion (thanks!).
> 
> V2:
> - Different approach; instead of using IDs to distinguish die and
> panic events, rely on address comparison like other notifiers do
> and as per Petr's suggestion;
> 
> - Removed ACK from Steven since the code changed.
> 
> [...]

Hi Steve, Alan - sorry for the ping (and I'm aware you're OOO Steve, saw
your auto-response email heh).

So, is this version good enough? Appreciate the reviews and in case it's
good, let me know your preference for picking it in your tree - I could
resend the patch alone if you prefer (not in the series), for example.

Thanks,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 04/11] um: Improve panic notifiers consistency and ordering

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Currently the panic notifiers from user mode linux don't follow
> the convention for most of the other notifiers present in the
> kernel (indentation, priority setting, numeric return).
> More important, the priorities could be improved, since it's a
> special case (userspace), hence we could run the notifiers earlier;
> user mode linux shouldn't care much with other panic notifiers but
> the ordering among the mconsole and arch notifier is important,
> given that the arch one effectively triggers a core dump.
> 
> Fix that by running the mconsole notifier as the first panic
> notifier, followed by the architecture one (that coredumps).
> 
> Cc: Anton Ivanov 
> Cc: Johannes Berg 
> Cc: Richard Weinberger 
> Signed-off-by: Guilherme G. Piccoli 
> 
> V3:
> - No changes.
> [...]

Hi Johannes, sorry for the ping. Do you think you could pick this one?
Or if you prefer, I can resend it alone (not in the series) - let me
know your preference.

Thanks,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V3 01/11] ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths

[Guilherme G. Piccoli]

On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Currently the regular CPU shutdown path for ARM disables IRQs/FIQs
> in the secondary CPUs - smp_send_stop() calls ipi_cpu_stop(), which
> is responsible for that. IRQs are architecturally masked when we
> take an interrupt, but FIQs are high priority than IRQs, hence they
> aren't masked. With that said, it makes sense to disable FIQs here,
> but there's no need for (re-)disabling IRQs.
> 
> More than that: there is an alternative path for disabling CPUs,
> in the form of function crash_smp_send_stop(), which is used for
> kexec/panic path. This function relies on a SMP call that also
> triggers a busy-wait loop [at machine_crash_nonpanic_core()], but
> without disabling FIQs. This might lead to odd scenarios, like
> early interrupts in the boot of kexec'd kernel or even interrupts
> in secondary "disabled" CPUs while the main one still works in the
> panic path and assumes all secondary CPUs are (really!) off.
> 
> So, let's disable FIQs in both paths and *not* disable IRQs a second
> time, since they are already masked in both paths by the architecture.
> This way, we keep both CPU quiesce paths consistent and safe.
> 
> Cc: Marc Zyngier 
> Cc: Michael Kelley 
> Cc: Russell King 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V3:
> - No changes.
> 
> V2:
> - Small wording improvement (thanks Michael Kelley);
> - Only disable FIQs, since IRQs are masked by architecture
> definition when we take an interrupt. Thanks a lot Russell
> and Marc for the discussion [0].
> 
> Should we add a Fixes tag here? If so, maybe the proper target is:
> b23065313297 ("ARM: 6522/1: kexec: Add call to non-crashing cores through 
> IPI")
> 
> [0] https://lore.kernel.org/lkml/ymxcaqy6dwhoq...@shell.armlinux.org.uk/
> 
> 
>  arch/arm/kernel/machine_kexec.c | 2 ++
>  arch/arm/kernel/smp.c   | 5 ++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> index f567032a09c0..0b482bcb97f7 100644
> --- a/arch/arm/kernel/machine_kexec.c
> +++ b/arch/arm/kernel/machine_kexec.c
> @@ -77,6 +77,8 @@ void machine_crash_nonpanic_core(void *unused)
>  {
>   struct pt_regs regs;
>  
> + local_fiq_disable();
> +
>   crash_setup_regs(, get_irq_regs());
>   printk(KERN_DEBUG "CPU %u will stop doing anything useful since another 
> CPU has crashed\n",
>  smp_processor_id());
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 978db2d96b44..36e6efad89f3 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -600,6 +600,8 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
>   */
>  static void ipi_cpu_stop(unsigned int cpu)
>  {
> + local_fiq_disable();
> +
>   if (system_state <= SYSTEM_RUNNING) {
>   raw_spin_lock(_lock);
>   pr_crit("CPU%u: stopping\n", cpu);
> @@ -609,9 +611,6 @@ static void ipi_cpu_stop(unsigned int cpu)
>  
>   set_cpu_online(cpu, false);
>  
> - local_fiq_disable();
> - local_irq_disable();
> -
>   while (1) {
>   cpu_relax();
>   wfe();

[+CC all ARM folks I could find]

Hi folks, sorry for the ping. Any reviews are greatly appreciated in
this one - this is the V3 of the series but I never got any specific
review for this patch.

Based on a previous discussion with Russell and Marc [0], seems this one
makes sense and fixes an inconsistency related to kdump/panic.

Really appreciate reviews or at least some indication on which path I
should take to get this potentially merged.

Cheers,


Guilherme


[0] https://lore.kernel.org/lkml/ymxcaqy6dwhoq...@shell.armlinux.org.uk/

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 09/11] video/hyperv_fb: Avoid taking busy spinlock on panic path

[Guilherme G. Piccoli]

The Hyper-V framebuffer code registers a panic notifier in order
to try updating its fbdev if the kernel crashed. The notifier
callback is straightforward, but it calls the vmbus_sendpacket()
routine eventually, and such function takes a spinlock for the
ring buffer operations.

Panic path runs in atomic context, with local interrupts and
preemption disabled, and all secondary CPUs shutdown. That said,
taking a spinlock might cause a lockup if a secondary CPU was
disabled with such lock taken. Fix it here by checking if the
ring buffer spinlock is busy on Hyper-V framebuffer panic notifier;
if so, bail-out avoiding the potential lockup scenario.

Cc: Andrea Parri (Microsoft) 
Cc: Dexuan Cui 
Cc: Haiyang Zhang 
Cc: "K. Y. Srinivasan" 
Cc: Michael Kelley 
Cc: Stephen Hemminger 
Cc: Tianyu Lan 
Cc: Wei Liu 
Tested-by: Fabio A M Martins 
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- simplified the code based on Michael's suggestion - thanks!

V2:
- new patch, based on the discussion in [0].
[0] 
https://lore.kernel.org/lkml/2787b476-6366-1c83-db80-0393da417...@igalia.com/


 drivers/hv/ring_buffer.c| 13 +
 drivers/video/fbdev/hyperv_fb.c |  8 +++-
 include/linux/hyperv.h  |  2 ++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 59a4aa86d1f3..c6692fd5ab15 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -280,6 +280,19 @@ void hv_ringbuffer_cleanup(struct hv_ring_buffer_info 
*ring_info)
ring_info->pkt_buffer_size = 0;
 }
 
+/*
+ * Check if the ring buffer spinlock is available to take or not; used on
+ * atomic contexts, like panic path (see the Hyper-V framebuffer driver).
+ */
+
+bool hv_ringbuffer_spinlock_busy(struct vmbus_channel *channel)
+{
+   struct hv_ring_buffer_info *rinfo = >outbound;
+
+   return spin_is_locked(>ring_lock);
+}
+EXPORT_SYMBOL_GPL(hv_ringbuffer_spinlock_busy);
+
 /* Write to the ring buffer. */
 int hv_ringbuffer_write(struct vmbus_channel *channel,
const struct kvec *kv_list, u32 kv_count,
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 886c564787f1..e1b65a01fb96 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -783,12 +783,18 @@ static void hvfb_ondemand_refresh_throttle(struct 
hvfb_par *par,
 static int hvfb_on_panic(struct notifier_block *nb,
 unsigned long e, void *p)
 {
+   struct hv_device *hdev;
struct hvfb_par *par;
struct fb_info *info;
 
par = container_of(nb, struct hvfb_par, hvfb_panic_nb);
-   par->synchronous_fb = true;
info = par->info;
+   hdev = device_to_hv_device(info->device);
+
+   if (hv_ringbuffer_spinlock_busy(hdev->channel))
+   return NOTIFY_DONE;
+
+   par->synchronous_fb = true;
if (par->need_docopy)
hvfb_docopy(par, 0, dio_fb_size);
synthvid_update(info, 0, 0, INT_MAX, INT_MAX);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 3b42264333ef..646f1da9f27e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1341,6 +1341,8 @@ struct hv_ring_buffer_debug_info {
 int hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
struct hv_ring_buffer_debug_info *debug_info);
 
+bool hv_ringbuffer_spinlock_busy(struct vmbus_channel *channel);
+
 /* Vmbus interface */
 #define vmbus_driver_register(driver)  \
__vmbus_driver_register(driver, THIS_MODULE, KBUILD_MODNAME)
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 02/11] notifier: Add panic notifiers info and purge trailing whitespaces

[Guilherme G. Piccoli]

Although many notifiers are mentioned in the comments, the panic
notifiers infrastructure is not. Also, the file contains some
trailing whitespaces. Fix both issues here.

Cc: Arjan van de Ven 
Cc: Cong Wang 
Cc: Sebastian Andrzej Siewior 
Cc: Valentin Schneider 
Cc: Xiaoming Ni 
Reviewed-by: Baoquan He 
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- Added Baoquan review tag - thanks!

V2:
- No changes.


 include/linux/notifier.h | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index aef88c2d1173..d5b01f2e3fcc 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -208,12 +208,12 @@ static inline int notifier_to_errno(int ret)
 
 /*
  * Declared notifiers so far. I can imagine quite a few more chains
- * over time (eg laptop power reset chains, reboot chain (to clean 
+ * over time (eg laptop power reset chains, reboot chain (to clean
  * device units up), device [un]mount chain, module load/unload chain,
- * low memory chain, screenblank chain (for plug in modular 
screenblankers) 
+ * low memory chain, screenblank chain (for plug in modular screenblankers)
  * VC switch chains (for loadable kernel svgalib VC switch helpers) etc...
  */
- 
+
 /* CPU notfiers are defined in include/linux/cpu.h. */
 
 /* netdevice notifiers are defined in include/linux/netdevice.h */
@@ -224,6 +224,8 @@ static inline int notifier_to_errno(int ret)
 
 /* Virtual Terminal events are defined in include/linux/vt.h. */
 
+/* Panic notifiers are defined in include/linux/panic_notifier.h. */
+
 #define NETLINK_URELEASE   0x0001  /* Unicast netlink socket released */
 
 /* Console keyboard events.
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 01/11] ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths

[Guilherme G. Piccoli]

Currently the regular CPU shutdown path for ARM disables IRQs/FIQs
in the secondary CPUs - smp_send_stop() calls ipi_cpu_stop(), which
is responsible for that. IRQs are architecturally masked when we
take an interrupt, but FIQs are high priority than IRQs, hence they
aren't masked. With that said, it makes sense to disable FIQs here,
but there's no need for (re-)disabling IRQs.

More than that: there is an alternative path for disabling CPUs,
in the form of function crash_smp_send_stop(), which is used for
kexec/panic path. This function relies on a SMP call that also
triggers a busy-wait loop [at machine_crash_nonpanic_core()], but
without disabling FIQs. This might lead to odd scenarios, like
early interrupts in the boot of kexec'd kernel or even interrupts
in secondary "disabled" CPUs while the main one still works in the
panic path and assumes all secondary CPUs are (really!) off.

So, let's disable FIQs in both paths and *not* disable IRQs a second
time, since they are already masked in both paths by the architecture.
This way, we keep both CPU quiesce paths consistent and safe.

Cc: Marc Zyngier 
Cc: Michael Kelley 
Cc: Russell King 
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- No changes.

V2:
- Small wording improvement (thanks Michael Kelley);
- Only disable FIQs, since IRQs are masked by architecture
definition when we take an interrupt. Thanks a lot Russell
and Marc for the discussion [0].

Should we add a Fixes tag here? If so, maybe the proper target is:
b23065313297 ("ARM: 6522/1: kexec: Add call to non-crashing cores through IPI")

[0] https://lore.kernel.org/lkml/ymxcaqy6dwhoq...@shell.armlinux.org.uk/


 arch/arm/kernel/machine_kexec.c | 2 ++
 arch/arm/kernel/smp.c   | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index f567032a09c0..0b482bcb97f7 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -77,6 +77,8 @@ void machine_crash_nonpanic_core(void *unused)
 {
struct pt_regs regs;
 
+   local_fiq_disable();
+
crash_setup_regs(, get_irq_regs());
printk(KERN_DEBUG "CPU %u will stop doing anything useful since another 
CPU has crashed\n",
   smp_processor_id());
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 978db2d96b44..36e6efad89f3 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -600,6 +600,8 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
  */
 static void ipi_cpu_stop(unsigned int cpu)
 {
+   local_fiq_disable();
+
if (system_state <= SYSTEM_RUNNING) {
raw_spin_lock(_lock);
pr_crit("CPU%u: stopping\n", cpu);
@@ -609,9 +611,6 @@ static void ipi_cpu_stop(unsigned int cpu)
 
set_cpu_online(cpu, false);
 
-   local_fiq_disable();
-   local_irq_disable();
-
while (1) {
cpu_relax();
wfe();
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 06/11] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

Currently the tracing dump_on_oops feature is implemented through
separate notifiers, one for die/oops and the other for panic;
given they have the same functionality, let's unify them.

Also improve the function comment and change the priority of the
notifier to make it execute earlier, avoiding showing useless trace
data (like the callback names for the other notifiers); finally,
we also removed an unnecessary header inclusion.

Cc: Petr Mladek 
Cc: Sergei Shtylyov 
Cc: Steven Rostedt 
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- Removed goto usage, as per Steven suggestion (thanks!).

V2:
- Different approach; instead of using IDs to distinguish die and
panic events, rely on address comparison like other notifiers do
and as per Petr's suggestion;

- Removed ACK from Steven since the code changed.


 kernel/trace/trace.c | 55 ++--
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d3005279165d..0bacdbcb132f 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -19,7 +19,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -9776,40 +9775,40 @@ static __init int tracer_init_tracefs(void)
 
 fs_initcall(tracer_init_tracefs);
 
-static int trace_panic_handler(struct notifier_block *this,
-  unsigned long event, void *unused)
-{
-   if (ftrace_dump_on_oops)
-   ftrace_dump(ftrace_dump_on_oops);
-   return NOTIFY_OK;
-}
+static int trace_die_panic_handler(struct notifier_block *self,
+   unsigned long ev, void *unused);
 
 static struct notifier_block trace_panic_notifier = {
-   .notifier_call  = trace_panic_handler,
-   .next   = NULL,
-   .priority   = 150   /* priority: INT_MAX >= x >= 0 */
+   .notifier_call = trace_die_panic_handler,
+   .priority = INT_MAX - 1,
 };
 
-static int trace_die_handler(struct notifier_block *self,
-unsigned long val,
-void *data)
-{
-   switch (val) {
-   case DIE_OOPS:
-   if (ftrace_dump_on_oops)
-   ftrace_dump(ftrace_dump_on_oops);
-   break;
-   default:
-   break;
-   }
-   return NOTIFY_OK;
-}
-
 static struct notifier_block trace_die_notifier = {
-   .notifier_call = trace_die_handler,
-   .priority = 200
+   .notifier_call = trace_die_panic_handler,
+   .priority = INT_MAX - 1,
 };
 
+/*
+ * The idea is to execute the following die/panic callback early, in order
+ * to avoid showing irrelevant information in the trace (like other panic
+ * notifier functions); we are the 2nd to run, after hung_task/rcu_stall
+ * warnings get disabled (to prevent potential log flooding).
+ */
+static int trace_die_panic_handler(struct notifier_block *self,
+   unsigned long ev, void *unused)
+{
+   if (!ftrace_dump_on_oops)
+   return NOTIFY_DONE;
+
+   /* The die notifier requires DIE_OOPS to trigger */
+   if (self == _die_notifier && ev != DIE_OOPS)
+   return NOTIFY_DONE;
+
+   ftrace_dump(ftrace_dump_on_oops);
+
+   return NOTIFY_DONE;
+}
+
 /*
  * printk is set to max of 1024, we really don't need it that big.
  * Nothing should be printing 1000 characters anyway.
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 11/11] panic: Fixes the panic_print NMI backtrace setting

[Guilherme G. Piccoli]

Commit 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
panic_print")
introduced a setting for the "panic_print" kernel parameter to allow
users to request a NMI backtrace on panic. Problem is that the panic_print
handling happens after the secondary CPUs are already disabled, hence
this option ended-up being kind of a no-op - kernel skips the NMI trace
in idling CPUs, which is the case of offline CPUs.

Fix it by checking the NMI backtrace bit in the panic_print prior to
the CPU disabling function.

Fixes: 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
panic_print")
Cc: Feng Tang 
Cc: Petr Mladek 
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- No changes.

V2:
- new patch, there was no V1 of this one.

Hi folks, thanks upfront for reviews. This is a new patch, fixing an issue
I found in my tests, so I shoved it into this fixes series.

Notice that while at it, I got rid of the "crash_kexec_post_notifiers"
local copy in panic(). This was introduced by commit b26e27ddfd2a
("kexec: use core_param for crash_kexec_post_notifiers boot option"),
but it is not clear from comments or commit message why this local copy
is required.

My understanding is that it's a mechanism to prevent some concurrency,
in case some other CPU modify this variable while panic() is running.
I find it very unlikely, hence I removed it - but if people consider
this copy needed, I can respin this patch and keep it, even providing a
comment about that, in order to be explict about its need.

Let me know your thoughts! Cheers,

Guilherme


 kernel/panic.c | 47 +++
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index c6eb8f8db0c0..b025a8f21c17 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -180,9 +180,6 @@ static void panic_print_sys_info(bool console_flush)
return;
}
 
-   if (panic_print & PANIC_PRINT_ALL_CPU_BT)
-   trigger_all_cpu_backtrace();
-
if (panic_print & PANIC_PRINT_TASK_INFO)
show_state();
 
@@ -199,6 +196,30 @@ static void panic_print_sys_info(bool console_flush)
ftrace_dump(DUMP_ALL);
 }
 
+/*
+ * Helper that triggers the NMI backtrace (if set in panic_print)
+ * and then performs the secondary CPUs shutdown - we cannot have
+ * the NMI backtrace after the CPUs are off!
+ */
+static void panic_other_cpus_shutdown(void)
+{
+   if (panic_print & PANIC_PRINT_ALL_CPU_BT)
+   trigger_all_cpu_backtrace();
+
+   /*
+* Note that smp_send_stop() is the usual SMP shutdown function,
+* which unfortunately may not be hardened to work in a panic
+* situation. If we want to do crash dump after notifier calls
+* and kmsg_dump, we will need architecture dependent extra
+* bits in addition to stopping other CPUs, hence we rely on
+* crash_smp_send_stop() for that.
+*/
+   if (!crash_kexec_post_notifiers)
+   smp_send_stop();
+   else
+   crash_smp_send_stop();
+}
+
 /**
  * panic - halt the system
  * @fmt: The text string to print
@@ -214,7 +235,6 @@ void panic(const char *fmt, ...)
long i, i_next = 0, len;
int state = 0;
int old_cpu, this_cpu;
-   bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
 
if (panic_on_warn) {
/*
@@ -289,23 +309,10 @@ void panic(const char *fmt, ...)
 *
 * Bypass the panic_cpu check and call __crash_kexec directly.
 */
-   if (!_crash_kexec_post_notifiers) {
+   if (!crash_kexec_post_notifiers)
__crash_kexec(NULL);
 
-   /*
-* Note smp_send_stop is the usual smp shutdown function, which
-* unfortunately means it may not be hardened to work in a
-* panic situation.
-*/
-   smp_send_stop();
-   } else {
-   /*
-* If we want to do crash dump after notifier calls and
-* kmsg_dump, we will need architecture dependent extra
-* works in addition to stopping other CPUs.
-*/
-   crash_smp_send_stop();
-   }
+   panic_other_cpus_shutdown();
 
/*
 * Run any panic handlers, including those that might need to
@@ -326,7 +333,7 @@ void panic(const char *fmt, ...)
 *
 * Bypass the panic_cpu check and call __crash_kexec directly.
 */
-   if (_crash_kexec_post_notifiers)
+   if (crash_kexec_post_notifiers)
__crash_kexec(NULL);
 
 #ifdef CONFIG_VT
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 07/11] notifiers: Add tracepoints to the notifiers infrastructure

[Guilherme G. Piccoli]

Currently there is no way to show the callback names for registered,
unregistered or executed notifiers. This is very useful for debug
purposes, hence add this functionality here in the form of notifiers'
tracepoints, one per operation.

Cc: Arjan van de Ven 
Cc: Cong Wang 
Cc: Sebastian Andrzej Siewior 
Cc: Steven Rostedt 
Cc: Valentin Schneider 
Cc: Xiaoming Ni 
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- Yet another major change - thanks to Arjan's great suggestion,
refactored the code to make use of tracepoints instead of guarding
the output with a Kconfig debug setting.

V2:
- Major improvement thanks to the great idea from Xiaoming - changed
all the ksym wheel reinvention to printk %ps modifier;

- Instead of ifdefs, using IS_ENABLED() - thanks Steven.

- Removed an unlikely() hint on debug path.


 include/trace/events/notifiers.h | 69 
 kernel/notifier.c|  6 +++
 2 files changed, 75 insertions(+)
 create mode 100644 include/trace/events/notifiers.h

diff --git a/include/trace/events/notifiers.h b/include/trace/events/notifiers.h
new file mode 100644
index ..e8f30631aef5
--- /dev/null
+++ b/include/trace/events/notifiers.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM notifiers
+
+#if !defined(_TRACE_NOTIFIERS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_NOTIFIERS_H
+
+#include 
+
+DECLARE_EVENT_CLASS(notifiers_info,
+
+   TP_PROTO(void *cb),
+
+   TP_ARGS(cb),
+
+   TP_STRUCT__entry(
+   __field(void *, cb)
+   ),
+
+   TP_fast_assign(
+   __entry->cb = cb;
+   ),
+
+   TP_printk("%ps", __entry->cb)
+);
+
+/*
+ * notifiers_register - called upon notifier callback registration
+ *
+ * @cb:callback pointer
+ *
+ */
+DEFINE_EVENT(notifiers_info, notifiers_register,
+
+   TP_PROTO(void *cb),
+
+   TP_ARGS(cb)
+);
+
+/*
+ * notifiers_unregister - called upon notifier callback unregistration
+ *
+ * @cb:callback pointer
+ *
+ */
+DEFINE_EVENT(notifiers_info, notifiers_unregister,
+
+   TP_PROTO(void *cb),
+
+   TP_ARGS(cb)
+);
+
+/*
+ * notifiers_run - called upon notifier callback execution
+ *
+ * @cb:callback pointer
+ *
+ */
+DEFINE_EVENT(notifiers_info, notifiers_run,
+
+   TP_PROTO(void *cb),
+
+   TP_ARGS(cb)
+);
+
+#endif /* _TRACE_NOTIFIERS_H */
+
+/* This part must be outside protection */
+#include 
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 0d5bd62c480e..2f2783f59a31 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -7,6 +7,9 @@
 #include 
 #include 
 
+#define CREATE_TRACE_POINTS
+#include 
+
 /*
  * Notifier list for kernel code which wants to be called
  * at shutdown. This is used to stop any idling DMA operations
@@ -37,6 +40,7 @@ static int notifier_chain_register(struct notifier_block **nl,
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
+   trace_notifiers_register((void*)n->notifier_call);
return 0;
 }
 
@@ -46,6 +50,7 @@ static int notifier_chain_unregister(struct notifier_block 
**nl,
while ((*nl) != NULL) {
if ((*nl) == n) {
rcu_assign_pointer(*nl, n->next);
+   trace_notifiers_unregister((void*)n->notifier_call);
return 0;
}
nl = &((*nl)->next);
@@ -84,6 +89,7 @@ static int notifier_call_chain(struct notifier_block **nl,
continue;
}
 #endif
+   trace_notifiers_run((void*)nb->notifier_call);
ret = nb->notifier_call(nb, val, v);
 
if (nr_calls)
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 00/11] The panic notifiers refactor - fixes/clean-ups (V3)

[Guilherme G. Piccoli]

Hey everybody, this the third iteration of the panic notifiers
fixes/clean-ups;

V2 available at:
https://lore.kernel.org/lkml/20220719195325.402745-1-gpicc...@igalia.com/

V1 (including the refactor) available at:
https://lore.kernel.org/lkml/20220427224924.592546-1-gpicc...@igalia.com/


There wasn't much change here compared to V2 (the specifics are in the
patches), but a global change is that I've rebased against 6.0-rc1.
One patch got merged in -next, another one was re-submit in a standalone
format (requested by maintainer), so both of these are not here anymore.


As usual, tested 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 (QEMU guests and Steam Deck).

Here is the link with the .config files used:
https://people.igalia.com/gpiccoli/panic_notifiers_configs/6.0-rc1/


About the merge strategy: I've noticed there is a difference in maintainers
preferences (and my preference as well), so I see 3 strategies for merge:

(a) Maintainers pick patches that are good from the series and merge in
their trees;

(b) Some maintainer would pick the whole series and merge, at once, given
that everything is fine/ack/reviewed.

(c) I must re-send patches individually once they are reviewed/acked, as
standalone patches to the relevant maintainers, so they can merge it in
their trees.

I'm willing to do what's best for everybody - (a) is my choice when possible,
(b) seems to stall things and potentially cause conflicts, (c) seems to be
the compromise. I'll do that as per preference of the respective maintainers.


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

Guilherme


Guilherme G. Piccoli (11):
  ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths
  notifier: Add panic notifiers info and purge trailing whitespaces
  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
  notifiers: Add tracepoints to the notifiers infrastructure
  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/hv/ring_buffer.c |  13 
 drivers/hv/vmbus_drv.c   | 109 +++
 drivers/parisc/power.c   |  17 +++--
 drivers/video/fbdev/hyperv_fb.c  |  16 -
 include/linux/hyperv.h   |   2 +
 include/linux/notifier.h |   8 ++-
 include/trace/events/notifiers.h |  69 +++
 kernel/notifier.c|   6 ++
 kernel/panic.c   |  47 +++--
 kernel/trace/trace.c |  55 
 18 files changed, 302 insertions(+), 142 deletions(-)
 create mode 100644 include/trace/events/notifiers.h

-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 03/11] alpha: Clean-up the panic notifier code

[Guilherme G. Piccoli]

The alpha panic notifier has some code issues, not following
the conventions of other notifiers. Also, it might halt the
machine but still it is set to run as early as possible, which
doesn't seem to be a good idea.

So, let's clean the code and set the notifier to run as the
latest, following the same approach other architectures are
doing - also, remove the unnecessary include of a header already
included indirectly.

Cc: Ivan Kokshaysky 
Cc: Matt Turner 
Cc: Richard Henderson 
Reviewed-by: Petr Mladek 
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- No changes.

V2:
- Fixed rth email address;
- Added Petr's review tag - thanks!


 arch/alpha/kernel/setup.c | 36 +++-
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index b4fbbba30aa2..d88bdf852753 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -41,19 +41,11 @@
 #include 
 #include 
 #endif
-#include 
 #include 
 #include 
 #include 
 #include 
 
-static int alpha_panic_event(struct notifier_block *, unsigned long, void *);
-static struct notifier_block alpha_panic_block = {
-   alpha_panic_event,
-NULL,
-INT_MAX /* try to do it first */
-};
-
 #include 
 #include 
 #include 
@@ -435,6 +427,21 @@ static const struct sysrq_key_op srm_sysrq_reboot_op = {
 };
 #endif
 
+static int alpha_panic_event(struct notifier_block *this,
+unsigned long event, void *ptr)
+{
+   /* If we are using SRM and serial console, just hard halt here. */
+   if (alpha_using_srm && srmcons_output)
+   __halt();
+
+   return NOTIFY_DONE;
+}
+
+static struct notifier_block alpha_panic_block = {
+   .notifier_call = alpha_panic_event,
+   .priority = INT_MIN, /* may not return, do it last */
+};
+
 void __init
 setup_arch(char **cmdline_p)
 {
@@ -1427,19 +1434,6 @@ const struct seq_operations cpuinfo_op = {
.show   = show_cpuinfo,
 };
 
-
-static int
-alpha_panic_event(struct notifier_block *this, unsigned long event, void *ptr)
-{
-#if 1
-   /* FIXME FIXME FIXME */
-   /* If we are using SRM and serial console, just hard halt here. */
-   if (alpha_using_srm && srmcons_output)
-   __halt();
-#endif
-return NOTIFY_DONE;
-}
-
 static __init int add_pcspkr(void)
 {
struct platform_device *pd;
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 10/11] drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers

[Guilherme G. Piccoli]

Currently Hyper-V guests are among the most relevant users of the panic
infrastructure, like panic notifiers, kmsg dumpers, etc. The reasons rely
both in cleaning-up procedures (closing hypervisor <-> guest connection,
disabling some paravirtualized timer) as well as to data collection
(sending panic information to the hypervisor) and framebuffer management.

The thing is: some notifiers are related to others, ordering matters, some
functionalities are duplicated and there are lots of conditionals behind
sending panic information to the hypervisor. As part of an effort to
clean-up the panic notifiers mechanism and better document things, we
hereby address some of the issues/complexities of Hyper-V panic handling
through the following changes:

(a) We have die and panic notifiers on vmbus_drv.c and both have goals of
sending panic information to the hypervisor, though the panic notifier is
also responsible for a cleaning-up procedure.

This commit clears the code by splitting the panic notifier in two, one
for closing the vmbus connection whereas the other is only for sending
panic info to hypervisor. With that, it was possible to merge the die and
panic notifiers in a single/well-documented function, and clear some
conditional complexities on sending such information to the hypervisor.

(b) There is a Hyper-V framebuffer panic notifier, which relies in doing
a vmbus operation that demands a valid connection. So, we must order this
notifier with the panic notifier from vmbus_drv.c, to guarantee that the
framebuffer code executes before the vmbus connection is unloaded.

Also, this commit removes a useless header.

Although there is code rework and re-ordering, we expect that this change
has no functional regressions but instead optimize the path and increase
panic reliability on Hyper-V. This was tested on Hyper-V with success.

Cc: Andrea Parri (Microsoft) 
Cc: Dexuan Cui 
Cc: Haiyang Zhang 
Cc: "K. Y. Srinivasan" 
Cc: Petr Mladek 
Cc: Stephen Hemminger 
Cc: Tianyu Lan 
Cc: Wei Liu 
Reviewed-by: Michael Kelley 
Tested-by: Fabio A M Martins 
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- Added Michael's review tag - thanks!

V2:
- Unfortunately we cannot rely in the crash shutdown (custom) handler
to perform the vmbus unload - arm64 architecture doesn't have this
"feature" [0]. So, in V2 we kept the notifier behavior and always
unload the vmbus connection, no matter what - thanks Michael for
pointing that;

- Removed the Fixes tags as per Michael suggestion;

- As per Petr suggestion, we abandoned the idea of distinguish among
notifiers using an id - so, in V2 we rely in the old and good address
comparison for that. Thanks Petr for the enriching discussion!

[0] 
https://lore.kernel.org/lkml/427a8277-49f0-4317-d6c3-4a15d7070...@igalia.com/


 drivers/hv/vmbus_drv.c  | 109 +++-
 drivers/video/fbdev/hyperv_fb.c |   8 +++
 2 files changed, 74 insertions(+), 43 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 23c680d1a0f5..0a77e2bb0b70 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -25,7 +25,6 @@
 #include 
 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -69,53 +68,74 @@ static int hyperv_report_reg(void)
return !sysctl_record_panic_msg || !hv_panic_page;
 }
 
-static int hyperv_panic_event(struct notifier_block *nb, unsigned long val,
+/*
+ * The panic notifier below is responsible solely for unloading the
+ * vmbus connection, which is necessary in a panic event.
+ *
+ * Notice an intrincate relation of this notifier with Hyper-V
+ * framebuffer panic notifier exists - we need vmbus connection alive
+ * there in order to succeed, so we need to order both with each other
+ * [see hvfb_on_panic()] - this is done using notifiers' priorities.
+ */
+static int hv_panic_vmbus_unload(struct notifier_block *nb, unsigned long val,
  void *args)
+{
+   vmbus_initiate_unload(true);
+   return NOTIFY_DONE;
+}
+static struct notifier_block hyperv_panic_vmbus_unload_block = {
+   .notifier_call  = hv_panic_vmbus_unload,
+   .priority   = INT_MIN + 1, /* almost the latest one to execute */
+};
+
+static int hv_die_panic_notify_crash(struct notifier_block *self,
+unsigned long val, void *args);
+
+static struct notifier_block hyperv_die_report_block = {
+   .notifier_call = hv_die_panic_notify_crash,
+};
+static struct notifier_block hyperv_panic_report_block = {
+   .notifier_call = hv_die_panic_notify_crash,
+};
+
+/*
+ * The following callback works both as die and panic notifier; its
+ * goal is to provide panic information to the hypervisor unless the
+ * kmsg dumper is used [see hv_kmsg_dump()], which provides more
+ * information but isn't always available.
+ *
+ * Notice that both the panic/die report notifiers are registered only
+ * if we have the capability HV_FEATURE_GUEST_CRASH_MSR_A

[PATCH V3 04/11] um: Improve panic notifiers consistency and ordering

[Guilherme G. Piccoli]

Currently the panic notifiers from user mode linux don't follow
the convention for most of the other notifiers present in the
kernel (indentation, priority setting, numeric return).
More important, the priorities could be improved, since it's a
special case (userspace), hence we could run the notifiers earlier;
user mode linux shouldn't care much with other panic notifiers but
the ordering among the mconsole and arch notifier is important,
given that the arch one effectively triggers a core dump.

Fix that by running the mconsole notifier as the first panic
notifier, followed by the architecture one (that coredumps).

Cc: Anton Ivanov 
Cc: Johannes Berg 
Cc: Richard Weinberger 
Signed-off-by: Guilherme G. Piccoli 

V3:
- No changes.

V2:
- Kept the notifier header to avoid implicit usage - thanks
Johannes for the suggestion!
---
 arch/um/drivers/mconsole_kern.c | 7 +++
 arch/um/kernel/um_arch.c| 8 
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index 8ca67a692683..69af3ce8407a 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -846,13 +846,12 @@ static int notify_panic(struct notifier_block *self, 
unsigned long unused1,
 
mconsole_notify(notify_socket, MCONSOLE_PANIC, message,
strlen(message) + 1);
-   return 0;
+   return NOTIFY_DONE;
 }
 
 static struct notifier_block panic_exit_notifier = {
-   .notifier_call  = notify_panic,
-   .next   = NULL,
-   .priority   = 1
+   .notifier_call  = notify_panic,
+   .priority   = INT_MAX, /* run as soon as possible */
 };
 
 static int add_notifier(void)
diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index e0de60e503b9..ae272878f692 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -247,13 +247,13 @@ static int panic_exit(struct notifier_block *self, 
unsigned long unused1,
bust_spinlocks(0);
uml_exitcode = 1;
os_dump_core();
-   return 0;
+
+   return NOTIFY_DONE;
 }
 
 static struct notifier_block panic_exit_notifier = {
-   .notifier_call  = panic_exit,
-   .next   = NULL,
-   .priority   = 0
+   .notifier_call  = panic_exit,
+   .priority   = INT_MAX - 1, /* run as 2nd notifier, won't return */
 };
 
 void uml_finishsetup(void)
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 05/11] parisc: Replace regular spinlock with spin_trylock on panic path

[Guilherme G. Piccoli]

The panic notifiers' callbacks execute in an atomic context, with
interrupts/preemption disabled, and all CPUs not running the panic
function are off, so it's very dangerous to wait on a regular
spinlock, there's a risk of deadlock.

Refactor the panic notifier of parisc/power driver to make use
of spin_trylock - for that, we've added a second version of the
soft-power function. Also, some comments were reorganized and
trailing white spaces, useless header inclusion and blank lines
were removed.

Cc: "James E.J. Bottomley" 
Cc: Jeroen Roovers 
Acked-by: Helge Deller  # parisc
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- s/in/on as per Jeroen's suggestion - thanks!

V2:
- Added Helge's ACK - thanks!


 arch/parisc/include/asm/pdc.h |  1 +
 arch/parisc/kernel/firmware.c | 27 +++
 drivers/parisc/power.c| 17 ++---
 3 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/arch/parisc/include/asm/pdc.h b/arch/parisc/include/asm/pdc.h
index b643092d4b98..7a106008e258 100644
--- a/arch/parisc/include/asm/pdc.h
+++ b/arch/parisc/include/asm/pdc.h
@@ -83,6 +83,7 @@ int pdc_do_firm_test_reset(unsigned long ftc_bitmap);
 int pdc_do_reset(void);
 int pdc_soft_power_info(unsigned long *power_reg);
 int pdc_soft_power_button(int sw_control);
+int pdc_soft_power_button_panic(int sw_control);
 void pdc_io_reset(void);
 void pdc_io_reset_devices(void);
 int pdc_iodc_getc(void);
diff --git a/arch/parisc/kernel/firmware.c b/arch/parisc/kernel/firmware.c
index 6a7e315bcc2e..3b1f7641e3c9 100644
--- a/arch/parisc/kernel/firmware.c
+++ b/arch/parisc/kernel/firmware.c
@@ -1232,15 +1232,18 @@ int __init pdc_soft_power_info(unsigned long *power_reg)
 }
 
 /*
- * pdc_soft_power_button - Control the soft power button behaviour
- * @sw_control: 0 for hardware control, 1 for software control 
+ * pdc_soft_power_button{_panic} - Control the soft power button behaviour
+ * @sw_control: 0 for hardware control, 1 for software control
  *
  *
  * This PDC function places the soft power button under software or
  * hardware control.
- * Under software control the OS may control to when to allow to shut 
- * down the system. Under hardware control pressing the power button 
+ * Under software control the OS may control to when to allow to shut
+ * down the system. Under hardware control pressing the power button
  * powers off the system immediately.
+ *
+ * The _panic version relies on spin_trylock to prevent deadlock
+ * on panic path.
  */
 int pdc_soft_power_button(int sw_control)
 {
@@ -1254,6 +1257,22 @@ int pdc_soft_power_button(int sw_control)
return retval;
 }
 
+int pdc_soft_power_button_panic(int sw_control)
+{
+   int retval;
+   unsigned long flags;
+
+   if (!spin_trylock_irqsave(_lock, flags)) {
+   pr_emerg("Couldn't enable soft power button\n");
+   return -EBUSY; /* ignored by the panic notifier */
+   }
+
+   retval = mem_pdc_call(PDC_SOFT_POWER, PDC_SOFT_POWER_ENABLE, 
__pa(pdc_result), sw_control);
+   spin_unlock_irqrestore(_lock, flags);
+
+   return retval;
+}
+
 /*
  * pdc_io_reset - Hack to avoid overlapping range registers of Bridges devices.
  * Primarily a problem on T600 (which parisc-linux doesn't support) but
diff --git a/drivers/parisc/power.c b/drivers/parisc/power.c
index 456776bd8ee6..8512884de2cf 100644
--- a/drivers/parisc/power.c
+++ b/drivers/parisc/power.c
@@ -37,7 +37,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -175,16 +174,21 @@ static void powerfail_interrupt(int code, void *x)
 
 
 
-/* parisc_panic_event() is called by the panic handler.
- * As soon as a panic occurs, our tasklets above will not be
- * executed any longer. This function then re-enables the 
- * soft-power switch and allows the user to switch off the system
+/*
+ * parisc_panic_event() is called by the panic handler.
+ *
+ * As soon as a panic occurs, our tasklets above will not
+ * be executed any longer. This function then re-enables
+ * the soft-power switch and allows the user to switch off
+ * the system. We rely in pdc_soft_power_button_panic()
+ * since this version spin_trylocks (instead of regular
+ * spinlock), preventing deadlocks on panic path.
  */
 static int parisc_panic_event(struct notifier_block *this,
unsigned long event, void *ptr)
 {
/* re-enable the soft-power switch */
-   pdc_soft_power_button(0);
+   pdc_soft_power_button_panic(0);
return NOTIFY_DONE;
 }
 
@@ -193,7 +197,6 @@ static struct notifier_block parisc_panic_block = {
.priority   = INT_MAX,
 };
 
-
 static int __init power_init(void)
 {
unsigned long ret;
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH V3 08/11] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

The altera_edac panic notifier performs some data collection with
regards errors detected; such code relies in the regmap layer to
perform reads/writes, so the code is abstracted and there is some
risk level to execute that, since the panic path runs in atomic
context, with interrupts/preemption and secondary CPUs disabled.

Users want the information collected in this panic notifier though,
so in order to balance the risk/benefit, let's skip the altera panic
notifier if kdump is loaded. While at it, remove a useless header
and encompass a macro inside the sole ifdef block it is used.

Cc: Borislav Petkov 
Cc: Petr Mladek 
Cc: Tony Luck 
Acked-by: Dinh Nguyen 
Signed-off-by: Guilherme G. Piccoli 

---

V3:
- added the ack tag from Dinh - thanks!
- had a good discussion with Boris about that in V2 [0],
hopefully we can continue and reach a consensus in this V3.
[0] 
https://lore.kernel.org/lkml/46137c67-25b4-6657-33b7-cffdc7afc...@igalia.com/

V2:
- new patch, based on the discussion in [1].
[1] 
https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/


 drivers/edac/altera_edac.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index e7e8e624a436..741fe5539154 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -16,7 +16,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -24,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "altera_edac.h"
@@ -2063,22 +2063,30 @@ static const struct irq_domain_ops a10_eccmgr_ic_ops = {
 };
 
 /** Stratix 10 EDAC Double Bit Error Handler /
-#define to_a10edac(p, m) container_of(p, struct altr_arria10_edac, m)
-
 #ifdef CONFIG_64BIT
 /* panic routine issues reboot on non-zero panic_timeout */
 extern int panic_timeout;
 
+#define to_a10edac(p, m) container_of(p, struct altr_arria10_edac, m)
+
 /*
  * The double bit error is handled through SError which is fatal. This is
  * called as a panic notifier to printout ECC error info as part of the panic.
+ *
+ * Notice that if kdump is set, we take the risk avoidance approach and
+ * skip the notifier, given that users are expected to have access to a
+ * full vmcore.
  */
 static int s10_edac_dberr_handler(struct notifier_block *this,
  unsigned long event, void *ptr)
 {
-   struct altr_arria10_edac *edac = to_a10edac(this, panic_notifier);
+   struct altr_arria10_edac *edac;
int err_addr, dberror;
 
+   if (kexec_crash_loaded())
+   return NOTIFY_DONE;
+
+   edac = to_a10edac(this, panic_notifier);
regmap_read(edac->ecc_mgr_map, S10_SYSMGR_ECC_INTSTAT_DERR_OFST,
);
regmap_write(edac->ecc_mgr_map, S10_SYSMGR_UE_VAL_OFST, dberror);
-- 
2.37.2


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 10/13] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 17/08/2022 19:19, Borislav Petkov wrote:
> On Wed, Aug 17, 2022 at 07:09:26PM -0300, Guilherme G. Piccoli wrote:
>> Again - a matter of a trade-off, a good compromise must be agreed by all
>> parties (kdump maintainers are usually extremely afraid of taking risks
>> to not break kdump).
> 
> Right, and logging hw errors from a panic notifier feels simply weird.
> 
> x86 has its own, special notifier exactly for that and it is independent
> from the panic path but it gets run right after the exception raised due
> to the hw error is done.
> 
> Dunno if ARM has such facilities tho...
> 
> Thx.
> 

Yeah, MCE stuff right? Not sure for ARM and other architectures as well.
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 10/13] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 17/08/2022 14:31, Borislav Petkov wrote:
> [...]
> 
> How does the fact that kdump is loaded, obviate the need to print
> information about the errors?
> 
> Are you suggesting that people who have the whole vmcore would be able
> to piece together the error information?
> 

Hi Boris, thanks for chiming in.

So, this is part of an effort to clean-up the panic path. Currently, if
a kdump happens, *all* the panic notifiers are skipped by default,
including this one. In this scenario, this patch seems like a no-op.

But happens that in the refactor we are proposing [0], some notifiers
should run before the kdump. We are basically putting some ordering in
the way notifiers are executed, while documenting this properly and with
the goal of not increasing the failure risk for kdump.

This patch is useful so we can bring the altera EDAC notifier to run
earlier while not increasing the risk on kdump - this operation is a bit
"delicate" to happen in the panic scenario. The origin of this patch was
a discussion with Tony/Peter [1], guess we can call it a "compromise
solution".

Let me know if you disagree or have more questions, and in case you'd
like to check/participate in the whole panic notifiers refactor
discussion, it would be great =)
Cheers,


Guilherme


[0]
https://lore.kernel.org/lkml/20220427224924.592546-1-gpicc...@igalia.com/

[1]
https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 10/13] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 17/08/2022 19:00, Borislav Petkov wrote:
> On Wed, Aug 17, 2022 at 06:56:11PM -0300, Guilherme G. Piccoli wrote:
>> But do you agree that currently, in case of a kdump, that information
>> *is not collected*, with our without my patch?
> 
> If for some reason that panic notifier does not get run before kdump,
> then that's a bug and that driver should not use a panic notifier to log
> hw errors in the first place.
> 

Indeed, the notifiers don't run before kdump by default, in a conscious
decision of the kdump maintainers.

You might be right, in the sense that maybe the edac error handler
shouldn't run as a panic notifier. Let's see if Tony / Dinh can chime in
on that discussion - we could move it to run in the kexec event as well,
so it'd always run before a kdump, but maybe the risk it offers during
panic time is not worth.

Again - a matter of a trade-off, a good compromise must be agreed by all
parties (kdump maintainers are usually extremely afraid of taking risks
to not break kdump).

Cheers!

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 10/13] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 17/08/2022 18:46, Borislav Petkov wrote:
> On Wed, Aug 17, 2022 at 06:39:07PM -0300, Guilherme G. Piccoli wrote:
>> Sorry for the confusion, let me try to be a bit more clear:
> 
> I think you're missing the point. Lemme try again:
> 
> You *absolutely* must log those errors because they're important. It
> doesn't matter if this is done in a panic notifier and you're changing
> that whole shebang or through some other magic.
> 
> If you do, then this driver needs to *still* *log* those fatal errors -
> regardless through a panic notifier or some novel contraption it wants
> to use.
> 
> So if you want to change the panic notifiers, you *must* make sure those
> errors still do get logged.
> 
> Better?
> 

Boris, I understand the importance of the logs, for sure!

But do you agree that currently, in case of a kdump, that information
*is not collected*, with our without my patch?

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 10/13] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 17/08/2022 18:02, Borislav Petkov wrote:
> On Wed, Aug 17, 2022 at 05:28:34PM -0300, Guilherme G. Piccoli wrote:
>> My understanding is the same as yours, i.e., this is not possible to
>> collect from vmcore, it requires register reading. But again: if you
>> kdump your machine today, you won't collect this information, patch
>> changed nothing in that regard.
> 
> Why won't you be able to collect it? You can certainly access dmesg in
> the vmcore and see those errors logged there.

Sorry for the confusion, let me try to be a bit more clear:

(1) if we kdump but we *didn't run* s10_edac_dberr_handler() before
kdump, the information is lost, since s10_edac_dberr_handler() performs
register readings. That information is not contained inside the vmcore.

(2) If for some reason the function s10_edac_dberr_handler() *was
executed prior to kdump*, of course the registers information would be
on dmesg, easy to collect in the vmcore.

Makes sense?


> 
>> The one thing it changes is that you'd skip the altera register dump if
>> kdump is set AND you managed to also set "crash_kexec_post_notifiers".
> 
> What your patch changes is, it prevents s10_edac_dberr_handler() from
> logging potentially important fatal hw errors when kdump is loaded.

Agreed. If kdump is loaded, we cannot log that information (modulo that
we do not collect it today by default on kdump as well).
The other part of story (the reason of the patch) is that we plan to
start running this panic notifier a bit earlier, being able to collect
such edac logs with pstore, for example.


> 
> If Dinh is fine with that, I'll take the patch. But it looks like a bad
> idea to me.
> 

I think we should seek what the majority of the folks consider the best,
in order to converge to some well-accepted solution. I'm completely OK
in dropping this one and rework with some other idea, or we can leave
this panic notifier as is, continue running that a bit later.

Tony / Petr (when back), suggestions are welcome =)
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 10/13] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 17/08/2022 16:34, Borislav Petkov wrote:
> [...]
> 
> What is "the failure risk for kdump"?
> 
> Some of the notifiers which run before kdump might fail and thus prevent
> the machine from kdumping?
>

Exactly; some notifiers could break the machine and prevent a successful
kdump. The EDAC one is consider medium risk, due to invasive operations
(register readings on panic situation).


> [...] 
> My question stands: if kdump is loaded and the s10_edac_dberr_handler()
> does not read the the fatal errors and they don't get shown in dmesg
> before the machine panics, how do you intend to show that information to
> the user?
> 
> Because fatal errors are something you absolutely wanna show, at least,
> in dmesg!
> 
> I don't think you can "read" the errors from vmcore - they need to be
> read from the hw registers before the machine dies.
> 

My understanding is the same as yours, i.e., this is not possible to
collect from vmcore, it requires register reading. But again: if you
kdump your machine today, you won't collect this information, patch
changed nothing in that regard.

The one thing it changes is that you'd skip the altera register dump if
kdump is set AND you managed to also set "crash_kexec_post_notifiers".

In case you / Dinh / Tony disagrees with the patch, it's fine and we can
discard it, but then this notifier couldn't run early in the refactor we
are doing, it'd postponed to run later. This are is full of trade-offs,
we just need to choose what compromise solution is preferred by the
majority of developers =)

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 08/13] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 16/08/2022 12:52, Steven Rostedt wrote:
> On Tue, 16 Aug 2022 10:57:20 -0400
> Alan Stern  wrote:
> 
>>> static int trace_die_panic_handler(struct notifier_block *self,
>>> unsigned long ev, void *unused)
>>> {
>>> if (!ftrace_dump_on_oops)
>>> return NOTIFY_DONE;
>>>
>>> /* The die notifier requires DIE_OOPS to trigger */
>>> if (self == _die_notifier && ev != DIE_OOPS)
>>> return NOTIFY_DONE;
>>>
>>> ftrace_dump(ftrace_dump_on_oops);
>>>
>>> return NOTIFY_DONE;
>>> }  
>>
>> Or better yet:
>>
>>  if (ftrace_dump_on_oops) {
>>
>>  /* The die notifier requires DIE_OOPS to trigger */
>>  if (self != _die_notifier || ev == DIE_OOPS)
>>  ftrace_dump(ftrace_dump_on_oops);
>>  }
>>  return NOTIFY_DONE;
>>
> 
> That may be more consolidated but less easy to read and follow. This is far
> from a fast path.
> 
> As I maintain this bike-shed, I prefer the one I suggested ;-)
> 
> -- Steve

Perfect Steve and Alan, appreciate your suggestions!
I'll submit V3 using your change Steve - honestly, I'm not sure why in
the heck I put a goto there, yours is basically the same code, modulo
the goto heheh

A braino from me, for sure!
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 10/13] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 16/08/2022 15:44, Dinh Nguyen wrote:
> [...] 
>> V2:
>> - new patch, based on the discussion in [0].
>> [0] 
>> https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/
>>
> 
> Acked-by: Dinh Nguyen 

Thanks a lot Dinh!

There is something I'm asking for maintainers on patches in this series,
so here it goes for you (CC Borislav / Tony): do you think it's possible
to pick this one in your tree directly, from this series, or do you
prefer I re-submit as a standalone patch, on linux-edac maybe?

Thanks,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 06/13] um: Improve panic notifiers consistency and ordering

[Guilherme G. Piccoli]

On 09/08/2022 16:08, Johannes Berg wrote:
> [...]
>> Perfect, thank you! Let me take the opportunity to ask you something I'm
>> asking all the maintainers involved here - do you prefer taking the
>> patch through your tree, or to get it landed with the whole series, at
>> once, from some maintainer?
>>
> Hm. I don't think we'd really care, but so far I was thinking - since
> it's a series - it'd go through some appropriate tree all together. If
> you think it should be applied separately, let us know.
> 
> johannes

For me, it would be easier if maintainers pick the patches into their
-next/-fixes trees when they think the patch is good enough, but some
maintainers complained that prefer the whole series approach (and some
others are already taking the patches into their trees).

Given that, in case you do have a linux-um tree and feel OK with that, I
appreciate if you merge it, so I can remove the patch in next iteration.
If you prefer the whole series approach, OK as well, your call =)

Thanks,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 06/13] um: Improve panic notifiers consistency and ordering

[Guilherme G. Piccoli]

On 09/08/2022 15:09, Johannes Berg wrote:
> [...]
>>> V2:
>>> - Kept the notifier header to avoid implicit usage - thanks
>>> Johannes for the suggestion!
>>>
>>>  arch/um/drivers/mconsole_kern.c | 7 +++
>>>  arch/um/kernel/um_arch.c| 8 
>>>  2 files changed, 7 insertions(+), 8 deletions(-)
>>> [...]
>>
>> Hi Johannes, do you feel this one is good now, after your last review?
>> Thanks in advance,
>>
> 
> Yeah, no objections, my previous comment was just a minor almost style
> issue anyway.
> 
> johannes

Perfect, thank you! Let me take the opportunity to ask you something I'm
asking all the maintainers involved here - do you prefer taking the
patch through your tree, or to get it landed with the whole series, at
once, from some maintainer?

Cheers!

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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
>  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 
>> Cc: David Gow 
>> Cc: Evan Green 
>> Cc: Julius Werner 
>> Signed-off-by: Guilherme G. Piccoli 
> 
> Reviewed-by: Evan Green 

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

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 10/13] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

On 19/07/2022 16:53, Guilherme G. Piccoli wrote:
> The altera_edac panic notifier performs some data collection with
> regards errors detected; such code relies in the regmap layer to
> perform reads/writes, so the code is abstracted and there is some
> risk level to execute that, since the panic path runs in atomic
> context, with interrupts/preemption and secondary CPUs disabled.
> 
> Users want the information collected in this panic notifier though,
> so in order to balance the risk/benefit, let's skip the altera panic
> notifier if kdump is loaded. While at it, remove a useless header
> and encompass a macro inside the sole ifdef block it is used.
> 
> Cc: Dinh Nguyen 
> Cc: Petr Mladek 
> Cc: Tony Luck 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V2:
> - new patch, based on the discussion in [0].
> [0] 
> https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/
> 
>  drivers/edac/altera_edac.c | 16 
>  1 file changed, 12 insertions(+), 4 deletions(-)
> [...]

Hey Tony / Dinh, do you think this patch is good, based on the
discussion we had [0] last iteration?

Thanks in advance,


Guilherme


[0]
https://lore.kernel.org/lkml/599b72f6-76a4-8e6d-5432-56fb1ffd7...@igalia.com/

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 08/13] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 19/07/2022 16:53, Guilherme G. Piccoli wrote:
> Currently the tracing dump_on_oops feature is implemented
> through separate notifiers, one for die/oops and the other
> for panic - given they have the same functionality, let's
> unify them.
> 
> Also improve the function comment and change the priority of
> the notifier to make it execute earlier, avoiding showing useless
> trace data (like the callback names for the other notifiers);
> finally, we also removed an unnecessary header inclusion.
> 
> Cc: Petr Mladek 
> Cc: Sergei Shtylyov 
> Cc: Steven Rostedt 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V2:
> - Different approach; instead of using IDs to distinguish die and
> panic events, rely on address comparison like other notifiers do
> and as per Petr's suggestion;
> 
> - Removed ACK from Steven since the code changed.
> 
>  kernel/trace/trace.c | 55 ++--
>  1 file changed, 27 insertions(+), 28 deletions(-)
> [...]

Hi Sergei / Steve, do you think this version is good now, after your
last round of reviews?

Thanks,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 06/13] um: Improve panic notifiers consistency and ordering

[Guilherme G. Piccoli]

On 19/07/2022 16:53, Guilherme G. Piccoli wrote:
> Currently the panic notifiers from user mode linux don't follow
> the convention for most of the other notifiers present in the
> kernel (indentation, priority setting, numeric return).
> More important, the priorities could be improved, since it's a
> special case (userspace), hence we could run the notifiers earlier;
> user mode linux shouldn't care much with other panic notifiers but
> the ordering among the mconsole and arch notifier is important,
> given that the arch one effectively triggers a core dump.
> 
> Fix that by running the mconsole notifier as the first panic
> notifier, followed by the architecture one (that coredumps).
> 
> Cc: Anton Ivanov 
> Cc: Johannes Berg 
> Cc: Richard Weinberger 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V2:
> - Kept the notifier header to avoid implicit usage - thanks
> Johannes for the suggestion!
> 
>  arch/um/drivers/mconsole_kern.c | 7 +++
>  arch/um/kernel/um_arch.c| 8 
>  2 files changed, 7 insertions(+), 8 deletions(-)
> [...]

Hi Johannes, do you feel this one is good now, after your last review?
Thanks in advance,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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 
> Cc: David Gow 
> Cc: Evan Green 
> Cc: Julius Werner 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> 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

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 01/13] ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths

[Guilherme G. Piccoli]

On 19/07/2022 16:53, Guilherme G. Piccoli wrote:
> Currently the regular CPU shutdown path for ARM disables IRQs/FIQs
> in the secondary CPUs - smp_send_stop() calls ipi_cpu_stop(), which
> is responsible for that. IRQs are architecturally masked when we
> take an interrupt, but FIQs are high priority than IRQs, hence they
> aren't masked. With that said, it makes sense to disable FIQs here,
> but there's no need for (re-)disabling IRQs.
> 
> More than that: there is an alternative path for disabling CPUs,
> in the form of function crash_smp_send_stop(), which is used for
> kexec/panic path. This function relies on a SMP call that also
> triggers a busy-wait loop [at machine_crash_nonpanic_core()], but
> without disabling FIQs. This might lead to odd scenarios, like
> early interrupts in the boot of kexec'd kernel or even interrupts
> in secondary "disabled" CPUs while the main one still works in the
> panic path and assumes all secondary CPUs are (really!) off.
> 
> So, let's disable FIQs in both paths and *not* disable IRQs a second
> time, since they are already masked in both paths by the architecture.
> This way, we keep both CPU quiesce paths consistent and safe.
> 
> Cc: Marc Zyngier 
> Cc: Michael Kelley 
> Cc: Russell King 
> Signed-off-by: Guilherme G. Piccoli 
> 
> ---
> 
> V2:
> - Small wording improvement (thanks Michael Kelley);
> - Only disable FIQs, since IRQs are masked by architecture
> definition when we take an interrupt. Thanks a lot Russell
> and Marc for the discussion [0].
> 
> Should we add a Fixes tag here? If so, maybe the proper target is:
> b23065313297 ("ARM: 6522/1: kexec: Add call to non-crashing cores through 
> IPI")
> 
> [0] https://lore.kernel.org/lkml/ymxcaqy6dwhoq...@shell.armlinux.org.uk/
> 
>  arch/arm/kernel/machine_kexec.c | 2 ++
>  arch/arm/kernel/smp.c   | 5 ++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
> [...]

Hi Mark / Russell, do you think this one is good enough or is there room
for improvement?

Appreciate the reviews!
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 08/13] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

On 03/08/2022 06:52, Baoquan He wrote:
> [...]
>>
>> Although the switch-case code of original trace_die_handler() is werid, 
>> this unification is not much more comfortable. Just personal feeling
>> from code style, not strong opinion. Leave it to trace reviewers.
> 
> Please ignore this comment.
> 
> I use b4 to grab this patchset and applied, and started to check patch
> one by one. Then I realize it's all about cleanups which have got
> consensus in earlier rounds. Hope it can be merged when other people's
> concern is addressed, the whole series looks good to me, I have no
> strong concern to them.
> 

Thanks a lot for your reviews Baoquan, much appreciated =)

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 11/13] video/hyperv_fb: Avoid taking busy spinlock on panic path

[Guilherme G. Piccoli]

On 25/07/2022 15:09, Michael Kelley (LINUX) wrote:
> [...]
>> +bool hv_ringbuffer_spinlock_busy(struct vmbus_channel *channel)
>> +{
>> +struct hv_ring_buffer_info *rinfo = >outbound;
>> +
>> +if (spin_is_locked(>ring_lock))
>> +return true;
>> +
>> +return false;
> 
> Could simplify the code as just:
> 
>   return spin_is_locked(>ring_lock);
> 

Sure, makes sense! Thanks for the suggestion, I'll do that for V3.
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 07/13] parisc: Replace regular spinlock with spin_trylock on panic path

[Guilherme G. Piccoli]

On 21/07/2022 10:45, Helge Deller wrote:
> [...]
> Guilherme, I'd really prefer that you push the whole series at once through
> some generic tree.
> 
> Helge

Hmm..OK.

Some maintainers will take patches from here and merge, but given your
preference I can talk to Andrew to see if the can pick via his tree
(along with the generic panic patches).

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 07/13] parisc: Replace regular spinlock with spin_trylock on panic path

[Guilherme G. Piccoli]

On 19/07/2022 22:43, Jeroen Roovers wrote:
>  Hi Guilherme,
> [...] 
>> + *
>> + * The _panic version relies in spin_trylock to prevent deadlock
>> + * on panic path.
> 
> in => on
> 

Hi Jer, thanks for the suggestion!

Helge, do you think you could fix it when applying, if there's no other
issue in the patch?
Thanks,


Guilherme


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 09/13] notifier: Show function names on notifier routines if DEBUG_NOTIFIERS is set

[Guilherme G. Piccoli]

On 19/07/2022 19:04, Arjan van de Ven wrote:
> On 7/19/2022 2:00 PM, Guilherme G. Piccoli wrote:
>> On 19/07/2022 17:48, Arjan van de Ven wrote:
>>> [...]
>>> I would totally support an approach where instead of pr_info, there's a 
>>> tracepoint
>>> for these events (and that shouldnt' need to be conditional on a config 
>>> option)
>>>
>>> that's not what the patch does though.
>>
>> This is a good idea Arjan! We could use trace events or pr_debug() -
>> which one do you prefer?
>>
> 
> I'd go for a trace point to be honest
> 

ACK, I'll re-work it as a trace event then.
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 04/13] soc: bcm: brcmstb: Document panic notifier action and remove useless header

[Guilherme G. Piccoli]

On 20/07/2022 20:00, Florian Fainelli wrote:
> [...]
> 
> Applied to https://github.com/Broadcom/stblinux/commits/drivers/next, thanks!
> --
> Florian

Thanks Florian =)

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 09/13] notifier: Show function names on notifier routines if DEBUG_NOTIFIERS is set

[Guilherme G. Piccoli]

On 19/07/2022 17:33, Arjan van de Ven wrote:
> On 7/19/2022 12:53 PM, Guilherme G. Piccoli wrote:
>> Currently we have a debug infrastructure in the notifiers file, but
>> it's very simple/limited. Extend it by:
>>
>> (a) Showing all registered/unregistered notifiers' callback names;
> 
> 
> I'm not yet convinced that this is the right direction.
> The original intent for this "debug" feature was to be lightweight enough 
> that it could run in production, since at the time, rootkits
> liked to clobber/hijack notifiers and there were also some other signs of 
> corruption at the time.
> 
> By making something print (even at pr_info) for what are probably frequent 
> non-error operations, you turn something that is light
> into something that's a lot more heavy and generally that's not a great 
> idea... it'll be a performance surprise.
> 
> 

Is registering/un-registering notifiers a hot path, or performance
sensitive usually? For me, this patch proved to be very useful, and once
enabled, shows relatively few entries in dmesg, these operations aren't
so common thing it seems.

Also, if this Kconfig option was meant to run in production, maybe the
first thing would be have some sysfs tuning or anything able to turn it
on - I've worked with a variety of customers and the most terrifying
thing in servers is to install a new kernel and reboot heh

My understanding is that this debug infrastructure would be useful for
notifiers writers and people playing with the core notifiers
code...tracing would be much more useful in the context of checking if
some specific notifier got registered/executed in production environment
I guess.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 12/13] drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers

[Guilherme G. Piccoli]

Currently Hyper-V guests are among the most relevant users of the panic
infrastructure, like panic notifiers, kmsg dumpers, etc. The reasons rely
both in cleaning-up procedures (closing hypervisor <-> guest connection,
disabling some paravirtualized timer) as well as to data collection
(sending panic information to the hypervisor) and framebuffer management.

The thing is: some notifiers are related to others, ordering matters, some
functionalities are duplicated and there are lots of conditionals behind
sending panic information to the hypervisor. As part of an effort to
clean-up the panic notifiers mechanism and better document things, we
hereby address some of the issues/complexities of Hyper-V panic handling
through the following changes:

(a) We have die and panic notifiers on vmbus_drv.c and both have goals of
sending panic information to the hypervisor, though the panic notifier is
also responsible for a cleaning-up procedure.

This commit clears the code by splitting the panic notifier in two, one
for closing the vmbus connection whereas the other is only for sending
panic info to hypervisor. With that, it was possible to merge the die and
panic notifiers in a single/well-documented function, and clear some
conditional complexities on sending such information to the hypervisor.

(b) There is a Hyper-V framebuffer panic notifier, which relies in doing
a vmbus operation that demands a valid connection. So, we must order this
notifier with the panic notifier from vmbus_drv.c, to guarantee that the
framebuffer code executes before the vmbus connection is unloaded.

Also, this commit removes a useless header.

Although there is code rework and re-ordering, we expect that this change
has no functional regressions but instead optimize the path and increase
panic reliability on Hyper-V. This was tested on Hyper-V with success.

Cc: Andrea Parri (Microsoft) 
Cc: Dexuan Cui 
Cc: Haiyang Zhang 
Cc: "K. Y. Srinivasan" 
Cc: Michael Kelley 
Cc: Petr Mladek 
Cc: Stephen Hemminger 
Cc: Tianyu Lan 
Cc: Wei Liu 
Tested-by: Fabio A M Martins 
Signed-off-by: Guilherme G. Piccoli 

---


V2:
- Unfortunately we cannot rely in the crash shutdown (custom) handler
to perform the vmbus unload - arm64 architecture doesn't have this
"feature" [0]. So, in V2 we kept the notifier behavior and always
unload the vmbus connection, no matter what - thanks Michael for
pointing that;

- Removed the Fixes tags as per Michael suggestion;

- As per Petr suggestion, we abandoned the idea of distinguish among
notifiers using an id - so, in V2 we rely in the old and good address
comparison for that. Thanks Petr for the enriching discussion!

[0] 
https://lore.kernel.org/lkml/427a8277-49f0-4317-d6c3-4a15d7070...@igalia.com/


 drivers/hv/vmbus_drv.c  | 109 +++-
 drivers/video/fbdev/hyperv_fb.c |   8 +++
 2 files changed, 74 insertions(+), 43 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 547ae334e5cd..75c5623e7289 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -25,7 +25,6 @@
 #include 
 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -69,53 +68,74 @@ static int hyperv_report_reg(void)
return !sysctl_record_panic_msg || !hv_panic_page;
 }
 
-static int hyperv_panic_event(struct notifier_block *nb, unsigned long val,
+/*
+ * The panic notifier below is responsible solely for unloading the
+ * vmbus connection, which is necessary in a panic event.
+ *
+ * Notice an intrincate relation of this notifier with Hyper-V
+ * framebuffer panic notifier exists - we need vmbus connection alive
+ * there in order to succeed, so we need to order both with each other
+ * [see hvfb_on_panic()] - this is done using notifiers' priorities.
+ */
+static int hv_panic_vmbus_unload(struct notifier_block *nb, unsigned long val,
  void *args)
+{
+   vmbus_initiate_unload(true);
+   return NOTIFY_DONE;
+}
+static struct notifier_block hyperv_panic_vmbus_unload_block = {
+   .notifier_call  = hv_panic_vmbus_unload,
+   .priority   = INT_MIN + 1, /* almost the latest one to execute */
+};
+
+static int hv_die_panic_notify_crash(struct notifier_block *self,
+unsigned long val, void *args);
+
+static struct notifier_block hyperv_die_report_block = {
+   .notifier_call = hv_die_panic_notify_crash,
+};
+static struct notifier_block hyperv_panic_report_block = {
+   .notifier_call = hv_die_panic_notify_crash,
+};
+
+/*
+ * The following callback works both as die and panic notifier; its
+ * goal is to provide panic information to the hypervisor unless the
+ * kmsg dumper is used [see hv_kmsg_dump()], which provides more
+ * information but isn't always available.
+ *
+ * Notice that both the panic/die report notifiers are registered only
+ * if we have the capability HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE set.
+ */
+static int hv_die_panic_notify_c

[PATCH v2 06/13] um: Improve panic notifiers consistency and ordering

[Guilherme G. Piccoli]

Currently the panic notifiers from user mode linux don't follow
the convention for most of the other notifiers present in the
kernel (indentation, priority setting, numeric return).
More important, the priorities could be improved, since it's a
special case (userspace), hence we could run the notifiers earlier;
user mode linux shouldn't care much with other panic notifiers but
the ordering among the mconsole and arch notifier is important,
given that the arch one effectively triggers a core dump.

Fix that by running the mconsole notifier as the first panic
notifier, followed by the architecture one (that coredumps).

Cc: Anton Ivanov 
Cc: Johannes Berg 
Cc: Richard Weinberger 
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- Kept the notifier header to avoid implicit usage - thanks
Johannes for the suggestion!

 arch/um/drivers/mconsole_kern.c | 7 +++
 arch/um/kernel/um_arch.c| 8 
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index 8ca67a692683..69af3ce8407a 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -846,13 +846,12 @@ static int notify_panic(struct notifier_block *self, 
unsigned long unused1,
 
mconsole_notify(notify_socket, MCONSOLE_PANIC, message,
strlen(message) + 1);
-   return 0;
+   return NOTIFY_DONE;
 }
 
 static struct notifier_block panic_exit_notifier = {
-   .notifier_call  = notify_panic,
-   .next   = NULL,
-   .priority   = 1
+   .notifier_call  = notify_panic,
+   .priority   = INT_MAX, /* run as soon as possible */
 };
 
 static int add_notifier(void)
diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index 9838967d0b2f..970fdccc2f94 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -246,13 +246,13 @@ static int panic_exit(struct notifier_block *self, 
unsigned long unused1,
bust_spinlocks(0);
uml_exitcode = 1;
os_dump_core();
-   return 0;
+
+   return NOTIFY_DONE;
 }
 
 static struct notifier_block panic_exit_notifier = {
-   .notifier_call  = panic_exit,
-   .next   = NULL,
-   .priority   = 0
+   .notifier_call  = panic_exit,
+   .priority   = INT_MAX - 1, /* run as 2nd notifier, won't return */
 };
 
 void uml_finishsetup(void)
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 04/13] soc: bcm: brcmstb: Document panic notifier action and remove useless header

[Guilherme G. Piccoli]

The panic notifier of this driver is very simple code-wise, just a
memory write to a special position with some numeric code. But this
is not clear from the semantic point-of-view, and there is no public
documentation about that either.

After discussing this in the mailing-lists [0] and having Florian
explained it very well, document that in the code for the future
generations asking the same questions. Also, while at it, remove
a useless header.

[0] https://lore.kernel.org/lkml/781cafb0-8d06-8b56-907a-5175c2da1...@gmail.com

Cc: Brian Norris 
Cc: Doug Berger 
Cc: Justin Chen 
Cc: Lee Jones 
Cc: Markus Mayer 
Acked-by: Florian Fainelli 
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- Removed the Fixes tag;
- Added Florian's ACK - thanks!

 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c 
b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index 70ad0f3dce28..2a6adaa29596 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -25,7 +25,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -664,7 +663,20 @@ static void __iomem *brcmstb_ioremap_match(const struct 
of_device_id *matches,
 
return of_io_request_and_map(dn, index, dn->full_name);
 }
-
+/*
+ * The AON is a small domain in the SoC that can retain its state across
+ * various system wide sleep states and specific reset conditions; the
+ * AON DATA RAM is a small RAM of a few words (< 1KB) which can store
+ * persistent information across such events.
+ *
+ * The purpose of the below panic notifier is to help with notifying
+ * the bootloader that a panic occurred and so that it should try its
+ * best to preserve the DRAM contents holding that buffer for recovery
+ * by the kernel as opposed to wiping out DRAM clean again.
+ *
+ * Reference: comment from Florian Fainelli, at
+ * https://lore.kernel.org/lkml/781cafb0-8d06-8b56-907a-5175c2da1...@gmail.com
+ */
 static int brcmstb_pm_panic_notify(struct notifier_block *nb,
unsigned long action, void *data)
 {
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 09/13] notifier: Show function names on notifier routines if DEBUG_NOTIFIERS is set

[Guilherme G. Piccoli]

Currently we have a debug infrastructure in the notifiers file, but
it's very simple/limited. Extend it by:

(a) Showing all registered/unregistered notifiers' callback names;

(b) Adding a dynamic debug tuning to allow showing called notifiers'
function names. Notice that this should be guarded as a tunable since
it can flood the kernel log buffer.

Cc: Arjan van de Ven 
Cc: Cong Wang 
Cc: Sebastian Andrzej Siewior 
Cc: Steven Rostedt 
Cc: Valentin Schneider 
Cc: Xiaoming Ni 
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- Major improvement thanks to the great idea from Xiaoming - changed
all the ksym wheel reinvention to printk %ps modifier;

- Instead of ifdefs, using IS_ENABLED() - thanks Steven.

- Removed an unlikely() hint on debug path.

 kernel/notifier.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/kernel/notifier.c b/kernel/notifier.c
index 0d5bd62c480e..350761b34f8a 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -37,6 +37,10 @@ static int notifier_chain_register(struct notifier_block 
**nl,
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
+
+   if (IS_ENABLED(CONFIG_DEBUG_NOTIFIERS))
+   pr_info("notifiers: registered %ps\n", n->notifier_call);
+
return 0;
 }
 
@@ -46,6 +50,11 @@ static int notifier_chain_unregister(struct notifier_block 
**nl,
while ((*nl) != NULL) {
if ((*nl) == n) {
rcu_assign_pointer(*nl, n->next);
+
+   if (IS_ENABLED(CONFIG_DEBUG_NOTIFIERS))
+   pr_info("notifiers: unregistered %ps\n",
+   n->notifier_call);
+
return 0;
}
nl = &((*nl)->next);
@@ -77,13 +86,14 @@ static int notifier_call_chain(struct notifier_block **nl,
while (nb && nr_to_call) {
next_nb = rcu_dereference_raw(nb->next);
 
-#ifdef CONFIG_DEBUG_NOTIFIERS
-   if (unlikely(!func_ptr_is_kernel_text(nb->notifier_call))) {
-   WARN(1, "Invalid notifier called!");
-   nb = next_nb;
-   continue;
+   if (IS_ENABLED(CONFIG_DEBUG_NOTIFIERS)) {
+   if (!func_ptr_is_kernel_text(nb->notifier_call)) {
+   WARN(1, "Invalid notifier called!");
+   nb = next_nb;
+   continue;
+   }
+   pr_debug("notifiers: calling %ps\n", nb->notifier_call);
}
-#endif
ret = nb->notifier_call(nb, val, v);
 
if (nr_calls)
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 11/13] video/hyperv_fb: Avoid taking busy spinlock on panic path

[Guilherme G. Piccoli]

The Hyper-V framebuffer code registers a panic notifier in order
to try updating its fbdev if the kernel crashed. The notifier
callback is straightforward, but it calls the vmbus_sendpacket()
routine eventually, and such function takes a spinlock for the
ring buffer operations.

Panic path runs in atomic context, with local interrupts and
preemption disabled, and all secondary CPUs shutdown. That said,
taking a spinlock might cause a lockup if a secondary CPU was
disabled with such lock taken. Fix it here by checking if the
ring buffer spinlock is busy on Hyper-V framebuffer panic notifier;
if so, bail-out avoiding the potential lockup scenario.

Cc: Andrea Parri (Microsoft) 
Cc: Dexuan Cui 
Cc: Haiyang Zhang 
Cc: "K. Y. Srinivasan" 
Cc: Michael Kelley 
Cc: Stephen Hemminger 
Cc: Tianyu Lan 
Cc: Wei Liu 
Tested-by: Fabio A M Martins 
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- new patch, based on the discussion in [0].
[0] 
https://lore.kernel.org/lkml/2787b476-6366-1c83-db80-0393da417...@igalia.com/

 drivers/hv/ring_buffer.c| 16 
 drivers/video/fbdev/hyperv_fb.c |  8 +++-
 include/linux/hyperv.h  |  2 ++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 59a4aa86d1f3..9ceb3a7e8d19 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -280,6 +280,22 @@ void hv_ringbuffer_cleanup(struct hv_ring_buffer_info 
*ring_info)
ring_info->pkt_buffer_size = 0;
 }
 
+/*
+ * Check if the ring buffer spinlock is available to take or not; used on
+ * atomic contexts, like panic path (see the Hyper-V framebuffer driver).
+ */
+
+bool hv_ringbuffer_spinlock_busy(struct vmbus_channel *channel)
+{
+   struct hv_ring_buffer_info *rinfo = >outbound;
+
+   if (spin_is_locked(>ring_lock))
+   return true;
+
+   return false;
+}
+EXPORT_SYMBOL_GPL(hv_ringbuffer_spinlock_busy);
+
 /* Write to the ring buffer. */
 int hv_ringbuffer_write(struct vmbus_channel *channel,
const struct kvec *kv_list, u32 kv_count,
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 886c564787f1..e1b65a01fb96 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -783,12 +783,18 @@ static void hvfb_ondemand_refresh_throttle(struct 
hvfb_par *par,
 static int hvfb_on_panic(struct notifier_block *nb,
 unsigned long e, void *p)
 {
+   struct hv_device *hdev;
struct hvfb_par *par;
struct fb_info *info;
 
par = container_of(nb, struct hvfb_par, hvfb_panic_nb);
-   par->synchronous_fb = true;
info = par->info;
+   hdev = device_to_hv_device(info->device);
+
+   if (hv_ringbuffer_spinlock_busy(hdev->channel))
+   return NOTIFY_DONE;
+
+   par->synchronous_fb = true;
if (par->need_docopy)
hvfb_docopy(par, 0, dio_fb_size);
synthvid_update(info, 0, 0, INT_MAX, INT_MAX);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 3b42264333ef..646f1da9f27e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1341,6 +1341,8 @@ struct hv_ring_buffer_debug_info {
 int hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
struct hv_ring_buffer_debug_info *debug_info);
 
+bool hv_ringbuffer_spinlock_busy(struct vmbus_channel *channel);
+
 /* Vmbus interface */
 #define vmbus_driver_register(driver)  \
__vmbus_driver_register(driver, THIS_MODULE, KBUILD_MODNAME)
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 02/13] notifier: Add panic notifiers info and purge trailing whitespaces

[Guilherme G. Piccoli]

Although many notifiers are mentioned in the comments, the panic
notifiers infrastructure is not. Also, the file contains some
trailing whitespaces. Fix both issues here.

Cc: Arjan van de Ven 
Cc: Cong Wang 
Cc: Sebastian Andrzej Siewior 
Cc: Valentin Schneider 
Cc: Xiaoming Ni 
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- no change.

 include/linux/notifier.h | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index aef88c2d1173..d5b01f2e3fcc 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -208,12 +208,12 @@ static inline int notifier_to_errno(int ret)
 
 /*
  * Declared notifiers so far. I can imagine quite a few more chains
- * over time (eg laptop power reset chains, reboot chain (to clean 
+ * over time (eg laptop power reset chains, reboot chain (to clean
  * device units up), device [un]mount chain, module load/unload chain,
- * low memory chain, screenblank chain (for plug in modular 
screenblankers) 
+ * low memory chain, screenblank chain (for plug in modular screenblankers)
  * VC switch chains (for loadable kernel svgalib VC switch helpers) etc...
  */
- 
+
 /* CPU notfiers are defined in include/linux/cpu.h. */
 
 /* netdevice notifiers are defined in include/linux/netdevice.h */
@@ -224,6 +224,8 @@ static inline int notifier_to_errno(int ret)
 
 /* Virtual Terminal events are defined in include/linux/vt.h. */
 
+/* Panic notifiers are defined in include/linux/panic_notifier.h. */
+
 #define NETLINK_URELEASE   0x0001  /* Unicast netlink socket released */
 
 /* Console keyboard events.
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[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 
Cc: David Gow 
Cc: Evan Green 
Cc: Julius Werner 
Signed-off-by: Guilherme G. Piccoli 

---

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(_dev.lock))
+   return NOTIFY_DONE;
+
gsmi_shutdown_reason(GSMI_SHUTDOWN_PANIC);
return NOTIFY_DONE;
 }
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 07/13] parisc: Replace regular spinlock with spin_trylock on panic path

[Guilherme G. Piccoli]

The panic notifiers' callbacks execute in an atomic context, with
interrupts/preemption disabled, and all CPUs not running the panic
function are off, so it's very dangerous to wait on a regular
spinlock, there's a risk of deadlock.

Refactor the panic notifier of parisc/power driver to make use
of spin_trylock - for that, we've added a second version of the
soft-power function. Also, some comments were reorganized and
trailing white spaces, useless header inclusion and blank lines
were removed.

Cc: "James E.J. Bottomley" 
Acked-by: Helge Deller  # parisc
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- Added Helge's ACK - thanks!

 arch/parisc/include/asm/pdc.h |  1 +
 arch/parisc/kernel/firmware.c | 27 +++
 drivers/parisc/power.c| 17 ++---
 3 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/arch/parisc/include/asm/pdc.h b/arch/parisc/include/asm/pdc.h
index b643092d4b98..7a106008e258 100644
--- a/arch/parisc/include/asm/pdc.h
+++ b/arch/parisc/include/asm/pdc.h
@@ -83,6 +83,7 @@ int pdc_do_firm_test_reset(unsigned long ftc_bitmap);
 int pdc_do_reset(void);
 int pdc_soft_power_info(unsigned long *power_reg);
 int pdc_soft_power_button(int sw_control);
+int pdc_soft_power_button_panic(int sw_control);
 void pdc_io_reset(void);
 void pdc_io_reset_devices(void);
 int pdc_iodc_getc(void);
diff --git a/arch/parisc/kernel/firmware.c b/arch/parisc/kernel/firmware.c
index 6a7e315bcc2e..0e2f70b592f4 100644
--- a/arch/parisc/kernel/firmware.c
+++ b/arch/parisc/kernel/firmware.c
@@ -1232,15 +1232,18 @@ int __init pdc_soft_power_info(unsigned long *power_reg)
 }
 
 /*
- * pdc_soft_power_button - Control the soft power button behaviour
- * @sw_control: 0 for hardware control, 1 for software control 
+ * pdc_soft_power_button{_panic} - Control the soft power button behaviour
+ * @sw_control: 0 for hardware control, 1 for software control
  *
  *
  * This PDC function places the soft power button under software or
  * hardware control.
- * Under software control the OS may control to when to allow to shut 
- * down the system. Under hardware control pressing the power button 
+ * Under software control the OS may control to when to allow to shut
+ * down the system. Under hardware control pressing the power button
  * powers off the system immediately.
+ *
+ * The _panic version relies in spin_trylock to prevent deadlock
+ * on panic path.
  */
 int pdc_soft_power_button(int sw_control)
 {
@@ -1254,6 +1257,22 @@ int pdc_soft_power_button(int sw_control)
return retval;
 }
 
+int pdc_soft_power_button_panic(int sw_control)
+{
+   int retval;
+   unsigned long flags;
+
+   if (!spin_trylock_irqsave(_lock, flags)) {
+   pr_emerg("Couldn't enable soft power button\n");
+   return -EBUSY; /* ignored by the panic notifier */
+   }
+
+   retval = mem_pdc_call(PDC_SOFT_POWER, PDC_SOFT_POWER_ENABLE, 
__pa(pdc_result), sw_control);
+   spin_unlock_irqrestore(_lock, flags);
+
+   return retval;
+}
+
 /*
  * pdc_io_reset - Hack to avoid overlapping range registers of Bridges devices.
  * Primarily a problem on T600 (which parisc-linux doesn't support) but
diff --git a/drivers/parisc/power.c b/drivers/parisc/power.c
index 456776bd8ee6..8512884de2cf 100644
--- a/drivers/parisc/power.c
+++ b/drivers/parisc/power.c
@@ -37,7 +37,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -175,16 +174,21 @@ static void powerfail_interrupt(int code, void *x)
 
 
 
-/* parisc_panic_event() is called by the panic handler.
- * As soon as a panic occurs, our tasklets above will not be
- * executed any longer. This function then re-enables the 
- * soft-power switch and allows the user to switch off the system
+/*
+ * parisc_panic_event() is called by the panic handler.
+ *
+ * As soon as a panic occurs, our tasklets above will not
+ * be executed any longer. This function then re-enables
+ * the soft-power switch and allows the user to switch off
+ * the system. We rely in pdc_soft_power_button_panic()
+ * since this version spin_trylocks (instead of regular
+ * spinlock), preventing deadlocks on panic path.
  */
 static int parisc_panic_event(struct notifier_block *this,
unsigned long event, void *ptr)
 {
/* re-enable the soft-power switch */
-   pdc_soft_power_button(0);
+   pdc_soft_power_button_panic(0);
return NOTIFY_DONE;
 }
 
@@ -193,7 +197,6 @@ static struct notifier_block parisc_panic_block = {
.priority   = INT_MAX,
 };
 
-
 static int __init power_init(void)
 {
unsigned long ret;
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 05/13] alpha: Clean-up the panic notifier code

[Guilherme G. Piccoli]

The alpha panic notifier has some code issues, not following
the conventions of other notifiers. Also, it might halt the
machine but still it is set to run as early as possible, which
doesn't seem to be a good idea.

So, let's clean the code and set the notifier to run as the
latest, following the same approach other architectures are
doing - also, remove the unnecessary include of a header already
included indirectly.

Cc: Ivan Kokshaysky 
Cc: Matt Turner 
Cc: Richard Henderson 
Reviewed-by: Petr Mladek 
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- Fixed rth email address;
- Added Petr's review tag - thanks!

 arch/alpha/kernel/setup.c | 36 +++-
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index b4fbbba30aa2..d88bdf852753 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -41,19 +41,11 @@
 #include 
 #include 
 #endif
-#include 
 #include 
 #include 
 #include 
 #include 
 
-static int alpha_panic_event(struct notifier_block *, unsigned long, void *);
-static struct notifier_block alpha_panic_block = {
-   alpha_panic_event,
-NULL,
-INT_MAX /* try to do it first */
-};
-
 #include 
 #include 
 #include 
@@ -435,6 +427,21 @@ static const struct sysrq_key_op srm_sysrq_reboot_op = {
 };
 #endif
 
+static int alpha_panic_event(struct notifier_block *this,
+unsigned long event, void *ptr)
+{
+   /* If we are using SRM and serial console, just hard halt here. */
+   if (alpha_using_srm && srmcons_output)
+   __halt();
+
+   return NOTIFY_DONE;
+}
+
+static struct notifier_block alpha_panic_block = {
+   .notifier_call = alpha_panic_event,
+   .priority = INT_MIN, /* may not return, do it last */
+};
+
 void __init
 setup_arch(char **cmdline_p)
 {
@@ -1427,19 +1434,6 @@ const struct seq_operations cpuinfo_op = {
.show   = show_cpuinfo,
 };
 
-
-static int
-alpha_panic_event(struct notifier_block *this, unsigned long event, void *ptr)
-{
-#if 1
-   /* FIXME FIXME FIXME */
-   /* If we are using SRM and serial console, just hard halt here. */
-   if (alpha_using_srm && srmcons_output)
-   __halt();
-#endif
-return NOTIFY_DONE;
-}
-
 static __init int add_pcspkr(void)
 {
struct platform_device *pd;
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[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-gpicc...@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-ac62c498e...@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


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 09/13] notifier: Show function names on notifier routines if DEBUG_NOTIFIERS is set

[Guilherme G. Piccoli]

On 19/07/2022 17:48, Arjan van de Ven wrote:
> [...]
> I would totally support an approach where instead of pr_info, there's a 
> tracepoint
> for these events (and that shouldnt' need to be conditional on a config 
> option)
> 
> that's not what the patch does though.

This is a good idea Arjan! We could use trace events or pr_debug() -
which one do you prefer?

With that, we could maybe remove this Kconfig option, having it always
enabled, what do you think ?

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 13/13] panic: Fixes the panic_print NMI backtrace setting

[Guilherme G. Piccoli]

Commit 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
panic_print")
introduced a setting for the "panic_print" kernel parameter to allow
users to request a NMI backtrace on panic. Problem is that the panic_print
handling happens after the secondary CPUs are already disabled, hence
this option ended-up being kind of a no-op - kernel skips the NMI trace
in idling CPUs, which is the case of offline CPUs.

Fix it by checking the NMI backtrace bit in the panic_print prior to
the CPU disabling function.

Fixes: 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in 
panic_print")
Cc: Feng Tang 
Cc: Petr Mladek 
Signed-off-by: Guilherme G. Piccoli 

---


V2:
- new patch, there was no V1 of this one.


Hi folks, thanks upfront for reviews. This is a new patch, fixing an issue
I found in my tests, so I shoved it into this fixes series.

Notice that while at it, I got rid of the "crash_kexec_post_notifiers"
local copy in panic(). This was introduced by commit b26e27ddfd2a
("kexec: use core_param for crash_kexec_post_notifiers boot option"),
but it is not clear from comments or commit message why this local copy
is required.

My understanding is that it's a mechanism to prevent some concurrency,
in case some other CPU modify this variable while panic() is running.
I find it very unlikely, hence I removed it - but if people consider
this copy needed, I can respin this patch and keep it, even providing a
comment about that, in order to be explict about its need.

Let me know your thoughts! Cheers,

Guilherme


 kernel/panic.c | 47 +++
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index a3308af28a21..7fb604e95ef9 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -180,9 +180,6 @@ static void panic_print_sys_info(bool console_flush)
return;
}
 
-   if (panic_print & PANIC_PRINT_ALL_CPU_BT)
-   trigger_all_cpu_backtrace();
-
if (panic_print & PANIC_PRINT_TASK_INFO)
show_state();
 
@@ -199,6 +196,30 @@ static void panic_print_sys_info(bool console_flush)
ftrace_dump(DUMP_ALL);
 }
 
+/*
+ * Helper that triggers the NMI backtrace (if set in panic_print)
+ * and then performs the secondary CPUs shutdown - we cannot have
+ * the NMI backtrace after the CPUs are off!
+ */
+static void panic_other_cpus_shutdown(void)
+{
+   if (panic_print & PANIC_PRINT_ALL_CPU_BT)
+   trigger_all_cpu_backtrace();
+
+   /*
+* Note that smp_send_stop() is the usual SMP shutdown function,
+* which unfortunately may not be hardened to work in a panic
+* situation. If we want to do crash dump after notifier calls
+* and kmsg_dump, we will need architecture dependent extra
+* bits in addition to stopping other CPUs, hence we rely on
+* crash_smp_send_stop() for that.
+*/
+   if (!crash_kexec_post_notifiers)
+   smp_send_stop();
+   else
+   crash_smp_send_stop();
+}
+
 /**
  * panic - halt the system
  * @fmt: The text string to print
@@ -214,7 +235,6 @@ void panic(const char *fmt, ...)
long i, i_next = 0, len;
int state = 0;
int old_cpu, this_cpu;
-   bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
 
if (panic_on_warn) {
/*
@@ -289,23 +309,10 @@ void panic(const char *fmt, ...)
 *
 * Bypass the panic_cpu check and call __crash_kexec directly.
 */
-   if (!_crash_kexec_post_notifiers) {
+   if (!crash_kexec_post_notifiers)
__crash_kexec(NULL);
 
-   /*
-* Note smp_send_stop is the usual smp shutdown function, which
-* unfortunately means it may not be hardened to work in a
-* panic situation.
-*/
-   smp_send_stop();
-   } else {
-   /*
-* If we want to do crash dump after notifier calls and
-* kmsg_dump, we will need architecture dependent extra
-* works in addition to stopping other CPUs.
-*/
-   crash_smp_send_stop();
-   }
+   panic_other_cpus_shutdown();
 
/*
 * Run any panic handlers, including those that might need to
@@ -326,7 +333,7 @@ void panic(const char *fmt, ...)
 *
 * Bypass the panic_cpu check and call __crash_kexec directly.
 */
-   if (_crash_kexec_post_notifiers)
+   if (crash_kexec_post_notifiers)
__crash_kexec(NULL);
 
 #ifdef CONFIG_VT
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 10/13] EDAC/altera: Skip the panic notifier if kdump is loaded

[Guilherme G. Piccoli]

The altera_edac panic notifier performs some data collection with
regards errors detected; such code relies in the regmap layer to
perform reads/writes, so the code is abstracted and there is some
risk level to execute that, since the panic path runs in atomic
context, with interrupts/preemption and secondary CPUs disabled.

Users want the information collected in this panic notifier though,
so in order to balance the risk/benefit, let's skip the altera panic
notifier if kdump is loaded. While at it, remove a useless header
and encompass a macro inside the sole ifdef block it is used.

Cc: Dinh Nguyen 
Cc: Petr Mladek 
Cc: Tony Luck 
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- new patch, based on the discussion in [0].
[0] 
https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/

 drivers/edac/altera_edac.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index e7e8e624a436..741fe5539154 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -16,7 +16,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -24,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "altera_edac.h"
@@ -2063,22 +2063,30 @@ static const struct irq_domain_ops a10_eccmgr_ic_ops = {
 };
 
 /** Stratix 10 EDAC Double Bit Error Handler /
-#define to_a10edac(p, m) container_of(p, struct altr_arria10_edac, m)
-
 #ifdef CONFIG_64BIT
 /* panic routine issues reboot on non-zero panic_timeout */
 extern int panic_timeout;
 
+#define to_a10edac(p, m) container_of(p, struct altr_arria10_edac, m)
+
 /*
  * The double bit error is handled through SError which is fatal. This is
  * called as a panic notifier to printout ECC error info as part of the panic.
+ *
+ * Notice that if kdump is set, we take the risk avoidance approach and
+ * skip the notifier, given that users are expected to have access to a
+ * full vmcore.
  */
 static int s10_edac_dberr_handler(struct notifier_block *this,
  unsigned long event, void *ptr)
 {
-   struct altr_arria10_edac *edac = to_a10edac(this, panic_notifier);
+   struct altr_arria10_edac *edac;
int err_addr, dberror;
 
+   if (kexec_crash_loaded())
+   return NOTIFY_DONE;
+
+   edac = to_a10edac(this, panic_notifier);
regmap_read(edac->ecc_mgr_map, S10_SYSMGR_ECC_INTSTAT_DERR_OFST,
);
regmap_write(edac->ecc_mgr_map, S10_SYSMGR_UE_VAL_OFST, dberror);
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 08/13] tracing: Improve panic/die notifiers

[Guilherme G. Piccoli]

Currently the tracing dump_on_oops feature is implemented
through separate notifiers, one for die/oops and the other
for panic - given they have the same functionality, let's
unify them.

Also improve the function comment and change the priority of
the notifier to make it execute earlier, avoiding showing useless
trace data (like the callback names for the other notifiers);
finally, we also removed an unnecessary header inclusion.

Cc: Petr Mladek 
Cc: Sergei Shtylyov 
Cc: Steven Rostedt 
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- Different approach; instead of using IDs to distinguish die and
panic events, rely on address comparison like other notifiers do
and as per Petr's suggestion;

- Removed ACK from Steven since the code changed.

 kernel/trace/trace.c | 55 ++--
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b8dd54627075..2a436b645c70 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -19,7 +19,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -9777,40 +9776,40 @@ static __init int tracer_init_tracefs(void)
 
 fs_initcall(tracer_init_tracefs);
 
-static int trace_panic_handler(struct notifier_block *this,
-  unsigned long event, void *unused)
-{
-   if (ftrace_dump_on_oops)
-   ftrace_dump(ftrace_dump_on_oops);
-   return NOTIFY_OK;
-}
+static int trace_die_panic_handler(struct notifier_block *self,
+   unsigned long ev, void *unused);
 
 static struct notifier_block trace_panic_notifier = {
-   .notifier_call  = trace_panic_handler,
-   .next   = NULL,
-   .priority   = 150   /* priority: INT_MAX >= x >= 0 */
+   .notifier_call = trace_die_panic_handler,
+   .priority = INT_MAX - 1,
 };
 
-static int trace_die_handler(struct notifier_block *self,
-unsigned long val,
-void *data)
-{
-   switch (val) {
-   case DIE_OOPS:
-   if (ftrace_dump_on_oops)
-   ftrace_dump(ftrace_dump_on_oops);
-   break;
-   default:
-   break;
-   }
-   return NOTIFY_OK;
-}
-
 static struct notifier_block trace_die_notifier = {
-   .notifier_call = trace_die_handler,
-   .priority = 200
+   .notifier_call = trace_die_panic_handler,
+   .priority = INT_MAX - 1,
 };
 
+/*
+ * The idea is to execute the following die/panic callback early, in order
+ * to avoid showing irrelevant information in the trace (like other panic
+ * notifier functions); we are the 2nd to run, after hung_task/rcu_stall
+ * warnings get disabled (to prevent potential log flooding).
+ */
+static int trace_die_panic_handler(struct notifier_block *self,
+   unsigned long ev, void *unused)
+{
+   if (!ftrace_dump_on_oops)
+   goto out;
+
+   if (self == _die_notifier && ev != DIE_OOPS)
+   goto out;
+
+   ftrace_dump(ftrace_dump_on_oops);
+
+out:
+   return NOTIFY_DONE;
+}
+
 /*
  * printk is set to max of 1024, we really don't need it that big.
  * Nothing should be printing 1000 characters anyway.
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 01/13] ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths

[Guilherme G. Piccoli]

Currently the regular CPU shutdown path for ARM disables IRQs/FIQs
in the secondary CPUs - smp_send_stop() calls ipi_cpu_stop(), which
is responsible for that. IRQs are architecturally masked when we
take an interrupt, but FIQs are high priority than IRQs, hence they
aren't masked. With that said, it makes sense to disable FIQs here,
but there's no need for (re-)disabling IRQs.

More than that: there is an alternative path for disabling CPUs,
in the form of function crash_smp_send_stop(), which is used for
kexec/panic path. This function relies on a SMP call that also
triggers a busy-wait loop [at machine_crash_nonpanic_core()], but
without disabling FIQs. This might lead to odd scenarios, like
early interrupts in the boot of kexec'd kernel or even interrupts
in secondary "disabled" CPUs while the main one still works in the
panic path and assumes all secondary CPUs are (really!) off.

So, let's disable FIQs in both paths and *not* disable IRQs a second
time, since they are already masked in both paths by the architecture.
This way, we keep both CPU quiesce paths consistent and safe.

Cc: Marc Zyngier 
Cc: Michael Kelley 
Cc: Russell King 
Signed-off-by: Guilherme G. Piccoli 

---

V2:
- Small wording improvement (thanks Michael Kelley);
- Only disable FIQs, since IRQs are masked by architecture
definition when we take an interrupt. Thanks a lot Russell
and Marc for the discussion [0].

Should we add a Fixes tag here? If so, maybe the proper target is:
b23065313297 ("ARM: 6522/1: kexec: Add call to non-crashing cores through IPI")

[0] https://lore.kernel.org/lkml/ymxcaqy6dwhoq...@shell.armlinux.org.uk/

 arch/arm/kernel/machine_kexec.c | 2 ++
 arch/arm/kernel/smp.c   | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index f567032a09c0..0b482bcb97f7 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -77,6 +77,8 @@ void machine_crash_nonpanic_core(void *unused)
 {
struct pt_regs regs;
 
+   local_fiq_disable();
+
crash_setup_regs(, get_irq_regs());
printk(KERN_DEBUG "CPU %u will stop doing anything useful since another 
CPU has crashed\n",
   smp_processor_id());
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 73fc645fc4c7..0c24ec1fd88e 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -600,6 +600,8 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
  */
 static void ipi_cpu_stop(unsigned int cpu)
 {
+   local_fiq_disable();
+
if (system_state <= SYSTEM_RUNNING) {
raw_spin_lock(_lock);
pr_crit("CPU%u: stopping\n", cpu);
@@ -609,9 +611,6 @@ static void ipi_cpu_stop(unsigned int cpu)
 
set_cpu_online(cpu, false);
 
-   local_fiq_disable();
-   local_irq_disable();
-
while (1) {
cpu_relax();
wfe();
-- 
2.37.1


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Unstable tsc caused soft lockup in kdump kernel

[Guilherme G. Piccoli]

On 29/06/2022 07:25, Baoquan He wrote:
> Hi,
> 
> On a HP machine, after crash triggered via sysrq-c, kdump kernel will
> boot and get soft lockup as below. And this can be always reproduced.
> 
> From log, it seems that unreliable tsc was marked as unstable in
> clocksource_watchdog, then worker sched_clock_work was scheduled. And
> this tsc unstable marking always happened after sysrq-c is triggered.
> And the cpu where worker smp_call_function_many_cond is running won't
> be stopped and hang there and keep locks, even though the cpu should be
> stopped. While kdump kernel is running in a different cpu and boot, then
> soft lockup happened because other workers or the relevant threads are
> waiting for locks taken by the hang sched_clock_work worker.
> 
> Any idea or suggestion?
> 
> The normal kernel boot log and kdump kernel log, kernel config, are all
> attached, please check.
> 

Hi Baoquan, interesting issue! Do you happen to have a full kdump boot
log with the issue? Maybe collected through serial console, etc.
It seems the one attached is from a succeeding kdump by passing
"tsc=unstable" to the kdump kernel right?

Also, did you try to "forbid" tsc to get marked as unstable in the first
kernel, before kdump? I mean like a hack code change, just prevent
kernel doing that and seeing if it works. If that still fails, then it
seems the cause of the issue is the same as the cause of TSC getting
unstable - in other words, something would be causing both the kdump
kernel lockup AND the TSC unstable marking in the first kernel...

Cheers!

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 24/30] panic: Refactor the panic path

[Guilherme G. Piccoli]

Perfect Petr, thanks for your feedback!

I'll be out for some weeks, but after that what I'm doing is to split
the series in 2 parts:

(a) The general fixes, which should be reviewed by subsystem maintainers
and even merged individually by them.

(b) The proper panic refactor, which includes the notifiers list split,
etc. I'll think about what I consider the best solution for the
crash_dump required ones, and will try to split in very simple patches
to make it easier to review.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 24/30] panic: Refactor the panic path

[Guilherme G. Piccoli]

tifier into wrong list.
>> 
>> I would like to note again that the panic notifiers are optional to run,
>> while kdump is expectd once loaded, from the original purpose. I guess
>> people I know will still have this thought, e.g Hatayama, Masa, they are
>> truly often use panic notifiers like this on their company's system.


On 24/05/2022 11:44, Eric W. Biederman wrote:
> [...]
> Unfortunately I am also very grouchy.
> 
> Notifiers before kexec on panic are fundamentally broken.  So please
> just remove crash_kexec_post notifiers and be done with it.  Part of the
> deep issue is that firmware always has a common and broken
> implementation for anything that is not mission critical to
> motherboards.
> 
> Notifiers in any sense on these paths are just bollocks.  Any kind of
> notifier list is fundamentally fragile in the face of memory corruption
> and very very difficult to review.
> 
> So I am going to refresh my ancient NACK on this.
> 
> I can certainly appreciate that there are pieces of the reboot paths
> that can be improved.  I don't think making anything more feature full
> or flexible is any kind of real improvement.


Now, from the thread "Should arm64 have a custom crash shutdown
handler?" (link:
https://lore.kernel.org/lkml/427a8277-49f0-4317-d6c3-4a15d7070...@igalia.com/),
we have:

On 05/05/2022 08:10, Mark Rutland wrote:
>> On Wed, May 04, 2022 at 05:00:42PM -0300, Guilherme G. Piccoli wrote:
>>> [...]
>>> Currently, when we kexec in arm64, the function machine_crash_shutdown()
>>> is called as a handler to disable CPUs and (potentially) do extra
>>> quiesce work. In the aforementioned architectures, there's a way to
>>> override this function, if for example an hypervisor wish to have its
>>> guests running their own custom shutdown machinery.
>> 
>> What exactly do you need to do in this custom shutdown machinery?
>> 
>> The general expectation for arm64 is that any hypervisor can implement PSCI,
>> and as long as you have that, CPUs (and the VM as a whole) can be shutdown 
>> in a
>> standard way.
>> 
>> I suspect what you're actually after is a mechanism to notify the hypervisor
>> when the guest crashes, rather than changing the way the shutdown itself
>> occurs? If so, we already have panic notifiers, and QEMU has a "pvpanic"
>> device using that. See drivers/misc/pvpanic/.


OK, so it seems we have some points in which agreement exists, and some
points that there is no agreement and instead, we have antagonistic /
opposite views and needs. Let's start with the easier part heh


It seems everybody agrees that *we shouldn't over-engineer things*, and
as per Eric good words: making the panic path more feature-full or
increasing flexibility isn't a good idea. So, as a "corollary": the
panic level approach I'm proposing is not a good fit, I'll drop it and
let's go with something simpler.

Another point of agreement seems to be that _notifier lists in the panic
path are dangerous_, for *2 different reasons*:

(a) We cannot guarantee that people won't add crazy callbacks there, we
can plan and document things the best as possible - it'll never be
enough, somebody eventually would slip a nonsense callback that would
break things and defeat the planned purpose of such a list;

(b) As per Eric point, in a panic/crash situation we might have memory
corruption exactly in the list code / pointers, etc, so the notifier
lists are, by nature, a bit fragile. But I think we shouldn't consider
it completely "bollocks", since this approach has been used for a while
with a good success rate. So, lists aren't perfect at all, but at the
same time, they aren't completely useless.


Now, to the points in which there are conflicting / antagonistic
needs/views:

(I) Kdump should be the first thing to run, as it's been like that since
forever. But...notice that "crash_kexec_post_notifiers" was created
exactly as a way to circumvent that, so we can see this is not an
absolute truth. Some users really *require to execute* some special code
*before kdump*.
Worth noticing here that regular kexec invokes the drivers .shutdown()
handlers, while kdump [aka crash_kexec()] does not, so we must have a
way to run code before kdump in a crash situation.

(II) If *we need* to have some code always running before kdump/reboot
on panic path (like the Hyper-V vmbus connection unload), *where to add
such code*? Again, conflicting views. Some would say we should hardcode
this in the panic() function. Others, that we should use the custom
machine_crash_shutdown() infrastructure - but notice that this isn't
available in all architectures, like arm64. Finally, others suggest
to...use notifier lists! Which was more or less the approach we took in
this patch.

How can we reach consen

Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list

[Guilherme G. Piccoli]

On 19/05/2022 16:20, Scott Branden wrote:
> [...] 
>> Hi Scott / Desmond, thanks for the detailed answer! Is this adapter
>> designed to run in x86 only or you have other architectures' use cases?
> The adapter may be used in any PCIe design that supports DMA.
> So it may be possible to run in arm64 servers.
>>
>> [...]
>> With that said, and given this is a lightweight notifier that ideally
>> should run ASAP, I'd keep this one in the hypervisor list. We can
>> "adjust" the semantic of this list to include lightweight notifiers that
>> reset adapters.
> Sounds the best to keep system operating as tested today.
>>
>> With that said, Petr has a point - not always such list is going to be
>> called before kdump. So, that makes me think in another idea: what if we
>> have another list, but not on panic path, but instead in the custom
>> crash_shutdown()? Drivers could add callbacks there that must execute
>> before kexec/kdump, no matter what.
> It may be beneficial for some other drivers but for our use we would 
> then need to register for the panic path and the crash_shutdown path. 
> We notify the VK card for 2 purposes: one to stop DMA so memory stop 
> changing during a kdump.  And also to get the card into a good state so 
> resets happen cleanly.

Thanks Scott! With that, I guess it's really better to keep this
notifier in this hypervisor/early list - I'm planning to do that for V2.
Unless Petr or somebody has strong feelings against that, of course.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 12/30] parisc: Replace regular spinlock with spin_trylock on panic path

[Guilherme G. Piccoli]

On 28/04/2022 13:55, Helge Deller wrote:
> [...]
> You may add:
> Acked-by: Helge Deller  # parisc
> 
> Helge

Hi Helge, do you think would be possible to still pick this one for
v5.19 or do you prefer to hold for the next release?

I'm working on V2, so if it's merged for 5.19 I won't send it again.
Thanks,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list

[Guilherme G. Piccoli]

On 18/05/2022 19:17, Scott Branden wrote:
> Hi Guilherme,
> 
> +Desmond
> [...] 
 I'm afraid it breaks kdump if this device is not reset beforehand - it's
 a doorbell write, so not high risk I think...

 But in case the not-reset device can be probed normally in kdump kernel,
 then I'm fine in moving this to the reboot list! I don't have the HW to
 test myself.
>>>
>>> Good question. Well, it if has to be called before kdump then
>>> even "hypervisor" list is a wrong place because is not always
>>> called before kdump.
>> [...]
> We register to the panic notifier so that we can kill the VK card ASAP
> to stop DMAing things over to the host side.  If it is not notified then
> memory may not be frozen when kdump is occurring.
> Notifying the card on panic is also needed to allow for any type of 
> reset to occur.
> 
> So, the only thing preventing moving the notifier later is the chance
> that memory is modified while kdump is occurring.  Or, if DMA is 
> disabled before kdump already then this wouldn't be an issue and the 
> notification to the card (to allow for clean resets) can be done later.

Hi Scott / Desmond, thanks for the detailed answer! Is this adapter
designed to run in x86 only or you have other architectures' use cases?

I'm not expert on that, but I guess whether DMA is "kept" or not depends
a bit if IOMMU is used. IIRC, there was a copy of the DMAR table in
kdump (at least for Intel IOMMU). Also, devices are not properly
quiesced on kdump IIUC, we don't call shutdown/reset handlers, they're
skip due to the crash nature - so there is a risk of devices doing bad
things in the new kernel.

With that said, and given this is a lightweight notifier that ideally
should run ASAP, I'd keep this one in the hypervisor list. We can
"adjust" the semantic of this list to include lightweight notifiers that
reset adapters.

With that said, Petr has a point - not always such list is going to be
called before kdump. So, that makes me think in another idea: what if we
have another list, but not on panic path, but instead in the custom
crash_shutdown()? Drivers could add callbacks there that must execute
before kexec/kdump, no matter what.

Let me know your thoughts Scott / Desmond / Petr and all interested parties.
Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list

[Guilherme G. Piccoli]

On 19/05/2022 04:03, Petr Mladek wrote:
> [...]
> I would ignore it for now. If anyone would want to safe the log
> then they would need to read it. They will most likely use
> the existing kmsg_dump() infastructure. In fact, they should
> use it to avoid a code duplication.
> 
> Best Regards,
> Petr

Cool, thanks! I agree, let's expect people use kmsg_dump() as they should =)

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 24/30] panic: Refactor the panic path

[Guilherme G. Piccoli]

On 19/05/2022 20:45, Baoquan He wrote:
> [...]
>> I really appreciate the summary skill you have, to convert complex
>> problems in very clear and concise ideas. Thanks for that, very useful!
>> I agree with what was summarized above.
> 
> I want to say the similar words to Petr's reviewing comment when I went
> through the patches and traced each reviewing sub-thread to try to
> catch up. Petr has reivewed this series so carefully and given many
> comments I want to ack immediately.
> 
> I agree with most of the suggestions from Petr to this patch, except of
> one tiny concern, please see below inline comment.

Hi Baoquan, thanks! I'm glad you're also reviewing that =)


> [...]
> 
> I like the proposed skeleton of panic() and code style suggested by
> Petr very much. About panic_prefer_crash_dump which might need be added,
> I hope it has a default value true. This makes crash_dump execute at
> first by default just as before, unless people specify
> panic_prefer_crash_dump=0|n|off to disable it. Otherwise we need add
> panic_prefer_crash_dump=1 in kernel and in our distros to enable kdump,
> this is inconsistent with the old behaviour.

I'd like to understand better why the crash_kexec() must always be the
first thing in your use case. If we keep that behavior, we'll see all
sorts of workarounds - see the last patches of this series, Hyper-V and
PowerPC folks hardcoded "crash_kexec_post_notifiers" in order to force
execution of their relevant notifiers (like the vmbus disconnect,
specially in arm64 that has no custom machine_crash_shutdown, or the
fadump case in ppc). This led to more risk in kdump.

The thing is: with the notifiers' split, we tried to keep only the most
relevant/necessary stuff in this first list, things that ultimately
should improve kdump reliability or if not, at least not break it. My
feeling is that, with this series, we should change the idea/concept
that kdump must run first nevertheless, not matter what. We're here
trying to accommodate the antagonistic goals of hypervisors that need
some clean-up (even for kdump to work) VS. kdump users, that wish a
"pristine" system reboot ASAP after the crash.

Cheers,


Guilherme

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list

[Guilherme G. Piccoli]

On 18/05/2022 04:33, Petr Mladek wrote:
> [...]
> Anyway, I would distinguish it the following way.
> 
>   + If the notifier is preserving kernel log then it should be ideally
> treated as kmsg_dump().
> 
>   + It the notifier is saving another debugging data then it better
> fits into the "hypervisor" notifier list.
> 
>

Definitely, I agree - it's logical, since we want more info in the logs,
and happens some notifiers running in the informational list do that,
like ftrace_on_oops for example.


> Regarding the reliability. From my POV, any panic notifier enabled
> in a generic kernel should be reliable with more than 99,9%.
> Otherwise, they should not be in the notifier list at all.
> 
> An exception would be a platform-specific notifier that is
> called only on some specific platform and developers maintaining
> this platform agree on this.
> 
> The value "99,9%" is arbitrary. I am not sure if it is realistic
> even in the other code, for example, console_flush_on_panic()
> or emergency_restart(). I just want to point out that the border
> should be rather high. Otherwise we would back in the situation
> where people would want to disable particular notifiers.
> 

Totally agree, these percentages are just an example, 50% is ridiculous
low reliability in my example heheh

But some notifiers deep dive in abstraction layers (like regmap or GPIO
stuff) and it's hard to determine the probability of a lock issue (take
a spinlock already taken inside regmap code and live-lock forever, for
example). These are better to run, if possible, later than kdump or even
info list.

Thanks again for the good analysis Petr!
Cheers,


Guilherme



___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list

[Guilherme G. Piccoli]

On 18/05/2022 04:58, Petr Mladek wrote:
> [...]
>> I does similar things like kmsg_dump() so it should be called in
>> the same location (after info notifier list and before kdump).
>>
>> A solution might be to put it at these notifiers at the very
>> end of the "info" list or make extra "dump" notifier list.
> 
> I just want to point out that the above idea has problems.
> Notifiers storing kernel log need to be treated as kmsg_dump().
> In particular, we would  need to know if there are any.
> We do not need to call "info" notifier list before kdump
> when there is no kernel log dumper registered.
> 

Notifiers respect the priority concept, which is just a number that
orders the list addition (and the list is called in order).

I've used the last position to panic_print() [in patch 25] - one idea
here is to "reserve" the last position (represented by INT_MIN) for
notifiers that act like kmsg_dump(). I couldn't find any IIRC, but that
doesn't prevent us to save this position and comment about that.

Makes sense to you ?
Cheers!

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec