Re: [PATCH] pci: Add broken-GPE quirk for Lenovo Legion 16APH8 and 16AHP9 Nvidia GPUs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing NULL validation for the ACPI companion of the parent device leads to a potential NULL pointer dereference in `acpi_remove_pm_notifier()`.
--

--- Patch [1]: [PATCH] pci: Add broken-GPE quirk for Lenovo Legion 16APH8 and 16AHP9 Nvidia GPUs ---
commit c5f31e20fc4140461290146739333c87be79e5ce
Author: Lyude Paul <[email protected]>

pci: Add broken-GPE quirk for Lenovo Legion 16APH8 and 16AHP9 Nvidia GPUs

This commit adds a PCI quirk for specific Lenovo Legion laptops. It disables
spurious GPE wakeup events on the parent PCIe port of the Nvidia GPU after
it enters D3Cold.

Fixes: https://github.com/NVIDIA/open-gpu-kernel-modules/issues/905

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index b09f27f7846fc..4d1f1565b61d3 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
[ ... ]
> @@ -6165,6 +6165,82 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, 0x13b1,
>  			      PCI_CLASS_DISPLAY_VGA, 8,
>  			      quirk_reset_lenovo_thinkpad_p50_nvgpu);
>  
> +#ifdef CONFIG_ACPI
> +#ifdef CONFIG_DMI
[ ... ]
> +static void quirk_nvidia_dgpu_broken_gpe(struct pci_dev *pdev)
> +{
> +	struct acpi_device *parent_adev;
> +	struct device *parent_dev;
> +	int ret;
[ ... ]
> +	parent_dev = pci_physfn(pdev)->dev.parent;
> +	if (!parent_dev) {
> +		pci_err(pdev,
> +			"Can't find PCIe parent? Your Nvidia GPU will have broken runtime PM\n");
> +		return;
> +	}
> +	parent_adev = ACPI_COMPANION(parent_dev);
> +
> +	/* The spurious GPEs will be sent from the ACPI device for the PCIe port this GPU is
> +	 * connected to, so remove our PM notifier to turn them into a no-op.
> +	 */
> +	ret = acpi_remove_pm_notifier(ACPI_COMPANION(parent_dev));

[Severity: High]
Can this cause a NULL pointer dereference if the parent PCIe bridge lacks an
ACPI companion?

ACPI_COMPANION() can return NULL if the system is booted with acpi=off or if
the node simply lacks an ACPI companion.

If that happens, this passes NULL to acpi_remove_pm_notifier(), which
dereferences its argument directly:

drivers/acpi/device_pm.c:acpi_remove_pm_notifier() {
    ...
    if (!adev->wakeup.flags.notifier_present)
    ...
}

Should parent_adev be checked for NULL before calling
acpi_remove_pm_notifier()? 

Also, it looks like parent_adev is assigned but then
ACPI_COMPANION(parent_dev) is called again instead of using the local
variable.

> +	if (ACPI_FAILURE(ret))
> +		pci_err(pdev, "Removing PM notifier failed: %d\n", ret);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1