radeon_pm.c locking problem

radeon_pm.c locking problem

[John Zielinski]

There's a post in the lkml from a Stanford researcher working on a tool 
to sniff out locking problems.  In his post he gave an example from the 
radeon_pm.c file.  Just want to make sure the appropriate people noticed 
the post.  It's subject is "[BUGS] [CHECKER] 99 synchronization bugs and 
a lock summary database".

John




-------------------------------------------------------
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

Re: radeon_pm.c locking problem

[Kronos]

Il Fri, Jul 02, 2004 at 01:29:38PM -0400, John Zielinski ha scritto: 
> *This message was transferred with a trial version of CommuniGate(tm) Pro*
> There's a post in the lkml from a Stanford researcher working on a tool 
> to sniff out locking problems.  In his post he gave an example from the 
> radeon_pm.c file.  Just want to make sure the appropriate people noticed 
> the post.  It's subject is "[BUGS] [CHECKER] 99 synchronization bugs and 
> a lock summary database".

Hum, it seems a real bug because OUTPLL is a macro. The following line
(radeon_pm.c:323, but there are more of them):

OUTPLL(pllPIXCLKS_CNTL, INPLL(pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK);

is expanded in:

do {
        unsigned long flags;
        spin_lock_irqsave(&rinfo->reg_lock, flags);
        __OUTPLL(pllPIXCLKS_CNTL, INPLL(pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK);
        spin_unlock_irqrestore(&rinfo->reg_lock, flags);
} while(0)

and INPLL will try to reacquire the lock ->reg_lock. We could either use
__INPLL (which does not take the lock) or make OUTPLL an inline function
(in this way INPLL is called before entering the function).

Ben?

Luca
-- 
Home: http://kronoz.cjb.net
#include <stdio.h> 
int main(void) {printf("\t\t\b\b\b\b\b\b");
printf("\t\t\b\b\b\b\b\b");return 0;}


-------------------------------------------------------
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

Re: radeon_pm.c locking problem

[David Eger]

I saw the post; the function affected is this:

static void radeon_pm_program_v2clk(struct radeonfb_info *rinfo)
{
        /* Set v2clk to 65MHz */
        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_CNTL, INPLL(pllP2PLL_CNTL) & ~P2PLL_CNTL__P2PLL_SLEEP);
        mdelay(1);

        OUTPLL(pllP2PLL_CNTL, INPLL(pllP2PLL_CNTL) & ~P2PLL_CNTL__P2PLL_RESET);
        mdelay( 1);

        OUTPLL(pllPIXCLKS_CNTL,
                (INPLL(pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK)
                | (0x03 << PIXCLKS_CNTL__PIX2CLK_SRC_SEL__SHIFT));
        mdelay( 1);
}

and the checker program catches the cases where we do something like:

OUTPLL(foo, INPLL(bar) & ~blah);

because both OUTPLL and INPLL grab rinfo->reg_lock.

I think the right solution here - because it's logically one change, no:
Reprogramming the pixel clocks? is to just use the primitive __OUTPLL and
__INPLL and do the locking ourselves around the whole sequence.

-dte


-------------------------------------------------------
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

Re: radeon_pm.c locking problem

[Benjamin Herrenschmidt]

> is expanded in:
> 
> do {
>         unsigned long flags;
>         spin_lock_irqsave(&rinfo->reg_lock, flags);
>         __OUTPLL(pllPIXCLKS_CNTL, INPLL(pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK);
>         spin_unlock_irqrestore(&rinfo->reg_lock, flags);
> } while(0)
> 
> and INPLL will try to reacquire the lock ->reg_lock. We could either use
> __INPLL (which does not take the lock) or make OUTPLL an inline function
> (in this way INPLL is called before entering the function).

Make it an inline function...

Ben.




-------------------------------------------------------
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

Re: radeon_pm.c locking problem

[Kronos]

Il Mon, Jul 05, 2004 at 08:44:53AM -0500, Benjamin Herrenschmidt ha scritto: 
> *This message was transferred with a trial version of CommuniGate(tm) Pro*
> 
> > is expanded in:
> > 
> > do {
> >         unsigned long flags;
> >         spin_lock_irqsave(&rinfo->reg_lock, flags);
> >         __OUTPLL(pllPIXCLKS_CNTL, INPLL(pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK);
> >         spin_unlock_irqrestore(&rinfo->reg_lock, flags);
> > } while(0)
> > 
> > and INPLL will try to reacquire the lock ->reg_lock. We could either use
> > __INPLL (which does not take the lock) or make OUTPLL an inline function
> > (in this way INPLL is called before entering the function).
> 
> Make it an inline function...

Here it is:

--- linux-2.6/drivers/video/aty/radeonfb.h.orig	2004-07-05 18:27:05.000000000 +0200
+++ linux-2.6/drivers/video/aty/radeonfb.h	2004-07-05 18:38:52.000000000 +0200
@@ -393,37 +393,43 @@
 
 #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 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); 	\
-	} 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)
+static inline void _OUTPLL(struct radeonfb_info *rinfo, unsigned int addr, unsigned int val)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&rinfo->reg_lock, flags);
+	__OUTPLL(addr, val);
+	spin_unlock_irqrestore(&rinfo->reg_lock, flags);
+}
+
+#define OUTPLL(addr,val)	_OUTPLL(rinfo, addr, val)
+
+static inline void _OUTPLLP(struct radeonfb_info *rinfo, unsigned int addr, unsigned int val, unsigned int mask)
+{
+	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);
+}
+
+#define OUTPLLP(addr,val,mask)	_OUTPLLP(rinfo,addr,val,mask)
+
+static inline void _OUTREGP(struct radeonfb_info *rinfo, unsigned int addr, unsigned int val, unsigned int mask)
+{
+	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);
+}
+
+#define OUTREGP(addr,val,mask)	_OUTREGP(rinfo,addr,val,mask)
 
 #define MS_TO_HZ(ms)       ((ms * HZ + 999) / 1000)
 

Luca
-- 
Home: http://kronoz.cjb.net
Runtime error 6D at f000:a12f : user incompetente


-------------------------------------------------------
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

Re: radeon_pm.c locking problem

[David Eger]

On Mon, Jul 05, 2004 at 06:41:04PM +0200, Kronos wrote:
> Il Mon, Jul 05, 2004 at 08:44:53AM -0500, Benjamin Herrenschmidt ha scritto: 
> > > is expanded in:
> > > 
> > > do {
> > >         unsigned long flags;
> > >         spin_lock_irqsave(&rinfo->reg_lock, flags);
> > >         __OUTPLL(pllPIXCLKS_CNTL, INPLL(pllPIXCLKS_CNTL) & ~PIXCLKS_CNTL__PIX2CLK_SRC_SEL_MASK);
> > >         spin_unlock_irqrestore(&rinfo->reg_lock, flags);
> > > } while(0)
> > > 
> > > and INPLL will try to reacquire the lock ->reg_lock. We could either use
> > > __INPLL (which does not take the lock) or make OUTPLL an inline function
> > > (in this way INPLL is called before entering the function).
> > 
> > Make it an inline function...
> 
> Here it is:

Your patch is much, much prettier than mine (which is in mainline),
and Linus is hoping for such a prettier solution.

On the other hand, is there any reason we're taking this
rinfo->reg_lock?   Would it be possible for the fb driver to be called
in two separate contexts at the same time?

Also, having this macro be pretty sort of obsccures the fact that, 
the way the code is now, we're doing

	OUTPLL(foo0, bar0);
	OUTPLL(foo1, bar1);
	OUTPLL(foo2, bar2);

which translates to taking this lock three times in quick succession.
Bleah.

I had half a mind to put a radeon_fifo_wait() call in the
OUTREG() macro (because there needs to be a 1-1 correspondence 
between empty fifo slots and OUTREG calls, but then you add a
chunk of a loop, udelay, and printk to every register write...
again lots of overhead for a simple register write.... 
better to sprinkle the code with the write counts and hope they stay
in line?  Maybe.  That's the route I took, anyways.

-dte


-------------------------------------------------------
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

Re: radeon_pm.c locking problem

[Benjamin Herrenschmidt]

> Your patch is much, much prettier than mine (which is in mainline),
> and Linus is hoping for such a prettier solution.
> 
> On the other hand, is there any reason we're taking this
> rinfo->reg_lock?   Would it be possible for the fb driver to be called
> in two separate contexts at the same time?

QUick fix from the original code I took over from, never had time to
rework that part. I still plan to rewrite most of the mode setting
code in there to, among others, deal with dual head & such, but I lack
time and it's a lot of work.

> Also, having this macro be pretty sort of obsccures the fact that, 
> the way the code is now, we're doing
> 
> 	OUTPLL(foo0, bar0);
> 	OUTPLL(foo1, bar1);
> 	OUTPLL(foo2, bar2);
> 
> which translates to taking this lock three times in quick succession.
> Bleah.
> 
> I had half a mind to put a radeon_fifo_wait() call in the
> OUTREG() macro (because there needs to be a 1-1 correspondence 
> between empty fifo slots and OUTREG calls, but then you add a
> chunk of a loop, udelay, and printk to every register write...
> again lots of overhead for a simple register write.... 
> better to sprinkle the code with the write counts and hope they stay
> in line?  Maybe.  That's the route I took, anyways.
> 
> -dte
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>



-------------------------------------------------------
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