Re: [PATCH 01/13] platform/apple-gmux: Fix power for MMIO systems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] platform/x86: apple-gmux: unchecked discrete_pdev in gmux_set_discrete_state()
- [High] platform/x86: apple-gmux: gmux_resume() re-saves powered-off PCI bridge state
- [High] platform/x86: apple-gmux: GFX0 and HDAU PCI state restored before link poll
- [Medium] platform/x86: apple-gmux: saving discrete_pdev state gated on HDAU in PWRD off
- [Medium] platform/apple-gmux: unsynchronized discrete_pdev update in gmux_get_client_id()
--

commit 532665525b3aad0c7675ccb06880ee4bc4fc490b
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

platform/apple-gmux: Fix power for MMIO systems

This commit adds discrete GPU power management support for MMIO-based Apple
gmux systems using ACPI methods PWG1/PWG3, and adds a DMI quirk for
MacBookPro16,1 and MacBookPro16,4 to save/restore PCI bridge states and
toggle bridge power via PWRD.

> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 9c728ac4e045a..4e8786a5e58ec 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c

[ ... ]

> @@ -502,23 +508,171 @@ static int gmux_switch_ddc(enum vga_switcheroo_client_id id)

[ ... ]

>  static int gmux_set_discrete_state(struct apple_gmux_data *gmux_data,
>  				   enum vga_switcheroo_state state)
>  {
>  	reinit_completion(&gmux_data->powerchange_done);
> +	acpi_handle discrete_handle = ACPI_HANDLE(&gmux_data->discrete_pdev->dev);
> +	acpi_status status;
> +	unsigned long long result;
> +
> +	if (!discrete_handle)
> +		return -ENODEV;

[Severity: High]
Can gmux_set_discrete_state() in drivers/platform/x86/apple-gmux.c be called
while gmux_data->discrete_pdev is still NULL, or on non-MMIO gmux systems
where discrete_pdev and discrete_handle are not needed?

ACPI_HANDLE(&gmux_data->discrete_pdev->dev) immediately dereferences
gmux_data->discrete_pdev before checking whether it is NULL or whether
gmux_data->type is APPLE_GMUX_TYPE_MMIO.

In drivers/gpu/vga/vga_switcheroo.c, gmux_get_client_id() (which populates
gmux_data->discrete_pdev) is called from vga_switcheroo_enable() only after
both GPU clients and the handler have registered:

drivers/gpu/vga/vga_switcheroo.c:vga_switcheroo_register_client()
    if (vgasr_priv.registered_clients == 2 && vgasr_priv.handler)
        vga_switcheroo_enable();

However, if the discrete GPU driver registers and runtime-suspends before the
integrated GPU driver registers, vga_switcheroo_runtime_suspend() invokes
vga_switcheroo_power_switch() without checking vgasr_priv.active:

drivers/gpu/vga/vga_switcheroo.c:vga_switcheroo_power_switch() {
    ...
    vgasr_priv.handler->power_state(client->id, state);
}

Because client->id is still VGA_SWITCHEROO_UNKNOWN_ID and
gmux_set_power_state() only filters out id == VGA_SWITCHEROO_IGD:

drivers/platform/x86/apple-gmux.c:gmux_set_power_state() {
    if (id == VGA_SWITCHEROO_IGD)
        return 0;

    return gmux_set_discrete_state(apple_gmux_data, state);
}

gmux_set_discrete_state() is entered with gmux_data->discrete_pdev == NULL,
causing a NULL pointer dereference.

In addition, on APPLE_GMUX_TYPE_PIO and APPLE_GMUX_TYPE_INDEXED systems,
discrete_handle is unused in gmux_set_discrete_state(). Should the
discrete_pdev NULL check and discrete_handle lookup be moved inside the
gmux_data->type == APPLE_GMUX_TYPE_MMIO branches?

>  
>  	if (state == VGA_SWITCHEROO_ON) {
> -		gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 1);
> -		gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 3);
> +		if (gmux_data->type == APPLE_GMUX_TYPE_MMIO) {
> +			u16 vendor;
> +			int i;
> +
> +			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 2);
> +			msleep(100);
> +			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 3);
> +
> +			if (gmux_data->uses_pwrd) {
> +				acpi_handle handle;
> +
> +				status = acpi_execute_simple_method(discrete_handle, "PWRD", 0);
> +				if (ACPI_FAILURE(status))
> +					return -EIO;
> +
> +				if (gmux_save_restore_pci(NULL, "\\_SB_.PCI0.PEG0", 0, &handle))
> +					return -EIO;
> +
> +				if (gmux_save_restore_pci(handle, "EGP0", 0, &handle))
> +					return -EIO;
> +
> +				if (gmux_save_restore_pci(handle, "EGP1", 0, &handle))
> +					return -EIO;
> +
> +				if (gmux_save_restore_pci(handle, "GFX0", 0, NULL))
> +					return -EIO;
> +
> +				if (gmux_save_restore_pci(handle, "HDAU", 0, NULL))
> +					return -EIO;
> +			} else {
> +				status = acpi_evaluate_integer(discrete_handle,
> +							"PWG1", NULL, &result);
> +				if (ACPI_FAILURE(status) || result)
> +					return -EIO;
> +			}
> +
> +			for (i = 0; i < 1000; i++) {
> +				pci_read_config_word(gmux_data->discrete_pdev,
> +						     PCI_VENDOR_ID, &vendor);
> +				if (vendor != 0xffff)
> +					break;
> +				msleep(1);
> +			}
> +			if (vendor == 0xffff)
> +				return -ETIMEDOUT;

[Severity: High]
In gmux_set_discrete_state() in drivers/platform/x86/apple-gmux.c, when
gmux_data->uses_pwrd is true, does restoring GFX0 and HDAU before polling
PCI_VENDOR_ID on gmux_data->discrete_pdev run pci_restore_state() before the
endpoints are ready on the PCIe bus?

In the VGA_SWITCHEROO_ON branch, gmux_save_restore_pci() calls
pci_restore_state() on PEG0, EGP0, EGP1, GFX0, and HDAU before entering the
loop that waits up to 1000 ms for gmux_data->discrete_pdev (GFX0) to report
vendor != 0xffff.

While the upstream bridges (PEG0, EGP0, EGP1) need to be restored first so
configuration transactions can reach the downstream bus, if GFX0 and HDAU are
still coming out of reset and returning 0xffff when gmux_save_restore_pci()
runs on them, pci_restore_state() will drop the configuration writes (BARs,
command register, MSI, and PCIe capabilities) and clear pdev->state_saved.

Should GFX0 and HDAU be restored only after the vendor != 0xffff poll loop
succeeds?

[ ... ]

>  	} else {
> -		gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 1);
> -		gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 0);
> +		if (gmux_data->type == APPLE_GMUX_TYPE_MMIO) {
> +			if (gmux_data->uses_pwrd) {
> +				acpi_handle parent_handle = discrete_handle;
> +				bool saved_hda = false;
> +
> +				while (ACPI_SUCCESS(acpi_get_parent(parent_handle,
> +								&parent_handle))) {
> +					if (!saved_hda) {
> +						if (!gmux_save_restore_pci(parent_handle,
> +									"HDAU", true, NULL)) {
> +							saved_hda = true;
> +							pci_save_state(gmux_data->discrete_pdev);
> +							gmux_data->discrete_pdev->current_state =
> +									PCI_D3cold;
> +						}
> +					}

[Severity: Medium]
Why is saving gmux_data->discrete_pdev gated on gmux_save_restore_pci()
succeeding for HDAU in gmux_set_discrete_state() in
drivers/platform/x86/apple-gmux.c?

If the HDAU PCI device was removed via sysfs (so
acpi_get_first_physical_node() returns NULL in gmux_save_restore_pci()),
gmux_save_restore_pci(parent_handle, "HDAU", true, NULL) returns -EIO. In
that case, pci_save_state(gmux_data->discrete_pdev) is skipped and
gmux_data->discrete_pdev->current_state is not set to PCI_D3cold before
PWRD(1) powers off the bridge, and during VGA_SWITCHEROO_ON a failure in
gmux_save_restore_pci(handle, "HDAU", 0, NULL) aborts powering up the
discrete GPU.

Could gmux_data->discrete_pdev be saved directly before the parent traversal
loop, with HDAU saved and restored on a best-effort basis?

> +					if (gmux_save_restore_pci(parent_handle, NULL, true, NULL))
> +						break;
> +				}
> +				status = acpi_execute_simple_method(discrete_handle, "PWRD", 1);
> +				if (ACPI_FAILURE(status))
> +					return -EIO;
> +			}

[Severity: High]
What happens on MacBookPro16,1 and MacBookPro16,4 when gmux_resume() in
drivers/platform/x86/apple-gmux.c calls gmux_set_discrete_state() while
gmux_data->power_state is already VGA_SWITCHEROO_OFF?

In gmux_resume():

drivers/platform/x86/apple-gmux.c:gmux_resume() {
    gmux_enable_interrupts(gmux_data);
    gmux_write_switch_state(gmux_data);
    if (gmux_data->power_state == VGA_SWITCHEROO_OFF)
        gmux_set_discrete_state(gmux_data, gmux_data->power_state);
    return 0;
}

When the discrete GPU and its upstream bridges were already powered down via
gmux_set_discrete_state(gmux_data, VGA_SWITCHEROO_OFF) prior to system
suspend, calling gmux_set_discrete_state(gmux_data, VGA_SWITCHEROO_OFF) again
during resume re-runs the uses_pwrd loop and calls pci_save_state() on HDAU,
gmux_data->discrete_pdev, and the upstream bridges (EGP1, EGP0, PEG0).

As noted in the Power control comment above gmux_save_restore_pci(),
communicating with these bridges while the bridge is off results in a CPU
CATERR. Even if firmware powered the bridges back up in an uninitialized reset
state during S3 resume, calling pci_save_state() here overwrites the valid
pre-suspend saved_config_space and saved_cap_space of discrete_pdev, HDAU, and
the bridges before re-cutting power.

> @@ -552,10 +706,15 @@ static enum vga_switcheroo_client_id gmux_get_client_id(struct pci_dev *pdev)
>  	else if (pdev->vendor == PCI_VENDOR_ID_NVIDIA &&
>  		 pdev->device == 0x0863)
>  		return VGA_SWITCHEROO_IGD;
> -	else
> +	else {
> +		if (apple_gmux_data->discrete_pdev != pdev) {
> +			pci_dev_put(apple_gmux_data->discrete_pdev);
> +			apple_gmux_data->discrete_pdev = pci_dev_get(pdev);
> +		}
>  		return VGA_SWITCHEROO_DIS;
> -}
>  
> +	}
> +}

[Severity: Medium]
Can concurrent calls to gmux_get_client_id() in
drivers/platform/x86/apple-gmux.c race when updating
apple_gmux_data->discrete_pdev?

In drivers/gpu/vga/vga_switcheroo.c, callers of handler->get_client_id() do
not share a common lock:

- vga_switcheroo_enable() and vga_switcheroo_register_audio_client() call
  handler->get_client_id() while holding only vgasr_mutex.
- vga_switcheroo_lock_ddc() and vga_switcheroo_unlock_ddc() call
  handler->get_client_id() while holding only vgasr_priv.mux_hw_lock (and
  without checking vgasr_priv.active).

If the discrete GPU driver calls vga_switcheroo_lock_ddc() while the
integrated GPU driver concurrently registers and triggers
vga_switcheroo_enable(), both threads can observe
apple_gmux_data->discrete_pdev != pdev, call pci_dev_get(pdev), and leak a
struct pci_dev reference. Furthermore, if an external GPU (such as a
Thunderbolt eGPU audio client via sound/hda/controllers/intel.c:get_bound_vga)
passes another non-IGD struct pci_dev to gmux_get_client_id(), it can
overwrite apple_gmux_data->discrete_pdev and race on pci_dev_put().

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