On 4/1/26 12:48, Mikhail Gavrilov wrote:
> xa_alloc_cyclic() and xa_erase() use plain xa_lock()/xa_unlock()
> regardless of XA_FLAGS_LOCK_IRQ — the flag only affects lockdep
> annotations, not runtime locking.
>
> Switch amdgpu_pasid_alloc() to xa_alloc_cyclic_irq() which uses
> xa_lock_irq/xa_unlock_irq internally.
>
> For amdgpu_pasid_free(), use explicit xa_lock_irqsave/__xa_erase/
> xa_unlock_irqrestore since this function can be called from hardirq
> context via amdgpu_pasid_free_cb, where xa_erase_irq()'s
> xa_lock_irq/xa_unlock_irq would prematurely re-enable interrupts.
>
> Fixes: a3c0ee978e16 ("drm/amdgpu: replace PASID IDR with XArray")
> Signed-off-by: Mikhail Gavrilov <[email protected]>
> ---
>
> v2: Use xa_lock_irqsave/__xa_erase/xa_unlock_irqrestore for
> amdgpu_pasid_free() instead of xa_erase_irq(), since it can
> be called from hardirq context where xa_unlock_irq would
> prematurely re-enable interrupts. (Christian König)
> v1:
> https://lore.kernel.org/all/[email protected]/
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index a6ac3b4ce0df..6fc6859d9329 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -68,7 +68,7 @@ int amdgpu_pasid_alloc(unsigned int bits)
> if (bits == 0)
> return -EINVAL;
>
> - r = xa_alloc_cyclic(&amdgpu_pasid_xa, &pasid, xa_mk_value(0),
> + r = xa_alloc_cyclic_irq(&amdgpu_pasid_xa, &pasid, xa_mk_value(0),
> XA_LIMIT(1, (1U << bits) - 1),
> &amdgpu_pasid_xa_next, GFP_KERNEL);
> if (r < 0)
> @@ -84,8 +84,19 @@ int amdgpu_pasid_alloc(unsigned int bits)
> */
> void amdgpu_pasid_free(u32 pasid)
> {
> + unsigned long flags;
> +
> trace_amdgpu_pasid_freed(pasid);
> - xa_erase(&amdgpu_pasid_xa, pasid);
> +
> + /*
> + * Use irqsave because this can be called from IRQ context
> + * via amdgpu_pasid_free_cb. xa_erase_irq() is insufficient
> + * as it uses xa_lock_irq which can prematurely re-enable
> + * interrupts in hardirq context.
> + */
Please drop that comment or at least shorten it quite a bit.
Apart from that looks good to me.
Regards,
Christian.
> + xa_lock_irqsave(&amdgpu_pasid_xa, flags);
> + __xa_erase(&amdgpu_pasid_xa, pasid);
> + xa_unlock_irqrestore(&amdgpu_pasid_xa, flags);
> }
>
> static void amdgpu_pasid_free_cb(struct dma_fence *fence,
