Re: [PATCH RFC 2/2] checkpatch: warn on usage of VM_BUG_ON() and friends
On 24.08.22 18:52, Joe Perches wrote:
> On Wed, 2022-08-24 at 18:31 +0200, David Hildenbrand wrote:
>> checkpatch does not point out that VM_BUG_ON() and friends should be
>> avoided, however, Linus notes:
>>
>> VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally
>> no different, the only difference is "we can make the code smaller
>> because these are less important". [1]
>>
>> So let's warn on VM_BUG_ON() and friends as well. While at it, make it
>> clearer that the kernel really shouldn't be crashed.
>>
>> Note that there are some other *_BUG_ON flavors, but they are not all
>> bad: for example, KVM_BUG_ON() only triggers a WARN_ON_ONCE and then
>> flags KVM as being buggy, so we'll not care about them for now here.
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -4695,12 +4695,12 @@ sub process {
>> }
>> }
>>
>> -# avoid BUG() or BUG_ON()
>> -if ($line =~ /\b(?:BUG|BUG_ON)\b/) {
>> +# do not use BUG(), BUG_ON(), VM_BUG_ON() and friends.
>> +if ($line =~ /\b(?:BUG|BUG_ON|VM_BUG_ON|VM_BUG_ON_[A-Z]+)\b/) {
>
> Perhaps better as something like the below to pick up more variants
>
> if ($line =~
> /\b(?!KVM_|BUILD_)(?:[A-Z_]*_)?BUG(?:_ON)?(?:_[A-Z_]+)?\s*\(/
... then I'll have to scan the other cases if they do something similar
as KVM. ... well, okay, I'll do it. :)
>
>> my $msg_level = \&WARN;
>> $msg_level = \&CHK if ($file);
>> &{$msg_level}("AVOID_BUG",
>> - "Avoid crashing the kernel - try using
>> WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr);
>
> and maybe:
>
> "Do not crash the kernel unless it is
> unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than
> BUG() or variants\n" . $herecurr);
>
>
Yes, thanks!
--
Thanks,
David / dhildenb
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH RFC 2/2] checkpatch: warn on usage of VM_BUG_ON() and friends
On Wed, 2022-08-24 at 18:31 +0200, David Hildenbrand wrote:
> checkpatch does not point out that VM_BUG_ON() and friends should be
> avoided, however, Linus notes:
>
> VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally
> no different, the only difference is "we can make the code smaller
> because these are less important". [1]
>
> So let's warn on VM_BUG_ON() and friends as well. While at it, make it
> clearer that the kernel really shouldn't be crashed.
>
> Note that there are some other *_BUG_ON flavors, but they are not all
> bad: for example, KVM_BUG_ON() only triggers a WARN_ON_ONCE and then
> flags KVM as being buggy, so we'll not care about them for now here.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -4695,12 +4695,12 @@ sub process {
> }
> }
>
> -# avoid BUG() or BUG_ON()
> - if ($line =~ /\b(?:BUG|BUG_ON)\b/) {
> +# do not use BUG(), BUG_ON(), VM_BUG_ON() and friends.
> + if ($line =~ /\b(?:BUG|BUG_ON|VM_BUG_ON|VM_BUG_ON_[A-Z]+)\b/) {
Perhaps better as something like the below to pick up more variants
if ($line =~
/\b(?!KVM_|BUILD_)(?:[A-Z_]*_)?BUG(?:_ON)?(?:_[A-Z_]+)?\s*\(/
> my $msg_level = \&WARN;
> $msg_level = \&CHK if ($file);
> &{$msg_level}("AVOID_BUG",
> - "Avoid crashing the kernel - try using
> WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr);
and maybe:
"Do not crash the kernel unless it is
unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than
BUG() or variants\n" . $herecurr);
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH RFC 0/2] coding-style.rst: document BUG() and WARN() rules
As it seems to be rather unclear if/when to use BUG(), BUG_ON(),
VM_BUG_ON(), WARN_ON_ONCE(), ... let's try to document the result of a
recent discussion.
Details can be found in patch #1.
--
Here is some braindump after thinking about BUG_ON(), WARN_ON(), ... and
how it interacts with kdump.
I was wondering what the expectation on a system with armed kdump are,
for example, after we removed most BUG_ON() instances and replaced them
by WARN_ON_ONCE(). I would assume that we actually want to panic in some
cases to capture a proper system dump instead of continuing and eventually
ending up with a completely broken system where it's hard to extract any
useful debug information. We'd have to enable panic_on_warn. But we'd only
want to do that in case kdump is actually armed after boot.
So one idea would be to have some kind of "panic_on_warn_with_kdump" mode.
But then, we'd actually crash+kdump even on the most harmless WARN_ON()
conditions, because they all look alike. To compensate, we would need
some kind of "severity" levels of a warning -- at least some kind of
"this is harmless and we can easily recover, but please tell the
developers" vs. "this is real bad and unexpected, capture a dump
immediately instead of trying to recover and eventually failing miserably".
But then, maybe we really want something like BUG_ON() -- let's call it
CBUG_ON() for simplicity -- but be able to make it be usable in
conditionals (to implement recovery code if easily possible) and make the
runtime behavior configurable.
if (CBUG_ON(whatever))
try_to_recover()
Whereby, for example, "panic_on_cbug" and "panic_on_cbug_with_kdump"
could control the runtime behavior.
But this is just a braindump and I assume people reading along have other,
better ideas. Especially, a better name for CBUG.
Cc: Linus Torvalds
Cc: Andrew Morton
Cc: Ingo Molnar
Cc: David Laight
Cc: Jonathan Corbet
Cc: Andy Whitcroft
Cc: Joe Perches
Cc: Dwaipayan Ray
Cc: Lukas Bulwahn
Cc: Baoquan He
Cc: Vivek Goyal
Cc: Dave Young
David Hildenbrand (2):
coding-style.rst: document BUG() and WARN() rules ("do not crash the
kernel")
checkpatch: warn on usage of VM_BUG_ON() and friends
Documentation/process/coding-style.rst | 27 ++
scripts/checkpatch.pl | 6 +++---
2 files changed, 30 insertions(+), 3 deletions(-)
--
2.37.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH RFC 1/2] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
Linus notes [1] that the introduction of new code that uses VM_BUG_ON() is just as bad as BUG_ON(), because it will crash the kernel on distributions that enable CONFIG_DEBUG_VM (like Fedora): VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally no different, the only difference is "we can make the code smaller because these are less important". [2] This resulted in a more generic discussion about usage of BUG() and friends. While there might be corner cases that still deserve a BUG_ON(), most BUG_ON() cases should simply use WARN_ON_ONCE() and implement a recovery path if reasonable: The only possible case where BUG_ON can validly be used is "I have some fundamental data corruption and cannot possibly return an error". [2] As a very good approximation is the general rule: "absolutely no new BUG_ON() calls _ever_" [2] ... not even if something really shouldn't ever happen and is merely for documenting that an invariant always has to hold. There is only one good BUG_ON(): Now, that said, there is one very valid sub-form of BUG_ON(): BUILD_BUG_ON() is absolutely 100% fine. [2] While WARN will also crash the machine with panic_on_warn set, that's exactly to be expected: So we have two very different cases: the "virtual machine with good logging where a dead machine is fine" - use 'panic_on_warn'. And the actual real hardware with real drivers, running real loads by users. [3] The basic idea is that warnings will similarly get reported by users and be found during testing. However, in contrast to a BUG(), there is a way to actually influence the expected behavior (e.g., panic_on_warn) and to eventually keep the machine alive to extract some debug info. Ingo notes that not all WARN_ON_ONCE cases need recovery. If we don't ever expect this code to trigger in any case, recovery code is not really helpful. I'd prefer to keep all these warnings 'simple' - i.e. no attempted recovery & control flow, unless we ever expect these to trigger. [4] There have been different rules floating around that were never properly documented. Let's try to clarify. [1] https://lkml.kernel.org/r/CAHk-=wiEAH+ojSpAgx_Ep=nkpwhu8ado3v56bxccsu97oyj...@mail.gmail.com [2] https://lore.kernel.org/r/CAHk-=wg40eazofo16eviaj7mfqdhz2gvebvfsmf6gyzsprj...@mail.gmail.com [3] https://lore.kernel.org/r/CAHk-=wgF7K2gSSpy=m_=k3nov4zaceux9puqf1tjktjla2x...@mail.gmail.com Signed-off-by: David Hildenbrand --- Documentation/process/coding-style.rst | 27 ++ 1 file changed, 27 insertions(+) diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst index 03eb53fd029a..a6d81ff578fe 100644 --- a/Documentation/process/coding-style.rst +++ b/Documentation/process/coding-style.rst @@ -1186,6 +1186,33 @@ expression used. For instance: #endif /* CONFIG_SOMETHING */ +22) Do not crash the kernel +--- + +Do not add new code that uses BUG(), BUG_ON(), VM_BUG_ON(), ... to crash +the kernel if certain conditions are not met. Instead, use WARN_ON_ONCE() +with recovery code (if reasonable) instead. Unavoidable data corruption / +security issues might be a very rare exception to this rule and need good +justification. + +There is no need for WARN_ON_ONCE() recovery code if we never expect it to +possibly trigger unless something goes seriously wrong or some other code +is changed to break invariants. Note that VM_WARN_ON_ONCE() cannot be used +in conditionals. + +Usage of WARN() and friends is fine for something that is not expected to +be triggered easily. ``panic_on_warn`` users are not an excuse to not use +WARN(): whoever enables ``panic_on_warn`` explicitly asked the kernel to +crash in this case. + +However, WARN() and friends should not be used for something that is +expected to trigger easily, for example, by user space. pr_warn_once() +might be a reasonable replacement to notify the user. + +Note that BUILD_BUG_ON() is perfectly fine because it will make compilation +fail instead of crashing the kernel. + + Appendix I) References -- -- 2.37.1 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH RFC 2/2] checkpatch: warn on usage of VM_BUG_ON() and friends
checkpatch does not point out that VM_BUG_ON() and friends should be
avoided, however, Linus notes:
VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally
no different, the only difference is "we can make the code smaller
because these are less important". [1]
So let's warn on VM_BUG_ON() and friends as well. While at it, make it
clearer that the kernel really shouldn't be crashed.
Note that there are some other *_BUG_ON flavors, but they are not all
bad: for example, KVM_BUG_ON() only triggers a WARN_ON_ONCE and then
flags KVM as being buggy, so we'll not care about them for now here.
[1]
https://lore.kernel.org/r/CAHk-=wg40eazofo16eviaj7mfqdhz2gvebvfsmf6gyzsprj...@mail.gmail.com
Signed-off-by: David Hildenbrand
---
scripts/checkpatch.pl | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 79e759aac543..4c18acf17032 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4695,12 +4695,12 @@ sub process {
}
}
-# avoid BUG() or BUG_ON()
- if ($line =~ /\b(?:BUG|BUG_ON)\b/) {
+# do not use BUG(), BUG_ON(), VM_BUG_ON() and friends.
+ if ($line =~ /\b(?:BUG|BUG_ON|VM_BUG_ON|VM_BUG_ON_[A-Z]+)\b/) {
my $msg_level = \&WARN;
$msg_level = \&CHK if ($file);
&{$msg_level}("AVOID_BUG",
- "Avoid crashing the kernel - try using
WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr);
+ "Do not crash the kernel unless it is
unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than
BUG(), BUG_ON(), VM_BUG_ON(), ...\n" . $herecurr);
}
# avoid LINUX_VERSION_CODE
--
2.37.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 09/11] video/hyperv_fb: Avoid taking busy spinlock on panic path
The Hyper-V framebuffer code registers a panic notifier in order
to try updating its fbdev if the kernel crashed. The notifier
callback is straightforward, but it calls the vmbus_sendpacket()
routine eventually, and such function takes a spinlock for the
ring buffer operations.
Panic path runs in atomic context, with local interrupts and
preemption disabled, and all secondary CPUs shutdown. That said,
taking a spinlock might cause a lockup if a secondary CPU was
disabled with such lock taken. Fix it here by checking if the
ring buffer spinlock is busy on Hyper-V framebuffer panic notifier;
if so, bail-out avoiding the potential lockup scenario.
Cc: Andrea Parri (Microsoft)
Cc: Dexuan Cui
Cc: Haiyang Zhang
Cc: "K. Y. Srinivasan"
Cc: Michael Kelley
Cc: Stephen Hemminger
Cc: Tianyu Lan
Cc: Wei Liu
Tested-by: Fabio A M Martins
Signed-off-by: Guilherme G. Piccoli
---
V3:
- simplified the code based on Michael's suggestion - thanks!
V2:
- new patch, based on the discussion in [0].
[0]
https://lore.kernel.org/lkml/2787b476-6366-1c83-db80-0393da417...@igalia.com/
drivers/hv/ring_buffer.c| 13 +
drivers/video/fbdev/hyperv_fb.c | 8 +++-
include/linux/hyperv.h | 2 ++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 59a4aa86d1f3..c6692fd5ab15 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -280,6 +280,19 @@ void hv_ringbuffer_cleanup(struct hv_ring_buffer_info
*ring_info)
ring_info->pkt_buffer_size = 0;
}
+/*
+ * Check if the ring buffer spinlock is available to take or not; used on
+ * atomic contexts, like panic path (see the Hyper-V framebuffer driver).
+ */
+
+bool hv_ringbuffer_spinlock_busy(struct vmbus_channel *channel)
+{
+ struct hv_ring_buffer_info *rinfo = &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
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 02/11] notifier: Add panic notifiers info and purge trailing whitespaces
Although many notifiers are mentioned in the comments, the panic notifiers infrastructure is not. Also, the file contains some trailing whitespaces. Fix both issues here. Cc: Arjan van de Ven Cc: Cong Wang Cc: Sebastian Andrzej Siewior Cc: Valentin Schneider Cc: Xiaoming Ni Reviewed-by: Baoquan He Signed-off-by: Guilherme G. Piccoli --- V3: - Added Baoquan review tag - thanks! V2: - No changes. include/linux/notifier.h | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/include/linux/notifier.h b/include/linux/notifier.h index aef88c2d1173..d5b01f2e3fcc 100644 --- a/include/linux/notifier.h +++ b/include/linux/notifier.h @@ -208,12 +208,12 @@ static inline int notifier_to_errno(int ret) /* * Declared notifiers so far. I can imagine quite a few more chains - * over time (eg laptop power reset chains, reboot chain (to clean + * over time (eg laptop power reset chains, reboot chain (to clean * device units up), device [un]mount chain, module load/unload chain, - * low memory chain, screenblank chain (for plug in modular screenblankers) + * low memory chain, screenblank chain (for plug in modular screenblankers) * VC switch chains (for loadable kernel svgalib VC switch helpers) etc... */ - + /* CPU notfiers are defined in include/linux/cpu.h. */ /* netdevice notifiers are defined in include/linux/netdevice.h */ @@ -224,6 +224,8 @@ static inline int notifier_to_errno(int ret) /* Virtual Terminal events are defined in include/linux/vt.h. */ +/* Panic notifiers are defined in include/linux/panic_notifier.h. */ + #define NETLINK_URELEASE 0x0001 /* Unicast netlink socket released */ /* Console keyboard events. -- 2.37.2 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 01/11] ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths
Currently the regular CPU shutdown path for ARM disables IRQs/FIQs
in the secondary CPUs - smp_send_stop() calls ipi_cpu_stop(), which
is responsible for that. IRQs are architecturally masked when we
take an interrupt, but FIQs are high priority than IRQs, hence they
aren't masked. With that said, it makes sense to disable FIQs here,
but there's no need for (re-)disabling IRQs.
More than that: there is an alternative path for disabling CPUs,
in the form of function crash_smp_send_stop(), which is used for
kexec/panic path. This function relies on a SMP call that also
triggers a busy-wait loop [at machine_crash_nonpanic_core()], but
without disabling FIQs. This might lead to odd scenarios, like
early interrupts in the boot of kexec'd kernel or even interrupts
in secondary "disabled" CPUs while the main one still works in the
panic path and assumes all secondary CPUs are (really!) off.
So, let's disable FIQs in both paths and *not* disable IRQs a second
time, since they are already masked in both paths by the architecture.
This way, we keep both CPU quiesce paths consistent and safe.
Cc: Marc Zyngier
Cc: Michael Kelley
Cc: Russell King
Signed-off-by: Guilherme G. Piccoli
---
V3:
- No changes.
V2:
- Small wording improvement (thanks Michael Kelley);
- Only disable FIQs, since IRQs are masked by architecture
definition when we take an interrupt. Thanks a lot Russell
and Marc for the discussion [0].
Should we add a Fixes tag here? If so, maybe the proper target is:
b23065313297 ("ARM: 6522/1: kexec: Add call to non-crashing cores through IPI")
[0] https://lore.kernel.org/lkml/ymxcaqy6dwhoq...@shell.armlinux.org.uk/
arch/arm/kernel/machine_kexec.c | 2 ++
arch/arm/kernel/smp.c | 5 ++---
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index f567032a09c0..0b482bcb97f7 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -77,6 +77,8 @@ void machine_crash_nonpanic_core(void *unused)
{
struct pt_regs regs;
+ local_fiq_disable();
+
crash_setup_regs(®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
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 06/11] tracing: Improve panic/die notifiers
Currently the tracing dump_on_oops feature is implemented through
separate notifiers, one for die/oops and the other for panic;
given they have the same functionality, let's unify them.
Also improve the function comment and change the priority of the
notifier to make it execute earlier, avoiding showing useless trace
data (like the callback names for the other notifiers); finally,
we also removed an unnecessary header inclusion.
Cc: Petr Mladek
Cc: Sergei Shtylyov
Cc: Steven Rostedt
Signed-off-by: Guilherme G. Piccoli
---
V3:
- Removed goto usage, as per Steven suggestion (thanks!).
V2:
- Different approach; instead of using IDs to distinguish die and
panic events, rely on address comparison like other notifiers do
and as per Petr's suggestion;
- Removed ACK from Steven since the code changed.
kernel/trace/trace.c | 55 ++--
1 file changed, 27 insertions(+), 28 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d3005279165d..0bacdbcb132f 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -19,7 +19,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -9776,40 +9775,40 @@ static __init int tracer_init_tracefs(void)
fs_initcall(tracer_init_tracefs);
-static int trace_panic_handler(struct notifier_block *this,
- unsigned long event, void *unused)
-{
- if (ftrace_dump_on_oops)
- ftrace_dump(ftrace_dump_on_oops);
- return NOTIFY_OK;
-}
+static int trace_die_panic_handler(struct notifier_block *self,
+ unsigned long ev, void *unused);
static struct notifier_block trace_panic_notifier = {
- .notifier_call = trace_panic_handler,
- .next = NULL,
- .priority = 150 /* priority: INT_MAX >= x >= 0 */
+ .notifier_call = trace_die_panic_handler,
+ .priority = INT_MAX - 1,
};
-static int trace_die_handler(struct notifier_block *self,
-unsigned long val,
-void *data)
-{
- switch (val) {
- case DIE_OOPS:
- if (ftrace_dump_on_oops)
- ftrace_dump(ftrace_dump_on_oops);
- break;
- default:
- break;
- }
- return NOTIFY_OK;
-}
-
static struct notifier_block trace_die_notifier = {
- .notifier_call = trace_die_handler,
- .priority = 200
+ .notifier_call = trace_die_panic_handler,
+ .priority = INT_MAX - 1,
};
+/*
+ * The idea is to execute the following die/panic callback early, in order
+ * to avoid showing irrelevant information in the trace (like other panic
+ * notifier functions); we are the 2nd to run, after hung_task/rcu_stall
+ * warnings get disabled (to prevent potential log flooding).
+ */
+static int trace_die_panic_handler(struct notifier_block *self,
+ unsigned long ev, void *unused)
+{
+ if (!ftrace_dump_on_oops)
+ return NOTIFY_DONE;
+
+ /* The die notifier requires DIE_OOPS to trigger */
+ if (self == &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
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 11/11] panic: Fixes the panic_print NMI backtrace setting
Commit 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in
panic_print")
introduced a setting for the "panic_print" kernel parameter to allow
users to request a NMI backtrace on panic. Problem is that the panic_print
handling happens after the secondary CPUs are already disabled, hence
this option ended-up being kind of a no-op - kernel skips the NMI trace
in idling CPUs, which is the case of offline CPUs.
Fix it by checking the NMI backtrace bit in the panic_print prior to
the CPU disabling function.
Fixes: 8d470a45d1a6 ("panic: add option to dump all CPUs backtraces in
panic_print")
Cc: Feng Tang
Cc: Petr Mladek
Signed-off-by: Guilherme G. Piccoli
---
V3:
- No changes.
V2:
- new patch, there was no V1 of this one.
Hi folks, thanks upfront for reviews. This is a new patch, fixing an issue
I found in my tests, so I shoved it into this fixes series.
Notice that while at it, I got rid of the "crash_kexec_post_notifiers"
local copy in panic(). This was introduced by commit b26e27ddfd2a
("kexec: use core_param for crash_kexec_post_notifiers boot option"),
but it is not clear from comments or commit message why this local copy
is required.
My understanding is that it's a mechanism to prevent some concurrency,
in case some other CPU modify this variable while panic() is running.
I find it very unlikely, hence I removed it - but if people consider
this copy needed, I can respin this patch and keep it, even providing a
comment about that, in order to be explict about its need.
Let me know your thoughts! Cheers,
Guilherme
kernel/panic.c | 47 +++
1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/kernel/panic.c b/kernel/panic.c
index c6eb8f8db0c0..b025a8f21c17 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -180,9 +180,6 @@ static void panic_print_sys_info(bool console_flush)
return;
}
- if (panic_print & PANIC_PRINT_ALL_CPU_BT)
- trigger_all_cpu_backtrace();
-
if (panic_print & PANIC_PRINT_TASK_INFO)
show_state();
@@ -199,6 +196,30 @@ static void panic_print_sys_info(bool console_flush)
ftrace_dump(DUMP_ALL);
}
+/*
+ * Helper that triggers the NMI backtrace (if set in panic_print)
+ * and then performs the secondary CPUs shutdown - we cannot have
+ * the NMI backtrace after the CPUs are off!
+ */
+static void panic_other_cpus_shutdown(void)
+{
+ if (panic_print & PANIC_PRINT_ALL_CPU_BT)
+ trigger_all_cpu_backtrace();
+
+ /*
+* Note that smp_send_stop() is the usual SMP shutdown function,
+* which unfortunately may not be hardened to work in a panic
+* situation. If we want to do crash dump after notifier calls
+* and kmsg_dump, we will need architecture dependent extra
+* bits in addition to stopping other CPUs, hence we rely on
+* crash_smp_send_stop() for that.
+*/
+ if (!crash_kexec_post_notifiers)
+ smp_send_stop();
+ else
+ crash_smp_send_stop();
+}
+
/**
* panic - halt the system
* @fmt: The text string to print
@@ -214,7 +235,6 @@ void panic(const char *fmt, ...)
long i, i_next = 0, len;
int state = 0;
int old_cpu, this_cpu;
- bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
if (panic_on_warn) {
/*
@@ -289,23 +309,10 @@ void panic(const char *fmt, ...)
*
* Bypass the panic_cpu check and call __crash_kexec directly.
*/
- if (!_crash_kexec_post_notifiers) {
+ if (!crash_kexec_post_notifiers)
__crash_kexec(NULL);
- /*
-* Note smp_send_stop is the usual smp shutdown function, which
-* unfortunately means it may not be hardened to work in a
-* panic situation.
-*/
- smp_send_stop();
- } else {
- /*
-* If we want to do crash dump after notifier calls and
-* kmsg_dump, we will need architecture dependent extra
-* works in addition to stopping other CPUs.
-*/
- crash_smp_send_stop();
- }
+ panic_other_cpus_shutdown();
/*
* Run any panic handlers, including those that might need to
@@ -326,7 +333,7 @@ void panic(const char *fmt, ...)
*
* Bypass the panic_cpu check and call __crash_kexec directly.
*/
- if (_crash_kexec_post_notifiers)
+ if (crash_kexec_post_notifiers)
__crash_kexec(NULL);
#ifdef CONFIG_VT
--
2.37.2
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 07/11] notifiers: Add tracepoints to the notifiers infrastructure
Currently there is no way to show the callback names for registered,
unregistered or executed notifiers. This is very useful for debug
purposes, hence add this functionality here in the form of notifiers'
tracepoints, one per operation.
Cc: Arjan van de Ven
Cc: Cong Wang
Cc: Sebastian Andrzej Siewior
Cc: Steven Rostedt
Cc: Valentin Schneider
Cc: Xiaoming Ni
Signed-off-by: Guilherme G. Piccoli
---
V3:
- Yet another major change - thanks to Arjan's great suggestion,
refactored the code to make use of tracepoints instead of guarding
the output with a Kconfig debug setting.
V2:
- Major improvement thanks to the great idea from Xiaoming - changed
all the ksym wheel reinvention to printk %ps modifier;
- Instead of ifdefs, using IS_ENABLED() - thanks Steven.
- Removed an unlikely() hint on debug path.
include/trace/events/notifiers.h | 69
kernel/notifier.c| 6 +++
2 files changed, 75 insertions(+)
create mode 100644 include/trace/events/notifiers.h
diff --git a/include/trace/events/notifiers.h b/include/trace/events/notifiers.h
new file mode 100644
index ..e8f30631aef5
--- /dev/null
+++ b/include/trace/events/notifiers.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM notifiers
+
+#if !defined(_TRACE_NOTIFIERS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_NOTIFIERS_H
+
+#include
+
+DECLARE_EVENT_CLASS(notifiers_info,
+
+ TP_PROTO(void *cb),
+
+ TP_ARGS(cb),
+
+ TP_STRUCT__entry(
+ __field(void *, cb)
+ ),
+
+ TP_fast_assign(
+ __entry->cb = cb;
+ ),
+
+ TP_printk("%ps", __entry->cb)
+);
+
+/*
+ * notifiers_register - called upon notifier callback registration
+ *
+ * @cb:callback pointer
+ *
+ */
+DEFINE_EVENT(notifiers_info, notifiers_register,
+
+ TP_PROTO(void *cb),
+
+ TP_ARGS(cb)
+);
+
+/*
+ * notifiers_unregister - called upon notifier callback unregistration
+ *
+ * @cb:callback pointer
+ *
+ */
+DEFINE_EVENT(notifiers_info, notifiers_unregister,
+
+ TP_PROTO(void *cb),
+
+ TP_ARGS(cb)
+);
+
+/*
+ * notifiers_run - called upon notifier callback execution
+ *
+ * @cb:callback pointer
+ *
+ */
+DEFINE_EVENT(notifiers_info, notifiers_run,
+
+ TP_PROTO(void *cb),
+
+ TP_ARGS(cb)
+);
+
+#endif /* _TRACE_NOTIFIERS_H */
+
+/* This part must be outside protection */
+#include
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 0d5bd62c480e..2f2783f59a31 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -7,6 +7,9 @@
#include
#include
+#define CREATE_TRACE_POINTS
+#include
+
/*
* Notifier list for kernel code which wants to be called
* at shutdown. This is used to stop any idling DMA operations
@@ -37,6 +40,7 @@ static int notifier_chain_register(struct notifier_block **nl,
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
+ trace_notifiers_register((void*)n->notifier_call);
return 0;
}
@@ -46,6 +50,7 @@ static int notifier_chain_unregister(struct notifier_block
**nl,
while ((*nl) != NULL) {
if ((*nl) == n) {
rcu_assign_pointer(*nl, n->next);
+ trace_notifiers_unregister((void*)n->notifier_call);
return 0;
}
nl = &((*nl)->next);
@@ -84,6 +89,7 @@ static int notifier_call_chain(struct notifier_block **nl,
continue;
}
#endif
+ trace_notifiers_run((void*)nb->notifier_call);
ret = nb->notifier_call(nb, val, v);
if (nr_calls)
--
2.37.2
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 00/11] The panic notifiers refactor - fixes/clean-ups (V3)
Hey everybody, this the third iteration of the panic notifiers fixes/clean-ups; V2 available at: https://lore.kernel.org/lkml/20220719195325.402745-1-gpicc...@igalia.com/ V1 (including the refactor) available at: https://lore.kernel.org/lkml/20220427224924.592546-1-gpicc...@igalia.com/ There wasn't much change here compared to V2 (the specifics are in the patches), but a global change is that I've rebased against 6.0-rc1. One patch got merged in -next, another one was re-submit in a standalone format (requested by maintainer), so both of these are not here anymore. As usual, tested this series building for all affected architecture/drivers and also through some boot/runtime tests; below the test "matrix" used: Build tests (using cross-compilers): alpha, arm, arm64, parisc, um, x86_64. Boot/Runtime tests: x86_64 (QEMU guests and Steam Deck). Here is the link with the .config files used: https://people.igalia.com/gpiccoli/panic_notifiers_configs/6.0-rc1/ About the merge strategy: I've noticed there is a difference in maintainers preferences (and my preference as well), so I see 3 strategies for merge: (a) Maintainers pick patches that are good from the series and merge in their trees; (b) Some maintainer would pick the whole series and merge, at once, given that everything is fine/ack/reviewed. (c) I must re-send patches individually once they are reviewed/acked, as standalone patches to the relevant maintainers, so they can merge it in their trees. I'm willing to do what's best for everybody - (a) is my choice when possible, (b) seems to stall things and potentially cause conflicts, (c) seems to be the compromise. I'll do that as per preference of the respective maintainers. As usual, reviews / comments are always welcome, thanks in advance for them! Cheers, Guilherme Guilherme G. Piccoli (11): ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths notifier: Add panic notifiers info and purge trailing whitespaces alpha: Clean-up the panic notifier code um: Improve panic notifiers consistency and ordering parisc: Replace regular spinlock with spin_trylock on panic path tracing: Improve panic/die notifiers notifiers: Add tracepoints to the notifiers infrastructure EDAC/altera: Skip the panic notifier if kdump is loaded video/hyperv_fb: Avoid taking busy spinlock on panic path drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers panic: Fixes the panic_print NMI backtrace setting arch/alpha/kernel/setup.c| 36 +- arch/arm/kernel/machine_kexec.c | 2 + arch/arm/kernel/smp.c| 5 +- arch/parisc/include/asm/pdc.h| 1 + arch/parisc/kernel/firmware.c| 27 ++-- arch/um/drivers/mconsole_kern.c | 7 +- arch/um/kernel/um_arch.c | 8 +-- drivers/edac/altera_edac.c | 16 +++-- drivers/hv/ring_buffer.c | 13 drivers/hv/vmbus_drv.c | 109 +++ drivers/parisc/power.c | 17 +++-- drivers/video/fbdev/hyperv_fb.c | 16 - include/linux/hyperv.h | 2 + include/linux/notifier.h | 8 ++- include/trace/events/notifiers.h | 69 +++ kernel/notifier.c| 6 ++ kernel/panic.c | 47 +++-- kernel/trace/trace.c | 55 18 files changed, 302 insertions(+), 142 deletions(-) create mode 100644 include/trace/events/notifiers.h -- 2.37.2 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 03/11] alpha: Clean-up the panic notifier code
The alpha panic notifier has some code issues, not following
the conventions of other notifiers. Also, it might halt the
machine but still it is set to run as early as possible, which
doesn't seem to be a good idea.
So, let's clean the code and set the notifier to run as the
latest, following the same approach other architectures are
doing - also, remove the unnecessary include of a header already
included indirectly.
Cc: Ivan Kokshaysky
Cc: Matt Turner
Cc: Richard Henderson
Reviewed-by: Petr Mladek
Signed-off-by: Guilherme G. Piccoli
---
V3:
- No changes.
V2:
- Fixed rth email address;
- Added Petr's review tag - thanks!
arch/alpha/kernel/setup.c | 36 +++-
1 file changed, 15 insertions(+), 21 deletions(-)
diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index b4fbbba30aa2..d88bdf852753 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -41,19 +41,11 @@
#include
#include
#endif
-#include
#include
#include
#include
#include
-static int alpha_panic_event(struct notifier_block *, unsigned long, void *);
-static struct notifier_block alpha_panic_block = {
- alpha_panic_event,
-NULL,
-INT_MAX /* try to do it first */
-};
-
#include
#include
#include
@@ -435,6 +427,21 @@ static const struct sysrq_key_op srm_sysrq_reboot_op = {
};
#endif
+static int alpha_panic_event(struct notifier_block *this,
+unsigned long event, void *ptr)
+{
+ /* If we are using SRM and serial console, just hard halt here. */
+ if (alpha_using_srm && srmcons_output)
+ __halt();
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block alpha_panic_block = {
+ .notifier_call = alpha_panic_event,
+ .priority = INT_MIN, /* may not return, do it last */
+};
+
void __init
setup_arch(char **cmdline_p)
{
@@ -1427,19 +1434,6 @@ const struct seq_operations cpuinfo_op = {
.show = show_cpuinfo,
};
-
-static int
-alpha_panic_event(struct notifier_block *this, unsigned long event, void *ptr)
-{
-#if 1
- /* FIXME FIXME FIXME */
- /* If we are using SRM and serial console, just hard halt here. */
- if (alpha_using_srm && srmcons_output)
- __halt();
-#endif
-return NOTIFY_DONE;
-}
-
static __init int add_pcspkr(void)
{
struct platform_device *pd;
--
2.37.2
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 10/11] drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers
Currently Hyper-V guests are among the most relevant users of the panic
infrastructure, like panic notifiers, kmsg dumpers, etc. The reasons rely
both in cleaning-up procedures (closing hypervisor <-> guest connection,
disabling some paravirtualized timer) as well as to data collection
(sending panic information to the hypervisor) and framebuffer management.
The thing is: some notifiers are related to others, ordering matters, some
functionalities are duplicated and there are lots of conditionals behind
sending panic information to the hypervisor. As part of an effort to
clean-up the panic notifiers mechanism and better document things, we
hereby address some of the issues/complexities of Hyper-V panic handling
through the following changes:
(a) We have die and panic notifiers on vmbus_drv.c and both have goals of
sending panic information to the hypervisor, though the panic notifier is
also responsible for a cleaning-up procedure.
This commit clears the code by splitting the panic notifier in two, one
for closing the vmbus connection whereas the other is only for sending
panic info to hypervisor. With that, it was possible to merge the die and
panic notifiers in a single/well-documented function, and clear some
conditional complexities on sending such information to the hypervisor.
(b) There is a Hyper-V framebuffer panic notifier, which relies in doing
a vmbus operation that demands a valid connection. So, we must order this
notifier with the panic notifier from vmbus_drv.c, to guarantee that the
framebuffer code executes before the vmbus connection is unloaded.
Also, this commit removes a useless header.
Although there is code rework and re-ordering, we expect that this change
has no functional regressions but instead optimize the path and increase
panic reliability on Hyper-V. This was tested on Hyper-V with success.
Cc: Andrea Parri (Microsoft)
Cc: Dexuan Cui
Cc: Haiyang Zhang
Cc: "K. Y. Srinivasan"
Cc: Petr Mladek
Cc: Stephen Hemminger
Cc: Tianyu Lan
Cc: Wei Liu
Reviewed-by: Michael Kelley
Tested-by: Fabio A M Martins
Signed-off-by: Guilherme G. Piccoli
---
V3:
- Added Michael's review tag - thanks!
V2:
- Unfortunately we cannot rely in the crash shutdown (custom) handler
to perform the vmbus unload - arm64 architecture doesn't have this
"feature" [0]. So, in V2 we kept the notifier behavior and always
unload the vmbus connection, no matter what - thanks Michael for
pointing that;
- Removed the Fixes tags as per Michael suggestion;
- As per Petr suggestion, we abandoned the idea of distinguish among
notifiers using an id - so, in V2 we rely in the old and good address
comparison for that. Thanks Petr for the enriching discussion!
[0]
https://lore.kernel.org/lkml/427a8277-49f0-4317-d6c3-4a15d7070...@igalia.com/
drivers/hv/vmbus_drv.c | 109 +++-
drivers/video/fbdev/hyperv_fb.c | 8 +++
2 files changed, 74 insertions(+), 43 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 23c680d1a0f5..0a77e2bb0b70 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -25,7 +25,6 @@
#include
#include
-#include
#include
#include
#include
@@ -69,53 +68,74 @@ static int hyperv_report_reg(void)
return !sysctl_record_panic_msg || !hv_panic_page;
}
-static int hyperv_panic_event(struct notifier_block *nb, unsigned long val,
+/*
+ * The panic notifier below is responsible solely for unloading the
+ * vmbus connection, which is necessary in a panic event.
+ *
+ * Notice an intrincate relation of this notifier with Hyper-V
+ * framebuffer panic notifier exists - we need vmbus connection alive
+ * there in order to succeed, so we need to order both with each other
+ * [see hvfb_on_panic()] - this is done using notifiers' priorities.
+ */
+static int hv_panic_vmbus_unload(struct notifier_block *nb, unsigned long val,
void *args)
+{
+ vmbus_initiate_unload(true);
+ return NOTIFY_DONE;
+}
+static struct notifier_block hyperv_panic_vmbus_unload_block = {
+ .notifier_call = hv_panic_vmbus_unload,
+ .priority = INT_MIN + 1, /* almost the latest one to execute */
+};
+
+static int hv_die_panic_notify_crash(struct notifier_block *self,
+unsigned long val, void *args);
+
+static struct notifier_block hyperv_die_report_block = {
+ .notifier_call = hv_die_panic_notify_crash,
+};
+static struct notifier_block hyperv_panic_report_block = {
+ .notifier_call = hv_die_panic_notify_crash,
+};
+
+/*
+ * The following callback works both as die and panic notifier; its
+ * goal is to provide panic information to the hypervisor unless the
+ * kmsg dumper is used [see hv_kmsg_dump()], which provides more
+ * information but isn't always available.
+ *
+ * Notice that both the panic/die report notifiers are registered only
+ * if we have the capability HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE set.
+ */
+static int
[PATCH V3 04/11] um: Improve panic notifiers consistency and ordering
Currently the panic notifiers from user mode linux don't follow
the convention for most of the other notifiers present in the
kernel (indentation, priority setting, numeric return).
More important, the priorities could be improved, since it's a
special case (userspace), hence we could run the notifiers earlier;
user mode linux shouldn't care much with other panic notifiers but
the ordering among the mconsole and arch notifier is important,
given that the arch one effectively triggers a core dump.
Fix that by running the mconsole notifier as the first panic
notifier, followed by the architecture one (that coredumps).
Cc: Anton Ivanov
Cc: Johannes Berg
Cc: Richard Weinberger
Signed-off-by: Guilherme G. Piccoli
V3:
- No changes.
V2:
- Kept the notifier header to avoid implicit usage - thanks
Johannes for the suggestion!
---
arch/um/drivers/mconsole_kern.c | 7 +++
arch/um/kernel/um_arch.c| 8
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index 8ca67a692683..69af3ce8407a 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -846,13 +846,12 @@ static int notify_panic(struct notifier_block *self,
unsigned long unused1,
mconsole_notify(notify_socket, MCONSOLE_PANIC, message,
strlen(message) + 1);
- return 0;
+ return NOTIFY_DONE;
}
static struct notifier_block panic_exit_notifier = {
- .notifier_call = notify_panic,
- .next = NULL,
- .priority = 1
+ .notifier_call = notify_panic,
+ .priority = INT_MAX, /* run as soon as possible */
};
static int add_notifier(void)
diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index e0de60e503b9..ae272878f692 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -247,13 +247,13 @@ static int panic_exit(struct notifier_block *self,
unsigned long unused1,
bust_spinlocks(0);
uml_exitcode = 1;
os_dump_core();
- return 0;
+
+ return NOTIFY_DONE;
}
static struct notifier_block panic_exit_notifier = {
- .notifier_call = panic_exit,
- .next = NULL,
- .priority = 0
+ .notifier_call = panic_exit,
+ .priority = INT_MAX - 1, /* run as 2nd notifier, won't return */
};
void uml_finishsetup(void)
--
2.37.2
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 05/11] parisc: Replace regular spinlock with spin_trylock on panic path
The panic notifiers' callbacks execute in an atomic context, with
interrupts/preemption disabled, and all CPUs not running the panic
function are off, so it's very dangerous to wait on a regular
spinlock, there's a risk of deadlock.
Refactor the panic notifier of parisc/power driver to make use
of spin_trylock - for that, we've added a second version of the
soft-power function. Also, some comments were reorganized and
trailing white spaces, useless header inclusion and blank lines
were removed.
Cc: "James E.J. Bottomley"
Cc: Jeroen Roovers
Acked-by: Helge Deller # parisc
Signed-off-by: Guilherme G. Piccoli
---
V3:
- s/in/on as per Jeroen's suggestion - thanks!
V2:
- Added Helge's ACK - thanks!
arch/parisc/include/asm/pdc.h | 1 +
arch/parisc/kernel/firmware.c | 27 +++
drivers/parisc/power.c| 17 ++---
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/arch/parisc/include/asm/pdc.h b/arch/parisc/include/asm/pdc.h
index b643092d4b98..7a106008e258 100644
--- a/arch/parisc/include/asm/pdc.h
+++ b/arch/parisc/include/asm/pdc.h
@@ -83,6 +83,7 @@ int pdc_do_firm_test_reset(unsigned long ftc_bitmap);
int pdc_do_reset(void);
int pdc_soft_power_info(unsigned long *power_reg);
int pdc_soft_power_button(int sw_control);
+int pdc_soft_power_button_panic(int sw_control);
void pdc_io_reset(void);
void pdc_io_reset_devices(void);
int pdc_iodc_getc(void);
diff --git a/arch/parisc/kernel/firmware.c b/arch/parisc/kernel/firmware.c
index 6a7e315bcc2e..3b1f7641e3c9 100644
--- a/arch/parisc/kernel/firmware.c
+++ b/arch/parisc/kernel/firmware.c
@@ -1232,15 +1232,18 @@ int __init pdc_soft_power_info(unsigned long *power_reg)
}
/*
- * pdc_soft_power_button - Control the soft power button behaviour
- * @sw_control: 0 for hardware control, 1 for software control
+ * pdc_soft_power_button{_panic} - Control the soft power button behaviour
+ * @sw_control: 0 for hardware control, 1 for software control
*
*
* This PDC function places the soft power button under software or
* hardware control.
- * Under software control the OS may control to when to allow to shut
- * down the system. Under hardware control pressing the power button
+ * Under software control the OS may control to when to allow to shut
+ * down the system. Under hardware control pressing the power button
* powers off the system immediately.
+ *
+ * The _panic version relies on spin_trylock to prevent deadlock
+ * on panic path.
*/
int pdc_soft_power_button(int sw_control)
{
@@ -1254,6 +1257,22 @@ int pdc_soft_power_button(int sw_control)
return retval;
}
+int pdc_soft_power_button_panic(int sw_control)
+{
+ int retval;
+ unsigned long flags;
+
+ if (!spin_trylock_irqsave(&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
#include
#include
-#include
#include
#include
#include
@@ -175,16 +174,21 @@ static void powerfail_interrupt(int code, void *x)
-/* parisc_panic_event() is called by the panic handler.
- * As soon as a panic occurs, our tasklets above will not be
- * executed any longer. This function then re-enables the
- * soft-power switch and allows the user to switch off the system
+/*
+ * parisc_panic_event() is called by the panic handler.
+ *
+ * As soon as a panic occurs, our tasklets above will not
+ * be executed any longer. This function then re-enables
+ * the soft-power switch and allows the user to switch off
+ * the system. We rely in pdc_soft_power_button_panic()
+ * since this version spin_trylocks (instead of regular
+ * spinlock), preventing deadlocks on panic path.
*/
static int parisc_panic_event(struct notifier_block *this,
unsigned long event, void *ptr)
{
/* re-enable the soft-power switch */
- pdc_soft_power_button(0);
+ pdc_soft_power_button_panic(0);
return NOTIFY_DONE;
}
@@ -193,7 +197,6 @@ static struct notifier_block parisc_panic_block = {
.priority = INT_MAX,
};
-
static int __init power_init(void)
{
unsigned long ret;
--
2.37.2
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH V3 08/11] EDAC/altera: Skip the panic notifier if kdump is loaded
The altera_edac panic notifier performs some data collection with
regards errors detected; such code relies in the regmap layer to
perform reads/writes, so the code is abstracted and there is some
risk level to execute that, since the panic path runs in atomic
context, with interrupts/preemption and secondary CPUs disabled.
Users want the information collected in this panic notifier though,
so in order to balance the risk/benefit, let's skip the altera panic
notifier if kdump is loaded. While at it, remove a useless header
and encompass a macro inside the sole ifdef block it is used.
Cc: Borislav Petkov
Cc: Petr Mladek
Cc: Tony Luck
Acked-by: Dinh Nguyen
Signed-off-by: Guilherme G. Piccoli
---
V3:
- added the ack tag from Dinh - thanks!
- had a good discussion with Boris about that in V2 [0],
hopefully we can continue and reach a consensus in this V3.
[0]
https://lore.kernel.org/lkml/46137c67-25b4-6657-33b7-cffdc7afc...@igalia.com/
V2:
- new patch, based on the discussion in [1].
[1]
https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa2138527...@igalia.com/
drivers/edac/altera_edac.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index e7e8e624a436..741fe5539154 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -16,7 +16,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -24,6 +23,7 @@
#include
#include
#include
+#include
#include
#include "altera_edac.h"
@@ -2063,22 +2063,30 @@ static const struct irq_domain_ops a10_eccmgr_ic_ops = {
};
/** Stratix 10 EDAC Double Bit Error Handler /
-#define to_a10edac(p, m) container_of(p, struct altr_arria10_edac, m)
-
#ifdef CONFIG_64BIT
/* panic routine issues reboot on non-zero panic_timeout */
extern int panic_timeout;
+#define to_a10edac(p, m) container_of(p, struct altr_arria10_edac, m)
+
/*
* The double bit error is handled through SError which is fatal. This is
* called as a panic notifier to printout ECC error info as part of the panic.
+ *
+ * Notice that if kdump is set, we take the risk avoidance approach and
+ * skip the notifier, given that users are expected to have access to a
+ * full vmcore.
*/
static int s10_edac_dberr_handler(struct notifier_block *this,
unsigned long event, void *ptr)
{
- struct altr_arria10_edac *edac = to_a10edac(this, panic_notifier);
+ struct altr_arria10_edac *edac;
int err_addr, dberror;
+ if (kexec_crash_loaded())
+ return NOTIFY_DONE;
+
+ edac = to_a10edac(this, panic_notifier);
regmap_read(edac->ecc_mgr_map, S10_SYSMGR_ECC_INTSTAT_DERR_OFST,
&dberror);
regmap_write(edac->ecc_mgr_map, S10_SYSMGR_UE_VAL_OFST, dberror);
--
2.37.2
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH V3 0/9] Support RISCV64 arch and common commands
在 2022/8/22 下午3:28, Yixun Lan 写道:
Hi lijiang
On Mon, Aug 22, 2022 at 3:56 AM lijiang wrote:
Hi, Xianting
Thank you for the update.
On Sat, Aug 13, 2022 at 11:18 AM Xianting Tian
wrote:
This series of patches are for Crash-utility tool, it make crash tool support
RISCV64 arch and the common commands(*, bt, p, rd, mod, log, set, struct, task,
dis, help -r, help -m, and so on).
To make the crash tool work normally for RISCV64 arch, we need a Linux kernel
patch, which exports the kernel virtual memory layout, va_bits, phys_ram_base
to vmcoreinfo, it can simplify the development of crash tool.
The Linux kernel patch set:
https://lore.kernel.org/linux-riscv/20220811074150.3020189-1-xianting.t...@linux.alibaba.com/
[ Patch 1 ~ 4 already merged to Linux for-next branch, targeted for 5.20;
Patch 5, 6 already merged to Palmer's riscv-crash branch:
https://git.kernel.org/pub/scm/linux/kernel/git/palmer/linux.git/log/?h=riscv-crash
]
Can you also help to share the patch link for kexec-tools? Or share your
loading steps and generate a vmcore in riscv64 system?
I tried it with kexec-tools and got the following error:
[root@fedora-riscv kexec-tools]# ./configure
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking build system type... riscv64-unknown-linux-gnu
checking host system type... riscv64-unknown-linux-gnu
checking target system type... riscv64-unknown-linux-gnu
configure: error: unsupported architecture riscv64
Thanks.
Lianbo
This series of patches are tested on QEMU RISCV64 env and SoC platform of
T-head Xuantie 910 RISCV64 CPU.
Some test examples list as below
... ...
KERNEL: vmlinux
DUMPFILE: vmcore
CPUS: 1
DATE: Fri Jul 15 10:24:25 CST 2022
UPTIME: 00:00:33
LOAD AVERAGE: 0.05, 0.01, 0.00
TASKS: 41
NODENAME: buildroot
RELEASE: 5.18.9
VERSION: #30 SMP Fri Jul 15 09:47:03 CST 2022
MACHINE: riscv64 (unknown Mhz)
MEMORY: 1 GB
PANIC: "Kernel panic - not syncing: sysrq triggered crash"
PID: 113
COMMAND: "sh"
TASK: ff6002269600 [THREAD_INFO: ff6002269600]
CPU: 0
STATE: TASK_RUNNING (PANIC)
carsh>
crash> p mem_map
mem_map = $1 = (struct page *) 0xff603effbf00
crash> p /x *(struct page *) 0xff603effbf00
$5 = {
flags = 0x1000,
{
{
{
lru = {
next = 0xff603effbf08,
prev = 0xff603effbf08
},
{
__filler = 0xff603effbf08,
mlock_count = 0x3effbf08
}
},
mapping = 0x0,
index = 0x0,
private = 0x0
},
... ...
crash> mod
MODULE NAME BASE SIZE OBJECT FILE
0113e740 nvme_core 01133000 98304 (not loaded)
[CONFIG_KALLSYMS]
011542c0 nvme 0114c000 61440 (not loaded)
[CONFIG_KALLSYMS]
crash> rd 0113e740 8
0113e740: 810874f8 .t..
0113e750: 011542c8 726f635f656d766e .B..nvme_cor
0113e760: 0065 e...
0113e770:
crash> vtop 0113e740
VIRTUAL PHYSICAL
0113e740 8254d740
PGD: 810e9ff8 => 2001
P4D: => 2fffec01
PUD: 5605c2957470 => 20949801
PMD: 7fff7f1750c0 => 20947401
PTE: 0 => 209534e7
PAGE: 8254d000
PTE PHYSICAL FLAGS
209534e7 8254d000 (PRESENT|READ|WRITE|GLOBAL|ACCESSED|DIRTY)
PAGE PHYSICAL MAPPING INDEX CNT FLAGS
ff603f0777d8 8254d00000 1 0
crash> bt
PID: 113 TASK: ff600226c200 CPU: 0COMMAND: "sh"
#0 [ff2010333b90] riscv_crash_save_regs at 800078f8
#1 [ff2010333cf0] panic at 806578c6
#2 [ff2010333d50] sysrq_reset_seq_param_set at 8038c03c
#3 [ff2010333da0] __handle_sysrq at 8038c604
#4 [ff2010333e00] write_sysrq_trigger at 8038cae4
#5 [ff2010333e20] proc_reg_write at 801b7ee8
#6 [ff2010333e40] vfs_write at 80152bb2
#7 [ff2010333e80] ksys_write at 80152eda
#8 [ff2010333ed0] sys_write at 80152f52
---
Changes V1 -> V2:
1, Do the below fixes based on HAGIO KAZUHITO's comments:
Fix build warnings,
Use MACRO for Linux version,
Add description of x86_64 binary for riscv64 in README,
Fix build e
