Re: [patch 7/8] lock validator: fix ns83820.c irq-flags bug

Re: [patch 7/8] lock validator: fix ns83820.c irq-flags bug

[Jeff Garzik]

akpm@osdl.org wrote:
> From: Ingo Molnar <mingo@elte.hu>
> 
> Barry K. Nathan reported the following lockdep warning:
> 
> [  197.343948] BUG: warning at kernel/lockdep.c:1856/trace_hardirqs_on()
> [  197.345928]  [<c010329b>] show_trace_log_lvl+0x5b/0x105
> [  197.346359]  [<c0103896>] show_trace+0x1b/0x20
> [  197.346759]  [<c01038ed>] dump_stack+0x1f/0x24
> [  197.347159]  [<c012efa2>] trace_hardirqs_on+0xfb/0x185
> [  197.348873]  [<c029b009>] _spin_unlock_irq+0x24/0x2d
> [  197.350620]  [<e09034e8>] do_tx_done+0x171/0x179 [ns83820]
> [  197.350895]  [<e090445c>] ns83820_irq+0x149/0x20b [ns83820]
> [  197.351166]  [<c013b4b8>] handle_IRQ_event+0x1d/0x52
> [  197.353216]  [<c013c6c2>] handle_level_irq+0x97/0xe1
> [  197.355157]  [<c01048c3>] do_IRQ+0x8b/0xac
> [  197.355612]  [<c0102d9d>] common_interrupt+0x25/0x2c

The driver's locking is definitely wrong, but I don't think this is the 
fix, because PCI drivers with a single interrupt should be using 
spin_lock() in the interrupt handler.  Anything more would be uncivilized :)

/me starts to do a better patch...

Jesus, the locking here is awful.  No wonder there are bugs.

Since this driver isn't seeing a ton of work these days, I think the 
best thing to do would be to _simplify_ the locking.  This driver is 
grabbing so many locks, turning interrupts off+on so often that any 
benefit the multiple locks had is probably long gone, particularly on 
modern machines.

Let me see what I can do with it...

	Jeff

Re: [patch 7/8] lock validator: fix ns83820.c irq-flags bug

[Arjan van de Ven]

Jeff Garzik wrote:
> The driver's locking is definitely wrong, but I don't think this is the 
> fix, 

it's an obvious correct fix in the correctness sense though...

> 
> Jesus, the locking here is awful.  No wonder there are bugs.


... which given that fact, is for 2.6.17 probably the right thing, pending
a nicer fix for 2.6.18

I fully agree with you that the locking is trying to be WAAAY too smart
for it's own good, and that a much simpler scheme is called for.

Re: [patch 7/8] lock validator: fix ns83820.c irq-flags bug

[Jeff Garzik]

Arjan van de Ven wrote:
> Jeff Garzik wrote:
>> The driver's locking is definitely wrong, but I don't think this is 
>> the fix, 
> 
> it's an obvious correct fix in the correctness sense though...
> 
>>
>> Jesus, the locking here is awful.  No wonder there are bugs.
> 
> 
> ... which given that fact, is for 2.6.17 probably the right thing, pending
> a nicer fix for 2.6.18

I disagree, the patch is wrong too.

For normal PCI hardware, the in-ISR paths should all use spin_lock(), 
and the outside-ISR paths should use either spin_lock_irq() or 
spin_lock_irqsave().

	Jeff

Re: [patch 7/8] lock validator: fix ns83820.c irq-flags bug

[Arjan van de Ven]

Jeff Garzik wrote:
> Arjan van de Ven wrote:
>> Jeff Garzik wrote:
>>> The driver's locking is definitely wrong, but I don't think this is 
>>> the fix, 
>>
>> it's an obvious correct fix in the correctness sense though...
>>
>>>
>>> Jesus, the locking here is awful.  No wonder there are bugs.
>>
>>
>> ... which given that fact, is for 2.6.17 probably the right thing, 
>> pending
>> a nicer fix for 2.6.18
> 
> I disagree, the patch is wrong too.

wrong as in "not quite optimal", not wrong as in "buggy".

> For normal PCI hardware, the in-ISR paths should all use spin_lock(), 

only for per hardware locks obviously, not for per driver locks ;)


you are right that it's a lot nicer to do what you describe. No argument
from me on that part. But to call it "wrong" or "incorrect" is not quite
ok.  In terms of changing/fixing the approach we did was the simplest one.
Not the "make it look nice" one. Fix it by making the bug go away in the light
of a LOT of fishy locking.

You can demand that we first fix all the fishy locking first, and I can even
in part agree with that, but for -stable and 2.6.17 that is obviously out
of scope while a simple "make the bug go away" fix is not.

Re: [patch 7/8] lock validator: fix ns83820.c irq-flags bug

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

akpm@osdl.org wrote:
> diff -puN drivers/net/ns83820.c~lock-validator-fix-ns83820c-irq-flags-bug drivers/net/ns83820.c
> --- devel/drivers/net/ns83820.c~lock-validator-fix-ns83820c-irq-flags-bug	2006-06-08 22:18:34.000000000 -0700
> +++ devel-akpm/drivers/net/ns83820.c	2006-06-08 22:18:48.000000000 -0700
> @@ -804,7 +804,7 @@ static int ns83820_setup_rx(struct net_d
>  
>  		writel(dev->IMR_cache, dev->base + IMR);
>  		writel(1, dev->base + IER);
> -		spin_unlock_irq(&dev->misc_lock);
> +		spin_unlock(&dev->misc_lock);
>  
>  		kick_rx(ndev);
>  

The above code snippet removes the nested unlock-irq, but now the code 
is unbalanced, so IMO this patch _adds_ confusion.

I think the conservative patch for 2.6.17 is the one I have attached. 
Unless there are objections, that is what I will forward...

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1156 bytes --]

diff --git a/drivers/net/ns83820.c b/drivers/net/ns83820.c
index 706aed7..cee0e74 100644
--- a/drivers/net/ns83820.c
+++ b/drivers/net/ns83820.c
@@ -776,9 +776,11 @@ static int ns83820_setup_rx(struct net_d
 
 	ret = rx_refill(ndev, GFP_KERNEL);
 	if (!ret) {
+		unsigned long flags;
+
 		dprintk("starting receiver\n");
 		/* prevent the interrupt handler from stomping on us */
-		spin_lock_irq(&dev->rx_info.lock);
+		spin_lock_irqsave(&dev->rx_info.lock, flags);
 
 		writel(0x0001, dev->base + CCSR);
 		writel(0, dev->base + RFCR);
@@ -790,7 +792,7 @@ static int ns83820_setup_rx(struct net_d
 		phy_intr(ndev);
 
 		/* Okay, let it rip */
-		spin_lock_irq(&dev->misc_lock);
+		spin_lock(&dev->misc_lock);
 		dev->IMR_cache |= ISR_PHY;
 		dev->IMR_cache |= ISR_RXRCMP;
 		//dev->IMR_cache |= ISR_RXERR;
@@ -804,11 +806,11 @@ static int ns83820_setup_rx(struct net_d
 
 		writel(dev->IMR_cache, dev->base + IMR);
 		writel(1, dev->base + IER);
-		spin_unlock_irq(&dev->misc_lock);
+		spin_unlock(&dev->misc_lock);
 
 		kick_rx(ndev);
 
-		spin_unlock_irq(&dev->rx_info.lock);
+		spin_unlock_irqrestore(&dev->rx_info.lock, flags);
 	}
 	return ret;
 }

Re: [patch 7/8] lock validator: fix ns83820.c irq-flags bug

[Arjan van de Ven]

> The above code snippet removes the nested unlock-irq, but now the code 
> is unbalanced, so IMO this patch _adds_ confusion.
> 
> I think the conservative patch for 2.6.17 is the one I have attached. 
> Unless there are objections, that is what I will forward...


this looks entirely fair and reasonable

Acked-by: Arjan van de Ven <arjan@linux.intel.com>

Re: [patch 7/8] lock validator: fix ns83820.c irq-flags bug

[Benjamin LaHaise]

> The above code snippet removes the nested unlock-irq, but now the code 
> is unbalanced, so IMO this patch _adds_ confusion.
> 
> I think the conservative patch for 2.6.17 is the one I have attached. 
> Unless there are objections, that is what I will forward...

This looks reasonable and sufficiently conservative.  Reworking locking is 
something that I'm a bit more hesitant about, although folding misc_lock 
in with the other locks perhaps makes sense.  I would like to keep the 
split between tx and tx completion, though.  Also, any rework is going to 
need real testing, which is not something that a simple release cycle is 
likely to get enough coverage on.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <dont@kvack.org>.