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?
> +#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.
> +{
> + 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?
