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

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

> ---
>  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;
>  }
>  
>  /*
> 
>