[PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compilation logic

[PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compilation logic

[Javier Martinez Canillas]

xgifb staging driver uses a set of defines that hides the synchronization
mechanism used to access critical sections. Also, the use of spinlocks
can be disabled in compile time.

Since the spinlocks ABI only are used in contexts were critical section exists
(UP with preemption enabled and SMP machines), I think we should always have
the spinlocks enabled and let the spinlock ABI choose to include the spinlocks
or not. In the other hand if the driver doesn't need locking at all, then
maybe we should just delete the spinlock logic.

This patchset first replaces all the defines used with explicit definitions,
then removes all the defines and the spinlocks optional compilation logic.

The patchset is composed of the following patches:

[PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage
[PATCH 2/2] Staging: xgifb: Removes CRIT[FLAGS | BEGIN | END] defines and conditional spinlock compilation logic

[PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compila

[Javier Martinez Canillas]

xgifb staging driver uses a set of defines that hides the synchronization
mechanism used to access critical sections. Also, the use of spinlocks
can be disabled in compile time.

Since the spinlocks ABI only are used in contexts were critical section exists
(UP with preemption enabled and SMP machines), I think we should always have
the spinlocks enabled and let the spinlock ABI choose to include the spinlocks
or not. In the other hand if the driver doesn't need locking at all, then
maybe we should just delete the spinlock logic.

This patchset first replaces all the defines used with explicit definitions,
then removes all the defines and the spinlocks optional compilation logic.

The patchset is composed of the following patches:

[PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage
[PATCH 2/2] Staging: xgifb: Removes CRIT[FLAGS | BEGIN | END] defines and conditional spinlock compilation logic

[PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage

[Javier Martinez Canillas]

Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
---
 drivers/staging/xgifb/XGI_accel.c |   43 ++++++++++++++++++++----------------
 1 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_accel.c b/drivers/staging/xgifb/XGI_accel.c
index 7954974..6ef2148 100644
--- a/drivers/staging/xgifb/XGI_accel.c
+++ b/drivers/staging/xgifb/XGI_accel.c
@@ -216,19 +216,21 @@ void XGIfb_syncaccel(void)
 
 int fbcon_XGI_sync(struct fb_info *info)
 {
-    if(!XGIfb_accel) return 0;
-    CRITFLAGS
+	unsigned long critflags = 0;
 
-    XGI310Sync();
+	if (!XGIfb_accel)
+		return 0;
+
+	XGI310Sync();
 
-   CRITEND
-   return 0;
+	spin_unlock_irqrestore(&xgi_video_info.lockaccel, critflags);
+	return 0;
 }
 
 void fbcon_XGI_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
 {
-   int col=0;
-   CRITFLAGS
+	int col = 0;
+	unsigned long critflags;
 
 
    if(!rect->width || !rect->height)
@@ -249,19 +251,20 @@ void fbcon_XGI_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
 	}
 
 
-	   CRITBEGIN
-	   XGI310SetupForSolidFill(col, myrops[rect->rop], 0);
-	   XGI310SubsequentSolidFillRect(rect->dx, rect->dy, rect->width, rect->height);
-	   CRITEND
-	   XGI310Sync();
+	spin_lock_irqsave(&xgi_video_info.lockaccel, critflags);
+	XGI310SetupForSolidFill(col, myrops[rect->rop], 0);
+	XGI310SubsequentSolidFillRect(rect->dx, rect->dy, rect->width,
+				      rect->height);
+	spin_unlock_irqrestore(&xgi_video_info.lockaccel, critflags);
+	XGI310Sync();
 
 
 }
 
 void fbcon_XGI_copyarea(struct fb_info *info, const struct fb_copyarea *area)
 {
-   int xdir, ydir;
-   CRITFLAGS
+	int xdir, ydir;
+	unsigned long critflags;
 
 
    if(!XGIfb_accel) {
@@ -277,11 +280,13 @@ void fbcon_XGI_copyarea(struct fb_info *info, const struct fb_copyarea *area)
    if(area->sy < area->dy) ydir = 0;
    else                    ydir = 1;
 
-      CRITBEGIN
-      XGI310SetupForScreenToScreenCopy(xdir, ydir, 3, 0, -1);
-      XGI310SubsequentScreenToScreenCopy(area->sx, area->sy, area->dx, area->dy, area->width, area->height);
-      CRITEND
-      XGI310Sync();
+	spin_lock_irqsave(&xgi_video_info.lockaccel, critflags);
+	XGI310SetupForScreenToScreenCopy(xdir, ydir, 3, 0, -1);
+	XGI310SubsequentScreenToScreenCopy(area->sx, area->sy, area->dx,
+					   area->dy, area->width,
+					   area->height);
+	spin_unlock_irqrestore(&xgi_video_info.lockaccel, critflags);
+	XGI310Sync();
 
 }
 
-- 
1.7.2.3

[PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage

[Javier Martinez Canillas]

Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
---
 drivers/staging/xgifb/XGI_accel.c |   43 ++++++++++++++++++++----------------
 1 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_accel.c b/drivers/staging/xgifb/XGI_accel.c
index 7954974..6ef2148 100644
--- a/drivers/staging/xgifb/XGI_accel.c
+++ b/drivers/staging/xgifb/XGI_accel.c
@@ -216,19 +216,21 @@ void XGIfb_syncaccel(void)
 
 int fbcon_XGI_sync(struct fb_info *info)
 {
-    if(!XGIfb_accel) return 0;
-    CRITFLAGS
+	unsigned long critflags = 0;
 
-    XGI310Sync();
+	if (!XGIfb_accel)
+		return 0;
+
+	XGI310Sync();
 
-   CRITEND
-   return 0;
+	spin_unlock_irqrestore(&xgi_video_info.lockaccel, critflags);
+	return 0;
 }
 
 void fbcon_XGI_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
 {
-   int col=0;
-   CRITFLAGS
+	int col = 0;
+	unsigned long critflags;
 
 
    if(!rect->width || !rect->height)
@@ -249,19 +251,20 @@ void fbcon_XGI_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
 	}
 
 
-	   CRITBEGIN
-	   XGI310SetupForSolidFill(col, myrops[rect->rop], 0);
-	   XGI310SubsequentSolidFillRect(rect->dx, rect->dy, rect->width, rect->height);
-	   CRITEND
-	   XGI310Sync();
+	spin_lock_irqsave(&xgi_video_info.lockaccel, critflags);
+	XGI310SetupForSolidFill(col, myrops[rect->rop], 0);
+	XGI310SubsequentSolidFillRect(rect->dx, rect->dy, rect->width,
+				      rect->height);
+	spin_unlock_irqrestore(&xgi_video_info.lockaccel, critflags);
+	XGI310Sync();
 
 
 }
 
 void fbcon_XGI_copyarea(struct fb_info *info, const struct fb_copyarea *area)
 {
-   int xdir, ydir;
-   CRITFLAGS
+	int xdir, ydir;
+	unsigned long critflags;
 
 
    if(!XGIfb_accel) {
@@ -277,11 +280,13 @@ void fbcon_XGI_copyarea(struct fb_info *info, const struct fb_copyarea *area)
    if(area->sy < area->dy) ydir = 0;
    else                    ydir = 1;
 
-      CRITBEGIN
-      XGI310SetupForScreenToScreenCopy(xdir, ydir, 3, 0, -1);
-      XGI310SubsequentScreenToScreenCopy(area->sx, area->sy, area->dx, area->dy, area->width, area->height);
-      CRITEND
-      XGI310Sync();
+	spin_lock_irqsave(&xgi_video_info.lockaccel, critflags);
+	XGI310SetupForScreenToScreenCopy(xdir, ydir, 3, 0, -1);
+	XGI310SubsequentScreenToScreenCopy(area->sx, area->sy, area->dx,
+					   area->dy, area->width,
+					   area->height);
+	spin_unlock_irqrestore(&xgi_video_info.lockaccel, critflags);
+	XGI310Sync();
 
 }
 
-- 
1.7.2.3

[PATCH 2/2] Staging: xgifb: Removes CRIT[FLAGS | BEGIN | END] defines and conditional spinlock compilation logic

[Javier Martinez Canillas]

Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
---
 drivers/staging/xgifb/XGI_accel.h |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_accel.h b/drivers/staging/xgifb/XGI_accel.h
index 5a0395b..786ac24 100644
--- a/drivers/staging/xgifb/XGI_accel.h
+++ b/drivers/staging/xgifb/XGI_accel.h
@@ -18,19 +18,8 @@
 #ifndef _XGIFB_ACCEL_H
 #define _XGIFB_ACCEL_H
 
-/* Guard accelerator accesses with spin_lock_irqsave? Works well without. */
-#undef XGIFB_USE_SPINLOCKS
 
-#ifdef XGIFB_USE_SPINLOCKS
 #include <linux/spinlock.h>
-#define CRITBEGIN  spin_lock_irqsave(&xgi_video_info.lockaccel), critflags);
-#define CRITEND	   spin_unlock_irqrestore(&xgi_video_info.lockaccel), critflags);
-#define CRITFLAGS  unsigned long critflags;
-#else
-#define CRITBEGIN
-#define CRITEND
-#define CRITFLAGS
-#endif
 
 /* Definitions for the XGI engine communication. */
 
-- 
1.7.2.3

[PATCH 2/2] Staging: xgifb: Removes CRIT[FLAGS | BEGIN | END] defines and conditional spinlock compi

[Javier Martinez Canillas]

Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
---
 drivers/staging/xgifb/XGI_accel.h |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_accel.h b/drivers/staging/xgifb/XGI_accel.h
index 5a0395b..786ac24 100644
--- a/drivers/staging/xgifb/XGI_accel.h
+++ b/drivers/staging/xgifb/XGI_accel.h
@@ -18,19 +18,8 @@
 #ifndef _XGIFB_ACCEL_H
 #define _XGIFB_ACCEL_H
 
-/* Guard accelerator accesses with spin_lock_irqsave? Works well without. */
-#undef XGIFB_USE_SPINLOCKS
 
-#ifdef XGIFB_USE_SPINLOCKS
 #include <linux/spinlock.h>
-#define CRITBEGIN  spin_lock_irqsave(&xgi_video_info.lockaccel), critflags);
-#define CRITEND	   spin_unlock_irqrestore(&xgi_video_info.lockaccel), critflags);
-#define CRITFLAGS  unsigned long critflags;
-#else
-#define CRITBEGIN
-#define CRITEND
-#define CRITFLAGS
-#endif
 
 /* Definitions for the XGI engine communication. */
 
-- 
1.7.2.3

Re: [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage

[Dan Carpenter]

Hi Javier,

Your idea was good but there are a couple problems with this patch.  I'm
afraid you're going to need to fix it and resend.

You put the patch description in the 0/2 patch.  It was a pretty decent
patch description.  Unfortunately the 0/2 descriptions get thrown away,
and do not get included in the final git log.  So they should go with
the individual patches.

On Sun, Feb 20, 2011 at 05:53:17PM +0100, Javier Martinez Canillas wrote:
>  int fbcon_XGI_sync(struct fb_info *info)
>  {
> -    if(!XGIfb_accel) return 0;
> -    CRITFLAGS
> +	unsigned long critflags = 0;

It's better to just call it "flags" instead of "critflags" so that it's
consistent with the rest of the kernel.

It's also not a good idea to initialize it to zero.  I know that gcc
prints a warning, but you can't just set it to zero to silence the
warning. Also this change should have been mentioned in the patch
description.  Every behavior change should be described in the change
log.

>  
> -    XGI310Sync();
> +	if (!XGIfb_accel)
> +		return 0;
> +
> +	XGI310Sync();
>  
> -   CRITEND
> -   return 0;
> +	spin_unlock_irqrestore(&xgi_video_info.lockaccel, critflags);

I know you didn't introduce it, but this makes no sense at all.  It's a
random unlock, and in the original code critflags was uninitialized.

The only reason this works is because XGIfb_accel is always 0.  So you 
can remove this function, and remove all the code that depends on
XGIfb_accel.  (In a separate patch of course).

regards,
dan carpenter

Re: [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END]

[Dan Carpenter]

Hi Javier,

Your idea was good but there are a couple problems with this patch.  I'm
afraid you're going to need to fix it and resend.

You put the patch description in the 0/2 patch.  It was a pretty decent
patch description.  Unfortunately the 0/2 descriptions get thrown away,
and do not get included in the final git log.  So they should go with
the individual patches.

On Sun, Feb 20, 2011 at 05:53:17PM +0100, Javier Martinez Canillas wrote:
>  int fbcon_XGI_sync(struct fb_info *info)
>  {
> -    if(!XGIfb_accel) return 0;
> -    CRITFLAGS
> +	unsigned long critflags = 0;

It's better to just call it "flags" instead of "critflags" so that it's
consistent with the rest of the kernel.

It's also not a good idea to initialize it to zero.  I know that gcc
prints a warning, but you can't just set it to zero to silence the
warning. Also this change should have been mentioned in the patch
description.  Every behavior change should be described in the change
log.

>  
> -    XGI310Sync();
> +	if (!XGIfb_accel)
> +		return 0;
> +
> +	XGI310Sync();
>  
> -   CRITEND
> -   return 0;
> +	spin_unlock_irqrestore(&xgi_video_info.lockaccel, critflags);

I know you didn't introduce it, but this makes no sense at all.  It's a
random unlock, and in the original code critflags was uninitialized.

The only reason this works is because XGIfb_accel is always 0.  So you 
can remove this function, and remove all the code that depends on
XGIfb_accel.  (In a separate patch of course).

regards,
dan carpenter

RE: [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compilation logic

[aaro.koskinen]

Hi,

From: Javier Martinez Canillas [martinez.javier@gmail.com]:
> xgifb staging driver uses a set of defines that hides the synchronization
> mechanism used to access critical sections. Also, the use of spinlocks
> can be disabled in compile time.
>
> Since the spinlocks ABI only are used in contexts were critical section exists
> (UP with preemption enabled and SMP machines), I think we should always have
> the spinlocks enabled and let the spinlock ABI choose to include the spinlocks
> or not. In the other hand if the driver doesn't need locking at all, then
> maybe we should just delete the spinlock logic.

I think these should be just deleted. The acceleration functions are used by the framebuffer
layer which should take care of concurrent access. Or can you point out some real scenario
where the spinlocks are needed?

A.

RE: [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END]

[aaro.koskinen]

Hi,

From: Javier Martinez Canillas [martinez.javier@gmail.com]:
> xgifb staging driver uses a set of defines that hides the synchronization
> mechanism used to access critical sections. Also, the use of spinlocks
> can be disabled in compile time.
>
> Since the spinlocks ABI only are used in contexts were critical section exists
> (UP with preemption enabled and SMP machines), I think we should always have
> the spinlocks enabled and let the spinlock ABI choose to include the spinlocks
> or not. In the other hand if the driver doesn't need locking at all, then
> maybe we should just delete the spinlock logic.

I think these should be just deleted. The acceleration functions are used by the framebuffer
layer which should take care of concurrent access. Or can you point out some real scenario
where the spinlocks are needed?

A.