Page MenuHomeFreeBSD

acpi_pcib: Trust decoded bus range from _CRS over _BBN
ClosedPublic

Authored by jhb on Oct 16 2023, 5:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 10 2024, 6:30 AM
Unknown Object (File)
Jan 8 2024, 3:35 AM
Unknown Object (File)
Dec 20 2023, 8:26 AM
Unknown Object (File)
Dec 10 2023, 12:02 AM
Unknown Object (File)
Nov 19 2023, 11:51 AM
Unknown Object (File)
Oct 29 2023, 9:07 AM
Unknown Object (File)
Oct 29 2023, 9:07 AM
Unknown Object (File)
Oct 24 2023, 7:26 PM
Subscribers

Details

Summary

Currently if _BBN doesn't match the first bus in the decoded bus range
from _CRS for a Host to PCI bridge, the driver fails to attach as a
defensive measure.

There is now firmware in the field where these do not match, and the
_BBN values are clearly wrong, so rather than failing attach, trust
the range from _CRS over _BBN.

Reported by: gibbs

Diff Detail

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

Event Timeline

jhb requested review of this revision.Oct 16 2023, 5:16 PM

Functionally: lgtm. One quibble over an old ERROR message that's now just a warning message and how it might be adjusted.

sys/dev/acpica/acpi_pcib_acpi.c
520–531

does this message need to be adjusted to indicate that we're overriding the found decoded range?

This revision is now accepted and ready to land.Oct 16 2023, 5:25 PM
sys/dev/acpica/acpi_pcib_acpi.c
520–531

I changed this when integrating John's patch to this:

if (decode_bus_range(sc, &start, &end) == 0) {
    if (sc->ap_bus != start) {
        device_printf(dev,
            "WARNING: BIOS configured bus number (%d) is not within "
            "decoded bus number range (%ju - %ju). "
            "Using range start (%ju) as bus number.\n",
            sc->ap_bus, (uintmax_t)start, (uintmax_t)end,
            (uintmax_t)start);
        sc->ap_bus = start;
    }
}
sys/dev/acpica/acpi_pcib_acpi.c
520–531

I'll go with Justin's message. To be clear, we aren't overriding the range, we are just overriding the bus number of the bridge.

sys/dev/acpica/acpi_pcib_acpi.c
520–531

Small nit. "decoded_bus_range" kinda implies the return value will be the range. That's why I called the function "decode_bus_range" instead.

sys/dev/acpica/acpi_pcib_acpi.c
520–531

I hadn't actually seen that part of your diff at first. I had used decoded originally (even when the return value was an errno) as the idea I was trying to communicate was "fetch the first bus number decoded by the bus" and now "fetch the entire range of bus numbers (start and end) decoded by the bus". If this were C++ the signature would be something like: std::optional<std::pair<rman_res_t, rman_res_t>> acpi_pcib_acpi::decoded_bus_range(). But C is not really conducive to that sort of return value, hence returning the actual range via out parameters instead (but logically still "returning" start and end) with the return value just being a success/failure indicator (the return value should probably be a bool rather than an errno value though).

sys/dev/acpica/acpi_pcib_acpi.c
520–531

Mmm, but perhaps the implicit verb there is my problem and get_decoded_bus_range or fetch_decoded_bus_range would be clearer? (In particular, we aren't doing any real decoding (e.g. we aren't parsing the result of _CRS) but are instead returning a cached result of a previous decoding of _CRS.)

sys/dev/acpica/acpi_pcib_acpi.c
520–531

I think a "get_decoded_bus_range()" that returns a bool makes the most sense.