On 8/28/26 17:59, Zhu, Lingshan wrote: > On 8/28/2026 9:08 PM, Christian König wrote: > >> On 8/28/26 11:53, Zhu Lingshan wrote: >>> This commit introduces a new helper >>> amdgpu_lookup_queue_by_doorbell which helps >>> look up a user queue with the given doorbell id >>> in a xarray. >>> >>> This function takes a kref of the user space queue. > > Hello Christian > > Thanks for your comments. > >> Well absolutely clear NAK to the whole approach. >> >> This is the nonsense Sunil and I have worked quite hard to remove and we >> certainly shouldn't repeat such mistakes. >> >> When the userq needs to be used from interrupt context we need to hold the >> xa_lock_irqsave() or otherwise we don't have any guarantee that the userq, >> userq_mgr or associated fpriv went out of scope. > > Holding the spin lock by xa_lock_irqsave() can surely avoid racing with the > destruction process, however, it does not apply to all scenarios, for > example, you can not hold spin lock in mes_userq_reset_queue(), > because it calls either amdgpu_mes_reset_queue_mmio or > amdgpu_mes_reset_queue_mmio, both of them acquire the MES mutex through > amdgpu_mes_lock.
Yeah which is exactly the reason why mes_userq_reset_queue() should *NOT* be called from non IOCTL context. > Another thing, out of the topic is, holding xa_lock does not guarantee > fpriv/userq_mgr alive, for example, when drm_device->unplugged is true, all > amdgpu teardown paths in amdgpu_drm_release are skipped, > and the fpriv/userq_mgr is freed, no matter whether holding the xa spin lock. That would clearly be a massive bug. Those objects still need to be cleaned up independent of device hot plug. > So IMHO since we have userq->kref, lets use it to maintain the lifecycle of > the queues. > >> Grabbing references from this side would obviously result in circle >> dependencies. > > I am not sure, we should use the lock/unlock and kref_put/get in pairs in > sequence, can you name some circle dependencies or AB-BA lockings as examples? That is not AB-BA locking, but circle dependencies. E.g. A reference B, B referencing C, C referencing A again. Regards, Christian. > > Thanks > Lingshan > >> Regards, >> Christian. >> >>> Signed-off-by: Zhu Lingshan <[email protected]> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 30 +++++++++++++++++++++++ >>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h | 2 ++ >>> 2 files changed, 32 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >>> index 0a816b3c5ff9..e0639f844a8e 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >>> @@ -609,6 +609,36 @@ struct amdgpu_usermode_queue *amdgpu_userq_get(struct >>> amdgpu_userq_mgr *uq_mgr, >>> return queue; >>> } >>> >>> +/** >>> + * amdgpu_lookup_queue_by_doorbell - look up a user queue by doorbell >>> + * @xa: user queue XArray indexed by doorbell >>> + * @doorbell: doorbell index >>> + * >>> + * Return: A queue with the doorbell indexed, or NULL if no such a queue >>> found. >>> + * >>> + * This function increases kref of the queue, the caller >>> + * must release the reference with amdgpu_userq_put(). >>> + */ >>> +struct amdgpu_usermode_queue * >>> +amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell) >>> +{ >>> + struct amdgpu_usermode_queue *queue; >>> + unsigned long flags; >>> + >>> + xa_lock_irqsave(xa, flags); >>> + queue = xa_load(xa, doorbell); >>> + if (!queue) >>> + goto out_unlock; >>> + >>> + if (!kref_get_unless_zero(&queue->refcount)) >>> + queue = NULL; >>> + >>> +out_unlock: >>> + xa_unlock_irqrestore(xa, flags); >>> + >>> + return queue; >>> +} >>> + >>> void amdgpu_userq_put(struct amdgpu_usermode_queue *queue) >>> { >>> if (queue) >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h >>> index 6412a7f7b6ef..8fc73862f64e 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h >>> @@ -151,6 +151,8 @@ struct amdgpu_db_info { >>> }; >>> >>> struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr >>> *uq_mgr, u32 qid); >>> +struct amdgpu_usermode_queue * >>> +amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell); >>> void amdgpu_userq_put(struct amdgpu_usermode_queue *queue); >>> >>> int amdgpu_userq_ioctl(struct drm_device *dev, void *data, struct drm_file >>> *filp);
