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

[sashiko-bot@kernel.org]

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