config_intrhook: provide config_intrhook_drain

[imp]

Summary

config_intrhook_drain will remove the hook from the list as
config_intrhook_disestablish does if the hook hasn't been called. If it has,
config_intrhook_drain will wait for the hook to be disestablished in the normal
course (or expidited, it's up to the driver to decide how and when
to call config_intrhook_disestablish).

This is indtended for removable devices that use config_intrhook and might be
attached early in boot, but that may be removed before the kernel can call the
config_intrhook or before it ends. To prevent all races, the detach routine
will need to call config_intrhook_train.

[imp]

update with done and man page

[imp]

Rework, per mav's suggestion.

[jhb]

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

share/man/man9/config_intrhook.9

62–64

65–67

68–70

sys/kern/subr_autoconf.c

275

[jhb]

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.

[imp]

Tweaks

[imp]

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.

[imp]

man page tweak I missed from jhb

[jhb]

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.