Page MenuHomeFreeBSD

config_intrhook: provide config_intrhook_drain
ClosedPublic

Authored by imp on Mar 2 2021, 12:00 AM.

Details

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.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp requested review of this revision.Mar 2 2021, 12:00 AM
imp created this revision.

update with done and man page

Rework, per mav's suggestion.

This revision is now accepted and ready to land.Mar 4 2021, 2:33 PM

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
61–63
64–66
67–69
sys/kern/subr_autoconf.c
274

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.

This revision now requires review to proceed.Mar 8 2021, 5:18 PM
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.

man page tweak I missed from jhb

imp marked 4 inline comments as done.Mar 8 2021, 9:45 PM
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.

This revision is now accepted and ready to land.Mar 10 2021, 8:19 PM
This revision was automatically updated to reflect the committed changes.