Page MenuHomeFreeBSD

uart(4): Add a concept of "unique" serial devices
ClosedPublic

Authored by cperciva on Mar 29 2022, 7:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 31, 12:09 AM
Unknown Object (File)
Oct 16 2024, 6:04 AM
Unknown Object (File)
Oct 7 2024, 10:49 AM
Unknown Object (File)
Oct 1 2024, 7:32 PM
Unknown Object (File)
Oct 1 2024, 12:12 PM
Unknown Object (File)
Sep 29 2024, 9:33 AM
Unknown Object (File)
Sep 27 2024, 1:55 PM
Unknown Object (File)
Sep 26 2024, 6:13 AM
Subscribers

Details

Summary

FreeBSD detects serial ports twice: First, very early in the boot
process, in order to obtain a usable console; and second, during
the device probe/attach process. When a UART is discovered during
device probing, FreeBSD attempts to determine whether it is a
device which was already being used as a console; without this,
the console doesn't work in userland.

Unfortunately it's possible for a UART to be mapped to a different
location in memory when it is discovered on a bus than it has when
it is announced via the ACPI SPCR table; this breaks the matching
process, which relies on comparing bus addresses.

To address this, we introduce a concept of "unique" serial devices,
i.e. devices which are guaranteed to be present *only once* on any
system. If we discover one of these during device probing, we can
match it to a same-PCI-vendor-and-device-numbers console which was
announced via the ACPI SPCR table, regardless of the differing bus
addresses.

At present, the only unique serial device is the "Amazon PCI serial
device" (vendor 0x1d0f, device 0x8250) found in some EC2 instances.
This unbreaks the serial console on those systems.

Diff Detail

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

Event Timeline

cperciva created this revision.

Third try. It's a bit hacky, but in discussions with EC2 folks it sounds like this is the best way to identify that the UART we discovered is the console we configured from SPCR.

sys/dev/uart/uart_bus_pci.c
226

We usually use #defines for these
Or at the very least a comment...
but do we even need a table?
Don't we have these ids from the ACPI tables?

227

I'd just use count rather than having a sentinel like this at the end.

sys/dev/uart/uart_bus_pci.c
226

I was matching the style of the pci_ns8250_ids table earlier, which simply lists vendor and device IDs as raw integers.

For setting up the console early in the kernel boot process, we get the IDs from the ACPI tables (that happens in the uart_cpu_acpi.c bits below). The bit here is to tell us that when we see the same PCI vendor/device pair during PCI bus probing, it's guaranteed to be the same device. (Unlike, say, a 0x1028 / 0x0008 UART, which might conceivably be one of multiple Dell Remote Access Card IIIs in a system.)

227

If I was writing this as entirely new code I would do that too. I wanted to match the style of the uart_pci_match code (which uses the same sentinel in its table).

sys/dev/uart/uart_bus_pci.c
226

Are the dell devices in the ACPI tables too? If not, then it doesn't matter...

227

at least use PCIV_INVALID then. But you wrote new code to look at the table, so there's no reason to have a sentinel.

247

Please use PCIV_INVALID here.

sys/dev/uart/uart_cpu.h
61

You don' t need valid. vendor == 0 is not legal either and will never happen. You can test to see if it's set at all.

sys/dev/uart/uart_cpu_acpi.c
191

I don't think you need valid here at all... The whole struct will be initialized to 0, which is never a valid vendor.

rpokala added inline comments.
sys/dev/uart/uart_bus_pci.c
226

... vendor and device IDs as raw integers.

Raw integers used for identification -- rather than for math -- are a tool of the devil.

sys/dev/uart/uart_bus_pci.c
226

Sure, but there's 76 lines of PCI vendors/devices already in this file. Maybe replacing all of those with #defines can be done later...

247

That bit is going away since I'm changing the code to eliminate the sentinel.

sys/dev/uart/uart_cpu_acpi.c
191

Got it. I wasn't sure if anything other than PCIV_INVALID was reserved.

Updated to add comments, use the table size instead of a sentinel,
and eliminate an unnecessary 'valid' flag.

OK. I'm happy enough with this. Go ahead and commit it when you are happy as well.

This revision is now accepted and ready to land.Mar 31 2022, 9:31 PM