[v1] drm/amdgpu: add a missing lock for AMDGPU_SCHED

[PATCH] drm/amdgpu: add a missing lock for AMDGPU_SCHED

Posted by Chia-I Wu 2 years, 9 months ago

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
index e9b45089a28a6..863b2a34b2d64 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
@@ -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

Re: [PATCH] drm/amdgpu: add a missing lock for AMDGPU_SCHED

Posted by Christian König 2 years, 9 months ago

Am 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.

> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> index e9b45089a28a6..863b2a34b2d64 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> @@ -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;

Re: [PATCH] drm/amdgpu: add a missing lock for AMDGPU_SCHED

Posted by Greg KH 2 years, 9 months ago

On Tue, Apr 25, 2023 at 05:48:27PM -0700, Chia-I Wu wrote:
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> Cc: stable@vger.kernel.org

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

Re: [PATCH] drm/amdgpu: add a missing lock for AMDGPU_SCHED

Posted by Chia-I Wu 2 years, 9 months ago

On Tue, Apr 25, 2023 at 9:58 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 25, 2023 at 05:48:27PM -0700, Chia-I Wu wrote:
> > Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> > Cc: stable@vger.kernel.org
>
> 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.
>
> thanks,
>
> greg k-h

RE: [PATCH] drm/amdgpu: add a missing lock for AMDGPU_SCHED

Posted by Chen, Guchun 2 years, 9 months ago

From coding style's perspective, this lock/unlock handling should be put into amdgpu_ctx_priority_override.

Regards,
Guchun

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Chia-
> I Wu
> Sent: Wednesday, April 26, 2023 8:48 AM
> To: dri-devel@lists.freedesktop.org
> Cc: Pan, Xinhui <Xinhui.Pan@amd.com>; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org; amd-gfx@lists.freedesktop.org; Daniel Vetter
> <daniel@ffwll.ch>; Deucher, Alexander <Alexander.Deucher@amd.com>;
> David Airlie <airlied@gmail.com>; Koenig, Christian
> <Christian.Koenig@amd.com>
> Subject: [PATCH] drm/amdgpu: add a missing lock for AMDGPU_SCHED
> 
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> index e9b45089a28a6..863b2a34b2d64 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> @@ -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

Re: [PATCH] drm/amdgpu: add a missing lock for AMDGPU_SCHED

Posted by Chia-I Wu 2 years, 9 months ago

On Tue, Apr 25, 2023 at 7:27 PM Chen, Guchun <Guchun.Chen@amd.com> wrote:
>
> 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.
>
> Regards,
> Guchun
>
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Chia-
> > I Wu
> > Sent: Wednesday, April 26, 2023 8:48 AM
> > To: dri-devel@lists.freedesktop.org
> > Cc: Pan, Xinhui <Xinhui.Pan@amd.com>; linux-kernel@vger.kernel.org;
> > stable@vger.kernel.org; amd-gfx@lists.freedesktop.org; Daniel Vetter
> > <daniel@ffwll.ch>; Deucher, Alexander <Alexander.Deucher@amd.com>;
> > David Airlie <airlied@gmail.com>; Koenig, Christian
> > <Christian.Koenig@amd.com>
> > Subject: [PATCH] drm/amdgpu: add a missing lock for AMDGPU_SCHED
> >
> > Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> > index e9b45089a28a6..863b2a34b2d64 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> > @@ -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
>