Re: [PATCH] PCI: Disable ACPI 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] PCI: unbalanced ACPI power dependent cleanup in quirk_msi_peg0_no_wakeup()
--

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

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

This patch adds a PCI final fixup quirk for MSI MS-1592 and MS-1594 boards to
disable ACPI wakeup on the dGPU PCIe root port (0000:00:06.0), preventing
firmware from switching the touchpad's GPIO interrupt pin into ACPI GPE mode
during D3cold transitions.

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 7572b358b50fc..385c9d6db19e0 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3969,6 +3969,56 @@ static void quirk_apple_poweroff_thunderbolt(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
>  			       PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
>  			       quirk_apple_poweroff_thunderbolt);
> +
[ ... ]
> +static void quirk_msi_peg0_no_wakeup(struct pci_dev *dev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
> +
> +	if (!adev || 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);
> +	adev->wakeup.flags.valid = 0;
> +}
> +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]
Does clearing adev->wakeup.flags.valid in a DECLARE_PCI_FIXUP_FINAL quirk
without removing the ACPI power resource dependent leave a dangling pointer
on resource->dependents if the PCI device is later removed?

During device enumeration, pci_device_add() -> device_add() ->
acpi_device_notify() -> pci_acpi_setup() runs before pci_fixup_final quirks
are applied:

drivers/pci/pci-acpi.c:pci_acpi_setup() {
	...
	if (!adev->wakeup.flags.valid)
		return;

	device_set_wakeup_capable(dev, true);
	if (pci_dev->bridge_d3)
		device_wakeup_enable(dev);

	acpi_pci_wakeup(pci_dev, false);
	acpi_device_power_add_dependent(adev, dev);
	...
}

Because adev->wakeup.flags.valid is still set when pci_acpi_setup() runs,
acpi_device_power_add_dependent() links a struct acpi_power_dependent_device
entry holding an unreferenced dep->dev = &dev->dev pointer onto the D0 power
resource's dependents list (here, \_SB.PC00.PEG0.PXP).

When quirk_msi_peg0_no_wakeup() subsequently clears adev->wakeup.flags.valid,
any later removal of the root port (for example via sysfs remove ->
pci_stop_and_remove_bus_device() -> pci_destroy_dev() -> device_del() ->
acpi_device_notify_remove() -> pci_acpi_cleanup()) will skip removing that
dependent entry:

drivers/pci/pci-acpi.c:pci_acpi_cleanup() {
	struct acpi_device *adev = ACPI_COMPANION(dev);
	struct pci_dev *pci_dev = to_pci_dev(dev);

	if (adev->wakeup.flags.valid) {
		acpi_device_power_remove_dependent(adev, dev);
		if (pci_dev->bridge_d3)
			device_wakeup_disable(dev);

		device_set_wakeup_capable(dev, false);
	}
}

Once struct pci_dev is freed, if the power resource is turned on again with
another dependent present (such as during a PCI bus rescan),
__acpi_power_on() iterates resource->dependents and passes the freed
dep->dev pointer to dev_dbg() and pm_request_resume().

In addition, clearing adev->wakeup.flags.valid causes
acpi_free_power_resources_lists() to skip freeing adev->wakeup.resources on
ACPI device teardown, and causes pci_acpi_setup() on a subsequent bus rescan
to return early before calling acpi_dev_power_up_children_with_adr(adev).

Since pci_acpi_setup() and pm_runtime_put() have already run by the time
final quirks execute, and runtime/system suspend paths check
device_can_wakeup(&dev->dev) (in pci_Full_Power_On / pci_target_state /
pci_finish_runtime_suspend / pci_prepare_to_sleep) before invoking
acpi_pci_wakeup(), is setting device_wakeup_disable(&dev->dev) and
device_set_wakeup_capable(&dev->dev, false) sufficient on its own without
clearing adev->wakeup.flags.valid, or should
acpi_device_power_remove_dependent(adev, &dev->dev) be called if
adev->wakeup.flags.valid is cleared?

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