Re: [PATCH] NMI trigger switch support for debugging

[AKIYAMA Nobuyuki <akiyama.nobuyuk@jp.fujitsu.com>]

Hi Mikael,

Thank you for reviewing.

Mikael Pettersson wrote:

>AKIYAMA Nobuyuki writes:
> > +int unknown_nmi_panic = 0;
>
>It's a kernel coding standard to _not_ explicitly initialise
>static-extent data to zero.
>
>  
>
OK, thanks.

> > +/*
> > + * 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?
>
>  
>
This code checks whether unknown_nmi_panic is changed to another state.
Only when state is changed, I'd like to go next step.

old_state  unknown_nmi_panic   condition
0          0                 : TRUE(no change, return)
none zero  none zero         : TRUE(no change, return)
0          none zero         : FALSE(changed, go next step)
none zero  0                 : FALSE(changed, go next step)


> > +	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.
>  
>

The only one user can obtain NMI callback.
If unknown_nmi_panic is 0 at this step, it says that I have obtained
NMI callback before.
So, I think invoking release_lapic_nmi() has no problem.


Regards,
Nobuyuki Akiyama