Re: [PATCH 03/10] vga_switcheroo: Add pre_switch callback to client ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Duplicate and conflicting kernel-doc entry for `@gpu_bound` instead of `@pre_switch`.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but if the hardware mux switch fails, the active client is permanently marked as inactive, breaking the vga_switcheroo state machine.
--

--- Patch [3]: [PATCH 03/10] vga_switcheroo: Add pre_switch callback to client ops ---
commit 5537cd5fab2c2375821fd4433e71171511ada8ff
Author: Thomas Zimmermann <tzimmermann@suse.de>

vga_switcheroo: Add pre_switch callback to client ops

This commit adds a pre_switch callback to vga_switcheroo_client_ops to
inform clients about upcoming output switches, replacing the hard-coded
fb_switch_outputs() call.

> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 22cf52b78b755..bdf1e56ae8910 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -733,8 +733,10 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
>  	if (!active->driver_power_control)
>  		set_audio_state(active->id, VGA_SWITCHEROO_OFF);
>  
> +	if (new_client->ops->pre_switch)
> +		new_client->ops->pre_switch(new_client->pdev);
>  #if defined(CONFIG_FB)
> -	if (new_client->fb_info)
> +	else if (new_client->fb_info)
>  		fb_switch_outputs(new_client->fb_info);
>  #endif
>  

[Severity: High]
This is a pre-existing issue, but does vga_switchto_stage2() leave the
state machine corrupted if the hardware mux switch fails?

Earlier in vga_switchto_stage2(), the active client is marked as inactive:

	active->active = false;

If the handler's switchto() callback fails shortly after this patch's
hunk:

	ret = vgasr_priv.handler->switchto(new_client->id);
	mutex_unlock(&vgasr_priv.mux_hw_lock);
	if (ret)
		return ret;

Will returning early here leave the original active client permanently
marked as inactive? Since find_active_client() relies on this flag, it
appears subsequent switch commands would silently abort because they
can no longer find an active client. Should the active flag be restored
in this error path?

> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index 7e6ac0114d556..d2ce30d8b6f78 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -127,23 +127,29 @@ struct vga_switcheroo_handler {
>   * @set_gpu_state: do the equivalent of suspend/resume for the card.
>   * 	Mandatory. This should not cut power to the discrete GPU,
>   * 	which is the job of the handler
> + * @gpu_bound: Optional. Called before switching the outputs to the device.

[Severity: Low]
This isn't a bug, but should this be named @pre_switch instead of
@gpu_bound? 

It looks like @gpu_bound is already defined further down in this struct's
documentation, and this new description matches the purpose of the new
pre_switch callback.

>   * @reprobe: poll outputs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707135724.247562-1-tzimmermann@suse.de?part=3