lkml.org 
[lkml]   [2007]   [Jul]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[PATCH][netdrvr] lib8390: comment on locking by Alan Cox Re: 2.6.20->2.6.21 - networking dies after random time
    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 <jarkao2@o2.pl>

    Subject: lib8390: comment on locking by Alan Cox

    Additional explanation of problems with locking by Alan Cox.

    Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
    Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
    Cc: Paul Gortmaker <p_gortmaker@yahoo.com>
    Cc: Jeff Garzik <jeff@garzik.org>

    ---

    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-07-26 14:37    [W:4.208 / U:0.728 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site