PCI: save and restore device state during bus reset
Subsystems (2)
PATCH 1/2 PCI: save and restore device state during bus reset
A secondary bus reset causes settings to be lost by all downstream devices on the tree. The code is currently saving and restoring device states only when called from the VFIO path via pci_probe_reset_bus and pci_reset_bus functions.
Moving the save and restore into pci_reset_bridge_secondary_bus so that all users of the API have the same behavior.
--- drivers/pci/pci.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)
drivers/pci/pci.c +15 -21
@@ -3860,19 +3860,6 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)pci_reset_secondary_bus (dev); } -/** -* pci_reset_bridge_secondary_bus - Reset the secondary bus on a PCI bridge. -* @dev: Bridge device -* -* Use the bridge control register to assert reset on the secondary bus. -* Devices on the secondary bus are left in power-on state. -*/ -void pci_reset_bridge_secondary_bus (struct pci_dev *dev) -{ -pcibios_reset_secondary_bus (dev); -} -EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus); -static int pci_parent_bus_reset (struct pci_dev *dev,int probe) {struct pci_dev *pdev;
@@ -4362,6 +4349,21 @@ static void pci_slot_restore(struct pci_slot *slot) } } +/** +* pci_reset_bridge_secondary_bus - Reset the secondary bus on a PCI bridge. +* @dev: Bridge device +* +* Use the bridge control register to assert reset on the secondary bus. +* Devices on the secondary bus are left in power-on state. +*/ +void pci_reset_bridge_secondary_bus (struct pci_dev *dev) +{ +pci_bus_save_and_disable (dev->bus); +pcibios_reset_secondary_bus (dev); +pci_bus_restore (dev->bus); +} +EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus); +static int pci_slot_reset (struct pci_slot *slot,int probe) {int rc;
@@ -4504,12 +4506,8 @@ int pci_reset_bus(struct pci_bus *bus)if (rc)return rc; -pci_bus_save_and_disable (bus); - rc =pci_bus_reset (bus,0 ); -pci_bus_restore (bus); -return rc; }EXPORT_SYMBOL_GPL (pci_reset_bus);
@@ -4528,8 +4526,6 @@ int pci_try_reset_bus(struct pci_bus *bus)if (rc)return rc; -pci_bus_save_and_disable (bus); -if (pci_bus_trylock (bus)) {might_sleep ();pci_reset_bridge_secondary_bus (bus->self);
@@ -4537,8 +4533,6 @@ int pci_try_reset_bus(struct pci_bus *bus) }else rc = -EAGAIN ; -pci_bus_restore (bus); -return rc; }EXPORT_SYMBOL_GPL (pci_try_reset_bus);
Signature · 1 line
1.9.1
PATCH 2/2 PCI: add CRS support to error handling path
The PCIE spec allows an endpoint device to extend the initialization time beyond 1 second by issuing Configuration Request Retry Status (CRS) for a vendor ID read request.
This basically means "I'm busy now, please call me back later".
There are two moving parts to CRS support from the SW perspective. One part is to determine if CRS is supported or not. The second part is to set the CRS visibility register.
As part of the probe, the Linux kernel sets the above two conditions in pci_enable_crs function. The kernel is also honoring the returned CRS in pci_bus_read_dev_vendor_id function if supported. The function will poll up to specified amount of time while endpoint is returning CRS response.
The PCIe spec also allows CRS to be issued during cold, warm, hot and FLR resets.
The hot reset is initiated by starting a secondary bus reset. This patch is adding vendor ID read immediately after a bus reset so that the initialization procedure can be extended by the amount of time endpoint requires.
--- drivers/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
drivers/pci/pci.c +39 -0
@@ -3829,6 +3829,44 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)return 0 ; } +/* +* Mostly copy paste from pci_walk_bus with the exceptions of hard coded +* work and removed locks. +*/ +static void pci_bus_probe_crs (struct pci_bus *top) +{ +struct pci_dev *dev; +struct pci_bus *bus; +struct list_head *next; +int retval; +u32 l; + + bus = top; + next = top->devices.next; +for (;;) { +if (next == &bus->devices) { +/* end of this bus, go up or finish */ +if (bus == top) +break ; + next = bus->self->bus_list.next; + bus = bus->self->bus; +continue ; + } + dev = list_entry(next, struct pci_dev, bus_list); +if (dev->subordinate) { +/* this is a pci-pci bridge, do its devices next */ + next = dev->subordinate->devices.next; + bus = dev->subordinate; + }else + next = dev->bus_list.next; + + retval =pci_bus_read_dev_vendor_id (dev->bus, dev->devfn, &l, +60 *1000 ); +if (retval) +break ; + } +} +void pci_reset_secondary_bus (struct pci_dev *dev) {u16 ctrl;
@@ -4361,6 +4399,7 @@ void pci_reset_bridge_secondary_bus(struct pci_dev *dev)pci_bus_save_and_disable (dev->bus);pcibios_reset_secondary_bus (dev);pci_bus_restore (dev->bus); +pci_bus_probe_crs (dev->subordinate); }EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus);
Signature · 1 line
1.9.1
Re: [PATCH 2/2] PCI: add CRS support to error handling path
drivers/pci/pci.c void pci_reset_bridge_secondary_bus(struct pci_dev *dev) · line 4403
Quoted 4 lines · Sinan Kaya
drivers/pci/pci.c +1
-0pcibios_reset_secondary_bus (dev);pci_bus_restore (dev->bus); +pci_bus_probe_crs (dev->subordinate); }
I should have done this instead.
pcibios_reset_secondary_bus(dev);
+ pci_bus_probe_crs(dev->subordinate);
pci_bus_restore(dev->bus);
I can fix it on the next version. Looking forward to feedback.
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH 2/2] PCI: add CRS support to error handling path
In reply to Sinan KayaHi Bjorn,
Quoted 79 lines · Sinan Kaya
The PCIE spec allows an endpoint device to extend the initialization time beyond 1 second by issuing Configuration Request Retry Status (CRS) for a vendor ID read request.
This basically means "I'm busy now, please call me back later".
There are two moving parts to CRS support from the SW perspective. One part is to determine if CRS is supported or not. The second part is to set the CRS visibility register.
As part of the probe, the Linux kernel sets the above two conditions in
pci_enable_crsfunction. The kernel is also honoring the returned CRS inpci_bus_read_dev_vendor_idfunction if supported. The function will poll up to specified amount of time while endpoint is returning CRS response.The PCIe spec also allows CRS to be issued during cold, warm, hot and FLR resets.
The hot reset is initiated by starting a secondary bus reset. This patch is adding vendor ID read immediately after a bus reset so that the initialization procedure can be extended by the amount of time endpoint requires.
Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)drivers/pci/pci.c +38
-0@@ -3829,6 +3829,44 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)return 0 ; } +/* +* Mostly copy paste from pci_walk_bus with the exceptions of hard coded +* work and removed locks. +*/ +static void pci_bus_probe_crs (struct pci_bus *top) +{ +struct pci_dev *dev; +struct pci_bus *bus; +struct list_head *next; +int retval; +u32 l; + + bus = top; + next = top->devices.next; +for (;;) { +if (next == &bus->devices) { +/* end of this bus, go up or finish */ +if (bus == top) +break ; + next = bus->self->bus_list.next; + bus = bus->self->bus; +continue ; + } + dev = list_entry(next, struct pci_dev, bus_list); +if (dev->subordinate) { +/* this is a pci-pci bridge, do its devices next */ + next = dev->subordinate->devices.next; + bus = dev->subordinate; + }else + next = dev->bus_list.next; + + retval =pci_bus_read_dev_vendor_id (dev->bus, dev->devfn, &l, +60 *1000 ); +if (retval) +break ; + } +} +void pci_reset_secondary_bus (struct pci_dev *dev) {u16 ctrl;
drivers/pci/pci.c void pci_reset_bridge_secondary_bus(struct pci_dev *dev) · line 4405
Quoted 7 lines
drivers/pci/pci.c +1
-0@@ -4361,6 +4399,7 @@ void pci_reset_bridge_secondary_bus(struct pci_dev *dev)pci_bus_save_and_disable (dev->bus);pcibios_reset_secondary_bus (dev);pci_bus_restore (dev->bus); +pci_bus_probe_crs (dev->subordinate); }EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus);
Any feedback on the direction?
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH 2/2] PCI: add CRS support to error handling path
In reply to Sinan KayaQuoted 63 lines · Sinan Kaya
The PCIE spec allows an endpoint device to extend the initialization time beyond 1 second by issuing Configuration Request Retry Status (CRS) for a vendor ID read request.
This basically means "I'm busy now, please call me back later".
There are two moving parts to CRS support from the SW perspective. One part is to determine if CRS is supported or not. The second part is to set the CRS visibility register.
As part of the probe, the Linux kernel sets the above two conditions in
pci_enable_crsfunction. The kernel is also honoring the returned CRS inpci_bus_read_dev_vendor_idfunction if supported. The function will poll up to specified amount of time while endpoint is returning CRS response.The PCIe spec also allows CRS to be issued during cold, warm, hot and FLR resets.
The hot reset is initiated by starting a secondary bus reset. This patch is adding vendor ID read immediately after a bus reset so that the initialization procedure can be extended by the amount of time endpoint requires.
Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)drivers/pci/pci.c +27
-0@@ -3829,6 +3829,44 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)return 0 ; } +/* +* Mostly copy paste from pci_walk_bus with the exceptions of hard coded +* work and removed locks. +*/ +static void pci_bus_probe_crs (struct pci_bus *top) +{ +struct pci_dev *dev; +struct pci_bus *bus; +struct list_head *next; +int retval; +u32 l; + + bus = top; + next = top->devices.next; +for (;;) { +if (next == &bus->devices) { +/* end of this bus, go up or finish */ +if (bus == top) +break ; + next = bus->self->bus_list.next; + bus = bus->self->bus; +continue ; + } + dev = list_entry(next, struct pci_dev, bus_list); +if (dev->subordinate) { +/* this is a pci-pci bridge, do its devices next */ + next = dev->subordinate->devices.next;
drivers/pci/pci.c static int pci_pm_reset(struct pci_dev *dev, int probe) · line 3868
Quoted 10 lines
drivers/pci/pci.c +10
-0+ bus = dev->subordinate; + }else + next = dev->bus_list.next; + + retval =pci_bus_read_dev_vendor_id (dev->bus, dev->devfn, &l, +60 *1000 ); +if (retval) +break ; + } +}
Sigh. Man, this is ugly. Maybe we're locked into the current strategy and don't really have a choice, but I really don't like it.
drivers/pci/pci.c void pci_reset_bridge_secondary_bus(struct pci_dev *dev) · line 4402
Quoted 9 lines
drivers/pci/pci.c +2
-0+void pci_reset_secondary_bus (struct pci_dev *dev) {u16 ctrl;@@ -4361,6 +4399,7 @@ void pci_reset_bridge_secondary_bus(struct pci_dev *dev)pci_bus_save_and_disable (dev->bus);pcibios_reset_secondary_bus (dev);pci_bus_restore (dev->bus); +pci_bus_probe_crs (dev->subordinate);
This looks backwards -- pci_bus_restore() uses config accesses, so surely you want to do the CRS check *before* that, right? Oh, never mind, I see you already caught this.
You mentioned several kinds of reset where CRS is allowed. Doesn't this fix only one of them? I know we support at least FLR reset also.
Quoted 10 lines
drivers/pci/pci.c +0
-0}EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus);Signature · 6 lines
1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] PCI: add CRS support to error handling path
Quoted 69 lines · Bjorn Helgaas
Quoted 68 lines · Sinan Kaya
The PCIE spec allows an endpoint device to extend the initialization time beyond 1 second by issuing Configuration Request Retry Status (CRS) for a vendor ID read request.
This basically means "I'm busy now, please call me back later".
There are two moving parts to CRS support from the SW perspective. One part is to determine if CRS is supported or not. The second part is to set the CRS visibility register.
As part of the probe, the Linux kernel sets the above two conditions in
pci_enable_crsfunction. The kernel is also honoring the returned CRS inpci_bus_read_dev_vendor_idfunction if supported. The function will poll up to specified amount of time while endpoint is returning CRS response.The PCIe spec also allows CRS to be issued during cold, warm, hot and FLR resets.
The hot reset is initiated by starting a secondary bus reset. This patch is adding vendor ID read immediately after a bus reset so that the initialization procedure can be extended by the amount of time endpoint requires.
Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)drivers/pci/pci.c +32
-0@@ -3829,6 +3829,44 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)return 0 ; } +/* +* Mostly copy paste from pci_walk_bus with the exceptions of hard coded +* work and removed locks. +*/ +static void pci_bus_probe_crs (struct pci_bus *top) +{ +struct pci_dev *dev; +struct pci_bus *bus; +struct list_head *next; +int retval; +u32 l; + + bus = top; + next = top->devices.next; +for (;;) { +if (next == &bus->devices) { +/* end of this bus, go up or finish */ +if (bus == top) +break ; + next = bus->self->bus_list.next; + bus = bus->self->bus; +continue ; + } + dev = list_entry(next, struct pci_dev, bus_list); +if (dev->subordinate) { +/* this is a pci-pci bridge, do its devices next */ + next = dev->subordinate->devices.next; + bus = dev->subordinate; + }else + next = dev->bus_list.next; + + retval = pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &l,
Quoted 8 lines
drivers/pci/pci.c static int pci_pm_reset(struct pci_dev *dev, int probe) · line 3868
Quoted 5 lines
drivers/pci/pci.c +5
-0+ 60 * 1000); +if (retval) +break ; + } +}Sigh. Man, this is ugly. Maybe we're locked into the current strategy and don't really have a choice, but I really don't like it.
I can add a locked version of the walkbus API.
Then, I can minimize this code to a couple of lines. How does that sound?
Quoted 14 lines
drivers/pci/pci.c void pci_reset_bridge_secondary_bus(struct pci_dev *dev) · line 4402
Quoted 10 lines
drivers/pci/pci.c +2
-0+void pci_reset_secondary_bus (struct pci_dev *dev) {u16 ctrl;@@ -4361,6 +4399,7 @@ void pci_reset_bridge_secondary_bus(struct pci_dev *dev)pci_bus_save_and_disable (dev->bus);pcibios_reset_secondary_bus (dev);pci_bus_restore (dev->bus); +pci_bus_probe_crs (dev->subordinate);This looks backwards --
pci_bus_restore()uses config accesses, so surely you want to do the CRS check *before* that, right? Oh, never mind, I see you already caught this.
Yep, I was waiting for your feedback before re-posting.
Quoted 3 lines
You mentioned several kinds of reset where CRS is allowed. Doesn't this fix only one of them? I know we support at least FLR reset also.
The CRS is for hot reset, warm reset and FLR reset. There is nothing we can do in SW for warm reset. This patch is to address hot reset caused by SBR.
I was hoping that Alex would help us for directions on the FLR reset later.
Quoted 12 lines
Quoted 11 lines
drivers/pci/pci.c +0
-0}EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus);Signature · 6 lines
1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH 2/2] PCI: add CRS support to error handling path
Quoted 79 lines · Sinan Kaya
Quoted 69 lines · Bjorn Helgaas
Quoted 68 lines · Sinan Kaya
The PCIE spec allows an endpoint device to extend the initialization time beyond 1 second by issuing Configuration Request Retry Status (CRS) for a vendor ID read request.
This basically means "I'm busy now, please call me back later".
There are two moving parts to CRS support from the SW perspective. One part is to determine if CRS is supported or not. The second part is to set the CRS visibility register.
As part of the probe, the Linux kernel sets the above two conditions in
pci_enable_crsfunction. The kernel is also honoring the returned CRS inpci_bus_read_dev_vendor_idfunction if supported. The function will poll up to specified amount of time while endpoint is returning CRS response.The PCIe spec also allows CRS to be issued during cold, warm, hot and FLR resets.
The hot reset is initiated by starting a secondary bus reset. This patch is adding vendor ID read immediately after a bus reset so that the initialization procedure can be extended by the amount of time endpoint requires.
Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)drivers/pci/pci.c +32
-0@@ -3829,6 +3829,44 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)return 0 ; } +/* +* Mostly copy paste from pci_walk_bus with the exceptions of hard coded +* work and removed locks. +*/ +static void pci_bus_probe_crs (struct pci_bus *top) +{ +struct pci_dev *dev; +struct pci_bus *bus; +struct list_head *next; +int retval; +u32 l; + + bus = top; + next = top->devices.next; +for (;;) { +if (next == &bus->devices) { +/* end of this bus, go up or finish */ +if (bus == top) +break ; + next = bus->self->bus_list.next; + bus = bus->self->bus; +continue ; + } + dev = list_entry(next, struct pci_dev, bus_list); +if (dev->subordinate) { +/* this is a pci-pci bridge, do its devices next */ + next = dev->subordinate->devices.next; + bus = dev->subordinate; + }else + next = dev->bus_list.next; + + retval = pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &l,Quoted 8 lines
drivers/pci/pci.c static int pci_pm_reset(struct pci_dev *dev, int probe) · line 3868
Quoted 5 lines
drivers/pci/pci.c +5
-0+ 60 * 1000); +if (retval) +break ; + } +}Sigh. Man, this is ugly. Maybe we're locked into the current strategy and don't really have a choice, but I really don't like it.
Quoted 2 lines
I can add a locked version of the walkbus API.
Then, I can minimize this code to a couple of lines. How does that sound?
I didn't mean that, I meant the whole idea of having to walk the whole hierarchy and touch each device. It's sort of like we're enumerating things, but not really, so this checking is kinda sorta parallel to the enumeration path.
Quoted 7 lines
Quoted 2 lines
You mentioned several kinds of reset where CRS is allowed. Doesn't this fix only one of them? I know we support at least FLR reset also.
The CRS is for hot reset, warm reset and FLR reset. There is nothing we can do in SW for warm reset. This patch is to address hot reset caused by SBR.
I was hoping that Alex would help us for directions on the FLR reset later.
What sort of help from Alex were you hoping for? Is fixing the FLR path harder than this one? If we're going to fix one path, I'd prefer to fix them all at the same time rather than tripping over this again later.
Re: [PATCH 2/2] PCI: add CRS support to error handling path
Quoted 80 lines · Bjorn Helgaas
Quoted 79 lines · Sinan Kaya
Quoted 77 lines · Bjorn Helgaas
Quoted 63 lines · Sinan Kaya
The PCIE spec allows an endpoint device to extend the initialization time beyond 1 second by issuing Configuration Request Retry Status (CRS) for a vendor ID read request.
This basically means "I'm busy now, please call me back later".
There are two moving parts to CRS support from the SW perspective. One part is to determine if CRS is supported or not. The second part is to set the CRS visibility register.
As part of the probe, the Linux kernel sets the above two conditions in
pci_enable_crsfunction. The kernel is also honoring the returned CRS inpci_bus_read_dev_vendor_idfunction if supported. The function will poll up to specified amount of time while endpoint is returning CRS response.The PCIe spec also allows CRS to be issued during cold, warm, hot and FLR resets.
The hot reset is initiated by starting a secondary bus reset. This patch is adding vendor ID read immediately after a bus reset so that the initialization procedure can be extended by the amount of time endpoint requires.
Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)drivers/pci/pci.c +27
-0@@ -3829,6 +3829,44 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)return 0 ; } +/* +* Mostly copy paste from pci_walk_bus with the exceptions of hard coded +* work and removed locks. +*/ +static void pci_bus_probe_crs (struct pci_bus *top) +{ +struct pci_dev *dev; +struct pci_bus *bus; +struct list_head *next; +int retval; +u32 l; + + bus = top; + next = top->devices.next; +for (;;) { +if (next == &bus->devices) { +/* end of this bus, go up or finish */ +if (bus == top) +break ; + next = bus->self->bus_list.next; + bus = bus->self->bus; +continue ; + } + dev = list_entry(next, struct pci_dev, bus_list); +if (dev->subordinate) { +/* this is a pci-pci bridge, do its devices next */ + next = dev->subordinate->devices.next;drivers/pci/pci.c static int pci_pm_reset(struct pci_dev *dev, int probe) · line 3868
Quoted 10 lines
drivers/pci/pci.c +10
-0+ bus = dev->subordinate; + }else + next = dev->bus_list.next; + + retval =pci_bus_read_dev_vendor_id (dev->bus, dev->devfn, &l, +60 *1000 ); +if (retval) +break ; + } +}Sigh. Man, this is ugly. Maybe we're locked into the current strategy and don't really have a choice, but I really don't like it.
Quoted 7 lines
Quoted 2 lines
I can add a locked version of the walkbus API.
Then, I can minimize this code to a couple of lines. How does that sound?I didn't mean that, I meant the whole idea of having to walk the whole hierarchy and touch each device. It's sort of like we're enumerating things, but not really, so this checking is kinda sorta parallel to the enumeration path.
Well, we have to do this to run the CRS algorithm against all the devices that are issuing the CRS. Hot reset is a broadcast message. There could be multiple devices issuing a CRS in the tree. We should not start talking to the device before CRS procedure is finished for the device.
The restore_bus routine blindly assumes that all devices under this tree are accessible.
However, I take your concern about save and restore. We could potentially get rid of both save, restore and CRS code and then, try to re-enumerate. I am afraid that can also interfere with the AER error handling notifications present in the AER driver. A device driver wouldn't get a chance to quiesce itself before re-enumeration and can leave dangling threads around.
I'm curious if there is any way we can rescan the bus without involving the endpoint drivers. If we can do that then this could potentially work. Re-enumeration can also potentially assign different resources that the endpoint already has mapped. I see even a bigger problem there.
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
You mentioned several kinds of reset where CRS is allowed. Doesn't this fix only one of them? I know we support at least FLR reset also.
The CRS is for hot reset, warm reset and FLR reset. There is nothing we can do in SW for warm reset. This patch is to address hot reset caused by SBR.
Quoted 7 lines
Quoted 1 line
I was hoping that Alex would help us for directions on the FLR reset later.
What sort of help from Alex were you hoping for? Is fixing the FLR path harder than this one? If we're going to fix one path, I'd prefer to fix them all at the same time rather than tripping over this again later.
I don't mind taking a stab at all paths if possible. Last time I checked, there is already some code trying to find the endpoint. Alex is on the CC. he can always review my change.
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 38 lines · Sinan Kaya
A secondary bus reset causes settings to be lost by all downstream devices on the tree. The code is currently saving and restoring device states only when called from the VFIO path via
pci_probe_reset_busandpci_reset_busfunctions.Moving the save and restore into
pci_reset_bridge_secondary_busso that all users of the API have the same behavior.Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)drivers/pci/pci.c +0
-13@@ -3860,19 +3860,6 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)pci_reset_secondary_bus (dev); } -/** -* pci_reset_bridge_secondary_bus - Reset the secondary bus on a PCI bridge. -* @dev: Bridge device -* -* Use the bridge control register to assert reset on the secondary bus. -* Devices on the secondary bus are left in power-on state. -*/ -void pci_reset_bridge_secondary_bus (struct pci_dev *dev) -{ -pcibios_reset_secondary_bus (dev); -} -EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus); -static int pci_parent_bus_reset (struct pci_dev *dev,int probe) {struct pci_dev *pdev;
drivers/pci/pci.c static void pci_slot_restore(struct pci_slot *slot) · line 4357
Quoted 9 lines
drivers/pci/pci.c +6
-0@@ -4362,6 +4349,21 @@ static void pci_slot_restore(struct pci_slot *slot) } } +/** +* pci_reset_bridge_secondary_bus - Reset the secondary bus on a PCI bridge. +* @dev: Bridge device +* +* Use the bridge control register to assert reset on the secondary bus. +* Devices on the secondary bus are left in power-on state.
This comment is no longer correct (you don't leave the devices in power-on state).
drivers/pci/pci.c static void pci_slot_restore(struct pci_slot *slot) · line 4361
Quoted 4 lines
drivers/pci/pci.c +4
-0+*/ +void pci_reset_bridge_secondary_bus (struct pci_dev *dev) +{ +pci_bus_save_and_disable (dev->bus);
"dev" is a bridge, and dev->bus is the bus on the *primary* side of the bridge, so I think this saves and disables the bridge itself. Don't you want dev->subordinate here instead?
Quoted 42 lines
drivers/pci/pci.c +5
-8+pcibios_reset_secondary_bus (dev); +pci_bus_restore (dev->bus); +} +EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus); +static int pci_slot_reset (struct pci_slot *slot,int probe) {int rc;@@ -4504,12 +4506,8 @@ int pci_reset_bus(struct pci_bus *bus)if (rc)return rc; -pci_bus_save_and_disable (bus); - rc =pci_bus_reset (bus,0 ); -pci_bus_restore (bus); -return rc; }EXPORT_SYMBOL_GPL (pci_reset_bus);@@ -4528,8 +4526,6 @@ int pci_try_reset_bus(struct pci_bus *bus)if (rc)return rc; -pci_bus_save_and_disable (bus); -if (pci_bus_trylock (bus)) {might_sleep ();pci_reset_bridge_secondary_bus (bus->self);@@ -4537,8 +4533,6 @@ int pci_try_reset_bus(struct pci_bus *bus) }else rc = -EAGAIN ; -pci_bus_restore (bus); -return rc; }EXPORT_SYMBOL_GPL (pci_try_reset_bus);Signature · 1 line
1.9.1
Hi Bjorn,
Quoted 3 lines · Bjorn Helgaas
This comment is no longer correct (you don't leave the devices in power-on state).
Sorry about that, I can fix the comment.
Quoted 9 lines
drivers/pci/pci.c static void pci_slot_restore(struct pci_slot *slot) · line 4361
Quoted 5 lines
+ */
drivers/pci/pci.c +3
-0+void pci_reset_bridge_secondary_bus (struct pci_dev *dev) +{ +pci_bus_save_and_disable (dev->bus);"dev" is a bridge, and
dev->busis the bus on the *primary* side of the bridge, so I think this saves and disables the bridge itself. Don't you wantdev->subordinatehere instead?
Yes, I need the subordinate.
Quoted 6 lines
Quoted 6 lines
drivers/pci/pci.c +5
-0+pcibios_reset_secondary_bus (dev); +pci_bus_restore (dev->bus); +} +EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus); +
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 43 lines · Sinan Kaya
A secondary bus reset causes settings to be lost by all downstream devices on the tree. The code is currently saving and restoring device states only when called from the VFIO path via
pci_probe_reset_busandpci_reset_busfunctions.Moving the save and restore into
pci_reset_bridge_secondary_busso that all users of the API have the same behavior.Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)drivers/pci/pci.c +2
-13@@ -3860,19 +3860,6 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)pci_reset_secondary_bus (dev); } -/** -* pci_reset_bridge_secondary_bus - Reset the secondary bus on a PCI bridge. -* @dev: Bridge device -* -* Use the bridge control register to assert reset on the secondary bus. -* Devices on the secondary bus are left in power-on state. -*/ -void pci_reset_bridge_secondary_bus (struct pci_dev *dev) -{ -pcibios_reset_secondary_bus (dev); -} -EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus); -static int pci_parent_bus_reset (struct pci_dev *dev,int probe) {struct pci_dev *pdev;@@ -4362,6 +4349,21 @@ static void pci_slot_restore(struct pci_slot *slot) } } +/** +* pci_reset_bridge_secondary_bus - Reset the secondary bus on a PCI bridge.
drivers/pci/pci.c static void pci_slot_restore(struct pci_slot *slot) · line 4363
Quoted 10 lines
drivers/pci/pci.c +10
-0+* @dev: Bridge device +* +* Use the bridge control register to assert reset on the secondary bus. +* Devices on the secondary bus are left in power-on state. +*/ +void pci_reset_bridge_secondary_bus (struct pci_dev *dev) +{ +pci_bus_save_and_disable (dev->bus); +pcibios_reset_secondary_bus (dev); +pci_bus_restore (dev->bus);
This path eventually writes the Bridge Control register:
pci_reset_bridge_secondary_bus
pcibios_reset_secondary_bus
pci_reset_secondary_bus
pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl)
But I think it'd be easy to call this on a non-bridge device, and I don't think there's anything in the path that checks whether this is actually a bridge. I wonder if we should check that somewhere, or maybe even change the interface so it takes a struct pci_bus instead of a pci_dev.
Quoted 40 lines
drivers/pci/pci.c +3
-8+} +EXPORT_SYMBOL_GPL (pci_reset_bridge_secondary_bus); +static int pci_slot_reset (struct pci_slot *slot,int probe) {int rc;@@ -4504,12 +4506,8 @@ int pci_reset_bus(struct pci_bus *bus)if (rc)return rc; -pci_bus_save_and_disable (bus); - rc =pci_bus_reset (bus,0 ); -pci_bus_restore (bus); -return rc; }EXPORT_SYMBOL_GPL (pci_reset_bus);@@ -4528,8 +4526,6 @@ int pci_try_reset_bus(struct pci_bus *bus)if (rc)return rc; -pci_bus_save_and_disable (bus); -if (pci_bus_trylock (bus)) {might_sleep ();pci_reset_bridge_secondary_bus (bus->self);@@ -4537,8 +4533,6 @@ int pci_try_reset_bus(struct pci_bus *bus) }else rc = -EAGAIN ; -pci_bus_restore (bus); -return rc; }EXPORT_SYMBOL_GPL (pci_try_reset_bus);Signature · 1 line
1.9.1
Quoted 8 lines · Bjorn Helgaas
drivers/pci/pci.c static void pci_slot_restore(struct pci_slot *slot) · line 4363
Quoted 1 line
drivers/pci/pci.c +1
-0+pci_bus_restore (dev->bus);This path eventually writes the Bridge Control register:
pci_reset_bridge_secondary_bus
pcibios_reset_secondary_bus
pci_reset_secondary_bus
pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl)
Quoted 6 lines
But I think it'd be easy to call this on a non-bridge device, and I don't think there's anything in the path that checks whether this is actually a bridge. I wonder if we should check that somewhere, or maybe even change the interface so it takes a struct
pci_businstead of apci_dev.
I'll change it to pci_bus.
Quoted 1 line
Quoted 1 line
Quoted 1 line
+}
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 9 lines · Sinan Kaya
Quoted 8 lines · Bjorn Helgaas
drivers/pci/pci.c static void pci_slot_restore(struct pci_slot *slot) · line 4363
Quoted 1 line
drivers/pci/pci.c +1
-0+pci_bus_restore (dev->bus);This path eventually writes the Bridge Control register:
pci_reset_bridge_secondary_bus
pcibios_reset_secondary_bus
pci_reset_secondary_bus
pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl)
Quoted 12 lines
Quoted 6 lines
But I think it'd be easy to call this on a non-bridge device, and I don't think there's anything in the path that checks whether this is actually a bridge. I wonder if we should check that somewhere, or maybe even change the interface so it takes a struct
pci_businstead of apci_dev.I'll change it to
pci_bus.Quoted 1 line
Quoted 1 line
Quoted 1 line
+}
Changing the API seems to become a bigger mess. pcibios_reset_secondary_bus has weaker friends in different architectures.
I can move the check in pci_parent_bus_reset
if (pci_is_root_bus(dev->bus) || dev->subordinate ||
!dev->bus->self || dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
return -ENOTTY;inside pci_reset_bridge_secondary_bus instead.
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.