On Saturday, August 13, 2016 12:18:26 AM Rafael J. Wysocki wrote:
> 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@xxxxxxxxx> 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.
Of course, I guess you'll say that it may not be able to signal wakeup from
D3hot as well in that case, which is correct. :-)
So I guess the one-liner change in pci_target_state() would be fine if Bjorn
likes it.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
