[PATCH] staging: sm750fb: Fix lock context error

Subsystems: staging - silicon motion sm750 frame buffer driver, staging subsystem, the rest

STALE3596d

3 messages, 2 authors, 2016-10-24 · open the first message on its own page

[PATCH] staging: sm750fb: Fix lock context error

From: Alex Briskin <hidden>
Date: 2016-10-23 12:37:57

Sparse error fix - different lock contexts for basic block.
Acquirement and release of spin lock was dependent on two separate
unprotected variable evaluations. Instead the condition evaluation result
is stored in a local boolean variable to make sure that the same context
that called the spin_lock will evoke spin_unlock.

Signed-off-by: Alex Briskin <redacted>
---
 drivers/staging/sm750fb/sm750.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 0663ec0..4f0f98e 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c

@@ -164,6 +164,7 @@ static void lynxfb_ops_fillrect(struct fb_info *info,
 	struct sm750_dev *sm750_dev;
 	unsigned int base, pitch, Bpp, rop;
 	u32 color;
+	bool lock_in_this_context = false;
 
 	if (info->state != FBINFO_STATE_RUNNING)
 		return;

@@ -187,7 +188,8 @@ 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)
+	lock_in_this_context = sm750_dev->fb_count > 1;
+	if (lock_in_this_context)
 		spin_lock(&sm750_dev->slock);
 
 	sm750_dev->accel.de_fillrect(&sm750_dev->accel,

@@ -195,7 +197,7 @@ static void lynxfb_ops_fillrect(struct fb_info *info,
 				     region->dx, region->dy,
 				     region->width, region->height,
 				     color, rop);
-	if (sm750_dev->fb_count > 1)
+	if (lock_in_this_context)
 		spin_unlock(&sm750_dev->slock);
 }

@@ -205,6 +207,7 @@ static void lynxfb_ops_copyarea(struct fb_info *info,
 	struct lynxfb_par *par;
 	struct sm750_dev *sm750_dev;
 	unsigned int base, pitch, Bpp;
+	bool lock_in_this_context = false;
 
 	par = info->par;
 	sm750_dev = par->dev;

@@ -221,7 +224,8 @@ 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)
+	lock_in_this_context = sm750_dev->fb_count > 1;
+	if (lock_in_this_context)
 		spin_lock(&sm750_dev->slock);
 
 	sm750_dev->accel.de_copyarea(&sm750_dev->accel,

@@ -229,7 +233,7 @@ static void lynxfb_ops_copyarea(struct fb_info *info,
 				     base, pitch, Bpp, region->dx, region->dy,
 				     region->width, region->height,
 				     HW_ROP2_COPY);
-	if (sm750_dev->fb_count > 1)
+	if (lock_in_this_context)
 		spin_unlock(&sm750_dev->slock);
 }

-- 
2.7.4

Re: [PATCH] staging: sm750fb: Fix lock context error

From: Dan Carpenter <hidden>
Date: 2016-10-24 10:41:39

On Sun, Oct 23, 2016 at 03:37:57PM +0300, Alex Briskin wrote:

Sparse error fix - different lock contexts for basic block.
Acquirement and release of spin lock was dependent on two separate
unprotected variable evaluations. Instead the condition evaluation result
is stored in a local boolean variable to make sure that the same context
that called the spin_lock will evoke spin_unlock.

I kind of feel like this doesn't improve readability.

Sparse kind of sucks at flow analysis so I feel like working around
Sparse limitations is a waste of time.

regards,
dan carpenter

Re: [PATCH] staging: sm750fb: Fix lock context error

From: <hidden>
Date: 2016-10-24 12:12:40

On Monday, 24 October 2016 13:41:39 IDT Dan Carpenter wrote:

evaluation

Hi Dan,
Thanks you for your feedback, it kind of felt like workaround.
However, it seems to me like bad design decision took place where locking/
unlocking is dependent on unprotected variable.
From what I could understand from the driver source code the fb_count value 
shouldn't change while in those functions, so the code is safe "as is" and my 
change should only assist with readability. 
So the patch fails to improve readability it is unnecessary.

Best regards,
Alex

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help