Patchwork 8139too: send NETDEV_CHANGE manually when autoneg is disabled

login
register
mail settings
Submitter Veaceslav Falico
Date March 11, 2013, 5:58 p.m.
Message ID <20130311175809.GA22957@redhat.com>
Download mbox | patch
Permalink /patch/226626/
State RFC
Delegated to: David Miller
Headers show

Comments

Veaceslav Falico - March 11, 2013, 5:58 p.m.
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(-)
Ben Hutchings - March 11, 2013, 9:31 p.m.
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.

Patch

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