Page MenuHomeFreeBSD
Differential D32125

bus: Cleanup device_probe_child()
ClosedPublic
Actions

Authored by mav on Sep 24 2021, 7:34 PM.
Tags
None
Referenced Files
F137940684: D32125.id95666.diff
Thu, Nov 27, 12:00 PM
Unknown Object (File)
Sun, Nov 16, 8:33 AM
Unknown Object (File)
Tue, Nov 11, 6:13 AM
Unknown Object (File)
Mon, Nov 10, 8:22 PM
Unknown Object (File)
Mon, Nov 10, 4:05 PM
Unknown Object (File)
Mon, Nov 10, 4:04 PM
Unknown Object (File)
Mon, Nov 10, 12:26 PM
Unknown Object (File)
Mon, Nov 10, 12:17 PM
View All Files
Subscribers
None

Details

Reviewers
imp
jhb
Commits
rGd97bfe3ff840: bus: Cleanup device_probe_child()
rGf73c2bbf811b: bus: Cleanup device_probe_child()
Summary

When device driver probe method returns 0, i.e. absolute priority, do not remove its class from the device just to set it back few lines later, that may change the device unit number, etc. and after which we'd better call the probe again.

If during search we found some driver with absolute priority, we do not need to set device driver and class since we haven't removed them before.

It should not happen, but if second probe method call failed, remove the driver and possibly the class from the device as it was when we started.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mav requested review of this revision.Sep 24 2021, 7:34 PM
mav created this revision.
imp added a comment.Sep 24 2021, 8:27 PM

This looks good... I have one question though...

sys/kern/subr_bus.c
2216

Why is the hasclass test needed here? If we've probed the device and this second probe fails, shouldn't we always set devclass to NULL?

mav added inline comments.Sep 24 2021, 8:31 PM
sys/kern/subr_bus.c
2216

There can be other drivers of the same class. And if we haven't set the class for the device (it can be set by the bus) we should not remove it.

imp accepted this revision.Sep 24 2021, 8:46 PM

OK I'm happy. A suggested place to add a comment, but if you don't want to, I can do it as a followup.

sys/kern/subr_bus.c
2078

We should add a comment here that says we try to preserve the devclass of an object that the bus has set maybe?

2216

Makes sense... I'd forgotten that was why this was here...

This revision is now accepted and ready to land.Sep 24 2021, 8:46 PM
jhb accepted this revision.Sep 25 2021, 12:17 AM
jhb added inline comments.
sys/kern/subr_bus.c
2188

Unindenting all of this with the early failure is a nice cleanup.

2196

Why did you move this earlier? I guess it matches the main probe loop above? Either order is probably fine though.

2216

This is what lets hinted devices work for example. I think that's the main case of pre-set device classes. devctl set driver is another instance.

mav added inline comments.Sep 25 2021, 12:32 AM
sys/kern/subr_bus.c
2196

Yes, just to match the above code. Also the previous code missed error handling, so would need to be touched any way.

Closed by commit rGf73c2bbf811b: bus: Cleanup device_probe_child() (authored by mav). · Explain WhySep 25 2021, 12:36 AM
This revision was automatically updated to reflect the committed changes.
mav added a commit: rGf73c2bbf811b: bus: Cleanup device_probe_child().
imp added inline comments.Sep 25 2021, 12:37 AM
sys/kern/subr_bus.c
2089

I still think this should be >= DS_ALIVE... But I don't know if it is possible to get here for >.

2216

Yea, hinted devices are set by the bus and probe just approves / rejects the device.
Disabled devices will also be similar, but will be in DS_ALIVE.

mav added a commit: rGd97bfe3ff840: bus: Cleanup device_probe_child().Jan 4 2022, 5:18 PM

Revision Contents
Changeset List

PathSize
sys/
kern/
50 lines

Diff 95687

View Options

sys/kern/subr_bus.c

Loading...