[PATCH 0/2] Minor autonegociation changes, testers welcome

[PATCH 0/2] Minor autonegociation changes, testers welcome

[David Decotigny]

The following 2 patches fix a few minor issues noticed by Ben and that
I tried to address here. The code compiles and "looks" good but I
don't have the required hardware to test. If you have access to the
affected NICs, I would really appreciate your help testing the
patches:
 - stmmac: ethtool -a ethX
           ethtool -A ethX [same params except:] autoneg off
 - dl2k: ethtool -s ethX speed 10 autoneg off # should work
         likewise with 100, but not with 1000 (ethtool error)

IMHO, the main differences that could be experienced relate to the
return value of ethtool, which should now report errors more
frequently than before (most were ignored).

David Decotigny (2):
  net/stmmac: don't go through ethtool to start autonegociation
  net/dl2k: Don't reconfigure link @100Mbps when disabling autoneg
    @1Gbps

 drivers/net/dl2k.c                  |   19 ++++++-------------
 drivers/net/stmmac/stmmac_ethtool.c |   13 ++-----------
 2 files changed, 8 insertions(+), 24 deletions(-)

-- 
1.7.3.1

[PATCH 1/2] net/stmmac: don't go through ethtool to start autonegociation

[David Decotigny]

The driver used to call phy's ethtool configuration routine to start
autonegociation. This change has it call directly phy's routine to
start autonegociation.

IMPORTANT: initial version was hiding phy_start_aneg() return value,
           this patch returns it (<0 upon error).

Tested: module compiles, NOT tested on real hardware.
Signed-off-by: David Decotigny <decot@google.com>
---
 drivers/net/stmmac/stmmac_ethtool.c |   13 ++-----------
 1 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/stmmac/stmmac_ethtool.c b/drivers/net/stmmac/stmmac_ethtool.c
index 6f5aaeb..9c05cf0 100644
--- a/drivers/net/stmmac/stmmac_ethtool.c
+++ b/drivers/net/stmmac/stmmac_ethtool.c
@@ -236,17 +236,8 @@ stmmac_set_pauseparam(struct net_device *netdev,
 	priv->flow_ctrl = new_pause;
 
 	if (phy->autoneg) {
-		if (netif_running(netdev)) {
-			struct ethtool_cmd cmd = { .cmd = ETHTOOL_SSET };
-			/* auto-negotiation automatically restarted */
-			cmd.supported = phy->supported;
-			cmd.advertising = phy->advertising;
-			cmd.autoneg = phy->autoneg;
-			ethtool_cmd_speed_set(&cmd, phy->speed);
-			cmd.duplex = phy->duplex;
-			cmd.phy_address = phy->addr;
-			ret = phy_ethtool_sset(phy, &cmd);
-		}
+		if (netif_running(netdev))
+			ret = phy_start_aneg(phy);
 	} else
 		priv->hw->mac->flow_ctrl(priv->ioaddr, phy->duplex,
 					 priv->flow_ctrl, priv->pause);
-- 
1.7.3.1

Re: [PATCH 1/2] net/stmmac: don't go through ethtool to start autonegociation

[Giuseppe CAVALLARO]

On 5/10/2011 2:19 AM, David Decotigny wrote:
> The driver used to call phy's ethtool configuration routine to start
> autonegociation. This change has it call directly phy's routine to
> start autonegociation.
> 
> IMPORTANT: initial version was hiding phy_start_aneg() return value,
>            this patch returns it (<0 upon error).
> 
> Tested: module compiles, NOT tested on real hardware.
> Signed-off-by: David Decotigny <decot@google.com>

Sorry for the delay, I'm doing some tests with the stmmac on a "real"
HW. I'll come back asap.

Regards
Peppe

> ---
>  drivers/net/stmmac/stmmac_ethtool.c |   13 ++-----------
>  1 files changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/stmmac/stmmac_ethtool.c b/drivers/net/stmmac/stmmac_ethtool.c
> index 6f5aaeb..9c05cf0 100644
> --- a/drivers/net/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/stmmac/stmmac_ethtool.c
> @@ -236,17 +236,8 @@ stmmac_set_pauseparam(struct net_device *netdev,
>  	priv->flow_ctrl = new_pause;
>  
>  	if (phy->autoneg) {
> -		if (netif_running(netdev)) {
> -			struct ethtool_cmd cmd = { .cmd = ETHTOOL_SSET };
> -			/* auto-negotiation automatically restarted */
> -			cmd.supported = phy->supported;
> -			cmd.advertising = phy->advertising;
> -			cmd.autoneg = phy->autoneg;
> -			ethtool_cmd_speed_set(&cmd, phy->speed);
> -			cmd.duplex = phy->duplex;
> -			cmd.phy_address = phy->addr;
> -			ret = phy_ethtool_sset(phy, &cmd);
> -		}
> +		if (netif_running(netdev))
> +			ret = phy_start_aneg(phy);
>  	} else
>  		priv->hw->mac->flow_ctrl(priv->ioaddr, phy->duplex,
>  					 priv->flow_ctrl, priv->pause);

Re: [PATCH 1/2] net/stmmac: don't go through ethtool to start autonegociation

[Giuseppe CAVALLARO]

On 5/10/2011 3:47 PM, Giuseppe CAVALLARO wrote:
> On 5/10/2011 2:19 AM, David Decotigny wrote:
>> The driver used to call phy's ethtool configuration routine to start
>> autonegociation. This change has it call directly phy's routine to
>> start autonegociation.
>>
>> IMPORTANT: initial version was hiding phy_start_aneg() return value,
>>            this patch returns it (<0 upon error).
>>
>> Tested: module compiles, NOT tested on real hardware.
>> Signed-off-by: David Decotigny <decot@google.com>
> 
> Sorry for the delay, I'm doing some tests with the stmmac on a "real"
> HW. I'll come back asap.


Hello.

I've tested the patch and, as discussed with David, I have sent it again
plus a new fix in the stmmac_set_pauseparam I have done while testing
ethtool -A|-a on my ST box.

Best Regards,
Peppe

> 
> Regards
> Peppe
> 
>> ---
>>  drivers/net/stmmac/stmmac_ethtool.c |   13 ++-----------
>>  1 files changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/stmmac/stmmac_ethtool.c b/drivers/net/stmmac/stmmac_ethtool.c
>> index 6f5aaeb..9c05cf0 100644
>> --- a/drivers/net/stmmac/stmmac_ethtool.c
>> +++ b/drivers/net/stmmac/stmmac_ethtool.c
>> @@ -236,17 +236,8 @@ stmmac_set_pauseparam(struct net_device *netdev,
>>  	priv->flow_ctrl = new_pause;
>>  
>>  	if (phy->autoneg) {
>> -		if (netif_running(netdev)) {
>> -			struct ethtool_cmd cmd = { .cmd = ETHTOOL_SSET };
>> -			/* auto-negotiation automatically restarted */
>> -			cmd.supported = phy->supported;
>> -			cmd.advertising = phy->advertising;
>> -			cmd.autoneg = phy->autoneg;
>> -			ethtool_cmd_speed_set(&cmd, phy->speed);
>> -			cmd.duplex = phy->duplex;
>> -			cmd.phy_address = phy->addr;
>> -			ret = phy_ethtool_sset(phy, &cmd);
>> -		}
>> +		if (netif_running(netdev))
>> +			ret = phy_start_aneg(phy);
>>  	} else
>>  		priv->hw->mac->flow_ctrl(priv->ioaddr, phy->duplex,
>>  					 priv->flow_ctrl, priv->pause);
> 
> --
> 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
> 

[PATCH 2/2] net/dl2k: Don't reconfigure link @100Mbps when disabling autoneg @1Gbps

[David Decotigny]

The initial version of the driver used to force the link to 100Mbps
when auto-negociation was disabled on a 1Gbps link, ignoring the
requested link speed. Instead, this change refuses to change anything
when it is asked to configure the link speed at 1Gbps without
auto-negociation, but acts as requested in all the other cases.

IMPORTANT: Previously, the return value from mii_set_media() was
           ignored. This patch uses it for its own return value.

Tested: module compiling, NOT tested on real hardware.
Signed-off-by: David Decotigny <decot@google.com>
---
 drivers/net/dl2k.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dl2k.c b/drivers/net/dl2k.c
index c445457..1a4856b 100644
--- a/drivers/net/dl2k.c
+++ b/drivers/net/dl2k.c
@@ -1211,24 +1211,17 @@ static int rio_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 	if (cmd->autoneg == AUTONEG_ENABLE) {
 		if (np->an_enable)
 			return 0;
-		else {
-			np->an_enable = 1;
-			mii_set_media(dev);
-			return 0;
-		}
+
+		np->an_enable = 1;
 	} else {
-		np->an_enable = 0;
-		if (np->speed == 1000) {
-			ethtool_cmd_speed_set(cmd, SPEED_100);
-			cmd->duplex = DUPLEX_FULL;
-			printk("Warning!! Can't disable Auto negotiation in 1000Mbps, change to Manual 100Mbps, Full duplex.\n");
-		}
 		switch (ethtool_cmd_speed(cmd)) {
 		case SPEED_10:
+			np->an_enable = 0;
 			np->speed = 10;
 			np->full_duplex = (cmd->duplex == DUPLEX_FULL);
 			break;
 		case SPEED_100:
+			np->an_enable = 0;
 			np->speed = 100;
 			np->full_duplex = (cmd->duplex == DUPLEX_FULL);
 			break;
@@ -1236,9 +1229,9 @@ static int rio_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 		default:
 			return -EINVAL;
 		}
-		mii_set_media(dev);
 	}
-	return 0;
+
+	return mii_set_media(dev);
 }
 
 static u32 rio_get_link(struct net_device *dev)
-- 
1.7.3.1

Re: [PATCH 2/2] net/dl2k: Don't reconfigure link @100Mbps when disabling autoneg @1Gbps

[Ben Hutchings]

On Mon, 2011-05-09 at 17:19 -0700, David Decotigny wrote:
> The initial version of the driver used to force the link to 100Mbps
> when auto-negociation was disabled on a 1Gbps link, ignoring the
> requested link speed. Instead, this change refuses to change anything
> when it is asked to configure the link speed at 1Gbps without
> auto-negociation, but acts as requested in all the other cases.
> 
> IMPORTANT: Previously, the return value from mii_set_media() was
>            ignored. This patch uses it for its own return value.
> 
> Tested: module compiling, NOT tested on real hardware.
> Signed-off-by: David Decotigny <decot@google.com>
[...]

The changes to validation look fine.  However, I noticed that there's a
call to netif_carrier_off() at the top of this function.  This means
that in the error and shortcut cases, the interface will be left
disabled!  It's an existing bug but might be made slightly worse by this
change.

Please also move the call to netif_carrier_off() down to the end, just
before the call to mii_set_media() which actually alters the link.

Ben.

-- 
Ben Hutchings, Senior Software 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 2/2] net/dl2k: Don't reconfigure link @100Mbps when disabling autoneg @1Gbps

[David Decotigny]

Hi all,

Yes, right, I will send the updated patch together with the stmmac
update (if any).

I just hope that changing the netdev_private fields without committing
to hardware and before calling netif_carrier_off() will not create too
much confusion. I don't think so, but I still wish I could test.

Regards,

--
David Decotigny



On Tue, May 10, 2011 at 11:55 AM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> On Mon, 2011-05-09 at 17:19 -0700, David Decotigny wrote:
>> The initial version of the driver used to force the link to 100Mbps
>> when auto-negociation was disabled on a 1Gbps link, ignoring the
>> requested link speed. Instead, this change refuses to change anything
>> when it is asked to configure the link speed at 1Gbps without
>> auto-negociation, but acts as requested in all the other cases.
>>
>> IMPORTANT: Previously, the return value from mii_set_media() was
>>            ignored. This patch uses it for its own return value.
>>
>> Tested: module compiling, NOT tested on real hardware.
>> Signed-off-by: David Decotigny <decot@google.com>
> [...]
>
> The changes to validation look fine.  However, I noticed that there's a
> call to netif_carrier_off() at the top of this function.  This means
> that in the error and shortcut cases, the interface will be left
> disabled!  It's an existing bug but might be made slightly worse by this
> change.
>
> Please also move the call to netif_carrier_off() down to the end, just
> before the call to mii_set_media() which actually alters the link.
>
> Ben.
>
> --
> Ben Hutchings, Senior Software 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 2/2] net/dl2k: Don't reconfigure link @100Mbps when disabling autoneg @1Gbps

[Florian Weimer]

* David Decotigny:

> Tested: module compiling, NOT tested on real hardware.

To my knowledge, dl2k is broken.  Some sort of synchronization
primitives are missing.  Under load, the NIC's notion of ring buffer
status diverges from the host's view. 8-(

-- 
Florian Weimer                <fweimer@bfk.de>
BFK edv-consulting GmbH       http://www.bfk.de/
Kriegsstraße 100              tel: +49-721-96201-1
D-76133 Karlsruhe             fax: +49-721-96201-99

dl2k/stmmac patches (was Re: [PATCH 2/2] net/dl2k: Don't reconfigure link @100Mbps when disabling autoneg @1Gbps)

[David Decotigny]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Hi all,

An update on the patch series I posted earlier (dl2k/stmmac autoneg
minor changes: see my posts on 05/09/11 17:19 PST)...

I'd suggest dropping the patch concerning dl2k, as I don't have any way
to claim it's any good at all (though I'm pretty confident it doesn't
make things any worse).

However, for the other patch of the same series (stmmac), please
consider for inclusion the new version prepared by Giuseppe instead of
my initial patch, for his version readily integrates my patch and has
been tested on real hardware:
  stmmac: don't go through ethtool to start auto-negotiation
  stmmac: fix autoneg in set_pauseparam

Regards,
Thank you!

On 05/11/11 02:47, Florian Weimer wrote:
> * David Decotigny:
> 
>> Tested: module compiling, NOT tested on real hardware.
> 
> To my knowledge, dl2k is broken.  Some sort of synchronization
> primitives are missing.  Under load, the NIC's notion of ring buffer
> status diverges from the host's view. 8-(
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3NVCYACgkQld7vhusVrCHUEACggeKyCmoEHy9AzYec/aW8cDjU
GyAAoIiESxUAFWuKBmCOA31X/V0fvC+Y
=6hXY
-----END PGP SIGNATURE-----