[PATCH v1 0/2] PCI: Probe for reset support earlier

[PATCH v1 0/2] PCI: Probe for reset support earlier

[Bjorn Helgaas]

The PCI core currently uses a late_initcall to probe each device for
whether it supports reset.  This is dangerous because a driver may have
already claimed the device by this point, and the PCI core should not
interfere with the driver by touching the device on its own.

These patches move the probe to be earlier, during enumeration, before a
driver has a chance to claim the device.

---

Bjorn Helgaas (2):
      PCI: Probe for device reset support before driver claim
      PCI: Remove redundant probes for device reset support


 drivers/pci/pci-sysfs.c |    3 +--
 drivers/pci/pci.c       |   15 ++++++---------
 drivers/pci/probe.c     |    3 +++
 3 files changed, 10 insertions(+), 11 deletions(-)

[PATCH v1 1/2] PCI: Probe for device reset support before driver claim

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Previously we called pci_probe_reset_function() in this path:

  pci_sysfs_init                              # late_initcall
    for_each_pci_dev(dev)
      pci_create_sysfs_dev_files(dev)
	pci_create_capabilities_sysfs(dev)
	  pci_probe_reset_function
	    pci_dev_specific_reset
	    pcie_has_flr
              pcie_capability_read_dword

pci_sysfs_init() is a late_initcall, and a driver may have already claimed
one of these devices and enabled runtime power management for it, so the
device could already be in D3hot by the time we get to pci_sysfs_init().

The device itself should respond to the config read even while it's in
D3hot, but if an upstream bridge is also in D3hot, the read won't even
reach the device because the bridge won't forward it downstream to the
device.  If the bridge is a PCIe port, it should complete the read as an
Unsupported Request, which may be reported to the CPU as an exception or as
invalid data.

Avoid this case by probing for reset support from pci_init_capabilities(),
before a driver can claim the device.  The device should be in D0 and fully
accessible at that point.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci-sysfs.c |    3 +--
 drivers/pci/probe.c     |    3 +++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index eb6bee8724cc..4933f0270471 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1542,11 +1542,10 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 	/* Active State Power Management */
 	pcie_aspm_create_sysfs_dev_files(dev);
 
-	if (!pci_probe_reset_function(dev)) {
+	if (dev->reset_fn) {
 		retval = device_create_file(&dev->dev, &reset_attr);
 		if (retval)
 			goto error;
-		dev->reset_fn = 1;
 	}
 	return 0;
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ef5377438a1e..489660d0d384 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2121,6 +2121,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
 
 	/* Advanced Error Reporting */
 	pci_aer_init(dev);
+
+	if (pci_probe_reset_function(dev) == 0)
+		dev->reset_fn = 1;
 }
 
 /*

[PATCH v1 2/2] PCI: Remove redundant probes for device reset support

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

We probe every device for whether it supports reset so we can tell whether
to create a sysfs "reset" file for it.  We do that probe in
pci_init_capabilities() during enumeration and save the result in
dev->reset_fn.  The result doesn't depend on any other devices on the bus
and shouldn't change after boot, so we don't need to do the probe again.

Remove the pci_probe_reset_function() calls and rely on the dev->reset_fn
we found during enumeration.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f6a4dd10d9b0..4db740e4f50a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4450,9 +4450,8 @@ int pci_reset_function(struct pci_dev *dev)
 {
 	int rc;
 
-	rc = pci_probe_reset_function(dev);
-	if (rc)
-		return rc;
+	if (!dev->reset_fn)
+		return -ENOTTY;
 
 	pci_dev_lock(dev);
 	pci_dev_save_and_disable(dev);
@@ -4487,9 +4486,8 @@ int pci_reset_function_locked(struct pci_dev *dev)
 {
 	int rc;
 
-	rc = pci_probe_reset_function(dev);
-	if (rc)
-		return rc;
+	if (!dev->reset_fn)
+		return -ENOTTY;
 
 	pci_dev_save_and_disable(dev);
 
@@ -4511,9 +4509,8 @@ int pci_try_reset_function(struct pci_dev *dev)
 {
 	int rc;
 
-	rc = pci_probe_reset_function(dev);
-	if (rc)
-		return rc;
+	if (!dev->reset_fn)
+		return -ENOTTY;
 
 	if (!pci_dev_trylock(dev))
 		return -EAGAIN;

Re: [PATCH v1 1/2] PCI: Probe for device reset support before driver claim

[Rafael J. Wysocki]

On Friday, February 16, 2018 11:50:11 PM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Previously we called pci_probe_reset_function() in this path:
> 
>   pci_sysfs_init                              # late_initcall
>     for_each_pci_dev(dev)
>       pci_create_sysfs_dev_files(dev)
> 	pci_create_capabilities_sysfs(dev)
> 	  pci_probe_reset_function
> 	    pci_dev_specific_reset
> 	    pcie_has_flr
>               pcie_capability_read_dword
> 
> pci_sysfs_init() is a late_initcall, and a driver may have already claimed
> one of these devices and enabled runtime power management for it, so the
> device could already be in D3hot by the time we get to pci_sysfs_init().

It also may be in D3cold if, say, AML can cut off power from it or ACPI
power resources can be used for that.

> The device itself should respond to the config read even while it's in
> D3hot, but if an upstream bridge is also in D3hot, the read won't even
> reach the device because the bridge won't forward it downstream to the
> device.  If the bridge is a PCIe port, it should complete the read as an
> Unsupported Request, which may be reported to the CPU as an exception or as
> invalid data.
> 
> Avoid this case by probing for reset support from pci_init_capabilities(),
> before a driver can claim the device.  The device should be in D0 and fully
> accessible at that point.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci-sysfs.c |    3 +--
>  drivers/pci/probe.c     |    3 +++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index eb6bee8724cc..4933f0270471 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1542,11 +1542,10 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  	/* Active State Power Management */
>  	pcie_aspm_create_sysfs_dev_files(dev);
>  
> -	if (!pci_probe_reset_function(dev)) {
> +	if (dev->reset_fn) {
>  		retval = device_create_file(&dev->dev, &reset_attr);
>  		if (retval)
>  			goto error;
> -		dev->reset_fn = 1;
>  	}
>  	return 0;
>  
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ef5377438a1e..489660d0d384 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2121,6 +2121,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  
>  	/* Advanced Error Reporting */
>  	pci_aer_init(dev);
> +
> +	if (pci_probe_reset_function(dev) == 0)
> +		dev->reset_fn = 1;
>  }
>  
>  /*
> 
> 

Re: [PATCH v1 2/2] PCI: Remove redundant probes for device reset support

[Rafael J. Wysocki]

On Friday, February 16, 2018 11:50:18 PM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> We probe every device for whether it supports reset so we can tell whether
> to create a sysfs "reset" file for it.  We do that probe in
> pci_init_capabilities() during enumeration and save the result in
> dev->reset_fn.  The result doesn't depend on any other devices on the bus
> and shouldn't change after boot, so we don't need to do the probe again.
> 
> Remove the pci_probe_reset_function() calls and rely on the dev->reset_fn
> we found during enumeration.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..4db740e4f50a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4450,9 +4450,8 @@ int pci_reset_function(struct pci_dev *dev)
>  {
>  	int rc;
>  
> -	rc = pci_probe_reset_function(dev);
> -	if (rc)
> -		return rc;
> +	if (!dev->reset_fn)
> +		return -ENOTTY;
>  
>  	pci_dev_lock(dev);
>  	pci_dev_save_and_disable(dev);
> @@ -4487,9 +4486,8 @@ int pci_reset_function_locked(struct pci_dev *dev)
>  {
>  	int rc;
>  
> -	rc = pci_probe_reset_function(dev);
> -	if (rc)
> -		return rc;
> +	if (!dev->reset_fn)
> +		return -ENOTTY;
>  
>  	pci_dev_save_and_disable(dev);
>  
> @@ -4511,9 +4509,8 @@ int pci_try_reset_function(struct pci_dev *dev)
>  {
>  	int rc;
>  
> -	rc = pci_probe_reset_function(dev);
> -	if (rc)
> -		return rc;
> +	if (!dev->reset_fn)
> +		return -ENOTTY;
>  
>  	if (!pci_dev_trylock(dev))
>  		return -EAGAIN;
> 
> 

Re: [PATCH v1 0/2] PCI: Probe for reset support earlier

[Bjorn Helgaas]

On Fri, Feb 16, 2018 at 04:50:03PM -0600, Bjorn Helgaas wrote:
> The PCI core currently uses a late_initcall to probe each device for
> whether it supports reset.  This is dangerous because a driver may have
> already claimed the device by this point, and the PCI core should not
> interfere with the driver by touching the device on its own.
> 
> These patches move the probe to be earlier, during enumeration, before a
> driver has a chance to claim the device.
> 
> ---
> 
> Bjorn Helgaas (2):
>       PCI: Probe for device reset support before driver claim
>       PCI: Remove redundant probes for device reset support
> 
> 
>  drivers/pci/pci-sysfs.c |    3 +--
>  drivers/pci/pci.c       |   15 ++++++---------
>  drivers/pci/probe.c     |    3 +++
>  3 files changed, 10 insertions(+), 11 deletions(-)

Applied both to pci/virtualization with Rafael's reviewed-by (and
update to comment to s/D3hot/D3/) for v4.17.