From: Petr Mladek <pmladek@suse.com> To: "Guilherme G. Piccoli" <gpiccoli@igalia.com> Cc: akpm@linux-foundation.org, bhe@redhat.com, kexec@lists.infradead.org, linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org, netdev@vger.kernel.org, x86@kernel.org, kernel-dev@igalia.com, kernel@gpiccoli.net, halves@canonical.com, fabiomirmar@gmail.com, alejandro.j.jimenez@oracle.com, andriy.shevchenko@linux.intel.com, arnd@arndb.de, bp@alien8.de, corbet@lwn.net, d.hatayama@jp.fujitsu.com, dave.hansen@linux.intel.com, dyoung@redhat.com, feng.tang@intel.com, gregkh@linuxfoundation.org, mikelley@microsoft.com, hidehiro.kawai.ez@hitachi.com, jgross@suse.com, john.ogness@linutronix.de, keescook@chromium.org, luto@kernel.org, mhiramat@kernel.org, mingo@redhat.com, paulmck@kernel.org, peterz@infradead.org, rostedt@goodmis.org, senozhatsky@chromium.org, stern@rowland.harvard.edu, tglx@linutronix.de, vgoyal@redhat.com, vkuznets@redhat.com, will@kernel.org, xuqiang36@huawei.com, linux-edac@vger.kernel.org, Tony Luck <tony.luck@intel.com>, Dinh Nguyen <dinguyen@kernel.org> Subject: Re: [PATCH V3 08/11] EDAC/altera: Skip the panic notifier if kdump is loaded Date: Fri, 9 Dec 2022 17:03:54 +0100 [thread overview] Message-ID: <Y5Ncaur0S4rEbath@alley> (raw) In-Reply-To: <20220819221731.480795-9-gpiccoli@igalia.com> 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 <bp@alien8.de> > Cc: Petr Mladek <pmladek@suse.com> > Cc: Tony Luck <tony.luck@intel.com> > Acked-by: Dinh Nguyen <dinguyen@kernel.org> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > > --- > > 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-cffdc7afc0d7@igalia.com/ > > V2: > - new patch, based on the discussion in [1]. > [1] https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa21385279df@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 <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/20220719195325.402745-11-gpiccoli@igalia.com > + 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
WARNING: multiple messages have this Message-ID (diff)
From: Petr Mladek <pmladek@suse.com> To: "Guilherme G. Piccoli" <gpiccoli@igalia.com> Cc: akpm@linux-foundation.org, bhe@redhat.com, kexec@lists.infradead.org, linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org, netdev@vger.kernel.org, x86@kernel.org, kernel-dev@igalia.com, kernel@gpiccoli.net, halves@canonical.com, fabiomirmar@gmail.com, alejandro.j.jimenez@oracle.com, andriy.shevchenko@linux.intel.com, arnd@arndb.de, bp@alien8.de, corbet@lwn.net, d.hatayama@jp.fujitsu.com, dave.hansen@linux.intel.com, dyoung@redhat.com, feng.tang@intel.com, gregkh@linuxfoundation.org, mikelley@microsoft.com, hidehiro.kawai.ez@hitachi.com, jgross@suse.com, john.ogness@linutronix.de, keescook@chromium.org, luto@kernel.org, mhiramat@kernel.org, mingo@redhat.com, paulmck@kernel.org, peterz@infradead.org, rostedt@goodmis.org, senozhatsky@chromium.org, stern@rowland.harvard.edu, tglx@linutronix.de, vgoyal@redhat.com, vkuznets@redhat.com, will@kernel.org, xuqiang36@huawei.com, linux-edac@vger.kernel.org, Tony Luck <tony.luck@intel.com>, Dinh Nguyen <dinguyen@kernel.org> Subject: Re: [PATCH V3 08/11] EDAC/altera: Skip the panic notifier if kdump is loaded Date: Fri, 9 Dec 2022 17:03:54 +0100 [thread overview] Message-ID: <Y5Ncaur0S4rEbath@alley> (raw) In-Reply-To: <20220819221731.480795-9-gpiccoli@igalia.com> 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 <bp@alien8.de> > Cc: Petr Mladek <pmladek@suse.com> > Cc: Tony Luck <tony.luck@intel.com> > Acked-by: Dinh Nguyen <dinguyen@kernel.org> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > > --- > > 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-cffdc7afc0d7@igalia.com/ > > V2: > - new patch, based on the discussion in [1]. > [1] https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa21385279df@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 <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/20220719195325.402745-11-gpiccoli@igalia.com > + 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 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
next prev parent reply other threads:[~2022-12-09 16:04 UTC|newest] Thread overview: 128+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-19 22:17 [PATCH V3 00/11] The panic notifiers refactor - fixes/clean-ups (V3) Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-08-19 22:17 ` [PATCH V3 01/11] ARM: Disable FIQs (but not IRQs) on CPUs shutdown paths Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-09-18 13:58 ` Guilherme G. Piccoli 2022-09-18 13:58 ` Guilherme G. Piccoli 2022-09-18 13:58 ` Guilherme G. Piccoli 2022-10-17 14:00 ` Guilherme G. Piccoli 2022-10-17 14:00 ` Guilherme G. Piccoli 2022-10-17 14:00 ` Guilherme G. Piccoli 2022-10-17 14:17 ` Russell King (Oracle) 2022-10-17 14:17 ` Russell King (Oracle) 2022-10-17 14:17 ` Russell King (Oracle) 2022-10-17 14:50 ` Guilherme G. Piccoli 2022-10-17 14:50 ` Guilherme G. Piccoli 2022-10-17 14:50 ` Guilherme G. Piccoli 2022-10-17 17:47 ` Russell King (Oracle) 2022-10-17 17:47 ` Russell King (Oracle) 2022-10-17 17:47 ` Russell King (Oracle) 2022-10-17 19:43 ` Guilherme G. Piccoli 2022-10-17 19:43 ` Guilherme G. Piccoli 2022-10-17 19:43 ` Guilherme G. Piccoli 2022-08-19 22:17 ` [PATCH V3 02/11] notifier: Add panic notifiers info and purge trailing whitespaces Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-11-22 13:19 ` Guilherme G. Piccoli 2022-11-22 13:19 ` Guilherme G. Piccoli 2022-08-19 22:17 ` [PATCH V3 03/11] alpha: Clean-up the panic notifier code Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-11-22 13:22 ` Guilherme G. Piccoli 2022-11-22 13:22 ` Guilherme G. Piccoli 2022-11-22 13:22 ` Guilherme G. Piccoli 2022-08-19 22:17 ` [PATCH V3 04/11] um: Improve panic notifiers consistency and ordering Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-09-18 14:00 ` Guilherme G. Piccoli 2022-09-18 14:00 ` Guilherme G. Piccoli 2022-09-18 14:00 ` Guilherme G. Piccoli 2022-09-18 21:19 ` Richard Weinberger 2022-09-18 21:19 ` Richard Weinberger 2022-09-18 21:19 ` Richard Weinberger 2022-09-19 11:44 ` Guilherme G. Piccoli 2022-09-19 11:44 ` Guilherme G. Piccoli 2022-09-19 11:44 ` Guilherme G. Piccoli 2022-10-17 14:01 ` Guilherme G. Piccoli 2022-10-17 14:01 ` Guilherme G. Piccoli 2022-10-17 14:10 ` Richard Weinberger 2022-10-17 14:10 ` Richard Weinberger 2022-10-17 14:22 ` Guilherme G. Piccoli 2022-10-17 14:22 ` Guilherme G. Piccoli 2022-08-19 22:17 ` [PATCH V3 05/11] parisc: Replace regular spinlock with spin_trylock on panic path Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-08-19 22:17 ` [PATCH V3 06/11] tracing: Improve panic/die notifiers Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-09-18 14:04 ` Guilherme G. Piccoli 2022-09-18 14:04 ` Guilherme G. Piccoli 2022-10-17 14:02 ` Guilherme G. Piccoli 2022-10-17 14:02 ` Guilherme G. Piccoli 2022-10-20 21:29 ` Steven Rostedt 2022-10-20 21:29 ` Steven Rostedt 2022-10-20 21:53 ` Guilherme G. Piccoli 2022-10-20 21:53 ` Guilherme G. Piccoli 2022-10-20 22:22 ` Steven Rostedt 2022-10-20 22:22 ` Steven Rostedt 2022-10-20 22:37 ` Guilherme G. Piccoli 2022-10-20 22:37 ` Guilherme G. Piccoli 2022-11-22 13:27 ` Guilherme G. Piccoli 2022-11-22 13:27 ` Guilherme G. Piccoli 2022-12-13 23:51 ` Guilherme G. Piccoli 2022-12-13 23:51 ` Guilherme G. Piccoli 2022-12-14 0:06 ` Steven Rostedt 2022-12-14 0:06 ` Steven Rostedt 2022-12-14 0:52 ` Guilherme G. Piccoli 2022-12-14 0:52 ` Guilherme G. Piccoli 2022-08-19 22:17 ` [PATCH V3 07/11] notifiers: Add tracepoints to the notifiers infrastructure Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-09-18 14:07 ` Guilherme G. Piccoli 2022-09-18 14:07 ` Guilherme G. Piccoli 2022-10-17 14:04 ` Guilherme G. Piccoli 2022-10-17 14:04 ` Guilherme G. Piccoli 2022-11-22 13:30 ` Guilherme G. Piccoli 2022-11-22 13:30 ` Guilherme G. Piccoli 2022-08-19 22:17 ` [PATCH V3 08/11] EDAC/altera: Skip the panic notifier if kdump is loaded Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-09-18 14:10 ` Guilherme G. Piccoli 2022-09-18 14:10 ` Guilherme G. Piccoli 2022-10-17 14:05 ` Guilherme G. Piccoli 2022-10-17 14:05 ` Guilherme G. Piccoli 2022-11-22 13:33 ` Guilherme G. Piccoli 2022-11-22 13:33 ` Guilherme G. Piccoli 2022-11-22 15:06 ` Borislav Petkov 2022-11-22 15:06 ` Borislav Petkov 2023-02-10 16:01 ` Guilherme G. Piccoli 2023-02-10 16:01 ` Guilherme G. Piccoli 2022-12-09 16:03 ` Petr Mladek [this message] 2022-12-09 16:03 ` Petr Mladek 2022-08-19 22:17 ` [PATCH V3 09/11] video/hyperv_fb: Avoid taking busy spinlock on panic path Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-09-18 14:12 ` Guilherme G. Piccoli 2022-09-18 14:12 ` Guilherme G. Piccoli 2022-10-04 16:17 ` Michael Kelley (LINUX) 2022-10-04 16:17 ` Michael Kelley (LINUX) 2022-08-19 22:17 ` [PATCH V3 10/11] drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-10-04 16:24 ` Michael Kelley (LINUX) 2022-10-04 16:24 ` Michael Kelley (LINUX) 2022-10-04 17:20 ` Guilherme G. Piccoli 2022-10-04 17:20 ` Guilherme G. Piccoli 2022-10-17 15:26 ` Michael Kelley (LINUX) 2022-10-17 15:26 ` Michael Kelley (LINUX) 2022-11-10 21:32 ` Guilherme G. Piccoli 2022-11-10 21:32 ` Guilherme G. Piccoli 2022-11-11 22:47 ` Wei Liu 2022-11-11 22:47 ` Wei Liu 2022-11-11 23:16 ` Wei Liu 2022-11-11 23:16 ` Wei Liu 2022-11-12 21:53 ` Guilherme G. Piccoli 2022-11-12 21:53 ` Guilherme G. Piccoli 2022-08-19 22:17 ` [PATCH V3 11/11] panic: Fixes the panic_print NMI backtrace setting Guilherme G. Piccoli 2022-08-19 22:17 ` Guilherme G. Piccoli 2022-09-18 14:13 ` Guilherme G. Piccoli 2022-09-18 14:13 ` Guilherme G. Piccoli 2022-11-22 13:35 ` Guilherme G. Piccoli 2022-11-22 13:35 ` Guilherme G. Piccoli
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Y5Ncaur0S4rEbath@alley \ --to=pmladek@suse.com \ --cc=akpm@linux-foundation.org \ --cc=alejandro.j.jimenez@oracle.com \ --cc=andriy.shevchenko@linux.intel.com \ --cc=arnd@arndb.de \ --cc=bhe@redhat.com \ --cc=bp@alien8.de \ --cc=corbet@lwn.net \ --cc=d.hatayama@jp.fujitsu.com \ --cc=dave.hansen@linux.intel.com \ --cc=dinguyen@kernel.org \ --cc=dyoung@redhat.com \ --cc=fabiomirmar@gmail.com \ --cc=feng.tang@intel.com \ --cc=gpiccoli@igalia.com \ --cc=gregkh@linuxfoundation.org \ --cc=halves@canonical.com \ --cc=hidehiro.kawai.ez@hitachi.com \ --cc=jgross@suse.com \ --cc=john.ogness@linutronix.de \ --cc=keescook@chromium.org \ --cc=kernel-dev@igalia.com \ --cc=kernel@gpiccoli.net \ --cc=kexec@lists.infradead.org \ --cc=linux-edac@vger.kernel.org \ --cc=linux-hyperv@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --cc=mhiramat@kernel.org \ --cc=mikelley@microsoft.com \ --cc=mingo@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=paulmck@kernel.org \ --cc=peterz@infradead.org \ --cc=rostedt@goodmis.org \ --cc=senozhatsky@chromium.org \ --cc=stern@rowland.harvard.edu \ --cc=tglx@linutronix.de \ --cc=tony.luck@intel.com \ --cc=vgoyal@redhat.com \ --cc=vkuznets@redhat.com \ --cc=will@kernel.org \ --cc=x86@kernel.org \ --cc=xuqiang36@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.