* [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compilation logic
@ 2011-02-20 16:53 Javier Martinez Canillas
2011-02-20 16:53 ` [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage Javier Martinez Canillas
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Javier Martinez Canillas @ 2011-02-20 16:53 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Bill Pemberton, Arnaud Patard, Randy Dunlap, devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage 2011-02-20 16:53 [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compilation logic Javier Martinez Canillas @ 2011-02-20 16:53 ` Javier Martinez Canillas 2011-02-20 17:53 ` Dan Carpenter 2011-02-20 16:53 ` [PATCH 2/2] Staging: xgifb: Removes CRIT[FLAGS | BEGIN | END] defines and conditional spinlock compilation logic Javier Martinez Canillas 2011-02-20 19:36 ` [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd " aaro.koskinen 2 siblings, 1 reply; 5+ messages in thread From: Javier Martinez Canillas @ 2011-02-20 16:53 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Bill Pemberton, Arnaud Patard, Randy Dunlap, devel, linux-kernel, kernel-janitors, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage 2011-02-20 16:53 ` [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage Javier Martinez Canillas @ 2011-02-20 17:53 ` Dan Carpenter 0 siblings, 0 replies; 5+ messages in thread From: Dan Carpenter @ 2011-02-20 17:53 UTC (permalink / raw) To: Javier Martinez Canillas Cc: Greg Kroah-Hartman, Randy Dunlap, devel, kernel-janitors, linux-kernel, Arnaud Patard 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] Staging: xgifb: Removes CRIT[FLAGS | BEGIN | END] defines and conditional spinlock compilation logic 2011-02-20 16:53 [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compilation logic Javier Martinez Canillas 2011-02-20 16:53 ` [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage Javier Martinez Canillas @ 2011-02-20 16:53 ` Javier Martinez Canillas 2011-02-20 19:36 ` [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd " aaro.koskinen 2 siblings, 0 replies; 5+ messages in thread From: Javier Martinez Canillas @ 2011-02-20 16:53 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Bill Pemberton, Arnaud Patard, Randy Dunlap, devel, linux-kernel, kernel-janitors, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compilation logic 2011-02-20 16:53 [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compilation logic Javier Martinez Canillas 2011-02-20 16:53 ` [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage Javier Martinez Canillas 2011-02-20 16:53 ` [PATCH 2/2] Staging: xgifb: Removes CRIT[FLAGS | BEGIN | END] defines and conditional spinlock compilation logic Javier Martinez Canillas @ 2011-02-20 19:36 ` aaro.koskinen 2 siblings, 0 replies; 5+ messages in thread From: aaro.koskinen @ 2011-02-20 19:36 UTC (permalink / raw) To: martinez.javier, gregkh Cc: wfp5p, apatard, randy.dunlap, devel, linux-kernel, kernel-janitors 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-02-20 19:36 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-02-20 16:53 [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd conditional spinlock compilation logic Javier Martinez Canillas 2011-02-20 16:53 ` [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines usage Javier Martinez Canillas 2011-02-20 17:53 ` Dan Carpenter 2011-02-20 16:53 ` [PATCH 2/2] Staging: xgifb: Removes CRIT[FLAGS | BEGIN | END] defines and conditional spinlock compilation logic Javier Martinez Canillas 2011-02-20 19:36 ` [PATCH 0/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END] defines nd " aaro.koskinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox