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