Hi Peter, Thanks for your review. On Mon, 28 Feb 2022 at 17:37, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote: > > On Mon, Feb 28, 2022 at 01:23:51PM +0530, Sumit Garg wrote: > > Allow a magic sysrq to be triggered from an NMI context. This is done > > *why* though? > I should have copied the reasoning from v1 cover letter [1] to this single patch as well. Will do it in v3. The basic idea is to enhance kernel's NMI debuggability for CPUs stuck in hard lockups. As an example, one should be able to launch kdb as well as other diagnostics offered by magic sysrq in NMI context. [1] https://lore.kernel.org/linux-arm-kernel/1595333413-30052-1-git-send-email-sumit.garg@xxxxxxxxxx/ > > > +#define SYSRQ_NMI_FIFO_SIZE 2 > > +static DEFINE_KFIFO(sysrq_nmi_fifo, int, SYSRQ_NMI_FIFO_SIZE); > > + > > +static void sysrq_do_nmi_work(struct irq_work *work) > > That naming don't make sense, it does the !NMI work, from IRQ context. > Will rename it to sysrq_do_irq_work(). > > +{ > > + const struct sysrq_key_op *op_p; > > + int orig_suppress_printk; > > + int key; > > + > > + orig_suppress_printk = suppress_printk; > > + suppress_printk = 0; > > + > > + rcu_sysrq_start(); > > + rcu_read_lock(); > > + > > + if (kfifo_peek(&sysrq_nmi_fifo, &key)) { > > + op_p = __sysrq_get_key_op(key); > > + if (op_p) > > + op_p->handler(key); > > + } > > + > > + rcu_read_unlock(); > > + rcu_sysrq_end(); > > + > > + suppress_printk = orig_suppress_printk; > > + > > + kfifo_reset_out(&sysrq_nmi_fifo); > > +} > > + > > +static DEFINE_IRQ_WORK(sysrq_nmi_work, sysrq_do_nmi_work); > > + > > void __handle_sysrq(int key, bool check_mask) > > { > > const struct sysrq_key_op *op_p; > > @@ -573,6 +612,10 @@ void __handle_sysrq(int key, bool check_mask) > > int orig_suppress_printk; > > int i; > > > > + /* Skip sysrq handling if one already in progress */ > > + if (!kfifo_is_empty(&sysrq_nmi_fifo)) > > + return; > > + > > orig_suppress_printk = suppress_printk; > > suppress_printk = 0; > > > > @@ -596,7 +639,13 @@ void __handle_sysrq(int key, bool check_mask) > > if (!check_mask || sysrq_on_mask(op_p->enable_mask)) { > > pr_info("%s\n", op_p->action_msg); > > console_loglevel = orig_log_level; > > - op_p->handler(key); > > + > > + if (in_nmi() && !op_p->nmi_safe) { > > + kfifo_put(&sysrq_nmi_fifo, key); > > + irq_work_queue(&sysrq_nmi_work); > > + } else { > > + op_p->handler(key); > > + } > > } else { > > pr_info("This sysrq operation is disabled.\n"); > > console_loglevel = orig_log_level; > > I'm missing the point of that kfifo stuff; afaict it only ever buffers > _1_ key, might as well use a simple variable, no? Yeah you are right, using a single key buffer should also suffice. The original idea was to queue multiple sysrq and handle them one by one but that turned out to be unsafe. -Sumit
