Page MenuHomeFreeBSD

bus: retire DF_REBID
ClosedPublic

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

Details

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
rG 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.Sep 22 2021, 3:24 PM
imp created this revision.
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.

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.

sys/kern/subr_bus.c
2201

And this section should probably moved inside the following if (pri < 0)

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?

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.

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.

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 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
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.

Address at least some of the feedback from the review.

This revision now requires review to proceed.Sep 23 2021, 7:45 PM
This revision is now accepted and ready to land.Sep 23 2021, 7:46 PM
This revision was automatically updated to reflect the committed changes.