Verify Command-register BME readback in the existing PCI bus-mastering methods. Return ENXIO for inaccessible configuration space and EIO when the requested state did not take effect. Keep their existing signatures and do not add a transaction wait to ordinary enable or disable calls. Add pcie_disable_busmaster_drain() as an explicit, potentially sleeping PCI bus method. Disable and verify bus mastering, then reuse the PCIe Transactions Pending wait with the configured completion timeout and a 10 ms floor. Forward the operation through VGA and host-bridge PCI children so it reaches the function which owns configuration space. Neither operation serializes callers or retains quarantine state. Drivers must prevent concurrent configuration restores or DMA enables and discard stale DMA addresses before restoring bus mastering or allowing another driver to attach. Document when to use ordinary disable versus disable-and-drain, including the limited scope of the readback ordering and non-posted-request drain. Neither is a device reset or a universal posted-DMA drain. MFC after: 2 weeks
Details
Details
Pattern may replace analogs in drivers around things like firmware resets or failed init.
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 | ||
|---|---|---|
| 680 | 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 | ||
|---|---|---|
| 680 | 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 | ||
|---|---|---|
| 680 | 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. | |
| 7100 | 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. | |
| 7100 | 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 | ||