diff mbox series

[v4,2/3] PCI: Cache CRS Software Visibiliy in struct pci_dev

Message ID 20200307172044.29645-3-stanspas@amazon.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series Improve PCI device post-reset readiness polling | expand

Commit Message

Stanislav Spassov March 7, 2020, 5:20 p.m. UTC 
From: Stanislav Spassov <stanspas@amazon.de>

Arguably, since CRS SV is a capability of Root Ports and determines
how Root Ports will handle incoming CRS Completions, it makes more
sense to store this flag in the struct pci_bus that represents the
port's secondary bus, and to cache it in any buses further down the
hierarchy.

However, storing the flag in struct pci_dev allows individual devices
to be marked as not supporting CRS properly even when CRS SV is enabled
on their parent Root Port. This way, future code that relies on the new
flag will not be misled that it is safe to probe a device by relying on
CRS SV to not cause platform timeouts (See the note on CRS SV on p. 553
of PCI Express Base Specification r5.0 from May 22, 2019).

Note: Endpoints integrated into the Root Complex (RCiEP) may also be
capable of using CRS. In that case, the software visibility is
controlled using a Root Complex Register Block (RCRB). This is currently
not supported by the kernel. The code introduced here would simply not
set the newly introduced flag for RCiEP as for those bus->self is NULL.

Signed-off-by: Stanislav Spassov <stanspas@amazon.de>
---
 drivers/pci/probe.c | 8 +++++++-
 include/linux/pci.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Bjorn Helgaas Sept. 12, 2021, 1:32 p.m. UTC | #1 
On Sat, Mar 07, 2020 at 06:20:43PM +0100, Stanislav Spassov wrote:
> From: Stanislav Spassov <stanspas@amazon.de>
> 
> Arguably, since CRS SV is a capability of Root Ports and determines
> how Root Ports will handle incoming CRS Completions, it makes more
> sense to store this flag in the struct pci_bus that represents the
> port's secondary bus, and to cache it in any buses further down the
> hierarchy.
> 
> However, storing the flag in struct pci_dev allows individual devices
> to be marked as not supporting CRS properly even when CRS SV is enabled
> on their parent Root Port. This way, future code that relies on the new
> flag will not be misled that it is safe to probe a device by relying on
> CRS SV to not cause platform timeouts (See the note on CRS SV on p. 553
> of PCI Express Base Specification r5.0 from May 22, 2019).

If we find devices that don't support CRS properly, I think we should
quirk them directly with something other than "crssv_enabled".

> Note: Endpoints integrated into the Root Complex (RCiEP) may also be
> capable of using CRS. In that case, the software visibility is
> controlled using a Root Complex Register Block (RCRB). This is currently
> not supported by the kernel. The code introduced here would simply not
> set the newly introduced flag for RCiEP as for those bus->self is NULL.
> 
> Signed-off-by: Stanislav Spassov <stanspas@amazon.de>
> ---
>  drivers/pci/probe.c | 8 +++++++-
>  include/linux/pci.h | 3 +++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 512cb4312ddd..eeff8a07f330 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1080,9 +1080,11 @@ static void pci_enable_crs(struct pci_dev *pdev)
>  
>  	/* Enable CRS Software Visibility if supported */
>  	pcie_capability_read_word(pdev, PCI_EXP_RTCAP, &root_cap);
> -	if (root_cap & PCI_EXP_RTCAP_CRSVIS)
> +	if (root_cap & PCI_EXP_RTCAP_CRSVIS) {
>  		pcie_capability_set_word(pdev, PCI_EXP_RTCTL,
>  					 PCI_EXP_RTCTL_CRSSVE);
> +		pdev->crssv_enabled = true;

I'd use "crssv_enabled = 1" instead of mixing the "unsigned int" and
"bool" ideas.

> +	}
>  }
>  
>  static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> @@ -2414,6 +2416,10 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  	list_add_tail(&dev->bus_list, &bus->devices);
>  	up_write(&pci_bus_sem);
>  
> +	/* Propagate CRS Software Visibility bit from the parent bridge */
> +	if (bus->self)
> +		dev->crssv_enabled = bus->self->crssv_enabled;

I think we should use pcie_find_root_port() instead of propagating it.
SV is a property of the Root Port, and devices below it have no idea
whether it's enabled at the Root Port.

>  	ret = pcibios_add_device(dev);
>  	WARN_ON(ret < 0);
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 3840a541a9de..1c222c7c2572 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -354,6 +354,9 @@ struct pci_dev {
>  						      user sysfs */
>  	unsigned int	clear_retrain_link:1;	/* Need to clear Retrain Link
>  						   bit manually */
> +	unsigned int	crssv_enabled:1;	/* Configuration Request Retry
> +						   Status Software Visibility
> +						   enabled on (parent) bridge */
>  	unsigned int	d3_delay;	/* D3->D0 transition time in ms */
>  	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
>  
> -- 
> 2.25.1
> 
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 
> 
>
Spassov, Stanislav Sept. 13, 2021, 4:06 p.m. UTC | #2 
On Sun, 2021-09-12 at 08:32 -0500, Bjorn Helgaas wrote:
> On Sat, Mar 07, 2020 at 06:20:43PM +0100, Stanislav Spassov wrote:
> > However, storing the flag in struct pci_dev allows individual devices
> > to be marked as not supporting CRS properly even when CRS SV is enabled
> > on their parent Root Port. This way, future code that relies on the new
> > flag will not be misled that it is safe to probe a device by relying on
> > CRS SV to not cause platform timeouts (See the note on CRS SV on p. 553
> > of PCI Express Base Specification r5.0 from May 22, 2019).
> 
> If we find devices that don't support CRS properly, I think we should
> quirk them directly with something other than "crssv_enabled".

I am definitely open to suggestions here. Based on precedent such as the
broken_intx_masking field in struct pci_dev and how it is set from
pci/quirks.c, we could have a new field "broken_crs".

In hindsight, the code from PATCH 3/3 which is conditionally executed based
on this flag, should be okay to execute unconditionally: polling the Vendor
ID is safer than polling Command when CRS SV is enabled but it is still not
any more dangerous when CRS SV is disabled. This consideration allows us
to drop the current PATCH 2/3 altogether. I will implement this approach in
the next reversion of the series (it is old and needs rebasing anyway).



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
diff mbox series

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 512cb4312ddd..eeff8a07f330 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1080,9 +1080,11 @@  static void pci_enable_crs(struct pci_dev *pdev)
 
 	/* Enable CRS Software Visibility if supported */
 	pcie_capability_read_word(pdev, PCI_EXP_RTCAP, &root_cap);
-	if (root_cap & PCI_EXP_RTCAP_CRSVIS)
+	if (root_cap & PCI_EXP_RTCAP_CRSVIS) {
 		pcie_capability_set_word(pdev, PCI_EXP_RTCTL,
 					 PCI_EXP_RTCTL_CRSSVE);
+		pdev->crssv_enabled = true;
+	}
 }
 
 static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
@@ -2414,6 +2416,10 @@  void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	list_add_tail(&dev->bus_list, &bus->devices);
 	up_write(&pci_bus_sem);
 
+	/* Propagate CRS Software Visibility bit from the parent bridge */
+	if (bus->self)
+		dev->crssv_enabled = bus->self->crssv_enabled;
+
 	ret = pcibios_add_device(dev);
 	WARN_ON(ret < 0);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3840a541a9de..1c222c7c2572 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -354,6 +354,9 @@  struct pci_dev {
 						      user sysfs */
 	unsigned int	clear_retrain_link:1;	/* Need to clear Retrain Link
 						   bit manually */
+	unsigned int	crssv_enabled:1;	/* Configuration Request Retry
+						   Status Software Visibility
+						   enabled on (parent) bridge */
 	unsigned int	d3_delay;	/* D3->D0 transition time in ms */
 	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */