Hi Jason, On 24/04/2026 19:30, Jason Gunthorpe wrote:
On Thu, Apr 16, 2026 at 06:17:50AM -0700, Matt Evans wrote:+ + dma_resv_lock(priv->dmabuf->resv, NULL); vdev = READ_ONCE(priv->vdev); + if (READ_ONCE(priv->revoked) || !vdev) {Why is this read once? It is inside the resv lock so it is stable?+ pr_debug_ratelimited("%s VA 0x%lx, pgoff 0x%lx: DMABUF revoked/cleaned up\n", + __func__, vmf->address, vma->vm_pgoff); + dma_resv_unlock(priv->dmabuf->resv); + return VM_FAULT_SIGBUS; + } + /* vdev is usable */ + + if (!vfio_device_try_get_registration(&vdev->vdev)) { + /* + * If vdev != NULL (above), the registration should + * already be >0 and so this try_get should never + * fail. + */ + dev_warn(&vdev->pdev->dev, "%s: Unexpected registration failure\n", + __func__); + dma_resv_unlock(priv->dmabuf->resv); + return VM_FAULT_SIGBUS; + } + dma_resv_unlock(priv->dmabuf->resv); + scoped_guard(rwsem_read, &vdev->memory_lock) { - if (!priv->revoked) { + if (!READ_ONCE(priv->revoked)) {Same here, it is not read once since you hold the memory_lock it is stable.
I used them more as an 'eyecatcher' to complement the comment. Although they're not strictly required (compiler barriers at the lock/unlocks), they stand out to say "something's going on here".
Revoked/status is read first holding resv, and _must be read a second time_ once that's released and after memory_lock is taken, so two READ_ONCEs seemed appropriate to show this.
But if you feel strongly, sure, I can remove it (though I'd add, say, a /* Re-read status value */ comment).
Matt
