Re: [PATCH -v4 2/2] PCIe: Add PCIe runtime D3cold support

[Huang Ying <ying.huang@intel.com>]

On Wed, 2012-06-06 at 15:52 +0200, Rafael J. Wysocki wrote:
> On Tuesday, June 05, 2012, Huang Ying wrote:
> > On Sat, 2012-06-02 at 01:10 +0200, Rafael J. Wysocki wrote:
> > > On Friday, June 01, 2012, Huang Ying wrote:
> > > > On Thu, 2012-05-31 at 21:01 +0200, Rafael J. Wysocki wrote:
> > > > > On Thursday, May 31, 2012, Huang Ying wrote:
> > > > > > On Wed, 2012-05-30 at 23:49 +0200, Rafael J. Wysocki wrote:
> > > > > > > On Tuesday, May 22, 2012, Rafael J. Wysocki wrote:
> > > > > > > > On Friday, May 18, 2012, Huang Ying wrote:
[snip] 
> > > > > Besides, this hunk of the $subject patch:
> > > > > 
> > > > > > @@ -731,8 +791,8 @@ int pci_set_power_state(struct pci_dev *
> > > > > >       int error;
> > > > > >  
> > > > > >       /* bound the state we're entering */
> > > > > > -     if (state > PCI_D3hot)
> > > > > > -             state = PCI_D3hot;
> > > > > > +     if (state > PCI_D3cold)
> > > > > > +             state = PCI_D3cold;
> > > > > >       else if (state < PCI_D0)
> > > > > >               state = PCI_D0;
> > > > > >       else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))
> > > > > > @@ -747,10 +807,15 @@ int pci_set_power_state(struct pci_dev *
> > > > > >  
> > > > > >       /* This device is quirked not to be put into D3, so
> > > > > >          don't put it in D3 */
> > > > > > -     if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
> > > > > > +     if (state >= PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
> > > > > >               return 0;
> > > > > >  
> > > > > > -     error = pci_raw_set_power_state(dev, state);
> > > > > > +     /*
> > > > > > +      * To put device in D3cold, we put device into D3hot in native
> > > > > > +      * way, then put device into D3cold with platform ops
> > > > > > +      */
> > > > > > +     error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
> > > > > > +                                     PCI_D3hot : state);
> > > > > >  
> > > > > >       if (!__pci_complete_power_transition(dev, state))
> > > > > >               error = 0;
> > > > > 
> > > > > should be merged separately, because it will affect suspend/hibernation code
> > > > > paths.  Namely, it will change the behavior in such a way that some devices
> > > > > put into D3hot previously will be put into D3cold now during system suspend.
> > > > 
> > > > Yes.  This patch enables both runtime D3cold and D3cold during system
> > > > suspend.  How about separate this patch into the following patches?
> > > > 
> > > > - Add d3cold disable logic, including flags: no_d3cold, d3cold_allowed,
> > > > runtime_d3cold, and disable runtime d3cold (because part of runtime
> > > > d3cold support will be enabled by system d3cold support).
> > > > 
> > > > - system d3cold support for PCIe port
> > > > 
> > > > - system d3cold support in PCI core
> > > > 
> > > > - runtime d3cold support for PCIe port
> > > > 
> > > > - runtime d3cold support in PCI core
> > > 
> > > Sounds good in principle.
> > 
> > Thought it again.
> > 
> > If my understanding were correct, in most cases, The value to put
> > devices into D3cold during system suspend/hibernate may be questionable.
> > Because after Linux put the devices into lower power state,  the
> > firmware may put devices into D3cold state before entering system
> > suspend state.
> > 
> > So, I think maybe we can just constrain the target state to D3hot in
> > pci_set_power_state() if system suspend/hibernate is ongoing
> > (dev->runtime_d3cold is not set).  That is something as follow:
> > 
> > /* D3cold during system suspend/hibernate is not supported yet */
> > if (dev->runtime_d3cold && state >= PCI_D3cold)
> > 	state = PCI_D3cold;
> > else if (state > PCI_D3hot)
> > 	state = PCI_D3hot;
> > 
> > This way, we can just make some minor change to this patch and maybe
> > work on D3cold during system suspend/hibernate after some discussion.
> 
> I don't honestly think we should add such ugly checks to pci_set_power_state().
> 
> Why don't we ensure that acpi_pm_device_sleep_state() doesn't return 4
> (D3cold) if acpi_target_sleep_state is different from 0 instead?

We can add that into acpi_target_sleep_state.  But pci_set_power_sate()
is public API and used by many places already, do a quick search in
recent kernel source.

$ grep 'pci_set_power_state' -r . | grep -v PCI_D3hot | grep -v PCI_D0 | grep -v pci_choose_state | wc -l

yields:
55

$ grep 'pci_set_power_state' -r . | grep -v PCI_D3hot | grep -v PCI_D0 | grep -v pci_choose_state | grep -i d3cold

yields:
./drivers/misc/cb710/core.c:            pci_set_power_state(pdev, PCI_D3cold);
./arch/x86/pci/mrst.c:  pci_set_power_state(dev, PCI_D3cold);

Although we can check/fix them one by one.  We need to prevent future
pci_set_power_state to accept PCI_D3cold in some situation.

So I think we need to constrain the parameter of pci_set_power_state
anyway.  Maybe something like below in pci_set_power_state is better
than my previous one.

/* D3cold during system suspend/hibernate is not supported yet */
if (!dev->runtime_d3cold && state > PCI_D3hot)
	state = PCI_D3hot;

if (state > PCI_D3cold)
	state = PCI_D3cold;

Best Regards,
Huang Ying