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