* radeon_pm.c locking problem @ 2004-07-02 17:29 John Zielinski 2004-07-02 18:52 ` Kronos 2004-07-02 20:14 ` David Eger 0 siblings, 2 replies; 7+ messages in thread From: John Zielinski @ 2004-07-02 17:29 UTC (permalink / raw) To: linux-fbdev-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: radeon_pm.c locking problem 2004-07-02 17:29 radeon_pm.c locking problem John Zielinski @ 2004-07-02 18:52 ` Kronos 2004-07-05 13:44 ` Benjamin Herrenschmidt 2004-07-02 20:14 ` David Eger 1 sibling, 1 reply; 7+ messages in thread From: Kronos @ 2004-07-02 18:52 UTC (permalink / raw) To: John Zielinski; +Cc: linux-fbdev-devel, Benjamin Herrenschmidt 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: radeon_pm.c locking problem 2004-07-02 18:52 ` Kronos @ 2004-07-05 13:44 ` Benjamin Herrenschmidt 2004-07-05 16:41 ` Kronos 0 siblings, 1 reply; 7+ messages in thread From: Benjamin Herrenschmidt @ 2004-07-05 13:44 UTC (permalink / raw) To: Kronos; +Cc: John Zielinski, Linux Fbdev development list > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: radeon_pm.c locking problem 2004-07-05 13:44 ` Benjamin Herrenschmidt @ 2004-07-05 16:41 ` Kronos 2004-07-05 21:24 ` David Eger 0 siblings, 1 reply; 7+ messages in thread From: Kronos @ 2004-07-05 16:41 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Kronos, John Zielinski, Linux Fbdev development list 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: radeon_pm.c locking problem 2004-07-05 16:41 ` Kronos @ 2004-07-05 21:24 ` David Eger 2004-07-05 22:03 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 7+ messages in thread From: David Eger @ 2004-07-05 21:24 UTC (permalink / raw) To: Kronos; +Cc: Benjamin Herrenschmidt, John Zielinski, Linux Fbdev development list 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: radeon_pm.c locking problem 2004-07-05 21:24 ` David Eger @ 2004-07-05 22:03 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 7+ messages in thread From: Benjamin Herrenschmidt @ 2004-07-05 22:03 UTC (permalink / raw) To: David Eger; +Cc: Kronos, John Zielinski, Linux Fbdev development list > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: radeon_pm.c locking problem 2004-07-02 17:29 radeon_pm.c locking problem John Zielinski 2004-07-02 18:52 ` Kronos @ 2004-07-02 20:14 ` David Eger 1 sibling, 0 replies; 7+ messages in thread From: David Eger @ 2004-07-02 20:14 UTC (permalink / raw) To: John Zielinski; +Cc: linux-fbdev-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2004-07-05 22:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-07-02 17:29 radeon_pm.c locking problem John Zielinski 2004-07-02 18:52 ` Kronos 2004-07-05 13:44 ` Benjamin Herrenschmidt 2004-07-05 16:41 ` Kronos 2004-07-05 21:24 ` David Eger 2004-07-05 22:03 ` Benjamin Herrenschmidt 2004-07-02 20:14 ` David Eger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).