Re: [PATCH v2 10/13] PCI: Avoid going from D3cold to D3hot for system sleep

[Lukas Wunner <lukas@wunner.de>]

On Thu, Aug 04, 2016 at 03:07:56AM +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 4, 2016 at 2:45 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Thu, Aug 04, 2016 at 01:50:39AM +0200, Rafael J. Wysocki wrote:
> >> On Wed, Aug 3, 2016 at 2:28 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >> > On Mon, Jul 18, 2016 at 03:39:15PM +0200, Rafael J. Wysocki wrote:
> >> >> On Saturday, June 18, 2016 12:14:07 AM Lukas Wunner wrote:
> >> >> > On Fri, Jun 17, 2016 at 04:09:24PM -0500, Bjorn Helgaas wrote:
> >> >> > > On Fri, May 13, 2016 at 01:15:31PM +0200, Lukas Wunner wrote:
> >> >> > > > There are devices wich are not power-managed by the platform, yet can be
> >> >> > > > runtime suspended to D3cold with some other mechanism.  When putting the
> >> >> > > > system to sleep, we currently handle such devices improperly by trying
> >> >> > > > to transition them from D3cold to D3hot (the default power state defined
> >> >> > > > at the beginning of pci_target_state()).  Avoid that.
> >> >> > > >
> >> >> > > > An example for devices affected by this are Thunderbolt controllers
> >> >> > > > built into Macs which can be put into D3cold with nonstandard ACPI
> >> >> > > > methods.
> >> >> > > >
> >> >> > > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> >> >> > > > ---
> >> >> > > >  drivers/pci/pci.c | 2 ++
> >> >> > > >  1 file changed, 2 insertions(+)
> >> >> > > >
> >> >> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> >> > > > index 791dfe7..6af9911 100644
> >> >> > > > --- a/drivers/pci/pci.c
> >> >> > > > +++ b/drivers/pci/pci.c
> >> >> > > > @@ -1943,6 +1943,8 @@ static pci_power_t pci_target_state(struct pci_dev *dev)
> >> >> > > >                               && !(dev->pme_support & (1 << target_state)))
> >> >> > > >                                 target_state--;
> >> >> > > >                 }
> >> >> > > > +       } else if (dev->current_state == PCI_D3cold) {
> >> >> > > > +               target_state = PCI_D3cold;
> >> >> > > >         }
> >> >> > >
> >> > I will update this patch with Bjorn's suggestion to also leave the
> >> > device in D3cold if it is wakeup-capable. The idea is to just change
> >> > the default state in the first line of the function like this:
> >> >
> >> > -       pci_power_t target_state = PCI_D3hot;
> >> > +       pci_power_t target_state =
> >> > +               dev->current_state == PCI_D3cold ? PCI_D3cold : PCI_D3hot;
> >>
> >> That should work (even though it is a little clumsy IMO).
> >
> > Not sure why that is clumsy but happy to use something else if you
> > have a suggestion?
> 
> The clumsy thing is that we'd take the target_state as D3cold only if
> the device already was in that state.
> 
> Otherwise, we'd take D3hot as the target state for the same device,
> which doesn't seem particularly consistent to me.
> 
> Not that I have better ideas ATM, but then the current code works for
> my use cases. :-)

The goal is to afford direct-complete to devices which are not power-
manageable by the platform but can still be runtime suspended to D3cold.
Right now we wake those devices up from D3cold to D3hot before going to
sleep, which is a waste of energy and prolongs the suspend sequence
(waking up the Thunderbolt controller takes 2 seconds).

The de facto standard to power manage such devices seems to be with
dev_pm_domain_set(). That's what vga_switcheroo does and I'll move
to that as well for v3 of this series.

I could add a "bool can_power_off" to struct dev_pm_domain.

Then I could change pci_target_state() like this:

 	pci_power_t target_state = PCI_D3hot;
 
 	if (platform_pci_power_manageable(dev)) {
 		[...]
+	} else if (dev->dev.pm_domain && dev->dev.pm_domain.can_power_off) {
+		target_state = PCI_D3cold;
 	} else if [...]

Another idea would be to add a ->choose_state hook to dev_pm_domain,
but that would have to return a PCI-specific power state, so we'd be
in clumsy territory again.

Thoughts?

Lukas