Read back the Command register in the existing pci_enable_busmaster() and pci_disable_busmaster() methods. Return ENXIO when a Command read returns all ones, or EIO when the requested BME state does not take effect. Preserve their signatures and do not add a transaction wait. An all-ones response does not establish permanent device removal. Configuration-access exception handling remains the responsibility of the host-controller driver; these methods do not make arbitrary faulting configuration accesses recoverable. Clarify the existing Transactions Pending documentation: the bit tracks the function's outstanding non-posted requests, not posted writes or DMA engine idleness. A clear bit need not mean successful completion, and a port's bit does not account for requests forwarded from endpoints. Do not add a combined disable-and-drain method or claim a generic DMA teardown guarantee.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Leave the ordinary PCI configuration and bus-mastering methods unchanged.
Why? Is it out of fear of regressions, or do we legitimately want to ignore errors by default? Existing pci_enable_busmaster()/pci_disable_busmaster() callers already don't check the return values, though today I think these functions can't fail.
Should these be bus methods like the existing functions are?
| share/man/man9/pci.9 | ||
|---|---|---|
| 693 | When should a driver author use this variant over the existing one? The man page should provide some guidance. | |
Comment Actions
Yes probably.
| share/man/man9/pci.9 | ||
|---|---|---|
| 693 | I'm not sure on the implications of changing the KPI.. I would be in favor of changing the existing function to work like pci_enable_busmaster_checked and just ignore or cast away the return as needed. Can a change like that MFC or no? | |
| share/man/man9/pci.9 | ||
|---|---|---|
| 693 | The KPIs are the same: int func(device_t);. All of the existing callers of pci_enable_busmaster() and pci_disable_busmaster() are just ignoring the return value. So as far as I can see, you could just change the existing implementations of pci_enable_busmaster() and pci_disable_busmaster(). As for whether it's a good idea, I'm not sure, but you can try it and see if anything breaks. All else being equal, it'd be better to avoid adding new variants of the existing functions. | |
Comment Actions
Alter existing bme function.
I am thinking disable and drain is different enough from disable with the timeout we might want to keep this separate but not a strongly held opinion.
Comment Actions
What is the purpose of the function? Why it is useful to drain the non-posted transactions? After clearing the bus-master enable bit, how does it make a difference?
Also, I got an impression from the man page reading that you interpret the Status.Transactions_Pending bit set as the presence of unfinished DMA transfer, which it definitely does not mean.
Also, you only wait for the clearance of the transaction pending on the device port, why don't you wait for the same state on the root/upstream PCIe port? (But this discussion perhaps requires answering the question of usefulness of draining the transactions first).
| sys/dev/pci/pci.c | ||
|---|---|---|
| 3083 | On x86, if the read fails on the north bridge, the system returns all 1's as the response, indeed. But, is this the case for other arches, e.g. arm(64)? AFAIR it results in the DATA ABORT exception instead. | |
| 7104 | This is arguably pcie_disable_busmaster_drain_method(). | |
Comment Actions
The idea was to use this to help build fallback DMA fences in drivers for partial init or failed shutdown. There are some issues in hardware (aquantia - see Linux 7a1bb49461b1, VFs) where I want to fence off DMA, they'd still require device specific cleanup/reset and internal handling a BME fence until safe recovery.
Also, I got an impression from the man page reading that you interpret the Status.Transactions_Pending bit set as the presence of unfinished DMA transfer, which it definitely does not mean.
You are right and I overstated, it doesn't guarantee DMA engines are idle, nor do anything for posted writes nor invalidated cached descriptors.
Also, you only wait for the clearance of the transaction pending on the device port, why don't you wait for the same state on the root/upstream PCIe port? (But this discussion perhaps requires answering the question of usefulness of draining the transactions first).
As far as I understand from the spec 7.5.3.5 defines a root/switch port’s Transactions Pending bit as requests issued using its own or shadow function’s Requester ID, rather than requests forwarded for an endpoint. Polling it would therefore not strengthen this endpoint wait.
| sys/dev/pci/pci.c | ||
|---|---|---|
| 3083 | I'm not sure if this is spec or convention. rk_pcie_read_config does while generic_pcie_read_config does not provide fault recovery. https://courses.cs.washington.edu/courses/cse470/26sp/readings/PCI%20Express%20Base%20Specification%20Revision%206.0.pdf pg 221 the callout box makes it seem more of a convention. | |
| 7104 | Agreed | |
| sys/dev/pci/pci.c | ||
|---|---|---|
| 3083 | does not was meant to imply does not guarantee; I don't know what generic arm endpoint hw will do. | |
Comment Actions
I do not see any relation of the referenced commit to the function you propose, really.
I am still not sure that we should do this. Also I am not sure that (many) of the devices properly report this bit. For instance, one unnamed high-end network card implements PCI configuration space in firmware. I would ask knowledgeable people if we can rely on that bit, but I highly doubt.
Also, even if we can, I do not see it making much difference. Pending TLP should be resolved fast.
| sys/dev/pci/pci.c | ||
|---|---|---|
| 3083 | ||
Comment Actions
I removed drain from this review. The Aquantia reference is an example of why disabling rings alone may be insufficient before releasing DMA mappings, but it does not justify waiting for Transactions Pending and needs to be dealt with in the driver.
With VFs, you have a giant side channel into the host and that channel can be lossy, during storms or perhaps intentionally so from a guest, that influence lifecycle on both sides. So I'd like to find some way to prevent the affected function from issuing further DMA when normal shutdown cannot be confirmed devices at key transitions during lifecycle changes.
I am still not sure that we should do this. Also I am not sure that (many) of the devices properly report this bit. For instance, one unnamed high-end network card implements PCI configuration space in firmware. I would ask knowledgeable people if we can rely on that bit, but I highly doubt.
Also, even if we can, I do not see it making much difference. Pending TLP should be resolved fast.
Noted and deferred. Is your concern specifically that firmware may report a stale or incomplete Transactions Pending state? I wouldn’t consider a firmware implementation inherently disqualifying, but I agree that neither reliable reporting nor the usefulness of this wait has been established here.
Comment Actions
I am curious what, in your opinion, drivers should do if they get an error from PCI config space access? We tried to disable the busmaster (or any other enable bit in the config register, see below) and it failed, then what?
In fact, as a specific example, mlx5(4) driver has the health monitor which checks if the PCI config space is accessible, and then avoids further poking the device. What could driver do, if the communication with the device was lost? Might be, it could start retraining of the PCIe link, but mlx5 does not try that right now. It is some fatal problem with the firmware most likely. Link retrain should do hard reset of the PCIe downlink, but it might not if firmware is in so broken state that config is not responding.
That said, why checking the busmaster bit fiddling only? There are other enable bits in the command register, and if doing that, it probably should be applied to all of them. I am not sure that it is useful.