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