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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] platform/x86: apple-gmux: premature teardown in gmux_shutdown()
- [Medium] platform/x86: apple-gmux: unconditional EFI write in
  gmux_shutdown()
- [Medium] platform/x86: apple-gmux: redundant NVRAM writes in
  gmux_switchto()
- [Low] platform/x86: apple-gmux: malformed Co-developed trailer in
  commit
--

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

> platform/apple-gmux: Write switch state to EFI on MMIO devices
>
> 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.
>
> Tested-by: Sebastian Zygadlo <szgit@icloud.com>
> Tested-by: Yossef Osman <yossef.osman@proton.me>
> Co-developed: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
> Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
> Signed-off-by: Andre Eikmeyer <dev@deq.rocks>

[Severity: Low]
This isn't a bug, but the Co-developed: trailer should be
Co-developed-by:.

> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 37ba207793a2e..c4a953b5b3e2d 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c

[ ... ]

> @@ -489,6 +490,32 @@ 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() ||
> +			!efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE))
> +		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;
> @@ -497,6 +524,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;
>  }

[Severity: Medium]
Does gmux_switchto() write the non-volatile gpu-active EFI variable to
NVRAM even when switch_state_display has not changed?

Whenever the discrete GPU runtime-suspends,
vga_switcheroo_runtime_suspend() calls
vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD) regardless of the
current display mux state.  Calling efivar_set_variable() with
EFI_VARIABLE_NON_VOLATILE when switch_state_display is already id causes
redundant NVRAM flash wear and extra runtime suspend latency on every
discrete GPU wake/suspend cycle.

> @@ -1240,6 +1269,15 @@ static void gmux_remove(struct pnp_dev *pnp)
>  	kfree(gmux_data);
>  }
>  
> +static void gmux_shutdown(struct pnp_dev *pnp)
> +{
> +	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
> +
> +	gmux_write_switch_state_efi(gmux_data);

[Severity: Medium]
Should gmux_shutdown() check gmux_data->type == APPLE_GMUX_TYPE_MMIO
before calling gmux_write_switch_state_efi(gmux_data), matching
gmux_switchto()?  Without that check, every shutdown or reboot on
non-MMIO (PIO and indexed) gmux systems writes the T2-specific
gpu-active variable to NVRAM.

> +
> +	gmux_remove(pnp);
> +}

[Severity: High]
Can calling gmux_remove(pnp) from gmux_shutdown() cause a NULL pointer
dereference when pci_device_shutdown() runs for the discrete GPU?

PCI devices are registered in subsys_initcall(acpi_init), before PNPACPI
devices are registered in fs_initcall(pnpacpi_init).  During shutdown,
device_shutdown() walks devices_kset in reverse registration order, so
pnp_device_shutdown() -> gmux_shutdown() -> gmux_remove() runs before
pci_device_shutdown() on the discrete GPU.

gmux_remove() calls vga_switcheroo_unregister_handler(), which clears
vgasr_priv.handler to NULL, and frees gmux_data.  When
pci_device_shutdown() then calls pm_runtime_resume() on a
runtime-suspended discrete GPU, vga_switcheroo_runtime_resume() ->
vga_switcheroo_power_switch() dereferences vgasr_priv.handler:

drivers/gpu/vga/vga_switcheroo.c:vga_switcheroo_power_switch() {
	...
	if (!vgasr_priv.handler->power_state)
		return;
	...
}

In addition, pnp_device_shutdown() does not unbind the driver, so
freeing gmux_data here leaves a dangling drvdata pointer in pnp->data
that will be used and freed again if pnp_device_remove() is called.

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