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

On Tue, 2012-08-21 at 18:45 +0200, Johan Gunnarsson wrote:
[...]

quoted

quoted

quoted

quoted

@@ -2405,19 +2421,20 @@ static int do_sset(struct cmd_context

*ctx)

quoted

quoted

quoted

 			}
 			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

quoted

quoted

quoted

+				 * supported speeds and duplexes.
+				 */
+				ecmd.advertising = (ecmd.advertising &
+					~ALL_ADVERTISED_MODES) |
+					(ALL_ADVERTISED_MODES &

ecmd.supported);

quoted

quoted

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.