Page MenuHomeFreeBSD

New pci device hints wiring.
ClosedPublic

Authored by imp on Dec 20 2017, 1:17 AM.

Details

Summary

Return domain, bus, slot, and function for the transport settings in
PATH_INQ requests for nvme.

Be pedantic and check the return value of resource_string_value. Given
that we've just found it, this is likely overkill, but if we can't
find the entry, better to fail safe.

Add device location wiring to the pci bus. This allows one to specify,
for example, that if there's an igb card in bus 12, slot 0, function
0, it should be assigned igb5. If there isn't, or there's one in a
different slot, normal numbering rules apply (hinted units are
skipped). Adding 'hint.igb.5.at="pci12:0:0"' or
'hint.igb.5.at="pci0:12:0:0"' to /boot/device.hints will accomplish
this. The quotes are important, and these are the only two forms that
are supported to avoid embedding a parser in the kernel.

Test Plan

Note: this mixes three commits together: the wiring, a tiny fix to acpi, and a patch to have the nvme sim pass data up the CAM stack about the device's location. These will be separate commits.

Diff Detail

Repository
rS 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 created this revision.Dec 20 2017, 1:17 AM
imp edited the test plan for this revision. (Show Details)Dec 20 2017, 1:19 AM
imp added reviewers: peter, jhb, scottl.
imp added inline comments.
sys/dev/pci/pci.c
4251–4252 ↗(On Diff #36785)

The simple string match is intentional. That way we don't have to parse and guess intent, nor make sure that the parser is hardened against attack.

imp added a reviewer: rpokala.Dec 20 2017, 1:21 AM
jhb added inline comments.Dec 20 2017, 1:27 AM
share/man/man4/pci.4
541 ↗(On Diff #36785)

Maybe use '.Xr igb 4', etc. for device drivers in this text?

sys/dev/acpica/acpi.c
1022 ↗(On Diff #36785)

This shouldn't fail due to the resource_find_dev(), that's why it dosen't bother checking. Could perhaps put '(void) ' in front of it if you want.

sys/dev/pci/pci.c
4249 ↗(On Diff #36785)

This can't ever fail either.

rpokala added inline comments.Dec 20 2017, 2:05 AM
share/man/man4/pci.4
534 ↗(On Diff #36785)

s/there's/there is/

536 ↗(On Diff #36785)

s/there's/there is/

sys/dev/pci/pci.c
4247 ↗(On Diff #36785)

If I'm reading subr_hints.c correctly, resource_find_dev() will return 0 if there is a hint.<name>.*.at, and populate unit in the process. A non-zero return means there are no more "at" hints for this driver, so the break skips over the rest of the loop. Is that correct?

4249 ↗(On Diff #36785)

And then resource_string_value() populates at with the pointer to the hint.<name>.<unit>.at=... string in question?

4256 ↗(On Diff #36785)

I think

		if (strcmp(at, me1) != 0 &&
		    strcmp(at, me2) != 0)
			continue;
		*unitp = unit;
		break;

would be much clearer like this:

		if (strcmp(at, me1) == 0 ||
		    strcmp(at, me2) == 0) {
			*unitp = unit;
			break;
		}
imp updated this revision to Diff 36817.Dec 20 2017, 3:38 PM

Simplify the code a bit, make changes from review (except
one). Prepare for landing.

imp updated this revision to Diff 36818.Dec 20 2017, 3:41 PM

missed one .Xr

imp marked 8 inline comments as done.Dec 20 2017, 3:48 PM

Updated with all these suggestions.

sys/dev/acpica/acpi.c
1022 ↗(On Diff #36785)

OK. Also simplified the loop construct a bit instead :)

sys/dev/pci/pci.c
4247 ↗(On Diff #36785)

Correct. However, I've simplified the loop so that we have a direct while loop.

4249 ↗(On Diff #36785)

Correct.

4256 ↗(On Diff #36785)

I disagree. We're doing a elimination loop. We may need to add more things that eliminate in the future and your change would interfere with that.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 20 2017, 7:14 PM
This revision was automatically updated to reflect the committed changes.
imp marked 4 inline comments as done.