Re: callout_stop => callout_halt

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   Date: Mon, 17 Nov 2014 01:22:59 +0900
   From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>

   Here is a patch that includes only ones that require interlock
   and rearrangements. Others can be fixed by simply replacing
   callout_stop with callout_halt (already committed).

   --- a/sys/dev/sysmon/sysmon_envsys_events.c
   +++ b/sys/dev/sysmon/sysmon_envsys_events.c
   @@ -612,11 +612,19 @@ sme_events_destroy(struct sysmon_envsys *sme)
    {
    	KASSERT(mutex_owned(&sme->sme_mtx));

   -	callout_stop(&sme->sme_callout);
   +	/*
   +	 * Unset before callout_halt to ensure callout is not scheduled again
   +	 * during callout_halt
   +	 */
   +	sme->sme_flags &= ~SME_CALLOUT_INITIALIZED;
   +
   +	mutex_enter(&sme->sme_callout_mtx);
   +	callout_halt(&sme->sme_callout, &sme->sme_callout_mtx);

Can't do callout_halt like that because although it unlocks
sme_callout_mtx, it will sleep while sme_mtx is held.

It's not clear why you have to hold sme_callout_mtx here, though -- or
why it even exists independently of sme_mtx.  The author of this code
failed to write down any locking rules!

My inclination is to nix sme_callout_mtx altogether and replace every
use of it by sme_mtx (there are obviously some races where one thread
holds one and another thread holds another and they try to touch the
same data structure), but I don't know this code well enough to say
for sure.

   +++ b/sys/dev/usb/ohci.c
   @@ -350,9 +350,7 @@ ohci_detach(struct ohci_softc *sc, int flags)
    	if (rv != 0)
    		return (rv);
    
   -	callout_stop(&sc->sc_tmo_rhsc);
   -
   -	usb_delay_ms(&sc->sc_bus, 300); /* XXX let stray task complete */
   +	callout_halt(&sc->sc_tmo_rhsc, &sc->sc_intr_lock);

I wouldn't remove the delay just yet -- there may be USB tasks, like I
mentioned earlier, still pending which this completely stupid delay
works around.  (If code inspection can rule out the possibility of
such USB tasks, though, go for it.)