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