Do we want to say anything about the fact you can't call drain if you haven't first called establish in the manpage?

The idea is sound, and I think the implementation is generally good.

s/expidited/expedited/ and s/indtended/intended/ in the log.

Note that it's slightly different from callout_drain() in that callout_drain() silently "drops" calls to callout_reset() by refusing to reschedule a callout once it is being drained. In this case, this relies on the driver to be a good citizen and not call establish again while the hook is running if the driver has called disestablish. You could handle this by adding an ICHS_DRAINING that you set before you enter the sleep, and making establish just bail without doing anything if it's called with the state set to ICHS_DRAINING. This might also really want to have a 'config_intrhook_init(hook, func, arg)' macro or function. That would be useful for setting a "safe" state of ICHS_INIT (which would be 0 which might be mostly backwards compat safe as these things are usually in softc's that are zeroed on allocation IIRC) so you don't end up with ICHS_DRAINING as the random garbage in ich_state that would prevent the initial establish from working.

In D29005#651098, @jhb wrote:

The idea is sound, and I think the implementation is generally good.

s/expidited/expedited/ and s/indtended/intended/ in the log.

Note that it's slightly different from callout_drain() in that callout_drain() silently "drops" calls to callout_reset() by refusing to reschedule a callout once it is being drained. In this case, this relies on the driver to be a good citizen and not call establish again while the hook is running if the driver has called disestablish. You could handle this by adding an ICHS_DRAINING that you set before you enter the sleep, and making establish just bail without doing anything if it's called with the state set to ICHS_DRAINING. This might also really want to have a 'config_intrhook_init(hook, func, arg)' macro or function. That would be useful for setting a "safe" state of ICHS_INIT (which would be 0 which might be mostly backwards compat safe as these things are usually in softc's that are zeroed on allocation IIRC) so you don't end up with ICHS_DRAINING as the random garbage in ich_state that would prevent the initial establish from working.

No drivers do this today. The contract is supposed to be establish, get your callback, do things and disestablish either immediately or after a interrupt-driver state-machine completes. There's no adding it back in the contract.... This is different than callouts.
Adding API requirements would make this non-MFCable, though in practice I think we're good (all the in-tree uses are part of softc (zeroed) or are malloced with M_ZERO).

So I generally agree it would be nice to make this more robust, but I'd disagree that it's in scope for this review, mostly because I don't want to botch any MFC potential.

In D29005#652304, @imp wrote:

In D29005#651098, @jhb wrote:

The idea is sound, and I think the implementation is generally good.

s/expidited/expedited/ and s/indtended/intended/ in the log.

Note that it's slightly different from callout_drain() in that callout_drain() silently "drops" calls to callout_reset() by refusing to reschedule a callout once it is being drained. In this case, this relies on the driver to be a good citizen and not call establish again while the hook is running if the driver has called disestablish. You could handle this by adding an ICHS_DRAINING that you set before you enter the sleep, and making establish just bail without doing anything if it's called with the state set to ICHS_DRAINING. This might also really want to have a 'config_intrhook_init(hook, func, arg)' macro or function. That would be useful for setting a "safe" state of ICHS_INIT (which would be 0 which might be mostly backwards compat safe as these things are usually in softc's that are zeroed on allocation IIRC) so you don't end up with ICHS_DRAINING as the random garbage in ich_state that would prevent the initial establish from working.

No drivers do this today. The contract is supposed to be establish, get your callback, do things and disestablish either immediately or after a interrupt-driver state-machine completes. There's no adding it back in the contract.... This is different than callouts.
Adding API requirements would make this non-MFCable, though in practice I think we're good (all the in-tree uses are part of softc (zeroed) or are malloced with M_ZERO).

So I generally agree it would be nice to make this more robust, but I'd disagree that it's in scope for this review, mostly because I don't want to botch any MFC potential.

Oh, I had misread the code in subr_autoconf.c and we don't need some sort of ICHS_DRAINING.