drm/amdgpu: add a missing lock for AMDGPU_SCHED
Subsystems (3)
PATCH 1/1 drm/amdgpu: add a missing lock for AMDGPU_SCHED
--- drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c +5 -1
@@ -38,6 +38,7 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev, {struct fd f =fdget (fd);struct amdgpu_fpriv *fpriv; +struct amdgpu_ctx_mgr *mgr;struct amdgpu_ctx *ctx;uint32_t id;int r;
@@ -51,8 +52,11 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev,return r; } -idr_for_each_entry (&fpriv->ctx_mgr.ctx_handles, ctx, id) + mgr = &fpriv->ctx_mgr; +mutex_lock (&mgr->lock); +idr_for_each_entry (&mgr->ctx_handles, ctx, id)amdgpu_ctx_priority_override (ctx, priority); +mutex_unlock (&mgr->lock);fdput (f);return 0 ;
Signature · 1 line
2.40.0.634.g4ca3ef3211-goog
From coding style's perspective, this lock/unlock handling should be put into amdgpu_ctx_priority_override.
Regards,
Guchun
Quoted 47 lines
-----Original Message-----
From: amd-gfx <[email protected]> On Behalf Of Chia- I Wu
Sent: Wednesday, April 26, 2023 8:48 AM
To: [email protected]
Cc: Pan, Xinhui <[email protected]>; [email protected]; [email protected]; [email protected]; Daniel Vetter <[email protected]>; Deucher, Alexander <[email protected]>; David Airlie <[email protected]>; Koenig, Christian <[email protected]>
Subject: [PATCH] drm/amdgpu: add a missing lock forAMDGPU_SCHEDSigned-off-by Chia-I Wu <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c +5
-1@@ -38,6 +38,7 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev, {struct fd f =fdget (fd);struct amdgpu_fpriv *fpriv; +struct amdgpu_ctx_mgr *mgr;struct amdgpu_ctx *ctx;uint32_t id;int r;@@ -51,8 +52,11 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev, return r; } -idr_for_each_entry (&fpriv->ctx_mgr.ctx_handles, ctx, id) + mgr = &fpriv->ctx_mgr; +mutex_lock (&mgr->lock); +idr_for_each_entry (&mgr->ctx_handles, ctx, id)amdgpu_ctx_priority_override (ctx, priority); +mutex_unlock (&mgr->lock);fdput (f);return 0;
--
2.40.0.634.g4ca3ef3211-goog
Quoted 2 lines · Guchun
From coding style's perspective, this lock/unlock handling should be put into
amdgpu_ctx_priority_override.
The locking is to protect mgr->ctx_handles.
Quoted 52 lines
Regards,
GuchunQuoted 47 lines
-----Original Message-----
From: amd-gfx <[email protected]> On Behalf Of Chia- I Wu
Sent: Wednesday, April 26, 2023 8:48 AM
To: [email protected]
Cc: Pan, Xinhui <[email protected]>; [email protected]; [email protected]; [email protected]; Daniel Vetter <[email protected]>; Deucher, Alexander <[email protected]>; David Airlie <[email protected]>; Koenig, Christian <[email protected]>
Subject: [PATCH] drm/amdgpu: add a missing lock forAMDGPU_SCHEDSigned-off-by Chia-I Wu <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c +5
-1@@ -38,6 +38,7 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev, {struct fd f =fdget (fd);struct amdgpu_fpriv *fpriv; +struct amdgpu_ctx_mgr *mgr;struct amdgpu_ctx *ctx;uint32_t id;int r;@@ -51,8 +52,11 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev, return r; } -idr_for_each_entry (&fpriv->ctx_mgr.ctx_handles, ctx, id) + mgr = &fpriv->ctx_mgr; +mutex_lock (&mgr->lock); +idr_for_each_entry (&mgr->ctx_handles, ctx, id)amdgpu_ctx_priority_override (ctx, priority); +mutex_unlock (&mgr->lock);fdput (f);return 0;--
2.40.0.634.g4ca3ef3211-goog
Greg KH
maintainerQuoted 2 lines · Chia-I Wu
Signed-off-by Chia-I Wu <[email protected]>
I know I can not take patches without any changelog text at all, maybe the DRM developers are more lax, but it's not a good idea at all.
thanks,
greg k-h
Quoted 7 lines · Greg KH
Quoted 2 lines · Chia-I Wu
Signed-off-by Chia-I Wu <[email protected]>I know I can not take patches without any changelog text at all, maybe the DRM developers are more lax, but it's not a good idea at all.
Oops, sorry. I've sent out v2 to address both review feedback.
Quoted 4 lines
thanks,
greg k-h
Christian König
maintainerAm 26.04.23 um 02:48 schrieb Chia-I Wu:
Good catch, but you need some commit message here. Something like "Need to hold the lock while iterating the idr to make sure no context is destroyed." should be sufficient.
Apart from that looks good to me.
Regards,
Christian.
Quoted 31 lines
Signed-off-by Chia-I Wu <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c +5
-1@@ -38,6 +38,7 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev, {struct fd f =fdget (fd);struct amdgpu_fpriv *fpriv; +struct amdgpu_ctx_mgr *mgr;struct amdgpu_ctx *ctx;uint32_t id;int r;@@ -51,8 +52,11 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev,return r; } -idr_for_each_entry (&fpriv->ctx_mgr.ctx_handles, ctx, id) + mgr = &fpriv->ctx_mgr; +mutex_lock (&mgr->lock); +idr_for_each_entry (&mgr->ctx_handles, ctx, id)amdgpu_ctx_priority_override (ctx, priority); +mutex_unlock (&mgr->lock);fdput (f);return 0 ;