From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Koenig, Christian" Subject: Re: [BUG] gpu: drm: amdgpu: Possible sleep-in-atomic-context bugs in amdgpu_uvd_ring_begin_use() Date: Sat, 15 Sep 2018 09:23:30 +0000 Message-ID: <68eb2303-ec42-470e-a060-487a14e2708c@email.android.com> References: , <9dd9f4ed-f338-8970-b29e-200ef0eafb62@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0998783355==" Return-path: In-Reply-To: <9dd9f4ed-f338-8970-b29e-200ef0eafb62-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Content-Language: de-DE List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: Jia-Ju Bai Cc: "Zhou, David(ChunMing)" , "airlied-cv59FeDIM0c@public.gmane.org" , Linux Kernel Mailing List , "dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" , "Deucher, Alexander" --===============0998783355== Content-Language: de-DE Content-Type: multipart/alternative; boundary="_000_68eb2303ec42470ea060487a14e2708cemailandroidcom_" --_000_68eb2303ec42470ea060487a14e2708cemailandroidcom_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable No, the problem is the function pointer analysis. In other words the KIQ ring is sometimes used in atomic and even interrupt = context. But the UVD ring is never used in atomic context. But I don't see a way a static analysis could ever figure that out. Regards, Christian. Am 15.09.2018 11:18 schrieb Jia-Ju Bai : Sorry, I am still not clear why the call chain I proposed is incorrect... I find a conditional in amdgpu_mm_wreg(): if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) return amdgpu_virt_kiq_wreg(adev, reg, v); Is amdgpu_virt_kiq_wreg() never called from WREG32() or RREG32()? Best wishes, Jia-Ju Bai On 2018/9/15 17:10, Koenig, Christian wrote: amdgpu_ring_alloc() does call amdgpu_uvd_begin_use(), but never in the call= chain you proposed. Thinking about it I actually don't see a way a statically analysis could ev= er figure that out. Christian. Am 15.09.2018 11:05 schrieb Jia-Ju Bai : Thanks for your reply. On 2018/9/15 17:01, Koenig, Christian wrote: Sorry to say that but your analysis tool is buggy. The proposed call paths will never trigger. Could you please explain which piece of the call path is incorrect? I am not very sure of my function pointer analysis. Does amdgpu_ring_alloc() never calls amdgpu_uvd_ring_begin_use()? Thanks in advance. Best wishes, Jia-Ju Bai Regards, Christian. Am 15.09.2018 10:59 schrieb Jia-Ju Bai : The driver may sleep with holding a spinlock. The function call paths (from bottom to top) in Linux-4.17 are: [FUNC] mutex_lock_nested drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c, 1477: mutex_lock_nested in amdgpu_dpm_enable_uvd drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c, 1154: amdgpu_dpm_enable_uvd in amdgpu_uvd_ring_begin_use drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c, 80: [FUNC_PTR]amdgpu_uvd_ring_begin_use in amdgpu_ring_alloc drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c, 199: amdgpu_ring_alloc in amdgpu_virt_kiq_wreg drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, 207: amdgpu_virt_kiq_wreg in amdgpu_mm_wreg drivers/gpu/drm/amd/amdgpu/vi.c, 207: amdgpu_mm_wreg in vi_gc_cac_rreg drivers/gpu/drm/amd/amdgpu/vi.c, 206: _raw_spin_lock_irqsave in vi_gc_cac_rreg [FUNC] mutex_lock_nested drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c, 1477: mutex_lock_nested in amdgpu_dpm_enable_uvd drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c, 1154: amdgpu_dpm_enable_uvd in amdgpu_uvd_ring_begin_use drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c, 80: [FUNC_PTR]amdgpu_uvd_ring_begin_use in amdgpu_ring_alloc drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c, 199: amdgpu_ring_alloc in amdgpu_virt_kiq_wreg drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, 207: amdgpu_virt_kiq_wreg in amdgpu_mm_wreg drivers/gpu/drm/amd/amdgpu/soc15.c, 106: amdgpu_mm_wreg in soc15_pcie_rreg drivers/gpu/drm/amd/amdgpu/soc15.c, 105: _raw_spin_lock_irqsave in soc15_pcie_rreg [FUNC] mutex_lock_nested drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c, 1477: mutex_lock_nested in amdgpu_dpm_enable_uvd drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c, 1154: amdgpu_dpm_enable_uvd in amdgpu_uvd_ring_begin_use drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c, 80: [FUNC_PTR]amdgpu_uvd_ring_begin_use in amdgpu_ring_alloc drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c, 199: amdgpu_ring_alloc in amdgpu_virt_kiq_wreg drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, 207: amdgpu_virt_kiq_wreg in amdgpu_mm_wreg drivers/gpu/drm/amd/amdgpu/cik.c, 139: amdgpu_mm_wreg in cik_uvd_ctx_wreg drivers/gpu/drm/amd/amdgpu/cik.c, 138: _raw_spin_lock_irqsave in cik_uvd_ctx_wreg [FUNC] mutex_lock_nested drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c, 1477: mutex_lock_nested in amdgpu_dpm_enable_uvd drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c, 1154: amdgpu_dpm_enable_uvd in amdgpu_uvd_ring_begin_use drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c, 80: [FUNC_PTR]amdgpu_uvd_ring_begin_use in amdgpu_ring_alloc drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c, 199: amdgpu_ring_alloc in amdgpu_virt_kiq_wreg drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, 207: amdgpu_virt_kiq_wreg in amdgpu_mm_wreg drivers/gpu/drm/amd/amdgpu/dce_v6_0.c, 126: amdgpu_mm_wreg in dce_v6_0_audio_endpt_rreg drivers/gpu/drm/amd/amdgpu/dce_v6_0.c, 125: _raw_spin_lock_irqsave in dce_v6_0_audio_endpt_rreg Note that [FUNC_PTR] means a function pointer call is used. These bugs are found by my static analysis tool DSAC. Best wishes, Jia-Ju Bai --_000_68eb2303ec42470ea060487a14e2708cemailandroidcom_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
No, the problem is the function pointer analysis.

In other words the KIQ ring is sometimes used in atomic a= nd even interrupt context.

But the UVD ring is never used in atomic context.

But I don't see a way a static analysis could ever figure= that out.

Regards,
Christian.

Am 15.09.2018 11:18 schrieb Jia-Ju Bai <baiji= aju1990-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
Sorry, I am still not clear why the call chain I proposed is incorrect= ...

I find a conditional in amdgpu_mm_wreg():

    if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amd= gpu_sriov_runtime(adev))
        return amdgpu_virt_kiq_wreg(adev, reg= , v);

Is amdgpu_virt_kiq_wreg() never called from WREG32() or RREG32()?


Best wishes,
Jia-Ju Bai


On 2018/9/15 17:10, Koenig, Christian wrote:=
amdgpu_ring_alloc() does call amdgpu_uvd_begin_use(), but never in the= call chain you proposed.

Thinking about it I actually don't see a way a statically= analysis could ever figure that out.

Christian.

Am 15.09.2018 11:05 schrieb Jia-Ju Bai <baijiaju1990-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
Thanks for your reply.

On 2018/9/15 17:01, Koenig, Christian wrote:
Sorry to say that but your analysis tool is buggy.

The proposed call paths will never trigger.

Could you please explain which piece of the call path is incorrect?
I am not very sure of my function pointer analysis.
Does amdgpu_ring_alloc() never calls amdgpu_uvd_ring_begin_use()?

Thanks in advance.


Best wishes,
Jia-Ju Bai


Regards,
Christian.

Am 15.09.2018 10:59 schrieb Jia-Ju Bai <baijiaju1990-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
The driver may sleep with holding a spinlock.
The function call paths (from bottom to top) in Linux-4.17 are:

[FUNC] mutex_lock_nested
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c, 1477:
     mutex_lock_nested in amdgpu_dpm_enable_uvd
drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c, 1154:
     amdgpu_dpm_enable_uvd in amdgpu_uvd_ring_begin_use=
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c, 80:
     [FUNC_PTR]amdgpu_uvd_ring_begin_use in amdgpu_ring= _alloc
drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c, 199:
     amdgpu_ring_alloc in amdgpu_virt_kiq_wreg
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, 207:
     amdgpu_virt_kiq_wreg in amdgpu_mm_wreg
drivers/gpu/drm/amd/amdgpu/vi.c, 207:
     amdgpu_mm_wreg in vi_gc_cac_rreg
drivers/gpu/drm/amd/amdgpu/vi.c, 206:
     _raw_spin_lock_irqsave in vi_gc_cac_rreg

[FUNC] mutex_lock_nested
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c, 1477:
     mutex_lock_nested in amdgpu_dpm_enable_uvd
drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c, 1154:
     amdgpu_dpm_enable_uvd in amdgpu_uvd_ring_begin_use=
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c, 80:
     [FUNC_PTR]amdgpu_uvd_ring_begin_use in amdgpu_ring= _alloc
drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c, 199:
     amdgpu_ring_alloc in amdgpu_virt_kiq_wreg
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, 207:
     amdgpu_virt_kiq_wreg in amdgpu_mm_wreg
drivers/gpu/drm/amd/amdgpu/soc15.c, 106:
     amdgpu_mm_wreg in soc15_pcie_rreg
drivers/gpu/drm/amd/amdgpu/soc15.c, 105:
     _raw_spin_lock_irqsave in soc15_pcie_rreg

[FUNC] mutex_lock_nested
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c, 1477:
     mutex_lock_nested in amdgpu_dpm_enable_uvd
drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c, 1154:
     amdgpu_dpm_enable_uvd in amdgpu_uvd_ring_begin_use=
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c, 80:
     [FUNC_PTR]amdgpu_uvd_ring_begin_use in amdgpu_ring= _alloc
drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c, 199:
     amdgpu_ring_alloc in amdgpu_virt_kiq_wreg
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, 207:
     amdgpu_virt_kiq_wreg in amdgpu_mm_wreg
drivers/gpu/drm/amd/amdgpu/cik.c, 139:
     amdgpu_mm_wreg in cik_uvd_ctx_wreg
drivers/gpu/drm/amd/amdgpu/cik.c, 138:
     _raw_spin_lock_irqsave in cik_uvd_ctx_wreg

[FUNC] mutex_lock_nested
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c, 1477:
     mutex_lock_nested in amdgpu_dpm_enable_uvd
drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c, 1154:
     amdgpu_dpm_enable_uvd in amdgpu_uvd_ring_begin_use=
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c, 80:
     [FUNC_PTR]amdgpu_uvd_ring_begin_use in amdgpu_ring= _alloc
drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c, 199:
     amdgpu_ring_alloc in amdgpu_virt_kiq_wreg
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, 207:
     amdgpu_virt_kiq_wreg in amdgpu_mm_wreg
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c, 126:
     amdgpu_mm_wreg in dce_v6_0_audio_endpt_rreg
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c, 125:
     _raw_spin_lock_irqsave in dce_v6_0_audio_endpt_rre= g

Note that [FUNC_PTR] means a function pointer call is used.

These bugs are found by my static analysis tool DSAC.


Best wishes,
Jia-Ju Bai



--_000_68eb2303ec42470ea060487a14e2708cemailandroidcom_-- --===============0998783355== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4Cg== --===============0998783355==--