| Message ID | 20230818194027.27559-5-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Use LPS0 constraints to opt devices into D3 | expand |
On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > Intel systems that need to have PCIe ports in D3 for low power idle > specify this by constraints on the ACPI PNP0D80 device. As this information > is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop > at 2024. This will allow future systems to rely on the constraints check > and ACPI checks to set up policy like non-Intel systems do. So I'm not sure about the value of this change. The behavior is made Intel-specific in [14a 1/1] and it will be that way at least for some time. This change only sets the date after which it won't be Intel-specific any more, but for what reason exactly? And why is 2024 the cut-off year (and not 2025, for example)? If Intel platforms continue to be OK with putting all PCIe ports into D3hot beyond 2024, why restrict the kernel from doing so on them? > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v13->v14: > * Use a variable instead > v12->v13: > * New patch > --- > drivers/pci/pci.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index bfdad2eb36d13..c8787d898c377 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3037,16 +3037,22 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) > return false; > > /* > - * Allow Intel PCIe ports from 2015 onward to go into D3 to > + * Allow Intel PCIe ports from 2015 to 2024 to go into D3 to > * achieve additional energy conservation on some platforms. > * > + * Intel systems from 2025 onward that need this are expected > + * to specify this in an LPS0 device constraint instead. > + * > * This is only set for Intel PCIe ports as it causes problems > * on both AMD Rembrandt and Phoenix platforms where USB keyboards > * can not be used to wake the system from suspend. > */ > - if (bridge->vendor == PCI_VENDOR_ID_INTEL && > - dmi_get_bios_year() >= 2015) > - return true; > + if (bridge->vendor == PCI_VENDOR_ID_INTEL) { > + int year = dmi_get_bios_year(); > + > + if (year >= 2015 && year <= 2024) > + return true; > + } > break; > } > > -- > 2.34.1 >
On 8/21/2023 1:46 PM, Rafael J. Wysocki wrote: > On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> >> Intel systems that need to have PCIe ports in D3 for low power idle >> specify this by constraints on the ACPI PNP0D80 device. As this information >> is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop >> at 2024. This will allow future systems to rely on the constraints check >> and ACPI checks to set up policy like non-Intel systems do. > > So I'm not sure about the value of this change. > > The behavior is made Intel-specific in [14a 1/1] and it will be that > way at least for some time. This change only sets the date after > which it won't be Intel-specific any more, but for what reason > exactly? > > And why is 2024 the cut-off year (and not 2025, for example)? No particular reason other than it's a few kernel cycles to get this tested and working or revert it if it's a bad idea after all. > > If Intel platforms continue to be OK with putting all PCIe ports into > D3hot beyond 2024, why restrict the kernel from doing so on them? OK let me try to explain my thought process. The reason that root ports were put into D3 on Intel systems was that it's required for the system to get into the deepest state. At the time that it was introduced there wasn't a way for the firmware to express this desire for root ports that were not power manageable by ACPI. Constraints are a good way to express it, so by limiting the Intel hardcode to a number of years gets everyone onto the same codepaths. But that being said - if you would rather keep Intel as hardcode forever this patch can be dropped from the series. > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v13->v14: >> * Use a variable instead >> v12->v13: >> * New patch >> --- >> drivers/pci/pci.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index bfdad2eb36d13..c8787d898c377 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -3037,16 +3037,22 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) >> return false; >> >> /* >> - * Allow Intel PCIe ports from 2015 onward to go into D3 to >> + * Allow Intel PCIe ports from 2015 to 2024 to go into D3 to >> * achieve additional energy conservation on some platforms. >> * >> + * Intel systems from 2025 onward that need this are expected >> + * to specify this in an LPS0 device constraint instead. >> + * >> * This is only set for Intel PCIe ports as it causes problems >> * on both AMD Rembrandt and Phoenix platforms where USB keyboards >> * can not be used to wake the system from suspend. >> */ >> - if (bridge->vendor == PCI_VENDOR_ID_INTEL && >> - dmi_get_bios_year() >= 2015) >> - return true; >> + if (bridge->vendor == PCI_VENDOR_ID_INTEL) { >> + int year = dmi_get_bios_year(); >> + >> + if (year >= 2015 && year <= 2024) >> + return true; >> + } >> break; >> } >> >> -- >> 2.34.1 >>
On Mon, Aug 21, 2023 at 9:18 PM Limonciello, Mario <mario.limonciello@amd.com> wrote: > > > > On 8/21/2023 1:46 PM, Rafael J. Wysocki wrote: > > On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello > > <mario.limonciello@amd.com> wrote: > >> > >> Intel systems that need to have PCIe ports in D3 for low power idle > >> specify this by constraints on the ACPI PNP0D80 device. As this information > >> is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop > >> at 2024. This will allow future systems to rely on the constraints check > >> and ACPI checks to set up policy like non-Intel systems do. > > > > So I'm not sure about the value of this change. > > > > The behavior is made Intel-specific in [14a 1/1] and it will be that > > way at least for some time. This change only sets the date after > > which it won't be Intel-specific any more, but for what reason > > exactly? > > > > And why is 2024 the cut-off year (and not 2025, for example)? > > No particular reason other than it's a few kernel cycles to get this > tested and working or revert it if it's a bad idea after all. > > > > > If Intel platforms continue to be OK with putting all PCIe ports into > > D3hot beyond 2024, why restrict the kernel from doing so on them? > > OK let me try to explain my thought process. > > The reason that root ports were put into D3 on Intel systems was that > it's required for the system to get into the deepest state. > > At the time that it was introduced there wasn't a way for the firmware > to express this desire for root ports that were not power manageable by > ACPI. > > Constraints are a good way to express it, so by limiting the Intel > hardcode to a number of years gets everyone onto the same codepaths. Assuming that the will be used in future systems, but that is beyond the control of anyone involved here I think. > But that being said - if you would rather keep Intel as hardcode forever > this patch can be dropped from the series. This change can be made at any time and I don't see a particular reason for making it right now.
On 8/21/2023 2:24 PM, Rafael J. Wysocki wrote: > On Mon, Aug 21, 2023 at 9:18 PM Limonciello, Mario > <mario.limonciello@amd.com> wrote: >> >> >> >> On 8/21/2023 1:46 PM, Rafael J. Wysocki wrote: >>> On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello >>> <mario.limonciello@amd.com> wrote: >>>> >>>> Intel systems that need to have PCIe ports in D3 for low power idle >>>> specify this by constraints on the ACPI PNP0D80 device. As this information >>>> is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop >>>> at 2024. This will allow future systems to rely on the constraints check >>>> and ACPI checks to set up policy like non-Intel systems do. >>> >>> So I'm not sure about the value of this change. >>> >>> The behavior is made Intel-specific in [14a 1/1] and it will be that >>> way at least for some time. This change only sets the date after >>> which it won't be Intel-specific any more, but for what reason >>> exactly? >>> >>> And why is 2024 the cut-off year (and not 2025, for example)? >> >> No particular reason other than it's a few kernel cycles to get this >> tested and working or revert it if it's a bad idea after all. >> >>> >>> If Intel platforms continue to be OK with putting all PCIe ports into >>> D3hot beyond 2024, why restrict the kernel from doing so on them? >> >> OK let me try to explain my thought process. >> >> The reason that root ports were put into D3 on Intel systems was that >> it's required for the system to get into the deepest state. >> >> At the time that it was introduced there wasn't a way for the firmware >> to express this desire for root ports that were not power manageable by >> ACPI. >> >> Constraints are a good way to express it, so by limiting the Intel >> hardcode to a number of years gets everyone onto the same codepaths. > > Assuming that the will be used in future systems, but that is beyond > the control of anyone involved here I think. > >> But that being said - if you would rather keep Intel as hardcode forever >> this patch can be dropped from the series. > > This change can be made at any time and I don't see a particular > reason for making it right now. OK, then after Bjorn reviews the other patches of 14.a and 14.c I'll drop this patch. Thanks!
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index bfdad2eb36d13..c8787d898c377 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3037,16 +3037,22 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) return false; /* - * Allow Intel PCIe ports from 2015 onward to go into D3 to + * Allow Intel PCIe ports from 2015 to 2024 to go into D3 to * achieve additional energy conservation on some platforms. * + * Intel systems from 2025 onward that need this are expected + * to specify this in an LPS0 device constraint instead. + * * This is only set for Intel PCIe ports as it causes problems * on both AMD Rembrandt and Phoenix platforms where USB keyboards * can not be used to wake the system from suspend. */ - if (bridge->vendor == PCI_VENDOR_ID_INTEL && - dmi_get_bios_year() >= 2015) - return true; + if (bridge->vendor == PCI_VENDOR_ID_INTEL) { + int year = dmi_get_bios_year(); + + if (year >= 2015 && year <= 2024) + return true; + } break; }
Intel systems that need to have PCIe ports in D3 for low power idle specify this by constraints on the ACPI PNP0D80 device. As this information is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop at 2024. This will allow future systems to rely on the constraints check and ACPI checks to set up policy like non-Intel systems do. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v13->v14: * Use a variable instead v12->v13: * New patch --- drivers/pci/pci.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)