Ayaz Abdulla wrote:
Ayaz Abdulla wrote:
Manfred Spraul wrote:
Ayaz Abdulla wrote:
This patch adds support for configuration of various parameters. This
includes module parameters and ethtool commands.
+
+ if (netif_running(dev)) {
+ nv_start_rx(dev);
+ nv_start_tx(dev);
+ nv_enable_irq(dev);
+ }
+
Why no spin_lock() calls? Otherwise start_xmit could be running
concurrently, or the irq handler could run if the interrupt is shared.
You are correct about irq handler for shared interrupt. I call
netif_carrier_off() so the xmit routine should not be called.
Actually, I take that back. Irq is disabled, so this is ok.
I was afraid of two races:
a) a concurrent ifdown - someone closes a network device during an
ethtool -s command. dev_close() clears the __LINK_STATE_START bit. If
that happens, then the first call of netif_runnig() would return true
and the second false, which would leak the locks.
I've checked the locking: both dev_close() and dev_ethtool run under
rtnl_lock(), which is a mutex. Thus the race doesn't exist.
b) I prefer simple locking rules - e.g. all calls to nv_start_tx() under
spinlock. You broke that "rule". But it's ok, it's only a question of
personal preference.
Thus: I take back my objection.
But I have a new one: where is the netif_carrier_on()? The test case
would be a change of the pause settings with autonegotiation off.
nv_set_pauseparam() calls netif_carrier_off() - and I don't see the
corresponding netif_carrier_on().
--
Manfred
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
