linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Add support for drivers to decide bridge D3 policy
@ 2023-12-03  4:10 Mario Limonciello
  2023-12-03  4:10 ` [PATCH v2 1/4] PCI: Make d3cold_allowed sysfs attribute read only Mario Limonciello
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Mario Limonciello @ 2023-12-03  4:10 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rafael J . Wysocki, Hans de Goede, Shyam Sundar S K,
	open list:PCI SUBSYSTEM, open list:X86 PLATFORM DRIVERS,
	Ilpo Järvinen, Lukas Wunner, Kai-Heng Feng, linux-acpi,
	linux-pm, Mario Limonciello

The policy for whether PCI bridges are allowed to select D3 is dictated
by empirical results that are enumerated into pci_bridge_d3_possible().

In Windows this behaves differently in that Windows internal policy is
not used for devices when a power engine plugin driver provided by the
SOC vendor is installed.  This driver is used to decide the policy in
those cases.

This series implements a system that lets drivers register such a policy
control as well. It isn't activated for any SOCs by default.

This is heavily leveraged from the work in [1]

[1] https://lore.kernel.org/platform-driver-x86/20230906184354.45846-1-mario.limonciello@amd.com/

v1->v2:
 * Pick up tags
 * Rebase on v6.7-rc4

Mario Limonciello (4):
  PCI: Make d3cold_allowed sysfs attribute read only
  PCI: Refresh root ports in pci_bridge_d3_update()
  ACPI: x86: s2idle: Export symbol for fetching constraints for module
    use
  platform/x86/amd: pmc: Add support for using constraints to decide D3
    policy

 Documentation/ABI/testing/sysfs-bus-pci |  4 +-
 drivers/acpi/x86/s2idle.c               |  1 +
 drivers/pci/pci-acpi.c                  |  2 +-
 drivers/pci/pci-sysfs.c                 | 14 +-----
 drivers/pci/pci.c                       | 12 ++++--
 drivers/platform/x86/amd/pmc/pmc.c      | 57 +++++++++++++++++++++++++
 include/linux/pci.h                     |  1 -
 7 files changed, 72 insertions(+), 19 deletions(-)


base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a
-- 
2.34.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/4] PCI: Make d3cold_allowed sysfs attribute read only
  2023-12-03  4:10 [PATCH v2 0/4] Add support for drivers to decide bridge D3 policy Mario Limonciello
@ 2023-12-03  4:10 ` Mario Limonciello
  2023-12-03  4:10 ` [PATCH v2 2/4] PCI: Refresh root ports in pci_bridge_d3_update() Mario Limonciello
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Mario Limonciello @ 2023-12-03  4:10 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rafael J . Wysocki, Hans de Goede, Shyam Sundar S K,
	open list:PCI SUBSYSTEM, open list:X86 PLATFORM DRIVERS,
	Ilpo Järvinen, Lukas Wunner, Kai-Heng Feng, linux-acpi,
	linux-pm, Mario Limonciello, Rafael J. Wysocki

Before kernel 3.6 D3cold was considered "opt-in" for PCIe devices.
Userspace was able to opt PCIe devices into d3cold support by using
the `d3cold_allowed` sysfs attribute. The policy changed to default
to enabled with commit 4f9c1397e2e8 ("PCI/PM: Enable D3/D3cold by
default for most devices").  The sysfs file remains however and
can potentially allow userspace to prevent the SoC from getting into
the deepest sleep state on modern systems.

For debugging purposes `pcie_port_pm=` can be used to control whether
a PCI port will go into D3cold and runtime PM can be turned off by
`/sys/bus/pci/devices/*/power/control` on PCI end points.

Change the sysfs attribute to a noop that ignores the input when written
and shows a warning. Simplify the internal kernel logic to drop
`d3cold_allowed`.

Link: https://lore.kernel.org/linux-pci/20230918132424.GA11357@wunner.de/
Acked-by: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 Documentation/ABI/testing/sysfs-bus-pci |  4 ++--
 drivers/pci/pci-acpi.c                  |  2 +-
 drivers/pci/pci-sysfs.c                 | 14 ++------------
 drivers/pci/pci.c                       |  3 +--
 include/linux/pci.h                     |  1 -
 5 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index ecf47559f495..b5db141dfee6 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -283,8 +283,8 @@ Description:
 		device will never be put into D3Cold state.  If it is set, the
 		device may be put into D3Cold state if other requirements are
 		satisfied too.  Reading this attribute will show the current
-		value of d3cold_allowed bit.  Writing this attribute will set
-		the value of d3cold_allowed bit.
+		value of no_d3cold bit.
+		Writing to this attribute is deprecated and will do nothing.
 
 What:		/sys/bus/pci/devices/.../sriov_totalvfs
 Date:		November 2012
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 004575091596..cf108259aef6 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -911,7 +911,7 @@ pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
 {
 	int acpi_state, d_max;
 
-	if (pdev->no_d3cold || !pdev->d3cold_allowed)
+	if (pdev->no_d3cold)
 		d_max = ACPI_STATE_D3_HOT;
 	else
 		d_max = ACPI_STATE_D3_COLD;
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 2321fdfefd7d..084301729120 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -522,17 +522,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	unsigned long val;
-
-	if (kstrtoul(buf, 0, &val) < 0)
-		return -EINVAL;
-
-	pdev->d3cold_allowed = !!val;
-	pci_bridge_d3_update(pdev);
-
-	pm_runtime_resume(dev);
-
+	dev_warn_once(dev, "pci: writing to d3cold_allowed is deprecated\n");
 	return count;
 }
 
@@ -540,7 +530,7 @@ static ssize_t d3cold_allowed_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	return sysfs_emit(buf, "%u\n", pdev->d3cold_allowed);
+	return sysfs_emit(buf, "%u\n", !pdev->no_d3cold);
 }
 static DEVICE_ATTR_RW(d3cold_allowed);
 #endif
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 55bc3576a985..72505794cc72 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3069,7 +3069,7 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 	bool *d3cold_ok = data;
 
 	if (/* The device needs to be allowed to go D3cold ... */
-	    dev->no_d3cold || !dev->d3cold_allowed ||
+	    dev->no_d3cold ||
 
 	    /* ... and if it is wakeup capable to do so from D3cold. */
 	    (device_may_wakeup(&dev->dev) &&
@@ -3206,7 +3206,6 @@ void pci_pm_init(struct pci_dev *dev)
 	dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
 	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
 	dev->bridge_d3 = pci_bridge_d3_possible(dev);
-	dev->d3cold_allowed = true;
 
 	dev->d1_support = false;
 	dev->d2_support = false;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 60ca768bc867..2f4f6013c61a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -376,7 +376,6 @@ struct pci_dev {
 	unsigned int	no_d1d2:1;	/* D1 and D2 are forbidden */
 	unsigned int	no_d3cold:1;	/* D3cold is forbidden */
 	unsigned int	bridge_d3:1;	/* Allow D3 for bridge */
-	unsigned int	d3cold_allowed:1;	/* D3cold is allowed by user */
 	unsigned int	mmio_always_on:1;	/* Disallow turning off io/mem
 						   decoding during BAR sizing */
 	unsigned int	wakeup_prepared:1;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/4] PCI: Refresh root ports in pci_bridge_d3_update()
  2023-12-03  4:10 [PATCH v2 0/4] Add support for drivers to decide bridge D3 policy Mario Limonciello
  2023-12-03  4:10 ` [PATCH v2 1/4] PCI: Make d3cold_allowed sysfs attribute read only Mario Limonciello
@ 2023-12-03  4:10 ` Mario Limonciello
  2023-12-12 19:25   ` Rafael J. Wysocki
  2023-12-03  4:10 ` [PATCH v2 3/4] ACPI: x86: s2idle: Export symbol for fetching constraints for module use Mario Limonciello
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Mario Limonciello @ 2023-12-03  4:10 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rafael J . Wysocki, Hans de Goede, Shyam Sundar S K,
	open list:PCI SUBSYSTEM, open list:X86 PLATFORM DRIVERS,
	Ilpo Järvinen, Lukas Wunner, Kai-Heng Feng, linux-acpi,
	linux-pm, Mario Limonciello

If pci_d3cold_enable() or pci_d3cold_disable() is called on a root
port it is ignored because there is no upstream bridge.

If called on a root port, use `no_d3cold` variable to decide policy
and also immediately refresh whether D3 is possible.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 72505794cc72..3d4aaecda457 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3023,6 +3023,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (pci_bridge_d3_disable)
 			return false;
 
+		if (bridge->no_d3cold)
+			return false;
+
 		/*
 		 * Hotplug ports handled by firmware in System Management Mode
 		 * may not be put into D3 by the OS (Thunderbolt on non-Macs).
@@ -3098,7 +3101,11 @@ void pci_bridge_d3_update(struct pci_dev *dev)
 	bool d3cold_ok = true;
 
 	bridge = pci_upstream_bridge(dev);
-	if (!bridge || !pci_bridge_d3_possible(bridge))
+	if (!bridge) {
+		dev->bridge_d3 = pci_bridge_d3_possible(dev);
+		return;
+	}
+	if (!pci_bridge_d3_possible(bridge))
 		return;
 
 	/*
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/4] ACPI: x86: s2idle: Export symbol for fetching constraints for module use
  2023-12-03  4:10 [PATCH v2 0/4] Add support for drivers to decide bridge D3 policy Mario Limonciello
  2023-12-03  4:10 ` [PATCH v2 1/4] PCI: Make d3cold_allowed sysfs attribute read only Mario Limonciello
  2023-12-03  4:10 ` [PATCH v2 2/4] PCI: Refresh root ports in pci_bridge_d3_update() Mario Limonciello
@ 2023-12-03  4:10 ` Mario Limonciello
  2023-12-03  4:10 ` [PATCH v2 4/4] platform/x86/amd: pmc: Add support for using constraints to decide D3 policy Mario Limonciello
  2023-12-04 14:08 ` [PATCH v2 0/4] Add support for drivers to decide bridge " Hans de Goede
  4 siblings, 0 replies; 9+ messages in thread
From: Mario Limonciello @ 2023-12-03  4:10 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rafael J . Wysocki, Hans de Goede, Shyam Sundar S K,
	open list:PCI SUBSYSTEM, open list:X86 PLATFORM DRIVERS,
	Ilpo Järvinen, Lukas Wunner, Kai-Heng Feng, linux-acpi,
	linux-pm, Mario Limonciello, Rafael J. Wysocki

The amd-pmc driver will be fetching constraints to make decisions at
suspend time. This driver can be compiled as a module, so export the
symbol for when it is a module.

Acked-by: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/x86/s2idle.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 7d64e655f1b8..427d5a8fb694 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -322,6 +322,7 @@ int acpi_get_lps0_constraint(struct acpi_device *adev)
 
 	return ACPI_STATE_UNKNOWN;
 }
+EXPORT_SYMBOL_GPL(acpi_get_lps0_constraint);
 
 static void lpi_check_constraints(void)
 {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 4/4] platform/x86/amd: pmc: Add support for using constraints to decide D3 policy
  2023-12-03  4:10 [PATCH v2 0/4] Add support for drivers to decide bridge D3 policy Mario Limonciello
                   ` (2 preceding siblings ...)
  2023-12-03  4:10 ` [PATCH v2 3/4] ACPI: x86: s2idle: Export symbol for fetching constraints for module use Mario Limonciello
@ 2023-12-03  4:10 ` Mario Limonciello
  2023-12-04 14:08 ` [PATCH v2 0/4] Add support for drivers to decide bridge " Hans de Goede
  4 siblings, 0 replies; 9+ messages in thread
From: Mario Limonciello @ 2023-12-03  4:10 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rafael J . Wysocki, Hans de Goede, Shyam Sundar S K,
	open list:PCI SUBSYSTEM, open list:X86 PLATFORM DRIVERS,
	Ilpo Järvinen, Lukas Wunner, Kai-Heng Feng, linux-acpi,
	linux-pm, Mario Limonciello

The default kernel policy will allow modern machines to effectively put
all PCIe bridges into PCI D3. This policy doesn't match what Windows uses.

In Windows the driver stack includes a "Power Engine Plugin" (uPEP driver)
to decide the policy for integrated devices using PEP device constraints.

Device constraints are expressed as a number in the _DSM of the PNP0D80
device and exported by the kernel in acpi_get_lps0_constraint().

Add support for SoCs to use constraints on Linux as well for deciding
target state for integrated PCI bridges.

Disable existing production SoCs by default with this change.

Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmc/pmc.c | 57 ++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index c3104714b480..9aa6cf4fabf1 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -793,6 +793,61 @@ static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
 	return 0;
 }
 
+static inline void amd_pmc_apply_constraint(struct pci_dev *pci_dev, bool apply)
+{
+	if (apply)
+		pci_d3cold_disable(pci_dev);
+	else
+		pci_d3cold_enable(pci_dev);
+}
+
+/*
+ * Constraints are specified in the ACPI LPS0 device and specify what the
+ * platform intended for devices that are internal to the SoC.
+ *
+ * If a constraint is present and >= to ACPI_STATE_D3, then enable D3.
+ * If a constraint is not present or < ACPI_STATE_D3, then disable D3.
+ */
+static void amd_pmc_check_constraints(struct amd_pmc_dev *pdev, bool apply)
+{
+	struct pci_dev *pci_dev = NULL;
+	struct acpi_device *adev;
+	int constraint;
+
+	switch (pdev->cpu_id) {
+	case AMD_CPU_ID_RV:
+	case AMD_CPU_ID_RN:
+	case AMD_CPU_ID_YC:
+	case AMD_CPU_ID_CB:
+	case AMD_CPU_ID_PS:
+	case AMD_CPU_ID_SP:
+		return;
+	default:
+		break;
+	}
+
+	while ((pci_dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev))) {
+		adev = ACPI_COMPANION(&pci_dev->dev);
+		if (!adev)
+			continue;
+		constraint = acpi_get_lps0_constraint(adev);
+		dev_dbg(&pci_dev->dev, "constraint is %d\n", constraint);
+
+		switch (constraint) {
+		case ACPI_STATE_UNKNOWN:
+		case ACPI_STATE_D0:
+		case ACPI_STATE_D1:
+		case ACPI_STATE_D2:
+			amd_pmc_apply_constraint(pci_dev, apply);
+			continue;
+		/* use the logic pci_bridge_d3_possible() to decide */
+		case ACPI_STATE_D3:
+		default:
+			continue;
+		}
+	}
+}
+
 static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
 {
 	struct rtc_device *rtc_device;
@@ -1099,6 +1154,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
 			amd_pmc_quirks_init(dev);
 	}
 
+	amd_pmc_check_constraints(dev, TRUE);
 	amd_pmc_dbgfs_register(dev);
 	pm_report_max_hw_sleep(U64_MAX);
 	return 0;
@@ -1114,6 +1170,7 @@ static void amd_pmc_remove(struct platform_device *pdev)
 
 	if (IS_ENABLED(CONFIG_SUSPEND))
 		acpi_unregister_lps0_dev(&amd_pmc_s2idle_dev_ops);
+	amd_pmc_check_constraints(dev, FALSE);
 	amd_pmc_dbgfs_unregister(dev);
 	pci_dev_put(dev->rdev);
 	mutex_destroy(&dev->lock);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/4] Add support for drivers to decide bridge D3 policy
  2023-12-03  4:10 [PATCH v2 0/4] Add support for drivers to decide bridge D3 policy Mario Limonciello
                   ` (3 preceding siblings ...)
  2023-12-03  4:10 ` [PATCH v2 4/4] platform/x86/amd: pmc: Add support for using constraints to decide D3 policy Mario Limonciello
@ 2023-12-04 14:08 ` Hans de Goede
  4 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2023-12-04 14:08 UTC (permalink / raw)
  To: Mario Limonciello, Bjorn Helgaas
  Cc: Rafael J . Wysocki, Shyam Sundar S K, open list:PCI SUBSYSTEM,
	open list:X86 PLATFORM DRIVERS, Ilpo Järvinen, Lukas Wunner,
	Kai-Heng Feng, linux-acpi, linux-pm

Hi all,

On 12/3/23 05:10, Mario Limonciello wrote:
> The policy for whether PCI bridges are allowed to select D3 is dictated
> by empirical results that are enumerated into pci_bridge_d3_possible().
> 
> In Windows this behaves differently in that Windows internal policy is
> not used for devices when a power engine plugin driver provided by the
> SOC vendor is installed.  This driver is used to decide the policy in
> those cases.
> 
> This series implements a system that lets drivers register such a policy
> control as well. It isn't activated for any SOCs by default.
> 
> This is heavily leveraged from the work in [1]
> 
> [1] https://lore.kernel.org/platform-driver-x86/20230906184354.45846-1-mario.limonciello@amd.com/

As I mentioned in the v1 thread, I expect this entire series to
go upstream through the PCI or ACPI trees, so I'm dropping this
from my queue.

Regards,

Hans



> 
> v1->v2:
>  * Pick up tags
>  * Rebase on v6.7-rc4
> 
> Mario Limonciello (4):
>   PCI: Make d3cold_allowed sysfs attribute read only
>   PCI: Refresh root ports in pci_bridge_d3_update()
>   ACPI: x86: s2idle: Export symbol for fetching constraints for module
>     use
>   platform/x86/amd: pmc: Add support for using constraints to decide D3
>     policy
> 
>  Documentation/ABI/testing/sysfs-bus-pci |  4 +-
>  drivers/acpi/x86/s2idle.c               |  1 +
>  drivers/pci/pci-acpi.c                  |  2 +-
>  drivers/pci/pci-sysfs.c                 | 14 +-----
>  drivers/pci/pci.c                       | 12 ++++--
>  drivers/platform/x86/amd/pmc/pmc.c      | 57 +++++++++++++++++++++++++
>  include/linux/pci.h                     |  1 -
>  7 files changed, 72 insertions(+), 19 deletions(-)
> 
> 
> base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/4] PCI: Refresh root ports in pci_bridge_d3_update()
  2023-12-03  4:10 ` [PATCH v2 2/4] PCI: Refresh root ports in pci_bridge_d3_update() Mario Limonciello
@ 2023-12-12 19:25   ` Rafael J. Wysocki
  2023-12-12 19:41     ` Mario Limonciello
  0 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2023-12-12 19:25 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Bjorn Helgaas, Rafael J . Wysocki, Hans de Goede,
	Shyam Sundar S K, open list:PCI SUBSYSTEM,
	open list:X86 PLATFORM DRIVERS, Ilpo Järvinen, Lukas Wunner,
	Kai-Heng Feng, linux-acpi, linux-pm

On Mon, Dec 4, 2023 at 7:07 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> If pci_d3cold_enable() or pci_d3cold_disable() is called on a root
> port it is ignored because there is no upstream bridge.

The kerneldoc comment of pci_bridge_d3_update() explains what that
function is for which also covers why it does not take effect when
called on root ports.

> If called on a root port, use `no_d3cold` variable to decide policy

It is unclear that this is about pci_bridge_d3_possible() which
applies to both D3hot and D3cold, not just D3cold AFAICS.  I don't
think that no_d3cold should affect the D3hot behavior.

> and also immediately refresh whether D3 is possible.

Which isn't correct AFAICS.

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/pci.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 72505794cc72..3d4aaecda457 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3023,6 +3023,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>                 if (pci_bridge_d3_disable)
>                         return false;
>
> +               if (bridge->no_d3cold)
> +                       return false;
> +
>                 /*
>                  * Hotplug ports handled by firmware in System Management Mode
>                  * may not be put into D3 by the OS (Thunderbolt on non-Macs).
> @@ -3098,7 +3101,11 @@ void pci_bridge_d3_update(struct pci_dev *dev)
>         bool d3cold_ok = true;
>
>         bridge = pci_upstream_bridge(dev);
> -       if (!bridge || !pci_bridge_d3_possible(bridge))
> +       if (!bridge) {
> +               dev->bridge_d3 = pci_bridge_d3_possible(dev);
> +               return;
> +       }
> +       if (!pci_bridge_d3_possible(bridge))
>                 return;
>
>         /*
> --
> 2.34.1
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/4] PCI: Refresh root ports in pci_bridge_d3_update()
  2023-12-12 19:25   ` Rafael J. Wysocki
@ 2023-12-12 19:41     ` Mario Limonciello
  2023-12-12 19:53       ` Rafael J. Wysocki
  0 siblings, 1 reply; 9+ messages in thread
From: Mario Limonciello @ 2023-12-12 19:41 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Bjorn Helgaas, Rafael J . Wysocki, Hans de Goede,
	Shyam Sundar S K, open list:PCI SUBSYSTEM,
	open list:X86 PLATFORM DRIVERS, Ilpo Järvinen, Lukas Wunner,
	Kai-Heng Feng, linux-acpi, linux-pm

On 12/12/2023 13:25, Rafael J. Wysocki wrote:
> On Mon, Dec 4, 2023 at 7:07 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> If pci_d3cold_enable() or pci_d3cold_disable() is called on a root
>> port it is ignored because there is no upstream bridge.
> 
> The kerneldoc comment of pci_bridge_d3_update() explains what that
> function is for which also covers why it does not take effect when
> called on root ports.

I'm sorry but can you clarify the intent of your comment?

Are you suggesting we should introduce a different function/logic for 
root ports, kernel doc should be updated, or root ports should be 
special cased in that function?

> 
>> If called on a root port, use `no_d3cold` variable to decide policy
> 
> It is unclear that this is about pci_bridge_d3_possible() which
> applies to both D3hot and D3cold, not just D3cold AFAICS.  I don't
> think that no_d3cold should affect the D3hot behavior.

IMO the semantics are confusing depending upon what device you called 
pci_d3cold_disable()/pci_d3cold_enable() with as an argument.

Both devices and root ports are used by existing driver in the kernel.

If you called pci_d3cold_disable() with a device, that actually prevents 
the /bridge above it/ from going to D3hot as well (bridge_d3 is set to 
the result)

> 
>> and also immediately refresh whether D3 is possible.
> 
> Which isn't correct AFAICS.

Why?

> 
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/pci/pci.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 72505794cc72..3d4aaecda457 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3023,6 +3023,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>>                  if (pci_bridge_d3_disable)
>>                          return false;
>>
>> +               if (bridge->no_d3cold)
>> +                       return false;
>> +
>>                  /*
>>                   * Hotplug ports handled by firmware in System Management Mode
>>                   * may not be put into D3 by the OS (Thunderbolt on non-Macs).
>> @@ -3098,7 +3101,11 @@ void pci_bridge_d3_update(struct pci_dev *dev)
>>          bool d3cold_ok = true;
>>
>>          bridge = pci_upstream_bridge(dev);
>> -       if (!bridge || !pci_bridge_d3_possible(bridge))
>> +       if (!bridge) {
>> +               dev->bridge_d3 = pci_bridge_d3_possible(dev);
>> +               return;
>> +       }
>> +       if (!pci_bridge_d3_possible(bridge))
>>                  return;
>>
>>          /*
>> --
>> 2.34.1
>>
>>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/4] PCI: Refresh root ports in pci_bridge_d3_update()
  2023-12-12 19:41     ` Mario Limonciello
@ 2023-12-12 19:53       ` Rafael J. Wysocki
  0 siblings, 0 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2023-12-12 19:53 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Rafael J. Wysocki, Bjorn Helgaas, Rafael J . Wysocki,
	Hans de Goede, Shyam Sundar S K, open list:PCI SUBSYSTEM,
	open list:X86 PLATFORM DRIVERS, Ilpo Järvinen, Lukas Wunner,
	Kai-Heng Feng, linux-acpi, linux-pm

On Tue, Dec 12, 2023 at 8:41 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 12/12/2023 13:25, Rafael J. Wysocki wrote:
> > On Mon, Dec 4, 2023 at 7:07 AM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> If pci_d3cold_enable() or pci_d3cold_disable() is called on a root
> >> port it is ignored because there is no upstream bridge.
> >
> > The kerneldoc comment of pci_bridge_d3_update() explains what that
> > function is for which also covers why it does not take effect when
> > called on root ports.
>
> I'm sorry but can you clarify the intent of your comment?
>
> Are you suggesting we should introduce a different function/logic for
> root ports, kernel doc should be updated, or root ports should be
> special cased in that function?

They are special-cased in that function already, because it updates an
upstream port for a change in a downstream device.

There are only 2 places really affected by no_d3cold:
pci_dev_check_d3cold() and the ACPI power state selection for PCI
devices. where the former is used for checking whether or not it is
valid to put an upstream bridge into D3hot/cold (which depends on
whether or not the downstream devices below it are allowed to use
D3cold).

The only place where no_d3cold affects root ports is the ACPI power
state selection, because the only way to program a root port into
D3cold is via ACPI.

> >
> >> If called on a root port, use `no_d3cold` variable to decide policy
> >
> > It is unclear that this is about pci_bridge_d3_possible() which
> > applies to both D3hot and D3cold, not just D3cold AFAICS.  I don't
> > think that no_d3cold should affect the D3hot behavior.
>
> IMO the semantics are confusing depending upon what device you called
> pci_d3cold_disable()/pci_d3cold_enable() with as an argument.
>
> Both devices and root ports are used by existing driver in the kernel.
>
> If you called pci_d3cold_disable() with a device, that actually prevents
> the /bridge above it/ from going to D3hot as well (bridge_d3 is set to
> the result)

Right, because (as per the PCI PM spec) putting an upstream bridge
into D3hot/cold effectively removes power from the bus segment below
it, so the devices on that bus segment go into D3cold.  If they are
not allowed to go into D3cold, the bridge needs to stay in a shallower
power state either.

> >
> >> and also immediately refresh whether D3 is possible.
> >
> > Which isn't correct AFAICS.
>
> Why?

Because it makes no_d3cold affect the ability of the given root port
to be programmed into D3hot via PMCSR.

> >
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >>   drivers/pci/pci.c | 9 ++++++++-
> >>   1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index 72505794cc72..3d4aaecda457 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -3023,6 +3023,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> >>                  if (pci_bridge_d3_disable)
> >>                          return false;
> >>
> >> +               if (bridge->no_d3cold)
> >> +                       return false;
> >> +
> >>                  /*
> >>                   * Hotplug ports handled by firmware in System Management Mode
> >>                   * may not be put into D3 by the OS (Thunderbolt on non-Macs).
> >> @@ -3098,7 +3101,11 @@ void pci_bridge_d3_update(struct pci_dev *dev)
> >>          bool d3cold_ok = true;
> >>
> >>          bridge = pci_upstream_bridge(dev);
> >> -       if (!bridge || !pci_bridge_d3_possible(bridge))
> >> +       if (!bridge) {
> >> +               dev->bridge_d3 = pci_bridge_d3_possible(dev);
> >> +               return;
> >> +       }
> >> +       if (!pci_bridge_d3_possible(bridge))
> >>                  return;
> >>
> >>          /*
> >> --
> >> 2.34.1
> >>
> >>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-12-12 19:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-03  4:10 [PATCH v2 0/4] Add support for drivers to decide bridge D3 policy Mario Limonciello
2023-12-03  4:10 ` [PATCH v2 1/4] PCI: Make d3cold_allowed sysfs attribute read only Mario Limonciello
2023-12-03  4:10 ` [PATCH v2 2/4] PCI: Refresh root ports in pci_bridge_d3_update() Mario Limonciello
2023-12-12 19:25   ` Rafael J. Wysocki
2023-12-12 19:41     ` Mario Limonciello
2023-12-12 19:53       ` Rafael J. Wysocki
2023-12-03  4:10 ` [PATCH v2 3/4] ACPI: x86: s2idle: Export symbol for fetching constraints for module use Mario Limonciello
2023-12-03  4:10 ` [PATCH v2 4/4] platform/x86/amd: pmc: Add support for using constraints to decide D3 policy Mario Limonciello
2023-12-04 14:08 ` [PATCH v2 0/4] Add support for drivers to decide bridge " Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).