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 <[email protected]> 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/[email protected]?part=1
