Re: [PATCH v2 2/2] PCI: Add missing link delays required by the PCIe spec

[Bjorn Helgaas]

On Tue, Nov 05, 2019 at 02:58:18PM +0200, Mika Westerberg wrote:
> On Tue, Nov 05, 2019 at 11:54:33AM +0200, Mika Westerberg wrote:
> > > For understandability, I think the wait needs to go in some function
> > > that contains "PCI_D0", e.g., platform_pci_set_power_state() or
> > > pci_power_up(), so it's connected with the transition from D3cold to
> > > D0.
> > > 
> > > Since pci_pm_default_resume_early() is the only caller of
> > > pci_power_up(), maybe we should just inline pci_power_up(), e.g.,
> > > something like this:
> > > 
> > >   static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
> > >   {
> > >     pci_power_state_t prev_state = pci_dev->current_state;
> > > 
> > >     if (platform_pci_power_manageable(pci_dev))
> > >       platform_pci_set_power_state(pci_dev, PCI_D0);
> > > 
> > >     pci_raw_set_power_state(pci_dev, PCI_D0);
> > >     pci_update_current_state(pci_dev, PCI_D0);
> > > 
> > >     pci_restore_state(pci_dev);
> > >     pci_pme_restore(pci_dev);
> > > 
> > >     if (prev_state == PCI_D3cold)
> > >       pci_bridge_wait_for_secondary_bus(dev);
> > >   }
> > 
> > OK, I'll see if this works.
> 
> Well, I think we want to execute pci_fixup_resume_early before we delay
> for the downstream component (same for runtime resume path). Currently
> nobody is using that for root/downstream ports but in theory at least
> some port may need it before it works properly. Also probably good idea
> to disable wake as soon as possible to avoid possible extra PME messages
> coming from the port.

OK, I wish we could connect it more closely with the actual power-on,
but I guess that makes sense.

> I feel that the following is the right place to perform the delay but if
> you think we can ignore the above, I will just do what you say and do it
> in pci_pm_default_resume_early() (and pci_restore_standard_config() for
> runtime path).
> 
> [The below diff does not have check for pci_dev->skip_bus_pm because I
>  was planning to move it inside pci_bridge_wait_for_secondary_bus() itself.]

What do you gain by moving it?  IIUC we want them to be the same
condition, and if one is in pci_pm_resume_noirq() and another is
inside pci_bridge_wait_for_secondary_bus(), it's hard to see that
they're connected.  I'd rather have pci_pm_resume_noirq() check it
once, save the result, and test that result before calling
pci_pm_default_resume_early() and pci_bridge_wait_for_secondary_bus().

> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 08d3bdbc8c04..3c0e52aaef79 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -890,6 +890,7 @@ static int pci_pm_resume_noirq(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +	pci_power_t prev_state = pci_dev->current_state;
>  
>  	if (dev_pm_may_skip_resume(dev))
>  		return 0;
> @@ -914,6 +915,9 @@ static int pci_pm_resume_noirq(struct device *dev)
>  	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  	pcie_pme_root_status_cleanup(pci_dev);
>  
> +	if (prev_state == PCI_D3cold)
> +		pci_bridge_wait_for_secondary_bus(pci_dev);
> +
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return 0;
>  
> @@ -1299,6 +1303,7 @@ static int pci_pm_runtime_resume(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +	pci_power_t prev_state = pci_dev->current_state;
>  	int error = 0;
>  
>  	/*
> @@ -1314,6 +1319,9 @@ static int pci_pm_runtime_resume(struct device *dev)
>  	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  	pci_pm_default_resume(pci_dev);
>  
> +	if (prev_state == PCI_D3cold)
> +		pci_bridge_wait_for_secondary_bus(pci_dev);
> +
>  	if (pm && pm->runtime_resume)
>  		error = pm->runtime_resume(dev);
>