Re: [PATCH] PCI/sysfs: Protect driver's D3cold preference from user space

[Lukas Wunner <lukas@wunner.de>]

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,

Lukas