Messages in this thread | | | Date | Tue, 4 May 2004 17:57:37 +0200 | From | Mikael Pettersson <> | Subject | Re: [PATCH] allow drivers to claim the lapic NMI watchdog HW |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |