Hello!
> 1: hard_start_xmit and tx_timeout are serialised wrt each other.
Add set_multicast_list() to this list.
> 2: timer functions, ioctl() and get_stats() have no guarantees.
Media timer is separate item, it is inivisible from outside at all.
ioctl() is allowed to sleep, hence top level cannot do anything
to serialize it wrt class 1. But it serializes it wrt itself
and open(), close(), which is not so easy by the way. 8)
get_stats() is very special thing. The problem with this is
that it is broken by design, returning pointer to some static structure
to code which knows nothing about its protection. It is pretty
clear that if get_stats() does not modify counters, it needs
no protection. But if it _does_ modify (== touch hardware registers),
as most of ethernet drivers do, top level cannot do anything with this
either in 2.2 or in 2.3. Protection wrt IRQs is out of area of its expertise.
I would advise to avoid touching statistics from get_stats, when it
is possible and to modify counters only in context, where they
are naturally serialized. F.e. tulip get_stats() changes only
rx_missed_errors. Does it really deserve mud with irq safe locks around?
> 3: The ISR can be called during get_stats(), start_xmit(),
> ioctl(), media_timer(), tx_timeout(), etc.
Well, nothing is protected wrt irqs.
> speedo_timer does mdio_read()s. speedo_tx_timeout() does mdio_read()s
> and mdio_write()'s. mdio functions are stateful. Race.
Are they touched in normal rx/tx path and/or irq? If they are not,
it is easy to repair with _separate_ mdio bh protected spinlock.
The problem can be with control registers, which are reprogrammed
at IRQ level.
> 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.
Do some drivers really use this? Beware, this function is smart,
but it is not so easy to use. First, timer handler must do timer_exit()
on exit. Second, del_timer_sync() cannot be called under spinlock, which
could be grabbed inside timer handler, it will be deadlock.
The best thing is to call it only in context _free_ of spinlocks:
i.e. open(), close(), ioctl(). See? That's why open()/close()/ioctl()
are called by top level without any locks held, though there are
not so much of drivers really sleeping there. Lots of operations
like to be done in normal process context. [ Including cli(), which
does not synchronize to timers, when called from under spinlock. ]
Alexey
|