Hi,
Very below is my patch proposal with a comment, which in my opinion
is precious enough to save it for future help in reading and
understanding the code.
I hope Alan will not blame me I've not asked for his permission before
sending, and he would ack this patch as it is or at least most of this.
Thanks & regards,
Jarek P.
On Wed, Jul 25, 2007 at 03:46:56PM +0100, Alan Cox wrote:
> > > The code in question lib8390.c does
> > >
> > > disable_irq();
> > > fiddle_with_the_network_card_hardware()
> > > enable_irq();
> > ...
> > >
> > > No idea how this affects the network card, as the code there must be
> > > able to handle interrupts, which are not originated from the card due to
> > > interrupt sharing.
> >
> > I think, in this last yesterday's patch Ingo could be right, yet!
> > The comment at the beginnig points this is done like that because
> > of chip's slowness. And problems with timing are mysterious.
> >
> > On the other hand author of this code didn't use spin_lock_irqsave
> > for some reason, probably after testing this option too. So, I hope
> > this is the right path, but alas, I'm not sure this patch has to
> > prove this 100%.
>
> The author (me) didn't use spin_lock_irqsave because the slowness of the
> card means that approach caused horrible problems like losing serial data
> at 38400 baud on some chips. Rememeber many 8390 nics on PCI were ISA
> chips with FPGA front ends.
>
> > Anyway, in my opinion this situation where interrupts could/have_to
> > be used for such strange things should confirm the need of more
> > options for handling irqs individually.
>
> Ok the logic behind the 8390 is very simple:
>
> Things to know
> - IRQ delivery is asynchronous to the PCI bus
> - Blocking the local CPU IRQ via spin locks was too slow
> - The chip has register windows needing locking work
>
> So the path was once (I say once as people appear to have changed it
> in the mean time and it now looks rather bogus if the changes to use
> disable_irq_nosync_irqsave are disabling the local IRQ)
>
>
> Take the page lock
> Mask the IRQ on chip
> Disable the IRQ (but not mask locally- someone seems to have
> broken this with the lock validator stuff)
> [This must be _nosync as the page lock may otherwise
> deadlock us]
> Drop the page lock and turn IRQs back on
>
> At this point an existing IRQ may still be running but we can't
> get a new one
>
> Take the lock (so we know the IRQ has terminated) but don't mask
> the IRQs on the processor
> Set irqlock [for debug]
>
> Transmit (slow as ****)
>
> re-enable the IRQ
>
>
> We have to use disable_irq because otherwise you will get delayed
> interrupts on the APIC bus deadlocking the transmit path.
>
> Quite hairy but the chip simply wasn't designed for SMP and you can't
> even ACK an interrupt without risking corrupting other parallel
> activities on the chip.
>
> Alan
>
------>
From: Jarek Poplawski <[email protected]>
Subject: lib8390: comment on locking by Alan Cox
Additional explanation of problems with locking by Alan Cox.
Signed-off-by: Jarek Poplawski <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Paul Gortmaker <[email protected]>
Cc: Jeff Garzik <[email protected]>
---
diff -Nurp 2.6.23-rc1-/drivers/net/lib8390.c 2.6.23-rc1/drivers/net/lib8390.c
--- 2.6.23-rc1-/drivers/net/lib8390.c 2007-07-09 01:32:17.000000000 +0200
+++ 2.6.23-rc1/drivers/net/lib8390.c 2007-07-26 13:55:17.000000000 +0200
@@ -143,6 +143,52 @@ static void __NS8390_init(struct net_dev
* annoying the transmit function is called bh atomic. That places
* restrictions on the user context callers as disable_irq won't save
* them.
+ *
+ * Additional explanation of problems with locking by Alan Cox:
+ *
+ * "The author (me) didn't use spin_lock_irqsave because the slowness of the
+ * card means that approach caused horrible problems like losing serial data
+ * at 38400 baud on some chips. Rememeber many 8390 nics on PCI were ISA
+ * chips with FPGA front ends.
+ *
+ * Ok the logic behind the 8390 is very simple:
+ *
+ * Things to know
+ * - IRQ delivery is asynchronous to the PCI bus
+ * - Blocking the local CPU IRQ via spin locks was too slow
+ * - The chip has register windows needing locking work
+ *
+ * So the path was once (I say once as people appear to have changed it
+ * in the mean time and it now looks rather bogus if the changes to use
+ * disable_irq_nosync_irqsave are disabling the local IRQ)
+ *
+ *
+ * Take the page lock
+ * Mask the IRQ on chip
+ * Disable the IRQ (but not mask locally- someone seems to have
+ * broken this with the lock validator stuff)
+ * [This must be _nosync as the page lock may otherwise
+ * deadlock us]
+ * Drop the page lock and turn IRQs back on
+ *
+ * At this point an existing IRQ may still be running but we can't
+ * get a new one
+ *
+ * Take the lock (so we know the IRQ has terminated) but don't mask
+ * the IRQs on the processor
+ * Set irqlock [for debug]
+ *
+ * Transmit (slow as ****)
+ *
+ * re-enable the IRQ
+ *
+ *
+ * We have to use disable_irq because otherwise you will get delayed
+ * interrupts on the APIC bus deadlocking the transmit path.
+ *
+ * Quite hairy but the chip simply wasn't designed for SMP and you can't
+ * even ACK an interrupt without risking corrupting other parallel
+ * activities on the chip." [lkml, 25 Jul 2007]
*/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
