Re: [PATCH] NMI trigger switch support for debugging

[Mikael Pettersson <mikpe@csd.uu.se>]

AKIYAMA Nobuyuki writes:
 > +int unknown_nmi_panic = 0;

It's a kernel coding standard to _not_ explicitly initialise
static-extent data to zero.

 > +/*
 > + * proc handler for /proc/sys/kernel/unknown_nmi_panic
 > + */
 > +int proc_unknown_nmi_panic(ctl_table *table, int write,
 > +                struct file *file, void __user *buffer, size_t *length)
 > +{
 > +	int old_state;
 > +
 > +	old_state = unknown_nmi_panic;
 > +	proc_dointvec(table, write, file, buffer, length);
 > +	if (!old_state == !unknown_nmi_panic)
 > +		return 0;

This conditional looks terribly obscure.
Can you simplify it or explain your intention here?

 > +	if (unknown_nmi_panic) {
 > +		if (reserve_lapic_nmi() < 0) {
 > +			unknown_nmi_panic = 0;
 > +			return -EBUSY;
 > +		} else {
 > +			set_nmi_callback(unknown_nmi_panic_callback);
 > +		}
 > +	} else {
 > +		release_lapic_nmi();

You're invoking release_lapic_nmi() in response to user
input, without having verified that _you_ had done a
reserve_lapic_nmi() before.

It looks like the code will do horrible things if the
operator invokes the sysctl incorrectly. Such errors do
happen, so code should include basic sanity checks.

/Mikael