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

[Vidya Sagar <vidyas@nvidia.com>]

On 11/6/2019 10:11 PM, Bjorn Helgaas wrote:
> On Tue, Nov 05, 2019 at 11:55:45AM +0100, Rafael J. Wysocki wrote:
>> On Monday, November 4, 2019 6:39:04 PM CET Bjorn Helgaas wrote:
>>> [+cc Andrew, Lukas]
>>>
>>> On Tue, Oct 15, 2019 at 05:44:47PM +0530, Vidya Sagar wrote:
>>>> On 10/15/2019 4:40 PM, Sinan Kaya wrote:
>>>>> ...
>>>>> 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.
>>>>
>>>> Yup. It is indeed doing it as part of pci_power_up() in pci.c file.
>>>> But, what is confusing to me is the order of the calls.
>>>> pci_power_up() has following calls in the same order.
>>>> 	pci_raw_set_power_state(dev, PCI_D0);
>>>> 	pci_update_current_state(dev, PCI_D0);
>>>> But, pci_raw_set_power_state() is accessing config space without calling
>>>> pci_device_is_present() whereas pci_update_current_state() which is called
>>>> later in the flow is calling pci_device_is_present()...!
>>>
>>> A device should always respond to config reads unless it is in D3cold
>>> or it is initializing after a reset.  IIUC you're doing a resume, not
>>> a reset, so I think your device must be coming out of D3cold.  That's
>>> typically done via ACPI, and I think we are missing some kind of delay
>>> before our first config access:
>>>
>>>    pci_power_up
>>>      platform_pci_set_power_state(PCI_D0)    # eg, ACPI
>>>      pci_raw_set_power_state
>>>        pci_read_config_word(PCI_PM_CTRL)     # <-- first config access
>>>        pci_write_config_word(PCI_PM_CTRL)
>>>        pci_read_config_word(PCI_PM_CTRL)
>>>      pci_update_current_state
>>>        if (... || !pci_device_is_present())
>>>
>>> Mika is working on some delays for the transition out of D3cold [1].
>>> He's more concerned with a secondary bus behind a bridge, so I don't
>>> think his patch addresses this case, but he's certainly familiar with
>>> this area.
>>>
>>> Huh, I'm really confused about this, too.  I don't
>>> understand how resume ever works without any delay between
>>> platform_pci_power_manageable() and the config reads in
>>> pci_raw_set_power_state().  I must be missing something.
>>
>> There is a delay in the runtime_d3cold case, see
>> __pci_start_power_transition().
> 
> I see the delay in __pci_start_power_transition(), but I don't see how
> it's relevant.  It's only called by pci_set_power_state(), and while
> many drivers call pci_set_power_state() from legacy .resume() methods,
> the pci_pm_resume_noirq() path where Vidya is seeing problems doesn't
> use it.
> 
>> But overall platform_pci_power_manageable() only checks whether or
>> not the platform firmware can change the power state of the device.
>> If it can, it is expected to take care of any necessary delays while
>> doing that (because there may be delays required by this particular
>> instance of the platform firmware, beyond what is mandated by the
>> PCI spec, or there may not be any need to wait at all). ...
> 
> That sounds like a reasonable argument for why firmware should be
> responsible for this delay, but I don't think that's very clear in the
> ACPI spec, so I wouldn't be surprised if it got missed.
> 
> Based on Vidya's backtrace, I think the resume path with problems is
> this:
> 
>    pci_pm_resume_noirq
>      pci_pm_default_resume_early
>        pci_power_up
>          if (platform_pci_power_manageable(dev))
>            platform_pci_set_power_state(dev, PCI_D0)  # <-- FW delay here?
>          pci_raw_set_power_state
>          pci_update_current_state
>            pci_device_is_present        # <-- config read returns CRS
> 
> So I think your suggestion is that Vidya's firmware should be doing
> the delay inside platform_pci_set_power_state()?
> 
> Vidya, you typically work on Tegra, so I assume this is on an arm64
> system?  Does it have ACPI?  Do you have access to the firmware
> developers to ask about who they expect to do the delays?
Yes. This is on arm64 (Tegra) and we don't have any ACPI or any other firmware
for that matter. PCIe is brought up directly in the kernel.

> 
>> In any case, I'm not sure how useful it is to add delays for
>> everyone in the cases in which a specific system needs a delay
>> because of its own PM implementation limitations.  It may be better
>> to quirk such systems explicitly as long as there are not too many
>> quirks in there, or we'll end up adding more and more *implicit*
>> quirks in the form of general delays.
> 
> I agree, a general delay doesn't sound good.  Are you thinking
> something like this?
> 
>    void pci_power_up(struct pci_dev *dev)
>    {
>      if (platform_pci_power_manageable(dev)) {
>        platform_pci_set_power_state(dev, PCI_D0);
>        if (dev->XXX)
>          msleep(dev->XXX);
>      }
>      ...
> 
> We already have dev->d3_delay and d3cold_delay, so it's getting a bit
> messy to keep them all straight.
> 
> Bjorn
>