Re: [PATCH][netdrvr] lib8390: comment on locking by Alan Cox Re: 2.6.20->2.6.21 - networking dies after random time

[Jeff Garzik <jeff@garzik.org>]

Jarek Poplawski wrote:
> 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>

applied


-
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/