[PATCH] radeonfb: remove reg_lock nonsense

[David Eger <eger@havoc.gtf.org>]

On Mon, Jul 05, 2004 at 10:34:29AM -0700, Linus Torvalds wrote:
> On Mon, 5 Jul 2004 akpm@osdl.org wrote:
> >
> > From: David Eger <eger@havoc.gtf.org>
> > 
> > radeonfb: the Stanford lock checker found us double-locking rinfo->reg_lock
> > via sequences like OUTPLL(foo, INPLL(bar) | baz ), as both OUTPLL and INPLL
> > grab the register lock.  This should fix the problem.
> 
> Ok, I applied it, since I don't have a better tested alternative, but it's 
> really very very ugly.
>
  [ generally well-meaning suggestions - snip ]  
>
> Ben, any comments? How did this code ever survive? Why didn't my G5 lock 
> up?

I agree, the patch is horrid.  In fact, now that I really look at it,
it seems that the spinlock is totally bogus.  This patch removes it.
Details below.

BenH: thoughts, objections?

-dte

This patch removes all of the reg_lock nonsense in radeonfb, reversing
the ugliness of my previous patch to placate the Stanford lock checker,
and getting rid of any false sense of security we had seeing a spinlock
in the radeonfb_info.

The truth seems, there's no rhyme nor reason to the use of the register 
lock in radeonfb_info.  It becomes apparent when you look 
past your nose ;-)

The macros INREG() and OUTREG() don't touch it while INPLL(), OUTPLL(),
OUTPLLP(), and OUTREGP() do.  Nonetheless, we have:

* radeonfb_engine_reset() writes to CLOCK_CNTL_INDEX without taking the
  register lock, while radeon_write_pll_regs() uses OUTREGP() to modify it.

* there are these nonsensical quick-successions of grabbing the same lock
  with OUTPLL() calls
  
I think the only places where sane register access arbitration takes
place is over in *cough* drivers/char/drm *cough* and the VC code
(which, AFAIK, already serializes access to the FB device, no?)

Signed-off-by: David Eger <eger@havoc.gtf.org>

diff -Nru a/drivers/video/aty/radeon_base.c b/drivers/video/aty/radeon_base.c
--- a/drivers/video/aty/radeon_base.c	2004-07-06 10:02:45 +02:00
+++ b/drivers/video/aty/radeon_base.c	2004-07-06 10:02:45 +02:00
@@ -2087,7 +2087,6 @@
 	rinfo->info = info;	
 	rinfo->pdev = pdev;
 	
-	spin_lock_init(&rinfo->reg_lock);
 	init_timer(&rinfo->lvds_timer);
 	rinfo->lvds_timer.function = radeon_lvds_timer_func;
 	rinfo->lvds_timer.data = (unsigned long)rinfo;
diff -Nru a/drivers/video/aty/radeon_pm.c b/drivers/video/aty/radeon_pm.c
--- a/drivers/video/aty/radeon_pm.c	2004-07-06 10:02:45 +02:00
+++ b/drivers/video/aty/radeon_pm.c	2004-07-06 10:02:45 +02:00
@@ -319,30 +319,26 @@
 
 static void radeon_pm_program_v2clk(struct radeonfb_info *rinfo)
 {
-	/* we use __INPLL and _OUTPLL and do the locking ourselves... */
-	unsigned long flags;
-	spin_lock_irqsave(&rinfo->reg_lock, flags);
 	/* Set v2clk to 65MHz */
-  	__OUTPLL(pllPIXCLKS_CNTL,
-  		__INPLL(rinfo, pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK);
+  	OUTPLL(pllPIXCLKS_CNTL,
+  		INPLL(pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK);
 	 
-  	__OUTPLL(pllP2PLL_REF_DIV, 0x0000000c);
-	__OUTPLL(pllP2PLL_CNTL, 0x0000bf00);
-	__OUTPLL(pllP2PLL_DIV_0, 0x00020074 | P2PLL_DIV_0__P2PLL_ATOMIC_UPDATE_W);
+  	OUTPLL(pllP2PLL_REF_DIV, 0x0000000c);
+	OUTPLL(pllP2PLL_CNTL, 0x0000bf00);
+	OUTPLL(pllP2PLL_DIV_0, 0x00020074 | P2PLL_DIV_0__P2PLL_ATOMIC_UPDATE_W);
 	
-	__OUTPLL(pllP2PLL_CNTL,
-		__INPLL(rinfo, pllP2PLL_CNTL) & ~P2PLL_CNTL__P2PLL_SLEEP);
+	OUTPLL(pllP2PLL_CNTL,
+		INPLL(pllP2PLL_CNTL) & ~P2PLL_CNTL__P2PLL_SLEEP);
 	mdelay(1);
 
-	__OUTPLL(pllP2PLL_CNTL,
-		__INPLL(rinfo, pllP2PLL_CNTL) & ~P2PLL_CNTL__P2PLL_RESET);
+	OUTPLL(pllP2PLL_CNTL,
+		INPLL(pllP2PLL_CNTL) & ~P2PLL_CNTL__P2PLL_RESET);
 	mdelay( 1);
 
-  	__OUTPLL(pllPIXCLKS_CNTL,
-  		(__INPLL(rinfo, pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK)
+  	OUTPLL(pllPIXCLKS_CNTL,
+  		(INPLL(pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK)
   		| (0x03 << PIXCLKS_CNTL__PIX2CLK_SRC_SEL__SHIFT));
 	mdelay( 1);	
-	spin_unlock_irqrestore(&rinfo->reg_lock, flags);
 }
 
 static void radeon_pm_low_current(struct radeonfb_info *rinfo)
@@ -397,7 +393,6 @@
 	u32 pixclks_cntl;
 	u32 disp_mis_cntl;
 	u32 disp_pwr_man;
-	u32 tmp;
 	
 	/* Force Core Clocks */
 	sclk_cntl = INPLL( pllSCLK_CNTL_M6);
@@ -503,9 +498,8 @@
 	
 	clk_pin_cntl &= ~CLK_PIN_CNTL__ACCESS_REGS_IN_SUSPEND;
 
-	/* because both INPLL and OUTPLL take the same lock, that's why. */
-	tmp = INPLL( pllMCLK_MISC) | MCLK_MISC__EN_MCLK_TRISTATE_IN_SUSPEND;
-	OUTPLL( pllMCLK_MISC, tmp);
+	OUTPLL( pllMCLK_MISC, 
+		INPLL( pllMCLK_MISC) | MCLK_MISC__EN_MCLK_TRISTATE_IN_SUSPEND);
 	
 	/* AGP PLL control */
 	OUTREG(BUS_CNTL1, INREG(BUS_CNTL1) |  BUS_CNTL1__AGPCLK_VALID);
@@ -525,9 +519,8 @@
 		| (0x20<<AGP_CNTL__MAX_IDLE_CLK__SHIFT));
 
 	/* ACPI mode */
-	/* because both INPLL and OUTPLL take the same lock, that's why. */
-	tmp = INPLL( pllPLL_PWRMGT_CNTL) & ~PLL_PWRMGT_CNTL__PM_MODE_SEL;
-	OUTPLL( pllPLL_PWRMGT_CNTL, tmp);
+	OUTPLL( pllPLL_PWRMGT_CNTL, 
+		INPLL( pllPLL_PWRMGT_CNTL) & ~PLL_PWRMGT_CNTL__PM_MODE_SEL);
 
 
 	disp_mis_cntl = INREG(DISP_MISC_CNTL);
@@ -778,7 +771,6 @@
 static void radeon_set_suspend(struct radeonfb_info *rinfo, int suspend)
 {
 	u16 pwr_cmd;
-	u32 tmp;
 
 	if (!rinfo->pm_reg)
 		return;
@@ -815,8 +807,9 @@
 
 			/* Reset the MDLL */
 			/* because both INPLL and OUTPLL take the same lock, that's why. */
-			tmp = INPLL( pllMDLL_CKO) | MDLL_CKO__MCKOA_RESET | MDLL_CKO__MCKOB_RESET;
-			OUTPLL( pllMDLL_CKO, tmp );
+			
+			OUTPLL( pllMDLL_CKO,
+				INPLL( pllMDLL_CKO) | MDLL_CKO__MCKOA_RESET | MDLL_CKO__MCKOB_RESET);
 		}
 
 		/* Switch PCI power managment to D2. */
diff -Nru a/drivers/video/aty/radeonfb.h b/drivers/video/aty/radeonfb.h
--- a/drivers/video/aty/radeonfb.h	2004-07-06 10:02:45 +02:00
+++ b/drivers/video/aty/radeonfb.h	2004-07-06 10:02:45 +02:00
@@ -311,9 +311,6 @@
 	int			asleep;
 	int			lock_blank;
 
-	/* Lock on register access */
-	spinlock_t		reg_lock;
-
 	/* Timer used for delayed LVDS operations */
 	struct timer_list	lvds_timer;
 	u32			pending_lvds_gen_cntl;
@@ -363,11 +360,11 @@
 	OUTREG(CLOCK_CNTL_INDEX, save);
 }
 
-#define __OUTPLL(addr,val)	\
-	do {	\
-		OUTREG8(CLOCK_CNTL_INDEX, (addr & 0x0000003f) | 0x00000080); \
-		OUTREG(CLOCK_CNTL_DATA, val); \
-} while(0)
+static inline u32 __OUTPLL(struct radeonfb_info *rinfo, u32 addr, u32 val)
+{
+		OUTREG8(CLOCK_CNTL_INDEX, (addr & 0x0000003f) | 0x00000080);
+		OUTREG(CLOCK_CNTL_DATA, val);
+}
 
 
 static inline u32 __INPLL(struct radeonfb_info *rinfo, u32 addr)
@@ -380,49 +377,25 @@
 	return data;
 }
 
-static inline u32 _INPLL(struct radeonfb_info *rinfo, u32 addr)
-{
-       	unsigned long flags;
-	u32 data;
-
-	spin_lock_irqsave(&rinfo->reg_lock, flags);
-	data = __INPLL(rinfo, addr);
-	spin_unlock_irqrestore(&rinfo->reg_lock, flags);
-	return data;
-}
-
-#define INPLL(addr)		_INPLL(rinfo, addr)
-
-#define OUTPLL(addr,val)	\
-	do {	\
-		unsigned long flags;\
-		spin_lock_irqsave(&rinfo->reg_lock, flags); \
-		__OUTPLL(addr, val); \
-		spin_unlock_irqrestore(&rinfo->reg_lock, flags); \
-	} while(0)
+#define INPLL(addr)       __INPLL(rinfo, addr)
+#define OUTPLL(addr, val) __OUTPLL(rinfo, addr, val)
 
 #define OUTPLLP(addr,val,mask)  					\
 	do {								\
-		unsigned long flags;                                    \
 		unsigned int _tmp;					\
-		spin_lock_irqsave(&rinfo->reg_lock, flags); 		\
 		_tmp  = __INPLL(rinfo,addr);				\
 		_tmp &= (mask);						\
 		_tmp |= (val);						\
-		__OUTPLL(addr, _tmp);					\
-		spin_unlock_irqrestore(&rinfo->reg_lock, flags); 	\
+		__OUTPLL(rinfo, addr, _tmp);					\
 	} while (0)
 
 #define OUTREGP(addr,val,mask)  					\
 	do {								\
-		unsigned long flags;                                    \
 		unsigned int _tmp;					\
-		spin_lock_irqsave(&rinfo->reg_lock, flags); 		\
 		_tmp = INREG(addr);				       	\
 		_tmp &= (mask);						\
 		_tmp |= (val);						\
 		OUTREG(addr, _tmp);					\
-		spin_unlock_irqrestore(&rinfo->reg_lock, flags); 	\
 	} while (0)
 
 #define MS_TO_HZ(ms)       ((ms * HZ + 999) / 1000)


-------------------------------------------------------
This SF.Net email sponsored by Black Hat Briefings & Training.
Attend Black Hat Briefings & Training, Las Vegas July 24-29 - 
digital self defense, top technical experts, no vendor pitches, 
unmatched networking opportunities. Visit www.blackhat.com