Re: [PATCH v2] ACPI: x86: Disable wakeup on MSI MS-1592 and MS-1594 dGPU root port

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ACPI: x86: quirk_msi_peg0_no_wakeup() fails to block _DSW() in D3cold
--

commit 234606e6357f6c832bc95f0db9d5907bb7aeaec6
Author: Victor Carreras <vicajilau@gmail.com>

ACPI: x86: Disable wakeup on MSI MS-1592 and MS-1594 dGPU root port

This patch adds a final PCI fixup quirk for MSI MS-1592 and MS-1594 boards on
root port 0000:00:06.0 to disable wakeup capability. This aims to prevent
\_SB.PC00.PEG0._DSW from arming wakeup during D3cold transitions so that BIOS
power-off methods do not switch the touchpad interrupt GPIO (GPP_A20) to ACPI
GPE mode.

Link: https://lore.kernel.org/r/179272791966.20251.3697951431087014523@gmail.com

> diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
> index 418951639f511..84a6ca7f10a8e 100644
> --- a/drivers/acpi/x86/utils.c
> +++ b/drivers/acpi/x86/utils.c
> @@ -665,3 +665,50 @@ void __init acpi_proc_quirk_mwait_check(void)

[ ... ]

> +static void quirk_msi_peg0_no_wakeup(struct pci_dev *dev)
> +{
> +	if (pci_dev_id(dev) != PCI_DEVID(0, PCI_DEVFN(6, 0)))
> +		return;
> +
> +	if (!dmi_check_system(msi_peg0_no_wakeup_dmi_table))
> +		return;
> +
> +	pci_info(dev, FW_BUG "Disabling ACPI wakeup to avoid touchpad GPIO (GPP_A20) takeover in D3cold\n");
> +	device_wakeup_disable(&dev->dev);
> +	device_set_wakeup_capable(&dev->dev, false);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x464d, quirk_msi_peg0_no_wakeup);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0xa74d, quirk_msi_peg0_no_wakeup);

[Severity: High]
Will clearing should_wakeup and can_wakeup in a final fixup without clearing
adev->wakeup.flags.valid actually prevent \_SB.PC00.PEG0._DSW(1, 0, 3) from
being evaluated when the dGPU and root port runtime-suspend to D3cold?

There appear to be two runtime-suspend paths that still evaluate _DSW on
0000:00:06.0:

1. Direct root port runtime suspend:

quirk_msi_peg0_no_wakeup() runs during pci_fixup_device(pci_fixup_final, dev)
in drivers/pci/bus.c:pci_bus_add_device(), before device_initial_probe() and
before pcie_portdrv_init() runs at device_initcall time.

When pcie_portdriver later binds to root port 0000:00:06.0 and probes the PME
service, pcie_pme_probe() -> pcie_pme_mark_devices() -> pcie_pme_can_wakeup()
in drivers/pci/pcie/pme.c sets can_wakeup back to true:

static int pcie_pme_can_wakeup(struct pci_dev *dev, void *ign)
{
	device_set_wakeup_capable(&dev->dev, true);
	return 0;
}

When 0000:00:06.0 later runtime-suspends, drivers/pci/pci.c:
pci_pm_runtime_suspend() -> pci_finish_runtime_suspend() executes:

	__pci_enable_wake(dev, target_state, pci_dev_run_wake(dev));

Unlike pci_enable_wake() (which checks device_may_wakeup()),
pci_dev_run_wake() checks dev->pme_support and device_can_wakeup(&dev->dev)
while ignoring should_wakeup. __pci_enable_wake() then calls
platform_pci_set_wakeup(dev, true) -> acpi_pci_wakeup(dev, true) in
drivers/pci/pci-acpi.c:

int acpi_pci_wakeup(struct pci_dev *dev, bool enable)
{
	if (acpi_pci_disabled)
		return 0;

	if (acpi_pm_device_can_wakeup(&dev->dev))
		return acpi_pm_set_device_wakeup(&dev->dev, enable);

	return acpi_pci_propagate_wakeup(dev->bus, enable);
}

Because acpi_pm_device_can_wakeup() in drivers/acpi/device_pm.c only checks
adev->wakeup.flags.valid (which is no longer cleared in v2),
acpi_pm_set_device_wakeup() -> acpi_enable_wakeup_device_power() ->
acpi_device_sleep_wake(adev, 1, 0, 3) will still evaluate \_SB.PC00.PEG0._DSW.

2. Wakeup propagation from child devices on subordinate bus 0000:01:

Even if dev->dev.power.can_wakeup stayed false on 0000:00:06.0, child devices
on bus 0000:01 (such as the NVIDIA dGPU at 0000:01:00.0) have their own
pme_support and power.can_wakeup = true. When a child device runtime-suspends,
pci_finish_runtime_suspend() -> __pci_enable_wake() calls
acpi_pci_wakeup(child, true) -> acpi_pci_propagate_wakeup(child->bus, true) in
drivers/pci/pci-acpi.c:

static int acpi_pci_propagate_wakeup(struct pci_bus *bus, bool enable)
{
	while (bus->parent) {
		if (acpi_pm_device_can_wakeup(&bus->self->dev))
			return acpi_pm_set_device_wakeup(&bus->self->dev, enable);

		bus = bus->parent;
	}

acpi_pci_propagate_wakeup() walks up to bus->self (0000:00:06.0), checks only
acpi_pm_device_can_wakeup(&bus->self->dev) (adev->wakeup.flags.valid), and
calls acpi_pm_set_device_wakeup(&bus->self->dev, true), evaluating
\_SB.PC00.PEG0._DSW(1, 0, 3).

Would clearing adev->wakeup.flags.valid in a DECLARE_PCI_FIXUP_EARLY or
DECLARE_PCI_FIXUP_HEADER quirk (which runs after pci_set_acpi_fwnode() in
pci_setup_device(), but before pci_acpi_setup() in pci_device_add() ->
device_add()) avoid this issue while keeping pci_acpi_setup() and
pci_acpi_cleanup() balanced?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024054711.21587-1-vicajilau@gmail.com?part=1