[PATCH 1/2] PCI: save and restore device state during bus reset

[PATCH 1/2] PCI: save and restore device state during bus reset

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

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

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d51..b209378 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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);
-- 
1.9.1

[PATCH 2/2] 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 | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b209378..ebd0fc6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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);
 
-- 
1.9.1

Re: [PATCH 2/2] PCI: add CRS support to error handling path

[Sinan Kaya]

On 9/1/2016 7:00 PM, Sinan Kaya wrote:
>  	pcibios_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.

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

[Sinan Kaya]

Hi Bjorn,

On 9/1/2016 7:00 PM, Sinan Kaya wrote:
> 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 | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b209378..ebd0fc6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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);
>  
> 

Any feedback on the direction?

-- 
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 1/2] PCI: save and restore device state during bus reset

[Bjorn Helgaas]

On Thu, Sep 01, 2016 at 07:00:00PM -0400, Sinan Kaya wrote:
> 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.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c | 36 +++++++++++++++---------------------
>  1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d51..b209378 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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.

This comment is no longer correct (you don't leave the devices in
power-on state).

> + */
> +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?

> +	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);
> -- 
> 1.9.1
> 

Re: [PATCH 1/2] PCI: save and restore device state during bus reset

[Sinan Kaya]

Hi Bjorn,

On 9/13/2016 12:42 PM, Bjorn Helgaas wrote:
> 
> This comment is no longer correct (you don't leave the devices in
> power-on state).

Sorry about that, I can fix the comment.

> 
>> + */
>> +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?

Yes, I need the subordinate. 

> 
>> +	pcibios_reset_secondary_bus(dev);
>> +	pci_bus_restore(dev->bus);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
>> +


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

[Bjorn Helgaas]

On Thu, Sep 01, 2016 at 07:00:01PM -0400, Sinan Kaya wrote:
> 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 | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b209378..ebd0fc6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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;
> +	}
> +}

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.

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

>  }
>  EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
>  
> -- 
> 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 2/2] PCI: add CRS support to error handling path

[Sinan Kaya]

On 9/13/2016 4:01 PM, Bjorn Helgaas wrote:
> On Thu, Sep 01, 2016 at 07:00:01PM -0400, Sinan Kaya wrote:
>> 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 | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index b209378..ebd0fc6 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -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;
>> +	}
>> +}
> 
> 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?

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

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

> 
>>  }
>>  EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
>>  
>> -- 
>> 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
> 


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

[Bjorn Helgaas]

On Tue, Sep 13, 2016 at 05:04:49PM -0400, Sinan Kaya wrote:
> On 9/13/2016 4:01 PM, Bjorn Helgaas wrote:
> > On Thu, Sep 01, 2016 at 07:00:01PM -0400, Sinan Kaya wrote:
> >> 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 | 39 +++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 39 insertions(+)
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index b209378..ebd0fc6 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -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;
> >> +	}
> >> +}
> > 
> > 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?

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.

> > 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 1/2] PCI: save and restore device state during bus reset

[Bjorn Helgaas]

On Thu, Sep 01, 2016 at 07:00:00PM -0400, Sinan Kaya wrote:
> 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.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c | 36 +++++++++++++++---------------------
>  1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d51..b209378 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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);

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.

> +}
> +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);
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] PCI: add CRS support to error handling path

[Sinan Kaya]

On 9/13/2016 5:47 PM, Bjorn Helgaas wrote:
> On Tue, Sep 13, 2016 at 05:04:49PM -0400, Sinan Kaya wrote:
>> On 9/13/2016 4:01 PM, Bjorn Helgaas wrote:
>>> On Thu, Sep 01, 2016 at 07:00:01PM -0400, Sinan Kaya wrote:
>>>> 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 | 39 +++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index b209378..ebd0fc6 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -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;
>>>> +	}
>>>> +}
>>>
>>> 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?
> 
> 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.


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

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. 


-- 
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 1/2] PCI: save and restore device state during bus reset

[Sinan Kaya]

On 9/13/2016 5:53 PM, Bjorn Helgaas wrote:
>> +	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.
> 

I'll change it to pci_bus.

>> > +}


-- 
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 1/2] PCI: save and restore device state during bus reset

[Sinan Kaya]

On 9/13/2016 7:20 PM, Sinan Kaya wrote:
> On 9/13/2016 5:53 PM, Bjorn Helgaas wrote:
>>> +	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.
>>
> 
> I'll change it to pci_bus.
> 
>>>> +}
> 
> 

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.

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