| Message ID | 20180831212639.10196-12-keith.busch@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI, error handling and hot plug | expand |
On Fri, Aug 31, 2018 at 03:26:34PM -0600, Keith Busch wrote: > --- a/drivers/pci/pcie/portdrv_pci.c > +++ b/drivers/pci/pcie/portdrv_pci.c > @@ -174,7 +174,9 @@ static int slot_reset_iter(struct device *device, void *data) > > static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev) > { > + pci_restore_state(dev); > device_for_each_child(&dev->dev, dev, slot_reset_iter); > + pci_save_state(dev); > return PCI_ERS_RESULT_RECOVERED; > } Shouldn't this be the other way round, i.e. save, reset, restore? Also, the function pcie_portdrv_slot_reset() was introduced in the prior patch, so it seems this is a fix for that other patch and the two should be squashed together. Thanks, Lukas
On Sun, Sep 02, 2018 at 02:34:02AM -0700, Lukas Wunner wrote: > On Fri, Aug 31, 2018 at 03:26:34PM -0600, Keith Busch wrote: > > --- a/drivers/pci/pcie/portdrv_pci.c > > +++ b/drivers/pci/pcie/portdrv_pci.c > > @@ -174,7 +174,9 @@ static int slot_reset_iter(struct device *device, void *data) > > > > static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev) > > { > > + pci_restore_state(dev); > > device_for_each_child(&dev->dev, dev, slot_reset_iter); > > + pci_save_state(dev); > > return PCI_ERS_RESULT_RECOVERED; > > } > > Shouldn't this be the other way round, i.e. save, reset, restore? No, this is the correct order. The state is originally saved in pcie_portdrv_probe, and we need to restore to that state in the slot reset callback. Once the slot has been restored through all the child drivers, then we need to save the state again for any future error handling. > Also, the function pcie_portdrv_slot_reset() was introduced in the prior > patch, so it seems this is a fix for that other patch and the two should > be squashed together. I guess they could be squashed, but the problems they're addressing seemed different enough to warrant separate change logs. The previous patch just set up the infrastructure for port error callbacks and chaining to child drivers. This patch is fixing a specific problem to restore the slot to a usable state.
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c index b1deab941f68..7b5b3d7ded5b 100644 --- a/drivers/pci/pcie/portdrv_pci.c +++ b/drivers/pci/pcie/portdrv_pci.c @@ -174,7 +174,9 @@ static int slot_reset_iter(struct device *device, void *data) static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev) { + pci_restore_state(dev); device_for_each_child(&dev->dev, dev, slot_reset_iter); + pci_save_state(dev); return PCI_ERS_RESULT_RECOVERED; }
The port's config space may be reset after a link reset, which is often implementated as a secondary bus reset. We need to restore the config space in order for downstream devices to successfully use it. Signed-off-by: Keith Busch <keith.busch@intel.com> --- drivers/pci/pcie/portdrv_pci.c | 2 ++ 1 file changed, 2 insertions(+)