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

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Friday, August 12, 2016 06:16:09 PM Lukas Wunner wrote:
> On Fri, Aug 12, 2016 at 02:50:04AM +0200, Rafael J. Wysocki wrote:
> > On Thu, Aug 11, 2016 at 3:20 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > So I would like to find a common ground and something you feel
> > > comfortable to ack. The problem I see with your suggested approach
> > > of subclassing struct dev_pm_domain in a struct pci_pm_domain is
> > > that I can easily envision Apple putting some custom methods in the
> > > DSDT to power a non-PCI device up and down. They're starting to use
> > > SPI and UART to attach devices in newer machines.
> > 
> > Those devices have no standard power state definitions.
> > 
> > The problem you have here really is PCI-specific, because you want to
> > use PCI PM along with the non-standard methods.
> 
> If I introduce a struct pci_pm_domain like you suggested, it would mean
> that *all* PCI devices using dev_pm_domain_set() have to be changed,
> else the container_of() wouldn't work. The resulting code bloat alone
> inhibits me from implementing this. Plus, it's a tripwire for anyone
> wishing to assign a dev_pm_domain to their PCI device.
> 
> > > Hence my suggestion to add a flag to struct dev_pm_domain, even
> > > though at the moment that flag would only be queried by the PCI core.
> > > I don't care if this is called can_power_off or power_manageable or
> > > whatever.
> > 
> > struct dev_pm_domain is way too generic for that though, as I'm sure
> > there are users of it where the can_power_off thing wouldn't make any
> > sense whatever.
> 
> That seems like a small tradeoff compared to introducing a struct
> pci_pm_domain.

I'm not going to apply any patches addding can_power_off or similar flags to
struct dev_pm_domain.

> If you dislike a can_power_off flag in struct dev_pm_domain, that only
> leaves the option to add a one-liner to pci_target_state(), unless I'm
> missing something.

I'm not sure why you are insisting on setting target_state to D3cold
before taking the platform_pci_power_manageable() branch.  Why don't
you simply rearrange the routine like

	pci_power_t target_state = PCI_D3hot;

	if (platform_pci_power_manageable(dev)) {
		...
		return target_state;
	}

	if (!dev->pm_cap)
		return PCI_D0;

	if (dev->current_state == PCI_D3cold)
		target_state = PCI_D3cold;

	if (device_may_wakeup(&dev->dev)) {
		...
	}

	return target_state;

And that would be fine by me.

That said I'm not sure why you want to use pci_target_state() so badly?

If you are going to use a PM domain, why do you still need that function?

> BTW there seems to be a contradiction in your statements on wakeup devices:
> 
> On Mon, Aug 08, 2016 at 01:32:54AM +0200, Rafael J. Wysocki wrote:
> > On Sunday, August 07, 2016 11:03:47 AM Lukas Wunner wrote:
> > > The reasoning is that going from D3cold to D3hot before system sleep
> > > just never makes sense, no matter if the device got there by standard
> > > or nonstandard means.
> >
> > That may not be true in theory.
> >
> > If this is a wakeup device, it may not be able to generate wakeup signals
> > from D3cold while the system is in the target system state, although it might
> > be able to generate those signals when the system is in S0 (in the ACPI case).
> 
> However earlier you 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:
> > > > Is there a reason you don't want to do this check for devices that
> > > > may wakeup?
> > >
> > > Fear of breaking things. It would mean that a device would be left in
> > > D3cold even though it may not be able to signal wakeup from that power
> > > state.
> >
> > Then it should not be put into D3_cold at run time too if it is wakeup-capable.
> 
> So on the one hand, you warn that a wakeup-capable device may have been
> put into D3cold at runtime but needs to be woken before system sleep
> because it might otherwise not be able to signal wakeup.

Yes, so specifically I'm concerned about the pci_target_state() invocation in
pci_dev_keep_suspended() which is done exactly for this purpose.

If you apply the "keep it in D3cold if already there" logic to that case, it
may lead to a wrong decision in theory.  Say the device is in D3cold and
platform_pci_choose_state() returns D1, but pci_no_d1d2() returns true,
the device will end up in D3cold, but it may not be able to signal wakeup
from that state after the system has been suspended.

> On the other hand you say that such devices should not be put into D3cold
> at runtime at all.
>
> Which one is it?

I said the latter under the assumption that the device would not be able to
signal wakeup from D3cold at all (including at run time).  I may have not
understand the context of your conversation with Bjorn correctly.

Thanks,
Rafael