| Message ID | 20250524185304.26698-2-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Slot reset fixes | expand |
On Sun, May 25, 2025 at 12:23:03AM +0530, Manivannan Sadhasivam wrote: > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4985,10 +4985,19 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev) > struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); > int ret; > > - if (host->reset_slot) { > + if (pci_is_root_bus(dev->bus) && host->reset_slot) { > + /* > + * Save the config space of the root port before doing the > + * reset, since the state could be lost. The device state > + * should've been saved by the caller. > + */ > + pci_save_state(dev); > ret = host->reset_slot(host, dev); Nit: Capitalize terms as the PCIe Base Spec does, i.e. "Root Port". "The device state" is ambiguous as the Root Port is a device itself and even referred to by the "dev" variable. I think what you mean is "The Endpoint state". Thanks, Lukas
On Sun, May 25, 2025 at 09:22:03AM +0200, Lukas Wunner wrote: > On Sun, May 25, 2025 at 12:23:03AM +0530, Manivannan Sadhasivam wrote: > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -4985,10 +4985,19 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev) > > struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); > > int ret; > > > > - if (host->reset_slot) { > > + if (pci_is_root_bus(dev->bus) && host->reset_slot) { > > + /* > > + * Save the config space of the root port before doing the > > + * reset, since the state could be lost. The device state > > + * should've been saved by the caller. > > + */ > > + pci_save_state(dev); > > ret = host->reset_slot(host, dev); > > Nit: Capitalize terms as the PCIe Base Spec does, i.e. "Root Port". > Ack. > "The device state" is ambiguous as the Root Port is a device itself > and even referred to by the "dev" variable. I think what you mean > is "The Endpoint state". > Yes! Will fix them while applying, thanks! - Mani
On Sun, May 25, 2025 at 01:28:18PM +0530, Manivannan Sadhasivam wrote: > On Sun, May 25, 2025 at 09:22:03AM +0200, Lukas Wunner wrote: > > "The device state" is ambiguous as the Root Port is a device itself > > and even referred to by the "dev" variable. I think what you mean > > is "The Endpoint state". > > > > Yes! Will fix them while applying, thanks! ICYMI, current controller/dw-rockchip branch still uses "The device state", not "The Endpoint state" in commit 56eecfc8f46f ("PCI/ERR: Add support for resetting the Root Ports in a platform specific way"). Otherwise LGTM. Thanks, Lukas
On Mon, May 26, 2025 at 08:54:37AM +0200, Lukas Wunner wrote: > On Sun, May 25, 2025 at 01:28:18PM +0530, Manivannan Sadhasivam wrote: > > On Sun, May 25, 2025 at 09:22:03AM +0200, Lukas Wunner wrote: > > > "The device state" is ambiguous as the Root Port is a device itself > > > and even referred to by the "dev" variable. I think what you mean > > > is "The Endpoint state". > > > > > > > Yes! Will fix them while applying, thanks! > > ICYMI, current controller/dw-rockchip branch still uses > "The device state", not "The Endpoint state" in commit > 56eecfc8f46f ("PCI/ERR: Add support for resetting the > Root Ports in a platform specific way"). > Ah, missed this comment. Incorported now! - Mani
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 4d396bbab4a8..6d6e9ce2bbcc 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4985,10 +4985,19 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev) struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); int ret; - if (host->reset_slot) { + if (pci_is_root_bus(dev->bus) && host->reset_slot) { + /* + * Save the config space of the root port before doing the + * reset, since the state could be lost. The device state + * should've been saved by the caller. + */ + pci_save_state(dev); ret = host->reset_slot(host, dev); if (ret) pci_err(dev, "failed to reset slot: %d\n", ret); + else + /* Now restore it on success */ + pci_restore_state(dev); return; }
host_bridge::reset_slot() is supposed to reset the PCI root port/slot. Once that happens, the config space content would be lost. This was reported by Niklas on the dw-rockchip based platform where the MPS setting of the root port was lost after the host_bridge::reset_slot() callback. Hence, save the config space before calling the host_bridge::reset_slot() callback and restore it afterwards. While at it, make sure that the callback is only called for root ports by checking if the bridge is behind the root bus. Fixes: d5c1e1c25b37 ("PCI/ERR: Add support for resetting the slots in a platform specific way") Reported-by: Niklas Cassel <cassel@kernel.org> Closes: https://lore.kernel.org/linux-pci/aC9OrPAfpzB_A4K2@ryzen Suggested-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/pci/pci.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)