Re: [PATCH 05/10] drm/amdgpu: Implement struct vga_switcheroo_client_ops.pre_switch

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 07.07.26 um 16:22 schrieb sashiko-bot@kernel.org:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] An AB-BA lock inversion is introduced between `vgasr_mutex` and `dev->clientlist_mutex`, leading to a potential deadlock.
> --
>
> --- Patch [5]: [PATCH 05/10] drm/amdgpu: Implement struct vga_switcheroo_client_ops.pre_switch ---
> commit 31dd42c0f56cf03f20324c5c0e3557fa630afd13
> Author: Thomas Zimmermann <tzimmermann@suse.de>
>
> drm/amdgpu: Implement struct vga_switcheroo_client_ops.pre_switch
>
> This commit implements the pre_switch callback for vga_switcheroo_client_ops
> in the amdgpu driver. It calls drm_client_dev_acquire_outputs() to push fbcon
> updates from vga_switcheroo into DRM's fbdev emulation.
>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 211d30f03d25f..6c80c6f4b07ca 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -1698,10 +1698,18 @@ static bool amdgpu_switcheroo_can_switch(struct pci_dev *pdev)
>>   	return atomic_read(&dev->open_count) == 0;
>>   }
>>   
>> +static void amdgpu_switcheroo_pre_switch(struct pci_dev *pdev)
>> +{
>> +	struct drm_device *dev = pci_get_drvdata(pdev);
>> +
>> +	drm_client_dev_acquire_outputs(dev);
> [Severity: High]
> Can this introduce an AB-BA lock inversion between vgasr_mutex and
> dev->clientlist_mutex?
>
> This callback is invoked by vga_switcheroo while vgasr_mutex is held, and
> drm_client_dev_acquire_outputs() then acquires dev->clientlist_mutex.
>
> Meanwhile, the hotplug path acquires clientlist_mutex first:
>
> drm_client_dev_hotplug()
>    mutex_lock(&dev->clientlist_mutex);
>
> and then eventually takes console_lock and vgasr_mutex (via
> vga_switcheroo_client_fb_set() called from register_framebuffer()) deeper in
> the call stack.

Patch 10 removes calls to vga_switcheroo_client_fb_set() entirely. I 
could try to reorganize the series to avoid the vulnerable intermediate 
states.

>
> If a hotplug event and a VGA switcheroo switch execute concurrently, could this
> lead to a deadlock?
>
>> +}
>> +
>>   static const struct vga_switcheroo_client_ops amdgpu_switcheroo_ops = {
>>   	.set_gpu_state = amdgpu_switcheroo_set_state,
>>   	.reprobe = NULL,
>>   	.can_switch = amdgpu_switcheroo_can_switch,
>> +	.pre_switch = amdgpu_switcheroo_pre_switch,
>>   };

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)