[PATCH net-next 0/3] ethtool: support for forward error correction mode setting on a link

[PATCH net-next 0/3] ethtool: support for forward error correction mode setting on a link

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

Forward Error Correction (FEC) modes i.e Base-R
and Reed-Solomon modes are introduced in 25G/40G/100G standards
for providing good BER at high speeds. Various networking devices
which support 25G/40G/100G provides ability to manage supported FEC
modes and the lack of FEC encoding control and reporting today is a
source for itneroperability issues for many vendors.
FEC capability as well as specific FEC mode i.e. Base-R
or RS modes can be requested or advertised through bits D44:47 of base link
codeword.

This patch set intends to provide option under ethtool to manage and report
FEC encoding settings for networking devices as per IEEE 802.3 bj, bm and by
specs.

Casey Leedom (2):
  cxgb4: core hardware/firmware support for Forward Error Correction on
    a link
  cxgb4: ethtool forward error correction management support

Vidya Sagar Ravipati (1):
  net: ethtool: add support for forward error correction modes

 drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c |  99 ++++++++++++++
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c         | 151 ++++++++++++++++-----
 include/linux/ethtool.h                            |   4 +
 include/uapi/linux/ethtool.h                       |  48 ++++++-
 net/core/ethtool.c                                 |  34 +++++
 5 files changed, 300 insertions(+), 36 deletions(-)

-- 
2.1.4

[PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Roopa Prabhu]

From: Vidya Sagar Ravipati <vidya.chowdary@gmail.com>

Forward Error Correction (FEC) modes i.e Base-R
and Reed-Solomon modes are introduced in 25G/40G/100G standards
for providing good BER at high speeds. Various networking devices
which support 25G/40G/100G provides ability to manage supported FEC
modes and the lack of FEC encoding control and reporting today is a
source for itneroperability issues for many vendors.
FEC capability as well as specific FEC mode i.e. Base-R
or RS modes can be requested or advertised through bits D44:47 of
base link codeword.

This patch set intends to provide option under ethtool to manage
and report FEC encoding settings for networking devices as per
IEEE 802.3 bj, bm and by specs.

set-fec/show-fec option(s) are designed to provide control and
report the FEC encoding on the link.

SET FEC option:
root@tor: ethtool --set-fec  swp1 encoding [off | RS | BaseR | auto]

Encoding: Types of encoding
Off    :  Turning off any encoding
RS     :  enforcing RS-FEC encoding on supported speeds
BaseR  :  enforcing Base R encoding on supported speeds
Auto   :  IEEE defaults for the speed/medium combination

Here are a few examples of what we would expect if encoding=auto:
- if autoneg is on, we are  expecting FEC to be negotiated as on or off
  as long as protocol supports it
- if the hardware is capable of detecting the FEC encoding on it's
      receiver it will reconfigure its encoder to match
- in absence of the above, the configuration would be set to IEEE
  defaults.

>From our  understanding , this is essentially what most hardware/driver
combinations are doing today in the absence of a way for users to
control the behavior.

SHOW FEC option:
root@tor: ethtool --show-fec  swp1
FEC parameters for swp1:
Active FEC encodings: RS
Configured FEC encodings:  RS | BaseR

ETHTOOL DEVNAME output modification:

ethtool devname output:
root@tor:~# ethtool swp1
Settings for swp1:
root@hpe-7712-03:~# ethtool swp18
Settings for swp18:
    Supported ports: [ FIBRE ]
    Supported link modes:   40000baseCR4/Full
                            40000baseSR4/Full
                            40000baseLR4/Full
                            100000baseSR4/Full
                            100000baseCR4/Full
                            100000baseLR4_ER4/Full
    Supported pause frame use: No
    Supports auto-negotiation: Yes
    Supported FEC modes: [RS | BaseR | None | Not reported]
    Advertised link modes:  Not reported
    Advertised pause frame use: No
    Advertised auto-negotiation: No
    Advertised FEC modes: [RS | BaseR | None | Not reported]
<<<< One or more FEC modes
    Speed: 100000Mb/s
    Duplex: Full
    Port: FIBRE
    PHYAD: 106
    Transceiver: internal
    Auto-negotiation: off
    Link detected: yes

This patch includes following changes
a) New ETHTOOL_SFECPARAM/SFECPARAM API, handled by
  the new get_fecparam/set_fecparam callbacks, provides support
  for configuration of forward error correction modes.
b) Link mode bits for FEC modes i.e. None (No FEC mode), RS, BaseR/FC
  are defined so that users can configure these fec modes for supported
  and advertising fields as part of link autonegotiation.

Signed-off-by: Vidya Sagar Ravipati <vidya.chowdary@gmail.com>
---
 include/linux/ethtool.h      |  4 ++++
 include/uapi/linux/ethtool.h | 48 +++++++++++++++++++++++++++++++++++++++++++-
 net/core/ethtool.c           | 34 +++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 83cc986..afdbb70 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -374,5 +374,9 @@ struct ethtool_ops {
 				      struct ethtool_link_ksettings *);
 	int	(*set_link_ksettings)(struct net_device *,
 				      const struct ethtool_link_ksettings *);
+	int	(*get_fecparam)(struct net_device *,
+				      struct ethtool_fecparam *);
+	int	(*set_fecparam)(struct net_device *,
+				      struct ethtool_fecparam *);
 };
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 7d4a594..9c041da 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1238,6 +1238,47 @@ struct ethtool_per_queue_op {
 	char	data[];
 };
 
+/**
+ * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
+ * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
+ * @active_fec: FEC mode which is active on porte
+ * @fec: Bitmask of supported/configured FEC modes
+ * @rsvd: Reserved for future extensions. i.e FEC bypass feature.
+ *
+ * Drivers should reject a non-zero setting of @autoneg when
+ * autoneogotiation is disabled (or not supported) for the link.
+ *
+ */
+struct ethtool_fecparam {
+	__u32   cmd;
+	/* bitmask of FEC modes */
+	__u32   active_fec;
+	__u32   fec;
+	__u32   reserved;
+};
+
+/**
+ * enum ethtool_fec_config_bits - flags definition of ethtool_fec_configuration
+ * @ETHTOOL_FEC_NONE: FEC mode configuration is not supported
+ * @ETHTOOL_FEC_AUTO: Default/Best FEC mode provided by driver
+ * @ETHTOOL_FEC_OFF: No FEC Mode
+ * @ETHTOOL_FEC_RS: Reed-Solomon Forward Error Detection mode
+ * @ETHTOOL_FEC_BASER: Base-R/Reed-Solomon Forward Error Detection mode
+ */
+enum ethtool_fec_config_bits {
+	ETHTOOL_FEC_NONE_BIT,
+	ETHTOOL_FEC_AUTO_BIT,
+	ETHTOOL_FEC_OFF_BIT,
+	ETHTOOL_FEC_RS_BIT,
+	ETHTOOL_FEC_BASER_BIT,
+};
+
+#define ETHTOOL_FEC_NONE		(1 << ETHTOOL_FEC_NONE_BIT)
+#define ETHTOOL_FEC_AUTO		(1 << ETHTOOL_FEC_AUTO_BIT)
+#define ETHTOOL_FEC_OFF			(1 << ETHTOOL_FEC_OFF_BIT)
+#define ETHTOOL_FEC_RS			(1 << ETHTOOL_FEC_RS_BIT)
+#define ETHTOOL_FEC_BASER		(1 << ETHTOOL_FEC_BASER_BIT)
+
 /* CMDs currently supported */
 #define ETHTOOL_GSET		0x00000001 /* DEPRECATED, Get settings.
 					    * Please use ETHTOOL_GLINKSETTINGS
@@ -1330,6 +1371,8 @@ struct ethtool_per_queue_op {
 #define ETHTOOL_SLINKSETTINGS	0x0000004d /* Set ethtool_link_settings */
 #define ETHTOOL_PHY_GTUNABLE	0x0000004e /* Get PHY tunable configuration */
 #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
+#define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
+#define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
@@ -1387,6 +1430,9 @@ enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_2500baseT_Full_BIT	= 47,
 	ETHTOOL_LINK_MODE_5000baseT_Full_BIT	= 48,
 
+	ETHTOOL_LINK_MODE_FEC_NONE_BIT	= 49,
+	ETHTOOL_LINK_MODE_FEC_RS_BIT	= 50,
+	ETHTOOL_LINK_MODE_FEC_BASER_BIT	= 51,
 
 	/* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
 	 * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
@@ -1395,7 +1441,7 @@ enum ethtool_link_mode_bit_indices {
 	 */
 
 	__ETHTOOL_LINK_MODE_LAST
-	  = ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+	  = ETHTOOL_LINK_MODE_FEC_BASER_BIT,
 };
 
 #define __ETHTOOL_LINK_MODE_LEGACY_MASK(base_name)	\
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 03111a2..559b9c4 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -2523,6 +2523,33 @@ static int set_phy_tunable(struct net_device *dev, void __user *useraddr)
 	return ret;
 }
 
+static int ethtool_get_fecparam(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_fecparam fecparam = { ETHTOOL_GFECPARAM };
+
+	if (!dev->ethtool_ops->get_fecparam)
+		return -EOPNOTSUPP;
+
+	dev->ethtool_ops->get_fecparam(dev, &fecparam);
+
+	if (copy_to_user(useraddr, &fecparam, sizeof(fecparam)))
+		return -EFAULT;
+	return 0;
+}
+
+static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_fecparam fecparam;
+
+	if (!dev->ethtool_ops->set_fecparam)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
+		return -EFAULT;
+
+	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
+}
+
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -2582,6 +2609,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GTUNABLE:
 	case ETHTOOL_PHY_GTUNABLE:
 	case ETHTOOL_GLINKSETTINGS:
+	case ETHTOOL_GFECPARAM:
 		break;
 	default:
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
@@ -2793,6 +2821,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_PHY_STUNABLE:
 		rc = set_phy_tunable(dev, useraddr);
 		break;
+	case ETHTOOL_GFECPARAM:
+		rc = ethtool_get_fecparam(dev, useraddr);
+		break;
+	case ETHTOOL_SFECPARAM:
+		rc = ethtool_set_fecparam(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
2.1.4

[PATCH net-next 2/3] cxgb4: core hardware/firmware support for Forward Error Correction on a link

[Roopa Prabhu]

From: Casey Leedom <leedom@chelsio.com>

Signed-off-by: Casey Leedom <leedom@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 152 ++++++++++++++++++++++-------
 1 file changed, 117 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index d5e316d..095894e 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -3690,11 +3690,64 @@ void t4_ulprx_read_la(struct adapter *adap, u32 *la_buf)
 		     FW_PORT_CAP_SPEED_40G | FW_PORT_CAP_SPEED_100G | \
 		     FW_PORT_CAP_ANEG)
 
+/* Translate Firmware Port Capabilities Pause specification to Common Code */
+static inline unsigned int fwcap_to_cc_pause(unsigned int fw_pause)
+{
+	unsigned int cc_pause = 0;
+
+	if (fw_pause & FW_PORT_CAP_FC_RX)
+		cc_pause |= PAUSE_RX;
+	if (fw_pause & FW_PORT_CAP_FC_TX)
+		cc_pause |= PAUSE_TX;
+
+	return cc_pause;
+}
+
+/* Translate Common Code Pause specification into Firmware Port Capabilities */
+static inline unsigned int cc_to_fwcap_pause(unsigned int cc_pause)
+{
+	unsigned int fw_pause = 0;
+
+	if (cc_pause & PAUSE_RX)
+		fw_pause |= FW_PORT_CAP_FC_RX;
+	if (cc_pause & PAUSE_TX)
+		fw_pause |= FW_PORT_CAP_FC_TX;
+
+	return fw_pause;
+}
+
+/* Translate Firmware Forward Error Correction specification to Common Code */
+static inline unsigned int fwcap_to_cc_fec(unsigned int fw_fec)
+{
+	unsigned int cc_fec = 0;
+
+	if (fw_fec & FW_PORT_CAP_FEC_RS)
+		cc_fec |= FEC_RS;
+	if (fw_fec & FW_PORT_CAP_FEC_BASER_RS)
+		cc_fec |= FEC_BASER_RS;
+
+	return cc_fec;
+}
+
+/* Translate Common Code Forward Error Correction specification to Firmware */
+static inline unsigned int cc_to_fwcap_fec(unsigned int cc_fec)
+{
+	unsigned int fw_fec = 0;
+
+	if (cc_fec & FEC_RS)
+		fw_fec |= FW_PORT_CAP_FEC_RS;
+	if (cc_fec & FEC_BASER_RS)
+		fw_fec |= FW_PORT_CAP_FEC_BASER_RS;
+
+	return fw_fec;
+}
+
 /**
  *	t4_link_l1cfg - apply link configuration to MAC/PHY
- *	@phy: the PHY to setup
- *	@mac: the MAC to setup
- *	@lc: the requested link configuration
+ *	@adapter: the adapter
+ *	@mbox: the Firmware Mailbox to use
+ *	@port: the Port ID
+ *	@lc: the Port's Link Configuration
  *
  *	Set up a port's MAC and PHY according to a desired link configuration.
  *	- If the PHY can auto-negotiate first decide what to advertise, then
@@ -3707,22 +3760,46 @@ int t4_link_l1cfg(struct adapter *adap, unsigned int mbox, unsigned int port,
 		  struct link_config *lc)
 {
 	struct fw_port_cmd c;
-	unsigned int mdi = FW_PORT_CAP_MDI_V(FW_PORT_CAP_MDI_AUTO);
-	unsigned int fc = 0, fec = 0, fw_fec = 0;
+	unsigned int fw_mdi = FW_PORT_CAP_MDI_V(FW_PORT_CAP_MDI_AUTO);
+	unsigned int fw_fc, cc_fec, fw_fec;
+	unsigned int rcap;
 
 	lc->link_ok = 0;
-	if (lc->requested_fc & PAUSE_RX)
-		fc |= FW_PORT_CAP_FC_RX;
-	if (lc->requested_fc & PAUSE_TX)
-		fc |= FW_PORT_CAP_FC_TX;
 
-	fec = lc->requested_fec & FEC_AUTO ? lc->auto_fec : lc->requested_fec;
+	/* Convert driver coding of Pause Frame Flow Control settings into the
+	 * Firmware's API.
+	 */
+	fw_fc = cc_to_fwcap_pause(lc->requested_fc);
+
+	/* Convert Common Code Forward Error Control settings into the
+	 * Firmware's API.  If the current Requested FEC has "Automatic"
+	 * (IEEE 802.3) specified, then we use whatever the Firmware
+	 * sent us as part of it's IEEE 802.3-based interpratation of
+	 * the Transceiver Module EPROM FEC parameters.  Otherwise we
+	 * use whatever is in the current Requested FEC settings.
+	 */
+	if (lc->requested_fec & FEC_AUTO)
+		cc_fec = lc->auto_fec;
+	else
+		cc_fec = lc->requested_fec;
+	fw_fec = cc_to_fwcap_fec(cc_fec);
 
-	if (fec & FEC_RS)
-		fw_fec |= FW_PORT_CAP_FEC_RS;
-	if (fec & FEC_BASER_RS)
-		fw_fec |= FW_PORT_CAP_FEC_BASER_RS;
+	/* Figure out what our Requested Port Capabilities are going to be.
+	 */
+	if (!(lc->supported & FW_PORT_CAP_ANEG)) {
+		rcap = (lc->supported & ADVERT_MASK) | fw_fc | fw_fec;
+		lc->fc = lc->requested_fc & (PAUSE_RX | PAUSE_TX);
+		lc->fec = cc_fec;
+	} else if (lc->autoneg == AUTONEG_DISABLE) {
+		rcap = lc->requested_speed | fw_fc | fw_fec | fw_mdi;
+		lc->fc = lc->requested_fc & (PAUSE_RX | PAUSE_TX);
+		lc->fec = cc_fec;
+	} else {
+		rcap = lc->advertising | fw_fc | fw_fec | fw_mdi;
+	}
 
+	/* And send that on to the Firmware ...
+	 */
 	memset(&c, 0, sizeof(c));
 	c.op_to_portid = cpu_to_be32(FW_CMD_OP_V(FW_PORT_CMD) |
 				     FW_CMD_REQUEST_F | FW_CMD_EXEC_F |
@@ -3730,19 +3807,7 @@ int t4_link_l1cfg(struct adapter *adap, unsigned int mbox, unsigned int port,
 	c.action_to_len16 =
 		cpu_to_be32(FW_PORT_CMD_ACTION_V(FW_PORT_ACTION_L1_CFG) |
 			    FW_LEN16(c));
-
-	if (!(lc->supported & FW_PORT_CAP_ANEG)) {
-		c.u.l1cfg.rcap = cpu_to_be32((lc->supported & ADVERT_MASK) |
-					     fc | fw_fec);
-		lc->fc = lc->requested_fc & (PAUSE_RX | PAUSE_TX);
-	} else if (lc->autoneg == AUTONEG_DISABLE) {
-		c.u.l1cfg.rcap = cpu_to_be32(lc->requested_speed | fc |
-					     fw_fec | mdi);
-		lc->fc = lc->requested_fc & (PAUSE_RX | PAUSE_TX);
-	} else
-		c.u.l1cfg.rcap = cpu_to_be32(lc->advertising | fc |
-					     fw_fec | mdi);
-
+	c.u.l1cfg.rcap = cpu_to_be32(rcap);
 	return t4_wr_mbox(adap, mbox, &c, sizeof(c), NULL);
 }
 
@@ -7345,19 +7410,28 @@ static const char *t4_link_down_rc_str(unsigned char link_down_rc)
 void t4_handle_get_port_info(struct port_info *pi, const __be64 *rpl)
 {
 	const struct fw_port_cmd *p = (const void *)rpl;
+	unsigned int acaps = be16_to_cpu(p->u.info.acap);
 	struct adapter *adap = pi->adapter;
 
 	/* link/module state change message */
-	int speed = 0, fc = 0;
+	int speed = 0, fc, fec;
 	struct link_config *lc;
 	u32 stat = be32_to_cpu(p->u.info.lstatus_to_modtype);
 	int link_ok = (stat & FW_PORT_CMD_LSTATUS_F) != 0;
 	u32 mod = FW_PORT_CMD_MODTYPE_G(stat);
 
+	/* Unfortunately the format of the Link Status returned by the
+	 * Firmware isn't the same as the Firmware Port Capabilities bitfield
+	 * used everywhere else ...
+	 */
+	fc = 0;
 	if (stat & FW_PORT_CMD_RXPAUSE_F)
 		fc |= PAUSE_RX;
 	if (stat & FW_PORT_CMD_TXPAUSE_F)
 		fc |= PAUSE_TX;
+
+	fec = fwcap_to_cc_fec(acaps);
+
 	if (stat & FW_PORT_CMD_LSPEED_V(FW_PORT_CAP_SPEED_100M))
 		speed = 100;
 	else if (stat & FW_PORT_CMD_LSPEED_V(FW_PORT_CAP_SPEED_1G))
@@ -7374,11 +7448,20 @@ void t4_handle_get_port_info(struct port_info *pi, const __be64 *rpl)
 	lc = &pi->link_cfg;
 
 	if (mod != pi->mod_type) {
+		/* When a new Transceiver Module is inserted, the Firmware
+		 * will examine any Forward Error Correction parameters
+		 * present in the Transceiver Module i2c EPROM and determine
+		 * the supported and recommended FEC settings from those
+		 * based on IEEE 802.3 standards.  We always record the
+		 * IEEE 802.3 recommended "automatic" settings.
+		 */
+		lc->auto_fec = fec;
+
 		pi->mod_type = mod;
 		t4_os_portmod_changed(adap, pi->port_id);
 	}
 	if (link_ok != lc->link_ok || speed != lc->speed ||
-	    fc != lc->fc) {	/* something changed */
+	    fc != lc->fc || fec != lc->fec) {	/* something changed */
 		if (!link_ok && lc->link_ok) {
 			unsigned char rc = FW_PORT_CMD_LINKDNRC_G(stat);
 
@@ -7390,6 +7473,8 @@ void t4_handle_get_port_info(struct port_info *pi, const __be64 *rpl)
 		lc->link_ok = link_ok;
 		lc->speed = speed;
 		lc->fc = fc;
+		lc->fec = fec;
+
 		lc->supported = be16_to_cpu(p->u.info.pcap);
 		lc->lp_advertising = be16_to_cpu(p->u.info.lpacap);
 
@@ -7479,7 +7564,8 @@ static void get_pci_mode(struct adapter *adapter, struct pci_params *p)
 /**
  *	init_link_config - initialize a link's SW state
  *	@lc: structure holding the link state
- *	@caps: link capabilities
+ *	@pcaps: link Port Capabilities
+ *	@acaps: link current Advertised Port Capabilities
  *
  *	Initializes the SW state maintained for each link, including the link's
  *	capabilities and default speed/flow-control/autonegotiation settings.
@@ -7492,15 +7578,11 @@ static void init_link_config(struct link_config *lc, unsigned int pcaps,
 	lc->requested_speed = 0;
 	lc->speed = 0;
 	lc->requested_fc = lc->fc = PAUSE_RX | PAUSE_TX;
-	lc->auto_fec = 0;
 
 	/* For Forward Error Control, we default to whatever the Firmware
 	 * tells us the Link is currently advertising.
 	 */
-	if (acaps & FW_PORT_CAP_FEC_RS)
-		lc->auto_fec |= FEC_RS;
-	if (acaps & FW_PORT_CAP_FEC_BASER_RS)
-		lc->auto_fec |= FEC_BASER_RS;
+	lc->auto_fec = fwcap_to_cc_fec(acaps);
 	lc->requested_fec = FEC_AUTO;
 	lc->fec = lc->auto_fec;
 
-- 
2.1.4

[PATCH net-next 3/3] cxgb4: ethtool forward error correction management support

[Roopa Prabhu]

From: Casey Leedom <leedom@chelsio.com>

Signed-off-by: Casey Leedom <leedom@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c | 100 +++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index e9bab72..76ea609 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -801,6 +801,104 @@ static int set_link_ksettings(struct net_device *dev,
 	return ret;
 }
 
+/* Translate the Firmware FEC value into the ethtool value. */
+static inline unsigned int fwcap_to_eth_fec(unsigned int fw_fec)
+{
+	unsigned int eth_fec = 0;
+
+	if (fw_fec & FW_PORT_CAP_FEC_RS)
+		eth_fec |= ETHTOOL_FEC_RS;
+	if (fw_fec & FW_PORT_CAP_FEC_BASER_RS)
+		eth_fec |= ETHTOOL_FEC_BASER;
+
+	/* if nothing is set, then FEC is off */
+	if (!eth_fec)
+		eth_fec = ETHTOOL_FEC_OFF;
+
+	return eth_fec;
+}
+
+/* Translate Common Code FEC value into ethtool value. */
+static inline unsigned int cc_to_eth_fec(unsigned int cc_fec)
+{
+	unsigned int eth_fec = 0;
+
+	if (cc_fec & FEC_AUTO)
+		eth_fec |= ETHTOOL_FEC_AUTO;
+	if (cc_fec & FEC_RS)
+		eth_fec |= ETHTOOL_FEC_RS;
+	if (cc_fec & FEC_BASER_RS)
+		eth_fec |= ETHTOOL_FEC_BASER;
+
+	/* if nothing is set, then FEC is off */
+	if (!eth_fec)
+		eth_fec = ETHTOOL_FEC_OFF;
+
+	return eth_fec;
+}
+
+/* Translate ethtool FEC value into Common Code value. */
+static inline unsigned int eth_to_cc_fec(unsigned int eth_fec)
+{
+	unsigned int cc_fec = 0;
+
+	if (eth_fec & ETHTOOL_FEC_OFF)
+		return cc_fec;
+
+	if (eth_fec & ETHTOOL_FEC_AUTO)
+		cc_fec |= FEC_AUTO;
+	if (eth_fec & ETHTOOL_FEC_RS)
+		cc_fec |= FEC_RS;
+	if (eth_fec & ETHTOOL_FEC_BASER)
+		cc_fec |= FEC_BASER_RS;
+
+	return cc_fec;
+}
+
+static int get_fecparam(struct net_device *dev, struct ethtool_fecparam *fec)
+{
+	const struct port_info *pi = netdev_priv(dev);
+	const struct link_config *lc = &pi->link_cfg;
+
+	/* Translate the Firmware FEC Support into the ethtool value.  We
+	 * always support IEEE 802.3 "automatic" selection of Link FEC type if
+	 * any FEC is supported.
+	 */
+	fec->fec = fwcap_to_eth_fec(lc->supported);
+	if (fec->fec != ETHTOOL_FEC_OFF)
+		fec->fec |= ETHTOOL_FEC_AUTO;
+
+	/* Translate the current internal FEC parameters into the
+	 * ethtool values.
+	 */
+	fec->active_fec = cc_to_eth_fec(lc->fec);
+
+	return 0;
+}
+
+static int set_fecparam(struct net_device *dev, struct ethtool_fecparam *fec)
+{
+	struct port_info *pi = netdev_priv(dev);
+	struct link_config *lc = &pi->link_cfg;
+	struct link_config old_lc;
+	int ret;
+
+	/* Save old Link Configuration in case the L1 Configure below
+	 * fails.
+	 */
+	old_lc = *lc;
+
+	/* Try to perform the L1 Configure and return the result of that
+	 * effort.  If it fails, revert the attempted change.
+	 */
+	lc->requested_fec = eth_to_cc_fec(fec->fec);
+	ret = t4_link_l1cfg(pi->adapter, pi->adapter->mbox,
+			    pi->tx_chan, lc);
+	if (ret)
+		*lc = old_lc;
+	return ret;
+}
+
 static void get_pauseparam(struct net_device *dev,
 			   struct ethtool_pauseparam *epause)
 {
@@ -1238,6 +1336,8 @@ static int get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
 static const struct ethtool_ops cxgb_ethtool_ops = {
 	.get_link_ksettings = get_link_ksettings,
 	.set_link_ksettings = set_link_ksettings,
+	.get_fecparam      = get_fecparam,
+	.set_fecparam      = set_fecparam,
 	.get_drvinfo       = get_drvinfo,
 	.get_msglevel      = get_msglevel,
 	.set_msglevel      = set_msglevel,
-- 
2.1.4

Re: [PATCH net-next 0/3] ethtool: support for forward error correction mode setting on a link

[Andrew Lunn]

On Sat, Jun 24, 2017 at 12:19:42PM -0700, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> Forward Error Correction (FEC) modes i.e Base-R
> and Reed-Solomon modes are introduced in 25G/40G/100G standards
> for providing good BER at high speeds. Various networking devices
> which support 25G/40G/100G provides ability to manage supported FEC
> modes and the lack of FEC encoding control and reporting today is a
> source for itneroperability issues for many vendors.

interoperability. The same typ0 exists in the first patch

> FEC capability as well as specific FEC mode i.e. Base-R
> or RS modes can be requested or advertised through bits D44:47 of base link
> codeword.

For my own education, is this logically a MAC or a PHY thing?  Is the
negotiation performed at the PHY level, and then FEC happens at the
MAC level?

I'm just trying to look ahead when we start getting 25G/40G/100G PHY
support. Does phylib need an extension, similar to how pause is
negotiated at the PHY, and then the MAC does the actual work?

	 Andrew

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Gal Pressman]

> ...
>
> SHOW FEC option:
> root@tor: ethtool --show-fec  swp1
> FEC parameters for swp1:
> Active FEC encodings: RS
> Configured FEC encodings:  RS | BaseR
>
> ETHTOOL DEVNAME output modification:
>
> ethtool devname output:
> root@tor:~# ethtool swp1
> Settings for swp1:
> root@hpe-7712-03:~# ethtool swp18
> Settings for swp18:
>     Supported ports: [ FIBRE ]
>     Supported link modes:   40000baseCR4/Full
>                             40000baseSR4/Full
>                             40000baseLR4/Full
>                             100000baseSR4/Full
>                             100000baseCR4/Full
>                             100000baseLR4_ER4/Full
>     Supported pause frame use: No
>     Supports auto-negotiation: Yes
>     Supported FEC modes: [RS | BaseR | None | Not reported]
>     Advertised link modes:  Not reported
>     Advertised pause frame use: No
>     Advertised auto-negotiation: No
>     Advertised FEC modes: [RS | BaseR | None | Not reported]
> <<<< One or more FEC modes
>     Speed: 100000Mb/s
>     Duplex: Full
>     Port: FIBRE
>     PHYAD: 106
>     Transceiver: internal
>     Auto-negotiation: off
>     Link detected: yes
What is the difference between the information in ethtool DEVNAME and ethtool --show-fec DEVNAME?
I can't find a usage of LINK_MODE_FEC_* bits in downstream patches.

>
> This patch includes following changes
> a) New ETHTOOL_SFECPARAM/SFECPARAM API, handled by
>   the new get_fecparam/set_fecparam callbacks, provides support
>   for configuration of forward error correction modes.
> b) Link mode bits for FEC modes i.e. None (No FEC mode), RS, BaseR/FC
>   are defined so that users can configure these fec modes for supported
>   and advertising fields as part of link autonegotiation.
>
> Signed-off-by: Vidya Sagar Ravipati <vidya.chowdary@gmail.com>
> ---
>  include/linux/ethtool.h      |  4 ++++
>  include/uapi/linux/ethtool.h | 48 +++++++++++++++++++++++++++++++++++++++++++-
>  net/core/ethtool.c           | 34 +++++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 83cc986..afdbb70 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -374,5 +374,9 @@ struct ethtool_ops {
>  				      struct ethtool_link_ksettings *);
>  	int	(*set_link_ksettings)(struct net_device *,
>  				      const struct ethtool_link_ksettings *);
> +	int	(*get_fecparam)(struct net_device *,
> +				      struct ethtool_fecparam *);
> +	int	(*set_fecparam)(struct net_device *,
> +				      struct ethtool_fecparam *);
>  };
>  #endif /* _LINUX_ETHTOOL_H */
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 7d4a594..9c041da 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1238,6 +1238,47 @@ struct ethtool_per_queue_op {
>  	char	data[];
>  };
>  
> +/**
> + * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
> + * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
> + * @active_fec: FEC mode which is active on porte
port.

> + * @fec: Bitmask of supported/configured FEC modes
> + * @rsvd: Reserved for future extensions. i.e FEC bypass feature.
> + *
> + * Drivers should reject a non-zero setting of @autoneg when
> + * autoneogotiation is disabled (or not supported) for the link.
Which @autoneg?

> + *
> + */
> +struct ethtool_fecparam {
> +	__u32   cmd;
> +	/* bitmask of FEC modes */
> +	__u32   active_fec;
> +	__u32   fec;
> +	__u32   reserved;
> +};
> +
> +/**
> + * enum ethtool_fec_config_bits - flags definition of ethtool_fec_configuration
> + * @ETHTOOL_FEC_NONE: FEC mode configuration is not supported
> + * @ETHTOOL_FEC_AUTO: Default/Best FEC mode provided by driver
> + * @ETHTOOL_FEC_OFF: No FEC Mode
> + * @ETHTOOL_FEC_RS: Reed-Solomon Forward Error Detection mode
> + * @ETHTOOL_FEC_BASER: Base-R/Reed-Solomon Forward Error Detection mode
> + */
> +enum ethtool_fec_config_bits {
> +	ETHTOOL_FEC_NONE_BIT,
> +	ETHTOOL_FEC_AUTO_BIT,
> +	ETHTOOL_FEC_OFF_BIT,
> +	ETHTOOL_FEC_RS_BIT,
> +	ETHTOOL_FEC_BASER_BIT,
> +};
> +
> +#define ETHTOOL_FEC_NONE		(1 << ETHTOOL_FEC_NONE_BIT)
> +#define ETHTOOL_FEC_AUTO		(1 << ETHTOOL_FEC_AUTO_BIT)
> +#define ETHTOOL_FEC_OFF			(1 << ETHTOOL_FEC_OFF_BIT)
> +#define ETHTOOL_FEC_RS			(1 << ETHTOOL_FEC_RS_BIT)
> +#define ETHTOOL_FEC_BASER		(1 << ETHTOOL_FEC_BASER_BIT)
> +
>  /* CMDs currently supported */
>  #define ETHTOOL_GSET		0x00000001 /* DEPRECATED, Get settings.
>  					    * Please use ETHTOOL_GLINKSETTINGS
> @@ -1330,6 +1371,8 @@ struct ethtool_per_queue_op {
>  #define ETHTOOL_SLINKSETTINGS	0x0000004d /* Set ethtool_link_settings */
>  #define ETHTOOL_PHY_GTUNABLE	0x0000004e /* Get PHY tunable configuration */
>  #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
> +#define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
> +#define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
>  
>  /* compatibility with older code */
>  #define SPARC_ETH_GSET		ETHTOOL_GSET
> @@ -1387,6 +1430,9 @@ enum ethtool_link_mode_bit_indices {
>  	ETHTOOL_LINK_MODE_2500baseT_Full_BIT	= 47,
>  	ETHTOOL_LINK_MODE_5000baseT_Full_BIT	= 48,
>  
> +	ETHTOOL_LINK_MODE_FEC_NONE_BIT	= 49,
> +	ETHTOOL_LINK_MODE_FEC_RS_BIT	= 50,
> +	ETHTOOL_LINK_MODE_FEC_BASER_BIT	= 51,
>  
>  	/* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
>  	 * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
> @@ -1395,7 +1441,7 @@ enum ethtool_link_mode_bit_indices {
>  	 */
>  
>  	__ETHTOOL_LINK_MODE_LAST
> -	  = ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> +	  = ETHTOOL_LINK_MODE_FEC_BASER_BIT,
>  };
>  
> ...

Re: [PATCH net-next 2/3] cxgb4: core hardware/firmware support for Forward Error Correction on a link

[David Miller]

From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Sat, 24 Jun 2017 12:19:44 -0700

> @@ -3690,11 +3690,64 @@ void t4_ulprx_read_la(struct adapter *adap, u32 *la_buf)
>  		     FW_PORT_CAP_SPEED_40G | FW_PORT_CAP_SPEED_100G | \
>  		     FW_PORT_CAP_ANEG)
>  
> +/* Translate Firmware Port Capabilities Pause specification to Common Code */
> +static inline unsigned int fwcap_to_cc_pause(unsigned int fw_pause)
> +{

Do not use "inline" in foo.c files, always let the compiler decide.

Same goes for patch #3.

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Jakub Kicinski]

On Sat, 24 Jun 2017 12:19:43 -0700, Roopa Prabhu wrote:
> Encoding: Types of encoding
> Off    :  Turning off any encoding
> RS     :  enforcing RS-FEC encoding on supported speeds
> BaseR  :  enforcing Base R encoding on supported speeds
> Auto   :  IEEE defaults for the speed/medium combination

Just to be sure - does auto mean autonegotiate as defined by IEEE or
some presets?  IIUC there is a notion of different length cables
defaulting to different strength of FEC in 25GE?

Thank you for doing this work!

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Dustin Byford]

Hi Gal,

On Sun Jun 25 16:38, Gal Pressman wrote:
> 
> > ...
> >
> > SHOW FEC option:
> > root@tor: ethtool --show-fec  swp1
> > FEC parameters for swp1:
> > Active FEC encodings: RS
> > Configured FEC encodings:  RS | BaseR
> >
> > ETHTOOL DEVNAME output modification:
> >
> > ethtool devname output:
> > root@tor:~# ethtool swp1
> > Settings for swp1:
> > root@hpe-7712-03:~# ethtool swp18
> > Settings for swp18:
> >     Supported ports: [ FIBRE ]
> >     Supported link modes:   40000baseCR4/Full
> >                             40000baseSR4/Full
> >                             40000baseLR4/Full
> >                             100000baseSR4/Full
> >                             100000baseCR4/Full
> >                             100000baseLR4_ER4/Full
> >     Supported pause frame use: No
> >     Supports auto-negotiation: Yes
> >     Supported FEC modes: [RS | BaseR | None | Not reported]
> >     Advertised link modes:  Not reported
> >     Advertised pause frame use: No
> >     Advertised auto-negotiation: No
> >     Advertised FEC modes: [RS | BaseR | None | Not reported]
> > <<<< One or more FEC modes
> >     Speed: 100000Mb/s
> >     Duplex: Full
> >     Port: FIBRE
> >     PHYAD: 106
> >     Transceiver: internal
> >     Auto-negotiation: off
> >     Link detected: yes

> What is the difference between the information in ethtool DEVNAME and ethtool --show-fec DEVNAME?

I think there are two questions there.  First, how does the FEC-related
information from glinksettings differ from what's retrieved via
gfecparam.  Second, how is that expressed through the ethtool UI.

Regarding the uapi (as we imagined it), glinksettings returns FEC
information through three fields:

@supported: the complete set of FEC modes the hardware supports, at any
speed, medium, or autoneg combination.

@advertising: the set of modes advertised to the link partner through
the relevant autoneg mechanism.

@lp_advertising: the set of modes the link partner is advertising
through autoneg.


gfecparam is used to fetch a couple more important facts about the FEC
configuration:

1) What FEC mode is currently active, either as a result of the autoneg
process, or a previous call to sfecparam.  This is returned in
sfecparam->active_fec

2) If autoneg is off, what is the currently configured FEC mode.  This
is a bitmask returned in gfecparam->fec.  I imagine it's typically a
single mode, but a mask makes it easier to implement a "don't care" policy,
or otherwise allow the NIC/driver to pick between a set of modes.


Regarding the UI.  ethtool DEVNAME gets most of its info from
glinksettings and it's easy to represent the FEC parameters affected by
autoneg there.  ethtool --show-fec simply reports the output of
gfecparam.  I agree the difference is subtle, perhaps it makes sense to
combine all the FEC information into ethtool DEVNAME?

> I can't find a usage of LINK_MODE_FEC_* bits in downstream patches.

I'm not sure what patches you're looking at, but I think those bits
directly affect the "Advertised FEC modes" and "Supported FEC Modes"
fields.


		--Dustin

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Dustin Byford]

Hi Jakub,

On Tue Jun 27 03:22, Jakub Kicinski wrote:
> On Sat, 24 Jun 2017 12:19:43 -0700, Roopa Prabhu wrote:
> > Encoding: Types of encoding
> > Off    :  Turning off any encoding
> > RS     :  enforcing RS-FEC encoding on supported speeds
> > BaseR  :  enforcing Base R encoding on supported speeds
> > Auto   :  IEEE defaults for the speed/medium combination
> 
> Just to be sure - does auto mean autonegotiate as defined by IEEE or
> some presets?  IIUC there is a notion of different length cables
> defaulting to different strength of FEC in 25GE?

In this context, "auto" doesn't mean autoneg.  Typically, if it's a
copper (CR) link autoneg has taken care of the FEC settings.  If you're
using sfecparam, you're probably dealing with optics where there is no
real autoneg.

Here, the term "auto", in its simplest implementation, would mean the
driver picks a preset based on the speed, cable (typically an SFF
cable), and hardware capability.  "IEEE defaults" in the comment refers
to a couple of tables you'll find in IEEE specs (sorry I can't dig them
up at the moment) that suggest FEC modes based on speed, medium
(CR/SR/LR) and perhaps length in the CR case.  So, yes, perhaps length
is part of the calculation for the appropriate "auto" mode.  This is
essentially why this patch set is important.  Drivers up until now can
be thought of as implementing "auto".  Sometimes that matches the
expectation of a link partner, especially if both sides support the
"IEEE defaults", but it's somewhat common for there to be a mismatch.
That can be because one side isn't using the "IEEE default" FEC mode for
the speed/medium/length, but it can also be because the hardware doesn't
support the most common FEC mode for a speed/medium/length.  We need a
way to explicitly set the FEC mode.  But, the hope is that "auto" is a
reasonable default.

		--Dustin

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Jakub Kicinski]

On Tue, 27 Jun 2017 23:27:34 -0700, Dustin Byford wrote:
> On Tue Jun 27 03:22, Jakub Kicinski wrote:
> > On Sat, 24 Jun 2017 12:19:43 -0700, Roopa Prabhu wrote:  
> > > Encoding: Types of encoding
> > > Off    :  Turning off any encoding
> > > RS     :  enforcing RS-FEC encoding on supported speeds
> > > BaseR  :  enforcing Base R encoding on supported speeds
> > > Auto   :  IEEE defaults for the speed/medium combination  
> > 
> > Just to be sure - does auto mean autonegotiate as defined by IEEE or
> > some presets?  IIUC there is a notion of different length cables
> > defaulting to different strength of FEC in 25GE?  
> 
> In this context, "auto" doesn't mean autoneg.  Typically, if it's a
> copper (CR) link autoneg has taken care of the FEC settings.  If you're
> using sfecparam, you're probably dealing with optics where there is no
> real autoneg.
> 
> Here, the term "auto", in its simplest implementation, would mean the
> driver picks a preset based on the speed, cable (typically an SFF
> cable), and hardware capability.  "IEEE defaults" in the comment refers
> to a couple of tables you'll find in IEEE specs (sorry I can't dig them
> up at the moment) that suggest FEC modes based on speed, medium
> (CR/SR/LR) and perhaps length in the CR case.  So, yes, perhaps length
> is part of the calculation for the appropriate "auto" mode.  This is
> essentially why this patch set is important.  Drivers up until now can
> be thought of as implementing "auto".  Sometimes that matches the
> expectation of a link partner, especially if both sides support the
> "IEEE defaults", but it's somewhat common for there to be a mismatch.
> That can be because one side isn't using the "IEEE default" FEC mode for
> the speed/medium/length, but it can also be because the hardware doesn't
> support the most common FEC mode for a speed/medium/length.  We need a
> way to explicitly set the FEC mode.  But, the hope is that "auto" is a
> reasonable default.


Thanks for the explanation!

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Andrew Lunn]

On Tue, Jun 27, 2017 at 03:22:39AM -0700, Jakub Kicinski wrote:
> On Sat, 24 Jun 2017 12:19:43 -0700, Roopa Prabhu wrote:
> > Encoding: Types of encoding
> > Off    :  Turning off any encoding
> > RS     :  enforcing RS-FEC encoding on supported speeds
> > BaseR  :  enforcing Base R encoding on supported speeds
> > Auto   :  IEEE defaults for the speed/medium combination
> 
> Just to be sure - does auto mean autonegotiate as defined by IEEE or
> some presets?

I don't know this field very well. Is this confusion likely to happen
a lot? Is there a better name for Auto which is less likely to be
confused?

	Andrew

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Dustin Byford]

Hi Andrew,

On Wed Jun 28 15:41, Andrew Lunn wrote:
> On Tue, Jun 27, 2017 at 03:22:39AM -0700, Jakub Kicinski wrote:
> > On Sat, 24 Jun 2017 12:19:43 -0700, Roopa Prabhu wrote:
> > > Encoding: Types of encoding
> > > Off    :  Turning off any encoding
> > > RS     :  enforcing RS-FEC encoding on supported speeds
> > > BaseR  :  enforcing Base R encoding on supported speeds
> > > Auto   :  IEEE defaults for the speed/medium combination
> > 
> > Just to be sure - does auto mean autonegotiate as defined by IEEE or
> > some presets?
> 
> I don't know this field very well. Is this confusion likely to happen
> a lot? Is there a better name for Auto which is less likely to be
> confused?

You're not the first, or the second to ask that question.  I agree it
could use clarification.

I always read auto in this context as automatic rather than autoneg.
The best I can come up with is to perhaps fully spell out "automatic" in
the documentation and the associated uapi enums.  It's accurate, and
hopefully different enough from "autoneg" to hint people away from the
IEEE autoneg concept.

		--Dustin

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Jakub Kicinski]

On Wed, 28 Jun 2017 14:47:51 -0700, Dustin Byford wrote:
> Hi Andrew,
> 
> On Wed Jun 28 15:41, Andrew Lunn wrote:
> > On Tue, Jun 27, 2017 at 03:22:39AM -0700, Jakub Kicinski wrote:  
> > > On Sat, 24 Jun 2017 12:19:43 -0700, Roopa Prabhu wrote:  
> > > > Encoding: Types of encoding
> > > > Off    :  Turning off any encoding
> > > > RS     :  enforcing RS-FEC encoding on supported speeds
> > > > BaseR  :  enforcing Base R encoding on supported speeds
> > > > Auto   :  IEEE defaults for the speed/medium combination  
> > > 
> > > Just to be sure - does auto mean autonegotiate as defined by IEEE or
> > > some presets?  
> > 
> > I don't know this field very well. Is this confusion likely to happen
> > a lot? Is there a better name for Auto which is less likely to be
> > confused?  
> 
> You're not the first, or the second to ask that question.  I agree it
> could use clarification.
> 
> I always read auto in this context as automatic rather than autoneg.
> The best I can come up with is to perhaps fully spell out "automatic" in
> the documentation and the associated uapi enums.  It's accurate, and
> hopefully different enough from "autoneg" to hint people away from the
> IEEE autoneg concept.

So perhaps just "default"?  Even saying something like ieee-selected
doesn't really help, because apparently there are two autonegs defined
- IEEE one and a "consortium" one...

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Andrew Lunn]

On Wed, Jun 28, 2017 at 02:47:51PM -0700, Dustin Byford wrote:
> Hi Andrew,
> 
> On Wed Jun 28 15:41, Andrew Lunn wrote:
> > On Tue, Jun 27, 2017 at 03:22:39AM -0700, Jakub Kicinski wrote:
> > > On Sat, 24 Jun 2017 12:19:43 -0700, Roopa Prabhu wrote:
> > > > Encoding: Types of encoding
> > > > Off    :  Turning off any encoding
> > > > RS     :  enforcing RS-FEC encoding on supported speeds
> > > > BaseR  :  enforcing Base R encoding on supported speeds
> > > > Auto   :  IEEE defaults for the speed/medium combination
> > > 
> > > Just to be sure - does auto mean autonegotiate as defined by IEEE or
> > > some presets?
> > 
> > I don't know this field very well. Is this confusion likely to happen
> > a lot? Is there a better name for Auto which is less likely to be
> > confused?
> 
> You're not the first, or the second to ask that question.  I agree it
> could use clarification.
> 
> I always read auto in this context as automatic rather than autoneg.

Hi Dustin

Automatic would be better.

Thanks
	Andrew

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Gal Pressman]

> Hi Gal,
>
> ...
>> What is the difference between the information in ethtool DEVNAME and ethtool --show-fec DEVNAME?
> I think there are two questions there.  First, how does the FEC-related
> information from glinksettings differ from what's retrieved via
> gfecparam.  Second, how is that expressed through the ethtool UI.
>
> Regarding the uapi (as we imagined it), glinksettings returns FEC
> information through three fields:
>
> @supported: the complete set of FEC modes the hardware supports, at any
> speed, medium, or autoneg combination.
>
> @advertising: the set of modes advertised to the link partner through
> the relevant autoneg mechanism.
>
> @lp_advertising: the set of modes the link partner is advertising
> through autoneg.
>
>
> gfecparam is used to fetch a couple more important facts about the FEC
> configuration:
>
> 1) What FEC mode is currently active, either as a result of the autoneg
> process, or a previous call to sfecparam.  This is returned in
> sfecparam->active_fec
>
> 2) If autoneg is off, what is the currently configured FEC mode.  This
> is a bitmask returned in gfecparam->fec.  I imagine it's typically a
> single mode, but a mask makes it easier to implement a "don't care" policy,
> or otherwise allow the NIC/driver to pick between a set of modes.
>
>
> Regarding the UI.  ethtool DEVNAME gets most of its info from
> glinksettings and it's easy to represent the FEC parameters affected by
> autoneg there.  ethtool --show-fec simply reports the output of
> gfecparam.  I agree the difference is subtle, perhaps it makes sense to
> combine all the FEC information into ethtool DEVNAME?
IMHO it does, but the current UI is fine too.
Thanks for the explanation.

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Casey Leedom]

| From: Jakub Kicinski <kubakici@wp.pl>
| Sent: Wednesday, June 28, 2017 6:00 PM
|     
| On Wed, 28 Jun 2017 14:47:51 -0700, Dustin Byford wrote:
| > 
| > You're not the first, or the second to ask that question.  I agree it
| > could use clarification.
| > 
| > I always read auto in this context as automatic rather than autoneg.
| > The best I can come up with is to perhaps fully spell out "automatic" in
| > the documentation and the associated uapi enums.  It's accurate, and
| > hopefully different enough from "autoneg" to hint people away from the
| > IEEE autoneg concept.
| 
| So perhaps just "default"?  Even saying something like ieee-selected
| doesn't really help, because apparently there are two autonegs defined
| - IEEE one and a "consortium" one...

First, sorry for the extreme delay in responding.  I was at the {25,100}Gb/s Ethernet "Plug Fest" all last week and then the holiday weekend added to the delay.

As Dustin notes, you're not the first to be confused by the use of the term "auto".  It absolutely means.  It essentially means: "Use standard FEC settings as specified by IEEE 802.3 interpretations of Cable Transceiver Module parameters."  But this is quite a mouthful.  In this sense, "auto" means "automatic".

Other keywords which we bandied about included: standard, default, ieee, ieee802.3, ieee802.3-default, transceiver-default, etc.  As you can see, the quest to make the option more obvious lead to increasing verbosity.

I think that in the end, we decided to go with a moderately short keyword with tons of documentation to make the meaning clear.

By the way, this isn't the end of this problem.  The new QSFP28 and SFP28 Port Types have introduced a problem which no one has ever seen with any preceding network technology.  With all previous networking technologies, a driver could look at an adapter Port Type and know what its Port Capabilities were in terms of Speeds, etc. and those Port Capabilities would never change.  With the new QSFP28 and SFP28 Port Types, the Physical Port Capabilities can change based on what Transceiver Modules you plug in.  For instance, with one QSFP28 Transceiver Module you might see {100,50,25}Gb/s and with a different one {40,10}Gb/s.  One Transceiver Module might support Reed-Solomon Forward Error Correction and another might also support BaseR/Reed-Solomon.

For the proposed FEC controls, we've tried to cope with this by having this "Automatic IEEE802.3 Transceiver Module-dictated FEC" via the "auto" selection (where most users will leave it we hope).  This allows the Firmware/Host Driver to automatically track the FEC needs of the currently plugged in Transceiver Module.

However, the first question which pops up is: what happens if a user explicitly selects a particular FEC for from the set offered by the current Transceiver Module, and then swap out Transceiver Modules to one which doesn't support that FEC?  Does the OS/Driver/Firmware "forget" the user's FEC setting?  Does it respect it and potentially not get a link established?  Do we "temporarily" put the user's FEC setting aside?  Or do we have FEC settings per-Transceiver Module Type?  Each choice has downsides in terms of "expected behavior" and the complexity of the abstraction model offered to the user.

In our discussions of the above, we decided that we should err in the direction of the absolutely simplest abstraction model, even when that might result in a failure to establish a link.  Our feeling was that doing anything else would result in endless user confusion.  Basically, if it takes longer than a simple paragraph to explain, you're probably doing the wrong thing.  So, we decided that if a user sets up a particular FEC setting, we keep that regardless of conflict with different Transceiver Modules.  (But flag such issues in the System Log in order to try to give the user a chance to understand what the new cable they plugged in wasn't working.)

As noted above, the "auto" FEC setting solves the problem of automatically tracking the FEC needs of whatever Transceiver Module happens to be plugged in.

And now with QSFP28 and SFP28, we have the same issue with possible Speeds (and other Link Parameters).  It may well be that we're going to need to extend this idea into Link Speeds.

Casey

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Jakub Kicinski]

On Thu, 6 Jul 2017 18:53:42 +0000, Casey Leedom wrote:
> However, the first question which pops up is: what happens if a user
> explicitly selects a particular FEC for from the set offered by the
> current Transceiver Module, and then swap out Transceiver Modules to
> one which doesn't support that FEC?  Does the OS/Driver/Firmware
> "forget" the user's FEC setting?  Does it respect it and potentially
> not get a link established?  Do we "temporarily" put the user's FEC
> setting aside?  Or do we have FEC settings per-Transceiver Module
> Type?  Each choice has downsides in terms of "expected behavior" and
> the complexity of the abstraction model offered to the user.
> 
> In our discussions of the above, we decided that we should err in the
> direction of the absolutely simplest abstraction model, even when
> that might result in a failure to establish a link.  Our feeling was
> that doing anything else would result in endless user confusion.
> Basically, if it takes longer than a simple paragraph to explain,
> you're probably doing the wrong thing.  So, we decided that if a user
> sets up a particular FEC setting, we keep that regardless of conflict
> with different Transceiver Modules.  (But flag such issues in the
> System Log in order to try to give the user a chance to understand
> what the new cable they plugged in wasn't working.)

IMHO if something gets replugged all the settings should be reset.
I feel that it's not entirely unlike replugging a USB adapter.  Perhaps
we should introduce some (devlink) notifications for SFP module events
so userspace can apply whatever static user config it has?

RE: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Wyborny, Carolyn]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Jakub Kicinski
> Sent: Thursday, July 06, 2017 12:02 PM
> To: Casey Leedom <leedom@chelsio.com>
> Cc: Dustin Byford <dustin@cumulusnetworks.com>; Andrew Lunn
> <andrew@lunn.ch>; Roopa Prabhu <roopa@cumulusnetworks.com>;
> davem@davemloft.net; linville@tuxdriver.com; netdev@vger.kernel.org;
> vidya.chowdary@gmail.com; olson@cumulusnetworks.com; Manoj Malviya
> <manojmalviya@chelsio.com>; Santosh Rastapur <santosh@chelsio.com>;
> yuval.mintz@qlogic.com; odedw@mellanox.com; ariela@mellanox.com;
> galp@mellanox.com; Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Subject: Re: [PATCH net-next 1/3] net: ethtool: add support for forward error
> correction modes
> 
[..]
> > In our discussions of the above, we decided that we should err in the
> > direction of the absolutely simplest abstraction model, even when
> > that might result in a failure to establish a link.  Our feeling was
> > that doing anything else would result in endless user confusion.
> > Basically, if it takes longer than a simple paragraph to explain,
> > you're probably doing the wrong thing.  So, we decided that if a user
> > sets up a particular FEC setting, we keep that regardless of conflict
> > with different Transceiver Modules.  (But flag such issues in the
> > System Log in order to try to give the user a chance to understand
> > what the new cable they plugged in wasn't working.)
> 
> IMHO if something gets replugged all the settings should be reset.
> I feel that it's not entirely unlike replugging a USB adapter.  Perhaps
> we should introduce some (devlink) notifications for SFP module events
> so userspace can apply whatever static user config it has?

This is an interesting dichotomy and we've been trying to resolve it as well as the module variations and permutations grow.  I agree with Casey that this bleeds into link speeds as well.  

I can see both perspectives:  try to apply to user settings even if they do something dumb and notify if things go bad; and, swapping modules should reset.  Notifications of some kind would help the devices manage this.  We tend to go with timers today.

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Casey Leedom]

| From: Jakub Kicinski <kubakici@wp.pl>
| Sent: Thursday, July 6, 2017 12:02 PM
|
| IMHO if something gets replugged all the settings should be reset.
| I feel that it's not entirely unlike replugging a USB adapter.  Perhaps
| we should introduce some (devlink) notifications for SFP module events
| so userspace can apply whatever static user config it has?

Absolutely a valid approach.  As are all of the ones I outlined.

But, and far more importantly, ideally _*ANY*_ such decision is made at an
architectural level to apply to all Link Parameters and Vendor Products.
The last thing a user wants to deal with is a hodge-podge of different
policies for different adapters from different vendors.
 
As I noted in my previous letter: this is something new that we've never
faced before with any prior networking technology.  All previous networking
technologies had a static set of Physical Port Capabilities fixed from the
moment a Host Diver/Firmware first see a Port.  We're now facing a situation
where these can change dynamically from moment to moment based on what
Transceiver Module is inserted.

With regard to this "architectural" issue, one way of trying to tease out
what model will be the simplest for users to work with is to ask: how do
users conceive of a "Port"?  I.e. when a user requests that a particular
Link Parameter be applied to a Port, are they thinking that it only applies
to the current instantaneous combination of Adapter Transceiver Module Cage
+ Transceiver Module?  Or do they conceptualize a "Port" as being a higher
level entity?

Or, let's make it Very Concrete with a specific example:

 1. User applies some set of Link Parameters.

 2. User attempts to bring Link up but it doesn't come up.

 3. User decides to try a different cable on the grounds that the first
    cable may be bad.

 4. New cable is accidentally of a completely different type with completely
    different subsequent Physical Port Capabilities, not capable of supporting
    the user's selected Link Parameters.

 5. User realizes this accidental mis-selection, and swaps in a third cable,
    similar to the first cable.

 6. User attempts to bring the Link up with the third cable.

If we reset all of the user-specified Link Parameters at point #3 and/or #5,
then the user's requested Link Parameter settings from point #1 will no
longer be in effect at point #6.  Is this expected/desirable?

In our discussions (Dustin, I, and others), we felt that the answer to this
above scenario question was "no".  I.e. that users would have a persistent
memory of having applied a set of Link Parameter settings to the "Port" and
would be annoyed/confused if those settings were "arbitrarily" reset at some
indefinite time.

But, this is a discussion and a decision that needs to be made.  Regardless
of what people finally decide is the "Right Answer", it needs to be
extremely well documented, it needs to be executed uniformly, and we may
want to explicitly notify the user at the points where something
unexpected/confusing is occurring.  Say, in the "Reset Unsupportable Link
Parameters When Transceiver Module Is Changed" model that you advocate, when
the user's Link Parameter settings are reset.

Casey

RE: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Wyborny, Carolyn]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Casey Leedom
> Sent: Thursday, July 06, 2017 2:54 PM
> To: Jakub Kicinski <kubakici@wp.pl>
> Cc: Dustin Byford <dustin@cumulusnetworks.com>; Andrew Lunn
> <andrew@lunn.ch>; Roopa Prabhu <roopa@cumulusnetworks.com>;
> davem@davemloft.net; linville@tuxdriver.com; netdev@vger.kernel.org;
> vidya.chowdary@gmail.com; olson@cumulusnetworks.com; Manoj Malviya
> <manojmalviya@chelsio.com>; Santosh Rastapur <santosh@chelsio.com>;
> yuval.mintz@qlogic.com; odedw@mellanox.com; ariela@mellanox.com;
> galp@mellanox.com; Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Subject: Re: [PATCH net-next 1/3] net: ethtool: add support for forward error
> correction modes
> 
> | From: Jakub Kicinski <kubakici@wp.pl>
> | Sent: Thursday, July 6, 2017 12:02 PM
> |
> | IMHO if something gets replugged all the settings should be reset.
> | I feel that it's not entirely unlike replugging a USB adapter.  Perhaps
> | we should introduce some (devlink) notifications for SFP module events
> | so userspace can apply whatever static user config it has?
> 
> Absolutely a valid approach.  As are all of the ones I outlined.
> 
[..]
> As I noted in my previous letter: this is something new that we've never
> faced before with any prior networking technology.  All previous networking
> technologies had a static set of Physical Port Capabilities fixed from the
> moment a Host Diver/Firmware first see a Port.  We're now facing a situation
> where these can change dynamically from moment to moment based on what
> Transceiver Module is inserted.

Agree with your points generally, but other networking hw vendors have dealt with this since SFP variant and other modules arrived on the scene.  

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Andrew Lunn]

On Thu, Jul 06, 2017 at 09:53:46PM +0000, Casey Leedom wrote:
> | From: Jakub Kicinski <kubakici@wp.pl>
> | Sent: Thursday, July 6, 2017 12:02 PM
> |
> | IMHO if something gets replugged all the settings should be reset.
> | I feel that it's not entirely unlike replugging a USB adapter.  Perhaps
> | we should introduce some (devlink) notifications for SFP module events
> | so userspace can apply whatever static user config it has?
> 
> Absolutely a valid approach.  As are all of the ones I outlined.
> 
> But, and far more importantly, ideally _*ANY*_ such decision is made at an
> architectural level to apply to all Link Parameters and Vendor Products.
> The last thing a user wants to deal with is a hodge-podge of different
> policies for different adapters from different vendors.

Yes.

SFP needs to becomes a Linux device, similar to Copper PHYs are Linux
devices. With some core code which all drivers can use, implement
ethtool --dump-module-eeprom, report speeds to the MAC using
adjust_link, etc..

> how do users conceive of a "Port"?

For a user, it is something they configure via /etc/network/interfaces
and then use ifup/ifdown on.

>  I.e. when a user requests that a particular
> Link Parameter be applied to a Port, are they thinking that it only applies
> to the current instantaneous combination of Adapter Transceiver Module Cage
> + Transceiver Module?  Or do they conceptualize a "Port" as being a higher
> level entity?
> 
> Or, let's make it Very Concrete with a specific example:
> 
>  1. User applies some set of Link Parameters.
> 
>  2. User attempts to bring Link up but it doesn't come up.

So these are effectively one step for the user, since the
configuration goes into /etc/network/interfaces, and it is only when
ifup is used is it applied. If the configuration is not valid, at this
point in time, i would expect ifup to give an error message.

>  3. User decides to try a different cable on the grounds that the first
>     cable may be bad.
> 
>  4. New cable is accidentally of a completely different type with completely
>     different subsequent Physical Port Capabilities, not capable of supporting
>     the user's selected Link Parameters.

And this is where it gets interesting, as you say. We are into a
hotplug model.

I think you also need to define 'cable' here. I assume you are not
talking about a piece of CAT 5 or glass fibre. You mean something
which is active. You are putting a different module into the SFP cage.

The extreme model would be, if you pull the module out, the whole
netdev is hot-unplugged. Plug a different modules in, the netdev is
hot-plugged. The user has to ifup it again, and would get an error
message if the configuration is invalid.

But i think this is too extreme.

I think the sfp device needs to give a hotplug event on unplug/plug.
A hot-unplug would result in an ifdown. And within the kernel, the
netdev is set down. If there is an "allow-hotplug" statement in
/etc/network/interfaces, on hot-plug, udev would try to ifup and get
an error and it will stay down. Without the "allow-hotplug" the
interface remains configured down until the user does an ifup and
would see an error message if the configuration is invalid.

    Andrew

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Andrew Lunn]

> Agree with your points generally, but other networking hw vendors
> have dealt with this since SFP variant and other modules arrived on
> the scene.

It seems like there is interest now in consolidating this, in the same
way that copper PHYs got consolidated. Yes, there are some MAC drivers
which still have there own PHY drivers, but most Ethernet MAC drivers
use phylib.

Now maybe the time for an sfplib.

    Andrew

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Casey Leedom]

| From: Wyborny, Carolyn <carolyn.wyborny@intel.com>
| Sent: Thursday, July 6, 2017 3:16 PM
| 
| Agree with your points generally, but other networking hw vendors have
| dealt with this since SFP variant and other modules arrived on the scene. 

The only case of this of which I'm aware is the SFP+ RJ45 1Gb/s
Transceiver Module.  But yes, that presaged what we're looking at
now as a much simpler example.

Casey

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Jakub Kicinski]

On Thu, 6 Jul 2017 21:53:46 +0000, Casey Leedom wrote:
> | From: Jakub Kicinski <kubakici@wp.pl>
> | Sent: Thursday, July 6, 2017 12:02 PM
> |
> | IMHO if something gets replugged all the settings should be reset.
> | I feel that it's not entirely unlike replugging a USB adapter.  Perhaps
> | we should introduce some (devlink) notifications for SFP module events
> | so userspace can apply whatever static user config it has?
> 
> Absolutely a valid approach.  As are all of the ones I outlined.
> 
> But, and far more importantly, ideally _*ANY*_ such decision is made at an
> architectural level to apply to all Link Parameters and Vendor Products.
> The last thing a user wants to deal with is a hodge-podge of different
> policies for different adapters from different vendors.

Agreed.  Once we decided we should make the expected behaviour very
clear the everyone.

Sorry if I'm misunderstanding - are you suggesting we should keep the
speed settings if hand-selected?  My feeling is those should be reset
if they are incompatible with the module.

> As I noted in my previous letter: this is something new that we've never
> faced before with any prior networking technology.  All previous networking
> technologies had a static set of Physical Port Capabilities fixed from the
> moment a Host Diver/Firmware first see a Port.  We're now facing a situation
> where these can change dynamically from moment to moment based on what
> Transceiver Module is inserted.
> 
> With regard to this "architectural" issue, one way of trying to tease out
> what model will be the simplest for users to work with is to ask: how do
> users conceive of a "Port"?  I.e. when a user requests that a particular
> Link Parameter be applied to a Port, are they thinking that it only applies
> to the current instantaneous combination of Adapter Transceiver Module Cage
> + Transceiver Module?  Or do they conceptualize a "Port" as being a higher
> level entity?

Hm.  I'm beginning to come around on this.  If my understanding of PHY
sub-layers is correct the FEC layer shouldn't be reset on module
unplug.  OTOH when someone replaces a DAC with an optical module,
keeping FEC around is not going to do any good to users...

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Casey Leedom]

| From: Andrew Lunn <andrew@lunn.ch>
| Sent: Thursday, July 6, 2017 3:33 PM
|     
| On Thu, Jul 06, 2017 at 09:53:46PM +0000, Casey Leedom wrote:
| > 
| > But, and far more importantly, ideally _*ANY*_ such decision is made at an
| > architectural level to apply to all Link Parameters and Vendor Products.
| > The last thing a user wants to deal with is a hodge-podge of different
| > policies for different adapters from different vendors.
| 
| Yes.
| 
| SFP needs to becomes a Linux device, similar to Copper PHYs are Linux
| devices. With some core code which all drivers can use, implement
| ethtool --dump-module-eeprom, report speeds to the MAC using
| adjust_link, etc..

Okay.  This "feels" like an implementation approach rather than a model
abstraction commentary, but it sounds like it would help ensure consistency
across vendors, etc.

| > how do users conceive of a "Port"?
| 
| For a user, it is something they configure via /etc/network/interfaces
| and then use ifup/ifdown on.
| 
| ...
| 
| And this is where it gets interesting, as you say. We are into a
| hotplug model.
| 
| I think you also need to define 'cable' here. I assume you are not
| talking about a piece of CAT 5 or glass fibre. You mean something
| which is active. You are putting a different module into the SFP cage.

Yes, I'm talking about active Transceiver Modules whether welded
onto the ends of copper-based cables or Optical Transceiver Modules.

| The extreme model would be, if you pull the module out, the whole
| netdev is hot-unplugged. Plug a different modules in, the netdev is
| hot-plugged. The user has to ifup it again, and would get an error
| message if the configuration is invalid.
| 
| But i think this is too extreme.

I agree.  This would force a completely new set of behavior on all users.
And it wouldn't match the paradigms used by any other OS.  (Yes,
I know that Linux tends to ignore such issues, but users do live in
mixed shops and it would be cruel to them to force radically different
operating paradigms between the systems they need to use.)

| I think the sfp device needs to give a hotplug event on unplug/plug.
| A hot-unplug would result in an ifdown. And within the kernel, the
| netdev is set down. If there is an "allow-hotplug" statement in
| /etc/network/interfaces, on hot-plug, udev would try to ifup and get
| an error and it will stay down. Without the "allow-hotplug" the
| interface remains configured down until the user does an ifup and
| would see an error message if the configuration is invalid.

Even this feels too extreme for me.  I think users would hate it.  They
did an ifup and swapped cables.  They expect the OS/Driver/Firmware
to continue trying to honor their ifup request.

Casey

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Casey Leedom]

| From: Jakub Kicinski <jakub.kicinski@netronome.com>
| Sent: Thursday, July 6, 2017 3:43 PM
|     
| On Thu, 6 Jul 2017 21:53:46 +0000, Casey Leedom wrote:
| > 
| > But, and far more importantly, ideally _*ANY*_ such decision is made at an
| > architectural level to apply to all Link Parameters and Vendor Products.
| > The last thing a user wants to deal with is a hodge-podge of different
| > policies for different adapters from different vendors.
| 
| Agreed.  Once we decided we should make the expected behaviour very
| clear the everyone.
| 
| Sorry if I'm misunderstanding - are you suggesting we should keep the
| speed settings if hand-selected?  My feeling is those should be reset
| if they are incompatible with the module.

Again, just to be perfectly clear, I don't think that there's a perfect
solution to this.  My personal feeling is that we need to think through all
of the usage scenarios and see what happens in the various models,
and more importantly, how easily we can explain the inevitable
repercussions to users.  Again, the "simplest model wins" philosophy.

But to your specific question: yes, I am saying that if the user selected
25Gb/s and accidentally swapped in a 10Gb/s cable, and then
swapped a different {100,25}Gb/s cable back in, the advertised
Speed(s) should be 25Gb/s with the newest cable, respecting
the original Link Parameter setting with the first cable.

And again, I don't believe that we'll come up with a perfect solution.
But hopefully we can come up with an abstraction model that's
easy to understand and use.  (And requires the fewest changes
to current accepted practices.)

| Hm.  I'm beginning to come around on this.  If my understanding of PHY
| sub-layers is correct the FEC layer shouldn't be reset on module
| unplug.  OTOH when someone replaces a DAC with an optical module,
| keeping FEC around is not going to do any good to users...

When I first stumbled across this issue several months ago I though
I must be dense.  Nothing this simple should be this complicated.
It's been extremely gratifying to find others similarly flummoxed ... :-)

Casey

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Andrew Lunn]

> Even this feels too extreme for me.  I think users would hate it.  They
> did an ifup and swapped cables.  They expect the OS/Driver/Firmware
> to continue trying to honor their ifup request.

Lets take a look around at other subsystems....

What happens if you hot-unplug a SATA drive?
An MMC card?
A USB device?
A PCIe card?
A CPU?
An SDRAM?

Do you know of any subsystem that tried to keep its configuration
across a hot unplug/plug event? I suspect they all require user space
to take some action to get the newly plugged hardware into operation.

       Andrew

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Jakub Kicinski]

On Fri, 7 Jul 2017 01:15:50 +0200, Andrew Lunn wrote:
> > Even this feels too extreme for me.  I think users would hate it.  They
> > did an ifup and swapped cables.  They expect the OS/Driver/Firmware
> > to continue trying to honor their ifup request.  
> 
> Lets take a look around at other subsystems....
> 
> What happens if you hot-unplug a SATA drive?
> An MMC card?
> A USB device?
> A PCIe card?
> A CPU?
> An SDRAM?
> 
> Do you know of any subsystem that tried to keep its configuration
> across a hot unplug/plug event? I suspect they all require user space
> to take some action to get the newly plugged hardware into operation.

That was exactly my thinking initially, but I'm not so sure anymore
because the layer to which configuration is applied does not reside on
the portion of HW which is hot swapped.  I'm very far from an expert on
the PHY layer stuff but modules start at the PMA layer?

If only IEEE defined for us what the behaviour should be :)

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Casey Leedom]

| From: Andrew Lunn <andrew@lunn.ch>
| Sent: Thursday, July 6, 2017 4:15 PM
|     
| > Even this feels too extreme for me.  I think users would hate it.  They
| > did an ifup and swapped cables.  They expect the OS/Driver/Firmware
| > to continue trying to honor their ifup request.
| 
| Lets take a look around at other subsystems....
| ...
| Do you know of any subsystem that tried to keep its configuration
| across a hot unplug/plug event? I suspect they all require user space
| to take some action to get the newly plugged hardware into operation.

I agree ... -ish ... :-)

If you choose to think of a cable unplug/plug event as "hot plug", then
the "reset" is the model that feels right.  But I'll note that this is also
presupposing what the right model is for users.  This is akin
to trying to decide what to make for dinner and deciding that a
hammer is the right tool.  If we end up deciding on Cracked
Crab (or Tree Nuts for the vegans amongst us), then the
hammer is quite possibly a good answer.

All I can continue to say is: keep on thinking about users.  How they're
using networking devices now, how they think about them, how we're
going to explain to them whatever changes they'll need to make to
their usage of network ports.  And yes, how this will fit into management
models for other OSes, etc.

The customers aren't always right, but their opinion matters a lot.

Casey

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Andrew Lunn]

> If you choose to think of a cable unplug/plug event as "hot plug", then
> the "reset" is the model that feels right.  But I'll note that this is also
> presupposing what the right model is for users.  This is akin
> to trying to decide what to make for dinner and deciding that a
> hammer is the right tool.  If we end up deciding on Cracked
> Crab (or Tree Nuts for the vegans amongst us), then the
> hammer is quite possibly a good answer.

It also depends on the chef. A skilled chef knows how to use and abuse
a hammer and can make quite good filo pastry using a hammer if need
be.

And when we are talking about SFP modules, we are not talking about
somebody who can just about boiling an egg and put bread in the
toaster.

Anybody using these SFP modules knows a lot about networking, is a
power user, and probably does not rely on plug and pray.

      Andrew

Re: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Dave Olson]

Andrew Lunn <andrew@lunn.ch> wrote:
> And when we are talking about SFP modules, we are not talking about
> somebody who can just about boiling an egg and put bread in the
> toaster.
> 
> Anybody using these SFP modules knows a lot about networking, is a
> power user, and probably does not rely on plug and pray.

We have strong evidence that this is not the case with many system and
network admins...

Casey is right; the simplest mental model for end-users that can be
made to work reasonably is the best answer.

And from a user perspective, that means (to me, from watching customer
cases), keeping the customers settings if at all possible, and spitting
out warnings and errors when it can't work.

As Casey keeps saying, there is no model that will work in all (or even
most) of the interesting use cases, where cables can change with some
frequency, especially when there are lots of ports (as on switches).

Dave Olson
olson@cumulusnetworks.com