linux-kernel - Re: [PATCH v11 9/9] PCI: ACPI: Use device constraints to decide PCI target state fallback policy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Aug 2023 14:13:34 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     "Limonciello, Mario" <mario.limonciello@....com>
Cc:     Bjorn Helgaas <helgaas@...nel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-acpi@...r.kernel.org,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Iain Lane <iain@...ngesquash.org.uk>,
        Shyam-sundar S-k <Shyam-sundar.S-k@....com>
Subject: Re: [PATCH v11 9/9] PCI: ACPI: Use device constraints to decide PCI
 target state fallback policy

On Thu, Aug 17, 2023 at 3:26 AM Limonciello, Mario
<mario.limonciello@....com> wrote:
>
>
>
> On 8/16/2023 5:38 PM, Bjorn Helgaas wrote:
> > [I see that you just posted a v12 that doesn't touch drivers/pci at
> > all.  I haven't looked at it yet, so maybe my questions/comments below
> > are no longer relevant.]
>
> I'm not married to either approach but I think that you'll like the v12
> approach better.
>
> Let me try to answer your questions anyway though because I think
> they're still applicable for understanding of this issue.
>
> >
> > On Wed, Aug 16, 2023 at 07:57:52AM -0500, Limonciello, Mario wrote:
> >> On 8/15/2023 6:48 PM, Bjorn Helgaas wrote:
> >>> On Wed, Aug 09, 2023 at 01:54:53PM -0500, Mario Limonciello wrote:
> >>>> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> >>>> PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
> >>>> storing a value to the `bridge_d3` variable in the `struct pci_dev`
> >>>> structure.
> >>>>
> >>>> pci_power_manageable() uses this variable to indicate a PCIe port can
> >>>> enter D3.
> >>>> pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
> >>>> decide whether to try to put a device into its target state for a sleep
> >>>> cycle via pci_prepare_to_sleep().
> >>>>
> >>>> For devices that support D3, the target state is selected by this policy:
> >>>> 1. If platform_pci_power_manageable():
> >>>>      Use platform_pci_choose_state()
> >>>> 2. If the device is armed for wakeup:
> >>>>      Select the deepest D-state that supports a PME.
> >>>> 3. Else:
> >>>>      Use D3hot.
> >>>>
> >>>> Devices are considered power manageable by the platform when they have
> >>>> one or more objects described in the table in section 7.3 of the ACPI 6.5
> >>>> specification.
> >>>>
> >>>> When devices are not considered power manageable; specs are ambiguous as
> >>>> to what should happen.  In this situation Windows 11 leaves PCIe
> >>>> ports in D0 while Linux puts them into D3 due to the above mentioned
> >>>> commit.
> >>>
> >>> Why would we not use the same policy as Windows 11?
> >>
> >> That's what I'm aiming to do with my patch series.
> >
> > OK, help me out because I think I have a hint of how this works, but
> > I'm still really confused.  Here's the sort of commit log I envision
> > (but I know it's all wrong, so help me out):
>
> I was intentionally trying to leave the actual problem out of the commit
> from your earlier feedback and just put it in the cover letter.
>
> But if it's better to keep in the commit message I'll return those details.

It is.

If you make a change in order to address a specific problem, that
problem needs to be described in the changelog of the patch making
that change.

Anything else is more or less confusing IMO.