Re: if_txtimer API to replace (*if_watchdog)()

[Jason Thorpe <thorpej%me.com@localhost>]

+ad@ +riastradh@

> On Jan 22, 2020, at 10:56 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> 
>> *
>> *     If an interlock is provided, the interlock will be acquired
>> *     before checking for timer expiration, and will invoke the
>> *     callback with the interlock held if the timer has expired.
>> *     NOTE: the callback is responsible for releasing the interlock.
> 
> Why?  I think we should keep lock/unlock in the same place,
> at least in the same layer, as much as possible unless there is
> a critical reason to do so.

Andrew / Taylor, would appreciate you weighing in on this, as well.

I went back-and-forth on this point.  My thinking there is:

a) The interlock must be taken in order to test for timer expiration.

b) The error handling in the callback will need to drop the lock if it decides to reset the interface.

c) If I drop the lock in if_txtimer_tick() before calling the callback, then the state of the timer will be indeterminate when the callback is called; the callback would have to reacquire the lock and re-check for timer expiration.

This is why I settled on this asymmetry.  I would add a KASSERT() to check the lock state on return from the callback, but I don't think it's possible to safely check mutex_owned() on a SPIN mutex.  Andrew, can you comment on this?

> We need to teach if_txtimer_tick not to call callout_schedule
> somehow on destruction, otherwise if_txtimer_tick can continue
> to run even after calling callout_halt.  wm avoids the flaw by
> checking sc_core_stopping at the beginning of wm_tick.

This is a general problem in all drivers that use callouts, and if_txtimer_tick() doesn't really make it any worse than existing drivers that use tick callouts themselves.  I think even wm(4)'s approach has a problem in the detach case.  I have some thoughts on how to solve this problem generally, and will attack this problem at a future time.

-- thorpej