Re: [RFT v2] tty/sysrq: Make sysrq handler NMI aware

[Peter Zijlstra <peterz@infradead.org>]

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?