Re: [PATCH] allow drivers to claim the lapic NMI watchdog HW

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

John Levon writes:
 > On Tue, May 04, 2004 at 04:33:01AM +0200, Mikael Pettersson wrote:
 > 
 > > +/* lapic_nmi_owner:
 > > + * +1: the lapic NMI hardware is assigned to the lapic NMI watchdog
 > > + *  0: the lapic NMI hardware is unassigned
 > 
 > If we're going to have a mini state machine, can't we at least use some
 > defines for each state...
 > 
 > > +		lapic_nmi_owner -= 2; /* +1 -> -1, 0 -> -2 */
 > 
 > ...and make this into some readable english via a little helper?

Thing is, using discrete states makes the code for the checks
and state changes more verbose. However, I can easily hide the
representation behind macros with understandable names.

 > > -EXPORT_SYMBOL(disable_lapic_nmi_watchdog);
 > > -EXPORT_SYMBOL(enable_lapic_nmi_watchdog);
 > > +EXPORT_SYMBOL(reassign_lapic_nmi_watchdog);
 > > +EXPORT_SYMBOL(release_lapic_nmi_watchdog);
 > 
 > I don't like this new naming. Since the patch is really all about
 > ownership of the local APIC, can't we call it something like
 > 
 > acquire_lapic_nmi()
 > release_lapic_nmi()

Yep, those are nicer names.

 > Neither perfctr nor oprofile have anything to do with watchdogs, so
 > this:
 > 
 > > -	disable_lapic_nmi_watchdog();
 > > +	if (reassign_lapic_nmi_watchdog() < 0) {
 > 
 > Looks a little weird now.

A little, yes.

I'll implement these changes this evening and post an updated patch tomorrow.

/Mikael