Hello there,
As you know the reset_slot() callback patches were merged recently.
Wilfred and I (mostly Wilfred), have been debugging DMA issues after the
reset_slot() callback has been invoked. The issue is reproduced when MPS
configuration is set to performance, but might be applicable for other
MPS configurations as well. The problem appears to be that reset_slot()
feature does not respect/restore the MPS configuration.
When having two Rock 5Bs, one in RC mode, one in EP mode:
# Boot with MPS bus performance
pci=pcie_bus_perf on kernel command line
# lspci RC
lspci -vvvs 0000:00:00.0 | grep -E MaxPayload
DevCap: MaxPayload 256 bytes, PhantFunc 0
DevCtl: MaxPayload 256 bytes, MaxReadReq 256 bytes
# Run pcitests
All tests pass
# Perform hot reset of EP
echo 1 > /sys/bus/pci/devices/0000:01:00.0/reset
# lspci RC
lspci -vvvs 0000:00:00.0 | grep -E MaxPayload
DevCap: MaxPayload 256 bytes, PhantFunc 0
DevCtl: MaxPayload 128 bytes, MaxReadReq 512 bytes
# Run pcitests
FAIL pci_ep_data_transfer.dma.READ_TEST
Compared to before, DMA read test now fails.
Wilfred has been able to track this down to being because MPS in the RC
is different before/after a ->reset_slot(), and simply re-storing MPS using
a dirty hack, the DMA read issues go away.
My first thinking was that pci_configure_device() (and thus pci_configure_mps())
was not called,
so I added some prints in pci_configure_mps().
pci_configure_mps() was not called for RC nor EP during ->reset_slot()
(I did not check if pcie_bus_configure_settings() was called.)
I tried Mani's earlier patch instead of what is queued up:
https://lore.kernel.org/linux-pci/20250221172309.120009-2-manivannan.sadhasivam@xxxxxxxxxx/
And in this version pci_configure_mps() does get called,
for both the RC and EP during ->retrain_link()
(I did not check if pcie_bus_configure_settings() was called.)
But... MaxPayload was still incorrectly set after retrain_link().
I think the solution is to add a call to add a pcie_bus_configure_settings()
call in pcie_do_recovery() / pci_host_recover_slots() / pci_host_reset_slot() /
pcibios_reset_secondary_bus().
Or possibly a:
list_for_each_entry(child, &bus->children, node)
pcie_bus_configure_settings(child);
as done in pci_host_probe().
I'm not sure if we need to make sure that pci_configure_device() /
pci_configure_mps() gets called as well. Since we did reset the endpoint,
it seems that calling pci_configure_mps() does make sense.
Normally, I would wait for Wilfred and myself to come up with a nice fix,
and test it, but, considering that the ->reset_slot() patches are queued
up already, and the merge window is opening soon, I'm sharing our findings
on the list, as it might take some time to come up with a nice solution.
Kind regards,
Niklas
P.S. Thanks to Wilfred for all the hours spent root-causing this.
