[PATCH V2 0/5] PCI: error handling clean up and add CRS support

[PATCH V2 0/5] PCI: error handling clean up and add CRS support

[Sinan Kaya]

Device states on the bus are not saved and restored for some of the bus
reset paths as:

1. IB/hfi1 via pci_reset_bridge_secondary_bus
2. PCI/AER via pci_reset_bridge_secondary_bus
3. PCI: dev_reset via parent bus reset

Changing the external API usage to pci_reset_bus outside of PCI code and
adding save/restore into pci_parent_bus_reset function. Note that
pci_parent_bus_reset is called with a device lock held.

A PCIe endpoint is allowed to issue CRS following an FLR request to
indicate that it is not ready to accept new requests. Changing the polling
mechanism in FLR wait function to go read the vendor ID instead of the
command/status register. A CRS indication will only be given if the address
to be read is vendor ID.

v1:
http://www.spinics.net/lists/linux-pci/msg53596.html
* initial implementation

Sinan Kaya (5):
  PCI/AER: replace pci_reset_bridge_secondary_bus with pci_reset_bus
  IB/hfi1: replace pci_reset_bridge_secondary_bus with pci_reset_bus
  PCI: save and restore bus on parent bus reset
  PCI: add CRS support to error handling path
  PCI: handle CRS returned by device after FLR

 drivers/infiniband/hw/hfi1/pcie.c  |  4 +---
 drivers/pci/pci.c                  | 28 +++++++++++++++++++++++++++-
 drivers/pci/pcie/aer/aerdrv.c      |  2 +-
 drivers/pci/pcie/aer/aerdrv_core.c |  2 +-
 4 files changed, 30 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH V2 1/5] PCI/AER: replace pci_reset_bridge_secondary_bus with pci_reset_bus

[Sinan Kaya]

The error handling path is not saving and restoring the settings on
other devices getting impacted by a secondary bus reset. Using a better
API to take care of this.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aer/aerdrv.c      | 2 +-
 drivers/pci/pcie/aer/aerdrv_core.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index 48d21e0..7880cd1 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -350,7 +350,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
 	pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, reg32);
 
-	pci_reset_bridge_secondary_bus(dev);
+	pci_reset_bus(dev->subordinate);
 	dev_printk(KERN_DEBUG, &dev->dev, "Root Port link has been reset\n");
 
 	/* Clear Root Error Status */
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 521e39c..0339cc3 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -403,7 +403,7 @@ static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
  */
 static pci_ers_result_t default_reset_link(struct pci_dev *dev)
 {
-	pci_reset_bridge_secondary_bus(dev);
+	pci_reset_bus(dev->subordinate);
 	dev_printk(KERN_DEBUG, &dev->dev, "downstream link has been reset\n");
 	return PCI_ERS_RESULT_RECOVERED;
 }
-- 
1.9.1

[PATCH V2 2/5] IB/hfi1: replace pci_reset_bridge_secondary_bus with pci_reset_bus

[Sinan Kaya]

Save and restore the settings on other devices impacted by the secondary
bus reset downstream.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/infiniband/hw/hfi1/pcie.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
index 89c68da..f8f21fb 100644
--- a/drivers/infiniband/hw/hfi1/pcie.c
+++ b/drivers/infiniband/hw/hfi1/pcie.c
@@ -861,9 +861,7 @@ static int trigger_sbr(struct hfi1_devdata *dd)
 	 * delay after a reset is required.  Per spec requirements,
 	 * the link is either working or not after that point.
 	 */
-	pci_reset_bridge_secondary_bus(dev->bus->self);
-
-	return 0;
+	return pci_reset_bus(dev->bus);
 }
 
 /*
-- 
1.9.1

[PATCH V2 3/5] PCI: save and restore bus on parent bus reset

[Sinan Kaya]

Device states on the bus are saved and restored for all bus resets except
the one initiated through pci_dev_reset. Filling the hole.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d51..8aecab1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -51,6 +51,10 @@ static void pci_pme_list_scan(struct work_struct *work);
 static LIST_HEAD(pci_pme_list);
 static DEFINE_MUTEX(pci_pme_list_mutex);
 static DECLARE_DELAYED_WORK(pci_pme_work, pci_pme_list_scan);
+static void pci_dev_lock(struct pci_dev *dev);
+static void pci_dev_unlock(struct pci_dev *dev);
+static void pci_bus_save_and_disable(struct pci_bus *bus);
+static void pci_bus_restore(struct pci_bus *bus);
 
 struct pci_pme_device {
 	struct list_head list;
@@ -3888,8 +3892,18 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
 	if (probe)
 		return 0;
 
+	if (!probe) {
+		pci_dev_unlock(dev);
+		pci_bus_save_and_disable(dev->bus);
+	}
+
 	pci_reset_bridge_secondary_bus(dev->bus->self);
 
+	if (!probe) {
+		pci_bus_restore(dev->bus);
+		pci_dev_lock(dev);
+	}
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH V2 4/5] PCI: add CRS support to error handling path

[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_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.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8aecab1..e913467 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3833,6 +3833,16 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
 	return 0;
 }
 
+static int pci_dev_probe_crs(struct pci_dev *dev, void *data)
+{
+	u32 l;
+
+	pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &l,
+				   60 * 1000);
+
+	return 0;
+}
+
 void pci_reset_secondary_bus(struct pci_dev *dev)
 {
 	u16 ctrl;
@@ -3874,6 +3884,7 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)
 void pci_reset_bridge_secondary_bus(struct pci_dev *dev)
 {
 	pcibios_reset_secondary_bus(dev);
+	pci_walk_bus(dev->subordinate, pci_dev_probe_crs, NULL);
 }
 EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
 
-- 
1.9.1

[PATCH V2 5/5] PCI: handle CRS returned by device after FLR

[Sinan Kaya]

An endpoint is allowed to issue CRS following an FLR request to indicate
that it is not ready to accept new requests. Changing the polling mechanism
in FLR wait function to go read the vendor ID instead of the command/status
register. A CRS indication will only be given if the address to be read is
vendor ID.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e913467..1def11e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3729,7 +3729,8 @@ static void pci_flr_wait(struct pci_dev *dev)
 
 	do {
 		msleep(100);
-		pci_read_config_dword(dev, PCI_COMMAND, &id);
+		pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &id,
+					   60 * 1000);
 	} while (i++ < 10 && id == ~0);
 
 	if (id == ~0)
-- 
1.9.1

Re: [PATCH V2 3/5] PCI: save and restore bus on parent bus reset

[Bjorn Helgaas]

Hi Sinan,

On Fri, Sep 16, 2016 at 04:06:32PM -0400, Sinan Kaya wrote:
> Device states on the bus are saved and restored for all bus resets except
> the one initiated through pci_dev_reset. Filling the hole.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d51..8aecab1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -51,6 +51,10 @@ static void pci_pme_list_scan(struct work_struct *work);
>  static LIST_HEAD(pci_pme_list);
>  static DEFINE_MUTEX(pci_pme_list_mutex);
>  static DECLARE_DELAYED_WORK(pci_pme_work, pci_pme_list_scan);
> +static void pci_dev_lock(struct pci_dev *dev);
> +static void pci_dev_unlock(struct pci_dev *dev);
> +static void pci_bus_save_and_disable(struct pci_bus *bus);
> +static void pci_bus_restore(struct pci_bus *bus);
>  
>  struct pci_pme_device {
>  	struct list_head list;
> @@ -3888,8 +3892,18 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
>  	if (probe)
>  		return 0;
>  
> +	if (!probe) {
> +		pci_dev_unlock(dev);
> +		pci_bus_save_and_disable(dev->bus);
> +	}
> +
>  	pci_reset_bridge_secondary_bus(dev->bus->self);
>  
> +	if (!probe) {
> +		pci_bus_restore(dev->bus);
> +		pci_dev_lock(dev);
> +	}

This pattern of "unlock, do something, relock" needs some
justification.  In general it's unsafe because the lock is protecting
*something*, and you have to assume that something can change as soon
as you unlock.  Maybe you know it's safe in this situation, and if so,
the explanation of why it's safe is what I'm looking for.

Also, you're now calling pci_reset_bridge_secondary_bus() with the dev
unlocked, where we called it with the dev locked before.  Some (but
worryingly, not all) of the other pci_reset_bridge_secondary_bus()
callers also have the dev locked.  I didn't look long enough to figure
out if there is a strategy there or if these inconsistencies are
latent bugs.

> +
>  	return 0;
>  }
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/5] PCI: save and restore bus on parent bus reset

[Sinan Kaya]

Hi Bjorn,

On 9/29/2016 5:49 PM, Bjorn Helgaas wrote:
>> +	}
> This pattern of "unlock, do something, relock" needs some
> justification.  In general it's unsafe because the lock is protecting
> *something*, and you have to assume that something can change as soon
> as you unlock.  Maybe you know it's safe in this situation, and if so,
> the explanation of why it's safe is what I'm looking for.

Agreed. 

The problem is that save and restore routines obtain the lock again and
they fails as the lock is already held.

The alternative is to change the dev_locks in save and restore to try_lock
so that it will work if locks were previously obtained or not. 

> 
> Also, you're now calling pci_reset_bridge_secondary_bus() with the dev
> unlocked, where we called it with the dev locked before.  Some (but
> worryingly, not all) of the other pci_reset_bridge_secondary_bus()
> callers also have the dev locked.  I didn't look long enough to figure
> out if there is a strategy there or if these inconsistencies are
> latent bugs.
> 

The goal of this routine is to reset the device not the bridge and the code
will use FLR or others if available. Therefore, it makes perfect sense to
obtain the device lock while doing this. 

The code tries to reset the bus if none of the other resets work. This is
where the problem is. It destroys the context for other devices.

I can fix get rid of this unlock, do something and then lock again business
by rewriting the locks in save and restore.

Sinan

-- 
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 V2 3/5] PCI: save and restore bus on parent bus reset

[Sinan Kaya]

On 9/29/2016 7:50 PM, Sinan Kaya wrote:
> Hi Bjorn,
> 
> On 9/29/2016 5:49 PM, Bjorn Helgaas wrote:
>>> +	}
>> This pattern of "unlock, do something, relock" needs some
>> justification.  In general it's unsafe because the lock is protecting
>> *something*, and you have to assume that something can change as soon
>> as you unlock.  Maybe you know it's safe in this situation, and if so,
>> the explanation of why it's safe is what I'm looking for.
> 
> Agreed. 
> 
> The problem is that save and restore routines obtain the lock again and
> they fails as the lock is already held.
> 
> The alternative is to change the dev_locks in save and restore to try_lock
> so that it will work if locks were previously obtained or not. 
> 
>>
>> Also, you're now calling pci_reset_bridge_secondary_bus() with the dev
>> unlocked, where we called it with the dev locked before.  Some (but
>> worryingly, not all) of the other pci_reset_bridge_secondary_bus()
>> callers also have the dev locked.  I didn't look long enough to figure
>> out if there is a strategy there or if these inconsistencies are
>> latent bugs.
>>
> 
> The goal of this routine is to reset the device not the bridge and the code
> will use FLR or others if available. Therefore, it makes perfect sense to
> obtain the device lock while doing this. 
> 
> The code tries to reset the bus if none of the other resets work. This is
> where the problem is. It destroys the context for other devices.
> 
> I can fix get rid of this unlock, do something and then lock again business
> by rewriting the locks in save and restore.
> 
> Sinan
> 

I looked at the code a little bit more. I missed the fact pci_parent_bus_reset
will only work if there is only PCI device on the bus and that device is the
reset requesting device.

I'll drop this patch as well as the infiniband version for the same reason.




-- 
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 V2 5/5] PCI: handle CRS returned by device after FLR

[Sinan Kaya]

On 9/16/2016 4:06 PM, Sinan Kaya wrote:
> An endpoint is allowed to issue CRS following an FLR request to indicate
> that it is not ready to accept new requests. Changing the polling mechanism
> in FLR wait function to go read the vendor ID instead of the command/status
> register. A CRS indication will only be given if the address to be read is
> vendor ID.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e913467..1def11e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3729,7 +3729,8 @@ static void pci_flr_wait(struct pci_dev *dev)
>  
>  	do {
>  		msleep(100);
> -		pci_read_config_dword(dev, PCI_COMMAND, &id);
> +		pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &id,
> +					   60 * 1000);

We found out during Mellanox CX3 testing that the card returns the vendor
ID earlier during FLR while other cards are OK with this implementaiton. 

I think we need both of the reads to support the CRS for the endpoints
and also the command id register read to make sure endpoint init is complete.


>  	} while (i++ < 10 && id == ~0);
>  
>  	if (id == ~0)
> 


-- 
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.