[PATCH] ethtool: don't overwrite useful bits in advertising bitfield

[PATCH] ethtool: don't overwrite useful bits in advertising bitfield

[Johan Gunnarsson]

There are bits in this bitfield that we want to leave untouched (PAUSE
and ASYM_PAUSE bits) when changing other bits (speed and duplex bits.)
Previously, these were always overwritten to zero when running commands
like "ethtool -s eth0 speed 10 duplex full autoneg off".

Signed-off-by: Johan Gunnarsson <johangu@axis.com>
---
 ethtool.c |   45 +++++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index e573357..efa12c7 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -46,6 +46,18 @@
 #define MAX_ADDR_LEN	32
 #endif
 
+#define ALL_ADVERTISED_MODES \
+	(ADVERTISED_10baseT_Half | \
+	 ADVERTISED_10baseT_Full | \
+	 ADVERTISED_100baseT_Half | \
+	 ADVERTISED_100baseT_Full | \
+	 ADVERTISED_1000baseT_Half | \
+	 ADVERTISED_1000baseT_Full | \
+	 ADVERTISED_2500baseX_Full | \
+	 ADVERTISED_10000baseT_Full | \
+	 ADVERTISED_20000baseMLD2_Full | \
+	 ADVERTISED_20000baseKR2_Full)
+
 #ifndef HAVE_NETIF_MSG
 enum {
 	NETIF_MSG_DRV		= 0x0001,
@@ -2202,6 +2214,7 @@ static int do_sset(struct cmd_context *ctx)
 	int autoneg_wanted = -1;
 	int phyad_wanted = -1;
 	int xcvr_wanted = -1;
+	int full_advertising_wanted = -1;
 	int advertising_wanted = -1;
 	int gset_changed = 0; /* did anything in GSET change? */
 	u32 wol_wanted = 0;
@@ -2277,7 +2290,7 @@ static int do_sset(struct cmd_context *ctx)
 			i += 1;
 			if (i >= argc)
 				exit_bad_args();
-			advertising_wanted = get_int(argp[i], 16);
+			full_advertising_wanted = get_int(argp[i], 16);
 		} else if (!strcmp(argp[i], "phyad")) {
 			gset_changed = 1;
 			i += 1;
@@ -2334,7 +2347,10 @@ static int do_sset(struct cmd_context *ctx)
 		}
 	}
 
-	if (advertising_wanted < 0) {
+	if (full_advertising_wanted < 0) {
+		/* User didn't supply a full advertisement bitfield:
+		 * construct one from the specified speed and duplex.
+		 */
 		if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF)
 			advertising_wanted = ADVERTISED_10baseT_Half;
 		else if (speed_wanted == SPEED_10 &&
@@ -2405,19 +2421,20 @@ static int do_sset(struct cmd_context *ctx)
 			}
 			if (autoneg_wanted == AUTONEG_ENABLE &&
 			    advertising_wanted == 0) {
-				ecmd.advertising = ecmd.supported &
-					(ADVERTISED_10baseT_Half |
-					 ADVERTISED_10baseT_Full |
-					 ADVERTISED_100baseT_Half |
-					 ADVERTISED_100baseT_Full |
-					 ADVERTISED_1000baseT_Half |
-					 ADVERTISED_1000baseT_Full |
-					 ADVERTISED_2500baseX_Full |
-					 ADVERTISED_10000baseT_Full |
-					 ADVERTISED_20000baseMLD2_Full |
-					 ADVERTISED_20000baseKR2_Full);
+				/* Auto negotation enabled, but with
+				 * unspecified speed and duplex: enable all
+				 * supported speeds and duplexes.
+				 */
+				ecmd.advertising = (ecmd.advertising &
+					~ALL_ADVERTISED_MODES) |
+					(ALL_ADVERTISED_MODES & ecmd.supported);
 			} else if (advertising_wanted > 0) {
-				ecmd.advertising = advertising_wanted;
+				/* Enable all requested modes */
+				ecmd.advertising = (ecmd.advertising &
+					~ALL_ADVERTISED_MODES) |
+					(advertising_wanted & ecmd.supported);
+			} else if (full_advertising_wanted > 0) {
+				ecmd.advertising = full_advertising_wanted;
 			}
 
 			/* Try to perform the update. */
-- 
1.7.10

Re: [PATCH] ethtool: don't overwrite useful bits in advertising bitfield

[Ben Hutchings]

On Tue, 2012-08-14 at 16:15 +0200, Johan Gunnarsson wrote:
> There are bits in this bitfield that we want to leave untouched (PAUSE
> and ASYM_PAUSE bits) when changing other bits (speed and duplex bits.)
> Previously, these were always overwritten to zero when running commands
> like "ethtool -s eth0 speed 10 duplex full autoneg off".

This is right in principle, but the implementation isn't quite right.

> Signed-off-by: Johan Gunnarsson <johangu@axis.com>
> ---
>  ethtool.c |   45 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index e573357..efa12c7 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -46,6 +46,18 @@
>  #define MAX_ADDR_LEN	32
>  #endif
>  
> +#define ALL_ADVERTISED_MODES \
> +	(ADVERTISED_10baseT_Half | \
> +	 ADVERTISED_10baseT_Full | \
> +	 ADVERTISED_100baseT_Half | \
> +	 ADVERTISED_100baseT_Full | \
> +	 ADVERTISED_1000baseT_Half | \
> +	 ADVERTISED_1000baseT_Full | \
> +	 ADVERTISED_2500baseX_Full | \
> +	 ADVERTISED_10000baseT_Full | \
> +	 ADVERTISED_20000baseMLD2_Full | \
> +	 ADVERTISED_20000baseKR2_Full)

This is missing the new 40G modes (not a regression, I realise).

[...]
> @@ -2405,19 +2421,20 @@ static int do_sset(struct cmd_context *ctx)
>  			}
>  			if (autoneg_wanted == AUTONEG_ENABLE &&
>  			    advertising_wanted == 0) {
> -				ecmd.advertising = ecmd.supported &
> -					(ADVERTISED_10baseT_Half |
> -					 ADVERTISED_10baseT_Full |
> -					 ADVERTISED_100baseT_Half |
> -					 ADVERTISED_100baseT_Full |
> -					 ADVERTISED_1000baseT_Half |
> -					 ADVERTISED_1000baseT_Full |
> -					 ADVERTISED_2500baseX_Full |
> -					 ADVERTISED_10000baseT_Full |
> -					 ADVERTISED_20000baseMLD2_Full |
> -					 ADVERTISED_20000baseKR2_Full);
> +				/* Auto negotation enabled, but with
> +				 * unspecified speed and duplex: enable all
> +				 * supported speeds and duplexes.
> +				 */
> +				ecmd.advertising = (ecmd.advertising &
> +					~ALL_ADVERTISED_MODES) |
> +					(ALL_ADVERTISED_MODES & ecmd.supported);

Perhaps we should also warn if there's a 'supported' flag we don't
recognise, because we don't know whether it's a link mode and we might
be failing to enable/disable it as requested.

>  			} else if (advertising_wanted > 0) {
> -				ecmd.advertising = advertising_wanted;
> +				/* Enable all requested modes */
> +				ecmd.advertising = (ecmd.advertising &
> +					~ALL_ADVERTISED_MODES) |
> +					(advertising_wanted & ecmd.supported);

I don't think the '& ecmd.supported' here is right.  If an autoneg
device supports some new link mode L that is not in
ALL_ADVERTISED_MODES, but not link mode M which the user requested, then
this can silently fail because the resulting advertising mask will
include L but not M.

We should either use advertising_wanted unmasked and let the driver
validate it, or report an error if it's not present in the supported
mask.  I think we should be consistent with the following case, i.e. let
the driver validate it.

Ben.

> +			} else if (full_advertising_wanted > 0) {
> +				ecmd.advertising = full_advertising_wanted;
>  			}
>  
>  			/* Try to perform the update. */

-- 
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] ethtool: don't overwrite useful bits in advertising bitfield

[Johan Gunnarsson]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Ben Hutchings
> Sent: den 20 augusti 2012 17:23
> To: Johan Gunnarsson
> Cc: netdev@vger.kernel.org; Mikael Starvik
> Subject: Re: [PATCH] ethtool: don't overwrite useful bits in
> advertising bitfield
> 
> On Tue, 2012-08-14 at 16:15 +0200, Johan Gunnarsson wrote:
> > There are bits in this bitfield that we want to leave untouched
> (PAUSE
> > and ASYM_PAUSE bits) when changing other bits (speed and duplex
> bits.)
> > Previously, these were always overwritten to zero when running
> commands
> > like "ethtool -s eth0 speed 10 duplex full autoneg off".
> 
> This is right in principle, but the implementation isn't quite right.
> 
> > Signed-off-by: Johan Gunnarsson <johangu@axis.com>
> > ---
> >  ethtool.c |   45 +++++++++++++++++++++++++++++++--------------
> >  1 file changed, 31 insertions(+), 14 deletions(-)
> >
> > diff --git a/ethtool.c b/ethtool.c
> > index e573357..efa12c7 100644
> > --- a/ethtool.c
> > +++ b/ethtool.c
> > @@ -46,6 +46,18 @@
> >  #define MAX_ADDR_LEN	32
> >  #endif
> >
> > +#define ALL_ADVERTISED_MODES \
> > +	(ADVERTISED_10baseT_Half | \
> > +	 ADVERTISED_10baseT_Full | \
> > +	 ADVERTISED_100baseT_Half | \
> > +	 ADVERTISED_100baseT_Full | \
> > +	 ADVERTISED_1000baseT_Half | \
> > +	 ADVERTISED_1000baseT_Full | \
> > +	 ADVERTISED_2500baseX_Full | \
> > +	 ADVERTISED_10000baseT_Full | \
> > +	 ADVERTISED_20000baseMLD2_Full | \
> > +	 ADVERTISED_20000baseKR2_Full)
> 
> This is missing the new 40G modes (not a regression, I realise).

I'll add the 40G modes. There is also a bunch of 10G modes missing. Shall I add these too?

> 
> [...]
> > @@ -2405,19 +2421,20 @@ static int do_sset(struct cmd_context *ctx)
> >  			}
> >  			if (autoneg_wanted == AUTONEG_ENABLE &&
> >  			    advertising_wanted == 0) {
> > -				ecmd.advertising = ecmd.supported &
> > -					(ADVERTISED_10baseT_Half |
> > -					 ADVERTISED_10baseT_Full |
> > -					 ADVERTISED_100baseT_Half |
> > -					 ADVERTISED_100baseT_Full |
> > -					 ADVERTISED_1000baseT_Half |
> > -					 ADVERTISED_1000baseT_Full |
> > -					 ADVERTISED_2500baseX_Full |
> > -					 ADVERTISED_10000baseT_Full |
> > -					 ADVERTISED_20000baseMLD2_Full |
> > -					 ADVERTISED_20000baseKR2_Full);
> > +				/* Auto negotation enabled, but with
> > +				 * unspecified speed and duplex: enable all
> > +				 * supported speeds and duplexes.
> > +				 */
> > +				ecmd.advertising = (ecmd.advertising &
> > +					~ALL_ADVERTISED_MODES) |
> > +					(ALL_ADVERTISED_MODES & ecmd.supported);
> 
> Perhaps we should also warn if there's a 'supported' flag we don't
> recognise, because we don't know whether it's a link mode and we might
> be failing to enable/disable it as requested.

You mean if ecmd.supported has bits enabled that ALL_ADVERTISED_MODES hasn't? I don't think that's a good idea, because that happens very often (for example PAUSE bits in my case.)

> 
> >  			} else if (advertising_wanted > 0) {
> > -				ecmd.advertising = advertising_wanted;
> > +				/* Enable all requested modes */
> > +				ecmd.advertising = (ecmd.advertising &
> > +					~ALL_ADVERTISED_MODES) |
> > +					(advertising_wanted & ecmd.supported);
> 
> I don't think the '& ecmd.supported' here is right.  If an autoneg
> device supports some new link mode L that is not in
> ALL_ADVERTISED_MODES, but not link mode M which the user requested,
> then
> this can silently fail because the resulting advertising mask will
> include L but not M.
> 
> We should either use advertising_wanted unmasked and let the driver
> validate it, or report an error if it's not present in the supported
> mask.  I think we should be consistent with the following case, i.e.
> let
> the driver validate it.

How about remove "& ecmd.supported", but also warn if trying to add an unsupported mode? Similar to the previous case.

> 
> Ben.
> 
> > +			} else if (full_advertising_wanted > 0) {
> > +				ecmd.advertising = full_advertising_wanted;
> >  			}
> >
> >  			/* Try to perform the update. */
> 
> --
> 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

RE: [PATCH] ethtool: don't overwrite useful bits in advertising bitfield

[Ben Hutchings]

On Tue, 2012-08-21 at 10:41 +0200, Johan Gunnarsson wrote:
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-
> > owner@vger.kernel.org] On Behalf Of Ben Hutchings
> > Sent: den 20 augusti 2012 17:23
> > To: Johan Gunnarsson
> > Cc: netdev@vger.kernel.org; Mikael Starvik
> > Subject: Re: [PATCH] ethtool: don't overwrite useful bits in
> > advertising bitfield
> > 
> > On Tue, 2012-08-14 at 16:15 +0200, Johan Gunnarsson wrote:
> > > There are bits in this bitfield that we want to leave untouched
> > (PAUSE
> > > and ASYM_PAUSE bits) when changing other bits (speed and duplex
> > bits.)
> > > Previously, these were always overwritten to zero when running
> > commands
> > > like "ethtool -s eth0 speed 10 duplex full autoneg off".
> > 
> > This is right in principle, but the implementation isn't quite right.
> > 
> > > Signed-off-by: Johan Gunnarsson <johangu@axis.com>
> > > ---
> > >  ethtool.c |   45 +++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 31 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/ethtool.c b/ethtool.c
> > > index e573357..efa12c7 100644
> > > --- a/ethtool.c
> > > +++ b/ethtool.c
> > > @@ -46,6 +46,18 @@
> > >  #define MAX_ADDR_LEN	32
> > >  #endif
> > >
> > > +#define ALL_ADVERTISED_MODES \
> > > +	(ADVERTISED_10baseT_Half | \
> > > +	 ADVERTISED_10baseT_Full | \
> > > +	 ADVERTISED_100baseT_Half | \
> > > +	 ADVERTISED_100baseT_Full | \
> > > +	 ADVERTISED_1000baseT_Half | \
> > > +	 ADVERTISED_1000baseT_Full | \
> > > +	 ADVERTISED_2500baseX_Full | \
> > > +	 ADVERTISED_10000baseT_Full | \
> > > +	 ADVERTISED_20000baseMLD2_Full | \
> > > +	 ADVERTISED_20000baseKR2_Full)
> > 
> > This is missing the new 40G modes (not a regression, I realise).
> 
> I'll add the 40G modes. There is also a bunch of 10G modes missing. Shall I add these too?

Yes please.

> > 
> > [...]
> > > @@ -2405,19 +2421,20 @@ static int do_sset(struct cmd_context *ctx)
> > >  			}
> > >  			if (autoneg_wanted == AUTONEG_ENABLE &&
> > >  			    advertising_wanted == 0) {
> > > -				ecmd.advertising = ecmd.supported &
> > > -					(ADVERTISED_10baseT_Half |
> > > -					 ADVERTISED_10baseT_Full |
> > > -					 ADVERTISED_100baseT_Half |
> > > -					 ADVERTISED_100baseT_Full |
> > > -					 ADVERTISED_1000baseT_Half |
> > > -					 ADVERTISED_1000baseT_Full |
> > > -					 ADVERTISED_2500baseX_Full |
> > > -					 ADVERTISED_10000baseT_Full |
> > > -					 ADVERTISED_20000baseMLD2_Full |
> > > -					 ADVERTISED_20000baseKR2_Full);
> > > +				/* Auto negotation enabled, but with
> > > +				 * unspecified speed and duplex: enable all
> > > +				 * supported speeds and duplexes.
> > > +				 */
> > > +				ecmd.advertising = (ecmd.advertising &
> > > +					~ALL_ADVERTISED_MODES) |
> > > +					(ALL_ADVERTISED_MODES & ecmd.supported);
> > 
> > Perhaps we should also warn if there's a 'supported' flag we don't
> > recognise, because we don't know whether it's a link mode and we might
> > be failing to enable/disable it as requested.
> 
> You mean if ecmd.supported has bits enabled that ALL_ADVERTISED_MODES
> hasn't? I don't think that's a good idea, because that happens very
> often (for example PAUSE bits in my case.)

No, I mean if it has bits enabled that are not defined at all (currently
any of bits 27-31).

> > 
> > >  			} else if (advertising_wanted > 0) {
> > > -				ecmd.advertising = advertising_wanted;
> > > +				/* Enable all requested modes */
> > > +				ecmd.advertising = (ecmd.advertising &
> > > +					~ALL_ADVERTISED_MODES) |
> > > +					(advertising_wanted & ecmd.supported);
> > 
> > I don't think the '& ecmd.supported' here is right.  If an autoneg
> > device supports some new link mode L that is not in
> > ALL_ADVERTISED_MODES, but not link mode M which the user requested,
> > then
> > this can silently fail because the resulting advertising mask will
> > include L but not M.
> > 
> > We should either use advertising_wanted unmasked and let the driver
> > validate it, or report an error if it's not present in the supported
> > mask.  I think we should be consistent with the following case, i.e.
> > let
> > the driver validate it.
> 
> How about remove "& ecmd.supported", but also warn if trying to add an
> unsupported mode? Similar to the previous case.
[...]

I don't think this is similar and I don't think we need to do both.

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] ethtool: don't overwrite useful bits in advertising bitfield

[Johan Gunnarsson]

> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: den 21 augusti 2012 17:12
> To: Johan Gunnarsson
> Cc: netdev@vger.kernel.org; Mikael Starvik
> Subject: RE: [PATCH] ethtool: don't overwrite useful bits in
> advertising bitfield
> 
> On Tue, 2012-08-21 at 10:41 +0200, Johan Gunnarsson wrote:
> >
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org [mailto:netdev-
> > > owner@vger.kernel.org] On Behalf Of Ben Hutchings
> > > Sent: den 20 augusti 2012 17:23
> > > To: Johan Gunnarsson
> > > Cc: netdev@vger.kernel.org; Mikael Starvik
> > > Subject: Re: [PATCH] ethtool: don't overwrite useful bits in
> > > advertising bitfield
> > >
> > > On Tue, 2012-08-14 at 16:15 +0200, Johan Gunnarsson wrote:
> > > > There are bits in this bitfield that we want to leave untouched
> > > (PAUSE
> > > > and ASYM_PAUSE bits) when changing other bits (speed and duplex
> > > bits.)
> > > > Previously, these were always overwritten to zero when running
> > > commands
> > > > like "ethtool -s eth0 speed 10 duplex full autoneg off".
> > >
> > > This is right in principle, but the implementation isn't quite
> right.
> > >
> > > > Signed-off-by: Johan Gunnarsson <johangu@axis.com>
> > > > ---
> > > >  ethtool.c |   45 +++++++++++++++++++++++++++++++--------------
> > > >  1 file changed, 31 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/ethtool.c b/ethtool.c
> > > > index e573357..efa12c7 100644
> > > > --- a/ethtool.c
> > > > +++ b/ethtool.c
> > > > @@ -46,6 +46,18 @@
> > > >  #define MAX_ADDR_LEN	32
> > > >  #endif
> > > >
> > > > +#define ALL_ADVERTISED_MODES \
> > > > +	(ADVERTISED_10baseT_Half | \
> > > > +	 ADVERTISED_10baseT_Full | \
> > > > +	 ADVERTISED_100baseT_Half | \
> > > > +	 ADVERTISED_100baseT_Full | \
> > > > +	 ADVERTISED_1000baseT_Half | \
> > > > +	 ADVERTISED_1000baseT_Full | \
> > > > +	 ADVERTISED_2500baseX_Full | \
> > > > +	 ADVERTISED_10000baseT_Full | \
> > > > +	 ADVERTISED_20000baseMLD2_Full | \
> > > > +	 ADVERTISED_20000baseKR2_Full)
> > >
> > > This is missing the new 40G modes (not a regression, I realise).
> >
> > I'll add the 40G modes. There is also a bunch of 10G modes missing.
> Shall I add these too?
> 
> Yes please.

Alright.

> 
> > >
> > > [...]
> > > > @@ -2405,19 +2421,20 @@ static int do_sset(struct cmd_context
> *ctx)
> > > >  			}
> > > >  			if (autoneg_wanted == AUTONEG_ENABLE &&
> > > >  			    advertising_wanted == 0) {
> > > > -				ecmd.advertising = ecmd.supported &
> > > > -					(ADVERTISED_10baseT_Half |
> > > > -					 ADVERTISED_10baseT_Full |
> > > > -					 ADVERTISED_100baseT_Half |
> > > > -					 ADVERTISED_100baseT_Full |
> > > > -					 ADVERTISED_1000baseT_Half |
> > > > -					 ADVERTISED_1000baseT_Full |
> > > > -					 ADVERTISED_2500baseX_Full |
> > > > -					 ADVERTISED_10000baseT_Full |
> > > > -					 ADVERTISED_20000baseMLD2_Full |
> > > > -					 ADVERTISED_20000baseKR2_Full);
> > > > +				/* Auto negotation enabled, but with
> > > > +				 * unspecified speed and duplex: enable
> all
> > > > +				 * supported speeds and duplexes.
> > > > +				 */
> > > > +				ecmd.advertising = (ecmd.advertising &
> > > > +					~ALL_ADVERTISED_MODES) |
> > > > +					(ALL_ADVERTISED_MODES &
> ecmd.supported);
> > >
> > > Perhaps we should also warn if there's a 'supported' flag we don't
> > > recognise, because we don't know whether it's a link mode and we
> might
> > > be failing to enable/disable it as requested.
> >
> > You mean if ecmd.supported has bits enabled that ALL_ADVERTISED_MODES
> > hasn't? I don't think that's a good idea, because that happens very
> > often (for example PAUSE bits in my case.)
> 
> No, I mean if it has bits enabled that are not defined at all
> (currently
> any of bits 27-31).

Not totally sure I follow. Care to show me?

> 
> > >
> > > >  			} else if (advertising_wanted > 0) {
> > > > -				ecmd.advertising = advertising_wanted;
> > > > +				/* Enable all requested modes */
> > > > +				ecmd.advertising = (ecmd.advertising &
> > > > +					~ALL_ADVERTISED_MODES) |
> > > > +					(advertising_wanted &
> ecmd.supported);
> > >
> > > I don't think the '& ecmd.supported' here is right.  If an autoneg
> > > device supports some new link mode L that is not in
> > > ALL_ADVERTISED_MODES, but not link mode M which the user requested,
> > > then
> > > this can silently fail because the resulting advertising mask will
> > > include L but not M.
> > >
> > > We should either use advertising_wanted unmasked and let the driver
> > > validate it, or report an error if it's not present in the
> supported
> > > mask.  I think we should be consistent with the following case,
> i.e.
> > > let
> > > the driver validate it.
> >
> > How about remove "& ecmd.supported", but also warn if trying to add
> an
> > unsupported mode? Similar to the previous case.
> [...]
> 
> I don't think this is similar and I don't think we need to do both.

So let's go for the removed "& ecmd.supported" then.

> 
> 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] ethtool: don't overwrite useful bits in advertising bitfield

[Ben Hutchings]

On Tue, 2012-08-21 at 18:45 +0200, Johan Gunnarsson wrote:
[...]
> > > > > @@ -2405,19 +2421,20 @@ static int do_sset(struct cmd_context
> > *ctx)
> > > > >  			}
> > > > >  			if (autoneg_wanted == AUTONEG_ENABLE &&
> > > > >  			    advertising_wanted == 0) {
> > > > > -				ecmd.advertising = ecmd.supported &
> > > > > -					(ADVERTISED_10baseT_Half |
> > > > > -					 ADVERTISED_10baseT_Full |
> > > > > -					 ADVERTISED_100baseT_Half |
> > > > > -					 ADVERTISED_100baseT_Full |
> > > > > -					 ADVERTISED_1000baseT_Half |
> > > > > -					 ADVERTISED_1000baseT_Full |
> > > > > -					 ADVERTISED_2500baseX_Full |
> > > > > -					 ADVERTISED_10000baseT_Full |
> > > > > -					 ADVERTISED_20000baseMLD2_Full |
> > > > > -					 ADVERTISED_20000baseKR2_Full);
> > > > > +				/* Auto negotation enabled, but with
> > > > > +				 * unspecified speed and duplex: enable
> > all
> > > > > +				 * supported speeds and duplexes.
> > > > > +				 */
> > > > > +				ecmd.advertising = (ecmd.advertising &
> > > > > +					~ALL_ADVERTISED_MODES) |
> > > > > +					(ALL_ADVERTISED_MODES &
> > ecmd.supported);
> > > >
> > > > Perhaps we should also warn if there's a 'supported' flag we don't
> > > > recognise, because we don't know whether it's a link mode and we
> > might
> > > > be failing to enable/disable it as requested.
> > >
> > > You mean if ecmd.supported has bits enabled that ALL_ADVERTISED_MODES
> > > hasn't? I don't think that's a good idea, because that happens very
> > > often (for example PAUSE bits in my case.)
> > 
> > No, I mean if it has bits enabled that are not defined at all
> > (currently
> > any of bits 27-31).
> 
> Not totally sure I follow. Care to show me?
[...]

<linux/ethtool.h> or ethtool-copy.h currently defines the meanings of
bits 0-26 in the supported field.  You define ALL_ADVERTISED_MODES to
include all of those that are link modes.  But some time in the future,
the remaining bits will be assigned to new capabilities.

If today's ethtool is used with a newer driver that sets bit 27 in its
supported field, ethtool can't tell whether that represents a new link
mode that should be included in ALL_ADVERTISED_MODES, or some other kind
of capability.  So it may not be able to set the driver's advertising
mask correctly.

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] ethtool: don't overwrite useful bits in advertising bitfield

[Johan Gunnarsson]

> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: den 21 augusti 2012 19:03
> To: Johan Gunnarsson
> Cc: netdev@vger.kernel.org; Mikael Starvik
> Subject: RE: [PATCH] ethtool: don't overwrite useful bits in
> advertising bitfield
> 
> [...]
> 
> <linux/ethtool.h> or ethtool-copy.h currently defines the meanings of
> bits 0-26 in the supported field.  You define ALL_ADVERTISED_MODES to
> include all of those that are link modes.  But some time in the future,
> the remaining bits will be assigned to new capabilities.
> 
> If today's ethtool is used with a newer driver that sets bit 27 in its
> supported field, ethtool can't tell whether that represents a new link
> mode that should be included in ALL_ADVERTISED_MODES, or some other
> kind
> of capability.  So it may not be able to set the driver's advertising
> mask correctly.

I see. Thanks. I'll submit new version of the patch.

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