Re: [PATCH] ethtool v4: add autoneg advertise feature

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Jeff Garzik]

Jeff@lime.pobox.com wrote:
> @@ -598,25 +608,7 @@ static void parse_cmdline(int argc, char
>  		}
>  	}
>  
> -	if (autoneg_wanted == AUTONEG_ENABLE){
> -		if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF)
> -			advertising_wanted = ADVERTISED_10baseT_Half;
> -		else if (speed_wanted == SPEED_10 &&
> -			 duplex_wanted == DUPLEX_FULL)
> -			advertising_wanted = ADVERTISED_10baseT_Full;
> -		else if (speed_wanted == SPEED_100 &&
> -			 duplex_wanted == DUPLEX_HALF)
> -			advertising_wanted = ADVERTISED_100baseT_Half;
> -		else if (speed_wanted == SPEED_100 &&
> -			 duplex_wanted == DUPLEX_FULL)
> -			advertising_wanted = ADVERTISED_100baseT_Full;
> -		else if (speed_wanted == SPEED_1000 &&
> -			 duplex_wanted == DUPLEX_HALF)
> -			advertising_wanted = ADVERTISED_1000baseT_Half;
> -		else if (speed_wanted == SPEED_1000 &&
> -			 duplex_wanted == DUPLEX_FULL)
> -			advertising_wanted = ADVERTISED_1000baseT_Full;
> -		else
> +	if ((autoneg_wanted == AUTONEG_ENABLE) && (advertising_wanted < 0)) {


This will change existing behavior of the tool, AFAICS.

	Jeff

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Jeff Kirsher]

On 8/23/06, Jeff Garzik <jgarzik@pobox.com> wrote:
> Jeff@lime.pobox.com wrote:
> > @@ -598,25 +608,7 @@ static void parse_cmdline(int argc, char
> >               }
> >       }
> >
> > -     if (autoneg_wanted == AUTONEG_ENABLE){
> > -             if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF)
> > -                     advertising_wanted = ADVERTISED_10baseT_Half;
> > -             else if (speed_wanted == SPEED_10 &&
> > -                      duplex_wanted == DUPLEX_FULL)
> > -                     advertising_wanted = ADVERTISED_10baseT_Full;
> > -             else if (speed_wanted == SPEED_100 &&
> > -                      duplex_wanted == DUPLEX_HALF)
> > -                     advertising_wanted = ADVERTISED_100baseT_Half;
> > -             else if (speed_wanted == SPEED_100 &&
> > -                      duplex_wanted == DUPLEX_FULL)
> > -                     advertising_wanted = ADVERTISED_100baseT_Full;
> > -             else if (speed_wanted == SPEED_1000 &&
> > -                      duplex_wanted == DUPLEX_HALF)
> > -                     advertising_wanted = ADVERTISED_1000baseT_Half;
> > -             else if (speed_wanted == SPEED_1000 &&
> > -                      duplex_wanted == DUPLEX_FULL)
> > -                     advertising_wanted = ADVERTISED_1000baseT_Full;
> > -             else
> > +     if ((autoneg_wanted == AUTONEG_ENABLE) && (advertising_wanted < 0)) {
>
>
> This will change existing behavior of the tool, AFAICS.
>

That is kinda the point.  The current beahvior only allows the user to
set the autonegotiation advertisement to one setting (i.e 100 Half or
100 Full or 10 Half or 10 Full).  With this patch the user is now able
to determine exactly speed's and duplex's will be advertised for
example, if a user is connected to a hub, they can tell the nic to
only advertise 100 half and 10 half.
The old way of setting autonegotiation was using the following command:
ethtool -s ethx speed 100 duplex full auto on
now the command would be
ethtool -s ethx auto on advertise 0x08
both commands would result in only advertising 100 FULL.

There still needs to be a change made to the man file to reflect the
change in the behavior of ethtool, which I have not done.  But this
patch will allow for greater flexibility in setting autonegotiation
speeds.

-- 
Cheers,
Jeff

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Michael Chan]

Jeff Kirsher wrote:

> The old way of setting autonegotiation was using the 
> following command:
> ethtool -s ethx speed 100 duplex full auto on
> now the command would be
> ethtool -s ethx auto on advertise 0x08
> both commands would result in only advertising 100 FULL.
> 
> There still needs to be a change made to the man file to reflect the
> change in the behavior of ethtool, which I have not done.  But this
> patch will allow for greater flexibility in setting autonegotiation
> speeds.
> 

It is more flexible, but less intuitive.  The user now has to
remember hex values instead of the more intuitive speed and
duplex.  Perhaps we can keep the old method of using speed and
duplex, while adding the new method of specifying hex values? 

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Jeff Kirsher]

On 8/24/06, Michael Chan <mchan@broadcom.com> wrote:
>
> It is more flexible, but less intuitive.  The user now has to
> remember hex values instead of the more intuitive speed and
> duplex.  Perhaps we can keep the old method of using speed and
> duplex, while adding the new method of specifying hex values?
>
>

That sounds fine to me.  I can send a modified patch, keeping the old
method available to users.

-- 
Cheers,
Jeff

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Auke Kok]

Michael Chan wrote:
> Jeff Kirsher wrote:
> 
>> The old way of setting autonegotiation was using the 
>> following command:
>> ethtool -s ethx speed 100 duplex full auto on
>> now the command would be
>> ethtool -s ethx auto on advertise 0x08
>> both commands would result in only advertising 100 FULL.
>>
>> There still needs to be a change made to the man file to reflect the
>> change in the behavior of ethtool, which I have not done.  But this
>> patch will allow for greater flexibility in setting autonegotiation
>> speeds.
> 
> It is more flexible, but less intuitive.  The user now has to
> remember hex values instead of the more intuitive speed and
> duplex.  Perhaps we can keep the old method of using speed and
> duplex, while adding the new method of specifying hex values? 

the patch doesn't remove the old method, it merely adds a second path to the 
speed/duplex setting. Using the "old" syntax will still work.

Cheers,

Auke

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Auke Kok]

Auke Kok wrote:
> Michael Chan wrote:
>> Jeff Kirsher wrote:
>>
>>> The old way of setting autonegotiation was using the following command:
>>> ethtool -s ethx speed 100 duplex full auto on
>>> now the command would be
>>> ethtool -s ethx auto on advertise 0x08
>>> both commands would result in only advertising 100 FULL.
>>>
>>> There still needs to be a change made to the man file to reflect the
>>> change in the behavior of ethtool, which I have not done.  But this
>>> patch will allow for greater flexibility in setting autonegotiation
>>> speeds.
>>
>> It is more flexible, but less intuitive.  The user now has to
>> remember hex values instead of the more intuitive speed and
>> duplex.  Perhaps we can keep the old method of using speed and
>> duplex, while adding the new method of specifying hex values? 
> 
> the patch doesn't remove the old method, it merely adds a second path to 
> the speed/duplex setting. Using the "old" syntax will still work.

hmm, I'm not saying I was wrong there and that if the patch did remove the old 
method there will certainly be a new patch that keeps the old functionality.

okay okay, I am ;)

Auke

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

On 8/24/06, Auke Kok <auke-jan.h.kok@intel.com> wrote:
> the patch doesn't remove the old method, it merely adds a second path to the
> speed/duplex setting. Using the "old" syntax will still work.
>
> Cheers,
>
> Auke
>

Actually, I did remove the functionality of the "old" method.  The
attached patch revises that.  I will also send this revised patch
inline and not as an attachment.

-- 
Cheers,
Jeff

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: autoneg_advertise.patch --]
[-- Type: text/x-patch; name="autoneg_advertise.patch", Size: 1633 bytes --]

ethtool v4: add autoneg advertise feature

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

adds the ability to change the advertised speed and duplex for a network interface.  Previously, a network interface was only able to advertise all supported speed's and duplex's, or one individual speed and duplex.  The feature allows the user to choose which supported speed's and duplex's to advertise by using the hex value.
---

 ethtool.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 87e22ab..b7f189a 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -99,6 +99,7 @@ static struct option {
 		"		[ duplex half|full ]\n"
 		"		[ port tp|aui|bnc|mii|fibre ]\n"
 		"		[ autoneg on|off ]\n"
+		"		[ advertise %%x ]\n"
 		"		[ phyad %%d ]\n"
 		"		[ xcvr internal|external ]\n"
 		"		[ wol p|u|m|b|a|g|s|d... ]\n"
@@ -549,6 +550,15 @@ static void parse_cmdline(int argc, char
 					show_usage(1);
 				}
 				break;
+			} else if (!strcmp(argp[i], "advertise")) {
+				gset_changed = 1;
+				i += 1;
+				if (i >= argc)
+					show_usage(1);
+				advertising_wanted = strtol(argp[i], NULL, 16);
+				if (advertising_wanted < 0)
+					show_usage(1);
+				break;
 			} else if (!strcmp(argp[i], "phyad")) {
 				gset_changed = 1;
 				i += 1;
@@ -601,7 +611,7 @@ static void parse_cmdline(int argc, char
 		}
 	}
 
-	if (autoneg_wanted == AUTONEG_ENABLE){
+	if ((autoneg_wanted == AUTONEG_ENABLE) && (advertising_wanted < 0)) {
 		if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF)
 			advertising_wanted = ADVERTISED_10baseT_Half;
 		else if (speed_wanted == SPEED_10 &&

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Bill Fink]

On Thu, 24 Aug 2006, Michael Chan wrote:

> Jeff Kirsher wrote:
> 
> > The old way of setting autonegotiation was using the 
> > following command:
> > ethtool -s ethx speed 100 duplex full auto on
> > now the command would be
> > ethtool -s ethx auto on advertise 0x08
> > both commands would result in only advertising 100 FULL.
> > 
> > There still needs to be a change made to the man file to reflect the
> > change in the behavior of ethtool, which I have not done.  But this
> > patch will allow for greater flexibility in setting autonegotiation
> > speeds.
> 
> It is more flexible, but less intuitive.  The user now has to
> remember hex values instead of the more intuitive speed and
> duplex.  Perhaps we can keep the old method of using speed and
> duplex, while adding the new method of specifying hex values? 

I agree.  Something like:

	ethtool -s ethx auto on advertise mode1+mode2+...+moden

For example:

	ethtool -s ethx auto on advertise 100-half+100-full

to set speed 100 either half or full duplex.

Maybe have some abbreviations such as 100-all (same as above) or
all-half (for all supported half duplex) or just all (for all supported
modes), which I suppose is the default.

Just an idea.

						-Bill

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Jeff Kirsher]

On 8/25/06, Bill Fink <billfink@mindspring.com> wrote:
> On Thu, 24 Aug 2006, Michael Chan wrote:
>
> > Jeff Kirsher wrote:
> >
> > > The old way of setting autonegotiation was using the
> > > following command:
> > > ethtool -s ethx speed 100 duplex full auto on
> > > now the command would be
> > > ethtool -s ethx auto on advertise 0x08
> > > both commands would result in only advertising 100 FULL.
> > >
> > > There still needs to be a change made to the man file to reflect the
> > > change in the behavior of ethtool, which I have not done.  But this
> > > patch will allow for greater flexibility in setting autonegotiation
> > > speeds.
> >
> > It is more flexible, but less intuitive.  The user now has to
> > remember hex values instead of the more intuitive speed and
> > duplex.  Perhaps we can keep the old method of using speed and
> > duplex, while adding the new method of specifying hex values?
>
> I agree.  Something like:
>
>         ethtool -s ethx auto on advertise mode1+mode2+...+moden
>
> For example:
>
>         ethtool -s ethx auto on advertise 100-half+100-full
>
> to set speed 100 either half or full duplex.
>
> Maybe have some abbreviations such as 100-all (same as above) or
> all-half (for all supported half duplex) or just all (for all supported
> modes), which I suppose is the default.
>
> Just an idea.
>
>                                                 -Bill
>

I agree that using a hex value is less intuitive, but with proper
documentation in the man file it would be easily understood.  It is
also easier to state
ethtool -s ethx autoneg on advertise 0x0F
than it would be to do:
ethtool -s ethx autoneg on advertise 100-half+100-full+10-half+10-full

Not that it is impossible to do, but the code to do the parsing would
not be as clean as it is to use a hex value.  Currently ethtool
already uses numeric values for messagelevel, phyad and sopass.  So I
am not suggesting something completely new.  I have already submitted
a patch to keep the old functionality while adding the new.  Only
thing left for this is to create the manual documentation so that
users can easily understand how to use the functionality.

10-half     = 0x01
10-full      = 0x02
100-half   = 0x04
100-full    = 0x08
1000-half = 0x10 (actually not supported by IEEE standards)
1000-full  = 0x20
auto        = 0x00 or 0x3F

In addition the code already tests the value that the user enters with
what is supported and only displays the supported values.

-- 
Cheers,
Jeff

Re: [PATCH] ethtool v4: add autoneg advertise feature

[Bill Fink]

On Fri, 25 Aug 2006, Jeff Kirsher wrote:

> On 8/25/06, Bill Fink <billfink@mindspring.com> wrote:
> >
> > I agree.  Something like:
> >
> >         ethtool -s ethx auto on advertise mode1+mode2+...+moden
> >
> > For example:
> >
> >         ethtool -s ethx auto on advertise 100-half+100-full
> >
> > to set speed 100 either half or full duplex.
> >
> > Maybe have some abbreviations such as 100-all (same as above) or
> > all-half (for all supported half duplex) or just all (for all supported
> > modes), which I suppose is the default.
> >
> > Just an idea.
> >
> >                                                 -Bill
> >
> 
> I agree that using a hex value is less intuitive, but with proper
> documentation in the man file it would be easily understood.  It is
> also easier to state
> ethtool -s ethx autoneg on advertise 0x0F
> than it would be to do:
> ethtool -s ethx autoneg on advertise 100-half+100-full+10-half+10-full

This could be abbreviated to:

	ethtool -s ethx autoneg on advertise 100-all+10-all

> Not that it is impossible to do, but the code to do the parsing would
> not be as clean as it is to use a hex value.  Currently ethtool
> already uses numeric values for messagelevel, phyad and sopass.  So I
> am not suggesting something completely new.  I have already submitted
> a patch to keep the old functionality while adding the new.  Only
> thing left for this is to create the manual documentation so that
> users can easily understand how to use the functionality.
> 
> 10-half     = 0x01
> 10-full      = 0x02
> 100-half   = 0x04
> 100-full    = 0x08
> 1000-half = 0x10 (actually not supported by IEEE standards)

I thought the above wasn't a supported option.

> 1000-full  = 0x20
> auto        = 0x00 or 0x3F
> 
> In addition the code already tests the value that the user enters with
> what is supported and only displays the supported values.

I agree that with decent documentation, use of the hex values shouldn't
be that difficult for most users, although using hex arithmetic might
be Greek to some.  I was just suggesting a possible alternative, but
I admit it's a fairly minor issue one way or the other.

						-Bill