Re: Potential BUG: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function(v3)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Luben & Iago,

no the code is completely correct and intentional like this.

See the code path which takes the lock in amdgpu_mm_wreg() is a workaround when the rmmio_size doesn't allow access to the full register BAR.

In this case the MM_INDEX/MM_DATA registers are used as side path and because of this we need the lock.

But this case can't happen when you use the MM_INDEX/MM_DATA pair directly as parameters for the function.

Regards,
Christian.

Am 20.11.19 um 01:23 schrieb Luben Tuikov:
Hi Iago,

Thank you for finding and reporting this potential double lock.

Yes indeed, I see it--it can indeed happen.

Now, since the primitives used--macros using "amdgpu_mm_(r|w)reg\(.*\)"--in
"amdgpu_device_vram_access()" do use their own register-access spinlocks,
it maybe wise to remove the spinlock take/release in "amdgpu_device_vram_access()".

We'll look into it and possibly submit another patch.

Thanks again.

Regards,
Luben

On 2019-11-16 11:21 a.m., Iago Abal wrote:
Hi,

With the help of a static bug finder (EBA - https://github.com/IagoAbal/eba) I have found a potential double lock in Linux Next tag next-20191115, file drivers/gpu/drm/amd/amdgpu/amdgpu_device.c.

This bug seems to be introduced by commit
e35e2b117f4 ("drm/amdgpu: add a generic fb accessing helper function(v3)").

The steps to reproduce it would be:

1. Start in function `amdgpu_device_vram_access`.
2. Enter for-loop `for (last += pos; pos <= last; pos += 4)`.
3. First lock: `spin_lock_irqsave(&adev->mmio_idx_lock, flags)`.
4. Call to `WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000)`.
    5. Note `#define WREG32_NO_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)`.
    6. Continue in function `amdgpu_mm_wreg`.
    7. Take else-branch in the third if-statement.
    8. Double lock: `spin_lock_irqsave(&adev->mmio_idx_lock, flags)`.

I think the control flow could reach that second lock, but you may know better.

Hope it helps!

-- iago
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux