Differential D32056

bus: retire DF_REBID
ClosedPublic
Actions

Authored by imp on Sep 22 2021, 3:24 PM.

Details

Reviewers

jhb
mav

Commits

rG7be46aeea126: bus: retire DF_REBID
rG0389e9be63c5: bus: retire DF_REBID

Summary

I did DF_REBID to allow for 'hoover' drivers that would attach to
otherwise unattached devices in the tree. This notion didn't catch on as
it was tricky to make work well and it was easier to just publish a /dev
node of some flavor by the parent device. It's been nothing but dead
weight for a long time.

Prompted by: mav
Sponsored by: Netflix

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 41679
Build 38568: arc lint + arc unit

Event Timeline

imp requested review of this revision.Sep 22 2021, 3:24 PM

imp created this revision.

Harbormaster completed remote builds in B41638: Diff 95506.Sep 22 2021, 3:24 PM

imp added reviewers: jhb, mav.Sep 22 2021, 3:24 PM

mav added inline comments.Sep 22 2021, 4:19 PM

sys/kern/subr_bus.c
2088	Would it better be be >= DS_ATTACHING here? Or what is the logic?
2190–2191	This section is not needed also.

mav added inline comments.Sep 22 2021, 4:22 PM

sys/kern/subr_bus.c
2131	I think this devclass devflags/removal could be moved after the following if (result == 0) section, so that in case of drivers with absolute priority we have not removed it just to re-set again.

mav added inline comments.Sep 22 2021, 4:29 PM

sys/kern/subr_bus.c
2201	And this section should probably moved inside the following if (pri < 0)

imp added inline comments.Sep 22 2021, 4:55 PM

sys/kern/subr_bus.c
2088	Originally, I think that attaching wasn't considered. How can you get here if it's attaching? This line dates from the initial commit by Doug (modulo some code motion and style tweaks) I think you're right that >= DS_ATTACHING would be better. Though I can't help but thing DS_ALIVE would also be good to filter. Why do you think it should be excluded?
2131	You may be right.... I'd like to address that separately though since I don't think this removal changes when we can / should do this.
2190–2191	because we'd never re-bid an driver now and need to detach?

mav added inline comments.Sep 22 2021, 5:01 PM

sys/kern/subr_bus.c
2088	As I see, DS_ALIVE means probed. It may not have sense to reprobe again, except one case -- new driver load, in which case you do want reprobe. Though in case of detach device drops into DS_NOTPRESENT, not DS_ALIVE, so it may not be a problem. Not sure how would end up in DS_ALIVE state for a long time.
2190–2191	We should never be attached here without rebid support, so there is nothing to detach.

imp added inline comments.Sep 22 2021, 5:24 PM

sys/kern/subr_bus.c
2088	Hmmm, it dates from very early on, but I chased it back to the device/bus split only... Still quite old.
2190–2191	I see why... yes, removing it.

imp added inline comments.Sep 22 2021, 5:58 PM

sys/kern/subr_bus.c
2088	You can get DS_ALIVE for long periods of time for devices that are disabled. We probe those devices to hold the unit, and then never attach them.

jhb accepted this revision.Sep 23 2021, 4:54 PM

jhb added inline comments.

sys/kern/subr_bus.c
2088	Hmmm, it dates from very early on, but I chased it back to the device/bus split only... Still quite old.
2088	We do want to make sure we don't break the device disable case (via hint.foo.0.disable=1 or devctl disable) just because some unrelated driver is kldloaded. Though I think those set a DF_DISABLED flag or the like?
2131	You'd have to special case the '0' case below to not re-set it though. Not sure device probe is a hot path such that it's worth the complexity to save a few cycles. Hmm, I guess the re-set below already checks child->devclass, so that one doesn't hurt to leave as-is. Moving the devflags inside the pro < 0 check as you suggest would also handle that without real added complexity. I agree that a separate commit would be better for these changes though.

This revision is now accepted and ready to land.Sep 23 2021, 4:54 PM

mav added inline comments.Sep 23 2021, 7:31 PM

sys/kern/subr_bus.c
2131	I was thinking not about hot path cycles, but about the fact that device_set_devclass() regenerates unit number. I want to make device_set_unit() possible inside probe methods, where it is much more sane/correct than in attach. But sure I can do it separately.