On 9/18/2023 08:24, Lukas Wunner wrote:
On Mon, Sep 18, 2023 at 08:14:21AM -0500, Mario Limonciello wrote:
On 9/18/2023 08:07, Mika Westerberg wrote:
On Mon, Sep 18, 2023 at 02:48:01PM +0200, Lukas Wunner wrote:
struct pci_dev contains two flags which govern whether the device may
suspend to D3cold:
* no_d3cold provides an opt-out for drivers (e.g. if a device is known
to not wake from D3cold)
* d3cold_allowed provides an opt-out for user space (default is true,
user space may set to false)
Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend"),
the user space setting overwrites the driver setting. Essentially user
space is trusted to know better than the driver whether D3cold is
working.
That feels unsafe and wrong. Assume that the change was introduced
inadvertently and do not overwrite no_d3cold when d3cold_allowed is
modified. Instead, consider d3cold_allowed in addition to no_d3cold
when choosing a suspend state for the device.
That way, user space may opt out of D3cold if the driver hasn't, but it
may no longer force an opt in if the driver has opted out.
Makes sense. I just wonder should the sysfs write fail from userspace
perspective if the driver has opted out and userspace tries to force it?
Or it does that already?
What's the history behind why userspace is allowed to opt a device out of
D3cold in the first place?
It feels like it should have been a debugging only thing to me.
That's a fair question.
Apparently the default for d3cold_allowed was originally "false"
and user space could opt in to D3cold. Then commit 4f9c1397e2e8
("PCI/PM: Enable D3/D3cold by default for most devices") changed
the default to "true". That was 11 years ago.
I agree that today this should all work automatically and a
user space option to disable D3cold on a per-device basis only
really makes sense as a debugging aid, hence belongs in debugfs.
Thanks. Then perhaps as part of moving it to debugfs it makes sense to
simplify the logic.
IE also drop the d3cold_allowed member from struct pci_dev and instead
make the debugfs item reflect the "no_d3cold" member.
