Manfred Spraul wrote:
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().
When interrupts are re-enabled (after the new configuration is set), the
nv_linkchange function will be called (due to link timer firing every
second) and make the appropriate netif_carrier_XX() call.
One optimization could to be just call nv_linkchange() directly instead
of calling nv_update_linkspeed().
--
Manfred
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may
contain
confidential information. Any unauthorized review, use, disclosure or
distribution
is prohibited. If you are not the intended recipient, please contact the
sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
-
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
