[PATCH 0/1] 3c59x: Acquire vortex lock instead of disabling irq

[PATCH 0/1] 3c59x: Acquire vortex lock instead of disabling irq

[Chase Douglas]

I believe an Ubuntu user has found an issue that has been resolved in the -rt
patchset for a few years. I pulled the fix from the patchset and built a test
kernel for the user, but I've yet to hear usable results back (test kernel
isn't booting right, but it may be due to other big changes that went in the
newer Ubuntu kernel I gave him). In the meantime, I'm sending this on for folks
to take a look at. All useful information can be found in the commit message,
including a link to the bug report with a BUG message.

-- Chase

[PATCH 1/1] 3c59x: Acquire vortex lock instead of disabling irq

[Chase Douglas]

Last year, threaded IRQ handlers were introduced to the mainline kernel.
This change requires the disable_irq function to sleep if any IRQ
handler threads for a given IRQ line are running.

Back in 2006, while working on the -rt patch set that had threaded IRQ
handlers, the vortex_timer function was causing scheduling bugs because
it is run in softirq context and called disable_irq. This patch was the
best fix determined at the time, and still exists in the .33 -rt
patchset. Now that threaded IRQ handlers are present in the mainline
kernel we need to apply the patch there as well.

http://lkml.org/lkml/2006/5/12/178

BugLink: http://bugs.launchpad.net/bugs/533335

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/net/3c59x.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
index f965431..bdaff0f 100644
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -1764,6 +1764,7 @@ vortex_timer(unsigned long data)
 	int next_tick = 60*HZ;
 	int ok = 0;
 	int media_status, old_window;
+	unsigned long flags;
 
 	if (vortex_debug > 2) {
 		pr_debug("%s: Media selection timer tick happened, %s.\n",
@@ -1771,7 +1772,7 @@ vortex_timer(unsigned long data)
 		pr_debug("dev->watchdog_timeo=%d\n", dev->watchdog_timeo);
 	}
 
-	disable_irq_lockdep(dev->irq);
+	spin_lock_irqsave(&vp->lock, flags);
 	old_window = ioread16(ioaddr + EL3_CMD) >> 13;
 	EL3WINDOW(4);
 	media_status = ioread16(ioaddr + Wn4_Media);
@@ -1851,7 +1852,7 @@ leave_media_alone:
 			 dev->name, media_tbl[dev->if_port].name);
 
 	EL3WINDOW(old_window);
-	enable_irq_lockdep(dev->irq);
+	spin_unlock_irqrestore(&vp->lock, flags);
 	mod_timer(&vp->timer, RUN_AT(next_tick));
 	if (vp->deferred)
 		iowrite16(FakeIntr, ioaddr + EL3_CMD);
-- 
1.6.3.3

Re: [PATCH 1/1] 3c59x: Acquire vortex lock instead of disabling irq

[David Miller]

From: Chase Douglas <chase.douglas@canonical.com>
Date: Wed, 17 Mar 2010 10:33:16 -0400

> Last year, threaded IRQ handlers were introduced to the mainline kernel.
> This change requires the disable_irq function to sleep if any IRQ
> handler threads for a given IRQ line are running.
> 
> Back in 2006, while working on the -rt patch set that had threaded IRQ
> handlers, the vortex_timer function was causing scheduling bugs because
> it is run in softirq context and called disable_irq. This patch was the
> best fix determined at the time, and still exists in the .33 -rt
> patchset. Now that threaded IRQ handlers are present in the mainline
> kernel we need to apply the patch there as well.
> 
> http://lkml.org/lkml/2006/5/12/178
> 
> BugLink: http://bugs.launchpad.net/bugs/533335
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>

This code is very much intentionally using disable_irq*().

The operation being performed here is extremely expensive,
and during that time if we have cpu interrupts disabled
serial devices will drop characters etc.

Re: [PATCH 1/1] 3c59x: Acquire vortex lock instead of disabling irq

[Chase Douglas]

On Wed, Mar 17, 2010 at 2:03 PM, David Miller <davem@davemloft.net> wrote:
> From: Chase Douglas <chase.douglas@canonical.com>
> Date: Wed, 17 Mar 2010 10:33:16 -0400
>
>> Last year, threaded IRQ handlers were introduced to the mainline kernel.
>> This change requires the disable_irq function to sleep if any IRQ
>> handler threads for a given IRQ line are running.
>>
>> Back in 2006, while working on the -rt patch set that had threaded IRQ
>> handlers, the vortex_timer function was causing scheduling bugs because
>> it is run in softirq context and called disable_irq. This patch was the
>> best fix determined at the time, and still exists in the .33 -rt
>> patchset. Now that threaded IRQ handlers are present in the mainline
>> kernel we need to apply the patch there as well.
>>
>> http://lkml.org/lkml/2006/5/12/178
>>
>> BugLink: http://bugs.launchpad.net/bugs/533335
>>
>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>
> This code is very much intentionally using disable_irq*().
>
> The operation being performed here is extremely expensive,
> and during that time if we have cpu interrupts disabled
> serial devices will drop characters etc.

If that's the case, what's the solution? It's not safe to call
disable_irq* in softirq context anymore. With the patch we have a
stable driver that may cause some serial devices to drop characters.
Without the patch we have an unstable driver that can lock up. To me
it seems the latter is preferable to the former, especially when the
lockup is occurs somewhat frequently.

-- Chase

Re: [PATCH 1/1] 3c59x: Acquire vortex lock instead of disabling irq

[David Miller]

From: Chase Douglas <chase.douglas@canonical.com>
Date: Wed, 17 Mar 2010 14:20:45 -0400

> If that's the case, what's the solution? It's not safe to call
> disable_irq* in softirq context anymore.

That's a huge problem, because such a restriction has broken
several drivers.  3c59x is not the only one which uses this
technique for this reason.  The 8390 one does too.

Re: [PATCH 1/1] 3c59x: Acquire vortex lock instead of disabling irq

[Chase Douglas]

On Wed, Mar 17, 2010 at 2:27 PM, David Miller <davem@davemloft.net> wrote:
> From: Chase Douglas <chase.douglas@canonical.com>
> Date: Wed, 17 Mar 2010 14:20:45 -0400
>
>> If that's the case, what's the solution? It's not safe to call
>> disable_irq* in softirq context anymore.
>
> That's a huge problem, because such a restriction has broken
> several drivers.  3c59x is not the only one which uses this
> technique for this reason.  The 8390 one does too.

So basically the answer right now is: it's broken and needs to be
reworked, and a switch to disabling irqs is deemed inadequate. Is that
accurate?

If that's a fair statement of the current drivers, then I can go back
and inform end users of this when they hit the bug.

-- Chase

Re: [PATCH 1/1] 3c59x: Acquire vortex lock instead of disabling irq

[David Miller]

From: Chase Douglas <chase.douglas@canonical.com>
Date: Wed, 17 Mar 2010 14:44:30 -0400

> On Wed, Mar 17, 2010 at 2:27 PM, David Miller <davem@davemloft.net> wrote:
>> From: Chase Douglas <chase.douglas@canonical.com>
>> Date: Wed, 17 Mar 2010 14:20:45 -0400
>>
>>> If that's the case, what's the solution? It's not safe to call
>>> disable_irq* in softirq context anymore.
>>
>> That's a huge problem, because such a restriction has broken
>> several drivers.  3c59x is not the only one which uses this
>> technique for this reason.  The 8390 one does too.
> 
> So basically the answer right now is: it's broken and needs to be
> reworked, and a switch to disabling irqs is deemed inadequate. Is that
> accurate?

Yep.  The bug is whatever caused disable_irq*() to stop working
where these drivers have been using it for what feels like
a century :-)