On 9/1/2026 4:50 PM, Zhu, Lingshan wrote: > On 8/31/2026 4:21 PM, Christian König wrote: > >> 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. > I think it is not about whether called from IOCTL, it is a common racing we > should fix, and holding a kref is a low haning fruit. > >>> 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. > I agree, when unplugged == true, means can not access any HW registers, so > this bug deserve another series to fix. > >>> 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. > A kref is an atomic counter, not a lock, that means we can try hold more than > one kref in a thread, and a kref not > depend on another. As far as I can see, current amdgpu driver does not have > such problems, do you see any occurrences?
Hello Christian, I have not heard back from you for three weeks, I wonder whether you have any further comments for this series, or shall I make any improvements? Thanks Lingshan > > Thanks > Lingshan > >> 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);
