From: Johan Gunnarsson <hidden> Date: 2012-08-14 14:23:44
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 <redacted>
---
ethtool.c | 45 +++++++++++++++++++++++++++++++--------------
1 file changed, 31 insertions(+), 14 deletions(-)
@@ -2202,6 +2214,7 @@ static int do_sset(struct cmd_context *ctx)intautoneg_wanted=-1;intphyad_wanted=-1;intxcvr_wanted=-1;+intfull_advertising_wanted=-1;intadvertising_wanted=-1;intgset_changed=0;/* did anything in GSET change? */u32wol_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);}elseif(!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:+*constructonefromthespecifiedspeedandduplex.+*/if(speed_wanted==SPEED_10&&duplex_wanted==DUPLEX_HALF)advertising_wanted=ADVERTISED_10baseT_Half;elseif(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+*unspecifiedspeedandduplex:enableall+*supportedspeedsandduplexes.+*/+ecmd.advertising=(ecmd.advertising&+~ALL_ADVERTISED_MODES)|+(ALL_ADVERTISED_MODES&ecmd.supported);}elseif(advertising_wanted>0){-ecmd.advertising=advertising_wanted;+/* Enable all requested modes */+ecmd.advertising=(ecmd.advertising&+~ALL_ADVERTISED_MODES)|+(advertising_wanted&ecmd.supported);+}elseif(full_advertising_wanted>0){+ecmd.advertising=full_advertising_wanted;}/* Try to perform the update. */
From: Ben Hutchings <hidden> Date: 2012-08-20 15:23:03
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.
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.
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.
From: Johan Gunnarsson <hidden> Date: 2012-08-21 08:39:35
-----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:
quoted
There are bits in this bitfield that we want to leave untouched
(PAUSE
quoted
and ASYM_PAUSE bits) when changing other bits (speed and duplex
bits.)
quoted
Previously, these were always overwritten to zero when running
commands
quoted
like "ethtool -s eth0 speed 10 duplex full autoneg off".
This is right in principle, but the implementation isn't quite right.
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.)
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.
quoted
+ } 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
From: Ben Hutchings <hidden> Date: 2012-08-21 15:11:54
On Tue, 2012-08-21 at 10:41 +0200, Johan Gunnarsson wrote:
quoted
-----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:
quoted
There are bits in this bitfield that we want to leave untouched
(PAUSE
quoted
and ASYM_PAUSE bits) when changing other bits (speed and duplex
bits.)
quoted
Previously, these were always overwritten to zero when running
commands
quoted
like "ethtool -s eth0 speed 10 duplex full autoneg off".
This is right in principle, but the implementation isn't quite right.
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).
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.
From: Johan Gunnarsson <hidden> Date: 2012-08-21 16:42:56
-----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:
quoted
quoted
-----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:
quoted
There are bits in this bitfield that we want to leave untouched
(PAUSE
quoted
and ASYM_PAUSE bits) when changing other bits (speed and duplex
bits.)
quoted
Previously, these were always overwritten to zero when running
commands
quoted
like "ethtool -s eth0 speed 10 duplex full autoneg off".
This is right in principle, but the implementation isn't quite
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
quoted
quoted
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).
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
quoted
quoted
mask. I think we should be consistent with the following case,
i.e.
quoted
quoted
let
the driver validate it.
How about remove "& ecmd.supported", but also warn if trying to add
an
quoted
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.
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
quoted
quoted
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.
From: Johan Gunnarsson <hidden> Date: 2012-08-24 09:47:37
-----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.