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 <bp@xxxxxxxxx> > >> Cc: Petr Mladek <pmladek@xxxxxxxx> > >> Cc: Tony Luck <tony.luck@xxxxxxxxx> > >> Acked-by: Dinh Nguyen <dinguyen@xxxxxxxxxx> > >> Signed-off-by: Guilherme G. Piccoli <gpiccoli@xxxxxxxxxx> > >> > >> --- > >> > >> 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@xxxxxxxxxx/ > >> > >> V2: > >> - new patch, based on the discussion in [1]. > >> [1] https://lore.kernel.org/lkml/62a63fc2-346f-f375-043a-fa21385279df@xxxxxxxxxx/ > >> > >> [...] > > > > 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
