Re: [PATCH v18 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Lukas Wunner <lukas@wunner.de>]

On Thu, Sep 14, 2023 at 09:04:29PM +0200, Lukas Wunner wrote:
> On Thu, Sep 14, 2023 at 10:33:03AM -0500, Bjorn Helgaas wrote:
> > dev->no_d3cold appears to be mainly an administrative policy knob
> > twidded via sysfs.
> 
> Actually the user space choice to disable D3cold is stored in a
> different flag called pdev->d3cold_allowed.
> 
> The fact that d3cold_allowed_store() indirectly modifies the
> no_d3cold flag as well looks like a bug that went unnoticed
> for a couple of years.  From a quick look, d3cold_allowed_store()
> should probably call pci_bridge_d3_update() instead of
> pci_d3cold_enable() / pci_d3cold_disable().  This was introduced by
> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend").
> Perhaps Mika can chime in whether this is indeed wrong.

Note that pci_dev_check_d3cold() checks both the no_d3cold flag
(which tells whether the *driver* disabled D3cold) and the d3cold_allowed
flag (which tells whether *user space* disabled D3cold).

Basically right now we allow user space to override the driver setting,
which feels unsafe.  (Does user space always know better than the driver
whether D3cold can safely be entered?  I don't think so.)

Thanks,

Lukas