newbus: Take the topolock in driver_module_handler()
AcceptedPublic
Actions

Authored by imp on Tue, Oct 28, 7:49 PM.

Details

Reviewers

jhb
markj

Summary

And sprinkler some asserts. Right now all module handlers are called
with Giant, but I'd like to drop that so push the newbus locking one
step further.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 68188
Build 65071: arc lint + arc unit

Event Timeline

imp created this revision.Tue, Oct 28, 7:49 PM

Herald added a subscriber: olce. · View Herald TranscriptTue, Oct 28, 7:49 PM

imp requested review of this revision.Tue, Oct 28, 7:49 PM

Harbormaster completed remote builds in B68188: Diff 165249.Tue, Oct 28, 7:49 PM

I haven't yet looked at Mark's changes to see how they conflict.

sys/kern/subr_bus.c
5273	Arguably, we should only create the devclass for MOD_LOAD (and pass `false` instead of `true` for `MOD_UNLOAD` and `MOD_QUIESCE`), and we should perhaps run the chained handlers not under the topo_lock (requiring the chained handlers to manage their own locking). You could fix both of those by pushing the devclass_find_internal() down into each case below, but that that's kind of tedious. This is probably fine for now.

emaste added a subscriber: emaste.Tue, Oct 28, 10:58 PM

imp added a child revision: D53451: newbus: Ensure that we hold topo lock for calls to devclass_find_internal.Wed, Oct 29, 6:09 PM

imp added reviewers: jhb, markj.Wed, Oct 29, 6:37 PM

imp added inline comments.

sys/kern/subr_bus.c
5273	Agreed. I'll do that as new patches in the series.

This change depends on D53451, not the other way around. If you commit just this one, the devclass_find_internal() assertion will fail in some cases.

In D53414#1220419, @markj wrote:

This change depends on D53451, not the other way around. If you commit just this one, the devclass_find_internal() assertion will fail in some cases.

OK. I'll see about re-arranging.

I've also handled John's feedback with a new review.

sys/kern/subr_bus.c
5273	D53676 should address this issue. I'm unsure about dropping topo lock around chain evh, but we'll start there. I wish there was some way to check all these.

imp removed a child revision: D53451: newbus: Ensure that we hold topo lock for calls to devclass_find_internal.Mon, Nov 10, 10:13 PM

imp added a parent revision: D53451: newbus: Ensure that we hold topo lock for calls to devclass_find_internal.

imp added a child revision: D53452: newbus: Add more asserts that we hold the topo lock.

imp retitled this revision from bus: Take the topolock in driver_module_handler() to newbus: Take the topolock in driver_module_handler().Mon, Nov 10, 10:15 PM

markj accepted this revision.Tue, Nov 11, 2:13 PM

This revision is now accepted and ready to land.Tue, Nov 11, 2:13 PM