Hey everybody, this the third iteration of the panic notifiers
fixes/clean-ups;
V2 available at:
https://lore.kernel.org/lkml/[email protected]/
V1 (including the refactor) available at:
https://lore.kernel.org/lkml/[email protected]/
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
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) <[email protected]>
Cc: Dexuan Cui <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Michael Kelley <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: Tianyu Lan <[email protected]>
Cc: Wei Liu <[email protected]>
Tested-by: Fabio A M Martins <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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/[email protected]/
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 = &channel->outbound;
+
+ return spin_is_locked(&rinfo->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
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) <[email protected]>
Cc: Dexuan Cui <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: Tianyu Lan <[email protected]>
Cc: Wei Liu <[email protected]>
Reviewed-by: Michael Kelley <[email protected]>
Tested-by: Fabio A M Martins <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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/[email protected]/
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 <linux/sched/task_stack.h>
#include <linux/delay.h>
-#include <linux/notifier.h>
#include <linux/panic_notifier.h>
#include <linux/ptrace.h>
#include <linux/screen_info.h>
@@ -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_crash(struct notifier_block *self,
+ unsigned long val, void *args)
{
struct pt_regs *regs;
+ bool is_die;
- vmbus_initiate_unload(true);
-
- /*
- * Hyper-V should be notified only once about a panic. If we will be
- * doing hv_kmsg_dump() with kmsg data later, don't do the notification
- * here.
- */
- if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE
- && hyperv_report_reg()) {
+ /* Don't notify Hyper-V unless we have a die oops event or panic. */
+ if (self == &hyperv_panic_report_block) {
+ is_die = false;
regs = current_pt_regs();
- hyperv_report_panic(regs, val, false);
+ } else { /* die event */
+ if (val != DIE_OOPS)
+ return NOTIFY_DONE;
+
+ is_die = true;
+ regs = ((struct die_args *)args)->regs;
}
- return NOTIFY_DONE;
-}
-
-static int hyperv_die_event(struct notifier_block *nb, unsigned long val,
- void *args)
-{
- struct die_args *die = args;
- struct pt_regs *regs = die->regs;
-
- /* Don't notify Hyper-V if the die event is other than oops */
- if (val != DIE_OOPS)
- return NOTIFY_DONE;
/*
- * Hyper-V should be notified only once about a panic. If we will be
- * doing hv_kmsg_dump() with kmsg data later, don't do the notification
- * here.
+ * Hyper-V should be notified only once about a panic/die. If we will
+ * be calling hv_kmsg_dump() later with kmsg data, don't do the
+ * notification here.
*/
if (hyperv_report_reg())
- hyperv_report_panic(regs, val, true);
+ hyperv_report_panic(regs, val, is_die);
+
return NOTIFY_DONE;
}
-static struct notifier_block hyperv_die_block = {
- .notifier_call = hyperv_die_event,
-};
-static struct notifier_block hyperv_panic_block = {
- .notifier_call = hyperv_panic_event,
-};
-
static const char *fb_mmio_name = "fb_range";
static struct resource *fb_mmio;
static struct resource *hyperv_mmio;
@@ -1538,16 +1558,17 @@ static int vmbus_bus_init(void)
if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
hv_kmsg_dump_register();
- register_die_notifier(&hyperv_die_block);
+ register_die_notifier(&hyperv_die_report_block);
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &hyperv_panic_report_block);
}
/*
- * Always register the panic notifier because we need to unload
- * the VMbus channel connection to prevent any VMbus
- * activity after the VM panics.
+ * Always register the vmbus unload panic notifier because we
+ * need to shut the VMbus channel connection on panic.
*/
atomic_notifier_chain_register(&panic_notifier_list,
- &hyperv_panic_block);
+ &hyperv_panic_vmbus_unload_block);
vmbus_request_offers();
@@ -2776,15 +2797,17 @@ static void __exit vmbus_exit(void)
if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
kmsg_dump_unregister(&hv_kmsg_dumper);
- unregister_die_notifier(&hyperv_die_block);
+ unregister_die_notifier(&hyperv_die_report_block);
+ atomic_notifier_chain_unregister(&panic_notifier_list,
+ &hyperv_panic_report_block);
}
/*
- * The panic notifier is always registered, hence we should
+ * The vmbus panic notifier is always registered, hence we should
* also unconditionally unregister it here as well.
*/
atomic_notifier_chain_unregister(&panic_notifier_list,
- &hyperv_panic_block);
+ &hyperv_panic_vmbus_unload_block);
free_page((unsigned long)hv_panic_page);
unregister_sysctl_table(hv_ctl_table_hdr);
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index e1b65a01fb96..9234d31d4305 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -1216,7 +1216,15 @@ static int hvfb_probe(struct hv_device *hdev,
par->fb_ready = true;
par->synchronous_fb = false;
+
+ /*
+ * We need to be sure this panic notifier runs _before_ the
+ * vmbus disconnect, so order it by priority. It must execute
+ * before the function hv_panic_vmbus_unload() [drivers/hv/vmbus_drv.c],
+ * which is almost at the end of list, with priority = INT_MIN + 1.
+ */
par->hvfb_panic_nb.notifier_call = hvfb_on_panic;
+ par->hvfb_panic_nb.priority = INT_MIN + 10,
atomic_notifier_chain_register(&panic_notifier_list,
&par->hvfb_panic_nb);
--
2.37.2
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 <[email protected]>
Cc: Michael Kelley <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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/[email protected]/
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(®s, 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(&stop_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
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 <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: Richard Weinberger <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
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
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 <[email protected]>
Cc: Petr Mladek <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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
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 <[email protected]>
Cc: Matt Turner <[email protected]>
Cc: Richard Henderson <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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 <linux/sysrq.h>
#include <linux/reboot.h>
#endif
-#include <linux/notifier.h>
#include <asm/setup.h>
#include <asm/io.h>
#include <linux/log2.h>
#include <linux/export.h>
-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 <linux/uaccess.h>
#include <asm/hwrpb.h>
#include <asm/dma.h>
@@ -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
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" <[email protected]>
Cc: Jeroen Roovers <[email protected]>
Acked-by: Helge Deller <[email protected]> # parisc
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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(&pdc_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(&pdc_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 <linux/module.h>
#include <linux/init.h>
#include <linux/kernel.h>
-#include <linux/notifier.h>
#include <linux/panic_notifier.h>
#include <linux/reboot.h>
#include <linux/sched/signal.h>
@@ -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
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 <[email protected]>
Cc: Sergei Shtylyov <[email protected]>
Cc: Steven Rostedt <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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 <linux/kallsyms.h>
#include <linux/security.h>
#include <linux/seq_file.h>
-#include <linux/notifier.h>
#include <linux/irqflags.h>
#include <linux/debugfs.h>
#include <linux/tracefs.h>
@@ -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 == &trace_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
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 <[email protected]>
Cc: Cong Wang <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Xiaoming Ni <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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 000000000000..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 <linux/tracepoint.h>
+
+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 <trace/define_trace.h>
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 <linux/vmalloc.h>
#include <linux/reboot.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/notifiers.h>
+
/*
* 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
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 <[email protected]>
Cc: Cong Wang <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Xiaoming Ni <[email protected]>
Reviewed-by: Baoquan He <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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
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 <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Tony Luck <[email protected]>
Acked-by: Dinh Nguyen <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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/[email protected]/
V2:
- new patch, based on the discussion in [1].
[1] https://lore.kernel.org/lkml/[email protected]/
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 <linux/kernel.h>
#include <linux/mfd/altera-sysmgr.h>
#include <linux/mfd/syscon.h>
-#include <linux/notifier.h>
#include <linux/of_address.h>
#include <linux/of_irq.h>
#include <linux/of_platform.h>
@@ -24,6 +23,7 @@
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/types.h>
+#include <linux/kexec.h>
#include <linux/uaccess.h>
#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,
&dberror);
regmap_write(edac->ecc_mgr_map, S10_SYSMGR_UE_VAL_OFST, dberror);
--
2.37.2
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 <[email protected]>
> Cc: Johannes Berg <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> 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
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 <[email protected]>
> Cc: Cong Wang <[email protected]>
> Cc: Sebastian Andrzej Siewior <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Xiaoming Ni <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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
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) <[email protected]>
> Cc: Dexuan Cui <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: "K. Y. Srinivasan" <[email protected]>
> Cc: Michael Kelley <[email protected]>
> Cc: Stephen Hemminger <[email protected]>
> Cc: Tianyu Lan <[email protected]>
> Cc: Wei Liu <[email protected]>
> Tested-by: Fabio A M Martins <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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/[email protected]/
>
>
> 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
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 <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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
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 <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Cc: Tony Luck <[email protected]>
> Acked-by: Dinh Nguyen <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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/[email protected]/
>
> V2:
> - new patch, based on the discussion in [1].
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> [...]
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/[email protected]/
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 <[email protected]>
> Cc: Sergei Shtylyov <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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
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 <[email protected]>
> Cc: Michael Kelley <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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/[email protected]/
>
>
> 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(®s, 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(&stop_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/[email protected]/
----- Ursprüngliche Mail -----
> Von: "Guilherme G. Piccoli" <[email protected]>
> 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 <[email protected]>
>> Cc: Johannes Berg <[email protected]>
>> Cc: Richard Weinberger <[email protected]>
>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>>
>> 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
On 18/09/2022 18:19, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Guilherme G. Piccoli" <[email protected]>
>> 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 <[email protected]>
>>> Cc: Johannes Berg <[email protected]>
>>> Cc: Richard Weinberger <[email protected]>
>>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>>>
>>> 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!
From: Guilherme G. Piccoli <[email protected]> Sent: Friday, August 19, 2022 3:17 PM
>
> 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) <[email protected]>
> Cc: Dexuan Cui <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: "K. Y. Srinivasan" <[email protected]>
> Cc: Michael Kelley <[email protected]>
> Cc: Stephen Hemminger <[email protected]>
> Cc: Tianyu Lan <[email protected]>
> Cc: Wei Liu <[email protected]>
> Tested-by: Fabio A M Martins <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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/[email protected]/
>
>
> 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 = &channel->outbound;
> +
> + return spin_is_locked(&rinfo->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
Reviewed-by: Michael Kelley <[email protected]>
From: Guilherme G. Piccoli <[email protected]> Sent: Friday, August 19, 2022 3:18 PM
>
> 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) <[email protected]>
> Cc: Dexuan Cui <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: "K. Y. Srinivasan" <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Cc: Stephen Hemminger <[email protected]>
> Cc: Tianyu Lan <[email protected]>
> Cc: Wei Liu <[email protected]>
> Reviewed-by: Michael Kelley <[email protected]>
> Tested-by: Fabio A M Martins <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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/[email protected]/
>
>
> drivers/hv/vmbus_drv.c | 109 +++++++++++++++++++-------------
> drivers/video/fbdev/hyperv_fb.c | 8 +++
> 2 files changed, 74 insertions(+), 43 deletions(-)
>
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 <[email protected]>
> 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 <linux/sched/task_stack.h>
>
> #include <linux/delay.h>
> -#include <linux/notifier.h>
> #include <linux/panic_notifier.h>
> #include <linux/ptrace.h>
> #include <linux/screen_info.h>
> @@ -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_crash(struct notifier_block *self,
> + unsigned long val, void *args)
> {
> struct pt_regs *regs;
> + bool is_die;
>
> - vmbus_initiate_unload(true);
> -
> - /*
> - * Hyper-V should be notified only once about a panic. If we will be
> - * doing hv_kmsg_dump() with kmsg data later, don't do the notification
> - * here.
> - */
> - if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE
> - && hyperv_report_reg()) {
> + /* Don't notify Hyper-V unless we have a die oops event or panic. */
> + if (self == &hyperv_panic_report_block) {
> + is_die = false;
> regs = current_pt_regs();
> - hyperv_report_panic(regs, val, false);
> + } else { /* die event */
> + if (val != DIE_OOPS)
> + return NOTIFY_DONE;
> +
> + is_die = true;
> + regs = ((struct die_args *)args)->regs;
> }
> - return NOTIFY_DONE;
> -}
> -
> -static int hyperv_die_event(struct notifier_block *nb, unsigned long val,
> - void *args)
> -{
> - struct die_args *die = args;
> - struct pt_regs *regs = die->regs;
> -
> - /* Don't notify Hyper-V if the die event is other than oops */
> - if (val != DIE_OOPS)
> - return NOTIFY_DONE;
>
> /*
> - * Hyper-V should be notified only once about a panic. If we will be
> - * doing hv_kmsg_dump() with kmsg data later, don't do the notification
> - * here.
> + * Hyper-V should be notified only once about a panic/die. If we will
> + * be calling hv_kmsg_dump() later with kmsg data, don't do the
> + * notification here.
> */
> if (hyperv_report_reg())
> - hyperv_report_panic(regs, val, true);
> + hyperv_report_panic(regs, val, is_die);
> +
> return NOTIFY_DONE;
> }
>
> -static struct notifier_block hyperv_die_block = {
> - .notifier_call = hyperv_die_event,
> -};
> -static struct notifier_block hyperv_panic_block = {
> - .notifier_call = hyperv_panic_event,
> -};
> -
> static const char *fb_mmio_name = "fb_range";
> static struct resource *fb_mmio;
> static struct resource *hyperv_mmio;
> @@ -1538,16 +1558,17 @@ static int vmbus_bus_init(void)
> if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
> hv_kmsg_dump_register();
>
> - register_die_notifier(&hyperv_die_block);
> + register_die_notifier(&hyperv_die_report_block);
> + atomic_notifier_chain_register(&panic_notifier_list,
> + &hyperv_panic_report_block);
> }
>
> /*
> - * Always register the panic notifier because we need to unload
> - * the VMbus channel connection to prevent any VMbus
> - * activity after the VM panics.
> + * Always register the vmbus unload panic notifier because we
> + * need to shut the VMbus channel connection on panic.
> */
> atomic_notifier_chain_register(&panic_notifier_list,
> - &hyperv_panic_block);
> + &hyperv_panic_vmbus_unload_block);
>
> vmbus_request_offers();
>
> @@ -2776,15 +2797,17 @@ static void __exit vmbus_exit(void)
>
> if (ms_hyperv.misc_features &
> HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
> kmsg_dump_unregister(&hv_kmsg_dumper);
> - unregister_die_notifier(&hyperv_die_block);
> + unregister_die_notifier(&hyperv_die_report_block);
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &hyperv_panic_report_block);
> }
>
> /*
> - * The panic notifier is always registered, hence we should
> + * The vmbus panic notifier is always registered, hence we should
> * also unconditionally unregister it here as well.
> */
> atomic_notifier_chain_unregister(&panic_notifier_list,
> - &hyperv_panic_block);
> + &hyperv_panic_vmbus_unload_block);
>
> free_page((unsigned long)hv_panic_page);
> unregister_sysctl_table(hv_ctl_table_hdr);
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index e1b65a01fb96..9234d31d4305 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -1216,7 +1216,15 @@ static int hvfb_probe(struct hv_device *hdev,
> par->fb_ready = true;
>
> par->synchronous_fb = false;
> +
> + /*
> + * We need to be sure this panic notifier runs _before_ the
> + * vmbus disconnect, so order it by priority. It must execute
> + * before the function hv_panic_vmbus_unload() [drivers/hv/vmbus_drv.c],
> + * which is almost at the end of list, with priority = INT_MIN + 1.
> + */
> par->hvfb_panic_nb.notifier_call = hvfb_on_panic;
> + par->hvfb_panic_nb.priority = INT_MIN + 10,
> atomic_notifier_chain_register(&panic_notifier_list,
> &par->hvfb_panic_nb);
>
> --
> 2.37.2
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 <[email protected]>
>
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
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 <[email protected]>
>> Cc: Petr Mladek <[email protected]>
>> Cc: Tony Luck <[email protected]>
>> Acked-by: Dinh Nguyen <[email protected]>
>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>>
>> ---
>>
>> 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/[email protected]/
>>
>> V2:
>> - new patch, based on the discussion in [1].
>> [1] https://lore.kernel.org/lkml/[email protected]/
>>
>> [...]
>
> 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
On 18/09/2022 18:19, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Guilherme G. Piccoli" <[email protected]>
>> 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 <[email protected]>
>>> Cc: Johannes Berg <[email protected]>
>>> Cc: Richard Weinberger <[email protected]>
>>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>>>
>>> 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
----- Ursprüngliche Mail -----
> Von: "Guilherme G. Piccoli" <[email protected]>
> Hi Richard / Johannes, is there any news on this one?
> Thanks in advance,
It's upstream:
git.kernel.org/linus/758dfdb9185cf94160f20e85bbe05583e3cd4ff4
Thanks,
//richard
On 17/10/2022 11:10, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Guilherme G. Piccoli" <[email protected]>
>> 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
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 <[email protected]>
> Cc: Sergei Shtylyov <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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
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 <[email protected]>
> Cc: Cong Wang <[email protected]>
> Cc: Sebastian Andrzej Siewior <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Xiaoming Ni <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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
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 <[email protected]>
>> Cc: Michael Kelley <[email protected]>
>> Cc: Russell King <[email protected]>
>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>>
Monthly ping - let me know if there's something I should improve in
order this fix is considered!
Thanks,
Guilherme
On Mon, Oct 17, 2022 at 11:00:46AM -0300, Guilherme G. Piccoli wrote:
> 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 <[email protected]>
> >> Cc: Michael Kelley <[email protected]>
> >> Cc: Russell King <[email protected]>
> >> Signed-off-by: Guilherme G. Piccoli <[email protected]>
> >>
>
> 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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
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
From: Guilherme G. Piccoli <[email protected]> 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 <[email protected]>
> >
>
> 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
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
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
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
On Fri, 19 Aug 2022 19:17:26 -0300
"Guilherme G. Piccoli" <[email protected]> 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 <[email protected]>
> Cc: Sergei Shtylyov <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
Sorry for the late reply.
Acked-by: Steven Rostedt (Google) <[email protected]>
-- Steve
On 20/10/2022 18:29, Steven Rostedt wrote:
> [...]
> Sorry for the late reply.
>
> Acked-by: Steven Rostedt (Google) <[email protected]>
>
> -- 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
On Thu, 20 Oct 2022 18:53:43 -0300
"Guilherme G. Piccoli" <[email protected]> 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
On 20/10/2022 19:22, Steven Rostedt wrote:
> On Thu, 20 Oct 2022 18:53:43 -0300
> "Guilherme G. Piccoli" <[email protected]> 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
[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 <[email protected]> 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 <[email protected]>
>>>
>>
>> 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
>
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.
Thanks,
Wei.
> Thanks in advance,
>
>
> Guilherme
>
> On 17/10/2022 12:26, Michael Kelley (LINUX) wrote:
> > From: Guilherme G. Piccoli <[email protected]> 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 <[email protected]>
> >>>
> >>
> >> 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
> >
On Fri, Nov 11, 2022 at 10:47:17PM +0000, 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.
On 11/11/2022 20:16, Wei Liu wrote:
> On Fri, Nov 11, 2022 at 10:47:17PM +0000, 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 =)
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 <[email protected]>
> Cc: Cong Wang <[email protected]>
> Cc: Sebastian Andrzej Siewior <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Xiaoming Ni <[email protected]>
> Reviewed-by: Baoquan He <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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.
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 <[email protected]>
>> Cc: Petr Mladek <[email protected]>
>> Cc: Tony Luck <[email protected]>
>> Acked-by: Dinh Nguyen <[email protected]>
>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>>
>> ---
>>
>> 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/[email protected]/
>>
>> V2:
>> - new patch, based on the discussion in [1].
>> [1] https://lore.kernel.org/lkml/[email protected]/
>>
>> [...]
>
> 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.
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 <[email protected]>
> Cc: Cong Wang <[email protected]>
> Cc: Sebastian Andrzej Siewior <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Xiaoming Ni <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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
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 <[email protected]>
> Cc: Matt Turner <[email protected]>
> Cc: Richard Henderson <[email protected]>
> Reviewed-by: Petr Mladek <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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
On 20/10/2022 19:22, Steven Rostedt wrote:
> On Thu, 20 Oct 2022 18:53:43 -0300
> "Guilherme G. Piccoli" <[email protected]> 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...
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 <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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
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 <[email protected]>
> >> Cc: Petr Mladek <[email protected]>
> >> Cc: Tony Luck <[email protected]>
> >> Acked-by: Dinh Nguyen <[email protected]>
> >> Signed-off-by: Guilherme G. Piccoli <[email protected]>
> >>
> >> ---
> >>
> >> 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/[email protected]/
> >>
> >> V2:
> >> - new patch, based on the discussion in [1].
> >> [1] https://lore.kernel.org/lkml/[email protected]/
> >>
> >> [...]
> >
> > 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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri 2022-08-19 19:17:28, 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 <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Cc: Tony Luck <[email protected]>
> Acked-by: Dinh Nguyen <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> 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/[email protected]/
>
> V2:
> - new patch, based on the discussion in [1].
> [1] https://lore.kernel.org/lkml/[email protected]/
>
>
> 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 <linux/kernel.h>
> #include <linux/mfd/altera-sysmgr.h>
> #include <linux/mfd/syscon.h>
> -#include <linux/notifier.h>
> #include <linux/of_address.h>
> #include <linux/of_irq.h>
> #include <linux/of_platform.h>
> @@ -24,6 +23,7 @@
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/types.h>
> +#include <linux/kexec.h>
> #include <linux/uaccess.h>
>
> #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;
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/[email protected]
> + edac = to_a10edac(this, panic_notifier);
> regmap_read(edac->ecc_mgr_map, S10_SYSMGR_ECC_INTSTAT_DERR_OFST,
> &dberror);
> regmap_write(edac->ecc_mgr_map, S10_SYSMGR_UE_VAL_OFST, dberror);
Best Regards,
Petr
On Tue, 13 Dec 2022 20:51:07 -0300
"Guilherme G. Piccoli" <[email protected]> wrote:
> > Acked-by: Steven Rostedt (Google) <[email protected]>
> >
> > -- Steve
>
> Hi Steve, since you were so kind in the other patch...decided to ping in
> this one as well heheh
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.
>
> Thanks again and sorry for the annoyance!
No, sorry for missing these.
-- Steve
On 20/10/2022 18:29, Steven Rostedt wrote:
> On Fri, 19 Aug 2022 19:17:26 -0300
> "Guilherme G. Piccoli" <[email protected]> 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 <[email protected]>
>> Cc: Sergei Shtylyov <[email protected]>
>> Cc: Steven Rostedt <[email protected]>
>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> Sorry for the late reply.
>
> Acked-by: Steven Rostedt (Google) <[email protected]>
>
> -- 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
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 =))
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 <[email protected]>
>>>> Cc: Petr Mladek <[email protected]>
>>>> Cc: Tony Luck <[email protected]>
>>>> Acked-by: Dinh Nguyen <[email protected]>
>>>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>>>>
>>>> ---
>>>>
>>>> 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/[email protected]/
>>>>
>>>> V2:
>>>> - new patch, based on the discussion in [1].
>>>> [1] https://lore.kernel.org/lkml/[email protected]/
>>>>
>>>> [...]
>>>
>>> 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/[email protected]
>
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