netdev
[Top] [All Lists]

Re: tx_timeout and timer serialisation

To: "netdev@xxxxxxxxxxx" <netdev@xxxxxxxxxxx>
Subject: Re: tx_timeout and timer serialisation
From: Andrew Morton <andrewm@xxxxxxxxxx>
Date: Tue, 02 May 2000 11:37:07 +1000
References: <390DA08D.8979B5B7@xxxxxxxxxx> <Pine.GSO.4.20.0005011126160.3573-100000@xxxxxxxxxxxxxxxx>
Sender: owner-netdev@xxxxxxxxxxx
jamal wrote:
> 
> Andrew,
> Hacking away those last minutes? ;->

Had a little problem with the corporate Amex.  24 hrs delay..

> Not Alexey, but i can give you some answers.

You're less entertaining :)

[ stuff ]

OK, thanks, guys.

1: hard_start_xmit and tx_timeout are serialised wrt each other.

2: timer functions, ioctl() and get_stats() have no guarantees.

3: The ISR can be called during get_stats(), start_xmit(),
   ioctl(), media_timer(), tx_timeout(), etc.

4: We have a lot of racy drivers :(

Let's pick some random samples:

eepro100
--------

speedo_timer does mdio_read()s.  speedo_tx_timeout() does mdio_read()s
and mdio_write()'s.  mdio functions are stateful.  Race.

3c515.c
------

WTF? cli()?  I stopped reading there.

epic100.c
---------

epic_timer() and mii_ioctl() do mdio_read()s

8139too.c
---------

Appears to get it right.  Way to go, Jeff!

3c59x.c
-------

vortex_timer() and vortex_ioctl() do mdio stuff.

sis900.c
--------

iotcl() and timer() do mdio functions.


acenic.c
--------

Well ace_timer is nice and safe, because the driver carefully constructs
the timer and then fails to add it to the kernel's timer table!



And that's just looking at the mdio functions.  I suspect that there is
more reentrable and stateful hardware bit twiddling going on here.


I suggest that we should be adding spinlocks to the ioctl(),
media_timer(), hard_start_xmit() and get_stats() fucntions ***As a
general rule*** and only remove them if the driver has been reviewed and
that non-raciness is demonstrable.


What fun.


Another issue: del_timer_sync().  It deletes a timer, but if that timer
happens to be running, del_timer_sync() blocks until the timer's handler
returns.  Things like the call to del_timer() in
acenic.c:ace_start_xmit() need to be carefully reviewed.


-- 
-akpm-

<Prev in Thread] Current Thread [Next in Thread>