[PATCH 00/13] Fixes for apple-gmux and runtime support

[PATCH 00/13] Fixes for apple-gmux and runtime support

[Atharva Tiwari]

This series fixes apple-gmux on T2 Macs (2018-2020):

  - Fix interrupt handling
  - Automatically turn on dGPU when a external monitor connects.
  - Fix base register read/write
  - Fix power handling
  - Improve documentation
  - New module parameter to toggle boot GPU.
  
It also adds vga_switcheroo runtime power management for T2 Macs, built
on gmux power control, so the dGPU is powered off automatically when
idle and users no longer need to toggle it manually.

Andre Eikmeyer (6):
  platform/apple-gmux: Write switch state to EFI on MMIO devices
  drm/amdgpu: Add Apple GMUX runtime PM support
  drm/amdgpu: Catch register access while GMUX keeps the GPU off
  drm/amd/display: Refuse DP AUX while GMUX keeps the GPU off
  drm/amdgpu: Skip AC/DC notification while the GPU is runtime suspended
  drm/amd/display: Skip panel power savings on eDP links without a panel

Atharva Tiwari (7):
  platform/apple-gmux: Fix power for MMIO systems
  PCI: Disable pcie port and access on D3 on specific amd devices
  platform/apple-gmux: Fix MMIO interface
  platform/apple-gmux: Fix Interrupts for MMIO systems
  platform/apple-gmux: Add parameter to configure boot gpu
  platform/apple-gmux: Add check to detect FSS
  platform/apple-gmux: Add documentation about iMacs

 arch/x86/pci/fixup.c                          |  30 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   4 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |  60 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  45 ++-
 .../gpu/drm/amd/amdgpu/amdgpu_reg_access.c    |  24 ++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  14 +
 .../display/amdgpu_dm/amdgpu_dm_backlight.c   |   3 +
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  19 +-
 drivers/gpu/drm/amd/pm/amdgpu_dpm.c           |  19 +-
 drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h       |   1 +
 drivers/pci/access.c                          |  18 +
 drivers/pci/pcie/portdrv.c                    |   2 +-
 drivers/platform/x86/Kconfig                  |   1 +
 drivers/platform/x86/apple-gmux.c             | 377 ++++++++++++++++--
 include/linux/apple-gmux.h                    |   1 +
 include/linux/pci.h                           |   7 +
 16 files changed, 554 insertions(+), 71 deletions(-)

-- 
2.55.0

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

[Atharva Tiwari]

Currently The dGPU on MMIO Systems, dies when we write the gmux values
to turn off the gpu.

On most systems we can just write PWG1 (link-training method) and the
dGPU comes up, then we write a MMIO variable, and run PWG3
(link-retraing method), to stablize the dGPU.

But on 16,1 and 16,4 this dosent work, instead we have to use PWRD
method, which turns off the entire bridge, and completely messes the
pci config, so we can't use pci to traverse through it. we save the
entire bridge using ACPI addresses then we run PWRD(1) to turn off
the bridge, then we write gmux states, and resume we do the vice
versa, but here we dont need to retrain the link and write the
MMIO variable.

Co-developed-by: Andre Eikmeyer <dev@deq.rocks>
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>

Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 177 ++++++++++++++++++++++++++++--
 1 file changed, 170 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 9c728ac4e..4e8786a5e 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -22,6 +22,7 @@
 #include <linux/pci.h>
 #include <linux/vga_switcheroo.h>
 #include <linux/debugfs.h>
+#include <linux/dmi.h>
 #include <acpi/video.h>
 #include <asm/io.h>
 
@@ -65,6 +66,11 @@ struct apple_gmux_data {
 
 	struct backlight_device *bdev;
 
+	/* power handling data */
+	struct pci_dev *discrete_pdev;
+	bool uses_pwrd;
+	enum apple_gmux_type type;
+
 	/* switcheroo data */
 	acpi_handle dhandle;
 	int gpe;
@@ -502,23 +508,171 @@ static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
 /**
  * DOC: Power control
  *
- * gmux is able to cut power to the discrete GPU. It automatically takes care
+ * gmux is able to cut power to the discrete GPU. On most
+ * Systems the gmux automatically takes care
  * of the correct sequence to tear down and bring up the power rails for
  * core voltage, VRAM and PCIe.
+ *
+ * But on most MMIO Systems, we have to PCIE link-train the discrete GPU after
+ * power on, we can do that using PWG1 and then to stablize we write to
+ * 0x8C340 and then link-train the PCIE link again (this time using PWG3).
+ *
+ * But on MacBookPro16,1 and MacBookPro16,4, the PWG1 method does not work
+ * instead we have to use PWRD method, which turns off the entire bridge and
+ * messes up the PCI config, so we can't use pci_upstream_bridge to traverse
+ * through the bridge, so we use ACPI addresses to save/restore bridges.
+ *
+ * NOTE: Communication to these bridges (not the dGPU/HDA) in any form
+ * when the bridge is off, will result in CPU CATERR.
  */
 
+static int gmux_save_restore_pci(acpi_handle parent_handle, const char *name,
+				bool save, acpi_handle *out_handle)
+{
+	acpi_status status;
+	acpi_handle handle;
+	struct acpi_device *adev;
+	struct device *dev;
+	struct pci_dev *pdev;
+
+	if (name) {
+		status = acpi_get_handle(parent_handle, name, &handle);
+		if (ACPI_FAILURE(status))
+			return -EIO;
+
+		if (out_handle)
+			*out_handle = handle;
+	} else
+		handle = parent_handle;
+
+	adev = acpi_fetch_acpi_dev(handle);
+	if (!adev)
+		return -EIO;
+
+	dev = acpi_get_first_physical_node(adev);
+	if (!dev || !dev_is_pci(dev))
+		return -EIO;
+
+	pdev = to_pci_dev(dev);
+
+	if (save) {
+		pci_save_state(pdev);
+		pdev->current_state = PCI_D3cold;
+	} else {
+		pdev->current_state = PCI_D0;
+		pci_restore_state(pdev);
+	}
+	return 0;
+}
+
 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;
 
 	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;
+
+			if (!gmux_data->uses_pwrd) {
+				void __iomem *bar = pci_iomap(gmux_data->discrete_pdev, 0, 0);
+
+				if (!bar)
+					return -ENOMEM;
+
+				iowrite32(ioread32(bar + 0x8c340) & 0x3fffffff, bar + 0x8c340);
+				pci_iounmap(gmux_data->discrete_pdev, bar);
+
+				status = acpi_evaluate_integer(discrete_handle,
+							"PWG3", NULL, &result);
+				if (ACPI_FAILURE(status) || result)
+					return -EIO;
+			}
+		} else {
+			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 1);
+			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 3);
+		}
 		pr_debug("Discrete card powered up\n");
 	} 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;
+						}
+					}
+					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;
+			}
+			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 1);
+			mdelay(10);
+			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 0);
+		} else {
+			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 1);
+			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 0);
+		}
 		pr_debug("Discrete card powered down\n");
 	}
 
@@ -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;
-}
 
+	}
+}
 static const struct vga_switcheroo_handler gmux_handler_no_ddc = {
 	.switchto = gmux_switchto,
 	.power_state = gmux_set_power_state,
@@ -803,6 +962,9 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	if (!gmux_data)
 		return -ENOMEM;
 	pnp_set_drvdata(pnp, gmux_data);
+	gmux_data->type = type;
+	gmux_data->uses_pwrd = (dmi_match(DMI_PRODUCT_NAME, "MacBookPro16,1") ||
+		 dmi_match(DMI_PRODUCT_NAME, "MacBookPro16,4"));
 
 	switch (type) {
 	case APPLE_GMUX_TYPE_MMIO:
@@ -993,6 +1155,7 @@ static void gmux_remove(struct pnp_dev *pnp)
 
 	gmux_fini_debugfs(gmux_data);
 	vga_switcheroo_unregister_handler();
+	pci_dev_put(gmux_data->discrete_pdev);
 	gmux_disable_interrupts(gmux_data);
 	if (gmux_data->gpe >= 0) {
 		acpi_disable_gpe(NULL, gmux_data->gpe);
-- 
2.55.0

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

[sashiko-bot]

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

[PATCH 02/13] PCI: Disable pcie port and access on D3 on specific amd devices

[Atharva Tiwari]

As mentioned in the previous commit, PWRD turns off the entire
bridge, and if we read/write anything to the config space
or the PCIE space when the bridge is dead, the CPU CATERR's,
so to avoid interaction when the bridge is dead,
we remove pcieport support for the bridges, and we restrict
access to those bridges when the bridge is dead.

Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
---
 arch/x86/pci/fixup.c       | 30 ++++++++++++++++++++++++++++++
 drivers/pci/access.c       | 18 ++++++++++++++++++
 drivers/pci/pcie/portdrv.c |  2 +-
 include/linux/pci.h        |  7 +++++++
 4 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 795d81c7a..66829941e 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -1094,6 +1094,35 @@ static void asus_disable_nvme_d3cold(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x9a09, asus_disable_nvme_d3cold);
 
+static const struct dmi_system_id quirk_disable_pcie_port_dmi_table[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,4"),
+		},
+	},
+	{}
+};
+
+static void quirk_disable_pcie_port(struct pci_dev *pdev)
+{
+	struct pci_dev *upstream = pdev;
+
+	if (dmi_check_system(quirk_disable_pcie_port_dmi_table)) {
+		while ((upstream = pci_upstream_bridge(upstream))) {
+			upstream->no_pcie_port = 1;
+			upstream->ignore_access_d3 = 1;
+		}
+	}
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7340, quirk_disable_pcie_port);
+
 #ifdef CONFIG_SUSPEND
 /*
  * Root Ports on some AMD SoCs advertise PME_Support for D3hot and D3cold, but
@@ -1179,4 +1208,5 @@ static void quirk_tuxeo_rp_d3(struct pci_dev *pdev)
 	}
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x1502, quirk_tuxeo_rp_d3);
+
 #endif /* CONFIG_SUSPEND */
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index b123da16b..5e9513067 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -559,6 +559,9 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
 
 int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
 {
+	if (pci_dev_ignore_access(dev))
+		return PCIBIOS_SUCCESSFUL;
+
 	if (pci_dev_is_disconnected(dev)) {
 		PCI_SET_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -569,6 +572,9 @@ EXPORT_SYMBOL(pci_read_config_byte);
 
 int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
 {
+	if (pci_dev_ignore_access(dev))
+		return PCIBIOS_SUCCESSFUL;
+
 	if (pci_dev_is_disconnected(dev)) {
 		PCI_SET_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -580,6 +586,9 @@ EXPORT_SYMBOL(pci_read_config_word);
 int pci_read_config_dword(const struct pci_dev *dev, int where,
 					u32 *val)
 {
+	if (pci_dev_ignore_access(dev))
+		return PCIBIOS_SUCCESSFUL;
+
 	if (pci_dev_is_disconnected(dev)) {
 		PCI_SET_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -590,6 +599,9 @@ EXPORT_SYMBOL(pci_read_config_dword);
 
 int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
 {
+	if (pci_dev_ignore_access(dev))
+		return PCIBIOS_SUCCESSFUL;
+
 	if (pci_dev_is_disconnected(dev))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
@@ -598,6 +610,9 @@ EXPORT_SYMBOL(pci_write_config_byte);
 
 int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
 {
+	if (pci_dev_ignore_access(dev))
+		return PCIBIOS_SUCCESSFUL;
+
 	if (pci_dev_is_disconnected(dev))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
@@ -607,6 +622,9 @@ EXPORT_SYMBOL(pci_write_config_word);
 int pci_write_config_dword(const struct pci_dev *dev, int where,
 					 u32 val)
 {
+	if (pci_dev_ignore_access(dev))
+		return PCIBIOS_SUCCESSFUL;
+
 	if (pci_dev_is_disconnected(dev))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index a9cbfc1d2..d2f19e566 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -688,7 +688,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	int type = pci_pcie_type(dev);
 	int status;
 
-	if (!pci_is_pcie(dev) ||
+	if (!pci_is_pcie(dev) || dev->no_pcie_port ||
 	    ((type != PCI_EXP_TYPE_ROOT_PORT) &&
 	     (type != PCI_EXP_TYPE_UPSTREAM) &&
 	     (type != PCI_EXP_TYPE_DOWNSTREAM) &&
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d31a8d107..8835b1c82 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -420,6 +420,8 @@ struct pci_dev {
 	unsigned int	no_d1d2:1;	/* D1 and D2 are forbidden */
 	unsigned int	no_d3cold:1;	/* D3cold is forbidden */
 	unsigned int	bridge_d3:1;	/* Allow D3 for bridge */
+	unsigned int	no_pcie_port:1; /* PCIE Port driver is forbidden */
+	unsigned int	ignore_access_d3:1;	/* Ignore access when device is in D3Cold */
 	unsigned int	d3cold_allowed:1;	/* D3cold is allowed by user */
 	unsigned int	mmio_always_on:1;	/* Disallow turning off io/mem
 						   decoding during BAR sizing */
@@ -2747,6 +2749,11 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
 	return READ_ONCE(dev->error_state) == pci_channel_io_perm_failure;
 }
 
+static inline bool pci_dev_ignore_access(const struct pci_dev *dev)
+{
+	return dev->ignore_access_d3 && dev->current_state == PCI_D3cold;
+}
+
 void pci_request_acs(void);
 bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
 bool pci_acs_path_enabled(struct pci_dev *start,
-- 
2.55.0

Re: [PATCH 02/13] PCI: Disable pcie port and access on D3 on specific amd devices

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] PCI: uninitialized *val in pci_read_config_*() when ignoring D3cold access
- [Critical] PCI: pci_user_read_config_*() and cap walks bypass pci_dev_ignore_access()
- [High] PCI: missing 0x7360 fixup in quirk_disable_pcie_port() for MacBookPro16,4
--

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

PCI: Disable pcie port and access on D3 on specific amd devices

This commit adds an early PCI fixup for Apple MacBookPro16,1 and
MacBookPro16,4 systems to disable the PCIe port driver and ignore config
space accesses on upstream bridges of the discrete GPU when those bridges
are in D3cold.

> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 795d81c7a4de4..66829941e8fe2 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -1094,6 +1094,35 @@ static void asus_disable_nvme_d3cold(struct pci_dev *pdev)
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x9a09, asus_disable_nvme_d3cold);
>  
> +static const struct dmi_system_id quirk_disable_pcie_port_dmi_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,4"),
> +		},
> +	},
> +	{}
> +};
> +
> +static void quirk_disable_pcie_port(struct pci_dev *pdev)
> +{
> +	struct pci_dev *upstream = pdev;
> +
> +	if (dmi_check_system(quirk_disable_pcie_port_dmi_table)) {
> +		while ((upstream = pci_upstream_bridge(upstream))) {
> +			upstream->no_pcie_port = 1;
> +			upstream->ignore_access_d3 = 1;
> +		}
> +	}
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7340, quirk_disable_pcie_port);

[Severity: High]
While quirk_disable_pcie_port_dmi_table in arch/x86/pci/fixup.c matches both
MacBookPro16,1 and MacBookPro16,4, DECLARE_PCI_FIXUP_EARLY is only registered
for PCI device ID 0x7340 (Navi 14, used in MacBookPro16,1).

MacBookPro16,4 uses the AMD Radeon Pro 5600M (Navi 12, PCI device ID 0x7360).
Without a matching fixup registration for 0x7360, will
quirk_disable_pcie_port() ever run on MacBookPro16,4, or will its upstream
bridges remain unprotected when apple-gmux powers them off?

[ ... ]

> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index b123da16b63ba..5e95130671767 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -559,6 +559,9 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
>  
>  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -569,6 +572,9 @@ EXPORT_SYMBOL(pci_read_config_byte);
>  
>  int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -580,6 +586,9 @@ EXPORT_SYMBOL(pci_read_config_word);
>  int pci_read_config_dword(const struct pci_dev *dev, int where,
>  					u32 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;

[Severity: Critical]
When pci_dev_ignore_access(dev) is true for a bridge in PCI_D3cold,
pci_read_config_byte(), pci_read_config_word(), and pci_read_config_dword()
in drivers/pci/access.c return PCIBIOS_SUCCESSFUL (0) without initializing
*val.

Many callers pass an uninitialized stack variable and rely on either a
non-zero return value or PCI_SET_ERROR_RESPONSE(val) (0xff / 0xffff /
0xffffffff) when the device cannot be read.

For example, in drivers/pci/pci-sysfs.c, secondary_bus_number_show() and
subordinate_bus_number_show() read world-readable sysfs attributes into
uninitialized stack variables:

drivers/pci/pci-sysfs.c:secondary_bus_number_show() {
	struct pci_dev *pci_dev = to_pci_dev(dev);
	u8 sec_bus;
	int err;

	pci_config_pm_runtime_get(pci_dev);
	err = pci_read_config_byte(pci_dev, PCI_SECONDARY_BUS, &sec_bus);
	pci_config_pm_runtime_put(pci_dev);

	if (err)
		return -EINVAL;

	return sysfs_emit(buf, "%u\n", sec_bus);
}

In addition, pci_update_current_state() and pci_power_up() in
drivers/pci/pci.c read PCI_PM_CTRL into an uninitialized stack variable
pmcsr and check PCI_POSSIBLE_ERROR(pmcsr):

drivers/pci/pci.c:pci_update_current_state() {
	...
	} else if (dev->pm_cap) {
		u16 pmcsr;

		pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
		if (PCI_POSSIBLE_ERROR(pmcsr)) {
			dev->current_state = PCI_D3cold;
			return;
		}
		dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
	}
}

If pmcsr happens not to be 0xffff on the stack, dev->current_state is
overwritten with D0..D3hot (or set to PCI_D0 in pci_power_up()), which
disarms pci_dev_ignore_access(dev) while the bridge is still powered off.
Similarly, during suspend on the driverless bridge,
pci_pm_suspend_noirq() -> pci_save_state() -> pci_save_pcie_state() ->
pcie_capability_read_word() will see PCIBIOS_SUCCESSFUL with *val == 0 and
overwrite the bridge's saved PCIe capability state with zeros.

Should pci_read_config_byte(), pci_read_config_word(), and
pci_read_config_dword() call PCI_SET_ERROR_RESPONSE(val) and return an error
such as PCIBIOS_DEVICE_NOT_FOUND (matching pci_dev_is_disconnected(dev)), or
otherwise ensure *val is never left uninitialized?

> @@ -590,6 +599,9 @@ EXPORT_SYMBOL(pci_read_config_dword);
>  
>  int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
> @@ -598,6 +610,9 @@ EXPORT_SYMBOL(pci_write_config_byte);
>  
>  int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
> @@ -607,6 +622,9 @@ EXPORT_SYMBOL(pci_write_config_word);
>  int pci_write_config_dword(const struct pci_dev *dev, int where,
>  					 u32 val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);

[Severity: Critical]
Does restricting only pci_read_config_*() and pci_write_config_*() in
drivers/pci/access.c leave other config space access paths unprotected when
the bridge is in PCI_D3cold?

Userspace config reads and writes via sysfs (pci_read_config() and
pci_write_config() in drivers/pci/pci-sysfs.c) and procfs (drivers/pci/proc.c)
call pci_user_read_config_*() and pci_user_write_config_*(), which are
generated by PCI_USER_READ_CONFIG and PCI_USER_WRITE_CONFIG in
drivers/pci/access.c:

drivers/pci/access.c:PCI_USER_READ_CONFIG() {
	...
	raw_spin_lock_irq(&pci_lock);
	if (unlikely(dev->block_cfg_access))
		pci_wait_cfg(dev);
	ret = dev->bus->ops->read(dev->bus, dev->devfn,
				  pos, sizeof(type), &data);
	raw_spin_unlock_irq(&pci_lock);
	...
}

Because dev->no_pcie_port = 1 prevents pcie_portdrv_probe() from binding to
the upstream bridges, runtime PM is not enabled on them and
pci_config_pm_runtime_get() in pci_read_config() is a no-op. Any unprivileged
process (such as lspci or udev) reading /sys/bus/pci/devices/.../config while
the bridge is powered off in PCI_D3cold will invoke dev->bus->ops->read()
directly on the dead bridge.

In addition, capability walks in drivers/pci/pci.c (pci_find_capability() ->
__pci_bus_find_cap_start(), pci_find_next_capability(), and
pci_find_next_ext_capability()) call pci_bus_read_config_*() directly rather
than pci_read_config_*(). Because the bridge has no bound driver (pm == NULL),
during system suspend pci_pm_suspend_noirq() in drivers/pci/pci-driver.c
runs:

  pci_pm_suspend_noirq()
    -> pci_save_state()
      -> pci_save_pcix_state()
        -> pci_find_capability(dev, PCI_CAP_ID_PCIX)
          -> __pci_bus_find_cap_start()
            -> pci_bus_read_config_word(bus, devfn, PCI_STATUS, &status)

Could these paths still access the powered-off bridge and trigger a hardware
crash unless PCI_USER_READ_CONFIG, PCI_USER_WRITE_CONFIG, and the capability
lookup paths (or a lower layer) also check pci_dev_ignore_access(dev)?

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

[PATCH 03/13] platform/apple-gmux: Fix MMIO interface

[Atharva Tiwari]

This patch brings a lot of fixes to MMIO interface such as:
1. Using 1ms waits instead of 100us.
2. Checking if Timed out before reading and writing
3. Adding missing wait before gmux_mmio_write32

Co-developed-by: Andre Eikmeyer <dev@deq.rocks>
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>

Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 59 ++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 4e8786a5e..076ed7ba6 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -237,8 +237,8 @@ static int gmux_mmio_wait(struct apple_gmux_data *gmux_data)
 	u8 gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
 
 	while (i && gwr) {
+		fsleep(1000);
 		gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
-		udelay(100);
 		i--;
 	}
 
@@ -250,12 +250,14 @@ static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
 	u8 val;
 
 	mutex_lock(&gmux_data->index_lock);
-	gmux_mmio_wait(gmux_data);
-	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
-	iowrite8(GMUX_MMIO_READ | sizeof(val),
-		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
-	gmux_mmio_wait(gmux_data);
-	val = ioread8(gmux_data->iomem_base);
+	if (gmux_mmio_wait(gmux_data)) {
+		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+		iowrite8(GMUX_MMIO_READ | sizeof(val),
+			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+		if (!gmux_mmio_wait(gmux_data))
+			return 0;
+		val = ioread8(gmux_data->iomem_base);
+	}
 	mutex_unlock(&gmux_data->index_lock);
 
 	return val;
@@ -265,14 +267,17 @@ static void gmux_mmio_write8(struct apple_gmux_data *gmux_data, int port,
 			      u8 val)
 {
 	mutex_lock(&gmux_data->index_lock);
-	gmux_mmio_wait(gmux_data);
-	iowrite8(val, gmux_data->iomem_base);
 
-	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
-	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
-		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+	if (gmux_mmio_wait(gmux_data)) {
+		iowrite8(val, gmux_data->iomem_base);
 
-	gmux_mmio_wait(gmux_data);
+		iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+		iowrite8(GMUX_MMIO_WRITE | sizeof(val),
+			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+
+		/* We already wrote the value there is no point to check if timeout */
+		gmux_mmio_wait(gmux_data);
+	}
 	mutex_unlock(&gmux_data->index_lock);
 }
 
@@ -281,12 +286,16 @@ static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int port)
 	u32 val;
 
 	mutex_lock(&gmux_data->index_lock);
-	gmux_mmio_wait(gmux_data);
-	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
-	iowrite8(GMUX_MMIO_READ | sizeof(val),
-		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
-	gmux_mmio_wait(gmux_data);
-	val = ioread32be(gmux_data->iomem_base);
+
+	if (gmux_mmio_wait(gmux_data)) {
+		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+		iowrite8(GMUX_MMIO_READ | sizeof(val),
+			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+		if (!gmux_mmio_wait(gmux_data))
+			return 0;
+
+		val = ioread32be(gmux_data->iomem_base);
+	}
 	mutex_unlock(&gmux_data->index_lock);
 
 	return val;
@@ -296,11 +305,13 @@ static void gmux_mmio_write32(struct apple_gmux_data *gmux_data, int port,
 			       u32 val)
 {
 	mutex_lock(&gmux_data->index_lock);
-	iowrite32be(val, gmux_data->iomem_base);
-	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
-	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
-		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
-	gmux_mmio_wait(gmux_data);
+	if (gmux_mmio_wait(gmux_data)) {
+		iowrite32be(val, gmux_data->iomem_base);
+		iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+		iowrite8(GMUX_MMIO_WRITE | sizeof(val),
+			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+		gmux_mmio_wait(gmux_data);
+	}
 	mutex_unlock(&gmux_data->index_lock);
 }
 
-- 
2.55.0

Re: [PATCH 03/13] platform/apple-gmux: Fix MMIO interface

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] platform/x86: apple-gmux: missing unlock on timeout in gmux_mmio_read8/32()
- [High] platform/x86: apple-gmux: uninitialized return in gmux_mmio_read8/32()
--

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

platform/apple-gmux: Fix MMIO interface

This patch updates the apple-gmux MMIO helpers to sleep for 1ms per poll
iteration in gmux_mmio_wait(), check for wait timeouts before performing MMIO
reads and writes, and wait before writing in gmux_mmio_write32().

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

[ ... ]

> @@ -250,12 +250,14 @@ static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
>  	u8 val;
>  
>  	mutex_lock(&gmux_data->index_lock);
> -	gmux_mmio_wait(gmux_data);
> -	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> -	iowrite8(GMUX_MMIO_READ | sizeof(val),
> -		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> -	gmux_mmio_wait(gmux_data);
> -	val = ioread8(gmux_data->iomem_base);
> +	if (gmux_mmio_wait(gmux_data)) {
> +		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +		iowrite8(GMUX_MMIO_READ | sizeof(val),
> +			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		if (!gmux_mmio_wait(gmux_data))
> +			return 0;

[Severity: High]
Does this early return in gmux_mmio_read8() in
drivers/platform/x86/apple-gmux.c leave gmux_data->index_lock locked when the
second gmux_mmio_wait(gmux_data) call times out and returns 0?

The same early return while holding gmux_data->index_lock also appears in
gmux_mmio_read32() below, which will cause subsequent GMIO accesses to
deadlock on mutex_lock(&gmux_data->index_lock).

> +		val = ioread8(gmux_data->iomem_base);
> +	}
>  	mutex_unlock(&gmux_data->index_lock);
>  
>  	return val;
>  }

[ ... ]

> @@ -281,12 +286,16 @@ static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int port)
>  	u32 val;
>  
>  	mutex_lock(&gmux_data->index_lock);
> -	gmux_mmio_wait(gmux_data);
> -	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> -	iowrite8(GMUX_MMIO_READ | sizeof(val),
> -		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> -	gmux_mmio_wait(gmux_data);
> -	val = ioread32be(gmux_data->iomem_base);
> +
> +	if (gmux_mmio_wait(gmux_data)) {
> +		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +		iowrite8(GMUX_MMIO_READ | sizeof(val),
> +			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		if (!gmux_mmio_wait(gmux_data))
> +			return 0;
> +
> +		val = ioread32be(gmux_data->iomem_base);
> +	}
>  	mutex_unlock(&gmux_data->index_lock);
>  
>  	return val;
>  }

[Severity: High]
Can gmux_mmio_read32() and gmux_mmio_read8() in
drivers/platform/x86/apple-gmux.c return an uninitialized val if the initial
gmux_mmio_wait(gmux_data) call times out and returns 0?

In both functions, val is declared without an initializer and is only
assigned inside the if (gmux_mmio_wait(gmux_data)) block before being
returned after mutex_unlock(&gmux_data->index_lock). Could val be initialized
to 0 (and returned after unlocking on both timeout paths)?

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

[PATCH 04/13] platform/apple-gmux: Fix Interrupts for MMIO systems

[Atharva Tiwari]

on MMIO systems, we need check to for failure in interrupts,
and we have implimented display hotplug to wake up the gpu.

Co-developed-by: Andre Eikmeyer <dev@deq.rocks>
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>

Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 96 +++++++++++++++++++++++++++----
 1 file changed, 84 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 076ed7ba6..bef87a57a 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -6,6 +6,8 @@
  *  Copyright (C) 2010-2012 Andreas Heider <andreas@meetr.de>
  *  Copyright (C) 2015 Lukas Wunner <lukas@wunner.de>
  *  Copyright (C) 2023 Orlando Chamberlain <orlandoch.dev@gmail.com>
+ *  Copyright (C) 2026 Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
+ *  Copyright (C) 2026 Andre Eikmeyer <dev@deq.rocks>
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -23,6 +25,8 @@
 #include <linux/vga_switcheroo.h>
 #include <linux/debugfs.h>
 #include <linux/dmi.h>
+#include <linux/workqueue.h>
+#include <linux/pm_runtime.h>
 #include <acpi/video.h>
 #include <asm/io.h>
 
@@ -66,6 +70,10 @@ struct apple_gmux_data {
 
 	struct backlight_device *bdev;
 
+	/* interrupt handling data */
+	bool external_state;
+	struct work_struct interrupt_work;
+
 	/* power handling data */
 	struct pci_dev *discrete_pdev;
 	bool uses_pwrd;
@@ -105,6 +113,7 @@ struct apple_gmux_config {
 
 #define GMUX_INTERRUPT_STATUS_ACTIVE	0
 #define GMUX_INTERRUPT_STATUS_DISPLAY	(1 << 0)
+#define GMUX_INTERRUPT_STATUS_FAILED	(1 << 1)
 #define GMUX_INTERRUPT_STATUS_POWER	(1 << 2)
 #define GMUX_INTERRUPT_STATUS_HOTPLUG	(1 << 3)
 
@@ -417,8 +426,8 @@ static const struct backlight_ops gmux_bl_ops = {
  * the discrete GPU can then be woken to drive the newly connected display.
  * The ability to switch AUX on these generations could be used to improve
  * reliability of hotplug detection by having the integrated GPU poll the
- * ports while the discrete GPU is asleep, but currently we do not make use
- * of this feature.
+ * ports while the discrete GPU is asleep, but currently we only make use of
+ * this feature on MMIO/T2 Macs.
  *
  * Our switching policy for the external port is that on those generations
  * which are able to switch it fully, the port is switched together with the
@@ -796,9 +805,10 @@ static const struct apple_gmux_config apple_gmux_mmio = {
  * value is read and then inverted.
  *
  * Because Linux masquerades as Darwin, it ends up in the notification-only code
- * path. On MMIO gmux's, this seems to lead to us being unable to clear interrupts,
- * unless we call GMSP(0). Without this, there is a flood of status=0 interrupts
- * that can't be cleared. This issue seems to be unique to MMIO gmux's.
+ * path. On MMIO gmux's, The Polarity is initally left in a undefined state leading
+ * to lots of interrupts, that we can't clear, to fix this we set the polarity to 0
+ * everytime we enable interrupts, On MMIO gmux's, GMLV reads the same GPIO pin as
+ * GMSP, and setting the polarity to 1, stops the Interrupts completely.
  */
 
 static inline void gmux_disable_interrupts(struct apple_gmux_data *gmux_data)
@@ -807,8 +817,11 @@ static inline void gmux_disable_interrupts(struct apple_gmux_data *gmux_data)
 		    GMUX_INTERRUPT_DISABLE);
 }
 
-static inline void gmux_enable_interrupts(struct apple_gmux_data *gmux_data)
+static inline void gmux_enable_interrupts(struct apple_gmux_data *gmux_data, bool do_gmsp)
 {
+	if (gmux_data->type == APPLE_GMUX_TYPE_MMIO && do_gmsp)
+		acpi_execute_simple_method(gmux_data->dhandle, "GMSP", 0);
+
 	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_ENABLE,
 		    GMUX_INTERRUPT_ENABLE);
 }
@@ -825,23 +838,78 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
 	/* to clear interrupts write back current status */
 	status = gmux_interrupt_get_status(gmux_data);
 	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
-	/* Prevent flood of status=0 interrupts */
-	if (gmux_data->config == &apple_gmux_mmio)
-		acpi_execute_simple_method(gmux_data->dhandle, "GMSP", 0);
 }
 
+static void gmux_notify_work(struct work_struct *work)
+{
+	struct apple_gmux_data *gmux_data =
+		container_of(work, struct apple_gmux_data, interrupt_work);
+	u8 status;
+	bool turn_on_gpu = false;
+
+	status = gmux_interrupt_get_status(gmux_data);
+	if (!status)
+		return;
+
+	gmux_disable_interrupts(gmux_data);
+	pr_debug("Notify handler called: status %d\n", status);
+	acpi_execute_simple_method(gmux_data->dhandle, "GMSP", 0);
+
+	status = gmux_interrupt_get_status(gmux_data);
+	while (status) {
+		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
+		if (status & GMUX_INTERRUPT_STATUS_FAILED) {
+			pr_warn_ratelimited("interrupt failed: status: 0x%x\n", status);
+			status = gmux_interrupt_get_status(gmux_data);
+			continue;
+		}
+		if (status & GMUX_INTERRUPT_STATUS_POWER)
+			complete(&gmux_data->powerchange_done);
+
+		if (status & GMUX_INTERRUPT_STATUS_HOTPLUG) {
+			/*
+			 * We get an interrupt every time a display connects or disconnects, but
+			 * the GET_DISPLAY port only tells us if a display is connected or not,
+			 * so we resume every time a display is connected or disconnected.
+			 */
+			bool external_state = !!gmux_read8(gmux_data,
+						GMUX_PORT_SWITCH_GET_DISPLAY);
+			if (external_state != gmux_data->external_state) {
+				gmux_data->external_state = external_state;
+				if (external_state)
+					pr_debug("A display has been connected\n");
+				else
+					pr_debug("All displays has been disconnected\n");
+			}
+			if (!gmux_data->external_switchable && gmux_data->discrete_pdev)
+				turn_on_gpu = true;
+		}
+		status = gmux_interrupt_get_status(gmux_data);
+	}
+
+	gmux_enable_interrupts(gmux_data, 0);
+
+	if (turn_on_gpu)
+		pm_runtime_resume(&gmux_data->discrete_pdev->dev);
+
+}
 static void gmux_notify_handler(acpi_handle device, u32 value, void *context)
 {
 	u8 status;
 	struct pnp_dev *pnp = (struct pnp_dev *)context;
 	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
 
+	if (gmux_data->type == APPLE_GMUX_TYPE_MMIO) {
+		schedule_work(&gmux_data->interrupt_work);
+		return;
+	}
+
 	status = gmux_interrupt_get_status(gmux_data);
 	gmux_disable_interrupts(gmux_data);
 	pr_debug("Notify handler called: status %d\n", status);
 
 	gmux_clear_interrupts(gmux_data);
-	gmux_enable_interrupts(gmux_data);
+	gmux_enable_interrupts(gmux_data, 0);
 
 	if (status & GMUX_INTERRUPT_STATUS_POWER)
 		complete(&gmux_data->powerchange_done);
@@ -935,7 +1003,7 @@ static int gmux_resume(struct device *dev)
 	struct pnp_dev *pnp = to_pnp_dev(dev);
 	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
 
-	gmux_enable_interrupts(gmux_data);
+	gmux_enable_interrupts(gmux_data, 1);
 	gmux_write_switch_state(gmux_data);
 	if (gmux_data->power_state == VGA_SWITCHEROO_OFF)
 		gmux_set_discrete_state(gmux_data, gmux_data->power_state);
@@ -996,6 +1064,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 			pr_err("couldn't remap gmux mmio region");
 			goto err_release;
 		}
+		INIT_WORK(&gmux_data->interrupt_work, gmux_notify_work);
 		goto get_version;
 	case APPLE_GMUX_TYPE_INDEXED:
 		gmux_data->config = &apple_gmux_index;
@@ -1115,7 +1184,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 
 	apple_gmux_data = gmux_data;
 	init_completion(&gmux_data->powerchange_done);
-	gmux_enable_interrupts(gmux_data);
+	gmux_enable_interrupts(gmux_data, 1);
 	gmux_read_switch_state(gmux_data);
 
 	/*
@@ -1150,6 +1219,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 err_unmap:
 	if (gmux_data->iomem_base)
 		iounmap(gmux_data->iomem_base);
+	if (gmux_data->type == APPLE_GMUX_TYPE_MMIO)
+		cancel_work_sync(&gmux_data->interrupt_work);
 err_release:
 	if (gmux_data->config->resource_type == IORESOURCE_MEM)
 		release_mem_region(gmux_data->iostart, gmux_data->iolen);
@@ -1168,6 +1239,7 @@ static void gmux_remove(struct pnp_dev *pnp)
 	vga_switcheroo_unregister_handler();
 	pci_dev_put(gmux_data->discrete_pdev);
 	gmux_disable_interrupts(gmux_data);
+	cancel_work_sync(&gmux_data->interrupt_work);
 	if (gmux_data->gpe >= 0) {
 		acpi_disable_gpe(NULL, gmux_data->gpe);
 		acpi_remove_notify_handler(gmux_data->dhandle,
-- 
2.55.0

Re: [PATCH 04/13] platform/apple-gmux: Fix Interrupts for MMIO systems

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] platform/apple-gmux: incorrect teardown order in gmux_remove() causes UAF
- [High] platform/apple-gmux: cancel_work_sync() on uninitialized work in gmux_remove()
- [High] platform/apple-gmux: self-deadlock on powerchange_done in gmux_notify_work()
- [High] platform/apple-gmux: iounmap() before cancel_work_sync() in gmux_probe()
- [High] platform/apple-gmux: missing interrupt_work cancellation in gmux_suspend()
- [High] platform/apple-gmux: unhandled status=0 GPEs and late init in gmux_probe()
- [High] platform/x86: apple-gmux: unreleased lock and uninit val in gmux_mmio_read8()
- [Medium] platform/apple-gmux: lost events and unbounded loop in gmux_notify_work()
--

commit 8b786eb41ee899344257c35e97176ec39dc10858
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

platform/apple-gmux: Fix Interrupts for MMIO systems

This commit moves MMIO GMUX interrupt handling to a workqueue item, checks
for failed interrupt status bits, and wakes the discrete GPU via runtime PM
when external display hotplug interrupts occur on MMIO systems.

> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 076ed7ba6af90..bef87a57ad43c 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c

[ ... ]

> @@ -825,23 +838,78 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
>  	/* to clear interrupts write back current status */
>  	status = gmux_interrupt_get_status(gmux_data);
>  	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> -	/* Prevent flood of status=0 interrupts */
> -	if (gmux_data->config == &apple_gmux_mmio)
> -		acpi_execute_simple_method(gmux_data->dhandle, "GMSP", 0);
>  }
>  
> +static void gmux_notify_work(struct work_struct *work)
> +{
> +	struct apple_gmux_data *gmux_data =
> +		container_of(work, struct apple_gmux_data, interrupt_work);
> +	u8 status;
> +	bool turn_on_gpu = false;
> +
> +	status = gmux_interrupt_get_status(gmux_data);
> +	if (!status)
> +		return;
> +
> +	gmux_disable_interrupts(gmux_data);
> +	pr_debug("Notify handler called: status %d\n", status);
> +	acpi_execute_simple_method(gmux_data->dhandle, "GMSP", 0);

[Severity: High]
If a GPE interrupt fires on an MMIO GMUX system while
GMUX_PORT_INTERRUPT_STATUS reads 0, doesn't returning early before calling
GMSP(0) leave the status=0 interrupt flood uncleared?

Previously, gmux_clear_interrupts() called GMSP(0) even when status was 0.
In gmux_probe(), acpi_install_notify_handler() and acpi_enable_gpe() are
called before init_completion(&gmux_data->powerchange_done) and before
gmux_enable_interrupts(gmux_data, 1) initializes the GPIO polarity via
GMSP(0):

gmux_probe()
    acpi_install_notify_handler(..., &gmux_notify_handler, pnp);
    acpi_enable_gpe(NULL, gmux_data->gpe);
    ...
    init_completion(&gmux_data->powerchange_done);
    gmux_enable_interrupts(gmux_data, 1);

If a GPE fires in that window (or during resume before gmux_resume() runs)
with status == 0, gmux_notify_work() returns without calling GMSP(0). And if
a GPE fires in that probe window with GMUX_INTERRUPT_STATUS_POWER set, won't
gmux_notify_work() call complete(&gmux_data->powerchange_done) before
init_completion(&gmux_data->powerchange_done) has initialized the waitqueue?

[Severity: High]
What happens in gmux_notify_work() if gmux_mmio_wait() times out inside
gmux_mmio_read8()?

On MMIO systems, gmux_interrupt_get_status() and gmux_read8() call
gmux_mmio_read8() (introduced in the earlier series patch "platform/apple-gmux:
Fix MMIO interface"), which has the same structure as gmux_mmio_read32():

static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
{
	u8 val;

	mutex_lock(&gmux_data->index_lock);
	if (gmux_mmio_wait(gmux_data)) {
		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
		iowrite8(GMUX_MMIO_READ | sizeof(val),
			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
		if (!gmux_mmio_wait(gmux_data))
			return 0;
		val = ioread8(gmux_data->iomem_base);
	}
	mutex_unlock(&gmux_data->index_lock);

	return val;
}

If the first gmux_mmio_wait() call fails, gmux_mmio_read8() (and
gmux_mmio_read32()) returns the uninitialized stack variable val, causing
gmux_notify_work() to treat stack garbage as the interrupt status and
potentially loop, complete gmux_data->powerchange_done, or wake the discrete
GPU.

If the second gmux_mmio_wait() call fails, gmux_mmio_read8() (and
gmux_mmio_read32()) returns 0 while still holding gmux_data->index_lock, so
the subsequent gmux_enable_interrupts() -> gmux_write8() -> gmux_mmio_write8()
call in gmux_notify_work() will immediately deadlock on gmux_data->index_lock.

> +
> +	status = gmux_interrupt_get_status(gmux_data);
> +	while (status) {
> +		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> +		if (status & GMUX_INTERRUPT_STATUS_FAILED) {
> +			pr_warn_ratelimited("interrupt failed: status: 0x%x\n", status);
> +			status = gmux_interrupt_get_status(gmux_data);
> +			continue;
> +		}

[Severity: Medium]
If GMUX_INTERRUPT_STATUS_FAILED is set in status at the same time as
GMUX_INTERRUPT_STATUS_POWER or GMUX_INTERRUPT_STATUS_HOTPLUG, doesn't writing
the full status value to GMUX_PORT_INTERRUPT_STATUS clear all of those bits in
hardware before the continue statement skips handling them?

Also, if clearing GMUX_PORT_INTERRUPT_STATUS fails (for example, if
gmux_mmio_wait() times out in gmux_mmio_write8() or a hardware status bit
remains stuck), could while (status) spin indefinitely on system_percpu_wq?

> +		if (status & GMUX_INTERRUPT_STATUS_POWER)
> +			complete(&gmux_data->powerchange_done);
> +
> +		if (status & GMUX_INTERRUPT_STATUS_HOTPLUG) {
> +			/*
> +			 * We get an interrupt every time a display connects or disconnects, but
> +			 * the GET_DISPLAY port only tells us if a display is connected or not,
> +			 * so we resume every time a display is connected or disconnected.
> +			 */
> +			bool external_state = !!gmux_read8(gmux_data,
> +						GMUX_PORT_SWITCH_GET_DISPLAY);
> +			if (external_state != gmux_data->external_state) {
> +				gmux_data->external_state = external_state;
> +				if (external_state)
> +					pr_debug("A display has been connected\n");
> +				else
> +					pr_debug("All displays has been disconnected\n");
> +			}
> +			if (!gmux_data->external_switchable && gmux_data->discrete_pdev)
> +				turn_on_gpu = true;
> +		}
> +		status = gmux_interrupt_get_status(gmux_data);
> +	}
> +
> +	gmux_enable_interrupts(gmux_data, 0);
> +
> +	if (turn_on_gpu)
> +		pm_runtime_resume(&gmux_data->discrete_pdev->dev);
> +
> +}

[Severity: High]
Won't calling synchronous pm_runtime_resume(&gmux_data->discrete_pdev->dev)
from inside gmux_notify_work() self-deadlock on gmux_data->powerchange_done
when the discrete GPU is runtime-suspended?

When discrete_pdev is runtime-suspended, pm_runtime_resume() synchronously
runs the power switch callback on the current worker thread:

gmux_notify_work() [executing gmux_data->interrupt_work]
  pm_runtime_resume(&gmux_data->discrete_pdev->dev)
    ...
      vga_switcheroo_runtime_resume()
        vga_switcheroo_power_switch()
          gmux_set_power_state()
            gmux_set_discrete_state(apple_gmux_data, VGA_SWITCHEROO_ON)
              reinit_completion(&gmux_data->powerchange_done);
              ...
              wait_for_completion_interruptible_timeout(&gmux_data->powerchange_done,
                                                        msecs_to_jiffies(200));

When the resulting GMUX_INTERRUPT_STATUS_POWER interrupt fires,
gmux_notify_handler() calls schedule_work(&gmux_data->interrupt_work).
Because gmux_data->interrupt_work is the same work_struct currently blocked in
pm_runtime_resume() and a work_struct cannot run concurrently with itself,
gmux_notify_work() cannot run to call complete(&gmux_data->powerchange_done)
until wait_for_completion_interruptible_timeout() times out after 200ms.

Would asynchronous pm_request_resume() avoid blocking interrupt_work here?

[ ... ]

> @@ -935,7 +1003,7 @@ static int gmux_resume(struct device *dev)
>  	struct pnp_dev *pnp = to_pnp_dev(dev);
>  	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
>  
> -	gmux_enable_interrupts(gmux_data);
> +	gmux_enable_interrupts(gmux_data, 1);
>  	gmux_write_switch_state(gmux_data);
>  	if (gmux_data->power_state == VGA_SWITCHEROO_OFF)
>  		gmux_set_discrete_state(gmux_data, gmux_data->power_state);

[Severity: High]
In gmux_suspend() right above gmux_resume(), gmux_disable_interrupts() is
called without cancelling gmux_data->interrupt_work:

static int gmux_suspend(struct device *dev)
{
	struct pnp_dev *pnp = to_pnp_dev(dev);
	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);

	gmux_disable_interrupts(gmux_data);
	return 0;
}

If gmux_data->interrupt_work is pending or running when gmux_suspend()
executes on an MMIO system, can gmux_notify_work() run concurrently with or
after gmux_suspend(), re-enabling interrupts via
gmux_enable_interrupts(gmux_data, 0) and calling
pm_runtime_resume(&gmux_data->discrete_pdev->dev) during system suspend?

[ ... ]

> @@ -1150,6 +1219,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  err_unmap:
>  	if (gmux_data->iomem_base)
>  		iounmap(gmux_data->iomem_base);
> +	if (gmux_data->type == APPLE_GMUX_TYPE_MMIO)
> +		cancel_work_sync(&gmux_data->interrupt_work);
>  err_release:
>  	if (gmux_data->config->resource_type == IORESOURCE_MEM)
>  		release_mem_region(gmux_data->iostart, gmux_data->iolen);

[Severity: High]
In the err_unmap error path of gmux_probe(), iounmap(gmux_data->iomem_base) is
called before cancel_work_sync(&gmux_data->interrupt_work). If
gmux_notify_work() was scheduled before gmux_probe() failed, won't
gmux_notify_work() access unmapped MMIO memory through gmux_data->iomem_base
before cancel_work_sync() waits for it to finish?

> @@ -1168,6 +1239,7 @@ static void gmux_remove(struct pnp_dev *pnp)
>  	vga_switcheroo_unregister_handler();
>  	pci_dev_put(gmux_data->discrete_pdev);
>  	gmux_disable_interrupts(gmux_data);
> +	cancel_work_sync(&gmux_data->interrupt_work);
>  	if (gmux_data->gpe >= 0) {
>  		acpi_disable_gpe(NULL, gmux_data->gpe);
>  		acpi_remove_notify_handler(gmux_data->dhandle,

[Severity: High]
In gmux_probe(), INIT_WORK(&gmux_data->interrupt_work, gmux_notify_work) is
only called in the APPLE_GMUX_TYPE_MMIO case. On APPLE_GMUX_TYPE_PIO and
APPLE_GMUX_TYPE_INDEXED systems, won't calling
cancel_work_sync(&gmux_data->interrupt_work) unconditionally in gmux_remove()
pass an uninitialized work_struct (with work->func == NULL) to
__flush_work() and trigger a WARN_ON(!work->func) warning?

[Severity: Critical]
In gmux_remove(), is the teardown ordering around cancel_work_sync() safe
against a racing ACPI notification?

Currently gmux_remove() does:

	pci_dev_put(gmux_data->discrete_pdev);
	gmux_disable_interrupts(gmux_data);
	cancel_work_sync(&gmux_data->interrupt_work);
	if (gmux_data->gpe >= 0) {
		acpi_disable_gpe(NULL, gmux_data->gpe);
		acpi_remove_notify_handler(gmux_data->dhandle,
					   ACPI_DEVICE_NOTIFY,
					   &gmux_notify_handler);
	}
	...
	if (gmux_data->iomem_base) {
		iounmap(gmux_data->iomem_base);
		release_mem_region(gmux_data->iostart, gmux_data->iolen);
	} else
		release_region(gmux_data->iostart, gmux_data->iolen);
	apple_gmux_data = NULL;
	kfree(gmux_data);

First, pci_dev_put(gmux_data->discrete_pdev) and gmux_disable_interrupts() are
called before cancel_work_sync(), so an already-running gmux_notify_work() can
still dereference gmux_data->discrete_pdev in pm_runtime_resume() and call
gmux_enable_interrupts(gmux_data, 0) after interrupts were disabled.

Second, because acpi_disable_gpe() and acpi_remove_notify_handler() are called
after cancel_work_sync(&gmux_data->interrupt_work), any GPE or in-flight
execution of gmux_notify_handler() before acpi_remove_notify_handler()
completes will call schedule_work(&gmux_data->interrupt_work) again, causing
gmux_notify_work() to run concurrently with iounmap(gmux_data->iomem_base) and
kfree(gmux_data).

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

[PATCH 05/13] platform/apple-gmux: Write switch state to EFI on MMIO devices

[Atharva Tiwari]

From: Andre Eikmeyer <dev@deq.rocks>

On T2 devices (MMIO), the T2 coprocessor reads gpu-active efi
variable, and macOS seems to write it, so to properly communciate
to the T2 coprocessor, we write gpu-active.

Co-developed: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
 drivers/platform/x86/Kconfig      |  1 +
 drivers/platform/x86/apple-gmux.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index c37e76066..01895d511 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -217,6 +217,7 @@ config APPLE_GMUX
 	depends on ACPI && PCI
 	depends on PNP
 	depends on BACKLIGHT_CLASS_DEVICE
+	depends on EFI
 	help
 	  This driver provides support for the gmux device found on many
 	  Apple laptops, which controls the display mux for the hybrid
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index bef87a57a..637b3b216 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -27,6 +27,7 @@
 #include <linux/dmi.h>
 #include <linux/workqueue.h>
 #include <linux/pm_runtime.h>
+#include <linux/efi.h>
 #include <acpi/video.h>
 #include <asm/io.h>
 
@@ -494,6 +495,31 @@ static void gmux_write_switch_state(struct apple_gmux_data *gmux_data)
 		gmux_write8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
 }
 
+#define APPLE_GPU_VARS_GUID EFI_GUID(0xfa4ce28d, 0xb62f, 0x4c99, \
+				     0x9c, 0xc3, 0x68, 0x15, 0x68, 0x6e, 0x30, 0xf9)
+
+static efi_char16_t gmux_gpu_active_name[] = L"gpu-active";
+
+static void gmux_write_switch_state_efi(struct apple_gmux_data *gmux_data)
+{
+	efi_guid_t guid = APPLE_GPU_VARS_GUID;
+	u32 active = gmux_data->switch_state_display == VGA_SWITCHEROO_IGD;
+	efi_status_t status;
+
+	if (!efivar_is_available())
+		return;
+
+	status = efivar_set_variable(gmux_gpu_active_name, &guid,
+				     EFI_VARIABLE_NON_VOLATILE |
+				     EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				     EFI_VARIABLE_RUNTIME_ACCESS,
+				     sizeof(active), &active);
+	if (status != EFI_SUCCESS)
+		pr_warn("writing gpu-active failed: 0x%lx\n", status);
+	else
+		pr_debug("gpu-active: %u\n", active);
+}
+
 static int gmux_switchto(enum vga_switcheroo_client_id id)
 {
 	apple_gmux_data->switch_state_ddc = id;
@@ -502,6 +528,8 @@ static int gmux_switchto(enum vga_switcheroo_client_id id)
 		apple_gmux_data->switch_state_external = id;
 
 	gmux_write_switch_state(apple_gmux_data);
+	if (apple_gmux_data->type == APPLE_GMUX_TYPE_MMIO)
+		gmux_write_switch_state_efi(apple_gmux_data);
 
 	return 0;
 }
@@ -1283,3 +1311,4 @@ MODULE_AUTHOR("Seth Forshee <seth.forshee@canonical.com>");
 MODULE_DESCRIPTION("Apple Gmux Driver");
 MODULE_LICENSE("GPL");
 MODULE_DEVICE_TABLE(pnp, gmux_device_ids);
+MODULE_IMPORT_NS("EFIVAR");
-- 
2.55.0

Re: [PATCH 05/13] platform/apple-gmux: Write switch state to EFI on MMIO devices

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] platform/x86: apple-gmux: NULL deref in gmux_write_switch_state_efi()
- [Medium] platform/apple-gmux: redundant NVRAM writes in gmux_switchto() on runtime suspend
--

commit ef107e78a2e02b388268e04355f8632ba5066eb7
Author: Andre Eikmeyer <dev@deq.rocks>

platform/apple-gmux: Write switch state to EFI on MMIO devices

This commit updates the apple-gmux driver to write the display mux switch
state to the gpu-active EFI variable when switching GPUs on MMIO (T2)
devices.

> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index bef87a57ad43c..637b3b216d4e3 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c

[ ... ]

> @@ -494,6 +495,31 @@ static void gmux_write_switch_state(struct apple_gmux_data *gmux_data)
>  		gmux_write8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
>  }
>  
> +#define APPLE_GPU_VARS_GUID EFI_GUID(0xfa4ce28d, 0xb62f, 0x4c99, \
> +				     0x9c, 0xc3, 0x68, 0x15, 0x68, 0x6e, 0x30, 0xf9)
> +
> +static efi_char16_t gmux_gpu_active_name[] = L"gpu-active";
> +
> +static void gmux_write_switch_state_efi(struct apple_gmux_data *gmux_data)
> +{
> +	efi_guid_t guid = APPLE_GPU_VARS_GUID;
> +	u32 active = gmux_data->switch_state_display == VGA_SWITCHEROO_IGD;
> +	efi_status_t status;
> +
> +	if (!efivar_is_available())
> +		return;
> +
> +	status = efivar_set_variable(gmux_gpu_active_name, &guid,
> +				     EFI_VARIABLE_NON_VOLATILE |
> +				     EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				     EFI_VARIABLE_RUNTIME_ACCESS,
> +				     sizeof(active), &active);

[Severity: High]
Should gmux_write_switch_state_efi() check efivar_supports_writes() instead
of efivar_is_available() before calling efivar_set_variable()?

In drivers/firmware/efi/efi.c:generic_ops_register(), generic_efivars is
registered even when EFI_RT_SUPPORTED_SET_VARIABLE is not supported, leaving
generic_ops.set_variable as NULL while efivar_is_available() returns true:

	if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
		generic_ops.set_variable = efi.set_variable;
		generic_ops.set_variable_nonblocking = efi.set_variable_nonblocking;
	}
	return efivars_register(&generic_efivars, &generic_ops);

When efivar_set_variable() calls efivar_set_variable_locked() in
drivers/firmware/efi/vars.c, setvar is called unconditionally:

	setvar = __efivars->ops->set_variable_nonblocking;
	if (!setvar || !nonblocking)
		 setvar = __efivars->ops->set_variable;

	return setvar(name, vendor, attr, data_size, data);

Can this cause a NULL function pointer dereference via
gmux_switchto() -> gmux_write_switch_state_efi() -> efivar_set_variable() on
systems where efivars is registered without write support?

> +	if (status != EFI_SUCCESS)
> +		pr_warn("writing gpu-active failed: 0x%lx\n", status);
> +	else
> +		pr_debug("gpu-active: %u\n", active);
> +}
> +
>  static int gmux_switchto(enum vga_switcheroo_client_id id)
>  {
>  	apple_gmux_data->switch_state_ddc = id;
> @@ -502,6 +528,8 @@ static int gmux_switchto(enum vga_switcheroo_client_id id)
>  		apple_gmux_data->switch_state_external = id;
>  
>  	gmux_write_switch_state(apple_gmux_data);
> +	if (apple_gmux_data->type == APPLE_GMUX_TYPE_MMIO)
> +		gmux_write_switch_state_efi(apple_gmux_data);

[Severity: Medium]
Will calling gmux_write_switch_state_efi() unconditionally in gmux_switchto()
cause redundant non-volatile NVRAM writes on every discrete GPU runtime
suspend?

In drivers/gpu/vga/vga_switcheroo.c:vga_switcheroo_runtime_suspend(), the
mux handler is called to switch to VGA_SWITCHEROO_IGD regardless of the
current mux state:

	if (vgasr_priv.handler->switchto) {
		mutex_lock(&vgasr_priv.mux_hw_lock);
		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
		mutex_unlock(&vgasr_priv.mux_hw_lock);
	}

When the display is already on VGA_SWITCHEROO_IGD and the discrete GPU goes
through runtime wake and suspend cycles (such as during PRIME offload or
display hotplug), gmux_switchto(VGA_SWITCHEROO_IGD) will write the
non-volatile gpu-active variable to NVRAM even though switch_state_display
did not change.

Would it make sense to skip calling gmux_write_switch_state_efi() when
apple_gmux_data->switch_state_display is already equal to id (while ensuring
the initial state is still synchronized if needed)?

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

[PATCH 06/13] platform/apple-gmux: Add parameter to configure boot gpu

[Atharva Tiwari]

Currently on Indexed and MMIO systems, the system can't change
GPU's on runtime, as the display is not trained on the secondary
GPU, to combat this, we add a module parameter to configure the boot
GPU, so users can change to iGPU or dGPU via reboot.

Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 637b3b216..6a9580f27 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -109,6 +109,10 @@ struct apple_gmux_config {
 	char *name;
 };
 
+static int boot_gpu = -1;
+module_param(boot_gpu, int, 0);
+MODULE_PARM_DESC(boot_gpu, "Configure the boot gpu (-1 = auto, 0 = iGPU, 1 = dGPU)");
+
 #define GMUX_INTERRUPT_ENABLE		0xff
 #define GMUX_INTERRUPT_DISABLE		0x00
 
@@ -1215,6 +1219,11 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	gmux_enable_interrupts(gmux_data, 1);
 	gmux_read_switch_state(gmux_data);
 
+	if (boot_gpu == 0)
+		gmux_switchto(VGA_SWITCHEROO_IGD);
+	else if (boot_gpu == 1)
+		gmux_switchto(VGA_SWITCHEROO_DIS);
+
 	/*
 	 * Retina MacBook Pros cannot switch the panel's AUX separately
 	 * and need eDP pre-calibration. They are distinguishable from
-- 
2.55.0

Re: [PATCH 06/13] platform/apple-gmux: Add parameter to configure boot gpu

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] platform/x86: apple-gmux: live mux switch and state desync in gmux_probe()
- [Low] platform/x86: apple-gmux: missing kernel-parameters.txt entry for boot_gpu
--

commit 67d1397b4ef1b203319bca9aa1d861e7cd7059c6
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

platform/apple-gmux: Add parameter to configure boot gpu

This commit adds a boot_gpu module parameter to drivers/platform/x86/apple-gmux.c
and calls gmux_switchto() during gmux_probe() when boot_gpu is set to 0 or 1.

> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 637b3b216d4e3..6a9580f27b5eb 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -109,6 +109,10 @@ struct apple_gmux_config {
>  	char *name;
>  };
>  
> +static int boot_gpu = -1;
> +module_param(boot_gpu, int, 0);
> +MODULE_PARM_DESC(boot_gpu, "Configure the boot gpu (-1 = auto, 0 = iGPU, 1 = dGPU)");

[Severity: Low]
Should this new module parameter also be documented in
Documentation/admin-guide/kernel-parameters.txt?

Because module_param(boot_gpu, int, 0) sets the sysfs permission to 0,
apple_gmux.boot_gpu= is configured via the kernel command line or module load
options, and kernel command-line parameters are expected to have a
corresponding entry in Documentation/admin-guide/kernel-parameters.txt.

> +
>  #define GMUX_INTERRUPT_ENABLE		0xff
>  #define GMUX_INTERRUPT_DISABLE		0x00
>  
> @@ -1215,6 +1219,11 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	gmux_enable_interrupts(gmux_data, 1);
>  	gmux_read_switch_state(gmux_data);
>  
> +	if (boot_gpu == 0)
> +		gmux_switchto(VGA_SWITCHEROO_IGD);
> +	else if (boot_gpu == 1)
> +		gmux_switchto(VGA_SWITCHEROO_DIS);

[Severity: High]
Does calling gmux_switchto() unconditionally in gmux_probe() switch the live
hardware display mux to an untrained GPU on the current boot and desynchronize
the hardware state from vga_switcheroo?

Looking at gmux_switchto() in drivers/platform/x86/apple-gmux.c:

    static int gmux_switchto(enum vga_switcheroo_client_id id)
    {
        apple_gmux_data->switch_state_ddc = id;
        apple_gmux_data->switch_state_display = id;
        if (apple_gmux_data->external_switchable)
            apple_gmux_data->switch_state_external = id;

        gmux_write_switch_state(apple_gmux_data);
        if (apple_gmux_data->type == APPLE_GMUX_TYPE_MMIO)
            gmux_write_switch_state_efi(apple_gmux_data);

        return 0;
    }

Calling gmux_probe() -> gmux_switchto() -> gmux_write_switch_state()
immediately writes GMUX_PORT_SWITCH_DDC, GMUX_PORT_SWITCH_DISPLAY, and
GMUX_PORT_SWITCH_EXTERNAL. When boot_gpu selects the non-boot GPU, this flips
the live panel mux to the secondary GPU that firmware has not trained on the
current boot, while vga_default_device() remains pointing to the EFI boot GPU.

Because vga_switcheroo_register_client() in drivers/gpu/vga/vga_switcheroo.c
determines the active client from vga_default_device():

    int vga_switcheroo_register_client(struct pci_dev *pdev,
                       const struct vga_switcheroo_client_ops *ops,
                       bool driver_power_control)
    {
        return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID, NULL,
                       pdev == vga_default_device(),
                       driver_power_control);
    }

vga_switcheroo still records the EFI boot GPU as active. If a user writes OFF
to vgaswitcheroo/switch in vga_switcheroo_debugfs_write(), it powers off the
client with !client->active, which is the GPU that gmux_probe() just switched
the display mux to.

Conversely, if the system booted on the iGPU and boot_gpu=1 is passed to
select the discrete GPU for the next reboot, the discrete GPU remains the
inactive client on the current boot. When the discrete GPU runtime-suspends,
vga_switcheroo_runtime_suspend() in drivers/gpu/vga/vga_switcheroo.c runs:

    if (vgasr_priv.handler->switchto) {
        mutex_lock(&vgasr_priv.mux_hw_lock);
        vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
        mutex_unlock(&vgasr_priv.mux_hw_lock);
    }

Wouldn't that call gmux_switchto(VGA_SWITCHEROO_IGD) and overwrite both the
mux state and the gpu-active EFI variable back to the iGPU before the system
reboots?

In addition, gmux_switchto() only calls gmux_write_switch_state_efi() when
apple_gmux_data->type == APPLE_GMUX_TYPE_MMIO, so on APPLE_GMUX_TYPE_INDEXED
systems no EFI variable is written for the next boot. And when the system is
already booted on the GPU matching boot_gpu, calling gmux_switchto()
unconditionally without checking gmux_data->switch_state_display rewrites the
mux ports and the non-volatile gpu-active EFI variable in NVRAM on every boot.

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

[PATCH 07/13] platform/apple-gmux: Add check to detect FSS

[Atharva Tiwari]

On MMIO macs, there is feature called FSS, we assume its
a display syncing technology between internal links
(for example 5K iMacs).

We detect it and then warn the user about it, if its broken/unavailable.

Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 2 ++
 include/linux/apple-gmux.h        | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 6a9580f27..dda281f2f 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -1224,6 +1224,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	else if (boot_gpu == 1)
 		gmux_switchto(VGA_SWITCHEROO_DIS);
 
+	if (type == APPLE_GMUX_TYPE_MMIO && gmux_read8(gmux_data, GMUX_PORT_FSS) & 1)
+		pr_warn("FSS Failure, display tearing expected\n");
 	/*
 	 * Retina MacBook Pros cannot switch the panel's AUX separately
 	 * and need eDP pre-calibration. They are distinguishable from
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
index 206d97ffd..a4fa84500 100644
--- a/include/linux/apple-gmux.h
+++ b/include/linux/apple-gmux.h
@@ -22,6 +22,7 @@
 #define GMUX_PORT_VERSION_RELEASE	0x06
 #define GMUX_PORT_SWITCH_DISPLAY	0x10
 #define GMUX_PORT_SWITCH_GET_DISPLAY	0x11
+#define GMUX_PORT_FSS			0x12
 #define GMUX_PORT_INTERRUPT_ENABLE	0x14
 #define GMUX_PORT_INTERRUPT_STATUS	0x16
 #define GMUX_PORT_SWITCH_DDC		0x28
-- 
2.55.0

[PATCH 08/13] platform/apple-gmux: Add documentation about iMacs

[Atharva Tiwari]

Add documentation about iMacs

Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index dda281f2f..9f0e9ecbf 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -453,6 +453,11 @@ static const struct backlight_ops gmux_bl_ops = {
  * switch the panel and the external DP connector and allocates a framebuffer
  * for the selected GPU.
  *
+ * There is iGPU present on iMac's aswell, and we can enable it using apple-set-os.
+ * But there arent any display lines going from the iGPU to the display. iGPU is only
+ * used for offloading then, it makes sense as iMac's display are massive (4K or 5K).
+ * A integrated GPU won't be able to handle that.
+ *
  * .. _US 8,687,007 B2: https://pimg-fpiw.uspto.gov/fdd/07/870/086/0.pdf
  * .. _NXP CBTL06141:   https://www.nxp.com/documents/data_sheet/CBTL06141.pdf
  * .. _NXP CBTL06142:   https://www.nxp.com/documents/data_sheet/CBTL06141.pdf
-- 
2.55.0

Re: [PATCH 08/13] platform/apple-gmux: Add documentation about iMacs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] platform/x86: apple-gmux: lock leak and uninit return in gmux_mmio_read8/32()
- [High] platform/x86: apple-gmux: NULL discrete_pdev deref in gmux_set_discrete_state()
- [High] platform/x86: apple-gmux: workqueue teardown bugs in gmux_probe/remove()
- [High] PCI: uninitialized output in pci_read_config_*() when access is ignored
- [High] platform/x86: apple-gmux: PCI state corruption in gmux_set_discrete_state()
--

commit 60c5ee5c904bb8e5a78842b541c5dc5de1769e8f
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

platform/apple-gmux: Add documentation about iMacs

This series updates the apple-gmux driver and PCI access layer to support MMIO
gmux power management, interrupts, boot GPU selection, FSS detection, and iMac
documentation.

> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index b123da16b63ba..5e95130671767 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -559,6 +559,9 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
>  
>  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -569,6 +572,9 @@ EXPORT_SYMBOL(pci_read_config_byte);
>  
>  int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -580,6 +586,9 @@ EXPORT_SYMBOL(pci_read_config_word);
>  int pci_read_config_dword(const struct pci_dev *dev, int where,
>  					u32 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;

[Severity: High]
Does returning PCIBIOS_SUCCESSFUL without setting *val in
pci_read_config_byte(), pci_read_config_word(), and pci_read_config_dword()
leave caller stack variables uninitialized when pci_dev_ignore_access(dev) is
true?

Callers such as pci_update_current_state() and pci_clear_and_set_config_dword()
pass an uninitialized stack variable to pci_read_config_word() or
pci_read_config_dword() expecting *val to be populated on return:

pci_update_current_state()
    u16 pmcsr;
    pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
    dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);

When dev->ignore_access_d3 is set and dev->current_state is PCI_D3cold, leaving
pmcsr uninitialized can overwrite dev->current_state and clear PCI_D3cold while
the upstream bridge is still powered down by PWRD(1), allowing subsequent
config accesses to hit the powered-off bridge.

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

[ ... ]

> @@ -244,12 +264,14 @@ static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
>  	u8 val;
>  
>  	mutex_lock(&gmux_data->index_lock);
> -	gmux_mmio_wait(gmux_data);
> -	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> -	iowrite8(GMUX_MMIO_READ | sizeof(val),
> -		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> -	gmux_mmio_wait(gmux_data);
> -	val = ioread8(gmux_data->iomem_base);
> +	if (gmux_mmio_wait(gmux_data)) {
> +		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +		iowrite8(GMUX_MMIO_READ | sizeof(val),
> +			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		if (!gmux_mmio_wait(gmux_data))
> +			return 0;
> +		val = ioread8(gmux_data->iomem_base);
> +	}
>  	mutex_unlock(&gmux_data->index_lock);
>  
>  	return val;

[ ... ]

> @@ -275,12 +300,16 @@ static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int port)
>  	u32 val;
>  
>  	mutex_lock(&gmux_data->index_lock);
> -	gmux_mmio_wait(gmux_data);
> -	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> -	iowrite8(GMUX_MMIO_READ | sizeof(val),
> -		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> -	gmux_mmio_wait(gmux_data);
> -	val = ioread32be(gmux_data->iomem_base);
> +
> +	if (gmux_mmio_wait(gmux_data)) {
> +		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +		iowrite8(GMUX_MMIO_READ | sizeof(val),
> +			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		if (!gmux_mmio_wait(gmux_data))
> +			return 0;
> +
> +		val = ioread32be(gmux_data->iomem_base);
> +	}
>  	mutex_unlock(&gmux_data->index_lock);
>  
>  	return val;

[Severity: High]
In gmux_mmio_read8() and gmux_mmio_read32(), if the second gmux_mmio_wait()
times out and returns 0, won't returning 0 directly leak gmux_data->index_lock
and deadlock subsequent gmux MMIO accesses?

Also, if the first gmux_mmio_wait() call returns 0, the if block is skipped and
both functions return the uninitialized local variable val (which can also be
read from userspace via debugfs in gmux_selected_port_data_read()). Should val
be initialized to 0 and the early return paths unlock gmux_data->index_lock
before returning?

[ ... ]

> @@ -502,23 +565,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() dereference a NULL gmux_data->discrete_pdev here?

gmux_data->discrete_pdev is only assigned in gmux_get_client_id() when
vga_switcheroo_enable() runs after both GPU clients have registered. If the
discrete GPU registers and enters runtime suspend before a second client
registers:

vga_switcheroo_runtime_suspend()
  -> vga_switcheroo_power_switch()
    -> gmux_set_power_state()
      -> gmux_set_discrete_state()

gmux_data->discrete_pdev is still NULL when evaluating
ACPI_HANDLE(&gmux_data->discrete_pdev->dev). In addition, checking
!discrete_handle unconditionally at the start of gmux_set_discrete_state()
will abort with -ENODEV on non-MMIO systems (APPLE_GMUX_TYPE_PIO and
APPLE_GMUX_TYPE_INDEXED) that do not use discrete_handle.

>  	if (state == VGA_SWITCHEROO_ON) {

[ ... ]

>  		pr_debug("Discrete card powered up\n");
>  	} 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;
> +						}
> +					}
> +					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 (where gmux_data->uses_pwrd
is true) when the system suspends and resumes while gmux_data->power_state is
already VGA_SWITCHEROO_OFF?

On system resume, gmux_resume() calls:

gmux_resume()
  -> gmux_set_discrete_state(gmux_data, VGA_SWITCHEROO_OFF)

When the discrete GPU and upstream bridges are already powered off in
PCI_D3cold, calling pci_save_state() and gmux_save_restore_pci(..., true, NULL)
again will overwrite the previously saved PCI configuration state of HDAU,
gmux_data->discrete_pdev, and the upstream bridges with readings from the
powered-down devices before a subsequent VGA_SWITCHEROO_ON can restore them.

[ ... ]

> @@ -823,6 +1101,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  			pr_err("couldn't remap gmux mmio region");
>  			goto err_release;
>  		}
> +		INIT_WORK(&gmux_data->interrupt_work, gmux_notify_work);
>  		goto get_version;
>  	case APPLE_GMUX_TYPE_INDEXED:
>  		gmux_data->config = &apple_gmux_index;

[ ... ]

> @@ -977,6 +1263,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  err_unmap:
>  	if (gmux_data->iomem_base)
>  		iounmap(gmux_data->iomem_base);
> +	if (gmux_data->type == APPLE_GMUX_TYPE_MMIO)
> +		cancel_work_sync(&gmux_data->interrupt_work);
>  err_release:
>  	if (gmux_data->config->resource_type == IORESOURCE_MEM)
>  		release_mem_region(gmux_data->iostart, gmux_data->iolen);
> @@ -993,7 +1281,9 @@ static void gmux_remove(struct pnp_dev *pnp)
>  
>  	gmux_fini_debugfs(gmux_data);
>  	vga_switcheroo_unregister_handler();
> +	pci_dev_put(gmux_data->discrete_pdev);
>  	gmux_disable_interrupts(gmux_data);
> +	cancel_work_sync(&gmux_data->interrupt_work);
>  	if (gmux_data->gpe >= 0) {
>  		acpi_disable_gpe(NULL, gmux_data->gpe);
>  		acpi_remove_notify_handler(gmux_data->dhandle,

[Severity: High]
Are there a few teardown ordering issues with gmux_data->interrupt_work in
gmux_probe() and gmux_remove()?

First, INIT_WORK(&gmux_data->interrupt_work, gmux_notify_work) is only called
in gmux_probe() when type is APPLE_GMUX_TYPE_MMIO, but gmux_remove() calls
cancel_work_sync(&gmux_data->interrupt_work) unconditionally, which triggers
WARN_ON(!work->func) in __flush_work() on PIO and indexed gmux devices.

Second, in the err_unmap path of gmux_probe(), iounmap(gmux_data->iomem_base)
is called before cancel_work_sync(&gmux_data->interrupt_work), so a running
gmux_notify_work() can access gmux_data->iomem_base after it has been unmapped.

Third, in gmux_remove(), pci_dev_put(gmux_data->discrete_pdev) and
cancel_work_sync(&gmux_data->interrupt_work) are called before
acpi_disable_gpe() and acpi_remove_notify_handler(). Can gmux_notify_handler()
reschedule interrupt_work after cancel_work_sync(), or can a running
gmux_notify_work() dereference gmux_data->discrete_pdev after pci_dev_put()?

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

[PATCH 09/13] drm/amdgpu: Add Apple GMUX runtime PM support

[Atharva Tiwari]

From: Andre Eikmeyer <dev@deq.rocks>

The T2 MacBookPro's uses Apple GMUX to control power to the discrete GPU,
but it does not expose the ATPX or ACPI power-resource interfaces currently
recognized by amdgpu. The driver therefore leaves runtime PM disabled even
though vga_switcheroo can switch the GPU reliably.

A dedicated GMUX runtime PM mode uses the existing vga_switcheroo PM domain
to sequence GPU power. A runtime-suspended GMUX GPU can remain powered off
across system sleep, following the smart-suspend handling already used by
BOCO devices.

On muxed systems the inactive GPU cannot reach the panel AUX channel. Like
the macOS driver, tell Display Core that eDP is disconnected before
dc_create() when the GMUX GPU is not the boot display device, and skip eDP
detection at init as the resume path already does. This avoids powering the
panel from the inactive GPU and retaining a fail-safe sink that reports as
connected and keeps the GPU runtime busy.

This allows the discrete GPU to enter D3cold while the integrated GPU is
primary and allows PRIME workloads and external displays to wake it on
demand.

This was tested on MacBookPro15,1, 16,1, 16,4 together
with the corresponding apple-gmux power-sequencing support.

Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    | 55 +++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       | 40 +++++++++-----
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 +++++
 drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h       |  1 +
 5 files changed, 93 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index a9c6f5d4a..6fd607587 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1194,6 +1194,8 @@ int amdgpu_device_mode1_reset(struct amdgpu_device *adev);
 int amdgpu_device_link_reset(struct amdgpu_device *adev);
 bool amdgpu_device_supports_atpx(struct amdgpu_device *adev);
 bool amdgpu_device_supports_px(struct amdgpu_device *adev);
+bool amdgpu_device_supports_gmux(struct amdgpu_device *adev);
+bool amdgpu_device_has_gmux_edp(struct amdgpu_device *adev);
 bool amdgpu_device_supports_boco(struct amdgpu_device *adev);
 bool amdgpu_device_supports_smart_shift(struct amdgpu_device *adev);
 int amdgpu_device_supports_baco(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 9269e780f..c1f196242 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -34,6 +34,7 @@
 #include <linux/slab.h>
 #include <linux/iommu.h>
 #include <linux/pci.h>
+#include <linux/dmi.h>
 #include <linux/pci-p2pdma.h>
 #include <linux/apple-gmux.h>
 #include <linux/nospec.h>
@@ -570,6 +571,45 @@ bool amdgpu_device_supports_px(struct amdgpu_device *adev)
 	return false;
 }
 
+static const struct dmi_system_id amdgpu_gmux_runpm_dmi_table[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,1"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,3"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,4"),
+		},
+	},
+	{ }
+};
+
+bool amdgpu_device_supports_gmux(struct amdgpu_device *adev)
+{
+	return dmi_check_system(amdgpu_gmux_runpm_dmi_table) &&
+	       !dev_is_removable(&adev->pdev->dev) &&
+	       apple_gmux_detect(NULL, NULL);
+}
+
+bool amdgpu_device_has_gmux_edp(struct amdgpu_device *adev)
+{
+	return amdgpu_device_supports_gmux(adev) &&
+	       (dmi_match(DMI_PRODUCT_NAME, "MacBookPro15,3") ||
+		dmi_match(DMI_PRODUCT_NAME, "MacBookPro16,1") ||
+		dmi_match(DMI_PRODUCT_NAME, "MacBookPro16,4"));
+}
+
 /**
  * amdgpu_device_supports_boco - Is the device a dGPU with ACPI power resources
  *
@@ -640,6 +680,9 @@ void amdgpu_device_detect_runtime_pm_mode(struct amdgpu_device *adev)
 			/* enable PX as runtime mode */
 			adev->pm.rpm_mode = AMDGPU_RUNPM_PX;
 			dev_info(adev->dev, "Using ATPX for runtime pm\n");
+		} else if (amdgpu_device_supports_gmux(adev)) {
+			adev->pm.rpm_mode = AMDGPU_RUNPM_GMUX;
+			dev_info(adev->dev, "Using Apple GMUX for runtime pm\n");
 		} else if (amdgpu_device_supports_boco(adev)) {
 			/* enable boco as runtime mode */
 			adev->pm.rpm_mode = AMDGPU_RUNPM_BOCO;
@@ -3767,7 +3810,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 {
 	struct pci_dev *pdev = adev->pdev;
 	int r, i;
-	bool px = false;
+	bool gmux, px = false;
 	u32 max_MBps;
 	int tmp;
 
@@ -4202,12 +4245,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
 
 	px = amdgpu_device_supports_px(adev);
+	gmux = amdgpu_device_supports_gmux(adev);
 
 	if (amdgpu_device_should_register_switcheroo(adev, px))
 		vga_switcheroo_register_client(adev->pdev,
-					       &amdgpu_switcheroo_ops, px);
+					       &amdgpu_switcheroo_ops, px || gmux);
 
-	if (px)
+	if (px || gmux)
 		vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
 
 	adev->pm_nb.notifier_call = amdgpu_device_pm_notifier;
@@ -4336,7 +4380,7 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 {
 	int i;
-	bool px;
+	bool gmux, px;
 
 	amdgpu_device_ip_fini(adev);
 	amdgpu_fence_driver_sw_fini(adev);
@@ -4367,11 +4411,12 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 	adev->xcp_mgr = NULL;
 
 	px = amdgpu_device_supports_px(adev);
+	gmux = amdgpu_device_supports_gmux(adev);
 
 	if (amdgpu_device_should_register_switcheroo(adev, px))
 		vga_switcheroo_unregister_client(adev->pdev);
 
-	if (px)
+	if (px || gmux)
 		vga_switcheroo_fini_domain_pm_ops(adev->dev);
 
 	if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index d9d794a7d..5ad7b6dd8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2494,11 +2494,12 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
 		DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
 
 	if (adev->pm.rpm_mode != AMDGPU_RUNPM_NONE) {
-		/* only need to skip on ATPX */
+		/* ATPX requires a full system-sleep transition. */
 		if (amdgpu_device_supports_px(adev))
 			dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_NO_DIRECT_COMPLETE);
-		/* we want direct complete for BOCO */
-		if (amdgpu_device_supports_boco(adev))
+		/* BOCO and GMUX can remain runtime suspended across system sleep. */
+		if (amdgpu_device_supports_boco(adev) ||
+		    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
 			dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_SMART_PREPARE |
 						DPM_FLAG_SMART_SUSPEND |
 						DPM_FLAG_MAY_SKIP_RESUME);
@@ -2610,7 +2611,9 @@ static int amdgpu_pmops_prepare(struct device *dev)
 	/* Return a positive number here so
 	 * DPM_FLAG_SMART_SUSPEND works properly
 	 */
-	if (amdgpu_device_supports_boco(adev) && pm_runtime_suspended(dev))
+	if ((amdgpu_device_supports_boco(adev) ||
+	     adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX) &&
+	    pm_runtime_suspended(dev))
 		return 1;
 
 	/* if we will not support s3 or s2i for the device
@@ -2881,6 +2884,7 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
 	}
 
 	adev->in_runpm = true;
+	/* GMUX dGPUs drive displays, so hotplug handlers may reopen them. */
 	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
 		drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
 
@@ -2891,7 +2895,8 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
 	 * platforms.
 	 * TODO: this may be also needed for PX capable platform.
 	 */
-	if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO)
+	if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO ||
+	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
 		adev->mp1_state = PP_MP1_STATE_UNLOAD;
 
 	ret = amdgpu_device_prepare(drm_dev);
@@ -2900,16 +2905,19 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
 	ret = amdgpu_device_suspend(drm_dev, false);
 	if (ret) {
 		adev->in_runpm = false;
-		if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO)
+		if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO ||
+		    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
 			adev->mp1_state = PP_MP1_STATE_NONE;
 		return ret;
 	}
 
-	if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO)
+	if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO ||
+	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
 		adev->mp1_state = PP_MP1_STATE_NONE;
 
-	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX) {
-		/* Only need to handle PCI state in the driver for ATPX
+	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
+	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX) {
+		/* Only need to handle PCI state in the driver for ATPX and GMUX.
 		 * PCI core handles it for _PR3.
 		 */
 		amdgpu_device_cache_pci_state(pdev);
@@ -2956,10 +2964,12 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
 	if (!pci_device_is_present(adev->pdev))
 		adev->no_hw_access = true;
 
-	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX) {
-		drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
+	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
+	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX) {
+		if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
+			drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
 
-		/* Only need to handle PCI state in the driver for ATPX
+		/* Only need to handle PCI state in the driver for ATPX and GMUX.
 		 * PCI core handles it for _PR3.
 		 */
 		pci_set_power_state(pdev, PCI_D0);
@@ -2979,12 +2989,14 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
 	}
 	ret = amdgpu_device_resume(drm_dev, false);
 	if (ret) {
-		if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
+		if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
+		    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
 			pci_disable_device(pdev);
 		return ret;
 	}
 
-	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
+	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
+	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
 		drm_dev->switch_power_state = DRM_SWITCH_POWER_ON;
 
 	amdgpu_restore_umd_profile_pstate_after_runpm(adev);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 47a0ff2ed..aee12cd76 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -81,6 +81,7 @@
 #include <linux/moduleparam.h>
 #include <linux/types.h>
 #include <linux/pm_runtime.h>
+#include <linux/vgaarb.h>
 #include <linux/pci.h>
 #include <linux/power_supply.h>
 #include <linux/firmware.h>
@@ -623,6 +624,10 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
 	else
 		init_data.bb_from_dmub = NULL;
 
+	if (amdgpu_device_has_gmux_edp(adev) &&
+	    adev->pdev != vga_default_device())
+		init_data.flags.edp_not_connected = true;
+
 	/* Display Core create. */
 	adev->dm.dc = dc_create(&init_data);
 
@@ -2545,6 +2550,15 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
 			dm->hpd_rx_offload_wq[aconnector->base.index].aconnector =
 				aconnector;
 
+		/* Like the resume path, skip eDP detection without a sink. */
+		if (link->connector_signal == SIGNAL_TYPE_EDP &&
+		    amdgpu_device_has_gmux_edp(adev) &&
+		    !link->edp_sink_present) {
+			drm_info(adev_to_drm(adev), "Skipping detection of inactive GMUX eDP link %d\n",
+				 i);
+			continue;
+		}
+
 		if (!dc_link_detect_connection_type(link, &new_connection_type))
 			drm_err(adev_to_drm(adev), "KMS: Failed to detect connector\n");
 
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
index 8e8a052e1..025fa806d 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
@@ -48,6 +48,7 @@ enum amdgpu_int_thermal_type {
 enum amdgpu_runpm_mode {
 	AMDGPU_RUNPM_NONE,
 	AMDGPU_RUNPM_PX,
+	AMDGPU_RUNPM_GMUX,
 	AMDGPU_RUNPM_BOCO,
 	AMDGPU_RUNPM_BACO,
 	AMDGPU_RUNPM_BAMACO,
-- 
2.55.0

Re: [PATCH 09/13] drm/amdgpu: Add Apple GMUX runtime PM support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/amdgpu: missing thunderbolt check in amdgpu_device_supports_gmux()
- [High] drm/amdgpu: NULL deref on GMUX runtime suspend if handler or iGPU not ready
- [High] drm/amdgpu: broken smart-suspend handling for GMUX in amdgpu_pci_probe()
- [Medium] drm/amdgpu: unconditional GMUX switcheroo runtime PM setup ignores runpm param
--

commit f1610061faecf7abbb15383a149d6591a343aafc
Author: Andre Eikmeyer <dev@deq.rocks>

drm/amdgpu: Add Apple GMUX runtime PM support

This commit adds runtime power management support for discrete GPUs on T2
MacBook Pro models controlled via Apple gmux using the vga_switcheroo power
domain. It also marks inactive gmux eDP panels as disconnected prior to
Display Core initialization so that disconnected internal panels do not hold
the discrete GPU runtime active.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 9269e780feb73..c1f1962425fee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
[ ... ]
> @@ -570,6 +571,45 @@ bool amdgpu_device_supports_px(struct amdgpu_device *adev)
>  	return false;
>  }
>  
> +static const struct dmi_system_id amdgpu_gmux_runpm_dmi_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,1"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,3"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,4"),
> +		},
> +	},
> +	{ }
> +};
> +
> +bool amdgpu_device_supports_gmux(struct amdgpu_device *adev)
> +{
> +	return dmi_check_system(amdgpu_gmux_runpm_dmi_table) &&
> +	       !dev_is_removable(&adev->pdev->dev) &&

[Severity: High]
Should amdgpu_device_supports_gmux() also check
!pci_is_thunderbolt_attached(adev->pdev)?

On MacBook Pro systems, Intel Thunderbolt hierarchies are identified via
pci_is_thunderbolt_attached(adev->pdev) rather than the ExternalFacingPort
property that sets dev_is_removable() (as seen in commit 8587d48d694d and in
amdgpu_device_should_register_switcheroo()):

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_device_should_register_switcheroo() {
	return !pci_is_thunderbolt_attached(adev->pdev) &&
	       (px || (!dev_is_removable(&adev->pdev->dev) &&
		       apple_gmux_detect(NULL, NULL)));
}

Because dmi_check_system() and apple_gmux_detect(NULL, NULL) are host-wide
checks, connecting a Thunderbolt external GPU to a MacBookPro15,1, 15,3,
16,1, or 16,4 causes amdgpu_device_supports_gmux() to return true for the
external GPU while amdgpu_device_should_register_switcheroo() returns false.

In amdgpu_device_init(), this skips vga_switcheroo_register_client() for the
Thunderbolt GPU, but still executes vga_switcheroo_init_domain_pm_ops()
because gmux is true, binding the external GPU to vga_switcheroo's power
domain without registering it as a switcheroo client. In addition,
amdgpu_device_has_gmux_edp() returns true for the external GPU on
MacBookPro15,3, 16,1, and 16,4, causing amdgpu_dm_init() to set
init_data.flags.edp_not_connected = true and
amdgpu_dm_initialize_drm_device() to skip eDP detection on the external GPU.

> +	       apple_gmux_detect(NULL, NULL);
> +}

[Severity: High]
Can this trigger a NULL pointer dereference when the 5-second runtime
autosuspend timer fires after amdgpu_pci_probe() if the apple-gmux handler or
the integrated GPU client has not been registered yet?

The inline helper apple_gmux_detect(NULL, NULL) only checks for hardware
presence in ACPI/PNP and returns true even if the apple-gmux driver
(CONFIG_APPLE_GMUX) has not probed or registered a vga_switcheroo handler.
Because amdgpu_pci_probe() does not call vga_switcheroo_client_probe_defer()
before enabling runtime autosuspend, two failure paths can occur when
vga_switcheroo_runtime_suspend() runs:

1. If apple-gmux has not probed yet (for instance, when built as a module and
not loaded in initramfs, or if its probe failed), vgasr_priv.handler is NULL
when vga_switcheroo_runtime_suspend() dereferences it:

drivers/gpu/vga/vga_switcheroo.c:vga_switcheroo_runtime_suspend() {
	...
	mutex_lock(&vgasr_mutex);
	if (vgasr_priv.handler->switchto) {
		mutex_lock(&vgasr_priv.mux_hw_lock);
		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
		mutex_unlock(&vgasr_priv.mux_hw_lock);
	}
	pci_bus_set_current_state(pdev->bus, PCI_D3cold);
	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);
	mutex_unlock(&vgasr_mutex);
	...
}

2. Even if apple-gmux has registered its handler, vga_switcheroo_enable()
only runs and invokes gmux_get_client_id() (which is the only place that
populates gmux_data->discrete_pdev) once two vga_switcheroo clients have
registered. If i915 has not probed or is disabled when amdgpu autosuspends,
client->id remains VGA_SWITCHEROO_UNKNOWN_ID, and
vga_switcheroo_power_switch() -> gmux_set_power_state() ->
gmux_set_discrete_state() dereferences gmux_data->discrete_pdev while it is
NULL:

drivers/platform/x86/apple-gmux.c:gmux_set_discrete_state() {
	reinit_completion(&gmux_data->powerchange_done);
	acpi_handle discrete_handle = ACPI_HANDLE(&gmux_data->discrete_pdev->dev);
	...
}

[ ... ]
> @@ -4202,12 +4245,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
>  
>  	px = amdgpu_device_supports_px(adev);
> +	gmux = amdgpu_device_supports_gmux(adev);
>  
>  	if (amdgpu_device_should_register_switcheroo(adev, px))
>  		vga_switcheroo_register_client(adev->pdev,
> -					       &amdgpu_switcheroo_ops, px);
> +					       &amdgpu_switcheroo_ops, px || gmux);
>  
> -	if (px)
> +	if (px || gmux)
>  		vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);

[Severity: Medium]
What happens here when the module parameter amdgpu.runpm is set to 0, 1, or 2
on a supported MacBook Pro?

In amdgpu_driver_load_kms(), amdgpu_device_init() runs before
amdgpu_device_detect_runtime_pm_mode() evaluates amdgpu_runtime_pm:

drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c:amdgpu_driver_load_kms() {
	...
	r = amdgpu_device_init(adev, flags);
	if (r) {
		dev_err(dev->dev, "Fatal error during GPU init\n");
		goto out;
	}

	amdgpu_device_detect_runtime_pm_mode(adev);
	...
}

Because amdgpu_device_init() sets driver_power_control to true (px || gmux)
and attaches adev->vga_pm_domain unconditionally whenever
amdgpu_device_supports_gmux(adev) is true:

1. With amdgpu.runpm=0, amdgpu_device_detect_runtime_pm_mode() leaves
adev->pm.rpm_mode as AMDGPU_RUNPM_NONE, so amdgpu_pci_probe() does not enable
runtime power management. At the same time, vga_switchoff() and
vga_switchon() in drivers/gpu/vga/vga_switcheroo.c return early when
client->driver_power_control is true, disabling manual vga_switcheroo power
control as well.

2. With amdgpu.runpm=1 or amdgpu.runpm=2 on an ASIC that reports
BACO_SUPPORT or MACO_SUPPORT, amdgpu_device_detect_runtime_pm_mode() selects
AMDGPU_RUNPM_BACO or AMDGPU_RUNPM_BAMACO while vga_pm_domain remains attached
to adev->dev. On runtime suspend, amdgpu_pmops_runtime_suspend() enters BACO
without saving PCI state or disabling the PCI device, and
vga_switcheroo_runtime_suspend() then cuts power to the GPU and bridge via
gmux.

[ ... ]
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index d9d794a7db964..5ad7b6dd8b2b5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2494,11 +2494,12 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>  		DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
>  
>  	if (adev->pm.rpm_mode != AMDGPU_RUNPM_NONE) {
> -		/* only need to skip on ATPX */
> +		/* ATPX requires a full system-sleep transition. */
>  		if (amdgpu_device_supports_px(adev))
>  			dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_NO_DIRECT_COMPLETE);
> -		/* we want direct complete for BOCO */
> -		if (amdgpu_device_supports_boco(adev))
> +		/* BOCO and GMUX can remain runtime suspended across system sleep. */
> +		if (amdgpu_device_supports_boco(adev) ||
> +		    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
>  			dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_SMART_PREPARE |
>  						DPM_FLAG_SMART_SUSPEND |
>  						DPM_FLAG_MAY_SKIP_RESUME);
> @@ -2610,7 +2611,9 @@ static int amdgpu_pmops_prepare(struct device *dev)
>  	/* Return a positive number here so
>  	 * DPM_FLAG_SMART_SUSPEND works properly
>  	 */
> -	if (amdgpu_device_supports_boco(adev) && pm_runtime_suspended(dev))
> +	if ((amdgpu_device_supports_boco(adev) ||
> +	     adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX) &&
> +	    pm_runtime_suspended(dev))
>  		return 1;

[Severity: High]
Is setting DPM_FLAG_SMART_PREPARE, DPM_FLAG_SMART_SUSPEND, and
DPM_FLAG_MAY_SKIP_RESUME safe for AMDGPU_RUNPM_GMUX when direct_complete is
not used during system sleep or hibernation?

When a gmux discrete GPU is runtime-suspended, amdgpu_pmops_runtime_suspend()
caches PCI state (setting pdev->state_saved = true), sets pdev->current_state
to PCI_D3cold, and sets drm_dev->switch_power_state to
DRM_SWITCH_POWER_DYNAMIC_OFF, while vga_switcheroo_runtime_suspend() cuts
power via apple-gmux (and powers down the PCIe bridge on MacBookPro16,1 and
16,4).

During system sleep or hibernation, direct_complete can be cleared in two
ways, neither of which is handled by vga_pm_domain or the amdgpu system sleep
callbacks:

1. When the companion HDA audio device (linked as a consumer to the discrete
GPU with DL_FLAG_PM_RUNTIME by quirk_gpu_hda()) is bound to snd_hda_intel,
its child PCM devices have .prepare = NULL and .suspend = do_pcm_suspend,
which clears direct_complete up to the HDA PCI device via
dpm_clear_superiors_direct_complete(). In pci_pm_suspend(), because
snd_hda_intel does not set DPM_FLAG_SMART_SUSPEND, pm_runtime_resume() is
called on the HDA device, which via rpm_get_suppliers() runtime-resumes the
gmux GPU during dpm_suspend() after amdgpu_pmops_prepare() already returned 1
and skipped amdgpu_device_prepare(). Then dpm_clear_superiors_direct_complete()
clears direct_complete on the GPU, so amdgpu_pmops_suspend() calls
amdgpu_device_suspend() without amdgpu_device_prepare() having run, and
because vga_pm_domain only implements runtime PM callbacks, gmux never cuts
power to the GPU for system sleep.

2. If direct_complete is false while the gmux GPU stays runtime-suspended
(for example, during hibernation in pci_pm_poweroff(), when
device_may_wakeup() is enabled, when a child amdgpu_bl backlight device whose
class PM ops have .prepare = NULL clears direct_complete, or when
snd_hda_intel is unbound), pci_pm_suspend() and pci_pm_poweroff() evaluate
(!dev_pm_smart_suspend(dev) || pci_dev_need_resume(pci_dev)) as false because
DPM_FLAG_SMART_SUSPEND is set and pci_dev_need_resume() is false for a
non-ACPI PCI device already in PCI_D3cold. They therefore skip
pm_runtime_resume() and invoke amdgpu_pmops_suspend() or
amdgpu_pmops_poweroff() -> amdgpu_device_suspend() while the GPU is powered
off:

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_device_suspend() {
	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
		return 0;
	...
}

Because amdgpu_device_suspend() only checks for DRM_SWITCH_POWER_OFF (and not
DRM_SWITCH_POWER_DYNAMIC_OFF or pm_runtime_suspended(dev)), it proceeds to
access registers on the powered-off GPU. On the subsequent resume path,
pci_pm_resume() (which does not check dev_pm_skip_resume()) sees
pci_dev->state_saved == true from runtime suspend, attempts
pci_restore_standard_config() on the powered-off device, and calls
amdgpu_pmops_resume(), where pci_device_is_present() reads 0xffff, sets
adev->no_hw_access = true, and runs amdgpu_device_resume() while gmux still
has power cut.

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

[PATCH 10/13] drm/amdgpu: Catch register access while GMUX keeps the GPU off

[Atharva Tiwari]

From: Andre Eikmeyer <dev@deq.rocks>

As mentioned previous any access to the GPU when the GMUX has
shut off the rails results in a CPU CATTER, skip and report
any register access after the GPU was runtime suspended for GMUX power-off.

Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |  5 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  5 ++++
 .../gpu/drm/amd/amdgpu/amdgpu_reg_access.c    | 24 +++++++++++++++++++
 4 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 6fd607587..ac8dfc3ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -881,6 +881,8 @@ struct amdgpu_device {
 	/* enable runtime pm on the device */
 	bool                            in_runpm;
 	bool                            has_pr3;
+	/* GMUX has cut power, any register access would hang the PEG link */
+	bool                            gmux_off;
 
 	bool                            ucode_sysfs_en;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c1f196242..8185934b4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -946,6 +946,11 @@ bool amdgpu_device_skip_hw_access(struct amdgpu_device *adev)
 	if (adev->no_hw_access)
 		return true;
 
+	if (unlikely(READ_ONCE(adev->gmux_off))) {
+		WARN_RATELIMIT(1, "%s: register access while powered off by GMUX\n",
+					dev_name(adev->dev));
+		return true;
+	}
 #ifdef CONFIG_LOCKDEP
 	/*
 	 * This is a bit complicated to understand, so worth a comment. What we assert
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 5ad7b6dd8..135798c76 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2932,6 +2932,9 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
 		amdgpu_device_baco_enter(adev);
 	}
 
+	if (adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
+		WRITE_ONCE(adev->gmux_off, true);
+
 	dev_dbg(&pdev->dev, "asic/device is runtime suspended\n");
 
 	return 0;
@@ -2960,6 +2963,8 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
 	if (adev->pm.rpm_mode == AMDGPU_RUNPM_NONE)
 		return -EINVAL;
 
+	WRITE_ONCE(adev->gmux_off, false);
+
 	/* Avoids registers access if device is physically gone */
 	if (!pci_device_is_present(adev->pdev))
 		adev->no_hw_access = true;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c
index 7468855c1..bb5dbeb7f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c
@@ -594,6 +594,9 @@ u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev, u32 reg_addr)
 	void __iomem *pcie_data_offset;
 	u32 r;
 
+	if (amdgpu_device_skip_hw_access(adev))
+		return 0;
+
 	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
 	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
@@ -677,6 +680,12 @@ u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev, u32 reg_addr)
 	void __iomem *pcie_data_offset;
 	u64 r;
 
+	if (amdgpu_device_skip_hw_access(adev))
+		return 0;
+
+	if (amdgpu_device_skip_hw_access(adev))
+		return 0;
+
 	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
 	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
@@ -706,6 +715,9 @@ u64 amdgpu_device_indirect_rreg64_ext(struct amdgpu_device *adev, u64 reg_addr)
 	void __iomem *pcie_data_offset;
 	u64 r;
 
+	if (amdgpu_device_skip_hw_access(adev))
+		return 0;
+
 	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
 	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
 	if ((reg_addr >> 32) && (adev->nbio.funcs->get_pcie_index_hi_offset))
@@ -762,6 +774,9 @@ void amdgpu_device_indirect_wreg(struct amdgpu_device *adev, u32 reg_addr,
 	void __iomem *pcie_index_offset;
 	void __iomem *pcie_data_offset;
 
+	if (amdgpu_device_skip_hw_access(adev))
+		return;
+
 	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
 	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
@@ -784,6 +799,9 @@ void amdgpu_device_indirect_wreg_ext(struct amdgpu_device *adev, u64 reg_addr,
 	void __iomem *pcie_index_hi_offset;
 	void __iomem *pcie_data_offset;
 
+	if (amdgpu_device_skip_hw_access(adev))
+		return;
+
 	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
 	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
 	if ((reg_addr >> 32) && (adev->nbio.funcs->get_pcie_index_hi_offset))
@@ -832,6 +850,9 @@ void amdgpu_device_indirect_wreg64(struct amdgpu_device *adev, u32 reg_addr,
 	void __iomem *pcie_index_offset;
 	void __iomem *pcie_data_offset;
 
+	if (amdgpu_device_skip_hw_access(adev))
+		return;
+
 	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
 	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
@@ -861,6 +882,9 @@ void amdgpu_device_indirect_wreg64_ext(struct amdgpu_device *adev, u64 reg_addr,
 	void __iomem *pcie_index_hi_offset;
 	void __iomem *pcie_data_offset;
 
+	if (amdgpu_device_skip_hw_access(adev))
+		return;
+
 	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
 	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
 	if ((reg_addr >> 32) && (adev->nbio.funcs->get_pcie_index_hi_offset))
-- 
2.55.0

Re: [PATCH 10/13] drm/amdgpu: Catch register access while GMUX keeps the GPU off

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/amdgpu: missing skip_hw_access check in amdgpu_device_indirect_rreg_ext()
--

commit 68a505d63bba52b15e530c1a5a5c68524afaf59d
Author: Andre Eikmeyer <dev@deq.rocks>

drm/amdgpu: Catch register access while GMUX keeps the GPU off

This commit tracks when the GPU is powered off via GMUX during runtime
suspend by setting adev->gmux_off, and skips and reports register accesses in
amdgpu_device_skip_hw_access() and indirect PCIe register access helpers to
prevent hanging the PEG link.

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c
> index 7468855c16a25..bb5dbeb7fa721 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c
> @@ -594,6 +594,9 @@ u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev, u32 reg_addr)
>  	void __iomem *pcie_data_offset;
>  	u32 r;
>  
> +	if (amdgpu_device_skip_hw_access(adev))
> +		return 0;
> +
>  	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
>  	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
>  
> @@ -677,6 +680,12 @@ u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev, u32 reg_addr)
>  	void __iomem *pcie_data_offset;
>  	u64 r;
>  
> +	if (amdgpu_device_skip_hw_access(adev))
> +		return 0;
> +
> +	if (amdgpu_device_skip_hw_access(adev))
> +		return 0;

[Severity: High]
Was one of these duplicate amdgpu_device_skip_hw_access(adev) checks in
amdgpu_device_indirect_rreg64() intended for amdgpu_device_indirect_rreg_ext()
just above it?

Currently amdgpu_device_indirect_rreg_ext() in
drivers/gpu/drm/amd/amdgpu/amdgpu_reg_access.c is the only indirect PCIe
register accessor without this guard:

u32 amdgpu_device_indirect_rreg_ext(struct amdgpu_device *adev, u64 reg_addr)
{
	unsigned long flags, pcie_index, pcie_index_hi, pcie_data;
	u32 r;
	void __iomem *pcie_index_offset;
	void __iomem *pcie_index_hi_offset;
	void __iomem *pcie_data_offset;

	if (unlikely(!adev->nbio.funcs)) {
		...
	}
	...
	spin_lock_irqsave(&adev->reg.pcie.lock, flags);
	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
	...
	writel(reg_addr, pcie_index_offset);
	readl(pcie_index_offset);

If RREG32_PCIE_EXT() -> amdgpu_reg_pcie_ext_rd32() ->
amdgpu_device_indirect_rreg_ext() is called while adev->gmux_off or
adev->no_hw_access is true, won't it still perform MMIO accesses on
adev->rmmio?

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

[PATCH 11/13] drm/amd/display: Refuse DP AUX while GMUX keeps the GPU off

[Atharva Tiwari]

From: Andre Eikmeyer <dev@deq.rocks>

fwupd reads /dev/drm_dp_aux* while the GPU is runtime suspended. The AUX
device holds no runtime PM reference, so the transfer touched registers of
the powered off GPU and caused a CPU CATTER.

Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 7261317d4..b099bf520 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -24,6 +24,7 @@
  *
  */
 
+#include <linux/pm_runtime.h>
 #include <linux/vmalloc.h>
 #include <drm/display/drm_dp_helper.h>
 #include <drm/display/drm_dp_mst_helper.h>
@@ -111,6 +112,20 @@ STATIC_IFN_KUNIT ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
 	if (WARN_ON(msg->size > 16))
 		return -E2BIG;
 
+	ddc = TO_DM_AUX(aux)->ddc_service;
+	adev = ddc->ctx->driver_context;
+
+	/*
+	 * User space reaches this through the drm_dp_aux_dev character device
+	 * without a runtime PM reference. Refuse the transfer while GMUX keeps
+	 * the GPU powered off, a register access then hangs the PEG link.
+	 */
+	pm_runtime_get_noresume(adev->dev);
+	if (READ_ONCE(adev->gmux_off)) {
+		pm_runtime_put_noidle(adev->dev);
+		return -EBUSY;
+	}
+
 	payload.address = msg->address;
 	payload.data = msg->buffer;
 	payload.length = msg->size;
@@ -126,13 +141,13 @@ STATIC_IFN_KUNIT ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
 	result = dc_link_aux_transfer_raw(TO_DM_AUX(aux)->ddc_service, &payload,
 				      &operation_result);
 
+	pm_runtime_put_autosuspend(adev->dev);
+
 	/*
 	 * w/a on certain intel platform where hpd is unexpected to pull low during
 	 * 1st sideband message transaction by return AUX_RET_ERROR_HPD_DISCON
 	 * aux transaction is succuess in such case, therefore bypass the error
 	 */
-	ddc = TO_DM_AUX(aux)->ddc_service;
-	adev = ddc->ctx->driver_context;
 	if (adev->dm.aux_hpd_discon_quirk) {
 		if (msg->address == DP_SIDEBAND_MSG_DOWN_REQ_BASE &&
 			operation_result == AUX_RET_ERROR_HPD_DISCON) {
-- 
2.55.0

Re: [PATCH 11/13] drm/amd/display: Refuse DP AUX while GMUX keeps the GPU off

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/amd/display: unsynchronized runtime PM check in dm_dp_aux_transfer()
- [Medium] drm/amd/display: NULL adev->dev dereference in dm_dp_aux_transfer() KUnit
--

commit 06da2e04484d301378e2740f320dbda5681bdad8
Author: Andre Eikmeyer <dev@deq.rocks>

drm/amd/display: Refuse DP AUX while GMUX keeps the GPU off

This commit adds a runtime PM reference and checks adev->gmux_off in
dm_dp_aux_transfer() to refuse DP AUX transfers while GMUX keeps the GPU
powered off.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 7261317d4c0f4..b099bf5206734 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c

[ ... ]

> @@ -111,6 +112,20 @@ STATIC_IFN_KUNIT ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
>  	if (WARN_ON(msg->size > 16))
>  		return -E2BIG;
>  
> +	ddc = TO_DM_AUX(aux)->ddc_service;
> +	adev = ddc->ctx->driver_context;
> +
> +	/*
> +	 * User space reaches this through the drm_dp_aux_dev character device
> +	 * without a runtime PM reference. Refuse the transfer while GMUX keeps
> +	 * the GPU powered off, a register access then hangs the PEG link.
> +	 */
> +	pm_runtime_get_noresume(adev->dev);

[Severity: Medium]
Can this dereference a NULL adev->dev pointer when running the
amdgpu_dm_mst_types_test KUnit suite with CONFIG_PM enabled?

In drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c,
the KUnit test cases (such as dm_mst_test_aux_transfer_native_read() and
dm_mst_test_aux_transfer_native_write()) allocate a mock struct
amdgpu_device via kunit_kzalloc() and wire it through
dm_mst_test_setup_dm_aux() without initializing adev->dev:

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c:
dm_mst_test_aux_transfer_native_read() {
	...
	dm_aux = kunit_kzalloc(test, sizeof(*dm_aux), GFP_KERNEL);
	adev = kunit_kzalloc(test, sizeof(*adev), GFP_KERNEL);
	...
	dm_mst_test_setup_dm_aux(dm_aux, ddc, link, dc, link_srv, ctx, adev);

	ret = drm_dp_dpcd_read(&dm_aux->aux, 4, buffer, sizeof(buffer));
	...
}

When drm_dp_dpcd_read() invokes dm_dp_aux_transfer(), passing adev->dev
(which is NULL) to pm_runtime_get_noresume(), pm_runtime_put_noidle(), or
pm_runtime_put_autosuspend() will dereference a NULL pointer.

> +	if (READ_ONCE(adev->gmux_off)) {
> +		pm_runtime_put_noidle(adev->dev);
> +		return -EBUSY;
> +	}
> +
>  	payload.address = msg->address;
>  	payload.data = msg->buffer;
>  	payload.length = msg->size;
> @@ -126,13 +141,13 @@ STATIC_IFN_KUNIT ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
>  	result = dc_link_aux_transfer_raw(TO_DM_AUX(aux)->ddc_service, &payload,
>  				      &operation_result);
>  
> +	pm_runtime_put_autosuspend(adev->dev);

[Severity: High]
Does pm_runtime_get_noresume() combined with checking adev->gmux_off prevent
dm_dp_aux_transfer() from accessing hardware registers while the GPU is
suspending, resuming, or runtime-suspended under other runtime PM modes?

In dm_dp_aux_transfer(), pm_runtime_get_noresume(adev->dev) only increments
dev->power.usage_count without acquiring dev->power.lock, checking
dev->power.runtime_status, or waiting on an in-flight RPM_SUSPENDING or
RPM_RESUMING transition.

In rpm_suspend(), the PM core checks usage_count before setting
runtime_status to RPM_SUSPENDING and invoking the driver callback. In
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c, amdgpu_pmops_runtime_suspend() only
sets adev->gmux_off at the very end of the function, after
amdgpu_device_suspend(), pci_disable_device(), and
pci_set_power_state(pdev, PCI_D3cold) have already run:

drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:amdgpu_pmops_runtime_suspend() {
	...
		amdgpu_device_cache_pci_state(pdev);
		pci_disable_device(pdev);
		pci_ignore_hotplug(pdev);
		pci_set_power_state(pdev, PCI_D3cold);
		drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
	} else if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO) {
		/* nothing to do */
	} else if ((adev->pm.rpm_mode == AMDGPU_RUNPM_BACO) ||
			(adev->pm.rpm_mode == AMDGPU_RUNPM_BAMACO)) {
		amdgpu_device_baco_enter(adev);
	}

	if (adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
		WRITE_ONCE(adev->gmux_off, true);
	...
}

And in amdgpu_pmops_runtime_resume(), adev->gmux_off is cleared at the very
beginning of the callback before PCI state is restored to PCI_D0,
pci_enable_device() is called, and amdgpu_device_resume() reinitializes the
device:

drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:amdgpu_pmops_runtime_resume() {
	...
	WRITE_ONCE(adev->gmux_off, false);

	/* Avoids registers access if device is physically gone */
	if (!pci_device_is_present(adev->pdev))
		adev->no_hw_access = true;

	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX) {
		if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
			drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;

		/* Only need to handle PCI state in the driver for ATPX and GMUX.
		 * PCI core handles it for _PR3.
		 */
		pci_set_power_state(pdev, PCI_D0);
		amdgpu_device_load_pci_state(pdev);
		ret = pci_enable_device(pdev);
	...
}

If userspace reads /dev/drm_dp_aux* concurrently with
amdgpu_pmops_runtime_suspend() or amdgpu_pmops_runtime_resume(),
dm_dp_aux_transfer() can observe adev->gmux_off == false and call
dc_link_aux_transfer_raw() while the PCI device is disabled or in
PCI_D3cold, or while vga_switcheroo_runtime_suspend() is cutting GMUX power.

In addition, on non-GMUX runtime PM configurations (such as
AMDGPU_RUNPM_PX, AMDGPU_RUNPM_BOCO, AMDGPU_RUNPM_BACO, or
AMDGPU_RUNPM_BAMACO), adev->gmux_off remains false while the GPU is runtime
suspended, and pm_runtime_get_noresume() will not wake the device before
dc_link_aux_transfer_raw() accesses AUX registers.

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

[PATCH 12/13] drm/amdgpu: Skip AC/DC notification while the GPU is runtime suspended

[Atharva Tiwari]

From: Andre Eikmeyer <dev@deq.rocks>

Plugging or unplugging the charger raises an ACPI AC event. The handler
sent the new power source to the SMU without a runtime PM reference, so
with GMUX keeping the GPU off every SMU message spun on skipped register
reads for seconds. A runtime resume racing with that left the SMU
firmware unable to start on MacBookPro15,1 and the GPU in runtime PM
error state. Record the power source only and let the resume apply it.

Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
 drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
index 808be6c42..b64f0b681 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
@@ -30,6 +30,7 @@
 #include "amd_pcie.h"
 #include "amdgpu_display.h"
 #include "hwmgr.h"
+#include <linux/pm_runtime.h>
 #include <linux/power_supply.h>
 #include "amdgpu_smu.h"
 
@@ -498,20 +499,26 @@ int amdgpu_dpm_smu_i2c_bus_access(struct amdgpu_device *adev,
 
 void amdgpu_pm_acpi_event_handler(struct amdgpu_device *adev)
 {
+	int active = 0;
+
 	if (adev->pm.dpm_enabled) {
 		mutex_lock(&adev->pm.mutex);
+		active = adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX ?
+				pm_runtime_get_if_active(adev->dev) : 1;
+
 		if (power_supply_is_system_supplied() > 0)
 			adev->pm.ac_power = true;
 		else
 			adev->pm.ac_power = false;
 
-		if (adev->powerplay.pp_funcs &&
-		    adev->powerplay.pp_funcs->notify_ac_dc)
-			amdgpu_dpm_notify_ac_dc(adev);
-
-		if (is_support_sw_smu(adev))
-			smu_set_ac_dc(adev->powerplay.pp_handle, true);
+		if (active) {
+			if (adev->powerplay.pp_funcs &&
+			    adev->powerplay.pp_funcs->notify_ac_dc)
+				amdgpu_dpm_notify_ac_dc(adev);
 
+			if (is_support_sw_smu(adev))
+				smu_set_ac_dc(adev->powerplay.pp_handle, true);
+		}
 		mutex_unlock(&adev->pm.mutex);
 	}
 }
-- 
2.55.0

Re: [PATCH 12/13] drm/amdgpu: Skip AC/DC notification while the GPU is runtime suspended

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/amdgpu: runtime PM reference leak in amdgpu_pm_acpi_event_handler()
- [High] drm/amdgpu: negative return treated as active in amdgpu_pm_acpi_event_handler()
- [Medium] drm/amdgpu: missed SMU AC/DC update in amdgpu_pm_acpi_event_handler() during resume
--

commit 5f8266206967cf1a10c12c800fc318910f1e455a
Author: Andre Eikmeyer <dev@deq.rocks>

drm/amdgpu: Skip AC/DC notification while the GPU is runtime suspended

This commit updates amdgpu_pm_acpi_event_handler() to check if a GMUX GPU is
runtime active via pm_runtime_get_if_active() before sending AC/DC power
source notifications to the SMU. When the GPU is suspended, it records the
updated power state in adev->pm.ac_power so it can be applied upon resume.

> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> index 808be6c425bff..b64f0b681fe1a 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c

[ ... ]

> @@ -498,20 +499,26 @@ int amdgpu_dpm_smu_i2c_bus_access(struct amdgpu_device *adev,
>  
>  void amdgpu_pm_acpi_event_handler(struct amdgpu_device *adev)
>  {
> +	int active = 0;
> +
>  	if (adev->pm.dpm_enabled) {
>  		mutex_lock(&adev->pm.mutex);
> +		active = adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX ?
> +				pm_runtime_get_if_active(adev->dev) : 1;

[Severity: Medium]
Can an AC/DC notification be missed if an ACPI event races with runtime
resume?

During rpm_resume() -> amdgpu_pmops_runtime_resume() ->
amdgpu_device_resume(), dev->power.runtime_status remains RPM_RESUMING until
amdgpu_pmops_runtime_resume() returns.

Inside amdgpu_device_resume(), adev->pm.ac_power is consumed without holding
adev->pm.mutex during amdgpu_device_ip_resume() -> pp_resume() ->
hwmgr_resume() on PowerPlay GPUs and during amdgpu_device_ip_late_init() ->
smu_late_init() on swsmu GPUs:

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_device_resume() {
	...
	r = amdgpu_device_ip_resume(adev);
	...
	r = amdgpu_device_ip_late_init(adev);
	if (r)
		goto exit;

	queue_delayed_work(system_dfl_wq, &adev->delayed_init_work,
			   msecs_to_jiffies(AMDGPU_RESUME_MS));
exit:
	...
	/* Make sure IB tests flushed */
	flush_delayed_work(&adev->delayed_init_work);
}

If amdgpu_pm_acpi_event_handler() runs after IP resume/late_init has already
applied the power state, while flush_delayed_work() or the remainder of
amdgpu_pmops_runtime_resume() is still executing, pm_runtime_get_if_active()
sees runtime_status == RPM_RESUMING and returns 0. Would that cause the
handler to skip notifying the SMU after the resume path has already passed
the point where adev->pm.ac_power is applied, leaving the GPU with stale
AC/DC limits?

> +
>  		if (power_supply_is_system_supplied() > 0)
>  			adev->pm.ac_power = true;
>  		else
>  			adev->pm.ac_power = false;
>  
> -		if (adev->powerplay.pp_funcs &&
> -		    adev->powerplay.pp_funcs->notify_ac_dc)
> -			amdgpu_dpm_notify_ac_dc(adev);
> -
> -		if (is_support_sw_smu(adev))
> -			smu_set_ac_dc(adev->powerplay.pp_handle, true);
> +		if (active) {

[Severity: High]
Should this check be (active > 0) instead of (active)?

pm_runtime_get_if_active() returns 1 when the device is active, 0 when it is
not active, and -EINVAL without incrementing dev->power.usage_count when
runtime PM is disabled (dev->power.disable_depth > 0 or !CONFIG_PM).

On GMUX systems, amdgpu_pci_probe() sets DPM_FLAG_SMART_PREPARE |
DPM_FLAG_SMART_SUSPEND | DPM_FLAG_MAY_SKIP_RESUME, which allows a
runtime-suspended GMUX GPU (powered off, with adev->pm.dpm_enabled still true
on PowerPlay GPUs) to remain suspended across system sleep while the PM core
increments dev->power.disable_depth during __device_suspend_late() through
dpm_resume_early().

If an ACPI AC/DC event runs while disable_depth > 0, active will be -EINVAL
and if (active) will evaluate to true, calling amdgpu_dpm_notify_ac_dc() or
smu_set_ac_dc() without a runtime PM reference while the GPU may be powered
off.

> +			if (adev->powerplay.pp_funcs &&
> +			    adev->powerplay.pp_funcs->notify_ac_dc)
> +				amdgpu_dpm_notify_ac_dc(adev);
>  
> +			if (is_support_sw_smu(adev))
> +				smu_set_ac_dc(adev->powerplay.pp_handle, true);
> +		}

[Severity: High]
Does amdgpu_pm_acpi_event_handler() leak the runtime PM usage reference on
adev->dev when the GPU is active?

When adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX and the GPU is in the RPM_ACTIVE
state, pm_runtime_get_if_active(adev->dev) increments dev->power.usage_count
and returns 1. Because there is no matching pm_runtime_put_autosuspend() or
pm_runtime_put() call after notifying the SMU, won't each ACPI AC/DC event
while the GPU is active permanently leak a runtime PM reference and prevent
the GPU from runtime suspending again?

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

[PATCH 13/13] drm/amd/display: Skip panel power savings on eDP links without a panel

[Atharva Tiwari]

From: Andre Eikmeyer <dev@deq.rocks>

When GMUX routes the internal panel to the integrated GPU, the eDP link
of the AMD GPU has no sink. It still exposed panel_power_savings, and
tuned writes it on every switch between AC and battery. Each write sends
a hotplug event and the following connector probe runtime resumes the
GPU that GMUX had powered off. Only create the attribute when the link
has a panel.

Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
index 703d0617d..c8fbd2167 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
@@ -761,6 +761,9 @@ amdgpu_dm_should_create_sysfs(struct amdgpu_dm_connector *amdgpu_dm_connector)
 	if (amdgpu_dm_connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
 		return false;
 
+	if (!amdgpu_dm_connector->dc_link->edp_sink_present)
+		return false;
+
 	if (link->panel_type != PANEL_TYPE_LCD && !link->panel_config.cacp.cacp_supported)
 		return false;
 
-- 
2.55.0

Re: [PATCH 13/13] drm/amd/display: Skip panel power savings on eDP links without a panel

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/amd/display: KUnit test failures in amdgpu_dm_should_create_sysfs()
--

commit 7217c1895d0e9a0b0a4343bbfd37bba3770b7920
Author: Andre Eikmeyer <dev@deq.rocks>

drm/amd/display: Skip panel power savings on eDP links without a panel

This commit updates amdgpu_dm_should_create_sysfs() to return false when
dc_link->edp_sink_present is false, preventing creation of the
panel_power_savings sysfs attribute on eDP links without an attached panel.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> index 703d0617df6d4..c8fbd2167b6b2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> @@ -761,6 +761,9 @@ amdgpu_dm_should_create_sysfs(struct amdgpu_dm_connector *amdgpu_dm_connector)
>  	if (amdgpu_dm_connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
>  		return false;
>  
> +	if (!amdgpu_dm_connector->dc_link->edp_sink_present)
> +		return false;

[Severity: Medium]
Does this break the existing KUnit tests for amdgpu_dm_should_create_sysfs()
in drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c?

In setup_test_connector(), fixture->link is zero-allocated with
kunit_kzalloc(), leaving edp_sink_present set to false:

	fixture->link = kunit_kzalloc(test, sizeof(*fixture->link), GFP_KERNEL);
	...
	fixture->aconnector->bl_idx = bl_idx;
	fixture->aconnector->dc_link = fixture->link;
	fixture->aconnector->base.dev = &fixture->adev->ddev;
	fixture->link->connector_signal = signal;

Because edp_sink_present is never initialized to true in
setup_test_connector() or in the test cases, amdgpu_dm_should_create_sysfs()
now returns false unconditionally in:

- dm_test_should_create_sysfs_no_backlight_index()
- dm_test_should_create_sysfs_oled_cacp()
- dm_test_should_create_sysfs_lcd_panel()

causing their KUNIT_EXPECT_TRUE() assertions to fail, while
dm_test_should_create_sysfs_oled_no_cacp() returns early at the
edp_sink_present check before testing the panel_type and cacp_supported
condition.

Should setup_test_connector() or those KUnit tests be updated to set
edp_sink_present = true?

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