From: John Zielinski <hidden> Date: 2004-07-02 17:29:49
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
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
From: David Eger <hidden> Date: 2004-07-02 20:14:15
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2004-07-05 13:45:06
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
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*
quoted
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).
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
From: David Eger <hidden> Date: 2004-07-05 21:24:47
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:
quoted
quoted
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2004-07-05 22:04:08
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 [off-list ref]
-------------------------------------------------------
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