Re: [PATCH v3] drm/amdgpu: hold a runtime PM reference for P2P dma-buf attachments

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect handling of negative return values from pm_runtime_get_if_active() causes a runtime PM usage count underflow.
--

commit ea3c97ff59ae3ca0f3d8bdf4b2a1ca3b95cda6f8
Author: Mike Lothian <mike@fireburn.co.uk>
drm/amdgpu: hold a runtime PM reference for P2P dma-buf attachments

This commit takes a runtime PM reference in amdgpu_dma_buf_attach() and drops
it in amdgpu_dma_buf_detach() to prevent P2P dma-buf attachments from falling
back to GTT when the exporting device is suspended. It uses
pm_runtime_get_if_active() to avoid deadlocks against the resume path.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index b33c300e26e2e..fae695c3e5315 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
[ ... ]
> @@ -100,15 +101,50 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf,
>  	    pci_p2pdma_distance(adev->pdev, attach->dev, false) < 0)
>  		attach->peer2peer = false;
>  
> +	/*
> +	 * P2P access needs the exporter awake for the lifetime of the
> +	 * attachment.  pm_runtime_get_if_active() never resumes the device,
> +	 * so it cannot deadlock against the reservation taken during resume.
> +	 * A negative return means runtime PM is disabled and the device
> +	 * cannot suspend, in which case the put in detach is a no-op.
> +	 */
> +	if (attach->peer2peer &&
> +	    !pm_runtime_get_if_active(adev_to_drm(adev)->dev))
> +		attach->peer2peer = false;

[Severity: High]
The comment states that the put in detach is a no-op, but is this accurate?

pm_runtime_get_if_active() can return a negative error code (such as -EINVAL
when runtime PM is disabled). In that case, !pm_runtime_get_if_active()
evaluates to false, bypassing the attach->peer2peer = false assignment.

Since the reference was not actually acquired, but attach->peer2peer remains
true, wouldn't the unbalanced pm_runtime_put_autosuspend() calls in
err_pm_put and amdgpu_dma_buf_detach() cause a PM usage count underflow
rather than acting as a no-op?

> +
>  	r = dma_resv_lock(bo->tbo.base.resv, NULL);
>  	if (r)
> -		return r;
> +		goto err_pm_put;
>  
>  	amdgpu_vm_bo_update_shared(bo);
>  
>  	dma_resv_unlock(bo->tbo.base.resv);
>  
>  	return 0;
> +
> +err_pm_put:
> +	if (attach->peer2peer)
> +		pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
> +	return r;
> +}
> +
> +/**
> + * amdgpu_dma_buf_detach - &dma_buf_ops.detach implementation
> + *
> + * @dmabuf: DMA-buf where we remove the attachment from
> + * @attach: the attachment to remove
> + *
> + * Drop the runtime PM reference taken in amdgpu_dma_buf_attach().
> + */
> +static void amdgpu_dma_buf_detach(struct dma_buf *dmabuf,
> +				  struct dma_buf_attachment *attach)
> +{
> +	struct drm_gem_object *obj = dmabuf->priv;
> +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> +	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> +
> +	if (attach->peer2peer)
> +		pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911183825.935140-1-mike@fireburn.co.uk?part=1