[PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Veaceslav Falico]

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(-)

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;
 }
 
-- 
1.7.1

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Ben Hutchings]

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;
>  }
>  

-- 
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.

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[David Miller]

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.

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Francois Romieu]

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 ?

-- 
Ueimor

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Veaceslav Falico]

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.

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Ben Hutchings]

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.

-- 
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.

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Veaceslav Falico]

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.
>

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Ben Hutchings]

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.

-- 
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.

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Veaceslav Falico]

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.
>

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Ben Hutchings]

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.

-- 
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.

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Veaceslav Falico]

On Thu, Mar 07, 2013 at 07:10:33PM +0000, Ben Hutchings wrote:
>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.

Sorry for late response - I've had a feeling that I was writing something
utterly dumb. My feeling was correct. I've looked through it and came up
with the next patch, though I have a feeling... :)

Anyway, you were right - the hardware does everything needed, and the
driver also. The normal way (with autoneg on) would be for the driver to
call mii_check_media() when it got the interrupt, which would verify if
state/duplex has changed and report it.

However, with autoneg off, and thus mii->force_duplex == 1,
mii_check_media() just returns 'nothing changed' back, without doing
anything else. This way, any media change while in autoneg off (like
up/down notifications) are completely hidden to the rest of the kernel.

I've modified the mii_check_media() to not verify for ->force_media and to
get all the media settings from mii_ethtool_gset(), which would take care
of whether autoneg is on or off and return the correct values. Tested,
works ok - it sees when the cable is dis/connected, notifies bonding of
speed/duplex changes, when enslaved.

Subject: [PATCH] mii: make mii_check_media() work with ->force_media == 1

mii_check_media() does nothing when ->force_media is set, and thus might
miss media change events reported by the drivers if the autonegotiation is
off.

Make mii_check_media() use mii_ethtool_gset() for getting media settings,
which cleans the code a lot and works with both autoneg off and on. This
allows us to catch link notifications even ->force_media on.

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
  drivers/net/mii.c |   45 ++++++++++++---------------------------------
  1 files changed, 12 insertions(+), 33 deletions(-)

diff --git a/drivers/net/mii.c b/drivers/net/mii.c
index 4a99c39..2447487 100644
--- a/drivers/net/mii.c
+++ b/drivers/net/mii.c
@@ -308,19 +308,14 @@ void mii_check_link (struct mii_if_info *mii)
   * @init_media: OK to save duplex mode in @mii
   *
   * Returns 1 if the duplex mode changed, 0 if not.
- * If the media type is forced, always returns 0.
   */
  unsigned int mii_check_media (struct mii_if_info *mii,
  			      unsigned int ok_to_print,
  			      unsigned int init_media)
  {
  	unsigned int old_carrier, new_carrier;
-	int advertise, lpa, media, duplex;
-	int lpa2 = 0;
-
-	/* if forced media, go no further */
-	if (mii->force_media)
-		return 0; /* duplex did not change */
+	int duplex;
+	struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET };
  
  	/* check current and old link status */
  	old_carrier = netif_carrier_ok(mii->dev) ? 1 : 0;
@@ -345,37 +340,21 @@ unsigned int mii_check_media (struct mii_if_info *mii,
  	 */
  	netif_carrier_on(mii->dev);
  
-	/* get MII advertise and LPA values */
-	if ((!init_media) && (mii->advertising))
-		advertise = mii->advertising;
-	else {
-		advertise = mii->mdio_read(mii->dev, mii->phy_id, MII_ADVERTISE);
-		mii->advertising = advertise;
-	}
-	lpa = mii->mdio_read(mii->dev, mii->phy_id, MII_LPA);
-	if (mii->supports_gmii)
-		lpa2 = mii->mdio_read(mii->dev, mii->phy_id, MII_STAT1000);
+	/*
+	 * save the previous state of the duplex, mii_ethtool_gset()
+	 * modifies it
+	 */
+	duplex = mii->full_duplex;
  
-	/* figure out media and duplex from advertise and LPA values */
-	media = mii_nway_result(lpa & advertise);
-	duplex = (media & ADVERTISE_FULL) ? 1 : 0;
-	if (lpa2 & LPA_1000FULL)
-		duplex = 1;
+	mii_ethtool_gset(mii, &ecmd);
  
  	if (ok_to_print)
  		netdev_info(mii->dev, "link up, %uMbps, %s-duplex, lpa 0x%04X\n",
-			    lpa2 & (LPA_1000FULL | LPA_1000HALF) ? 1000 :
-			    media & (ADVERTISE_100FULL | ADVERTISE_100HALF) ?
-			    100 : 10,
-			    duplex ? "full" : "half",
-			    lpa);
-
-	if ((init_media) || (mii->full_duplex != duplex)) {
-		mii->full_duplex = duplex;
-		return 1; /* duplex changed */
-	}
+			    ethtool_cmd_speed(&ecmd), 
+			    ecmd.duplex == DUPLEX_FULL ? "full" : "half",
+			    ecmd.lp_advertising);
  
-	return 0; /* duplex did not change */
+	return (init_media || (mii->full_duplex != duplex)) ? 1 : 0;
  }
  
  /**
-- 
1.7.1


>
>[...]
>> 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.
>
>-- 
>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.
>

Re: [PATCH] 8139too: send NETDEV_CHANGE manually when autoneg is disabled

[Ben Hutchings]

On Mon, 2013-03-11 at 18:58 +0100, Veaceslav Falico wrote:
[...]
> Anyway, you were right - the hardware does everything needed, and the
> driver also. The normal way (with autoneg on) would be for the driver to
> call mii_check_media() when it got the interrupt, which would verify if
> state/duplex has changed and report it.
> 
> However, with autoneg off, and thus mii->force_duplex == 1,
> mii_check_media() just returns 'nothing changed' back, without doing
> anything else. This way, any media change while in autoneg off (like
> up/down notifications) are completely hidden to the rest of the kernel.
> 
> I've modified the mii_check_media() to not verify for ->force_media and to
> get all the media settings from mii_ethtool_gset(), which would take care
> of whether autoneg is on or off and return the correct values. Tested,
> works ok - it sees when the cable is dis/connected, notifies bonding of
> speed/duplex changes, when enslaved.
> 
> Subject: [PATCH] mii: make mii_check_media() work with ->force_media == 1
> 
> mii_check_media() does nothing when ->force_media is set, and thus might
> miss media change events reported by the drivers if the autonegotiation is
> off.
> 
> Make mii_check_media() use mii_ethtool_gset() for getting media settings,
> which cleans the code a lot and works with both autoneg off and on. This
> allows us to catch link notifications even ->force_media on.
> 
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>   drivers/net/mii.c |   45 ++++++++++++---------------------------------
>   1 files changed, 12 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/mii.c b/drivers/net/mii.c
> index 4a99c39..2447487 100644
> --- a/drivers/net/mii.c
> +++ b/drivers/net/mii.c
> @@ -308,19 +308,14 @@ void mii_check_link (struct mii_if_info *mii)
>    * @init_media: OK to save duplex mode in @mii
>    *
>    * Returns 1 if the duplex mode changed, 0 if not.
> - * If the media type is forced, always returns 0.
>    */

Perhaps you could also correct the summary line and the description of
@init_media while you're doing this.

>   unsigned int mii_check_media (struct mii_if_info *mii,
>   			      unsigned int ok_to_print,
>   			      unsigned int init_media)
>   {
>   	unsigned int old_carrier, new_carrier;
> -	int advertise, lpa, media, duplex;
> -	int lpa2 = 0;
> -
> -	/* if forced media, go no further */
> -	if (mii->force_media)
> -		return 0; /* duplex did not change */
> +	int duplex;
> +	struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET };
>   
>   	/* check current and old link status */
>   	old_carrier = netif_carrier_ok(mii->dev) ? 1 : 0;
> @@ -345,37 +340,21 @@ unsigned int mii_check_media (struct mii_if_info *mii,
>   	 */
>   	netif_carrier_on(mii->dev);
>   
> -	/* get MII advertise and LPA values */
> -	if ((!init_media) && (mii->advertising))
> -		advertise = mii->advertising;
> -	else {
> -		advertise = mii->mdio_read(mii->dev, mii->phy_id, MII_ADVERTISE);
> -		mii->advertising = advertise;

Now mii->advertising won't be initialised properly.

> -	}
> -	lpa = mii->mdio_read(mii->dev, mii->phy_id, MII_LPA);
> -	if (mii->supports_gmii)
> -		lpa2 = mii->mdio_read(mii->dev, mii->phy_id, MII_STAT1000);
> +	/*
> +	 * save the previous state of the duplex, mii_ethtool_gset()
> +	 * modifies it
> +	 */
> +	duplex = mii->full_duplex;
>   
> -	/* figure out media and duplex from advertise and LPA values */
> -	media = mii_nway_result(lpa & advertise);
> -	duplex = (media & ADVERTISE_FULL) ? 1 : 0;
> -	if (lpa2 & LPA_1000FULL)
> -		duplex = 1;
> +	mii_ethtool_gset(mii, &ecmd);
>   
>   	if (ok_to_print)
>   		netdev_info(mii->dev, "link up, %uMbps, %s-duplex, lpa 0x%04X\n",
> -			    lpa2 & (LPA_1000FULL | LPA_1000HALF) ? 1000 :
> -			    media & (ADVERTISE_100FULL | ADVERTISE_100HALF) ?
> -			    100 : 10,
> -			    duplex ? "full" : "half",
> -			    lpa);
> -
> -	if ((init_media) || (mii->full_duplex != duplex)) {
> -		mii->full_duplex = duplex;
> -		return 1; /* duplex changed */
> -	}
> +			    ethtool_cmd_speed(&ecmd), 
> +			    ecmd.duplex == DUPLEX_FULL ? "full" : "half",
> +			    ecmd.lp_advertising);

This log message used to include the raw LPA register value, but you're
changing it to use ecmd.lp_advertising which has entirely different flag
values.  In order to get the raw LPA value out, I think you'll need to
delete mii_get_an() and split up mii_ethtool_gset() so you end up with:

static void __mii_check_media(struct mii_if_info *mii, struct ethtool_cmd *ecmd,
			      u16 *raw_lpa)
{
	*raw_lpa = 0;
	...
		mii->advertising = mii->mdio_read(mii, mii->phy_id, MII_ADVERTISE);
		ecmd->advertising |= mii_lpa_to_ethtool_lpa_t(mii->advertising);
	...
			*raw_lpa = mii->mdio_read(mii, mii->phy_id, MII_LPA);
			ecmd->lp_advertising =
				mii_lpa_to_ethtool_lpa_t(*raw_lpa);
	...
}

int mii_ethtool_gset(struct mii_if_info *mii, struct ethtool_cmd *ecmd)
{
	u16 raw_lpa;
	__mii_check_media(mii, ecmd, &raw_lpa);
	return 0;
}

unsigned int mii_check_media (struct mii_if_info *mii,
			      unsigned int ok_to_print,
			      unsigned int init_media)
{
	u16 raw_lpa;
	...
	__mii_check_media(mii, ecmd, &raw_lpa);
	...
	if (ok_to_print)
		netdev_info(mii->dev, "link up, %uMbps, %s-duplex, lpa 0x%04X\n",
			    ethtool_cmd_speed(&ecmd), 
			    ecmd.duplex == DUPLEX_FULL ? "full" : "half",
			    raw_lpa);
	...
}

> -	return 0; /* duplex did not change */
> +	return (init_media || (mii->full_duplex != duplex)) ? 1 : 0;
>   }
>   
>   /**

No need for parentheses or the ?: operator.

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.