Re: [PATCH] drm/fb-helper: Add missed unlocks in setcmap_legacy()

[Daniel Vetter]

On Thu, Dec 3, 2020 at 5:11 PM Peter Rosin  wrote:
>
> Hi!
>
> On 2020-12-03 15:42, Chuhong Yuan wrote:
> > setcmap_legacy() does not call drm_modeset_unlock_all() in some exits,
> > add the missed unlocks with goto to fix it.
> >
> > Fixes: 964c60063bff ("drm/fb-helper: separate the fb_setcmap helper into 
> > atomic and legacy paths")
> > Signed-off-by: Chuhong Yuan 
>
> Yup, my patch fumbled the locking. Sorry, and thanks for cleaning up my mess!
>
> Acked-by: Peter Rosin 
>
> (Spelled that as Ached-by at first, what does that mean??)

Merged already before I've seen your ack here (and we don't rebase so
can't add it now), thanks for the patch and all.
-Daniel

>
> Cheers,
> Peter
>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 15 ++-
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c 
> > b/drivers/gpu/drm/drm_fb_helper.c
> > index 1543d9d10970..8033467db4be 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -923,11 +923,15 @@ static int setcmap_legacy(struct fb_cmap *cmap, 
> > struct fb_info *info)
> >   drm_modeset_lock_all(fb_helper->dev);
> >   drm_client_for_each_modeset(modeset, _helper->client) {
> >   crtc = modeset->crtc;
> > - if (!crtc->funcs->gamma_set || !crtc->gamma_size)
> > - return -EINVAL;
> > + if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> >
> > - if (cmap->start + cmap->len > crtc->gamma_size)
> > - return -EINVAL;
> > + if (cmap->start + cmap->len > crtc->gamma_size) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> >
> >   r = crtc->gamma_store;
> >   g = r + crtc->gamma_size;
> > @@ -940,8 +944,9 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct 
> > fb_info *info)
> >   ret = crtc->funcs->gamma_set(crtc, r, g, b,
> >crtc->gamma_size, NULL);
> >   if (ret)
> > - return ret;
> > + goto out;
> >   }
> > +out:
> >   drm_modeset_unlock_all(fb_helper->dev);
> >
> >   return ret;
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/fb-helper: Add missed unlocks in setcmap_legacy()

[Peter Rosin]

Hi!

On 2020-12-03 15:42, Chuhong Yuan wrote:
> setcmap_legacy() does not call drm_modeset_unlock_all() in some exits,
> add the missed unlocks with goto to fix it.
> 
> Fixes: 964c60063bff ("drm/fb-helper: separate the fb_setcmap helper into 
> atomic and legacy paths")
> Signed-off-by: Chuhong Yuan 

Yup, my patch fumbled the locking. Sorry, and thanks for cleaning up my mess!

Acked-by: Peter Rosin 

(Spelled that as Ached-by at first, what does that mean??)

Cheers,
Peter

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 1543d9d10970..8033467db4be 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -923,11 +923,15 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct 
> fb_info *info)
>   drm_modeset_lock_all(fb_helper->dev);
>   drm_client_for_each_modeset(modeset, _helper->client) {
>   crtc = modeset->crtc;
> - if (!crtc->funcs->gamma_set || !crtc->gamma_size)
> - return -EINVAL;
> + if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
> + ret = -EINVAL;
> + goto out;
> + }
>  
> - if (cmap->start + cmap->len > crtc->gamma_size)
> - return -EINVAL;
> + if (cmap->start + cmap->len > crtc->gamma_size) {
> + ret = -EINVAL;
> + goto out;
> + }
>  
>   r = crtc->gamma_store;
>   g = r + crtc->gamma_size;
> @@ -940,8 +944,9 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct 
> fb_info *info)
>   ret = crtc->funcs->gamma_set(crtc, r, g, b,
>crtc->gamma_size, NULL);
>   if (ret)
> - return ret;
> + goto out;
>   }
> +out:
>   drm_modeset_unlock_all(fb_helper->dev);
>  
>   return ret;
> 

[PATCH] drm/fb-helper: Add missed unlocks in setcmap_legacy()

[Chuhong Yuan]

setcmap_legacy() does not call drm_modeset_unlock_all() in some exits,
add the missed unlocks with goto to fix it.

Fixes: 964c60063bff ("drm/fb-helper: separate the fb_setcmap helper into atomic 
and legacy paths")
Signed-off-by: Chuhong Yuan 
---
 drivers/gpu/drm/drm_fb_helper.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1543d9d10970..8033467db4be 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -923,11 +923,15 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct 
fb_info *info)
drm_modeset_lock_all(fb_helper->dev);
drm_client_for_each_modeset(modeset, _helper->client) {
crtc = modeset->crtc;
-   if (!crtc->funcs->gamma_set || !crtc->gamma_size)
-   return -EINVAL;
+   if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
+   ret = -EINVAL;
+   goto out;
+   }
 
-   if (cmap->start + cmap->len > crtc->gamma_size)
-   return -EINVAL;
+   if (cmap->start + cmap->len > crtc->gamma_size) {
+   ret = -EINVAL;
+   goto out;
+   }
 
r = crtc->gamma_store;
g = r + crtc->gamma_size;
@@ -940,8 +944,9 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct 
fb_info *info)
ret = crtc->funcs->gamma_set(crtc, r, g, b,
 crtc->gamma_size, NULL);
if (ret)
-   return ret;
+   goto out;
}
+out:
drm_modeset_unlock_all(fb_helper->dev);
 
return ret;
-- 
2.26.2