| Message ID | 20170620165013.11013-1-mail@dbalan.in (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 20, 2017 at 06:50:13PM +0200, Dhananjay Balan wrote: > The locking and unlocking code used by copy routines is common, so > moved it to a macro. > > Signed-off-by: Dhananjay Balan <mail@dbalan.in> > --- > drivers/staging/sm750fb/sm750.c | 81 ++++++++++++++++------------------------- > 1 file changed, 31 insertions(+), 50 deletions(-) > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c > index 386d4adcd91d..d8ab83aea46d 100644 > --- a/drivers/staging/sm750fb/sm750.c > +++ b/drivers/staging/sm750fb/sm750.c > @@ -156,12 +156,25 @@ static int lynxfb_ops_cursor(struct fb_info *info, struct fb_cursor *fbcursor) > return 0; > } > > +/* > + * If not using spin_lock, system will die if user frequently loads and > + * immediately unloads driver (dual) > + */ > +#define dual_safe_call(func, ...) \ > + do { \ > + if (sm750_dev->fb_count > 1) \ > + spin_lock(&sm750_dev->slock); \ > + func(__VA_ARGS__); \ > + if (sm750_dev->fb_count > 1) \ > + spin_unlock(&sm750_dev->slock); \ > + } while (0) > + I feel like this is the wrong approach. You could just make a small lock function and a small unlock function. Except that if statement seems kind of bogus. What happens if ->fb_count is 0 when we lock and 1 when we unlock? Why not lock unconditionally? It is not likely to be contested if there are no other users. > static void lynxfb_ops_fillrect(struct fb_info *info, > const struct fb_fillrect *region) > { > struct lynxfb_par *par; > struct sm750_dev *sm750_dev; > - unsigned int base, pitch, Bpp, rop; > + unsigned int base, pitch, bit_pp, rop; This part has nothing to do with locking... *frowny face* regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tue, 2017-06-20, 23:05 +0300-യ്ക്ക, Dan Carpenter എഴുതിയിരിക്കുന്നു: > On Tue, Jun 20, 2017 at 06:50:13PM +0200, Dhananjay Balan wrote: > > The locking and unlocking code used by copy routines is common, so > > moved it to a macro. > > > > Signed-off-by: Dhananjay Balan <mail@dbalan.in> > > --- > > drivers/staging/sm750fb/sm750.c | 81 ++++++++++++++++------------- > > ------------ > > 1 file changed, 31 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/staging/sm750fb/sm750.c > > b/drivers/staging/sm750fb/sm750.c > > index 386d4adcd91d..d8ab83aea46d 100644 > > --- a/drivers/staging/sm750fb/sm750.c > > +++ b/drivers/staging/sm750fb/sm750.c > > @@ -156,12 +156,25 @@ static int lynxfb_ops_cursor(struct fb_info > > *info, struct fb_cursor *fbcursor) > > return 0; > > } > > > > +/* > > + * If not using spin_lock, system will die if user frequently > > loads and > > + * immediately unloads driver (dual) > > + */ > > +#define dual_safe_call(func, ...) > > \ > > + do { > > \ > > + if (sm750_dev->fb_count > 1) > > \ > > + spin_lock(&sm750_dev->slock); > > \ > > + func(__VA_ARGS__); > > \ > > + if (sm750_dev->fb_count > 1) > > \ > > + spin_unlock(&sm750_dev->slock); > > \ > > + } while (0) > > + > > I feel like this is the wrong approach. You could just make a small > lock function and a small unlock function. Except that if statement > seems kind of bogus. What happens if ->fb_count is 0 when we lock > and > 1 when we unlock? Why not lock unconditionally? It is not likely to > be > contested if there are no other users. I've to admit that I don't have much info, but is fb_count supposed to change during this execution? > > > static void lynxfb_ops_fillrect(struct fb_info *info, > > const struct fb_fillrect *region) > > { > > struct lynxfb_par *par; > > struct sm750_dev *sm750_dev; > > - unsigned int base, pitch, Bpp, rop; > > + unsigned int base, pitch, bit_pp, rop; > > This part has nothing to do with locking... *frowny face* Sorry about that, I'll split this into separate commit. Thanks, Dhananjay Balan. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 21, 2017 at 10:59:01AM +0200, Dhananjay Balan wrote: > Tue, 2017-06-20, 23:05 +0300-യ്ക്ക, Dan Carpenter എഴുതിയിരിക്കുന്നു: > > On Tue, Jun 20, 2017 at 06:50:13PM +0200, Dhananjay Balan wrote: > > > The locking and unlocking code used by copy routines is common, so > > > moved it to a macro. > > > > > > Signed-off-by: Dhananjay Balan <mail@dbalan.in> > > > --- > > > drivers/staging/sm750fb/sm750.c | 81 ++++++++++++++++------------- > > > ------------ > > > 1 file changed, 31 insertions(+), 50 deletions(-) > > > > > > diff --git a/drivers/staging/sm750fb/sm750.c > > > b/drivers/staging/sm750fb/sm750.c > > > index 386d4adcd91d..d8ab83aea46d 100644 > > > --- a/drivers/staging/sm750fb/sm750.c > > > +++ b/drivers/staging/sm750fb/sm750.c > > > @@ -156,12 +156,25 @@ static int lynxfb_ops_cursor(struct fb_info > > > *info, struct fb_cursor *fbcursor) > > > return 0; > > > } > > > > > > +/* > > > + * If not using spin_lock, system will die if user frequently > > > loads and > > > + * immediately unloads driver (dual) > > > + */ > > > +#define dual_safe_call(func, ...) > > > \ > > > + do { > > > \ > > > + if (sm750_dev->fb_count > 1) > > > \ > > > + spin_lock(&sm750_dev->slock); > > > \ > > > + func(__VA_ARGS__); > > > \ > > > + if (sm750_dev->fb_count > 1) > > > \ > > > + spin_unlock(&sm750_dev->slock); > > > \ > > > + } while (0) > > > + > > > > I feel like this is the wrong approach. You could just make a small > > lock function and a small unlock function. Except that if statement > > seems kind of bogus. What happens if ->fb_count is 0 when we lock > > and > > 1 when we unlock? Why not lock unconditionally? It is not likely to > > be > > contested if there are no other users. > I've to admit that I don't have much info, but is fb_count supposed to > change during this execution? It could change, yes. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 20, 2017 at 06:50:13PM +0200, Dhananjay Balan wrote: > The locking and unlocking code used by copy routines is common, so > moved it to a macro. Ick, no, never "hide" locks like this, that's not good at all. We want to see the locks in the code where it happens, otherwise it is not obvious as to what is going on. sorry, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c index 386d4adcd91d..d8ab83aea46d 100644 --- a/drivers/staging/sm750fb/sm750.c +++ b/drivers/staging/sm750fb/sm750.c @@ -156,12 +156,25 @@ static int lynxfb_ops_cursor(struct fb_info *info, struct fb_cursor *fbcursor) return 0; } +/* + * If not using spin_lock, system will die if user frequently loads and + * immediately unloads driver (dual) + */ +#define dual_safe_call(func, ...) \ + do { \ + if (sm750_dev->fb_count > 1) \ + spin_lock(&sm750_dev->slock); \ + func(__VA_ARGS__); \ + if (sm750_dev->fb_count > 1) \ + spin_unlock(&sm750_dev->slock); \ + } while (0) + static void lynxfb_ops_fillrect(struct fb_info *info, const struct fb_fillrect *region) { struct lynxfb_par *par; struct sm750_dev *sm750_dev; - unsigned int base, pitch, Bpp, rop; + unsigned int base, pitch, bit_pp, rop; u32 color; if (info->state != FBINFO_STATE_RUNNING) @@ -176,26 +189,15 @@ static void lynxfb_ops_fillrect(struct fb_info *info, */ base = par->crtc.oScreen; pitch = info->fix.line_length; - Bpp = info->var.bits_per_pixel >> 3; + bit_pp = info->var.bits_per_pixel >> 3; - color = (Bpp == 1) ? region->color : + color = (bit_pp == 1) ? region->color : ((u32 *)info->pseudo_palette)[region->color]; rop = (region->rop != ROP_COPY) ? HW_ROP2_XOR : HW_ROP2_COPY; - /* - * If not use spin_lock,system will die if user load driver - * and immediately unload driver frequently (dual) - */ - if (sm750_dev->fb_count > 1) - spin_lock(&sm750_dev->slock); - - sm750_dev->accel.de_fillrect(&sm750_dev->accel, - base, pitch, Bpp, - region->dx, region->dy, - region->width, region->height, - color, rop); - if (sm750_dev->fb_count > 1) - spin_unlock(&sm750_dev->slock); + dual_safe_call(sm750_dev->accel.de_fillrect, &sm750_dev->accel, + base, pitch, bit_pp, region->dx, region->dy, + region->width, region->height, color, rop); } static void lynxfb_ops_copyarea(struct fb_info *info, @@ -203,7 +205,7 @@ static void lynxfb_ops_copyarea(struct fb_info *info, { struct lynxfb_par *par; struct sm750_dev *sm750_dev; - unsigned int base, pitch, Bpp; + unsigned int base, pitch, bit_pp; par = info->par; sm750_dev = par->dev; @@ -214,28 +216,18 @@ static void lynxfb_ops_copyarea(struct fb_info *info, */ base = par->crtc.oScreen; pitch = info->fix.line_length; - Bpp = info->var.bits_per_pixel >> 3; + bit_pp = info->var.bits_per_pixel >> 3; - /* - * If not use spin_lock, system will die if user load driver - * and immediately unload driver frequently (dual) - */ - if (sm750_dev->fb_count > 1) - spin_lock(&sm750_dev->slock); - - sm750_dev->accel.de_copyarea(&sm750_dev->accel, - base, pitch, region->sx, region->sy, - base, pitch, Bpp, region->dx, region->dy, - region->width, region->height, - HW_ROP2_COPY); - if (sm750_dev->fb_count > 1) - spin_unlock(&sm750_dev->slock); + dual_safe_call(sm750_dev->accel.de_copyarea, &sm750_dev->accel, + base, pitch, region->sx, region->sy, + base, pitch, bit_pp, region->dx, region->dy, + region->width, region->height, HW_ROP2_COPY); } static void lynxfb_ops_imageblit(struct fb_info *info, const struct fb_image *image) { - unsigned int base, pitch, Bpp; + unsigned int base, pitch, bit_pp; unsigned int fgcol, bgcol; struct lynxfb_par *par; struct sm750_dev *sm750_dev; @@ -248,7 +240,7 @@ static void lynxfb_ops_imageblit(struct fb_info *info, */ base = par->crtc.oScreen; pitch = info->fix.line_length; - Bpp = info->var.bits_per_pixel >> 3; + bit_pp = info->var.bits_per_pixel >> 3; /* TODO: Implement hardware acceleration for image->depth > 1 */ if (image->depth != 1) { @@ -265,21 +257,10 @@ static void lynxfb_ops_imageblit(struct fb_info *info, bgcol = image->bg_color; } - /* - * If not use spin_lock, system will die if user load driver - * and immediately unload driver frequently (dual) - */ - if (sm750_dev->fb_count > 1) - spin_lock(&sm750_dev->slock); - - sm750_dev->accel.de_imageblit(&sm750_dev->accel, - image->data, image->width >> 3, 0, - base, pitch, Bpp, - image->dx, image->dy, - image->width, image->height, - fgcol, bgcol, HW_ROP2_COPY); - if (sm750_dev->fb_count > 1) - spin_unlock(&sm750_dev->slock); + dual_safe_call(sm750_dev->accel.de_imageblit, &sm750_dev->accel, + image->data, image->width >> 3, 0, base, pitch, + bit_pp, image->dx, image->dy, image->width, + image->height, fgcol, bgcol, HW_ROP2_COPY); } static int lynxfb_ops_pan_display(struct fb_var_screeninfo *var,
The locking and unlocking code used by copy routines is common, so moved it to a macro. Signed-off-by: Dhananjay Balan <mail@dbalan.in> --- drivers/staging/sm750fb/sm750.c | 81 ++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 50 deletions(-)