Re: [PATCH] staging: sm750fb: always take the lock
On Mon, Jun 26, 2017 at 11:16 AM, Frans Klaver wrote: > On Mon, Jun 26, 2017 at 11:11 AM, Geert Uytterhoeven > wrote: >> On Mon, Jun 26, 2017 at 7:45 AM, AbdAllah-MEZITI >> wrote: >>> This patch >>> - will always take the lock >> >> Why? >> >> "The current code only takes the lock if multiple instances are in use. >> This is error-prone, and confuses static analyzers. >> As taking the lock in case of a single instance is harmful and cheap, >> change the code to always take the lock." > > I would argue that it's not harmful, lest people get confused about Sorry, I meant "harmless". I should start caring as much for commit messages I write for other people as for those I write for myself ;-) > it. And I agree that this explanation is much more useful than just > mentioning the warnings that you saw. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH] staging: sm750fb: always take the lock
On Mon, Jun 26, 2017 at 11:11 AM, Geert Uytterhoeven wrote: > On Mon, Jun 26, 2017 at 7:45 AM, AbdAllah-MEZITI > wrote: >> This patch >> - will always take the lock > > Why? > > "The current code only takes the lock if multiple instances are in use. > This is error-prone, and confuses static analyzers. > As taking the lock in case of a single instance is harmful and cheap, > change the code to always take the lock." I would argue that it's not harmful, lest people get confused about it. And I agree that this explanation is much more useful than just mentioning the warnings that you saw. Thanks, Frans
Re: [PATCH] staging: sm750fb: always take the lock
On Mon, Jun 26, 2017 at 7:45 AM, AbdAllah-MEZITI wrote: > This patch > - will always take the lock Why? "The current code only takes the lock if multiple instances are in use. This is error-prone, and confuses static analyzers. As taking the lock in case of a single instance is harmful and cheap, change the code to always take the lock." > - fix the sparse warning: > drivers/staging/sm750fb/sm750.c:159:13: warning: context imbalance in > 'lynxfb_ops_fillrect' - different lock contexts for basic block > drivers/staging/sm750fb/sm750.c:231:9: warning: context imbalance in > 'lynxfb_ops_copyarea' - different lock contexts for basic block > drivers/staging/sm750fb/sm750.c:235:13: warning: context imbalance in > 'lynxfb_ops_imageblit' - different lock contexts for basic block Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH] staging: sm750fb: always take the lock
There's no version number. Which one is the correct one? On Mon, Jun 26, 2017 at 7:45 AM, AbdAllah-MEZITI wrote: > This patch > - will always take the lock > - fix the sparse warning: > drivers/staging/sm750fb/sm750.c:159:13: warning: context imbalance in > 'lynxfb_ops_fillrect' - different lock contexts for basic block > drivers/staging/sm750fb/sm750.c:231:9: warning: context imbalance in > 'lynxfb_ops_copyarea' - different lock contexts for basic block > drivers/staging/sm750fb/sm750.c:235:13: warning: context imbalance in > 'lynxfb_ops_imageblit' - different lock contexts for basic block > > Signed-off-by: AbdAllah MEZITI > --- > drivers/staging/sm750fb/sm750.c | 18 ++ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c > index 386d4ad..4a22190 100644 > --- a/drivers/staging/sm750fb/sm750.c > +++ b/drivers/staging/sm750fb/sm750.c > @@ -186,16 +186,14 @@ static void lynxfb_ops_fillrect(struct fb_info *info, > * 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); > + 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); > + spin_unlock(&sm750_dev->slock); > } > > static void lynxfb_ops_copyarea(struct fb_info *info, > @@ -220,16 +218,14 @@ static void lynxfb_ops_copyarea(struct fb_info *info, > * 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); > + 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); > + spin_unlock(&sm750_dev->slock); > } > > static void lynxfb_ops_imageblit(struct fb_info *info, > @@ -269,8 +265,7 @@ static void lynxfb_ops_imageblit(struct fb_info *info, > * 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); > + spin_lock(&sm750_dev->slock); > > sm750_dev->accel.de_imageblit(&sm750_dev->accel, > image->data, image->width >> 3, 0, > @@ -278,8 +273,7 @@ static void lynxfb_ops_imageblit(struct fb_info *info, > image->dx, image->dy, > image->width, image->height, > fgcol, bgcol, HW_ROP2_COPY); > - if (sm750_dev->fb_count > 1) > - spin_unlock(&sm750_dev->slock); > + spin_unlock(&sm750_dev->slock); > } > > static int lynxfb_ops_pan_display(struct fb_var_screeninfo *var, > -- > 2.7.4 >
Re: [PATCH] staging: sm750fb: always take the lock
On Sun, Jun 25, 2017 at 11:39 PM, AbdAllah-MEZITI wrote: > Subject: [PATCH] staging: sm750fb: always take the lock When sending a new version of your patch, include a version number: Subject: [PATCH V2] staging: ... Frans
Re: [PATCH] staging: sm750fb: always take the lock
On Sun, Jun 25, 2017 at 11:39:20PM +0200, AbdAllah-MEZITI wrote: > Signed-off-by: AbdAllah MEZITI I can't take patches without any changelog text, sorry. greg k-h