On Thu, Sep 14, 2023 at 5:33 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Thu, Sep 14, 2023 at 04:53:32PM +0200, Lukas Wunner wrote: > > On Thu, Sep 14, 2023 at 09:31:38AM -0500, Mario Limonciello wrote: > > > On 9/14/2023 09:17, Lukas Wunner wrote: > > > > On Wed, Sep 13, 2023 at 11:36:49AM -0500, Mario Limonciello wrote: > > > > > On 9/13/2023 09:31, Lukas Wunner wrote: > > > > > > If this only affects system sleep, not runtime PM, what you can do is > > > > > > define a DECLARE_PCI_FIXUP_SUSPEND_LATE() which calls pci_d3cold_disable() > > > > > > and also define a DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY() which calls > > > > > > pci_d3cold_enable(). > > > > > > > > > > > > And I think you can make those calls conditional on pm_suspend_no_platform() > > > > > > to constrain to s2idle. > > > > > > > > > > > > User space should still be able to influence runtime PM via the > > > > > > d3cold_allowed flag (unless I'm missing something). > > > > > > > > > > The part you're missing is that D3hot is affected by this issue too, > > > > > otherwise it would be a good proposal. > > > > > > > > I recall that in an earlier version of the patch, you solved the issue > > > > by amending pci_bridge_d3_possible(). > > > > > > > > Changing the dev->no_d3cold flag indirectly influences the bridge_d3 > > > > flag (through pci_dev_check_d3cold() and pci_bridge_d3_update()). > > > > > > > > If dev->no_d3cold is set on a device below a port, that port is > > > > prevented from entring D3hot because it would result in the > > > > device effectively being in D3cold. > > > > > > > > So you might want to take a closer look at this approach despite > > > > the flag suggesting that it only influences D3cold. > > > > > > Ah; I hadn't considered setting it on a device below the port. In this > > > particular situation the only devices below the root port are USB > > > controllers. > > > > > > If those devices don't go into D3 the system can't enter hardware sleep. > > > > If you set dev->no_d3cold on the USB controllers, they should still > > be able to go to D3hot, but not D3cold, which perhaps might be sufficient. > > It should prevent D3cold *and* D3hot on the Root Port above. And if you > > set that on system sleep in a quirk and clear it on resume, runtime PM > > shouldn't be affected. > > dev->no_d3cold appears to be mainly an administrative policy knob > twidded via sysfs. > > There *are* a few cases where drivers (i915, nouveau, xhci) update it > via pci_d3cold_enable() or pci_d3cold_disable(), but they all look > vulnerable to issues if people use the sysfs knob, and I'm a little > dubious that they're legit in the first place. > > This AMD Root Port issue is not an administrative choice; it's purely > a functional problem of the device advertising that it supports PME# > but not actually being able to do it. So if we can do this by fixing > dev->pme_support (i.e., the copy of what it advertised), I'd rather do > that. Besides, it is not really necessary to prevent D3hot on the Root Port in question in all cases. It can go into D3 just fine if there are no wakeup devices under it and I suppose that the platform can achieve more energy savings (over the case when the port is always held in D0).
