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

[Thierry Reding <treding@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]

On Mon, Oct 14, 2019 at 01:21:31PM -0700, Sinan Kaya wrote:
> On 10/14/2019 1:20 AM, Thierry Reding wrote:
> > I think this makes sense, so:
> > 
> > Reviewed-by: Thierry Reding <treding@nvidia.com>
> > 
> > However, it looks like Sinan has researched this extensively in the past
> > and gave a presentation on this at Plumbers in 2017:
> > 
> > 	https://blog.linuxplumbersconf.org/2017/ocw/system/presentations/4732/original/crs.pdf
> > 
> > Adding Sinan to see if he has any concerns about this, since resume time
> > is explicitly mentioned in the above slides.
> 
> 
> Thanks for including me. Let me catch up here.
> 
> pci_dev_wait() is supposed to handle this case via pci_pm_reset().
> 
> /**
>  * pci_pm_reset - Put device into PCI_D3 and back into PCI_D0.
>  * @dev: Device to reset.
>  * @probe: If set, only check if the device can be reset this way.
>  */
> 
> Do you know if your execution path hits this function? We might have
> missed a use case.
> 

I see only a couple of callers of pci_device_is_present() in the tree,
this being from next-20191015:

	$ git grep -n pci_device_is_present
	drivers/net/ethernet/broadcom/tg3.c:9070:       if (!pci_device_is_present(tp->pdev))drivers/net/ethernet/broadcom/tg3.c:11785:      if (pci_device_is_present(tp->pdev)) {
	drivers/net/ethernet/intel/igb/igb_main.c:8838: if (!pci_device_is_present(pdev))
	drivers/nvme/host/pci.c:2866:   if (!pci_device_is_present(pdev)) {
	drivers/pci/hotplug/acpiphp_glue.c:650:         alive = pci_device_is_present(dev);
	drivers/pci/pci.c:935:      !pci_device_is_present(dev)) {
	drivers/pci/pci.c:5902:bool pci_device_is_present(struct pci_dev *pdev)
	drivers/pci/pci.c:5910:EXPORT_SYMBOL_GPL(pci_device_is_present);
	drivers/thunderbolt/nhi.c:939:  if (!pci_device_is_present(pdev)) {
	include/linux/pci.h:1206:bool pci_device_is_present(struct pci_dev *pdev);

The NVME driver has the call in the ->remove() callback, so I don't
think it's relevant here. Both TG3 and IGB ethernet drivers seem to call
this during resume and so does Thunderbolt.

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?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]