All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-06  2:54 David Decotigny
  2015-01-06  2:54   ` David Decotigny
                   ` (8 more replies)
  0 siblings, 9 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev, linux-kernel, linux-api
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot@googlers.com>

History:
  v2:
    - added ethtool_ops::get_compat_flags() to be able to check user
      parameters in generic ethtool layer when a driver does not
      support a new feature.
    - in generic ethtool code, EINVAL when userspace sets a link mode
      with bits 32..47 set, unless driver explicitly indicates that it
      supports it (get_compat_flags). also print a warning once (per
      lifetime, not per-interface) if driver sets bits 32..47 of link
      mode, unless it explicitly indicated that it supports 48bit link
      modes.
    - don't update phydev->advertising too early (ie. keep original behavior)
  v1:
    initial draft

This patch series increases the width of the supported/advertising
ethtool masks from 32 bits to 48. This should allow to breathe for a
couple more years (or... months?).

It should not cause any backward compatibility issues for
now. However, if user-space passed non-0 to previously-reserved fields
to ethtool_set_settings or ethtool_set_eee, they will be rejected by
non-updated (ie. no get_compat_flags) drivers. Updated drivers will
handle the new expanded link mode masks appropriately. It is
recommended we gradually adopt the new get/set API, and provide the
associated get_compat_flags(), in order to correctly support future
link modes. See ethtool.h for details.

I updated a couple drivers (mlx4, veth, tun), and some shared code in
drivers/net (phy, mii, mdio). It might be overkill for phy/mii/mdio,
and I might have missed other shared code in drivers/net. Please let
me know.

I used the compiler on my private copy to 1/ track where the
ethtool_cmd/ethtool_eee link mode fields were used 2/ track where the
link mode bitmaps are used and updated. So I believe that there is
some sort of transitive closure for the code I updated. Maybe tools
like coccinelle or clang could allow to automate these processes (1/
would probably be easy-ish, but 2/ seems a bit more complex)? I
reverted these internal "tracking" tricks for this version to minimize
impact on uapi/ethtool.h. The main resulting visible artifact of those
tricks is the new ethtool_link_mode_mask_t typedef (aka. u64). I kept
this trivial typedef here to help future refactoring, but I'd be happy
to rename it as plain "u64" if you prefer.

I can send updates to other drivers, even though it's rather pointless
to update 1G drivers at this point for example. Please let me know,
but I'd prefer to do this in follow-up patches outside this first
patch series.


############################################
# Patch Set Summary:

David Decotigny (8):
  net: ethtool: internal compatibility flags to reject non-zero reserved
    fields
  net: ethtool: extend link mode support to 48 bits
  net: phy: extend link mode support to 48 bits
  net: mii: extend link mode support to 48 bits
  net: mdio: extend link mode support to 48 bits
  net: veth: extend link mode support to 48 bits
  net: tun: extend link mode support to 48 bits
  net: mlx4_en: extend link mode support to 48 bits

 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |  73 ++++++++-----
 drivers/net/mdio.c                              |  59 +++++-----
 drivers/net/mii.c                               |  52 +++++----
 drivers/net/phy/phy.c                           |  30 +++---
 drivers/net/phy/phy_device.c                    |   4 +-
 drivers/net/tun.c                               |   4 +-
 drivers/net/veth.c                              |   4 +-
 include/linux/ethtool.h                         |  25 ++++-
 include/linux/mdio.h                            |  15 +--
 include/linux/mii.h                             |  31 +++---
 include/linux/phy.h                             |  10 +-
 include/uapi/linux/ethtool.h                    | 137 ++++++++++++++++++++----
 net/core/ethtool.c                              |  44 ++++++++
 13 files changed, 343 insertions(+), 145 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 1/8] net: ethtool: internal compatibility flags to reject non-zero reserved fields
@ 2015-01-06  2:54   ` David Decotigny
  0 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev, linux-kernel, linux-api
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot@googlers.com>

The public ethtool API progressively shrinks "reserved" fields to
expand some other fields (eg. link mode masks). This patch allows
drivers to declare that they fully support expanded fields. When they
don't do so, the generic ethtool layer may reject (-EINVAL) userspace
requests that assign values utilizing the newly allocated areas in
these expanded fields (ie. when some reserved field receives != 0).

Signed-off-by: David Decotigny <decot@googlers.com>
---
 include/linux/ethtool.h      | 19 +++++++++++++++++--
 include/uapi/linux/ethtool.h |  6 +++++-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 653dc9c..dcb08c1 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -44,6 +44,17 @@ extern int __ethtool_get_settings(struct net_device *dev,
 				  struct ethtool_cmd *cmd);
 
 /**
+ * enum ethtool_compat_flags - bit indices used for %get_compat_flags() bitmaps
+ * @__ETHTOOL_COMPAT_PLACEHOLDER_BIT: to avoid a compiler error,
+ *                                    superseded by next patches
+ */
+enum ethtool_compat_flags {
+	__ETHTOOL_COMPAT_PLACEHOLDER_BIT,
+};
+
+#define __ETH_COMPAT_MASK(name)	(1UL << (ETHTOOL_COMPAT_ ## name ## _BIT))
+
+/**
  * enum ethtool_phys_id_state - indicator state for physical identification
  * @ETHTOOL_ID_INACTIVE: Physical ID indicator should be deactivated
  * @ETHTOOL_ID_ACTIVE: Physical ID indicator should be activated
@@ -99,6 +110,11 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
 
 /**
  * struct ethtool_ops - optional netdev operations
+ * @get_compat_flags: Internal function. Returns the internal ethtool
+ *      compatibility flags: in the absence of this method, or of
+ *      specific compatilibilty flags, the generic layer enforces
+ *      additional constraints on the user space values before
+ *      calling the callbacks below.
  * @get_settings: Get various device settings including Ethernet link
  *	settings. The @cmd parameter is expected to have been cleared
  *	before get_settings is called. Returns a negative error code or
@@ -279,7 +295,6 @@ struct ethtool_ops {
 			       const struct ethtool_tunable *, void *);
 	int	(*set_tunable)(struct net_device *,
 			       const struct ethtool_tunable *, const void *);
-
-
+	u32	(*get_compat_flags)(struct net_device *);
 };
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 5f66d9c..d063368 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -88,7 +88,8 @@
  * other than @cmd that are not described as read-only or deprecated,
  * and must ignore all fields described as read-only.
  *
- * Deprecated fields should be ignored by both users and drivers.
+ * Deprecated and reserved fields should be ignored by both users and
+ * drivers. If reserved fields must be set, store the value 0 in them.
  */
 struct ethtool_cmd {
 	__u32	cmd;
@@ -300,6 +301,9 @@ struct ethtool_eeprom {
  * @tx_lpi_timer: Time in microseconds the interface delays prior to asserting
  *	its tx lpi (after reaching 'idle' state). Effective only when eee
  *	was negotiated and tx_lpi_enabled was set.
+ *
+ * Deprecated and reserved fields should be ignored by both users and
+ * drivers. If reserved fields must be set, store the value 0 in them.
  */
 struct ethtool_eee {
 	__u32	cmd;
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 1/8] net: ethtool: internal compatibility flags to reject non-zero reserved fields
@ 2015-01-06  2:54   ` David Decotigny
  0 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot-Ypc/8FJVVoBWk0Htik3J/w@public.gmane.org>

The public ethtool API progressively shrinks "reserved" fields to
expand some other fields (eg. link mode masks). This patch allows
drivers to declare that they fully support expanded fields. When they
don't do so, the generic ethtool layer may reject (-EINVAL) userspace
requests that assign values utilizing the newly allocated areas in
these expanded fields (ie. when some reserved field receives != 0).

Signed-off-by: David Decotigny <decot-Ypc/8FJVVoBWk0Htik3J/w@public.gmane.org>
---
 include/linux/ethtool.h      | 19 +++++++++++++++++--
 include/uapi/linux/ethtool.h |  6 +++++-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 653dc9c..dcb08c1 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -44,6 +44,17 @@ extern int __ethtool_get_settings(struct net_device *dev,
 				  struct ethtool_cmd *cmd);
 
 /**
+ * enum ethtool_compat_flags - bit indices used for %get_compat_flags() bitmaps
+ * @__ETHTOOL_COMPAT_PLACEHOLDER_BIT: to avoid a compiler error,
+ *                                    superseded by next patches
+ */
+enum ethtool_compat_flags {
+	__ETHTOOL_COMPAT_PLACEHOLDER_BIT,
+};
+
+#define __ETH_COMPAT_MASK(name)	(1UL << (ETHTOOL_COMPAT_ ## name ## _BIT))
+
+/**
  * enum ethtool_phys_id_state - indicator state for physical identification
  * @ETHTOOL_ID_INACTIVE: Physical ID indicator should be deactivated
  * @ETHTOOL_ID_ACTIVE: Physical ID indicator should be activated
@@ -99,6 +110,11 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
 
 /**
  * struct ethtool_ops - optional netdev operations
+ * @get_compat_flags: Internal function. Returns the internal ethtool
+ *      compatibility flags: in the absence of this method, or of
+ *      specific compatilibilty flags, the generic layer enforces
+ *      additional constraints on the user space values before
+ *      calling the callbacks below.
  * @get_settings: Get various device settings including Ethernet link
  *	settings. The @cmd parameter is expected to have been cleared
  *	before get_settings is called. Returns a negative error code or
@@ -279,7 +295,6 @@ struct ethtool_ops {
 			       const struct ethtool_tunable *, void *);
 	int	(*set_tunable)(struct net_device *,
 			       const struct ethtool_tunable *, const void *);
-
-
+	u32	(*get_compat_flags)(struct net_device *);
 };
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 5f66d9c..d063368 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -88,7 +88,8 @@
  * other than @cmd that are not described as read-only or deprecated,
  * and must ignore all fields described as read-only.
  *
- * Deprecated fields should be ignored by both users and drivers.
+ * Deprecated and reserved fields should be ignored by both users and
+ * drivers. If reserved fields must be set, store the value 0 in them.
  */
 struct ethtool_cmd {
 	__u32	cmd;
@@ -300,6 +301,9 @@ struct ethtool_eeprom {
  * @tx_lpi_timer: Time in microseconds the interface delays prior to asserting
  *	its tx lpi (after reaching 'idle' state). Effective only when eee
  *	was negotiated and tx_lpi_enabled was set.
+ *
+ * Deprecated and reserved fields should be ignored by both users and
+ * drivers. If reserved fields must be set, store the value 0 in them.
  */
 struct ethtool_eee {
 	__u32	cmd;
-- 
2.2.0.rc0.207.ga3a616c

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 2/8] net: ethtool: extend link mode support to 48 bits
  2015-01-06  2:54 [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits David Decotigny
  2015-01-06  2:54   ` David Decotigny
@ 2015-01-06  2:54 ` David Decotigny
  2015-01-06  2:54   ` David Decotigny
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev, linux-kernel, linux-api
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot@googlers.com>

This patch also ensures that requests from userspace asking to
advertise >= 32b link mode masks will be rejected, unless the driver
explicitly supports this.

Signed-off-by: David Decotigny <decot@googlers.com>
---
 include/linux/ethtool.h      |  12 +++-
 include/uapi/linux/ethtool.h | 131 ++++++++++++++++++++++++++++++++++++-------
 net/core/ethtool.c           |  44 +++++++++++++++
 3 files changed, 164 insertions(+), 23 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index dcb08c1..9baa80f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -45,15 +45,21 @@ extern int __ethtool_get_settings(struct net_device *dev,
 
 /**
  * enum ethtool_compat_flags - bit indices used for %get_compat_flags() bitmaps
- * @__ETHTOOL_COMPAT_PLACEHOLDER_BIT: to avoid a compiler error,
- *                                    superseded by next patches
+ * @ETHTOOL_COMPAT_SUPPORT_LINK_MODE_48b_BIT: when set, the driver
+ *      handles 48-bit link mode requests from userspace. In its
+ *      absence, the generic %ethtool_set_settings/%ethtool_set_eee
+ *      ioctl handlers will reject the request if user passed an
+ *      advertising link mode with any of the bits 32..47 set.
  */
 enum ethtool_compat_flags {
-	__ETHTOOL_COMPAT_PLACEHOLDER_BIT,
+	ETHTOOL_COMPAT_SUPPORT_LINK_MODE_48b_BIT,
 };
 
 #define __ETH_COMPAT_MASK(name)	(1UL << (ETHTOOL_COMPAT_ ## name ## _BIT))
 
+#define ETH_COMPAT_SUPPORT_LINK_MODE_48b	\
+	__ETH_COMPAT_MASK(SUPPORT_LINK_MODE_48b)
+
 /**
  * enum ethtool_phys_id_state - indicator state for physical identification
  * @ETHTOOL_ID_INACTIVE: Physical ID indicator should be deactivated
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index d063368..ab7c11d 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -23,14 +23,16 @@
 /**
  * struct ethtool_cmd - link control and status
  * @cmd: Command number = %ETHTOOL_GSET or %ETHTOOL_SSET
- * @supported: Bitmask of %SUPPORTED_* flags for the link modes,
- *	physical connectors and other link features for which the
- *	interface supports autonegotiation or auto-detection.
- *	Read-only.
- * @advertising: Bitmask of %ADVERTISED_* flags for the link modes,
- *	physical connectors and other link features that are
- *	advertised through autonegotiation or enabled for
- *	auto-detection.
+ * @supported: Low bits of bitmask of %SUPPORTED_* flags for the link
+ *	modes, physical connectors and other link features for which
+ *	the interface supports autonegotiation or auto-detection.
+ *	Read-only. Please do not access this field directly, use the
+ *	%ethtool_cmd_supported_* family of functions instead.
+ * @advertising: Low bits of bitmask of %ADVERTISED_* flags for the
+ *	link modes, physical connectors and other link features that
+ *	are advertised through autonegotiation or enabled for
+ *	auto-detection. Please do not access this field directly, use
+ *	the %ethtool_cmd_advertising_* family of functions instead.
  * @speed: Low bits of the speed
  * @duplex: Duplex mode; one of %DUPLEX_*
  * @port: Physical connector type; one of %PORT_*
@@ -56,10 +58,22 @@
  *	yield %ETH_TP_MDI_INVALID and writes may be ignored or rejected.
  *	When written successfully, the link should be renegotiated if
  *	necessary.
- * @lp_advertising: Bitmask of %ADVERTISED_* flags for the link modes
- *	and other link features that the link partner advertised
- *	through autonegotiation; 0 if unknown or not applicable.
- *	Read-only.
+ * @lp_advertising: Low bits of bitmask of %ADVERTISED_* flags for the
+ *      link modes and other link features that the link partner
+ *      advertised through autonegotiation; 0 if unknown or not
+ *      applicable.  Read-only. Please do not access this field
+ *      directly, use the %ethtool_cmd_lp_advertising_* family of
+ *      functions instead.
+ * @supported_hi: High bits of bitmask of %SUPPORTED_* flags. See
+ *      %supported. Read-only. Please do not access this field directly,
+ *      use the %ethtool_cmd_supported_* family of functions instead.
+ * @advertising_hi: High bits of bitmask of %ADVERTISING_* flags. See
+ *      %advertising. Please do not access this field directly, use the
+ *      %ethtool_cmd_advertising_* family of functions instead.
+ * @lp_advertising_hi: High bits of bitmask of %ADVERTISING_* flags.
+ *      See %lp_advertising. Read-only. Please do not access this
+ *      field directly, use the %ethtool_cmd_lp_advertising_* family
+ *      of functions instead.
  *
  * The link speed in Mbps is split between @speed and @speed_hi.  Use
  * the ethtool_cmd_speed() and ethtool_cmd_speed_set() functions to
@@ -108,7 +122,10 @@ struct ethtool_cmd {
 	__u8	eth_tp_mdix;
 	__u8	eth_tp_mdix_ctrl;
 	__u32	lp_advertising;
-	__u32	reserved[2];
+	__u16	supported_hi;
+	__u16	advertising_hi;
+	__u16	lp_advertising_hi;
+	__u16	reserved;
 };
 
 static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep,
@@ -124,6 +141,45 @@ static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
 	return (ep->speed_hi << 16) | ep->speed;
 }
 
+/**
+ * ETHTOOL_MAKE_LINK_MODE_ACCESSORS - create the link_mode accessors
+ * Macro to generate the %ethtool_cmd_supported_*,
+ * %ethtool_cmd_advertising_*, %ethtool_cmd_lp_advertising_*,
+ * %ethtool_eee_supported_*, %ethtool_eee_advertised_*,
+ * %ethtool_eee_lp_advertised_* families of functions.
+ *
+ *  @struct_name: either %ethtool_cmd or %ethtool_eee
+ *  @field_name: name of the fields in %struct_name to
+ *      access. supported/advertising/lp_advertising for ethtool_cmd,
+ *      supported/advertised/lp_advertised for ethtool_eee
+ *
+ * Generates the following static functions:
+ *  ethtool_cmd_supported(const struct ethtool_cmd*): returns
+ *      the 64b value of %supported fields (the upper bits 63..48 are 0)
+ *  ethtool_cmd_supported_set(struct ethtool_cmd*,
+ *      ethtool_link_mode_mask_t value): set the %supported fields to
+ *      given %value (only the lower 48 bits used, upper bits 63..48
+ *      ignored)
+ *
+ * Same doc for all the other functions.
+ */
+#define ETHTOOL_MAKE_LINK_MODE_ACCESSORS(struct_name, field_name)	\
+	static inline ethtool_link_mode_mask_t				\
+	struct_name ## _ ## field_name(const struct struct_name *cmd)	\
+	{ ethtool_link_mode_mask_t r = cmd->field_name;			\
+	  r |= ((__u64)cmd->field_name ## _hi) << 32; return r; }	\
+	static inline void						\
+	struct_name ## _ ## field_name ## _set(struct struct_name *cmd,	\
+					       ethtool_link_mode_mask_t mask) \
+	{ cmd->field_name = mask & 0xffffffff;				\
+	  cmd->field_name ## _hi = (mask >> 32) & 0xffff; }
+
+typedef __u64 ethtool_link_mode_mask_t;
+
+ETHTOOL_MAKE_LINK_MODE_ACCESSORS(ethtool_cmd, supported);
+ETHTOOL_MAKE_LINK_MODE_ACCESSORS(ethtool_cmd, advertising);
+ETHTOOL_MAKE_LINK_MODE_ACCESSORS(ethtool_cmd, lp_advertising);
+
 /* Device supports clause 22 register access to PHY or peripherals
  * using the interface defined in <linux/mii.h>.  This should not be
  * set if there are known to be no such peripherals present or if
@@ -288,12 +344,18 @@ struct ethtool_eeprom {
 /**
  * struct ethtool_eee - Energy Efficient Ethernet information
  * @cmd: ETHTOOL_{G,S}EEE
- * @supported: Mask of %SUPPORTED_* flags for the speed/duplex combinations
- *	for which there is EEE support.
- * @advertised: Mask of %ADVERTISED_* flags for the speed/duplex combinations
- *	advertised as eee capable.
- * @lp_advertised: Mask of %ADVERTISED_* flags for the speed/duplex
- *	combinations advertised by the link partner as eee capable.
+ * @supported: Low bits of mask of %SUPPORTED_* flags for the
+ *	speed/duplex combinations for which there is EEE
+ *	support. Please do not access this field directly, use the
+ *	%ethtool_eee_supported_* family of functions instead.
+ * @advertised: Low bits of mask of %ADVERTISED_* flags for the
+ *	speed/duplex combinations advertised as eee capable. Please do
+ *	not access this field directly, use the
+ *	%ethtool_eee_advertised_* family of functions instead.
+ * @lp_advertised: Low bits of mask of %ADVERTISED_* flags for the
+ *	speed/duplex combinations advertised by the link partner as
+ *	eee capable.  Please do not access this field directly, use
+ *	the %ethtool_eee_lp_advertised_* family of functions instead.
  * @eee_active: Result of the eee auto negotiation.
  * @eee_enabled: EEE configured mode (enabled/disabled).
  * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
@@ -301,6 +363,16 @@ struct ethtool_eeprom {
  * @tx_lpi_timer: Time in microseconds the interface delays prior to asserting
  *	its tx lpi (after reaching 'idle' state). Effective only when eee
  *	was negotiated and tx_lpi_enabled was set.
+ * @supported_hi: High bits of mask of %SUPPORTED_* flags. See
+ *      %supported. Please do not access this field directly, use the
+ *      %ethtool_eee_supported_* family of functions instead.
+ * @advertised_hi: High bits of mask of %ADVERTISING_* flags. See
+ *      %advertising. Please do not access this field directly, use
+ *      the %ethtool_eee_advertising_* family of functions instead.
+ * @lp_advertised_hi: High bits of mask of %ADVERTISING_* flags.
+ *      See %lp_advertising. Please do not access this field directly,
+ *      use the %ethtool_eee_lp_advertising_* family of functions
+ *      instead.
  *
  * Deprecated and reserved fields should be ignored by both users and
  * drivers. If reserved fields must be set, store the value 0 in them.
@@ -314,9 +386,17 @@ struct ethtool_eee {
 	__u32	eee_enabled;
 	__u32	tx_lpi_enabled;
 	__u32	tx_lpi_timer;
-	__u32	reserved[2];
+	__u16	supported_hi;
+	__u16	advertised_hi;
+	__u16	lp_advertised_hi;
+	__u16	reserved;
 };
 
+ETHTOOL_MAKE_LINK_MODE_ACCESSORS(ethtool_eee, supported);
+ETHTOOL_MAKE_LINK_MODE_ACCESSORS(ethtool_eee, advertised);
+ETHTOOL_MAKE_LINK_MODE_ACCESSORS(ethtool_eee, lp_advertised);
+
+
 /**
  * struct ethtool_modinfo - plugin module eeprom information
  * @cmd: %ETHTOOL_GMODULEINFO
@@ -1196,6 +1276,11 @@ enum ethtool_sfeatures_retval_bits {
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
 
+/*
+ * Do not use the following macros directly to update
+ * ethtool_cmd::supported, ethtool_eee::supported. Please use
+ * ethtool_(cmd|eee)_supported(|_set) instead.
+ */
 #define SUPPORTED_10baseT_Half		(1 << 0)
 #define SUPPORTED_10baseT_Full		(1 << 1)
 #define SUPPORTED_100baseT_Half		(1 << 2)
@@ -1228,6 +1313,12 @@ enum ethtool_sfeatures_retval_bits {
 #define SUPPORTED_56000baseSR4_Full	(1 << 29)
 #define SUPPORTED_56000baseLR4_Full	(1 << 30)
 
+/*
+ * Do not use the following macros directly to update
+ * ethtool_cmd::advertising, ethtool_cmd::lp_advertising,
+ * ethtool_eee::advertised, ethtool_eee::lp_advertised. Please use
+ * ethtool_(cmd|eee)_*(|_set).
+ */
 #define ADVERTISED_10baseT_Half		(1 << 0)
 #define ADVERTISED_10baseT_Full		(1 << 1)
 #define ADVERTISED_100baseT_Half	(1 << 2)
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 550892c..462a8f4 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -362,6 +362,19 @@ static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
 	if (err < 0)
 		return err;
 
+	/* Log a warning if driver reports high link mode bits when
+	 * it's not officially supporting them
+	 */
+	if (!dev->ethtool_ops->get_compat_flags ||
+	    !(dev->ethtool_ops->get_compat_flags(dev)
+	      & ETH_COMPAT_SUPPORT_LINK_MODE_48b)) {
+		WARN_ONCE((0 != cmd.supported_hi) ||
+			  (0 != cmd.advertising_hi) ||
+			  (0 != cmd.lp_advertising_hi),
+			  "%s: using ethtool link mode high bits",
+			  netdev_name(dev));
+	}
+
 	if (copy_to_user(useraddr, &cmd, sizeof(cmd)))
 		return -EFAULT;
 	return 0;
@@ -377,6 +390,15 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
 	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
 		return -EFAULT;
 
+	/* Reject the high advertising bits if driver doesn't support
+	 * them
+	 */
+	if (cmd.advertising_hi &&
+	    (!dev->ethtool_ops->get_compat_flags ||
+	     !(dev->ethtool_ops->get_compat_flags(dev)
+	       & ETH_COMPAT_SUPPORT_LINK_MODE_48b)))
+		return -EINVAL;
+
 	return dev->ethtool_ops->set_settings(dev, &cmd);
 }
 
@@ -955,6 +977,19 @@ static int ethtool_get_eee(struct net_device *dev, char __user *useraddr)
 	if (rc)
 		return rc;
 
+	/* Log a warning if driver reports high link mode bits when
+	 * it's not officially supporting them
+	 */
+	if (!dev->ethtool_ops->get_compat_flags ||
+	    !(dev->ethtool_ops->get_compat_flags(dev)
+	      & ETH_COMPAT_SUPPORT_LINK_MODE_48b)) {
+		WARN_ONCE((0 != edata.supported_hi) ||
+			  (0 != edata.advertised_hi) ||
+			  (0 != edata.lp_advertised_hi),
+			  "%s: using ethtool EEE link mode high bits",
+			  netdev_name(dev));
+	}
+
 	if (copy_to_user(useraddr, &edata, sizeof(edata)))
 		return -EFAULT;
 
@@ -971,6 +1006,15 @@ static int ethtool_set_eee(struct net_device *dev, char __user *useraddr)
 	if (copy_from_user(&edata, useraddr, sizeof(edata)))
 		return -EFAULT;
 
+	/* Reject the high advertising bits if driver doesn't support
+	 * them
+	 */
+	if (edata.advertised_hi &&
+	    (!dev->ethtool_ops->get_compat_flags ||
+	     !(dev->ethtool_ops->get_compat_flags(dev)
+		     & ETH_COMPAT_SUPPORT_LINK_MODE_48b)))
+		return -EINVAL;
+
 	return dev->ethtool_ops->set_eee(dev, &edata);
 }
 
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 3/8] net: phy: extend link mode support to 48 bits
@ 2015-01-06  2:54   ` David Decotigny
  0 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev, linux-kernel, linux-api
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot@googlers.com>

Signed-off-by: David Decotigny <decot@googlers.com>
---
 drivers/net/phy/phy.c        | 30 ++++++++++++++++--------------
 drivers/net/phy/phy_device.c |  4 ++--
 include/linux/phy.h          | 10 +++++-----
 3 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 767cd11..77d4eb9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -132,7 +132,7 @@ static inline int phy_aneg_done(struct phy_device *phydev)
 struct phy_setting {
 	int speed;
 	int duplex;
-	u32 setting;
+	ethtool_link_mode_mask_t setting;
 };
 
 /* A mapping of all SUPPORTED settings to speed/duplex */
@@ -227,7 +227,8 @@ static inline unsigned int phy_find_setting(int speed, int duplex)
  *   the mask in features.  Returns the index of the last setting
  *   if nothing else matches.
  */
-static inline unsigned int phy_find_valid(unsigned int idx, u32 features)
+static inline unsigned int phy_find_valid(unsigned int idx,
+					  ethtool_link_mode_mask_t features)
 {
 	while (idx < MAX_NUM_SETTINGS && !(settings[idx].setting & features))
 		idx++;
@@ -245,7 +246,7 @@ static inline unsigned int phy_find_valid(unsigned int idx, u32 features)
  */
 static void phy_sanitize_settings(struct phy_device *phydev)
 {
-	u32 features = phydev->supported;
+	ethtool_link_mode_mask_t features = phydev->supported;
 	unsigned int idx;
 
 	/* Sanitize settings based on PHY capabilities */
@@ -274,18 +275,19 @@ static void phy_sanitize_settings(struct phy_device *phydev)
 int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 {
 	u32 speed = ethtool_cmd_speed(cmd);
+	ethtool_link_mode_mask_t eth_adv = ethtool_cmd_advertising(cmd);
 
 	if (cmd->phy_address != phydev->addr)
 		return -EINVAL;
 
 	/* We make sure that we don't pass unsupported values in to the PHY */
-	cmd->advertising &= phydev->supported;
+	eth_adv &= phydev->supported;
 
 	/* Verify the settings we care about. */
 	if (cmd->autoneg != AUTONEG_ENABLE && cmd->autoneg != AUTONEG_DISABLE)
 		return -EINVAL;
 
-	if (cmd->autoneg == AUTONEG_ENABLE && cmd->advertising == 0)
+	if (cmd->autoneg == AUTONEG_ENABLE && eth_adv == 0)
 		return -EINVAL;
 
 	if (cmd->autoneg == AUTONEG_DISABLE &&
@@ -300,7 +302,7 @@ int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 
 	phydev->speed = speed;
 
-	phydev->advertising = cmd->advertising;
+	phydev->advertising = eth_adv;
 
 	if (AUTONEG_ENABLE == cmd->autoneg)
 		phydev->advertising |= ADVERTISED_Autoneg;
@@ -318,10 +320,10 @@ EXPORT_SYMBOL(phy_ethtool_sset);
 
 int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 {
-	cmd->supported = phydev->supported;
+	ethtool_cmd_supported_set(cmd, phydev->supported);
 
-	cmd->advertising = phydev->advertising;
-	cmd->lp_advertising = phydev->lp_advertising;
+	ethtool_cmd_advertising_set(cmd, phydev->advertising);
+	ethtool_cmd_lp_advertising_set(cmd, phydev->lp_advertising);
 
 	ethtool_cmd_speed_set(cmd, phydev->speed);
 	cmd->duplex = phydev->duplex;
@@ -1040,7 +1042,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 	    (phydev->interface == PHY_INTERFACE_MODE_RGMII) ||
 	     phy_is_internal(phydev))) {
 		int eee_lp, eee_cap, eee_adv;
-		u32 lp, cap, adv;
+		ethtool_link_mode_mask_t cap, adv, lp;
 		int status;
 		unsigned int idx;
 
@@ -1132,21 +1134,21 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
 				    MDIO_MMD_PCS, phydev->addr);
 	if (val < 0)
 		return val;
-	data->supported = mmd_eee_cap_to_ethtool_sup_t(val);
+	ethtool_eee_supported_set(data, mmd_eee_cap_to_ethtool_sup_t(val));
 
 	/* Get advertisement EEE */
 	val = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
 				    MDIO_MMD_AN, phydev->addr);
 	if (val < 0)
 		return val;
-	data->advertised = mmd_eee_adv_to_ethtool_adv_t(val);
+	ethtool_eee_advertised_set(data, mmd_eee_adv_to_ethtool_adv_t(val));
 
 	/* Get LP advertisement EEE */
 	val = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_LPABLE,
 				    MDIO_MMD_AN, phydev->addr);
 	if (val < 0)
 		return val;
-	data->lp_advertised = mmd_eee_adv_to_ethtool_adv_t(val);
+	ethtool_eee_lp_advertised_set(data, mmd_eee_adv_to_ethtool_adv_t(val));
 
 	return 0;
 }
@@ -1161,7 +1163,7 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
  */
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 {
-	int val = ethtool_adv_to_mmd_eee_adv_t(data->advertised);
+	int val = ethtool_adv_to_mmd_eee_adv_t(ethtool_eee_advertised(data));
 
 	phy_write_mmd_indirect(phydev, MDIO_AN_EEE_ADV, MDIO_MMD_AN,
 			       phydev->addr, val);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3fc91e8..4391dc7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -734,7 +734,7 @@ EXPORT_SYMBOL(phy_resume);
  */
 static int genphy_config_advert(struct phy_device *phydev)
 {
-	u32 advertise;
+	ethtool_link_mode_mask_t advertise;
 	int oldadv, adv, bmsr;
 	int err, changed = 0;
 
@@ -1086,7 +1086,7 @@ EXPORT_SYMBOL(genphy_soft_reset);
 int genphy_config_init(struct phy_device *phydev)
 {
 	int val;
-	u32 features;
+	ethtool_link_mode_mask_t features;
 
 	features = (SUPPORTED_TP | SUPPORTED_MII
 			| SUPPORTED_AUI | SUPPORTED_FIBRE |
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 22af8f8..aabf808 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -390,10 +390,10 @@ struct phy_device {
 	u32 interrupts;
 
 	/* Union of PHY and Attached devices' supported modes */
-	/* See mii.h for more info */
-	u32 supported;
-	u32 advertising;
-	u32 lp_advertising;
+	/* See ethtool.h for more info */
+	ethtool_link_mode_mask_t supported;
+	ethtool_link_mode_mask_t advertising;
+	ethtool_link_mode_mask_t lp_advertising;
 
 	int autoneg;
 
@@ -447,7 +447,7 @@ struct phy_driver {
 	u32 phy_id;
 	char *name;
 	unsigned int phy_id_mask;
-	u32 features;
+	ethtool_link_mode_mask_t features;
 	u32 flags;
 	const void *driver_data;
 
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 3/8] net: phy: extend link mode support to 48 bits
@ 2015-01-06  2:54   ` David Decotigny
  0 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot-Ypc/8FJVVoBWk0Htik3J/w@public.gmane.org>

Signed-off-by: David Decotigny <decot-Ypc/8FJVVoBWk0Htik3J/w@public.gmane.org>
---
 drivers/net/phy/phy.c        | 30 ++++++++++++++++--------------
 drivers/net/phy/phy_device.c |  4 ++--
 include/linux/phy.h          | 10 +++++-----
 3 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 767cd11..77d4eb9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -132,7 +132,7 @@ static inline int phy_aneg_done(struct phy_device *phydev)
 struct phy_setting {
 	int speed;
 	int duplex;
-	u32 setting;
+	ethtool_link_mode_mask_t setting;
 };
 
 /* A mapping of all SUPPORTED settings to speed/duplex */
@@ -227,7 +227,8 @@ static inline unsigned int phy_find_setting(int speed, int duplex)
  *   the mask in features.  Returns the index of the last setting
  *   if nothing else matches.
  */
-static inline unsigned int phy_find_valid(unsigned int idx, u32 features)
+static inline unsigned int phy_find_valid(unsigned int idx,
+					  ethtool_link_mode_mask_t features)
 {
 	while (idx < MAX_NUM_SETTINGS && !(settings[idx].setting & features))
 		idx++;
@@ -245,7 +246,7 @@ static inline unsigned int phy_find_valid(unsigned int idx, u32 features)
  */
 static void phy_sanitize_settings(struct phy_device *phydev)
 {
-	u32 features = phydev->supported;
+	ethtool_link_mode_mask_t features = phydev->supported;
 	unsigned int idx;
 
 	/* Sanitize settings based on PHY capabilities */
@@ -274,18 +275,19 @@ static void phy_sanitize_settings(struct phy_device *phydev)
 int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 {
 	u32 speed = ethtool_cmd_speed(cmd);
+	ethtool_link_mode_mask_t eth_adv = ethtool_cmd_advertising(cmd);
 
 	if (cmd->phy_address != phydev->addr)
 		return -EINVAL;
 
 	/* We make sure that we don't pass unsupported values in to the PHY */
-	cmd->advertising &= phydev->supported;
+	eth_adv &= phydev->supported;
 
 	/* Verify the settings we care about. */
 	if (cmd->autoneg != AUTONEG_ENABLE && cmd->autoneg != AUTONEG_DISABLE)
 		return -EINVAL;
 
-	if (cmd->autoneg == AUTONEG_ENABLE && cmd->advertising == 0)
+	if (cmd->autoneg == AUTONEG_ENABLE && eth_adv == 0)
 		return -EINVAL;
 
 	if (cmd->autoneg == AUTONEG_DISABLE &&
@@ -300,7 +302,7 @@ int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 
 	phydev->speed = speed;
 
-	phydev->advertising = cmd->advertising;
+	phydev->advertising = eth_adv;
 
 	if (AUTONEG_ENABLE == cmd->autoneg)
 		phydev->advertising |= ADVERTISED_Autoneg;
@@ -318,10 +320,10 @@ EXPORT_SYMBOL(phy_ethtool_sset);
 
 int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 {
-	cmd->supported = phydev->supported;
+	ethtool_cmd_supported_set(cmd, phydev->supported);
 
-	cmd->advertising = phydev->advertising;
-	cmd->lp_advertising = phydev->lp_advertising;
+	ethtool_cmd_advertising_set(cmd, phydev->advertising);
+	ethtool_cmd_lp_advertising_set(cmd, phydev->lp_advertising);
 
 	ethtool_cmd_speed_set(cmd, phydev->speed);
 	cmd->duplex = phydev->duplex;
@@ -1040,7 +1042,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 	    (phydev->interface == PHY_INTERFACE_MODE_RGMII) ||
 	     phy_is_internal(phydev))) {
 		int eee_lp, eee_cap, eee_adv;
-		u32 lp, cap, adv;
+		ethtool_link_mode_mask_t cap, adv, lp;
 		int status;
 		unsigned int idx;
 
@@ -1132,21 +1134,21 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
 				    MDIO_MMD_PCS, phydev->addr);
 	if (val < 0)
 		return val;
-	data->supported = mmd_eee_cap_to_ethtool_sup_t(val);
+	ethtool_eee_supported_set(data, mmd_eee_cap_to_ethtool_sup_t(val));
 
 	/* Get advertisement EEE */
 	val = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
 				    MDIO_MMD_AN, phydev->addr);
 	if (val < 0)
 		return val;
-	data->advertised = mmd_eee_adv_to_ethtool_adv_t(val);
+	ethtool_eee_advertised_set(data, mmd_eee_adv_to_ethtool_adv_t(val));
 
 	/* Get LP advertisement EEE */
 	val = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_LPABLE,
 				    MDIO_MMD_AN, phydev->addr);
 	if (val < 0)
 		return val;
-	data->lp_advertised = mmd_eee_adv_to_ethtool_adv_t(val);
+	ethtool_eee_lp_advertised_set(data, mmd_eee_adv_to_ethtool_adv_t(val));
 
 	return 0;
 }
@@ -1161,7 +1163,7 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
  */
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 {
-	int val = ethtool_adv_to_mmd_eee_adv_t(data->advertised);
+	int val = ethtool_adv_to_mmd_eee_adv_t(ethtool_eee_advertised(data));
 
 	phy_write_mmd_indirect(phydev, MDIO_AN_EEE_ADV, MDIO_MMD_AN,
 			       phydev->addr, val);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3fc91e8..4391dc7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -734,7 +734,7 @@ EXPORT_SYMBOL(phy_resume);
  */
 static int genphy_config_advert(struct phy_device *phydev)
 {
-	u32 advertise;
+	ethtool_link_mode_mask_t advertise;
 	int oldadv, adv, bmsr;
 	int err, changed = 0;
 
@@ -1086,7 +1086,7 @@ EXPORT_SYMBOL(genphy_soft_reset);
 int genphy_config_init(struct phy_device *phydev)
 {
 	int val;
-	u32 features;
+	ethtool_link_mode_mask_t features;
 
 	features = (SUPPORTED_TP | SUPPORTED_MII
 			| SUPPORTED_AUI | SUPPORTED_FIBRE |
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 22af8f8..aabf808 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -390,10 +390,10 @@ struct phy_device {
 	u32 interrupts;
 
 	/* Union of PHY and Attached devices' supported modes */
-	/* See mii.h for more info */
-	u32 supported;
-	u32 advertising;
-	u32 lp_advertising;
+	/* See ethtool.h for more info */
+	ethtool_link_mode_mask_t supported;
+	ethtool_link_mode_mask_t advertising;
+	ethtool_link_mode_mask_t lp_advertising;
 
 	int autoneg;
 
@@ -447,7 +447,7 @@ struct phy_driver {
 	u32 phy_id;
 	char *name;
 	unsigned int phy_id_mask;
-	u32 features;
+	ethtool_link_mode_mask_t features;
 	u32 flags;
 	const void *driver_data;
 
-- 
2.2.0.rc0.207.ga3a616c

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 4/8] net: mii: extend link mode support to 48 bits
  2015-01-06  2:54 [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits David Decotigny
                   ` (2 preceding siblings ...)
  2015-01-06  2:54   ` David Decotigny
@ 2015-01-06  2:54 ` David Decotigny
  2015-01-06  2:54 ` [PATCH net-next v2 5/8] net: mdio: " David Decotigny
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev, linux-kernel, linux-api
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot@googlers.com>

Signed-off-by: David Decotigny <decot@googlers.com>
---
 drivers/net/mii.c   | 52 +++++++++++++++++++++++++++++-----------------------
 include/linux/mii.h | 31 ++++++++++++++++---------------
 2 files changed, 45 insertions(+), 38 deletions(-)

diff --git a/drivers/net/mii.c b/drivers/net/mii.c
index 4a99c39..2be59ba 100644
--- a/drivers/net/mii.c
+++ b/drivers/net/mii.c
@@ -33,7 +33,7 @@
 #include <linux/ethtool.h>
 #include <linux/mii.h>
 
-static u32 mii_get_an(struct mii_if_info *mii, u16 addr)
+static ethtool_link_mode_mask_t mii_get_an(struct mii_if_info *mii, u16 addr)
 {
 	int advert;
 
@@ -56,14 +56,15 @@ int mii_ethtool_gset(struct mii_if_info *mii, struct ethtool_cmd *ecmd)
 {
 	struct net_device *dev = mii->dev;
 	u16 bmcr, bmsr, ctrl1000 = 0, stat1000 = 0;
-	u32 nego;
+	ethtool_link_mode_mask_t supported_link_modes, advertising_link_modes;
+	ethtool_link_mode_mask_t nego;
 
-	ecmd->supported =
+	supported_link_modes =
 	    (SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full |
 	     SUPPORTED_100baseT_Half | SUPPORTED_100baseT_Full |
 	     SUPPORTED_Autoneg | SUPPORTED_TP | SUPPORTED_MII);
 	if (mii->supports_gmii)
-		ecmd->supported |= SUPPORTED_1000baseT_Half |
+		supported_link_modes |= SUPPORTED_1000baseT_Half |
 			SUPPORTED_1000baseT_Full;
 
 	/* only supports twisted-pair */
@@ -76,7 +77,7 @@ int mii_ethtool_gset(struct mii_if_info *mii, struct ethtool_cmd *ecmd)
 	ecmd->phy_address = mii->phy_id;
 	ecmd->mdio_support = ETH_MDIO_SUPPORTS_C22;
 
-	ecmd->advertising = ADVERTISED_TP | ADVERTISED_MII;
+	advertising_link_modes = ADVERTISED_TP | ADVERTISED_MII;
 
 	bmcr = mii->mdio_read(dev, mii->phy_id, MII_BMCR);
 	bmsr = mii->mdio_read(dev, mii->phy_id, MII_BMSR);
@@ -85,23 +86,25 @@ int mii_ethtool_gset(struct mii_if_info *mii, struct ethtool_cmd *ecmd)
 		stat1000 = mii->mdio_read(dev, mii->phy_id, MII_STAT1000);
 	}
 	if (bmcr & BMCR_ANENABLE) {
-		ecmd->advertising |= ADVERTISED_Autoneg;
+		ethtool_link_mode_mask_t lp_adv;
+
+		advertising_link_modes |= ADVERTISED_Autoneg;
 		ecmd->autoneg = AUTONEG_ENABLE;
 
-		ecmd->advertising |= mii_get_an(mii, MII_ADVERTISE);
+		advertising_link_modes |= mii_get_an(mii, MII_ADVERTISE);
 		if (mii->supports_gmii)
-			ecmd->advertising |=
+			advertising_link_modes |=
 					mii_ctrl1000_to_ethtool_adv_t(ctrl1000);
 
 		if (bmsr & BMSR_ANEGCOMPLETE) {
-			ecmd->lp_advertising = mii_get_an(mii, MII_LPA);
-			ecmd->lp_advertising |=
-					mii_stat1000_to_ethtool_lpa_t(stat1000);
+			lp_adv = mii_get_an(mii, MII_LPA);
+			lp_adv |= mii_stat1000_to_ethtool_lpa_t(stat1000);
 		} else {
-			ecmd->lp_advertising = 0;
+			lp_adv = 0;
 		}
 
-		nego = ecmd->advertising & ecmd->lp_advertising;
+		ethtool_cmd_lp_advertising_set(ecmd, lp_adv);
+		nego = advertising_link_modes & lp_adv;
 
 		if (nego & (ADVERTISED_1000baseT_Full |
 			    ADVERTISED_1000baseT_Half)) {
@@ -128,6 +131,8 @@ int mii_ethtool_gset(struct mii_if_info *mii, struct ethtool_cmd *ecmd)
 	}
 
 	mii->full_duplex = ecmd->duplex;
+	ethtool_cmd_supported_set(ecmd, supported_link_modes);
+	ethtool_cmd_advertising_set(ecmd, advertising_link_modes);
 
 	/* ignore maxtxpkt, maxrxpkt for now */
 
@@ -168,13 +173,15 @@ int mii_ethtool_sset(struct mii_if_info *mii, struct ethtool_cmd *ecmd)
 	if (ecmd->autoneg == AUTONEG_ENABLE) {
 		u32 bmcr, advert, tmp;
 		u32 advert2 = 0, tmp2 = 0;
-
-		if ((ecmd->advertising & (ADVERTISED_10baseT_Half |
-					  ADVERTISED_10baseT_Full |
-					  ADVERTISED_100baseT_Half |
-					  ADVERTISED_100baseT_Full |
-					  ADVERTISED_1000baseT_Half |
-					  ADVERTISED_1000baseT_Full)) == 0)
+		ethtool_link_mode_mask_t ethtool_adv;
+
+		ethtool_adv = ethtool_cmd_advertising(ecmd);
+		if ((ethtool_adv & (ADVERTISED_10baseT_Half |
+				    ADVERTISED_10baseT_Full |
+				    ADVERTISED_100baseT_Half |
+				    ADVERTISED_100baseT_Full |
+				    ADVERTISED_1000baseT_Half |
+				    ADVERTISED_1000baseT_Full)) == 0)
 			return -EINVAL;
 
 		/* advertise only what has been requested */
@@ -184,11 +191,10 @@ int mii_ethtool_sset(struct mii_if_info *mii, struct ethtool_cmd *ecmd)
 			advert2 = mii->mdio_read(dev, mii->phy_id, MII_CTRL1000);
 			tmp2 = advert2 & ~(ADVERTISE_1000HALF | ADVERTISE_1000FULL);
 		}
-		tmp |= ethtool_adv_to_mii_adv_t(ecmd->advertising);
+		tmp |= ethtool_adv_to_mii_adv_t(ethtool_adv);
 
 		if (mii->supports_gmii)
-			tmp2 |=
-			      ethtool_adv_to_mii_ctrl1000_t(ecmd->advertising);
+			tmp2 |= ethtool_adv_to_mii_ctrl1000_t(ethtool_adv);
 		if (advert != tmp) {
 			mii->mdio_write(dev, mii->phy_id, MII_ADVERTISE, tmp);
 			mii->advertising = tmp;
diff --git a/include/linux/mii.h b/include/linux/mii.h
index 47492c9..6f5336c 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -106,7 +106,7 @@ static inline unsigned int mii_duplex (unsigned int duplex_lock,
  * settings to phy autonegotiation advertisements for the
  * MII_ADVERTISE register.
  */
-static inline u32 ethtool_adv_to_mii_adv_t(u32 ethadv)
+static inline u32 ethtool_adv_to_mii_adv_t(ethtool_link_mode_mask_t ethadv)
 {
 	u32 result = 0;
 
@@ -133,9 +133,9 @@ static inline u32 ethtool_adv_to_mii_adv_t(u32 ethadv)
  * A small helper function that translates MII_ADVERTISE bits
  * to ethtool advertisement settings.
  */
-static inline u32 mii_adv_to_ethtool_adv_t(u32 adv)
+static inline ethtool_link_mode_mask_t mii_adv_to_ethtool_adv_t(u32 adv)
 {
-	u32 result = 0;
+	ethtool_link_mode_mask_t result = 0;
 
 	if (adv & ADVERTISE_10HALF)
 		result |= ADVERTISED_10baseT_Half;
@@ -161,7 +161,8 @@ static inline u32 mii_adv_to_ethtool_adv_t(u32 adv)
  * settings to phy autonegotiation advertisements for the
  * MII_CTRL1000 register when in 1000T mode.
  */
-static inline u32 ethtool_adv_to_mii_ctrl1000_t(u32 ethadv)
+static inline u32
+ethtool_adv_to_mii_ctrl1000_t(ethtool_link_mode_mask_t ethadv)
 {
 	u32 result = 0;
 
@@ -181,9 +182,9 @@ static inline u32 ethtool_adv_to_mii_ctrl1000_t(u32 ethadv)
  * bits, when in 1000Base-T mode, to ethtool
  * advertisement settings.
  */
-static inline u32 mii_ctrl1000_to_ethtool_adv_t(u32 adv)
+static inline ethtool_link_mode_mask_t mii_ctrl1000_to_ethtool_adv_t(u32 adv)
 {
-	u32 result = 0;
+	ethtool_link_mode_mask_t result = 0;
 
 	if (adv & ADVERTISE_1000HALF)
 		result |= ADVERTISED_1000baseT_Half;
@@ -201,9 +202,9 @@ static inline u32 mii_ctrl1000_to_ethtool_adv_t(u32 adv)
  * bits, when in 1000Base-T mode, to ethtool
  * LP advertisement settings.
  */
-static inline u32 mii_lpa_to_ethtool_lpa_t(u32 lpa)
+static inline ethtool_link_mode_mask_t mii_lpa_to_ethtool_lpa_t(u32 lpa)
 {
-	u32 result = 0;
+	ethtool_link_mode_mask_t result = 0;
 
 	if (lpa & LPA_LPACK)
 		result |= ADVERTISED_Autoneg;
@@ -219,9 +220,9 @@ static inline u32 mii_lpa_to_ethtool_lpa_t(u32 lpa)
  * bits, when in 1000Base-T mode, to ethtool
  * advertisement settings.
  */
-static inline u32 mii_stat1000_to_ethtool_lpa_t(u32 lpa)
+static inline ethtool_link_mode_mask_t mii_stat1000_to_ethtool_lpa_t(u32 lpa)
 {
-	u32 result = 0;
+	ethtool_link_mode_mask_t result = 0;
 
 	if (lpa & LPA_1000HALF)
 		result |= ADVERTISED_1000baseT_Half;
@@ -239,7 +240,7 @@ static inline u32 mii_stat1000_to_ethtool_lpa_t(u32 lpa)
  * settings to phy autonegotiation advertisements for the
  * MII_CTRL1000 register when in 1000Base-X mode.
  */
-static inline u32 ethtool_adv_to_mii_adv_x(u32 ethadv)
+static inline u32 ethtool_adv_to_mii_adv_x(ethtool_link_mode_mask_t ethadv)
 {
 	u32 result = 0;
 
@@ -263,9 +264,9 @@ static inline u32 ethtool_adv_to_mii_adv_x(u32 ethadv)
  * bits, when in 1000Base-X mode, to ethtool
  * advertisement settings.
  */
-static inline u32 mii_adv_to_ethtool_adv_x(u32 adv)
+static inline ethtool_link_mode_mask_t mii_adv_to_ethtool_adv_x(u32 adv)
 {
-	u32 result = 0;
+	ethtool_link_mode_mask_t result = 0;
 
 	if (adv & ADVERTISE_1000XHALF)
 		result |= ADVERTISED_1000baseT_Half;
@@ -287,9 +288,9 @@ static inline u32 mii_adv_to_ethtool_adv_x(u32 adv)
  * bits, when in 1000Base-X mode, to ethtool
  * LP advertisement settings.
  */
-static inline u32 mii_lpa_to_ethtool_lpa_x(u32 lpa)
+static inline ethtool_link_mode_mask_t mii_lpa_to_ethtool_lpa_x(u32 lpa)
 {
-	u32 result = 0;
+	ethtool_link_mode_mask_t result = 0;
 
 	if (lpa & LPA_LPACK)
 		result |= ADVERTISED_Autoneg;
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 5/8] net: mdio: extend link mode support to 48 bits
  2015-01-06  2:54 [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits David Decotigny
                   ` (3 preceding siblings ...)
  2015-01-06  2:54 ` [PATCH net-next v2 4/8] net: mii: " David Decotigny
@ 2015-01-06  2:54 ` David Decotigny
  2015-01-06  2:54 ` [PATCH net-next v2 6/8] net: veth: " David Decotigny
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev, linux-kernel, linux-api
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot@googlers.com>

Signed-off-by: David Decotigny <decot@googlers.com>
---
 drivers/net/mdio.c   | 59 +++++++++++++++++++++++++++++-----------------------
 include/linux/mdio.h | 15 +++++++------
 2 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/drivers/net/mdio.c b/drivers/net/mdio.c
index 3e027ed..5cac2ac 100644
--- a/drivers/net/mdio.c
+++ b/drivers/net/mdio.c
@@ -148,9 +148,10 @@ int mdio45_nway_restart(const struct mdio_if_info *mdio)
 }
 EXPORT_SYMBOL(mdio45_nway_restart);
 
-static u32 mdio45_get_an(const struct mdio_if_info *mdio, u16 addr)
+static ethtool_link_mode_mask_t
+mdio45_get_an(const struct mdio_if_info *mdio, u16 addr)
 {
-	u32 result = 0;
+	ethtool_link_mode_mask_t result = 0;
 	int reg;
 
 	reg = mdio->mdio_read(mdio->dev, mdio->prtad, MDIO_MMD_AN, addr);
@@ -185,9 +186,11 @@ static u32 mdio45_get_an(const struct mdio_if_info *mdio, u16 addr)
  */
 void mdio45_ethtool_gset_npage(const struct mdio_if_info *mdio,
 			       struct ethtool_cmd *ecmd,
-			       u32 npage_adv, u32 npage_lpa)
+			       ethtool_link_mode_mask_t npage_adv,
+			       ethtool_link_mode_mask_t npage_lpa)
 {
 	int reg;
+	ethtool_link_mode_mask_t supported_link_modes, advertising_link_modes;
 	u32 speed;
 
 	BUILD_BUG_ON(MDIO_SUPPORTS_C22 != ETH_MDIO_SUPPORTS_C22);
@@ -206,64 +209,64 @@ void mdio45_ethtool_gset_npage(const struct mdio_if_info *mdio,
 	case MDIO_PMA_CTRL2_100BTX:
 	case MDIO_PMA_CTRL2_10BT:
 		ecmd->port = PORT_TP;
-		ecmd->supported = SUPPORTED_TP;
+		supported_link_modes = SUPPORTED_TP;
 		reg = mdio->mdio_read(mdio->dev, mdio->prtad, MDIO_MMD_PMAPMD,
 				      MDIO_SPEED);
 		if (reg & MDIO_SPEED_10G)
-			ecmd->supported |= SUPPORTED_10000baseT_Full;
+			supported_link_modes |= SUPPORTED_10000baseT_Full;
 		if (reg & MDIO_PMA_SPEED_1000)
-			ecmd->supported |= (SUPPORTED_1000baseT_Full |
+			supported_link_modes |= (SUPPORTED_1000baseT_Full |
 					    SUPPORTED_1000baseT_Half);
 		if (reg & MDIO_PMA_SPEED_100)
-			ecmd->supported |= (SUPPORTED_100baseT_Full |
+			supported_link_modes |= (SUPPORTED_100baseT_Full |
 					    SUPPORTED_100baseT_Half);
 		if (reg & MDIO_PMA_SPEED_10)
-			ecmd->supported |= (SUPPORTED_10baseT_Full |
+			supported_link_modes |= (SUPPORTED_10baseT_Full |
 					    SUPPORTED_10baseT_Half);
-		ecmd->advertising = ADVERTISED_TP;
+		advertising_link_modes = ADVERTISED_TP;
 		break;
 
 	case MDIO_PMA_CTRL2_10GBCX4:
 		ecmd->port = PORT_OTHER;
-		ecmd->supported = 0;
-		ecmd->advertising = 0;
+		supported_link_modes = 0;
+		advertising_link_modes = 0;
 		break;
 
 	case MDIO_PMA_CTRL2_10GBKX4:
 	case MDIO_PMA_CTRL2_10GBKR:
 	case MDIO_PMA_CTRL2_1000BKX:
 		ecmd->port = PORT_OTHER;
-		ecmd->supported = SUPPORTED_Backplane;
+		supported_link_modes = SUPPORTED_Backplane;
 		reg = mdio->mdio_read(mdio->dev, mdio->prtad, MDIO_MMD_PMAPMD,
 				      MDIO_PMA_EXTABLE);
 		if (reg & MDIO_PMA_EXTABLE_10GBKX4)
-			ecmd->supported |= SUPPORTED_10000baseKX4_Full;
+			supported_link_modes |= SUPPORTED_10000baseKX4_Full;
 		if (reg & MDIO_PMA_EXTABLE_10GBKR)
-			ecmd->supported |= SUPPORTED_10000baseKR_Full;
+			supported_link_modes |= SUPPORTED_10000baseKR_Full;
 		if (reg & MDIO_PMA_EXTABLE_1000BKX)
-			ecmd->supported |= SUPPORTED_1000baseKX_Full;
+			supported_link_modes |= SUPPORTED_1000baseKX_Full;
 		reg = mdio->mdio_read(mdio->dev, mdio->prtad, MDIO_MMD_PMAPMD,
 				      MDIO_PMA_10GBR_FECABLE);
 		if (reg & MDIO_PMA_10GBR_FECABLE_ABLE)
-			ecmd->supported |= SUPPORTED_10000baseR_FEC;
-		ecmd->advertising = ADVERTISED_Backplane;
+			supported_link_modes |= SUPPORTED_10000baseR_FEC;
+		advertising_link_modes = ADVERTISED_Backplane;
 		break;
 
 	/* All the other defined modes are flavours of optical */
 	default:
 		ecmd->port = PORT_FIBRE;
-		ecmd->supported = SUPPORTED_FIBRE;
-		ecmd->advertising = ADVERTISED_FIBRE;
+		supported_link_modes = SUPPORTED_FIBRE;
+		advertising_link_modes = ADVERTISED_FIBRE;
 		break;
 	}
 
 	if (mdio->mmds & MDIO_DEVS_AN) {
-		ecmd->supported |= SUPPORTED_Autoneg;
+		supported_link_modes |= SUPPORTED_Autoneg;
 		reg = mdio->mdio_read(mdio->dev, mdio->prtad, MDIO_MMD_AN,
 				      MDIO_CTRL1);
 		if (reg & MDIO_AN_CTRL1_ENABLE) {
 			ecmd->autoneg = AUTONEG_ENABLE;
-			ecmd->advertising |=
+			advertising_link_modes |=
 				ADVERTISED_Autoneg |
 				mdio45_get_an(mdio, MDIO_AN_ADVERTISE) |
 				npage_adv;
@@ -275,21 +278,22 @@ void mdio45_ethtool_gset_npage(const struct mdio_if_info *mdio,
 	}
 
 	if (ecmd->autoneg) {
-		u32 modes = 0;
+		ethtool_link_mode_mask_t modes = 0;
 		int an_stat = mdio->mdio_read(mdio->dev, mdio->prtad,
 					      MDIO_MMD_AN, MDIO_STAT1);
 
 		/* If AN is complete and successful, report best common
 		 * mode, otherwise report best advertised mode. */
 		if (an_stat & MDIO_AN_STAT1_COMPLETE) {
-			ecmd->lp_advertising =
+			ethtool_link_mode_mask_t lp_adv =
 				mdio45_get_an(mdio, MDIO_AN_LPA) | npage_lpa;
 			if (an_stat & MDIO_AN_STAT1_LPABLE)
-				ecmd->lp_advertising |= ADVERTISED_Autoneg;
-			modes = ecmd->advertising & ecmd->lp_advertising;
+				lp_adv |= ADVERTISED_Autoneg;
+			ethtool_cmd_lp_advertising_set(ecmd, lp_adv);
+			modes = advertising_link_modes & lp_adv;
 		}
 		if ((modes & ~ADVERTISED_Autoneg) == 0)
-			modes = ecmd->advertising;
+			modes = advertising_link_modes;
 
 		if (modes & (ADVERTISED_10000baseT_Full |
 			     ADVERTISED_10000baseKX4_Full |
@@ -338,6 +342,9 @@ void mdio45_ethtool_gset_npage(const struct mdio_if_info *mdio,
 			break;
 		}
 	}
+
+	ethtool_cmd_supported_set(ecmd, supported_link_modes);
+	ethtool_cmd_advertising_set(ecmd, advertising_link_modes);
 }
 EXPORT_SYMBOL(mdio45_ethtool_gset_npage);
 
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index b42963b..18c649a 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -69,7 +69,8 @@ extern int mdio45_links_ok(const struct mdio_if_info *mdio, u32 mmds);
 extern int mdio45_nway_restart(const struct mdio_if_info *mdio);
 extern void mdio45_ethtool_gset_npage(const struct mdio_if_info *mdio,
 				      struct ethtool_cmd *ecmd,
-				      u32 npage_adv, u32 npage_lpa);
+				      ethtool_link_mode_mask_t npage_adv,
+				      ethtool_link_mode_mask_t npage_lpa);
 
 /**
  * mdio45_ethtool_gset - get settings for ETHTOOL_GSET
@@ -97,9 +98,10 @@ extern int mdio_mii_ioctl(const struct mdio_if_info *mdio,
  * A small helper function that translates MMD EEE Capability (3.20) bits
  * to ethtool supported settings.
  */
-static inline u32 mmd_eee_cap_to_ethtool_sup_t(u16 eee_cap)
+static inline ethtool_link_mode_mask_t
+mmd_eee_cap_to_ethtool_sup_t(u16 eee_cap)
 {
-	u32 supported = 0;
+	ethtool_link_mode_mask_t supported = 0;
 
 	if (eee_cap & MDIO_EEE_100TX)
 		supported |= SUPPORTED_100baseT_Full;
@@ -125,9 +127,10 @@ static inline u32 mmd_eee_cap_to_ethtool_sup_t(u16 eee_cap)
  * and MMD EEE Link Partner Ability (7.61) bits to ethtool advertisement
  * settings.
  */
-static inline u32 mmd_eee_adv_to_ethtool_adv_t(u16 eee_adv)
+static inline ethtool_link_mode_mask_t
+mmd_eee_adv_to_ethtool_adv_t(u16 eee_adv)
 {
-	u32 adv = 0;
+	ethtool_link_mode_mask_t adv = 0;
 
 	if (eee_adv & MDIO_EEE_100TX)
 		adv |= ADVERTISED_100baseT_Full;
@@ -153,7 +156,7 @@ static inline u32 mmd_eee_adv_to_ethtool_adv_t(u16 eee_adv)
  * to EEE advertisements for the MMD EEE Advertisement (7.60) and
  * MMD EEE Link Partner Ability (7.61) registers.
  */
-static inline u16 ethtool_adv_to_mmd_eee_adv_t(u32 adv)
+static inline u16 ethtool_adv_to_mmd_eee_adv_t(ethtool_link_mode_mask_t adv)
 {
 	u16 reg = 0;
 
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 6/8] net: veth: extend link mode support to 48 bits
  2015-01-06  2:54 [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits David Decotigny
                   ` (4 preceding siblings ...)
  2015-01-06  2:54 ` [PATCH net-next v2 5/8] net: mdio: " David Decotigny
@ 2015-01-06  2:54 ` David Decotigny
  2015-01-06  2:54 ` [PATCH net-next v2 7/8] net: tun: " David Decotigny
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev, linux-kernel, linux-api
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot@googlers.com>

Signed-off-by: David Decotigny <decot@googlers.com>
---
 drivers/net/veth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 8ad5965..902e127 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -49,8 +49,8 @@ static struct {
 
 static int veth_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
-	cmd->supported		= 0;
-	cmd->advertising	= 0;
+	ethtool_cmd_supported_set(cmd, 0);
+	ethtool_cmd_advertising_set(cmd, 0);
 	ethtool_cmd_speed_set(cmd, SPEED_10000);
 	cmd->duplex		= DUPLEX_FULL;
 	cmd->port		= PORT_TP;
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 7/8] net: tun: extend link mode support to 48 bits
  2015-01-06  2:54 [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits David Decotigny
                   ` (5 preceding siblings ...)
  2015-01-06  2:54 ` [PATCH net-next v2 6/8] net: veth: " David Decotigny
@ 2015-01-06  2:54 ` David Decotigny
  2015-01-06  2:54 ` [PATCH net-next v2 8/8] net: mlx4_en: " David Decotigny
  2015-01-06 13:56 ` [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps " Amir Vadai
  8 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev, linux-kernel, linux-api
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot@googlers.com>

Signed-off-by: David Decotigny <decot@googlers.com>
---
 drivers/net/tun.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index c0df872..88f9078 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2257,8 +2257,8 @@ static struct miscdevice tun_miscdev = {
 
 static int tun_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
-	cmd->supported		= 0;
-	cmd->advertising	= 0;
+	ethtool_cmd_supported_set(cmd, 0);
+	ethtool_cmd_advertising_set(cmd, 0);
 	ethtool_cmd_speed_set(cmd, SPEED_10);
 	cmd->duplex		= DUPLEX_FULL;
 	cmd->port		= PORT_TP;
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next v2 8/8] net: mlx4_en: extend link mode support to 48 bits
  2015-01-06  2:54 [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits David Decotigny
                   ` (6 preceding siblings ...)
  2015-01-06  2:54 ` [PATCH net-next v2 7/8] net: tun: " David Decotigny
@ 2015-01-06  2:54 ` David Decotigny
  2015-01-06 13:56 ` [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps " Amir Vadai
  8 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06  2:54 UTC (permalink / raw)
  To: Amir Vadai, Florian Fainelli, netdev, linux-kernel, linux-api
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Saeed Mahameed,
	Venkata Duvvuru, Govindarajulu Varadarajan

From: David Decotigny <decot@googlers.com>

Signed-off-by: David Decotigny <decot@googlers.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 73 ++++++++++++++++---------
 1 file changed, 46 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index 90e0f04..8ccbe3f 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -46,6 +46,11 @@
 #define EN_ETHTOOL_SHORT_MASK cpu_to_be16(0xffff)
 #define EN_ETHTOOL_WORD_MASK  cpu_to_be32(0xffffffff)
 
+static u32 mlx4_en_get_compat_flags(struct net_device *unused_netdev)
+{
+	return ETH_COMPAT_SUPPORT_LINK_MODE_48b;
+}
+
 static int mlx4_en_moderation_update(struct mlx4_en_priv *priv)
 {
 	int i;
@@ -388,7 +393,8 @@ static u32 mlx4_en_autoneg_get(struct net_device *dev)
 	return autoneg;
 }
 
-static u32 ptys_get_supported_port(struct mlx4_ptys_reg *ptys_reg)
+static ethtool_link_mode_mask_t
+ptys_get_supported_port(struct mlx4_ptys_reg *ptys_reg)
 {
 	u32 eth_proto = be32_to_cpu(ptys_reg->eth_proto_cap);
 
@@ -465,7 +471,7 @@ enum ethtool_report {
 };
 
 /* Translates mlx4 link mode to equivalent ethtool Link modes/speed */
-static u32 ptys2ethtool_map[MLX4_LINK_MODES_SZ][3] = {
+static u64 ptys2ethtool_map[MLX4_LINK_MODES_SZ][3] = {
 	[MLX4_100BASE_TX] = {
 		SUPPORTED_100baseT_Full,
 		ADVERTISED_100baseT_Full,
@@ -558,10 +564,11 @@ static u32 ptys2ethtool_map[MLX4_LINK_MODES_SZ][3] = {
 		},
 };
 
-static u32 ptys2ethtool_link_modes(u32 eth_proto, enum ethtool_report report)
+static ethtool_link_mode_mask_t
+ptys2ethtool_link_modes(u32 eth_proto, enum ethtool_report report)
 {
 	int i;
-	u32 link_modes = 0;
+	ethtool_link_mode_mask_t link_modes = 0;
 
 	for (i = 0; i < MLX4_LINK_MODES_SZ; i++) {
 		if (eth_proto & MLX4_PROT_MASK(i))
@@ -570,7 +577,8 @@ static u32 ptys2ethtool_link_modes(u32 eth_proto, enum ethtool_report report)
 	return link_modes;
 }
 
-static u32 ethtool2ptys_link_modes(u32 link_modes, enum ethtool_report report)
+static u32 ethtool2ptys_link_modes(ethtool_link_mode_mask_t link_modes,
+				   enum ethtool_report report)
 {
 	int i;
 	u32 ptys_modes = 0;
@@ -601,6 +609,9 @@ static int ethtool_get_ptys_settings(struct net_device *dev,
 	struct mlx4_en_priv *priv = netdev_priv(dev);
 	struct mlx4_ptys_reg ptys_reg;
 	u32 eth_proto;
+	ethtool_link_mode_mask_t eth_supported;
+	ethtool_link_mode_mask_t eth_advertising;
+	ethtool_link_mode_mask_t eth_lp_advertising;
 	int ret;
 
 	memset(&ptys_reg, 0, sizeof(ptys_reg));
@@ -624,41 +635,44 @@ static int ethtool_get_ptys_settings(struct net_device *dev,
 	en_dbg(DRV, priv, "ptys_reg.eth_proto_lp_adv %x\n",
 	       be32_to_cpu(ptys_reg.eth_proto_lp_adv));
 
-	cmd->supported = 0;
-	cmd->advertising = 0;
+	eth_supported = 0;
+	eth_advertising = 0;
 
-	cmd->supported |= ptys_get_supported_port(&ptys_reg);
+	eth_supported |= ptys_get_supported_port(&ptys_reg);
 
 	eth_proto = be32_to_cpu(ptys_reg.eth_proto_cap);
-	cmd->supported |= ptys2ethtool_link_modes(eth_proto, SUPPORTED);
+	eth_supported |= ptys2ethtool_link_modes(eth_proto, SUPPORTED);
 
 	eth_proto = be32_to_cpu(ptys_reg.eth_proto_admin);
-	cmd->advertising |= ptys2ethtool_link_modes(eth_proto, ADVERTISED);
+	eth_advertising |= ptys2ethtool_link_modes(eth_proto, ADVERTISED);
 
-	cmd->supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;
-	cmd->advertising |= (priv->prof->tx_pause) ? ADVERTISED_Pause : 0;
+	eth_supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;
+	eth_advertising |= (priv->prof->tx_pause) ? ADVERTISED_Pause : 0;
 
-	cmd->advertising |= (priv->prof->tx_pause ^ priv->prof->rx_pause) ?
+	eth_advertising |= (priv->prof->tx_pause ^ priv->prof->rx_pause) ?
 		ADVERTISED_Asym_Pause : 0;
 
 	cmd->port = ptys_get_active_port(&ptys_reg);
-	cmd->transceiver = (SUPPORTED_TP & cmd->supported) ?
+	cmd->transceiver = (SUPPORTED_TP & eth_supported) ?
 		XCVR_EXTERNAL : XCVR_INTERNAL;
 
 	if (mlx4_en_autoneg_get(dev)) {
-		cmd->supported |= SUPPORTED_Autoneg;
-		cmd->advertising |= ADVERTISED_Autoneg;
+		eth_supported |= SUPPORTED_Autoneg;
+		eth_advertising |= ADVERTISED_Autoneg;
 	}
 
 	cmd->autoneg = (priv->port_state.flags & MLX4_EN_PORT_ANC) ?
 		AUTONEG_ENABLE : AUTONEG_DISABLE;
 
 	eth_proto = be32_to_cpu(ptys_reg.eth_proto_lp_adv);
-	cmd->lp_advertising = ptys2ethtool_link_modes(eth_proto, ADVERTISED);
+	eth_lp_advertising = ptys2ethtool_link_modes(eth_proto, ADVERTISED);
 
-	cmd->lp_advertising |= (priv->port_state.flags & MLX4_EN_PORT_ANC) ?
+	eth_lp_advertising |= (priv->port_state.flags & MLX4_EN_PORT_ANC) ?
 			ADVERTISED_Autoneg : 0;
 
+	ethtool_cmd_supported_set(cmd, eth_supported);
+	ethtool_cmd_advertising_set(cmd, eth_advertising);
+	ethtool_cmd_lp_advertising_set(cmd, eth_lp_advertising);
 	cmd->phy_address = 0;
 	cmd->mdio_support = 0;
 	cmd->maxtxpkt = 0;
@@ -673,27 +687,30 @@ static void ethtool_get_default_settings(struct net_device *dev,
 					 struct ethtool_cmd *cmd)
 {
 	struct mlx4_en_priv *priv = netdev_priv(dev);
+	ethtool_link_mode_mask_t eth_supported, eth_advertising;
 	int trans_type;
 
 	cmd->autoneg = AUTONEG_DISABLE;
-	cmd->supported = SUPPORTED_10000baseT_Full;
-	cmd->advertising = ADVERTISED_10000baseT_Full;
+	eth_supported = SUPPORTED_10000baseT_Full;
+	eth_advertising = ADVERTISED_10000baseT_Full;
 	trans_type = priv->port_state.transceiver;
 
 	if (trans_type > 0 && trans_type <= 0xC) {
 		cmd->port = PORT_FIBRE;
 		cmd->transceiver = XCVR_EXTERNAL;
-		cmd->supported |= SUPPORTED_FIBRE;
-		cmd->advertising |= ADVERTISED_FIBRE;
+		eth_supported |= SUPPORTED_FIBRE;
+		eth_advertising |= ADVERTISED_FIBRE;
 	} else if (trans_type == 0x80 || trans_type == 0) {
 		cmd->port = PORT_TP;
 		cmd->transceiver = XCVR_INTERNAL;
-		cmd->supported |= SUPPORTED_TP;
-		cmd->advertising |= ADVERTISED_TP;
+		eth_supported |= SUPPORTED_TP;
+		eth_advertising |= ADVERTISED_TP;
 	} else  {
 		cmd->port = -1;
 		cmd->transceiver = -1;
 	}
+	ethtool_cmd_supported_set(cmd, eth_supported);
+	ethtool_cmd_advertising_set(cmd, eth_advertising);
 }
 
 static int mlx4_en_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
@@ -747,13 +764,14 @@ static int mlx4_en_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 	struct mlx4_en_priv *priv = netdev_priv(dev);
 	struct mlx4_ptys_reg ptys_reg;
 	__be32 proto_admin;
+	const ethtool_link_mode_mask_t eth_adv = ethtool_cmd_advertising(cmd);
 	int ret;
 
-	u32 ptys_adv = ethtool2ptys_link_modes(cmd->advertising, ADVERTISED);
+	u32 ptys_adv = ethtool2ptys_link_modes(eth_adv, ADVERTISED);
 	int speed = ethtool_cmd_speed(cmd);
 
-	en_dbg(DRV, priv, "Set Speed=%d adv=0x%x autoneg=%d duplex=%d\n",
-	       speed, cmd->advertising, cmd->autoneg, cmd->duplex);
+	en_dbg(DRV, priv, "Set Speed=%d adv=0x%llx autoneg=%d duplex=%d\n",
+	       speed, eth_adv, cmd->autoneg, cmd->duplex);
 
 	if (!(priv->mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_ETH_PROT_CTRL) ||
 	    (cmd->duplex == DUPLEX_HALF))
@@ -1821,6 +1839,7 @@ static int mlx4_en_get_module_eeprom(struct net_device *dev,
 }
 
 const struct ethtool_ops mlx4_en_ethtool_ops = {
+	.get_compat_flags = mlx4_en_get_compat_flags,
 	.get_drvinfo = mlx4_en_get_drvinfo,
 	.get_settings = mlx4_en_get_settings,
 	.set_settings = mlx4_en_set_settings,
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
  2015-01-06  2:54 [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits David Decotigny
                   ` (7 preceding siblings ...)
  2015-01-06  2:54 ` [PATCH net-next v2 8/8] net: mlx4_en: " David Decotigny
@ 2015-01-06 13:56 ` Amir Vadai
  2015-01-06 17:36     ` David Decotigny
  2015-01-06 22:29     ` David Miller
  8 siblings, 2 replies; 24+ messages in thread
From: Amir Vadai @ 2015-01-06 13:56 UTC (permalink / raw)
  To: David Decotigny, Florian Fainelli, netdev, linux-kernel,
	linux-api, Saeed Mahameed
  Cc: David Decotigny, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

On 1/6/2015 4:54 AM, David Decotigny wrote:
> This patch series increases the width of the supported/advertising
> ethtool masks from 32 bits to 48. This should allow to breathe for a
> couple more years (or... months?).
Hi,

Nice work.
What worries me is that it is going to be for weeks more than years or
months...

Mellanox is about to release next month a driver for a new NIC, with 3
new speeds * few link modes for each + new link modes for 10G.
It seems that we will need to consume almost all the new bits.

Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
  2015-01-06 13:56 ` [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps " Amir Vadai
  2015-01-06 17:36     ` David Decotigny
@ 2015-01-06 17:36     ` David Decotigny
  1 sibling, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06 17:36 UTC (permalink / raw)
  To: Amir Vadai
  Cc: Florian Fainelli, netdev, linux-kernel, linux-api,
	Saeed Mahameed, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

Interesting. It seems that the band-aid I was proposing is already
obsolete. We could still use the remaining reserved 16 bits to encode
5 more bits per mask (that is: 53 bits / mask total). But if I
understand you, it would allow us to survive only a few months longer,
as opposed to a few weeks.

One short-term alternative solution I can imagine is the following:
/* For example bitmap-based for variable length: */
struct ethtool_link_mode {
  __u32 cmd;
  __u8 autoneg :1;
  __u8 duplex :2;
 __u16 supported_nbits;
  __u16 advertising_nbits;
  __u16 lp_advertising_nbits;
  __u32 reserved[4];
  __u8 masks[0];
};
/* Or simpler, statically limited to 64b / mask, but easier to migrate
to for driver authors: */
struct ethtool_link_mode {
  __u32 cmd;
  __u8 autoneg :1;
  __u8 duplex :2;
   __u64 supported;
  __u64 advertising;
  __u64 lp_advertising;
  __u32 reserved[4];
};
#define ETHTOOL_GLINK_MODE 0x0000004a
#define ETHTOOL_SLINK_MODE 0x0000004b
struct ethtool_ops {
...
   int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
   int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
};

The same thing required for EEE.

I am not sure about moving the autoneg and duplex fields into the new
struct. Especially the "duplex" field.

Then the idea would be to update the ethtool user-space tool to try
get/set_link mode when reporting/changing the autoneg/advertising
settings.

Both will require significant effort from the driver authors.
Especially if the variable-length bitmap approach is preferred:
 - most drivers currently use simple bitwise arithmetic in their code,
and that goes far beyond get/set_settings, it is sometimes part of the
core driver logic. They will have to migrate to the bitmap API if they
want to use the larger bitmaps (note: no change needed if they are
happy with <= 32b / mask)
 - we would have to progressively deprecate the use of #define
ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.

Any feedback welcome. In the meantime, I am going to propose a v3 of
current option with 53 bits / mask. I can also propose a prototype of
the scheme described above, please let me know.

On Tue, Jan 6, 2015 at 5:56 AM, Amir Vadai <amirv@mellanox.com> wrote:
> On 1/6/2015 4:54 AM, David Decotigny wrote:
>> This patch series increases the width of the supported/advertising
>> ethtool masks from 32 bits to 48. This should allow to breathe for a
>> couple more years (or... months?).
> Hi,
>
> Nice work.
> What worries me is that it is going to be for weeks more than years or
> months...
>
> Mellanox is about to release next month a driver for a new NIC, with 3
> new speeds * few link modes for each + new link modes for 10G.
> It seems that we will need to consume almost all the new bits.
>
> Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-06 17:36     ` David Decotigny
  0 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06 17:36 UTC (permalink / raw)
  To: Amir Vadai
  Cc: Florian Fainelli, netdev, linux-kernel, linux-api,
	Saeed Mahameed, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

Interesting. It seems that the band-aid I was proposing is already
obsolete. We could still use the remaining reserved 16 bits to encode
5 more bits per mask (that is: 53 bits / mask total). But if I
understand you, it would allow us to survive only a few months longer,
as opposed to a few weeks.

One short-term alternative solution I can imagine is the following:
/* For example bitmap-based for variable length: */
struct ethtool_link_mode {
  __u32 cmd;
  __u8 autoneg :1;
  __u8 duplex :2;
 __u16 supported_nbits;
  __u16 advertising_nbits;
  __u16 lp_advertising_nbits;
  __u32 reserved[4];
  __u8 masks[0];
};
/* Or simpler, statically limited to 64b / mask, but easier to migrate
to for driver authors: */
struct ethtool_link_mode {
  __u32 cmd;
  __u8 autoneg :1;
  __u8 duplex :2;
   __u64 supported;
  __u64 advertising;
  __u64 lp_advertising;
  __u32 reserved[4];
};
#define ETHTOOL_GLINK_MODE 0x0000004a
#define ETHTOOL_SLINK_MODE 0x0000004b
struct ethtool_ops {
...
   int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
   int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
};

The same thing required for EEE.

I am not sure about moving the autoneg and duplex fields into the new
struct. Especially the "duplex" field.

Then the idea would be to update the ethtool user-space tool to try
get/set_link mode when reporting/changing the autoneg/advertising
settings.

Both will require significant effort from the driver authors.
Especially if the variable-length bitmap approach is preferred:
 - most drivers currently use simple bitwise arithmetic in their code,
and that goes far beyond get/set_settings, it is sometimes part of the
core driver logic. They will have to migrate to the bitmap API if they
want to use the larger bitmaps (note: no change needed if they are
happy with <= 32b / mask)
 - we would have to progressively deprecate the use of #define
ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.

Any feedback welcome. In the meantime, I am going to propose a v3 of
current option with 53 bits / mask. I can also propose a prototype of
the scheme described above, please let me know.

On Tue, Jan 6, 2015 at 5:56 AM, Amir Vadai <amirv@mellanox.com> wrote:
> On 1/6/2015 4:54 AM, David Decotigny wrote:
>> This patch series increases the width of the supported/advertising
>> ethtool masks from 32 bits to 48. This should allow to breathe for a
>> couple more years (or... months?).
> Hi,
>
> Nice work.
> What worries me is that it is going to be for weeks more than years or
> months...
>
> Mellanox is about to release next month a driver for a new NIC, with 3
> new speeds * few link modes for each + new link modes for 10G.
> It seems that we will need to consume almost all the new bits.
>
> Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-06 17:36     ` David Decotigny
  0 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-06 17:36 UTC (permalink / raw)
  To: Amir Vadai
  Cc: Florian Fainelli, netdev, linux-kernel, linux-api,
	Saeed Mahameed, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

Interesting. It seems that the band-aid I was proposing is already
obsolete. We could still use the remaining reserved 16 bits to encode
5 more bits per mask (that is: 53 bits / mask total). But if I
understand you, it would allow us to survive only a few months longer,
as opposed to a few weeks.

One short-term alternative solution I can imagine is the following:
/* For example bitmap-based for variable length: */
struct ethtool_link_mode {
  __u32 cmd;
  __u8 autoneg :1;
  __u8 duplex :2;
 __u16 supported_nbits;
  __u16 advertising_nbits;
  __u16 lp_advertising_nbits;
  __u32 reserved[4];
  __u8 masks[0];
};
/* Or simpler, statically limited to 64b / mask, but easier to migrate
to for driver authors: */
struct ethtool_link_mode {
  __u32 cmd;
  __u8 autoneg :1;
  __u8 duplex :2;
   __u64 supported;
  __u64 advertising;
  __u64 lp_advertising;
  __u32 reserved[4];
};
#define ETHTOOL_GLINK_MODE 0x0000004a
#define ETHTOOL_SLINK_MODE 0x0000004b
struct ethtool_ops {
...
   int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
   int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
};

The same thing required for EEE.

I am not sure about moving the autoneg and duplex fields into the new
struct. Especially the "duplex" field.

Then the idea would be to update the ethtool user-space tool to try
get/set_link mode when reporting/changing the autoneg/advertising
settings.

Both will require significant effort from the driver authors.
Especially if the variable-length bitmap approach is preferred:
 - most drivers currently use simple bitwise arithmetic in their code,
and that goes far beyond get/set_settings, it is sometimes part of the
core driver logic. They will have to migrate to the bitmap API if they
want to use the larger bitmaps (note: no change needed if they are
happy with <= 32b / mask)
 - we would have to progressively deprecate the use of #define
ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.

Any feedback welcome. In the meantime, I am going to propose a v3 of
current option with 53 bits / mask. I can also propose a prototype of
the scheme described above, please let me know.

On Tue, Jan 6, 2015 at 5:56 AM, Amir Vadai <amirv@mellanox.com> wrote:
> On 1/6/2015 4:54 AM, David Decotigny wrote:
>> This patch series increases the width of the supported/advertising
>> ethtool masks from 32 bits to 48. This should allow to breathe for a
>> couple more years (or... months?).
> Hi,
>
> Nice work.
> What worries me is that it is going to be for weeks more than years or
> months...
>
> Mellanox is about to release next month a driver for a new NIC, with 3
> new speeds * few link modes for each + new link modes for 10G.
> It seems that we will need to consume almost all the new bits.
>
> Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-06 22:29     ` David Miller
  0 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2015-01-06 22:29 UTC (permalink / raw)
  To: amirv
  Cc: ddecotig, f.fainelli, netdev, linux-kernel, linux-api, saeedm,
	decot, jasowang, mst, herbert, viro, ben, yamato, xii, nhorman,
	xiyou.wangcong, fbl, teg, jiri, vyasevic, ebiederm,
	VenkatKumar.Duvvuru, _govind

From: Amir Vadai <amirv@mellanox.com>
Date: Tue, 6 Jan 2015 15:56:33 +0200

> Mellanox is about to release next month a driver for a new NIC, with 3
> new speeds * few link modes for each + new link modes for 10G.
> It seems that we will need to consume almost all the new bits.

This tells me that the approach to this problem needs to be rethought.

Maybe we just need to bite the bullet and make a new ETHTOOL_GSET_2
and ETHTOOL_SSET_2 or whatever you want to name them.

Then we can define a completely new structure, with 64-bit bitmaps
for link modes or whatever.  The ethtool_op callbacks work using
this structure, and only the net/core/ethtool.c code knows about
the older structure and translates to/from for ETHTOOL_{GSET,SSET}.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-06 22:29     ` David Miller
  0 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2015-01-06 22:29 UTC (permalink / raw)
  To: amirv-VPRAkNaXOzVWk0Htik3J/w
  Cc: ddecotig-Re5JQEeQqe8AvxtiuMwx3w,
	f.fainelli-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA, saeedm-VPRAkNaXOzVWk0Htik3J/w,
	decot-Ypc/8FJVVoBWk0Htik3J/w, jasowang-H+wXaHxf7aLQT0dZR+AlfA,
	mst-H+wXaHxf7aLQT0dZR+AlfA,
	herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q,
	viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
	ben-/+tVBieCtBitmTQ+vhA3Yw, yamato-H+wXaHxf7aLQT0dZR+AlfA,
	xii-hpIqsD4AKlfQT0dZR+AlfA, nhorman-2XuSBdqkA4R54TAoqtyWWQ,
	xiyou.wangcong-Re5JQEeQqe8AvxtiuMwx3w,
	fbl-H+wXaHxf7aLQT0dZR+AlfA, teg-B22kvLQNl6c,
	jiri-rHqAuBHg3fBzbRFIqnYvSA, vyasevic-H+wXaHxf7aLQT0dZR+AlfA,
	ebiederm-aS9lmoZGLiVWk0Htik3J/w,
	VenkatKumar.Duvvuru-iH1Dq9VlAzfQT0dZR+AlfA, _govind-KK0ffGbhmjU

From: Amir Vadai <amirv-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Date: Tue, 6 Jan 2015 15:56:33 +0200

> Mellanox is about to release next month a driver for a new NIC, with 3
> new speeds * few link modes for each + new link modes for 10G.
> It seems that we will need to consume almost all the new bits.

This tells me that the approach to this problem needs to be rethought.

Maybe we just need to bite the bullet and make a new ETHTOOL_GSET_2
and ETHTOOL_SSET_2 or whatever you want to name them.

Then we can define a completely new structure, with 64-bit bitmaps
for link modes or whatever.  The ethtool_op callbacks work using
this structure, and only the net/core/ethtool.c code knows about
the older structure and translates to/from for ETHTOOL_{GSET,SSET}.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
  2015-01-06 22:29     ` David Miller
  (?)
@ 2015-01-06 23:08     ` Ben Hutchings
  -1 siblings, 0 replies; 24+ messages in thread
From: Ben Hutchings @ 2015-01-06 23:08 UTC (permalink / raw)
  To: David Miller
  Cc: amirv, ddecotig, f.fainelli, netdev, linux-kernel, linux-api,
	saeedm, decot, jasowang, mst, herbert, viro, yamato, xii,
	nhorman, xiyou.wangcong, fbl, teg, jiri, vyasevic, ebiederm,
	VenkatKumar.Duvvuru, _govind

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

On Tue, 2015-01-06 at 17:29 -0500, David Miller wrote:
> From: Amir Vadai <amirv@mellanox.com>
> Date: Tue, 6 Jan 2015 15:56:33 +0200
> 
> > Mellanox is about to release next month a driver for a new NIC, with 3
> > new speeds * few link modes for each + new link modes for 10G.
> > It seems that we will need to consume almost all the new bits.
> 
> This tells me that the approach to this problem needs to be rethought.
> 
> Maybe we just need to bite the bullet and make a new ETHTOOL_GSET_2
> and ETHTOOL_SSET_2 or whatever you want to name them.
> 
> Then we can define a completely new structure, with 64-bit bitmaps
> for link modes or whatever.  The ethtool_op callbacks work using
> this structure, and only the net/core/ethtool.c code knows about
> the older structure and translates to/from for ETHTOOL_{GSET,SSET}.

Agreed.

Ben.

-- 
Ben Hutchings
This sentence contradicts itself - no actually it doesn't.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-08  8:40       ` Amir Vadai
  0 siblings, 0 replies; 24+ messages in thread
From: Amir Vadai @ 2015-01-08  8:40 UTC (permalink / raw)
  To: David Decotigny
  Cc: Florian Fainelli, netdev, linux-kernel, linux-api,
	Saeed Mahameed, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

On 1/6/2015 7:36 PM, David Decotigny wrote:
> Interesting. It seems that the band-aid I was proposing is already
> obsolete. We could still use the remaining reserved 16 bits to encode
> 5 more bits per mask (that is: 53 bits / mask total). But if I
> understand you, it would allow us to survive only a few months longer,
> as opposed to a few weeks.
> 
> One short-term alternative solution I can imagine is the following:
> /* For example bitmap-based for variable length: */
> struct ethtool_link_mode {
>   __u32 cmd;
>   __u8 autoneg :1;
>   __u8 duplex :2;
>  __u16 supported_nbits;
>   __u16 advertising_nbits;
>   __u16 lp_advertising_nbits;
>   __u32 reserved[4];
>   __u8 masks[0];
> };
> /* Or simpler, statically limited to 64b / mask, but easier to migrate
> to for driver authors: */
I think the first options is better. A driver will have to do changes in
order to support >32 link modes, so better change it once now, without
having to change it again for >64 link modes.

> struct ethtool_link_mode {
>   __u32 cmd;
>   __u8 autoneg :1;
>   __u8 duplex :2;
>    __u64 supported;
>   __u64 advertising;
>   __u64 lp_advertising;
>   __u32 reserved[4];
> };
> #define ETHTOOL_GLINK_MODE 0x0000004a
> #define ETHTOOL_SLINK_MODE 0x0000004b
> struct ethtool_ops {
> ...
>    int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
>    int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
> };
> 
> The same thing required for EEE.
Yeh :(

> 
> I am not sure about moving the autoneg and duplex fields into the new
> struct. Especially the "duplex" field.
I think so too. ethtool user space will call ETHTOOL_[GS]SET and after
that ETHTOOL_[GS]LINK_MODE (if supported). No need to get the
duplex/autoneg fields again.

> 
> Then the idea would be to update the ethtool user-space tool to try
> get/set_link mode when reporting/changing the autoneg/advertising
> settings.
> 
> Both will require significant effort from the driver authors.
> Especially if the variable-length bitmap approach is preferred:
>  - most drivers currently use simple bitwise arithmetic in their code,
> and that goes far beyond get/set_settings, it is sometimes part of the
> core driver logic. They will have to migrate to the bitmap API if they
> want to use the larger bitmaps (note: no change needed if they are
> happy with <= 32b / mask)
As I said above, it will save as doing this work again in the future,
and more problematic, save another version to backport in the future. In
addition, not all drivers will have to do it, only if >32 link speeds is
needed - this work will be required.

>  - we would have to progressively deprecate the use of #define
> ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.
Maybe we could use some macro juggling to define the legacy macro's
using enum for the first 32 bits, and fail the compilation if used on
>32. For example, calling this:
DEFINE_LINK_MODE(ADVERTISED_1000baseT_Full, 5)

Will add the following:
ADVERTISED_1000baseT_Full_SHIFT = 5
ADVERTISED_1000baseT_Full = (1<<5)

DEFINE_LINK_MODE(ADVERTISED_100000baseKR5_Full, 50) will add:
ADVERTISED_100000baseKR5_Full_SHIFT = 50
ADVERTISED_100000baseKR5_Full = #error new link speeds must be defined
using [gs]et_link_speed

This will break compilation if ADVERTISED_100000baseKR5_Full is used in
[gs]et_settings (I know the '#error' will not print something very
pretty - I used it only to explain what I meant)

> 
> Any feedback welcome. In the meantime, I am going to propose a v3 of
> current option with 53 bits / mask. I can also propose a prototype of
> the scheme described above, please let me know.
I think that it is better to do it once, and skip the 53 bits / mask
version.
I'll be happy to assist.

Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-08  8:40       ` Amir Vadai
  0 siblings, 0 replies; 24+ messages in thread
From: Amir Vadai @ 2015-01-08  8:40 UTC (permalink / raw)
  To: David Decotigny
  Cc: Florian Fainelli, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA, Saeed Mahameed,
	David S. Miller, Jason Wang, Michael S. Tsirkin, Herbert Xu,
	Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang, Neil Horman,
	WANG Cong, Flavio Leitner, Tom Gundersen, Jiri Pirko,
	Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

On 1/6/2015 7:36 PM, David Decotigny wrote:
> Interesting. It seems that the band-aid I was proposing is already
> obsolete. We could still use the remaining reserved 16 bits to encode
> 5 more bits per mask (that is: 53 bits / mask total). But if I
> understand you, it would allow us to survive only a few months longer,
> as opposed to a few weeks.
> 
> One short-term alternative solution I can imagine is the following:
> /* For example bitmap-based for variable length: */
> struct ethtool_link_mode {
>   __u32 cmd;
>   __u8 autoneg :1;
>   __u8 duplex :2;
>  __u16 supported_nbits;
>   __u16 advertising_nbits;
>   __u16 lp_advertising_nbits;
>   __u32 reserved[4];
>   __u8 masks[0];
> };
> /* Or simpler, statically limited to 64b / mask, but easier to migrate
> to for driver authors: */
I think the first options is better. A driver will have to do changes in
order to support >32 link modes, so better change it once now, without
having to change it again for >64 link modes.

> struct ethtool_link_mode {
>   __u32 cmd;
>   __u8 autoneg :1;
>   __u8 duplex :2;
>    __u64 supported;
>   __u64 advertising;
>   __u64 lp_advertising;
>   __u32 reserved[4];
> };
> #define ETHTOOL_GLINK_MODE 0x0000004a
> #define ETHTOOL_SLINK_MODE 0x0000004b
> struct ethtool_ops {
> ...
>    int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
>    int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
> };
> 
> The same thing required for EEE.
Yeh :(

> 
> I am not sure about moving the autoneg and duplex fields into the new
> struct. Especially the "duplex" field.
I think so too. ethtool user space will call ETHTOOL_[GS]SET and after
that ETHTOOL_[GS]LINK_MODE (if supported). No need to get the
duplex/autoneg fields again.

> 
> Then the idea would be to update the ethtool user-space tool to try
> get/set_link mode when reporting/changing the autoneg/advertising
> settings.
> 
> Both will require significant effort from the driver authors.
> Especially if the variable-length bitmap approach is preferred:
>  - most drivers currently use simple bitwise arithmetic in their code,
> and that goes far beyond get/set_settings, it is sometimes part of the
> core driver logic. They will have to migrate to the bitmap API if they
> want to use the larger bitmaps (note: no change needed if they are
> happy with <= 32b / mask)
As I said above, it will save as doing this work again in the future,
and more problematic, save another version to backport in the future. In
addition, not all drivers will have to do it, only if >32 link speeds is
needed - this work will be required.

>  - we would have to progressively deprecate the use of #define
> ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.
Maybe we could use some macro juggling to define the legacy macro's
using enum for the first 32 bits, and fail the compilation if used on
>32. For example, calling this:
DEFINE_LINK_MODE(ADVERTISED_1000baseT_Full, 5)

Will add the following:
ADVERTISED_1000baseT_Full_SHIFT = 5
ADVERTISED_1000baseT_Full = (1<<5)

DEFINE_LINK_MODE(ADVERTISED_100000baseKR5_Full, 50) will add:
ADVERTISED_100000baseKR5_Full_SHIFT = 50
ADVERTISED_100000baseKR5_Full = #error new link speeds must be defined
using [gs]et_link_speed

This will break compilation if ADVERTISED_100000baseKR5_Full is used in
[gs]et_settings (I know the '#error' will not print something very
pretty - I used it only to explain what I meant)

> 
> Any feedback welcome. In the meantime, I am going to propose a v3 of
> current option with 53 bits / mask. I can also propose a prototype of
> the scheme described above, please let me know.
I think that it is better to do it once, and skip the 53 bits / mask
version.
I'll be happy to assist.

Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-08  8:40       ` Amir Vadai
  0 siblings, 0 replies; 24+ messages in thread
From: Amir Vadai @ 2015-01-08  8:40 UTC (permalink / raw)
  To: David Decotigny
  Cc: Florian Fainelli, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA, Saeed Mahameed,
	David S. Miller, Jason Wang, Michael S. Tsirkin, Herbert Xu,
	Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang, Neil Horman,
	WANG Cong, Flavio Leitner, Tom Gundersen, Jiri Pirko,
	Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

On 1/6/2015 7:36 PM, David Decotigny wrote:
> Interesting. It seems that the band-aid I was proposing is already
> obsolete. We could still use the remaining reserved 16 bits to encode
> 5 more bits per mask (that is: 53 bits / mask total). But if I
> understand you, it would allow us to survive only a few months longer,
> as opposed to a few weeks.
> 
> One short-term alternative solution I can imagine is the following:
> /* For example bitmap-based for variable length: */
> struct ethtool_link_mode {
>   __u32 cmd;
>   __u8 autoneg :1;
>   __u8 duplex :2;
>  __u16 supported_nbits;
>   __u16 advertising_nbits;
>   __u16 lp_advertising_nbits;
>   __u32 reserved[4];
>   __u8 masks[0];
> };
> /* Or simpler, statically limited to 64b / mask, but easier to migrate
> to for driver authors: */
I think the first options is better. A driver will have to do changes in
order to support >32 link modes, so better change it once now, without
having to change it again for >64 link modes.

> struct ethtool_link_mode {
>   __u32 cmd;
>   __u8 autoneg :1;
>   __u8 duplex :2;
>    __u64 supported;
>   __u64 advertising;
>   __u64 lp_advertising;
>   __u32 reserved[4];
> };
> #define ETHTOOL_GLINK_MODE 0x0000004a
> #define ETHTOOL_SLINK_MODE 0x0000004b
> struct ethtool_ops {
> ...
>    int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
>    int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
> };
> 
> The same thing required for EEE.
Yeh :(

> 
> I am not sure about moving the autoneg and duplex fields into the new
> struct. Especially the "duplex" field.
I think so too. ethtool user space will call ETHTOOL_[GS]SET and after
that ETHTOOL_[GS]LINK_MODE (if supported). No need to get the
duplex/autoneg fields again.

> 
> Then the idea would be to update the ethtool user-space tool to try
> get/set_link mode when reporting/changing the autoneg/advertising
> settings.
> 
> Both will require significant effort from the driver authors.
> Especially if the variable-length bitmap approach is preferred:
>  - most drivers currently use simple bitwise arithmetic in their code,
> and that goes far beyond get/set_settings, it is sometimes part of the
> core driver logic. They will have to migrate to the bitmap API if they
> want to use the larger bitmaps (note: no change needed if they are
> happy with <= 32b / mask)
As I said above, it will save as doing this work again in the future,
and more problematic, save another version to backport in the future. In
addition, not all drivers will have to do it, only if >32 link speeds is
needed - this work will be required.

>  - we would have to progressively deprecate the use of #define
> ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.
Maybe we could use some macro juggling to define the legacy macro's
using enum for the first 32 bits, and fail the compilation if used on
>32. For example, calling this:
DEFINE_LINK_MODE(ADVERTISED_1000baseT_Full, 5)

Will add the following:
ADVERTISED_1000baseT_Full_SHIFT = 5
ADVERTISED_1000baseT_Full = (1<<5)

DEFINE_LINK_MODE(ADVERTISED_100000baseKR5_Full, 50) will add:
ADVERTISED_100000baseKR5_Full_SHIFT = 50
ADVERTISED_100000baseKR5_Full = #error new link speeds must be defined
using [gs]et_link_speed

This will break compilation if ADVERTISED_100000baseKR5_Full is used in
[gs]et_settings (I know the '#error' will not print something very
pretty - I used it only to explain what I meant)

> 
> Any feedback welcome. In the meantime, I am going to propose a v3 of
> current option with 53 bits / mask. I can also propose a prototype of
> the scheme described above, please let me know.
I think that it is better to do it once, and skip the 53 bits / mask
version.
I'll be happy to assist.

Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
  2015-01-08  8:40       ` Amir Vadai
  (?)
@ 2015-01-11 22:49         ` David Decotigny
  -1 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-11 22:49 UTC (permalink / raw)
  To: Amir Vadai
  Cc: Florian Fainelli, netdev, linux-kernel, linux-api,
	Saeed Mahameed, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

Thanks for the input. Please ignore this patch series: I'm preparing a
new version: new commands, bitmap-based that should allow us to live
happily ever after, should take your feedback into account. Will send
an RFC patch series in the next hours/days.

On Thu, Jan 8, 2015 at 12:40 AM, Amir Vadai <amirv@mellanox.com> wrote:
> On 1/6/2015 7:36 PM, David Decotigny wrote:
>> Interesting. It seems that the band-aid I was proposing is already
>> obsolete. We could still use the remaining reserved 16 bits to encode
>> 5 more bits per mask (that is: 53 bits / mask total). But if I
>> understand you, it would allow us to survive only a few months longer,
>> as opposed to a few weeks.
>>
>> One short-term alternative solution I can imagine is the following:
>> /* For example bitmap-based for variable length: */
>> struct ethtool_link_mode {
>>   __u32 cmd;
>>   __u8 autoneg :1;
>>   __u8 duplex :2;
>>  __u16 supported_nbits;
>>   __u16 advertising_nbits;
>>   __u16 lp_advertising_nbits;
>>   __u32 reserved[4];
>>   __u8 masks[0];
>> };
>> /* Or simpler, statically limited to 64b / mask, but easier to migrate
>> to for driver authors: */
> I think the first options is better. A driver will have to do changes in
> order to support >32 link modes, so better change it once now, without
> having to change it again for >64 link modes.
>
>> struct ethtool_link_mode {
>>   __u32 cmd;
>>   __u8 autoneg :1;
>>   __u8 duplex :2;
>>    __u64 supported;
>>   __u64 advertising;
>>   __u64 lp_advertising;
>>   __u32 reserved[4];
>> };
>> #define ETHTOOL_GLINK_MODE 0x0000004a
>> #define ETHTOOL_SLINK_MODE 0x0000004b
>> struct ethtool_ops {
>> ...
>>    int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
>>    int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
>> };
>>
>> The same thing required for EEE.
> Yeh :(
>
>>
>> I am not sure about moving the autoneg and duplex fields into the new
>> struct. Especially the "duplex" field.
> I think so too. ethtool user space will call ETHTOOL_[GS]SET and after
> that ETHTOOL_[GS]LINK_MODE (if supported). No need to get the
> duplex/autoneg fields again.
>
>>
>> Then the idea would be to update the ethtool user-space tool to try
>> get/set_link mode when reporting/changing the autoneg/advertising
>> settings.
>>
>> Both will require significant effort from the driver authors.
>> Especially if the variable-length bitmap approach is preferred:
>>  - most drivers currently use simple bitwise arithmetic in their code,
>> and that goes far beyond get/set_settings, it is sometimes part of the
>> core driver logic. They will have to migrate to the bitmap API if they
>> want to use the larger bitmaps (note: no change needed if they are
>> happy with <= 32b / mask)
> As I said above, it will save as doing this work again in the future,
> and more problematic, save another version to backport in the future. In
> addition, not all drivers will have to do it, only if >32 link speeds is
> needed - this work will be required.
>
>>  - we would have to progressively deprecate the use of #define
>> ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.
> Maybe we could use some macro juggling to define the legacy macro's
> using enum for the first 32 bits, and fail the compilation if used on
>>32. For example, calling this:
> DEFINE_LINK_MODE(ADVERTISED_1000baseT_Full, 5)
>
> Will add the following:
> ADVERTISED_1000baseT_Full_SHIFT = 5
> ADVERTISED_1000baseT_Full = (1<<5)
>
> DEFINE_LINK_MODE(ADVERTISED_100000baseKR5_Full, 50) will add:
> ADVERTISED_100000baseKR5_Full_SHIFT = 50
> ADVERTISED_100000baseKR5_Full = #error new link speeds must be defined
> using [gs]et_link_speed
>
> This will break compilation if ADVERTISED_100000baseKR5_Full is used in
> [gs]et_settings (I know the '#error' will not print something very
> pretty - I used it only to explain what I meant)
>
>>
>> Any feedback welcome. In the meantime, I am going to propose a v3 of
>> current option with 53 bits / mask. I can also propose a prototype of
>> the scheme described above, please let me know.
> I think that it is better to do it once, and skip the 53 bits / mask
> version.
> I'll be happy to assist.
>
> Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-11 22:49         ` David Decotigny
  0 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-11 22:49 UTC (permalink / raw)
  To: Amir Vadai
  Cc: Florian Fainelli, netdev, linux-kernel, linux-api,
	Saeed Mahameed, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

Thanks for the input. Please ignore this patch series: I'm preparing a
new version: new commands, bitmap-based that should allow us to live
happily ever after, should take your feedback into account. Will send
an RFC patch series in the next hours/days.

On Thu, Jan 8, 2015 at 12:40 AM, Amir Vadai <amirv@mellanox.com> wrote:
> On 1/6/2015 7:36 PM, David Decotigny wrote:
>> Interesting. It seems that the band-aid I was proposing is already
>> obsolete. We could still use the remaining reserved 16 bits to encode
>> 5 more bits per mask (that is: 53 bits / mask total). But if I
>> understand you, it would allow us to survive only a few months longer,
>> as opposed to a few weeks.
>>
>> One short-term alternative solution I can imagine is the following:
>> /* For example bitmap-based for variable length: */
>> struct ethtool_link_mode {
>>   __u32 cmd;
>>   __u8 autoneg :1;
>>   __u8 duplex :2;
>>  __u16 supported_nbits;
>>   __u16 advertising_nbits;
>>   __u16 lp_advertising_nbits;
>>   __u32 reserved[4];
>>   __u8 masks[0];
>> };
>> /* Or simpler, statically limited to 64b / mask, but easier to migrate
>> to for driver authors: */
> I think the first options is better. A driver will have to do changes in
> order to support >32 link modes, so better change it once now, without
> having to change it again for >64 link modes.
>
>> struct ethtool_link_mode {
>>   __u32 cmd;
>>   __u8 autoneg :1;
>>   __u8 duplex :2;
>>    __u64 supported;
>>   __u64 advertising;
>>   __u64 lp_advertising;
>>   __u32 reserved[4];
>> };
>> #define ETHTOOL_GLINK_MODE 0x0000004a
>> #define ETHTOOL_SLINK_MODE 0x0000004b
>> struct ethtool_ops {
>> ...
>>    int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
>>    int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
>> };
>>
>> The same thing required for EEE.
> Yeh :(
>
>>
>> I am not sure about moving the autoneg and duplex fields into the new
>> struct. Especially the "duplex" field.
> I think so too. ethtool user space will call ETHTOOL_[GS]SET and after
> that ETHTOOL_[GS]LINK_MODE (if supported). No need to get the
> duplex/autoneg fields again.
>
>>
>> Then the idea would be to update the ethtool user-space tool to try
>> get/set_link mode when reporting/changing the autoneg/advertising
>> settings.
>>
>> Both will require significant effort from the driver authors.
>> Especially if the variable-length bitmap approach is preferred:
>>  - most drivers currently use simple bitwise arithmetic in their code,
>> and that goes far beyond get/set_settings, it is sometimes part of the
>> core driver logic. They will have to migrate to the bitmap API if they
>> want to use the larger bitmaps (note: no change needed if they are
>> happy with <= 32b / mask)
> As I said above, it will save as doing this work again in the future,
> and more problematic, save another version to backport in the future. In
> addition, not all drivers will have to do it, only if >32 link speeds is
> needed - this work will be required.
>
>>  - we would have to progressively deprecate the use of #define
>> ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.
> Maybe we could use some macro juggling to define the legacy macro's
> using enum for the first 32 bits, and fail the compilation if used on
>>32. For example, calling this:
> DEFINE_LINK_MODE(ADVERTISED_1000baseT_Full, 5)
>
> Will add the following:
> ADVERTISED_1000baseT_Full_SHIFT = 5
> ADVERTISED_1000baseT_Full = (1<<5)
>
> DEFINE_LINK_MODE(ADVERTISED_100000baseKR5_Full, 50) will add:
> ADVERTISED_100000baseKR5_Full_SHIFT = 50
> ADVERTISED_100000baseKR5_Full = #error new link speeds must be defined
> using [gs]et_link_speed
>
> This will break compilation if ADVERTISED_100000baseKR5_Full is used in
> [gs]et_settings (I know the '#error' will not print something very
> pretty - I used it only to explain what I meant)
>
>>
>> Any feedback welcome. In the meantime, I am going to propose a v3 of
>> current option with 53 bits / mask. I can also propose a prototype of
>> the scheme described above, please let me know.
> I think that it is better to do it once, and skip the 53 bits / mask
> version.
> I'll be happy to assist.
>
> Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
@ 2015-01-11 22:49         ` David Decotigny
  0 siblings, 0 replies; 24+ messages in thread
From: David Decotigny @ 2015-01-11 22:49 UTC (permalink / raw)
  To: Amir Vadai
  Cc: Florian Fainelli, netdev, linux-kernel, linux-api,
	Saeed Mahameed, David S. Miller, Jason Wang, Michael S. Tsirkin,
	Herbert Xu, Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang,
	Neil Horman, WANG Cong, Flavio Leitner, Tom Gundersen,
	Jiri Pirko, Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
	Govindarajulu Varadarajan

Thanks for the input. Please ignore this patch series: I'm preparing a
new version: new commands, bitmap-based that should allow us to live
happily ever after, should take your feedback into account. Will send
an RFC patch series in the next hours/days.

On Thu, Jan 8, 2015 at 12:40 AM, Amir Vadai <amirv@mellanox.com> wrote:
> On 1/6/2015 7:36 PM, David Decotigny wrote:
>> Interesting. It seems that the band-aid I was proposing is already
>> obsolete. We could still use the remaining reserved 16 bits to encode
>> 5 more bits per mask (that is: 53 bits / mask total). But if I
>> understand you, it would allow us to survive only a few months longer,
>> as opposed to a few weeks.
>>
>> One short-term alternative solution I can imagine is the following:
>> /* For example bitmap-based for variable length: */
>> struct ethtool_link_mode {
>>   __u32 cmd;
>>   __u8 autoneg :1;
>>   __u8 duplex :2;
>>  __u16 supported_nbits;
>>   __u16 advertising_nbits;
>>   __u16 lp_advertising_nbits;
>>   __u32 reserved[4];
>>   __u8 masks[0];
>> };
>> /* Or simpler, statically limited to 64b / mask, but easier to migrate
>> to for driver authors: */
> I think the first options is better. A driver will have to do changes in
> order to support >32 link modes, so better change it once now, without
> having to change it again for >64 link modes.
>
>> struct ethtool_link_mode {
>>   __u32 cmd;
>>   __u8 autoneg :1;
>>   __u8 duplex :2;
>>    __u64 supported;
>>   __u64 advertising;
>>   __u64 lp_advertising;
>>   __u32 reserved[4];
>> };
>> #define ETHTOOL_GLINK_MODE 0x0000004a
>> #define ETHTOOL_SLINK_MODE 0x0000004b
>> struct ethtool_ops {
>> ...
>>    int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
>>    int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
>> };
>>
>> The same thing required for EEE.
> Yeh :(
>
>>
>> I am not sure about moving the autoneg and duplex fields into the new
>> struct. Especially the "duplex" field.
> I think so too. ethtool user space will call ETHTOOL_[GS]SET and after
> that ETHTOOL_[GS]LINK_MODE (if supported). No need to get the
> duplex/autoneg fields again.
>
>>
>> Then the idea would be to update the ethtool user-space tool to try
>> get/set_link mode when reporting/changing the autoneg/advertising
>> settings.
>>
>> Both will require significant effort from the driver authors.
>> Especially if the variable-length bitmap approach is preferred:
>>  - most drivers currently use simple bitwise arithmetic in their code,
>> and that goes far beyond get/set_settings, it is sometimes part of the
>> core driver logic. They will have to migrate to the bitmap API if they
>> want to use the larger bitmaps (note: no change needed if they are
>> happy with <= 32b / mask)
> As I said above, it will save as doing this work again in the future,
> and more problematic, save another version to backport in the future. In
> addition, not all drivers will have to do it, only if >32 link speeds is
> needed - this work will be required.
>
>>  - we would have to progressively deprecate the use of #define
>> ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.
> Maybe we could use some macro juggling to define the legacy macro's
> using enum for the first 32 bits, and fail the compilation if used on
>>32. For example, calling this:
> DEFINE_LINK_MODE(ADVERTISED_1000baseT_Full, 5)
>
> Will add the following:
> ADVERTISED_1000baseT_Full_SHIFT = 5
> ADVERTISED_1000baseT_Full = (1<<5)
>
> DEFINE_LINK_MODE(ADVERTISED_100000baseKR5_Full, 50) will add:
> ADVERTISED_100000baseKR5_Full_SHIFT = 50
> ADVERTISED_100000baseKR5_Full = #error new link speeds must be defined
> using [gs]et_link_speed
>
> This will break compilation if ADVERTISED_100000baseKR5_Full is used in
> [gs]et_settings (I know the '#error' will not print something very
> pretty - I used it only to explain what I meant)
>
>>
>> Any feedback welcome. In the meantime, I am going to propose a v3 of
>> current option with 53 bits / mask. I can also propose a prototype of
>> the scheme described above, please let me know.
> I think that it is better to do it once, and skip the 53 bits / mask
> version.
> I'll be happy to assist.
>
> Amir

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2015-01-11 22:50 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-06  2:54 [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits David Decotigny
2015-01-06  2:54 ` [PATCH net-next v2 1/8] net: ethtool: internal compatibility flags to reject non-zero reserved fields David Decotigny
2015-01-06  2:54   ` David Decotigny
2015-01-06  2:54 ` [PATCH net-next v2 2/8] net: ethtool: extend link mode support to 48 bits David Decotigny
2015-01-06  2:54 ` [PATCH net-next v2 3/8] net: phy: " David Decotigny
2015-01-06  2:54   ` David Decotigny
2015-01-06  2:54 ` [PATCH net-next v2 4/8] net: mii: " David Decotigny
2015-01-06  2:54 ` [PATCH net-next v2 5/8] net: mdio: " David Decotigny
2015-01-06  2:54 ` [PATCH net-next v2 6/8] net: veth: " David Decotigny
2015-01-06  2:54 ` [PATCH net-next v2 7/8] net: tun: " David Decotigny
2015-01-06  2:54 ` [PATCH net-next v2 8/8] net: mlx4_en: " David Decotigny
2015-01-06 13:56 ` [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps " Amir Vadai
2015-01-06 17:36   ` David Decotigny
2015-01-06 17:36     ` David Decotigny
2015-01-06 17:36     ` David Decotigny
2015-01-08  8:40     ` Amir Vadai
2015-01-08  8:40       ` Amir Vadai
2015-01-08  8:40       ` Amir Vadai
2015-01-11 22:49       ` David Decotigny
2015-01-11 22:49         ` David Decotigny
2015-01-11 22:49         ` David Decotigny
2015-01-06 22:29   ` David Miller
2015-01-06 22:29     ` David Miller
2015-01-06 23:08     ` Ben Hutchings

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.