svn commit: r274493 - head/sys/dev/ath

[John Baldwin]

On Friday, November 14, 2014 04:26:26 AM Adrian Chadd wrote:
> Author: adrian
> Date: Fri Nov 14 04:26:26 2014
> New Revision: 274493
> URL: https://svnweb.freebsd.org/changeset/base/274493
> 
> Log:
>   Migrate the callouts from using mutex locks to being mpsafe with
>   the locks being held by the callers.
> 
>   Kill callout_drain() and use callout_stop().
> 
> Modified:
>   head/sys/dev/ath/if_ath.c
> 
> Modified: head/sys/dev/ath/if_ath.c
> ============================================================================
> == --- head/sys/dev/ath/if_ath.c	Fri Nov 14 00:25:10 2014	(r274492)
> +++ head/sys/dev/ath/if_ath.c	Fri Nov 14 04:26:26 2014	(r274493)
> @@ -669,8 +669,8 @@ ath_attach(u_int16_t devid, struct ath_s
>  		goto bad;
>  	}
> 
> -	callout_init_mtx(&sc->sc_cal_ch, &sc->sc_mtx, 0);
> -	callout_init_mtx(&sc->sc_wd_ch, &sc->sc_mtx, 0);
> +	callout_init(&sc->sc_cal_ch, 1);	/* MPSAFE */
> +	callout_init(&sc->sc_wd_ch, 1);		/* MPSAFE */

Why did you do this?  You probably don't want this.  You can use 
callout_init_mtx() with callout_stop(), and if you do it closes races
between your callout routine and callout_stop().  If you use callout_init()
instead, then you need to patch your callout routines to now explicitly
check your own private flag after doing ATH_LOCK().  That is, you can
either do:

foo_attach()
{
	callout_init_mtx(&sc->timer, &sc->mtx, 0);
}

foo_timeout(void *arg)
{
	sc = arg;
	mtx_assert(&sc->mtx, MA_OWNED);
	/* periodic actions */
	callout_schedule() /* or callout_reset */
}

foo_start()
{
	mtx_lock(&sc->mtx);
	callout_reset(&sc->timer, ...);
}

foo_stop()
{

	mtx_lock(&sc->mtx);
	callout_stop(&sc->timer);
	/* foo_timeout will not execute after this point */
	mtx_unlock(&sc->mtx);
}

or you can do this:

foo_attach()
{
	callout_init(&sc->timer, CALLOUT_MPSAFE);
}

foo_timeout(void *arg)
{
	sc = arg;

	mtx_lock(&sc->mtx);
	if (sc->stopped) {
		mtx_unlock(&sc->mtx);
		return;
	}

	/* periodic actions */
	callout_schedule()
	mtx_unlock(&sc->mtx);
}

foo_start()
{

	mtx_lock(&sc->mtx);
	foo->stopped = 0;
	callout_reset(&sc->timer, ...);
}

foo_stop()
{

	mtx_lock(&sc->mtx);
	foo->stopped = 1;
	callout_stop(&sc->timer);
	/*
     * foo_timeout might still execute, but it will see the 'stopped'
	 * flag and return
	 */
	mtx_unlock(&sc->mtx);
}

Personally, I find the former (using callout_init_mtx()) a lot simpler to read 
and work with.  As it is you can now either switch back to using 
callout_init_mtx() and undo the changes to ath_calibrate() (but leave all your
other changes in place), or you now need to add a new stopped flag that you
set / clear in the places you do callout_stop() / callout_reset() and add a
new check for the new stopped flag in ath_calibrate().

-- 
John Baldwin