Re: [PATCH] PCI: Add CRS timeout for pci_device_is_present()

[Sinan Kaya <okaya@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1696 bytes --]

+Rafael

On 10/15/2019 2:30 AM, Thierry Reding wrote:
> Vidya, can you clarify for which device you're seeing the issues? Sounds
> like adding a call to pci_pm_reset() (via pci_reset_function()) at some
> point.
> 
> Sinan, it looks as if pci_pm_reset() (or any of its callers) is not used
> very widely. Is that just because most drivers haven't had a need for it
> yet? Or am I missing some core functionality that calls this for every
> device anyway?

pci_pm_reset() is there as an alternative reset path. We are not
supposed to call this function. Sorry for giving you wrong direction
here. pci_reset_function() should call it only if there is no other
suitable reset function is found.

I think the PCI core should be putting the device back D0 state as one
of the first actions before enumerating. Wake up could be a combination
of ACPI and/or PCI wake up depending on where your device sits in the
topology.

On the other hand, wake up code doesn't perform the CRS wait. CRS
wait is deferred until the first vendor id read in pci_scan_device().
I see that it already waits for 60 seconds.

Going back to the patch...

I think we need to find the path that actually needs this sleep and
put pci_dev_wait() there.

+++ b/drivers/pci/pci.c
@@ -5905,7 +5905,8 @@ bool pci_device_is_present(struct pci_dev *pdev)

 	if (pci_dev_is_disconnected(pdev))
 		return false;
-	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
+	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v,
+					  PCI_CRS_TIMEOUT);
 }

pci_device_is_present() is a too low-level function and it may not
be allowed to sleep. It uses 0 as timeout value.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]