diff mbox

8139too: send NETDEV_CHANGE manually when autoneg is disabled

Message ID 1362596784-19443-1-git-send-email-vfalico@redhat.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Veaceslav Falico March 6, 2013, 7:06 p.m. UTC
When setting autoneg off (with any additional parameters, like
speed/duplex), 8139too doesn't do an interface reset, and thus doesn't
notify anyone that its speed/duplex might have changed (bonding and bridge
will not see the speed changes, per example).

Verify if we've force_media and send notification manually, so that the
listeners have a chance to see the changes. It's quite ugly, however I
don't see anything better.

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/ethernet/realtek/8139too.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

Comments

Ben Hutchings March 6, 2013, 8:22 p.m. UTC | #1
On Wed, 2013-03-06 at 20:06 +0100, Veaceslav Falico wrote:
> When setting autoneg off (with any additional parameters, like
> speed/duplex), 8139too doesn't do an interface reset, and thus doesn't
> notify anyone that its speed/duplex might have changed (bonding and bridge
> will not see the speed changes, per example).
>
> Verify if we've force_media and send notification manually, so that the
> listeners have a chance to see the changes. It's quite ugly, however I
> don't see anything better.

Isn't this really a bug in mii_check_media()?  It shouldn't shortcut the
calls to netif_carrier_{off,on}() just because mii->force_media is set.

Ben.

> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>  drivers/net/ethernet/realtek/8139too.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c
> index 1276ac7..96ee18c 100644
> --- a/drivers/net/ethernet/realtek/8139too.c
> +++ b/drivers/net/ethernet/realtek/8139too.c
> @@ -2393,6 +2393,11 @@ static int rtl8139_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
>  	spin_lock_irq(&tp->lock);
>  	rc = mii_ethtool_sset(&tp->mii, cmd);
>  	spin_unlock_irq(&tp->lock);
> +	/*
> +	 * we don't restart on autoneg off, so notify manually
> +	 */
> +	if (tp->mii.force_media)
> +		call_netdevice_notifiers(NETDEV_CHANGE, dev);
>  	return rc;
>  }
>
David Miller March 6, 2013, 8:53 p.m. UTC | #2
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 6 Mar 2013 20:22:52 +0000

> On Wed, 2013-03-06 at 20:06 +0100, Veaceslav Falico wrote:
>> When setting autoneg off (with any additional parameters, like
>> speed/duplex), 8139too doesn't do an interface reset, and thus doesn't
>> notify anyone that its speed/duplex might have changed (bonding and bridge
>> will not see the speed changes, per example).
>>
>> Verify if we've force_media and send notification manually, so that the
>> listeners have a chance to see the changes. It's quite ugly, however I
>> don't see anything better.
> 
> Isn't this really a bug in mii_check_media()?  It shouldn't shortcut the
> calls to netif_carrier_{off,on}() just because mii->force_media is set.

I think mii_check_media() is responsible for handling this too.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Francois Romieu March 6, 2013, 9:35 p.m. UTC | #3
David Miller <davem@davemloft.net> :
> From: Ben Hutchings <bhutchings@solarflare.com>
[...]
> > Isn't this really a bug in mii_check_media()?  It shouldn't shortcut the
> > calls to netif_carrier_{off,on}() just because mii->force_media is set.
> 
> I think mii_check_media() is responsible for handling this too.

There are a few drivers that use mii_ethtool_sset as 8139too does
while not relying upon mii_check_media.

What ought to be done for those ?
Veaceslav Falico March 7, 2013, 10:27 a.m. UTC | #4
On Wed, Mar 06, 2013 at 03:53:23PM -0500, David Miller wrote:
>From: Ben Hutchings <bhutchings@solarflare.com>
>Date: Wed, 6 Mar 2013 20:22:52 +0000
>
>> On Wed, 2013-03-06 at 20:06 +0100, Veaceslav Falico wrote:
>>> When setting autoneg off (with any additional parameters, like
>>> speed/duplex), 8139too doesn't do an interface reset, and thus doesn't
>>> notify anyone that its speed/duplex might have changed (bonding and bridge
>>> will not see the speed changes, per example).
>>>
>>> Verify if we've force_media and send notification manually, so that the
>>> listeners have a chance to see the changes. It's quite ugly, however I
>>> don't see anything better.
>>
>> Isn't this really a bug in mii_check_media()?  It shouldn't shortcut the
>> calls to netif_carrier_{off,on}() just because mii->force_media is set.
>
>I think mii_check_media() is responsible for handling this too.

The mii_check_media() doesn't get called, AFAIK. The problem here is that,
after we call ethtool -s eth0 autoneg off speed X, with eth0 being
8139too, the speed/autoneg options are changed via mii_ethtool_sset(),
however the interface itself isn't down'ed/up'ed, and thus no NETDEV_
notifications are sent.

Other drivers either explicitly reset the interface after
ethtool.set_settings() call (like netxen_nic using ndo_close()/ndo_open()),
do it on the logic level (tg3) without involving mii_ethtool_sset(), or
just reset on their own (e100 iirc), so that most of them are responsible
for somehow triggering these events.

Silently changing speed can break things a bit - bonding relies on
interface speeds for 802.3ad/alb/tlb/active-backup iirc, bridge relies on
stp port cost etc. and they all get it via NETDEV_ notifications. So
without them, they would end up with outdated data, per example (eth2 being
8139too):

darkmag:~#grep 'Interface\|Speed' /proc/net/bonding/bond0
Slave Interface: eth0
Speed: 100 Mbps
Slave Interface: eth2
Speed: 100 Mbps
darkmag:~#ethtool -s eth2 autoneg off speed 10
darkmag:~#cat /sys/class/net/eth2/speed
10
darkmag:~#grep 'Interface\|Speed' /proc/net/bonding/bond0
Slave Interface: eth0
Speed: 100 Mbps
Slave Interface: eth2
Speed: 100 Mbps

However, I think that mii_check_media() is also wrong :), though I didn't
really dig into it. I'll check it when I'll have time.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings March 7, 2013, 3:52 p.m. UTC | #5
On Thu, 2013-03-07 at 11:27 +0100, Veaceslav Falico wrote:
> On Wed, Mar 06, 2013 at 03:53:23PM -0500, David Miller wrote:
> >From: Ben Hutchings <bhutchings@solarflare.com>
> >Date: Wed, 6 Mar 2013 20:22:52 +0000
> >
> >> On Wed, 2013-03-06 at 20:06 +0100, Veaceslav Falico wrote:
> >>> When setting autoneg off (with any additional parameters, like
> >>> speed/duplex), 8139too doesn't do an interface reset, and thus doesn't
> >>> notify anyone that its speed/duplex might have changed (bonding and bridge
> >>> will not see the speed changes, per example).
> >>>
> >>> Verify if we've force_media and send notification manually, so that the
> >>> listeners have a chance to see the changes. It's quite ugly, however I
> >>> don't see anything better.
> >>
> >> Isn't this really a bug in mii_check_media()?  It shouldn't shortcut the
> >> calls to netif_carrier_{off,on}() just because mii->force_media is set.
> >
> >I think mii_check_media() is responsible for handling this too.
> 
> The mii_check_media() doesn't get called, AFAIK. The problem here is that,
> after we call ethtool -s eth0 autoneg off speed X, with eth0 being
> 8139too, the speed/autoneg options are changed via mii_ethtool_sset(),
> however the interface itself isn't down'ed/up'ed, and thus no NETDEV_
> notifications are sent.

Does the hardware not send link interrupts if autoneg is disabled?

> Other drivers either explicitly reset the interface after
> ethtool.set_settings() call (like netxen_nic using ndo_close()/ndo_open()),
> do it on the logic level (tg3) without involving mii_ethtool_sset(), or
> just reset on their own (e100 iirc), so that most of them are responsible
> for somehow triggering these events.
> 
> Silently changing speed can break things a bit - bonding relies on
> interface speeds for 802.3ad/alb/tlb/active-backup iirc, bridge relies on
> stp port cost etc. and they all get it via NETDEV_ notifications. So
> without them, they would end up with outdated data, per example (eth2 being
> 8139too):
[...]

Yes, I get it.  But on real hardware, changing speed/duplex is always
going to break the link if it's up.  The link change notification should
result in kernel and userland notifications via linkwatch_do_dev().

(If you're testing against QEMU rather than real hardware, there could
be a bug in the emulation of link interrupts.)

Ben.
Veaceslav Falico March 7, 2013, 4:35 p.m. UTC | #6
On Thu, Mar 07, 2013 at 03:52:35PM +0000, Ben Hutchings wrote:
>On Thu, 2013-03-07 at 11:27 +0100, Veaceslav Falico wrote:
>> On Wed, Mar 06, 2013 at 03:53:23PM -0500, David Miller wrote:
>> >From: Ben Hutchings <bhutchings@solarflare.com>
>> >Date: Wed, 6 Mar 2013 20:22:52 +0000
>> >
>> >> On Wed, 2013-03-06 at 20:06 +0100, Veaceslav Falico wrote:
>> >>> When setting autoneg off (with any additional parameters, like
>> >>> speed/duplex), 8139too doesn't do an interface reset, and thus doesn't
>> >>> notify anyone that its speed/duplex might have changed (bonding and bridge
>> >>> will not see the speed changes, per example).
>> >>>
>> >>> Verify if we've force_media and send notification manually, so that the
>> >>> listeners have a chance to see the changes. It's quite ugly, however I
>> >>> don't see anything better.
>> >>
>> >> Isn't this really a bug in mii_check_media()?  It shouldn't shortcut the
>> >> calls to netif_carrier_{off,on}() just because mii->force_media is set.
>> >
>> >I think mii_check_media() is responsible for handling this too.
>>
>> The mii_check_media() doesn't get called, AFAIK. The problem here is that,
>> after we call ethtool -s eth0 autoneg off speed X, with eth0 being
>> 8139too, the speed/autoneg options are changed via mii_ethtool_sset(),
>> however the interface itself isn't down'ed/up'ed, and thus no NETDEV_
>> notifications are sent.
>
>Does the hardware not send link interrupts if autoneg is disabled?

Yes, it doesn't send them when the autoneg off is set.

>
>> Other drivers either explicitly reset the interface after
>> ethtool.set_settings() call (like netxen_nic using ndo_close()/ndo_open()),
>> do it on the logic level (tg3) without involving mii_ethtool_sset(), or
>> just reset on their own (e100 iirc), so that most of them are responsible
>> for somehow triggering these events.
>>
>> Silently changing speed can break things a bit - bonding relies on
>> interface speeds for 802.3ad/alb/tlb/active-backup iirc, bridge relies on
>> stp port cost etc. and they all get it via NETDEV_ notifications. So
>> without them, they would end up with outdated data, per example (eth2 being
>> 8139too):
>[...]
>
>Yes, I get it.  But on real hardware, changing speed/duplex is always
>going to break the link if it's up.  The link change notification should
>result in kernel and userland notifications via linkwatch_do_dev().
>
>(If you're testing against QEMU rather than real hardware, there could
>be a bug in the emulation of link interrupts.)

It's real hardware:

[    4.339413] 8139cp 0000:10:09.0: This (id 10ec:8139 rev 10) is not an 8139C+ compatible chip, use 8139too
[    4.348210] 8139too: 8139too Fast Ethernet driver 0.9.28
[    4.350017] 8139too 0000:10:09.0 eth2: RealTek RTL8139 at 0xffffc90004574100, 00:14:d1:1f:b9:49, IRQ 21

darkmag:~#ethtool eth2
Settings for eth2:
<snip>
         Supports auto-negotiation: Yes
         Advertised link modes:  10baseT/Half 10baseT/Full 
                                 100baseT/Half 100baseT/Full 
         Advertised pause frame use: No
         Advertised auto-negotiation: Yes
         Link partner advertised link modes:  10baseT/Half 10baseT/Full 
                                              100baseT/Half 100baseT/Full 
         Link partner advertised pause frame use: Symmetric
         Link partner advertised auto-negotiation: Yes
         Speed: 100Mb/s
         Duplex: Full
<snip>
darkmag:~#dmesg | tail -n 10 | grep 8139too
[  164.019622] 8139too 0000:10:09.0 eth2: link up, 100Mbps, full-duplex, lpa 0x45E1
darkmag:~#ethtool -s eth2 autoneg off speed 10
darkmag:~#dmesg | tail -n 10 | grep 8139too
[  164.019622] 8139too 0000:10:09.0 eth2: link up, 100Mbps, full-duplex, lpa 0x45E1
darkmag:~#ethtool eth2
Settings for eth2:
<snip>
         Supports auto-negotiation: Yes
         Advertised link modes:  Not reported
         Advertised pause frame use: No
         Advertised auto-negotiation: No
         Speed: 10Mb/s
         Duplex: Full

And, obviously, bonding's not updated:

darkmag:~#grep 'Interface\|Speed' /proc/net/bonding/bond0 
Slave Interface: eth0
Speed: 100 Mbps
Slave Interface: eth2
Speed: 100 Mbps

>
>Ben.
>
>-- 
>Ben Hutchings, Staff Engineer, Solarflare
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings March 7, 2013, 4:54 p.m. UTC | #7
On Thu, 2013-03-07 at 17:35 +0100, Veaceslav Falico wrote:
> On Thu, Mar 07, 2013 at 03:52:35PM +0000, Ben Hutchings wrote:
> >On Thu, 2013-03-07 at 11:27 +0100, Veaceslav Falico wrote:
> >> On Wed, Mar 06, 2013 at 03:53:23PM -0500, David Miller wrote:
> >> >From: Ben Hutchings <bhutchings@solarflare.com>
> >> >Date: Wed, 6 Mar 2013 20:22:52 +0000
> >> >
> >> >> On Wed, 2013-03-06 at 20:06 +0100, Veaceslav Falico wrote:
> >> >>> When setting autoneg off (with any additional parameters, like
> >> >>> speed/duplex), 8139too doesn't do an interface reset, and thus doesn't
> >> >>> notify anyone that its speed/duplex might have changed (bonding and bridge
> >> >>> will not see the speed changes, per example).
> >> >>>
> >> >>> Verify if we've force_media and send notification manually, so that the
> >> >>> listeners have a chance to see the changes. It's quite ugly, however I
> >> >>> don't see anything better.
> >> >>
> >> >> Isn't this really a bug in mii_check_media()?  It shouldn't shortcut the
> >> >> calls to netif_carrier_{off,on}() just because mii->force_media is set.
> >> >
> >> >I think mii_check_media() is responsible for handling this too.
> >>
> >> The mii_check_media() doesn't get called, AFAIK. The problem here is that,
> >> after we call ethtool -s eth0 autoneg off speed X, with eth0 being
> >> 8139too, the speed/autoneg options are changed via mii_ethtool_sset(),
> >> however the interface itself isn't down'ed/up'ed, and thus no NETDEV_
> >> notifications are sent.
> >
> >Does the hardware not send link interrupts if autoneg is disabled?
> 
> Yes, it doesn't send them when the autoneg off is set.
> 
> >
> >> Other drivers either explicitly reset the interface after
> >> ethtool.set_settings() call (like netxen_nic using ndo_close()/ndo_open()),
> >> do it on the logic level (tg3) without involving mii_ethtool_sset(), or
> >> just reset on their own (e100 iirc), so that most of them are responsible
> >> for somehow triggering these events.
> >>
> >> Silently changing speed can break things a bit - bonding relies on
> >> interface speeds for 802.3ad/alb/tlb/active-backup iirc, bridge relies on
> >> stp port cost etc. and they all get it via NETDEV_ notifications. So
> >> without them, they would end up with outdated data, per example (eth2 being
> >> 8139too):
> >[...]
> >
> >Yes, I get it.  But on real hardware, changing speed/duplex is always
> >going to break the link if it's up.  The link change notification should
> >result in kernel and userland notifications via linkwatch_do_dev().
> >
> >(If you're testing against QEMU rather than real hardware, there could
> >be a bug in the emulation of link interrupts.)
> 
> It's real hardware:
> 
> [    4.339413] 8139cp 0000:10:09.0: This (id 10ec:8139 rev 10) is not an 8139C+ compatible chip, use 8139too
> [    4.348210] 8139too: 8139too Fast Ethernet driver 0.9.28
> [    4.350017] 8139too 0000:10:09.0 eth2: RealTek RTL8139 at 0xffffc90004574100, 00:14:d1:1f:b9:49, IRQ 21
[...]

OK.  But it's generally not enough to send a 'something changed'
notification; the driver actually has to update the link state.  MAybe
in your test you reconfigure the other end of the link too, or it's
smart enough to do parallel detect.  But in general, changing the link
speed is going to change the link state, and the TX scheduler needs to
enabled or disabled accordingly.

Perhaps this driver needs to run a link polling timer while autoneg is
off.

Ben.
Veaceslav Falico March 7, 2013, 6:38 p.m. UTC | #8
On Thu, Mar 07, 2013 at 04:54:21PM +0000, Ben Hutchings wrote:
>On Thu, 2013-03-07 at 17:35 +0100, Veaceslav Falico wrote:
>> On Thu, Mar 07, 2013 at 03:52:35PM +0000, Ben Hutchings wrote:
>> >On Thu, 2013-03-07 at 11:27 +0100, Veaceslav Falico wrote:
>> >> On Wed, Mar 06, 2013 at 03:53:23PM -0500, David Miller wrote:
>> >> >From: Ben Hutchings <bhutchings@solarflare.com>
>> >> >Date: Wed, 6 Mar 2013 20:22:52 +0000
>> >> >
>> >> >> On Wed, 2013-03-06 at 20:06 +0100, Veaceslav Falico wrote:
>> >> >>> When setting autoneg off (with any additional parameters, like
>> >> >>> speed/duplex), 8139too doesn't do an interface reset, and thus doesn't
>> >> >>> notify anyone that its speed/duplex might have changed (bonding and bridge
>> >> >>> will not see the speed changes, per example).
>> >> >>>
>> >> >>> Verify if we've force_media and send notification manually, so that the
>> >> >>> listeners have a chance to see the changes. It's quite ugly, however I
>> >> >>> don't see anything better.
>> >> >>
>> >> >> Isn't this really a bug in mii_check_media()?  It shouldn't shortcut the
>> >> >> calls to netif_carrier_{off,on}() just because mii->force_media is set.
>> >> >
>> >> >I think mii_check_media() is responsible for handling this too.
>> >>
>> >> The mii_check_media() doesn't get called, AFAIK. The problem here is that,
>> >> after we call ethtool -s eth0 autoneg off speed X, with eth0 being
>> >> 8139too, the speed/autoneg options are changed via mii_ethtool_sset(),
>> >> however the interface itself isn't down'ed/up'ed, and thus no NETDEV_
>> >> notifications are sent.
>> >
>> >Does the hardware not send link interrupts if autoneg is disabled?
>>
>> Yes, it doesn't send them when the autoneg off is set.
>>
>> >
>> >> Other drivers either explicitly reset the interface after
>> >> ethtool.set_settings() call (like netxen_nic using ndo_close()/ndo_open()),
>> >> do it on the logic level (tg3) without involving mii_ethtool_sset(), or
>> >> just reset on their own (e100 iirc), so that most of them are responsible
>> >> for somehow triggering these events.
>> >>
>> >> Silently changing speed can break things a bit - bonding relies on
>> >> interface speeds for 802.3ad/alb/tlb/active-backup iirc, bridge relies on
>> >> stp port cost etc. and they all get it via NETDEV_ notifications. So
>> >> without them, they would end up with outdated data, per example (eth2 being
>> >> 8139too):
>> >[...]
>> >
>> >Yes, I get it.  But on real hardware, changing speed/duplex is always
>> >going to break the link if it's up.  The link change notification should
>> >result in kernel and userland notifications via linkwatch_do_dev().
>> >
>> >(If you're testing against QEMU rather than real hardware, there could
>> >be a bug in the emulation of link interrupts.)
>>
>> It's real hardware:
>>
>> [    4.339413] 8139cp 0000:10:09.0: This (id 10ec:8139 rev 10) is not an 8139C+ compatible chip, use 8139too
>> [    4.348210] 8139too: 8139too Fast Ethernet driver 0.9.28
>> [    4.350017] 8139too 0000:10:09.0 eth2: RealTek RTL8139 at 0xffffc90004574100, 00:14:d1:1f:b9:49, IRQ 21
>[...]
>
>OK.  But it's generally not enough to send a 'something changed'
>notification; the driver actually has to update the link state.  MAybe
>in your test you reconfigure the other end of the link too, or it's
>smart enough to do parallel detect.  But in general, changing the link
>speed is going to change the link state, and the TX scheduler needs to
>enabled or disabled accordingly.

Sorry, I think I didn't explain it clearly. The driver *does* notify
about link changes even when it has autonegotiation disabled.

The only thing that doesn't work is the notification when someone changes
it via ethtool. In the same moment.

i.e. when the other peer ifdowns the port - the driver does see it and
notifies about it.

The only scenario that doesn't work is when 8139too is manually (locally)
changed, by ethtool, to autoneg off. Then it doesn't notify anything. So,
effectively, if the administrator issues an 'ethtool -s eth0 autoneg off
speed X', bonding/bridge/etc. won't be notified about that. However, if the
cable is pulled afterwards - it will notify about link down.

Basically, when changing the interface manually to autoneg off and whatever
else, the driver doesn't issue the NETDEV_* event. That's what I've added.

Other drivers, after setting autoneg off, usually ifdown/ifup the interface
manually, and thus everything gets updated. That is another way to fix this
driver - to issue ->ndo_stop(); ->ndo_open(); (as netxen_nic/qlcnic/etc
do), but I think that just issuing a notification that we've changed the
state, without flapping, is better.

>
>Perhaps this driver needs to run a link polling timer while autoneg is
>off.
>
>Ben.
>
>-- 
>Ben Hutchings, Staff Engineer, Solarflare
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings March 7, 2013, 7:10 p.m. UTC | #9
On Thu, 2013-03-07 at 19:38 +0100, Veaceslav Falico wrote:
> On Thu, Mar 07, 2013 at 04:54:21PM +0000, Ben Hutchings wrote:
> >On Thu, 2013-03-07 at 17:35 +0100, Veaceslav Falico wrote:
> >> On Thu, Mar 07, 2013 at 03:52:35PM +0000, Ben Hutchings wrote:
> >> >On Thu, 2013-03-07 at 11:27 +0100, Veaceslav Falico wrote:
[...]
> >> >> Silently changing speed can break things a bit - bonding relies on
> >> >> interface speeds for 802.3ad/alb/tlb/active-backup iirc, bridge relies on
> >> >> stp port cost etc. and they all get it via NETDEV_ notifications. So
> >> >> without them, they would end up with outdated data, per example (eth2 being
> >> >> 8139too):
> >> >[...]
> >> >
> >> >Yes, I get it.  But on real hardware, changing speed/duplex is always
> >> >going to break the link if it's up.  The link change notification should
> >> >result in kernel and userland notifications via linkwatch_do_dev().
> >> >
> >> >(If you're testing against QEMU rather than real hardware, there could
> >> >be a bug in the emulation of link interrupts.)
> >>
> >> It's real hardware:
> >>
> >> [    4.339413] 8139cp 0000:10:09.0: This (id 10ec:8139 rev 10) is not an 8139C+ compatible chip, use 8139too
> >> [    4.348210] 8139too: 8139too Fast Ethernet driver 0.9.28
> >> [    4.350017] 8139too 0000:10:09.0 eth2: RealTek RTL8139 at 0xffffc90004574100, 00:14:d1:1f:b9:49, IRQ 21
> >[...]
> >
> >OK.  But it's generally not enough to send a 'something changed'
> >notification; the driver actually has to update the link state.  MAybe
> >in your test you reconfigure the other end of the link too, or it's
> >smart enough to do parallel detect.  But in general, changing the link
> >speed is going to change the link state, and the TX scheduler needs to
> >enabled or disabled accordingly.
> 
> Sorry, I think I didn't explain it clearly. The driver *does* notify
> about link changes even when it has autonegotiation disabled.
> 
> The only thing that doesn't work is the notification when someone changes
> it via ethtool. In the same moment.

The link *should* go down (assuming it was up before) because you likely
have a speed mismatch and won't be able to pass traffic.  I don't know
whether this is possible for the hardware to detect immediately; maybe a
PHY reset will ensure that it is detected.

[...]
> Other drivers, after setting autoneg off, usually ifdown/ifup the interface
> manually, and thus everything gets updated. That is another way to fix this
> driver - to issue ->ndo_stop(); ->ndo_open(); (as netxen_nic/qlcnic/etc
> do), but I think that just issuing a notification that we've changed the
> state, without flapping, is better.

The link *should* flap, though doing a full stop/start is likely
unnecessary.

Ben.
diff mbox

Patch

diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c
index 1276ac7..96ee18c 100644
--- a/drivers/net/ethernet/realtek/8139too.c
+++ b/drivers/net/ethernet/realtek/8139too.c
@@ -2393,6 +2393,11 @@  static int rtl8139_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 	spin_lock_irq(&tp->lock);
 	rc = mii_ethtool_sset(&tp->mii, cmd);
 	spin_unlock_irq(&tp->lock);
+	/*
+	 * we don't restart on autoneg off, so notify manually
+	 */
+	if (tp->mii.force_media)
+		call_netdevice_notifiers(NETDEV_CHANGE, dev);
 	return rc;
 }