[PATCH net-next 0/2] ice: support FEC automatic disable

[PATCH net-next 0/2] ice: support FEC automatic disable

[Jacob Keller]

This series implements support for users to configure automatic FEC
selection including the option of disabling FEC. It implements similar
behavior as a previous submission we made at

https://lore.kernel.org/netdev/20220714180311.933648-1-anthony.l.nguyen@intel.com/

This implementation varies, in that we now honor ETHTOOL_FEC_AUTO |
ETHTOOL_FEC_OFF as the new automatic plus disable mode.

This is in line with the request Jakub made to avoid using a new private
flag. I opted to use a bit-wise or of the two already supported flags rather
than trying to introduce a new flag.

I think this makes sense as essentially this is a request to automatically
select but also include "off" as a possible option. I'm not sure if this is
the best approach, but it seemed better than trying to add a new
ETHTOOL_FEC_AUTO_DISABLE or similarly confusing option. The need for this is
due to the quirk of how the ice firmware Link Establishment State Machine
works to decide what FEC mode to use.

Current userspace and the API already support multiple bit selection, though
it does have the downside of not guaranteeing consistency across drivers...
I'm open to alternative suggestions and implementations if someone has a
better suggestion.

Some alternatives we've considered already:

1) use a private flag

  Rejected for good reason, as private flags are difficult to discover and
  vary wildly across drivers. It also makes the driver behave differently to
  the same userspace request which may not be obvious to applications.

2) always treat ETHTOOL_FEC_AUTO as "automatic + allow disable"

  This could work, but it means that behavior will differ depending on the
  firmware version. Users have no way to know that and might be surprised to
  find the behavior differ across devices which have different firmware
  which do or don't support this variation of automatic selection.

2) introduce a new FEC mode to the ETHTOOL interface

  I considered just adding a brand new flag, but choosing a name here is
  relatively difficult. Most names read as some sort of "disable automatic
  selection" which isn't the best meaning.

3) use combined ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF (this series)

  This version simply accepts a combined bitwise OR of ETHTOOL_FEC_AUTO and
  ETHTOOL_FEC_OFF. This was previously rejected by ice so it should not
  cause compatibility issues. The API already supports it, though it was
  noted the semantics of this combination are not well defined and could
  behave differently across drivers.

  This version has the downside of not being explicit in the API now since
  drivers may not all interpret this combination the same way. Thats
  understandably a concern, but I'm not sure what the best approach to avoid
  that here.

  This version does allow users to explicitly request the new behavior, and
  allows reporting an error when firmware can't support it.

To aid in reporting errors to userspace, I also extended the .set_fecparam
to take the netlink extended ACK struct. This allows directly reporting why
the option didn't take when using the netlink backed interface for ethtool.


Jacob Keller (2):
  ethtool: pass netlink extended ACK to .set_fecparam
  ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  3 +-
 .../ethernet/cavium/liquidio/lio_ethtool.c    |  3 +-
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    |  3 +-
 .../ethernet/fungible/funeth/funeth_ethtool.c |  3 +-
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  3 +-
 .../net/ethernet/intel/i40e/i40e_ethtool.c    |  3 +-
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  1 +
 drivers/net/ethernet/intel/ice/ice_common.c   | 54 ++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_common.h   |  1 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c  | 15 ++++--
 drivers/net/ethernet/intel/ice/ice_main.c     |  3 +-
 drivers/net/ethernet/intel/ice/ice_type.h     |  9 +++-
 .../marvell/octeontx2/nic/otx2_ethtool.c      |  3 +-
 .../marvell/prestera/prestera_ethtool.c       |  3 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  3 +-
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  |  3 +-
 .../ethernet/pensando/ionic/ionic_ethtool.c   |  3 +-
 .../net/ethernet/qlogic/qede/qede_ethtool.c   |  3 +-
 drivers/net/ethernet/sfc/ethtool_common.c     |  3 +-
 drivers/net/ethernet/sfc/ethtool_common.h     |  3 +-
 .../net/ethernet/sfc/siena/ethtool_common.c   |  3 +-
 .../net/ethernet/sfc/siena/ethtool_common.h   |  3 +-
 drivers/net/netdevsim/ethtool.c               |  3 +-
 include/linux/ethtool.h                       |  3 +-
 net/ethtool/fec.c                             |  2 +-
 net/ethtool/ioctl.c                           |  2 +-
 26 files changed, 115 insertions(+), 26 deletions(-)


base-commit: 90b3bee3a23249977852079b908270afc6ee03bb
-- 
2.37.1.394.gc50926e1f488

[PATCH net-next 1/2] ethtool: pass netlink extended ACK to .set_fecparam

[Jacob Keller]

Add the netlink extended ACK structure pointer to the interface for
.set_fecparam. This allows reporting errors to the user appropriately when
using the netlink ethtool interface.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Derek Chickles <dchickles@marvell.com>
Cc: Satanand Burla <sburla@marvell.com>
Cc: Felix Manlunas <fmanlunas@marvell.com>
Cc: Raju Rangoju <rajur@chelsio.com>
Cc: Dimitris Michailidis <dmichail@fungible.com>
Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
Cc: Salil Mehta <salil.mehta@huawei.com>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: Sunil Goutham <sgoutham@marvell.com>
Cc: Geetha sowjanya <gakula@marvell.com>
Cc: Subbaraya Sundeep <sbhatta@marvell.com>
Cc: hariprasad <hkelam@marvell.com>
Cc: Taras Chornyi <tchornyi@marvell.com>
Cc: Saeed Mahameed <saeedm@nvidia.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Simon Horman <simon.horman@corigine.com>
Cc: Shannon Nelson <snelson@pensando.io>
Cc: Ariel Elior <aelior@marvell.com>
Cc: Manish Chopra <manishc@marvell.com>
Cc: Edward Cree <ecree.xilinx@gmail.com>
Cc: Martin Habets <habetsm.xilinx@gmail.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Cc: Fei Qin <fei.qin@corigine.com>
Cc: Louis Peens <louis.peens@corigine.com>
Cc: Yu Xiao <yu.xiao@corigine.com>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Yufeng Mo <moyufeng@huawei.com>
Cc: Sixiang Chen <sixiang.chen@corigine.com>
Cc: Yinjun Zhang <yinjun.zhang@corigine.com>
Cc: Hao Chen <chenhao288@hisilicon.com>
Cc: Guangbin Huang <huangguangbin2@huawei.com>
Cc: Sean Anderson <sean.anderson@seco.com>
Cc: Erik Ekman <erik@kryo.se>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: Jie Wang <wangjie125@huawei.com>
Cc: Moshe Tal <moshet@nvidia.com>
Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Cc: Marco Bonelli <marco@mebeim.net>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c         | 3 ++-
 drivers/net/ethernet/cavium/liquidio/lio_ethtool.c        | 3 ++-
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c        | 3 ++-
 drivers/net/ethernet/fungible/funeth/funeth_ethtool.c     | 3 ++-
 drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c        | 3 ++-
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c            | 3 ++-
 drivers/net/ethernet/intel/ice/ice_ethtool.c              | 4 +++-
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c | 3 ++-
 drivers/net/ethernet/marvell/prestera/prestera_ethtool.c  | 3 ++-
 drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c      | 3 ++-
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c      | 3 ++-
 drivers/net/ethernet/pensando/ionic/ionic_ethtool.c       | 3 ++-
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c           | 3 ++-
 drivers/net/ethernet/sfc/ethtool_common.c                 | 3 ++-
 drivers/net/ethernet/sfc/ethtool_common.h                 | 3 ++-
 drivers/net/ethernet/sfc/siena/ethtool_common.c           | 3 ++-
 drivers/net/ethernet/sfc/siena/ethtool_common.h           | 3 ++-
 drivers/net/netdevsim/ethtool.c                           | 3 ++-
 include/linux/ethtool.h                                   | 3 ++-
 net/ethtool/fec.c                                         | 2 +-
 net/ethtool/ioctl.c                                       | 2 +-
 21 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 87eb5362ad70..9ad00240ee06 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -2022,7 +2022,8 @@ static u32 bnxt_ethtool_forced_fec_to_fw(struct bnxt_link_info *link_info,
 }
 
 static int bnxt_set_fecparam(struct net_device *dev,
-			     struct ethtool_fecparam *fecparam)
+			     struct ethtool_fecparam *fecparam,
+			     struct netlink_ext_ack *extack)
 {
 	struct hwrm_port_phy_cfg_input *req;
 	struct bnxt *bp = netdev_priv(dev);
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c b/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c
index 2c10ae3f7fc1..d6ba8153c649 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c
@@ -3084,7 +3084,8 @@ static int lio_get_fecparam(struct net_device *netdev,
 }
 
 static int lio_set_fecparam(struct net_device *netdev,
-			    struct ethtool_fecparam *fec)
+			    struct ethtool_fecparam *fec,
+			    struct netlink_ext_ack *extack)
 {
 	struct lio *lio = GET_LIO(netdev);
 	struct octeon_device *oct = lio->oct_dev;
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index 77897edd2bc0..cd7f09aa63fb 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -834,7 +834,8 @@ static int get_fecparam(struct net_device *dev, struct ethtool_fecparam *fec)
 	return 0;
 }
 
-static int set_fecparam(struct net_device *dev, struct ethtool_fecparam *fec)
+static int set_fecparam(struct net_device *dev, struct ethtool_fecparam *fec,
+			struct netlink_ext_ack *extack)
 {
 	struct port_info *pi = netdev_priv(dev);
 	struct link_config *lc = &pi->link_cfg;
diff --git a/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c b/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
index 31aa185f4d17..31b15506ca4c 100644
--- a/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
+++ b/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
@@ -1087,7 +1087,8 @@ static int fun_get_fecparam(struct net_device *netdev,
 }
 
 static int fun_set_fecparam(struct net_device *netdev,
-			    struct ethtool_fecparam *fec)
+			    struct ethtool_fecparam *fec,
+			    struct netlink_ext_ack *extack)
 {
 	struct funeth_priv *fp = netdev_priv(netdev);
 	u64 fec_mode;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index 4c7988e308a2..82f7d70477e6 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -1673,7 +1673,8 @@ static int hns3_get_fecparam(struct net_device *netdev,
 }
 
 static int hns3_set_fecparam(struct net_device *netdev,
-			     struct ethtool_fecparam *fec)
+			     struct ethtool_fecparam *fec,
+			     struct netlink_ext_ack *extack)
 {
 	struct hnae3_handle *handle = hns3_get_handle(netdev);
 	struct hnae3_ae_dev *ae_dev = pci_get_drvdata(handle->pdev);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 156e92c43780..35e6eb65e237 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -1582,7 +1582,8 @@ static int i40e_get_fec_param(struct net_device *netdev,
 }
 
 static int i40e_set_fec_param(struct net_device *netdev,
-			      struct ethtool_fecparam *fecparam)
+			      struct ethtool_fecparam *fecparam,
+			      struct netlink_ext_ack *extack)
 {
 	struct i40e_netdev_priv *np = netdev_priv(netdev);
 	struct i40e_pf *pf = np->vsi->back;
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 60dca15635db..770101577a94 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -1012,9 +1012,11 @@ static int ice_set_fec_cfg(struct net_device *netdev, enum ice_fec_mode req_fec)
  * ice_set_fecparam - Set FEC link options
  * @netdev: network interface device structure
  * @fecparam: Ethtool structure to retrieve FEC parameters
+ * @extack: Netlink extended ACK response
  */
 static int
-ice_set_fecparam(struct net_device *netdev, struct ethtool_fecparam *fecparam)
+ice_set_fecparam(struct net_device *netdev, struct ethtool_fecparam *fecparam,
+		 struct netlink_ext_ack *extack)
 {
 	struct ice_netdev_priv *np = netdev_priv(netdev);
 	struct ice_vsi *vsi = np->vsi;
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
index 3f60a80e34c8..1bc5f661fe00 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
@@ -1024,7 +1024,8 @@ static int otx2_get_fecparam(struct net_device *netdev,
 }
 
 static int otx2_set_fecparam(struct net_device *netdev,
-			     struct ethtool_fecparam *fecparam)
+			     struct ethtool_fecparam *fecparam,
+			     struct netlink_ext_ack *extack)
 {
 	struct otx2_nic *pfvf = netdev_priv(netdev);
 	struct mbox *mbox = &pfvf->mbox;
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
index 1da7ff889417..4f19f0dfac5e 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
@@ -705,7 +705,8 @@ static int prestera_ethtool_get_fecparam(struct net_device *dev,
 }
 
 static int prestera_ethtool_set_fecparam(struct net_device *dev,
-					 struct ethtool_fecparam *fecparam)
+					 struct ethtool_fecparam *fecparam,
+					 struct netlink_ext_ack *extack)
 {
 	struct prestera_port *port = netdev_priv(dev);
 	struct prestera_port_mac_config cfg_mac;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index b811207fe5ed..1e2b82aff1cb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1668,7 +1668,8 @@ static int mlx5e_get_fecparam(struct net_device *netdev,
 }
 
 static int mlx5e_set_fecparam(struct net_device *netdev,
-			      struct ethtool_fecparam *fecparam)
+			      struct ethtool_fecparam *fecparam,
+			      struct netlink_ext_ack *extack)
 {
 	struct mlx5e_priv *priv = netdev_priv(netdev);
 	struct mlx5_core_dev *mdev = priv->mdev;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index eeb1455a4e5d..4c29da87c61f 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -1015,7 +1015,8 @@ nfp_port_get_fecparam(struct net_device *netdev,
 
 static int
 nfp_port_set_fecparam(struct net_device *netdev,
-		      struct ethtool_fecparam *param)
+		      struct ethtool_fecparam *param,
+		      struct netlink_ext_ack *extack)
 {
 	struct nfp_eth_table_port *eth_port;
 	struct nfp_port *port;
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index 01c22701482d..d9ff8899c67c 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -360,7 +360,8 @@ static int ionic_get_fecparam(struct net_device *netdev,
 }
 
 static int ionic_set_fecparam(struct net_device *netdev,
-			      struct ethtool_fecparam *fec)
+			      struct ethtool_fecparam *fec,
+			      struct netlink_ext_ack *extack)
 {
 	struct ionic_lif *lif = netdev_priv(netdev);
 	u8 fec_type;
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index 97a7ab0826ed..db3aab14907e 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -1912,7 +1912,8 @@ static int qede_get_fecparam(struct net_device *dev,
 }
 
 static int qede_set_fecparam(struct net_device *dev,
-			     struct ethtool_fecparam *fecparam)
+			     struct ethtool_fecparam *fecparam,
+			     struct netlink_ext_ack *extack)
 {
 	struct qede_dev *edev = netdev_priv(dev);
 	struct qed_link_params params;
diff --git a/drivers/net/ethernet/sfc/ethtool_common.c b/drivers/net/ethernet/sfc/ethtool_common.c
index bc840ede3053..2b1b505da4fb 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.c
+++ b/drivers/net/ethernet/sfc/ethtool_common.c
@@ -616,7 +616,8 @@ int efx_ethtool_get_fecparam(struct net_device *net_dev,
 }
 
 int efx_ethtool_set_fecparam(struct net_device *net_dev,
-			     struct ethtool_fecparam *fecparam)
+			     struct ethtool_fecparam *fecparam,
+			     struct netlink_ext_ack *extack)
 {
 	struct efx_nic *efx = efx_netdev_priv(net_dev);
 	int rc;
diff --git a/drivers/net/ethernet/sfc/ethtool_common.h b/drivers/net/ethernet/sfc/ethtool_common.h
index 659491932101..4cdc322f4616 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.h
+++ b/drivers/net/ethernet/sfc/ethtool_common.h
@@ -37,7 +37,8 @@ int efx_ethtool_set_link_ksettings(struct net_device *net_dev,
 int efx_ethtool_get_fecparam(struct net_device *net_dev,
 			     struct ethtool_fecparam *fecparam);
 int efx_ethtool_set_fecparam(struct net_device *net_dev,
-			     struct ethtool_fecparam *fecparam);
+			     struct ethtool_fecparam *fecparam,
+			     struct netlink_ext_ack *extack);
 int efx_ethtool_get_rxnfc(struct net_device *net_dev,
 			  struct ethtool_rxnfc *info, u32 *rule_locs);
 int efx_ethtool_set_rxnfc(struct net_device *net_dev,
diff --git a/drivers/net/ethernet/sfc/siena/ethtool_common.c b/drivers/net/ethernet/sfc/siena/ethtool_common.c
index 0207d07f54e3..9f65cb1c058c 100644
--- a/drivers/net/ethernet/sfc/siena/ethtool_common.c
+++ b/drivers/net/ethernet/sfc/siena/ethtool_common.c
@@ -616,7 +616,8 @@ int efx_siena_ethtool_get_fecparam(struct net_device *net_dev,
 }
 
 int efx_siena_ethtool_set_fecparam(struct net_device *net_dev,
-				   struct ethtool_fecparam *fecparam)
+				   struct ethtool_fecparam *fecparam,
+				   struct netlink_ext_ack *extack)
 {
 	struct efx_nic *efx = netdev_priv(net_dev);
 	int rc;
diff --git a/drivers/net/ethernet/sfc/siena/ethtool_common.h b/drivers/net/ethernet/sfc/siena/ethtool_common.h
index 04b375dc6800..7dadd95eb69f 100644
--- a/drivers/net/ethernet/sfc/siena/ethtool_common.h
+++ b/drivers/net/ethernet/sfc/siena/ethtool_common.h
@@ -34,7 +34,8 @@ int efx_siena_ethtool_set_link_ksettings(struct net_device *net_dev,
 int efx_siena_ethtool_get_fecparam(struct net_device *net_dev,
 				   struct ethtool_fecparam *fecparam);
 int efx_siena_ethtool_set_fecparam(struct net_device *net_dev,
-				   struct ethtool_fecparam *fecparam);
+				   struct ethtool_fecparam *fecparam,
+				   struct netlink_ext_ack *extack);
 int efx_siena_ethtool_get_rxnfc(struct net_device *net_dev,
 				struct ethtool_rxnfc *info, u32 *rule_locs);
 int efx_siena_ethtool_set_rxnfc(struct net_device *net_dev,
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index ffd9f84b6644..88f3738c5899 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -124,7 +124,8 @@ nsim_get_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
 }
 
 static int
-nsim_set_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
+nsim_set_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam,
+		  struct netlink_ext_ack *extack)
 {
 	struct netdevsim *ns = netdev_priv(dev);
 	u32 fec;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 99dc7bfbcd3c..3ce1ef7b1390 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -735,7 +735,8 @@ struct ethtool_ops {
 	int	(*get_fecparam)(struct net_device *,
 				      struct ethtool_fecparam *);
 	int	(*set_fecparam)(struct net_device *,
-				      struct ethtool_fecparam *);
+				      struct ethtool_fecparam *,
+				      struct netlink_ext_ack *extack);
 	void	(*get_ethtool_phy_stats)(struct net_device *,
 					 struct ethtool_stats *, u64 *);
 	int	(*get_phy_tunable)(struct net_device *,
diff --git a/net/ethtool/fec.c b/net/ethtool/fec.c
index 9f5a134e2e01..79931b0de177 100644
--- a/net/ethtool/fec.c
+++ b/net/ethtool/fec.c
@@ -295,7 +295,7 @@ int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info)
 		goto out_ops;
 	}
 
-	ret = dev->ethtool_ops->set_fecparam(dev, &fec);
+	ret = dev->ethtool_ops->set_fecparam(dev, &fec, info->extack);
 	if (ret < 0)
 		goto out_ops;
 	ethtool_notify(dev, ETHTOOL_MSG_FEC_NTF, NULL);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 6a7308de192d..2f397d016d3a 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2706,7 +2706,7 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 	fecparam.active_fec = 0;
 	fecparam.reserved = 0;
 
-	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
+	return dev->ethtool_ops->set_fecparam(dev, &fecparam, NULL);
 }
 
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
-- 
2.37.1.394.gc50926e1f488

[PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

[Jacob Keller]

The default Link Establishment State Machine (LESM) behavior does not
allow the use of FEC disabled if the media does not support FEC
disabled. However users may want to override this behavior.

To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a request
to automatically select an appropriate FEC mode including potentially
disabling FEC.

This is distinct from ETHTOOL_FEC_AUTO because that will not allow the LESM
to select FEC disabled. It is distinct from ETHTOOL_FEC_OFF because
FEC_OFF will always disable FEC without any LESM automatic selection.

This *does* mean that ice is now accepting one "bitwise OR" set for FEC
configuration, which is somewhat against the recommendations made in
6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not
sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make any
sense here.

With this change, users can opt to allow automatic FEC disable via

  ethtool --set-fec ethX encoding auto off

Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jakub Kicinski <kuba@kernel.org>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  1 +
 drivers/net/ethernet/intel/ice/ice_common.c   | 54 ++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_common.h   |  1 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c  | 11 +++-
 drivers/net/ethernet/intel/ice/ice_main.c     |  3 +-
 drivers/net/ethernet/intel/ice/ice_type.h     |  9 +++-
 6 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 9d2fa67d873e..5c5ebd70483c 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1123,6 +1123,7 @@ struct ice_aqc_get_phy_caps_data {
 #define ICE_AQC_PHY_FEC_25G_RS_528_REQ			BIT(2)
 #define ICE_AQC_PHY_FEC_25G_KR_REQ			BIT(3)
 #define ICE_AQC_PHY_FEC_25G_RS_544_REQ			BIT(4)
+#define ICE_AQC_PHY_FEC_DIS				BIT(5)
 #define ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN		BIT(6)
 #define ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN		BIT(7)
 #define ICE_AQC_PHY_FEC_MASK				ICE_M(0xdf, 0)
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 2c8c3fcc3dcd..96f51b7c9898 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -3253,8 +3253,11 @@ enum ice_fc_mode ice_caps_to_fc_mode(u8 caps)
  */
 enum ice_fec_mode ice_caps_to_fec_mode(u8 caps, u8 fec_options)
 {
-	if (caps & ICE_AQC_PHY_EN_AUTO_FEC)
+	if (caps & ICE_AQC_PHY_EN_AUTO_FEC) {
+		if (fec_options & ICE_AQC_PHY_FEC_DIS)
+			return ICE_FEC_DIS_AUTO;
 		return ICE_FEC_AUTO;
+	}
 
 	if (fec_options & (ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
 			   ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
@@ -3513,6 +3516,12 @@ ice_cfg_phy_fec(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg,
 		/* Clear all FEC option bits. */
 		cfg->link_fec_opt &= ~ICE_AQC_PHY_FEC_MASK;
 		break;
+	case ICE_FEC_DIS_AUTO:
+		/* Set No FEC and auto FEC */
+		if (!ice_fw_supports_fec_dis_auto(hw))
+			return -EOPNOTSUPP;
+		cfg->link_fec_opt |= ICE_AQC_PHY_FEC_DIS;
+		fallthrough;
 	case ICE_FEC_AUTO:
 		/* AND auto FEC bit, and all caps bits. */
 		cfg->caps &= ICE_AQC_PHY_CAPS_MASK;
@@ -5289,6 +5298,35 @@ ice_aq_get_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx,
 	return 0;
 }
 
+/**
+ * ice_is_fw_min_ver
+ * @hw: pointer to the hardware structure
+ * @branch: branch version
+ * @maj: major version
+ * @min: minor version
+ * @patch: patch version
+ *
+ * Checks if the firmware is minimum version
+ */
+static bool ice_is_fw_min_ver(struct ice_hw *hw, u8 branch, u8 maj, u8 min,
+			      u8 patch)
+{
+	if (hw->fw_branch == branch) {
+		if (hw->fw_maj_ver > maj)
+			return true;
+		if (hw->fw_maj_ver == maj) {
+			if (hw->fw_min_ver > min)
+				return true;
+			if (hw->fw_min_ver == min && hw->fw_patch >= patch)
+				return true;
+		}
+	} else if (hw->fw_branch > branch) {
+		return true;
+	}
+
+	return false;
+}
+
 /**
  * ice_fw_supports_link_override
  * @hw: pointer to the hardware structure
@@ -5510,3 +5548,17 @@ bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw)
 	}
 	return false;
 }
+
+/**
+ * ice_fw_supports_fec_dis_auto
+ * @hw: pointer to the hardware structure
+ *
+ * Checks if the firmware supports FEC disable in Auto FEC mode
+ */
+bool ice_fw_supports_fec_dis_auto(struct ice_hw *hw)
+{
+	return ice_is_fw_min_ver(hw, ICE_FW_FEC_DIS_AUTO_BRANCH,
+				     ICE_FW_FEC_DIS_AUTO_MAJ,
+				     ICE_FW_FEC_DIS_AUTO_MIN,
+				     ICE_FW_FEC_DIS_AUTO_PATCH);
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index f339bdc48062..a0baf80df356 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -110,6 +110,7 @@ int
 ice_aq_set_phy_cfg(struct ice_hw *hw, struct ice_port_info *pi,
 		   struct ice_aqc_set_phy_cfg_data *cfg, struct ice_sq_cd *cd);
 bool ice_fw_supports_link_override(struct ice_hw *hw);
+bool ice_fw_supports_fec_dis_auto(struct ice_hw *hw);
 int
 ice_get_link_default_override(struct ice_link_default_override_tlv *ldo,
 			      struct ice_port_info *pi);
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 770101577a94..a83b127a00b0 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -1020,9 +1020,17 @@ ice_set_fecparam(struct net_device *netdev, struct ethtool_fecparam *fecparam,
 {
 	struct ice_netdev_priv *np = netdev_priv(netdev);
 	struct ice_vsi *vsi = np->vsi;
+	struct ice_pf *pf = vsi->back;
 	enum ice_fec_mode fec;
 
 	switch (fecparam->fec) {
+	case ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF:
+		if (!ice_fw_supports_fec_dis_auto(&pf->hw)) {
+			NL_SET_ERR_MSG_MOD(extack, "FEC automatic disable is not supported by current firmware\n");
+			return -EOPNOTSUPP;
+		}
+		fec = ICE_FEC_DIS_AUTO;
+		break;
 	case ETHTOOL_FEC_AUTO:
 		fec = ICE_FEC_AUTO;
 		break;
@@ -1037,8 +1045,7 @@ ice_set_fecparam(struct net_device *netdev, struct ethtool_fecparam *fecparam,
 		fec = ICE_FEC_NONE;
 		break;
 	default:
-		dev_warn(ice_pf_to_dev(vsi->back), "Unsupported FEC mode: %d\n",
-			 fecparam->fec);
+		NL_SET_ERR_MSG_MOD(extack, "Requested FEC mode is not supported\n");
 		return -EINVAL;
 	}
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 3dbd91d8d83e..98fc40f20abd 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -2190,7 +2190,8 @@ static int ice_configure_phy(struct ice_vsi *vsi)
 	ice_cfg_phy_fec(pi, cfg, phy->curr_user_fec_req);
 
 	/* Can't provide what was requested; use PHY capabilities */
-	if (cfg->link_fec_opt !=
+	if (phy->curr_user_fec_req != ICE_FEC_DIS_AUTO &&
+	    cfg->link_fec_opt !=
 	    (cfg->link_fec_opt & pcaps->link_fec_options)) {
 		cfg->caps |= pcaps->caps & ICE_AQC_PHY_EN_AUTO_FEC;
 		cfg->link_fec_opt = pcaps->link_fec_options;
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index e1abfcee96dc..0b71c40e881b 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -107,7 +107,8 @@ enum ice_fec_mode {
 	ICE_FEC_NONE = 0,
 	ICE_FEC_RS,
 	ICE_FEC_BASER,
-	ICE_FEC_AUTO
+	ICE_FEC_AUTO,
+	ICE_FEC_DIS_AUTO
 };
 
 struct ice_phy_cache_mode_data {
@@ -1145,4 +1146,10 @@ struct ice_aq_get_set_rss_lut_params {
 #define ICE_FW_API_REPORT_DFLT_CFG_MIN		7
 #define ICE_FW_API_REPORT_DFLT_CFG_PATCH	3
 
+/* FW version for FEC disable in Auto FEC mode */
+#define ICE_FW_FEC_DIS_AUTO_BRANCH	1
+#define ICE_FW_FEC_DIS_AUTO_MAJ		7
+#define ICE_FW_FEC_DIS_AUTO_MIN		0
+#define ICE_FW_FEC_DIS_AUTO_PATCH	5
+
 #endif /* _ICE_TYPE_H_ */
-- 
2.37.1.394.gc50926e1f488

Re: [PATCH net-next 1/2] ethtool: pass netlink extended ACK to .set_fecparam

[Simon Horman]

On Tue, Aug 23, 2022 at 08:04:37AM -0700, Jacob Keller wrote:
> Add the netlink extended ACK structure pointer to the interface for
> .set_fecparam. This allows reporting errors to the user appropriately when
> using the netlink ethtool interface.

FWIIW, for the, trivial, nfp portion of this change:

Acked-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 1/2] ethtool: pass netlink extended ACK to .set_fecparam

[Jakub Kicinski]

On Tue, 23 Aug 2022 08:04:37 -0700 Jacob Keller wrote:
> Add the netlink extended ACK structure pointer to the interface for
> .set_fecparam. This allows reporting errors to the user appropriately when
> using the netlink ethtool interface.

Could you wrap it into a structure perhaps?

Would be good if we didn't have to modify the signature of the callback
next time we need to extend it (especially since struct ethtool_fecparam
is ioctl uABI so we can't really add fields there).

Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

[Jakub Kicinski]

On Tue, 23 Aug 2022 08:04:38 -0700 Jacob Keller wrote:
> The default Link Establishment State Machine (LESM) behavior does not

LESM is the algo as specified by the IEEE standard? If so could you add
the citation (section of the spec where it's defined)?

Is disabling the only customization we may want?

> allow the use of FEC disabled if the media does not support FEC
> disabled. However users may want to override this behavior.
> 
> To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a request
> to automatically select an appropriate FEC mode including potentially
> disabling FEC.
> 
> This is distinct from ETHTOOL_FEC_AUTO because that will not allow the LESM
> to select FEC disabled. It is distinct from ETHTOOL_FEC_OFF because
> FEC_OFF will always disable FEC without any LESM automatic selection.
> 
> This *does* mean that ice is now accepting one "bitwise OR" set for FEC
> configuration, which is somewhat against the recommendations made in
> 6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not
> sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make any
> sense here.
> 
> With this change, users can opt to allow automatic FEC disable via
> 
>   ethtool --set-fec ethX encoding auto off

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Gal Pressman]

On 23/08/2022 18:04, Jacob Keller wrote:
> 2) always treat ETHTOOL_FEC_AUTO as "automatic + allow disable"
>
>   This could work, but it means that behavior will differ depending on the
>   firmware version. Users have no way to know that and might be surprised to
>   find the behavior differ across devices which have different firmware
>   which do or don't support this variation of automatic selection.

Hi Jacob,
This is exactly how it's already implemented in mlx5, and I don't really
understand how firmware version is related? Is it specific to your
device firmware?
Maybe you can workaround that in the driver?

I feel like we're going the wrong way here having different flags
interpretations by different drivers.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jakub Kicinski]

On Wed, 24 Aug 2022 16:35:41 +0300 Gal Pressman wrote:
> On 23/08/2022 18:04, Jacob Keller wrote:
> > 2) always treat ETHTOOL_FEC_AUTO as "automatic + allow disable"
> >
> >   This could work, but it means that behavior will differ depending on the
> >   firmware version. Users have no way to know that and might be surprised to
> >   find the behavior differ across devices which have different firmware
> >   which do or don't support this variation of automatic selection.  
> 
> Hi Jacob,
> This is exactly how it's already implemented in mlx5, and I don't really
> understand how firmware version is related? Is it specific to your
> device firmware?
> Maybe you can workaround that in the driver?
> 
> I feel like we're going the wrong way here having different flags
> interpretations by different drivers.

Hm, according to my notes the drivers supporting a single bit and
multiple bits were evenly split when I wrote the docs. Either way
we're going to make someone unhappy?

While we're talking about mlx5 FEC, what does it report on get?
I wasn't sure if it reports the supported or configured mode.

RE: [PATCH net-next 1/2] ethtool: pass netlink extended ACK to .set_fecparam

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, August 23, 2022 3:14 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Michael Chan <michael.chan@broadcom.com>; Eric
> Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; Derek
> Chickles <dchickles@marvell.com>; Satanand Burla <sburla@marvell.com>; Felix
> Manlunas <fmanlunas@marvell.com>; Raju Rangoju <rajur@chelsio.com>;
> Dimitris Michailidis <dmichail@fungible.com>; Yisen Zhuang
> <yisen.zhuang@huawei.com>; Salil Mehta <salil.mehta@huawei.com>;
> Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Sunil Goutham <sgoutham@marvell.com>;
> Geetha sowjanya <gakula@marvell.com>; Subbaraya Sundeep
> <sbhatta@marvell.com>; hariprasad <hkelam@marvell.com>; Taras Chornyi
> <tchornyi@marvell.com>; Saeed Mahameed <saeedm@nvidia.com>; Leon
> Romanovsky <leon@kernel.org>; Simon Horman
> <simon.horman@corigine.com>; Shannon Nelson <snelson@pensando.io>; Ariel
> Elior <aelior@marvell.com>; Manish Chopra <manishc@marvell.com>; Edward
> Cree <ecree.xilinx@gmail.com>; Martin Habets <habetsm.xilinx@gmail.com>; Fei
> Qin <fei.qin@corigine.com>; Louis Peens <louis.peens@corigine.com>; Yu Xiao
> <yu.xiao@corigine.com>; Uwe Kleine-König <u.kleine-koenig@pengutronix.de>;
> Yufeng Mo <moyufeng@huawei.com>; Sixiang Chen
> <sixiang.chen@corigine.com>; Yinjun Zhang <yinjun.zhang@corigine.com>; Hao
> Chen <chenhao288@hisilicon.com>; Guangbin Huang
> <huangguangbin2@huawei.com>; Sean Anderson <sean.anderson@seco.com>;
> Erik Ekman <erik@kryo.se>; Ido Schimmel <idosch@nvidia.com>; Jie Wang
> <wangjie125@huawei.com>; Moshe Tal <moshet@nvidia.com>; Tonghao Zhang
> <xiangxia.m.yue@gmail.com>; Marco Bonelli <marco@mebeim.net>; Gustavo A.
> R. Silva <gustavoars@kernel.org>
> Subject: Re: [PATCH net-next 1/2] ethtool: pass netlink extended ACK to
> .set_fecparam
> 
> On Tue, 23 Aug 2022 08:04:37 -0700 Jacob Keller wrote:
> > Add the netlink extended ACK structure pointer to the interface for
> > .set_fecparam. This allows reporting errors to the user appropriately when
> > using the netlink ethtool interface.
> 
> Could you wrap it into a structure perhaps?
> 
> Would be good if we didn't have to modify the signature of the callback
> next time we need to extend it (especially since struct ethtool_fecparam
> is ioctl uABI so we can't really add fields there).

Yea that makes sense.

Thanks,
Jake

RE: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, August 23, 2022 3:18 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Greenwalt, Paul <paul.greenwalt@intel.com>
> Subject: Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled
> via ETHTOOL_SFECPARAM
> 
> On Tue, 23 Aug 2022 08:04:38 -0700 Jacob Keller wrote:
> > The default Link Establishment State Machine (LESM) behavior does not
> 
> LESM is the algo as specified by the IEEE standard? If so could you add
> the citation (section of the spec where it's defined)?
> 

Hm. I am not sure if its part of IEEE standard or if its just the name we used for that section of our firmware. I'll get back on this and if its not part of a standard I'll rephrase this a bit so thats clear.

> Is disabling the only customization we may want?
> 

Of that, I'm not sure.

> > allow the use of FEC disabled if the media does not support FEC
> > disabled. However users may want to override this behavior.
> >
> > To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a
> request
> > to automatically select an appropriate FEC mode including potentially
> > disabling FEC.
> >
> > This is distinct from ETHTOOL_FEC_AUTO because that will not allow the LESM
> > to select FEC disabled. It is distinct from ETHTOOL_FEC_OFF because
> > FEC_OFF will always disable FEC without any LESM automatic selection.
> >
> > This *does* mean that ice is now accepting one "bitwise OR" set for FEC
> > configuration, which is somewhat against the recommendations made in
> > 6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not
> > sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make
> any
> > sense here.
> >
> > With this change, users can opt to allow automatic FEC disable via
> >
> >   ethtool --set-fec ethX encoding auto off

RE: [PATCH net-next 0/2] ice: support FEC automatic disable

[Keller, Jacob E]

> -----Original Message-----
> From: Gal Pressman <gal@nvidia.com>
> Sent: Wednesday, August 24, 2022 6:36 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Saeed Mahameed <saeedm@nvidia.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
> 
> On 23/08/2022 18:04, Jacob Keller wrote:
> > 2) always treat ETHTOOL_FEC_AUTO as "automatic + allow disable"
> >
> >   This could work, but it means that behavior will differ depending on the
> >   firmware version. Users have no way to know that and might be surprised to
> >   find the behavior differ across devices which have different firmware
> >   which do or don't support this variation of automatic selection.
> 
> Hi Jacob,
> This is exactly how it's already implemented in mlx5, and I don't really
> understand how firmware version is related? Is it specific to your
> device firmware?
> Maybe you can workaround that in the driver?

For ice, the original "auto" implementation (which is handled by firmware) will automatically select an FEC mode based on the media type and using a state machine to go through options until it finds a valid link.

This implementation would never select "No FEC" (i.e. FEC_OFF) for certain module types which do not list "No FEC" as part of their auto negotiation supported list. (Despite this not actually being auto negotiation). Some of our customers were surprised by this and asked if we could change it, so new firmware has an option to allow choosing "No FEC". This is an "opt-in" that the driver must tell firmware when setting up FEC mode. This obviously is only available on newer firmware. Going with option 2 would result in differing behavior depending on what firmware and driver you're using.

I thought that was a bit confusing since userspace/users would not know which variant is in use, and my understanding from our customer engineers is that we don't want to change the behavior without an explicit request of some kind. That's where the original private flag came from.

As for working around this in the driver, I am not sure how feasible that is.

> 
> I feel like we're going the wrong way here having different flags
> interpretations by different drivers.

I would prefer to avoid that as well

Thanks,
Jake 

RE: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, August 23, 2022 3:18 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Greenwalt, Paul <paul.greenwalt@intel.com>
> Subject: Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled
> via ETHTOOL_SFECPARAM
> 
> On Tue, 23 Aug 2022 08:04:38 -0700 Jacob Keller wrote:
> > The default Link Establishment State Machine (LESM) behavior does not
> 
> LESM is the algo as specified by the IEEE standard? If so could you add
> the citation (section of the spec where it's defined)?
> 
> Is disabling the only customization we may want?
> 

Ok I got information from the other folks here. LESM is not a standard its just the name we used internally for how the firmware establishes link. I'll rephrase this whole section and clarify it.

I also clarified whats going on here in

https://lore.kernel.org/netdev/CO1PR11MB50895C42C04A408CF6151023D6739@CO1PR11MB5089.namprd11.prod.outlook.com/

Basically: the firmware has a process for automatically selecting FEC mode. On older firmware, this process didn't include the possibility of selecting "No FEC", or in other words of disabling FEC. The process firmware uses is a state machine that goes through the FEC modes known to be supported by the media type.

Some of our customers were confused about this and have asked if it was possible to allow disabling FEC. This is distinct from manually setting FEC_OFF, because it lets the firmwares existing state machine determine of the disabled mode is suitable or not. I understand it as the goal of being able to say "automatically select FEC for me, but if No FEC is suitable allow that".

The new firmware requires manually opting in with a new bit, and we don't want to change existing behavior, hence the new approach to using both AUTO and OFF together. If we instead go with "use the new mode when its available" then there is no way for users to know this easily since it would just depend on the firmware version.

I've got a proposed reword to the commit message as follows:

    Users can request automatic selection of FEC mode via the
    ETHTOOL_MSG_FEC_SET netlink message (or ETHTOOL_SFECPARAM ioctl). The ice
    driver implements this by asking firmware to select a suitable mode.

    The firmware selects a FEC mode automatically based on a table of supported
    FEC modes for the media type. Older versions of firmware will never select
    "No FEC" (i.e. disabling FEC).

    Newer versions of firmware support an automatic mode which also allows
    selecting the "No FEC" mode.

    To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a request
    to automatically select an appropriate FEC mode including potentially
    disabling FEC.

    This is done so that the existing behavior of ETHTOOL_FEC_AUTO remains
    unchanged, and users must actively select the new behavior. This is
    important since we do not want to change the behavior purely based on the
    firmware version. Additionally, this allows reporting an error if the mode
    is requested on a device still operating the older firmware version.

    This is distinct from ETHTOOL_FEC_OFF because that selection will always
    simply disable FEC without going through the firmware automatic selection
    state machine.

    This *does* mean that ice is now accepting one "bitwise OR" set for FEC
    configuration, which is somewhat against the recommendations made in
    6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not
    sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make any
    sense here.

Is that a better explanation of the reasoning?

Thanks,
Jake

> > allow the use of FEC disabled if the media does not support FEC
> > disabled. However users may want to override this behavior.
> >
> > To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a
> request
> > to automatically select an appropriate FEC mode including potentially
> > disabling FEC.
> >
> > This is distinct from ETHTOOL_FEC_AUTO because that will not allow the LESM
> > to select FEC disabled. It is distinct from ETHTOOL_FEC_OFF because
> > FEC_OFF will always disable FEC without any LESM automatic selection.
> >
> > This *does* mean that ice is now accepting one "bitwise OR" set for FEC
> > configuration, which is somewhat against the recommendations made in
> > 6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not
> > sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make
> any
> > sense here.
> >
> > With this change, users can opt to allow automatic FEC disable via
> >
> >   ethtool --set-fec ethX encoding auto off

Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

[Jakub Kicinski]

On Wed, 24 Aug 2022 21:29:31 +0000 Keller, Jacob E wrote:
> Ok I got information from the other folks here. LESM is not a
> standard its just the name we used internally for how the firmware
> establishes link. I'll rephrase this whole section and clarify it.

Hold up, I'm pretty sure this is in the standard.

RE: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, August 24, 2022 3:47 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Greenwalt, Paul <paul.greenwalt@intel.com>
> Subject: Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled
> via ETHTOOL_SFECPARAM
> 
> On Wed, 24 Aug 2022 21:29:31 +0000 Keller, Jacob E wrote:
> > Ok I got information from the other folks here. LESM is not a
> > standard its just the name we used internally for how the firmware
> > establishes link. I'll rephrase this whole section and clarify it.
> 
> Hold up, I'm pretty sure this is in the standard.

According to the folks I talked to what we have here, we didn't understand this as being from a standard, but if it is I'd love to read on it.

Thanks,
Jake

Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

[Jakub Kicinski]

On Wed, 24 Aug 2022 22:53:44 +0000 Keller, Jacob E wrote:
> > On Wed, 24 Aug 2022 21:29:31 +0000 Keller, Jacob E wrote:  
> > > Ok I got information from the other folks here. LESM is not a
> > > standard its just the name we used internally for how the firmware
> > > establishes link. I'll rephrase this whole section and clarify it.  
> > 
> > Hold up, I'm pretty sure this is in the standard.  
> 
> According to the folks I talked to what we have here, we didn't
> understand this as being from a standard, but if it is I'd love to
> read on it.

Table 110C–1—Host and cable assembly combinations
in IEEE 802.3 2018, that's what I was thinking of.

RE: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, August 24, 2022 4:03 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Greenwalt, Paul <paul.greenwalt@intel.com>
> Subject: Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled
> via ETHTOOL_SFECPARAM
> 
> On Wed, 24 Aug 2022 22:53:44 +0000 Keller, Jacob E wrote:
> > > On Wed, 24 Aug 2022 21:29:31 +0000 Keller, Jacob E wrote:
> > > > Ok I got information from the other folks here. LESM is not a
> > > > standard its just the name we used internally for how the firmware
> > > > establishes link. I'll rephrase this whole section and clarify it.
> > >
> > > Hold up, I'm pretty sure this is in the standard.
> >
> > According to the folks I talked to what we have here, we didn't
> > understand this as being from a standard, but if it is I'd love to
> > read on it.
> 
> Table 110C–1—Host and cable assembly combinations
> in IEEE 802.3 2018, that's what I was thinking of.

Ah. I am not sure if the state machine in firmware uses this table or not, but my guess is that it does.

Thanks,
Jake

Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL_SFECPARAM

[Jakub Kicinski]

On Wed, 24 Aug 2022 23:13:47 +0000 Keller, Jacob E wrote:
> > > According to the folks I talked to what we have here, we didn't
> > > understand this as being from a standard, but if it is I'd love to
> > > read on it.  
> > 
> > Table 110C–1—Host and cable assembly combinations
> > in IEEE 802.3 2018, that's what I was thinking of.  
> 
> Ah. I am not sure if the state machine in firmware uses this table or
> not, but my guess is that it does.

Yeah, I thought that's what AUTO means, otherwise if everyone did their
own thing we wouldn't get link up without real autoneg enabled, right?
Now the table only specifies minimum FEC requirements, in your case (and
perhaps for others) No FEC does not get used, presumably the selection
is only done between R and RS?

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Gal Pressman]

On 24/08/2022 20:40, Keller, Jacob E wrote:
>
>> -----Original Message-----
>> From: Gal Pressman <gal@nvidia.com>
>> Sent: Wednesday, August 24, 2022 6:36 AM
>> To: Keller, Jacob E <jacob.e.keller@intel.com>
>> Cc: Saeed Mahameed <saeedm@nvidia.com>; netdev@vger.kernel.org
>> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
>>
>> On 23/08/2022 18:04, Jacob Keller wrote:
>>> 2) always treat ETHTOOL_FEC_AUTO as "automatic + allow disable"
>>>
>>>   This could work, but it means that behavior will differ depending on the
>>>   firmware version. Users have no way to know that and might be surprised to
>>>   find the behavior differ across devices which have different firmware
>>>   which do or don't support this variation of automatic selection.
>> Hi Jacob,
>> This is exactly how it's already implemented in mlx5, and I don't really
>> understand how firmware version is related? Is it specific to your
>> device firmware?
>> Maybe you can workaround that in the driver?
> For ice, the original "auto" implementation (which is handled by firmware) will automatically select an FEC mode based on the media type and using a state machine to go through options until it finds a valid link.
>
> This implementation would never select "No FEC" (i.e. FEC_OFF) for certain module types which do not list "No FEC" as part of their auto negotiation supported list. (Despite this not actually being auto negotiation). Some of our customers were surprised by this and asked if we could change it, so new firmware has an option to allow choosing "No FEC". This is an "opt-in" that the driver must tell firmware when setting up FEC mode. This obviously is only available on newer firmware. Going with option 2 would result in differing behavior depending on what firmware and driver you're using.
>
> I thought that was a bit confusing since userspace/users would not know which variant is in use, and my understanding from our customer engineers is that we don't want to change the behavior without an explicit request of some kind. That's where the original private flag came from.
>
> As for working around this in the driver, I am not sure how feasible that is.
>
>> I feel like we're going the wrong way here having different flags
>> interpretations by different drivers.
> I would prefer to avoid that as well

Then maybe adding a new flag is the right thing to do here.

That way the existing auto mode will keep its current meaning (all modes
including off), which you'll be able to support on newer firmware
versions, and the new auto flag (all modes excluding off) will be
supported on all firmware versions.
Then maybe we can even add support for the new flag in mlx5 (I need to
check whether that's feasible with our hardware).

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Gal Pressman]

On 24/08/2022 19:25, Jakub Kicinski wrote:
> On Wed, 24 Aug 2022 16:35:41 +0300 Gal Pressman wrote:
>> On 23/08/2022 18:04, Jacob Keller wrote:
>>> 2) always treat ETHTOOL_FEC_AUTO as "automatic + allow disable"
>>>
>>>   This could work, but it means that behavior will differ depending on the
>>>   firmware version. Users have no way to know that and might be surprised to
>>>   find the behavior differ across devices which have different firmware
>>>   which do or don't support this variation of automatic selection.  
>> Hi Jacob,
>> This is exactly how it's already implemented in mlx5, and I don't really
>> understand how firmware version is related? Is it specific to your
>> device firmware?
>> Maybe you can workaround that in the driver?
>>
>> I feel like we're going the wrong way here having different flags
>> interpretations by different drivers.
> Hm, according to my notes the drivers supporting a single bit and
> multiple bits were evenly split when I wrote the docs. Either way
> we're going to make someone unhappy?

IMHO, passing both auto and off is confusing.
BTW, I think this is also inconsistent with the direction SONiC took (or
is taking?) with the auto fec mode interpretation?

> While we're talking about mlx5 FEC, what does it report on get?
> I wasn't sure if it reports the supported or configured mode.

I think it's the configured mode and the active mode.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jakub Kicinski]

On Thu, 25 Aug 2022 10:08:05 +0300 Gal Pressman wrote:
> Then maybe adding a new flag is the right thing to do here.
> 
> That way the existing auto mode will keep its current meaning (all modes
> including off), which you'll be able to support on newer firmware
> versions, and the new auto flag (all modes excluding off) will be
> supported on all firmware versions.
> Then maybe we can even add support for the new flag in mlx5 (I need to
> check whether that's feasible with our hardware).

Sorry, I misinterpreted your previous reply, somehow I thought you
quoted option (3), because my fallible reading of mlx5 was that it
accepts multiple flags.

(First) option 2 is fine.

Do you happen to have a link to what SONiC defined?
We really need to establish some expectations before we start extending
the API. Naively I thought the IEEE spec was more prescriptive :(

RE: [PATCH net-next 0/2] ice: support FEC automatic disable

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, August 25, 2022 9:30 AM
> To: Gal Pressman <gal@nvidia.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Saeed Mahameed
> <saeedm@nvidia.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
> 
> On Thu, 25 Aug 2022 10:08:05 +0300 Gal Pressman wrote:
> > Then maybe adding a new flag is the right thing to do here.
> >
> > That way the existing auto mode will keep its current meaning (all modes
> > including off), which you'll be able to support on newer firmware
> > versions, and the new auto flag (all modes excluding off) will be
> > supported on all firmware versions.
> > Then maybe we can even add support for the new flag in mlx5 (I need to
> > check whether that's feasible with our hardware).
> 
> Sorry, I misinterpreted your previous reply, somehow I thought you
> quoted option (3), because my fallible reading of mlx5 was that it
> accepts multiple flags.
> 
> (First) option 2 is fine.
> 


Even though existing behavior doesn't do that for ice right now and wouldn't be able to do that properly with old firmware?

Thanks,
Jake

> Do you happen to have a link to what SONiC defined?
> We really need to establish some expectations before we start extending
> the API. Naively I thought the IEEE spec was more prescriptive :(

Yea its a bit unfortunate :(

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jakub Kicinski]

On Thu, 25 Aug 2022 16:57:50 +0000 Keller, Jacob E wrote:
> > Sorry, I misinterpreted your previous reply, somehow I thought you
> > quoted option (3), because my fallible reading of mlx5 was that it
> > accepts multiple flags.
> > 
> > (First) option 2 is fine.
> 
> Even though existing behavior doesn't do that for ice right now and
> wouldn't be able to do that properly with old firmware?

Update your FW seems like a reasonable thing to ask customers to do.
Are there cables already qualified for the old FW which would switch
to No FEC under new rules?

Can you share how your FW picks the mode exactly?

There must be _some_ standardization here, because we're talking about
<5m cables, so we can safely assume it's linking to a ToR switch.

RE: [PATCH net-next 0/2] ice: support FEC automatic disable

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, August 25, 2022 10:30 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Gal Pressman <gal@nvidia.com>; Saeed Mahameed <saeedm@nvidia.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
> 
> On Thu, 25 Aug 2022 16:57:50 +0000 Keller, Jacob E wrote:
> > > Sorry, I misinterpreted your previous reply, somehow I thought you
> > > quoted option (3), because my fallible reading of mlx5 was that it
> > > accepts multiple flags.
> > >
> > > (First) option 2 is fine.
> >
> > Even though existing behavior doesn't do that for ice right now and
> > wouldn't be able to do that properly with old firmware?
> 
> Update your FW seems like a reasonable thing to ask customers to do.
> Are there cables already qualified for the old FW which would switch
> to No FEC under new rules?

Not sure I follow what you're asking here.

> 
> Can you share how your FW picks the mode exactly?
> 

I'm not entirely sure how it selects, other than it selects from the table of supported modes. It uses some state machine to go through options until a suitable link is made, but the details of how exactly it does this I'm not quite sure.

The old firmware never picks "No FEC" for some media types, because the standard was that those types would always require FEC of some kind (R or RS). However in practice the modules can work with no FEC anyways, and according to customer reports enabling this has helped with linking issues. That's the sum of my understanding thus far.

I would prefer this option of just "auto means also possibly disable", but I wasn't sure how other devices worked and we had some concerns about the change in behavior. Going with this option would significantly simplify things since I could bury the "set the auto disable flag if necessary" into a lower level of the code and only expose a single ICE_FEC_AUTO instead of ICE_FEC_AUTO_DIS...

Thus, I'm happy to respin this, as the new behavior when supported by firmware if we have some consensus there. I am happy to drop or respin the extack changes if you think thats still valuable as well in the event we need to extend that API in the future.

> There must be _some_ standardization here, because we're talking about
> <5m cables, so we can safely assume it's linking to a ToR switch.

I believe so.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jakub Kicinski]

On Thu, 25 Aug 2022 17:51:01 +0000 Keller, Jacob E wrote:
> > Update your FW seems like a reasonable thing to ask customers to do.
> > Are there cables already qualified for the old FW which would switch
> > to No FEC under new rules?  
> 
> Not sure I follow what you're asking here.

IIRC your NICs only work with qualified cables. I was asking if any of
the qualified cables would actually negotiate to No FEC under new rules.
Maybe such cables are very rare and there's no need to be super careful?

> > Can you share how your FW picks the mode exactly?
> 
> I'm not entirely sure how it selects, other than it selects from the
> table of supported modes. It uses some state machine to go through
> options until a suitable link is made, but the details of how exactly
> it does this I'm not quite sure.

State machine? So you're trying different FEC modes or reading module
data and picking one?

Various NICs do either or both, but I believe AUTO means the former.

> The old firmware never picks "No FEC" for some media types, because
> the standard was that those types would always require FEC of some
> kind (R or RS). However in practice the modules can work with no FEC
> anyways, and according to customer reports enabling this has helped
> with linking issues. That's the sum of my understanding thus far.

Well, according to the IEEE standard there sure are cables which don't
need FEC. Maybe your customers had problems linking up because switch
had a different selection algo?

> I would prefer this option of just "auto means also possibly
> disable", but I wasn't sure how other devices worked and we had some
> concerns about the change in behavior. Going with this option would
> significantly simplify things since I could bury the "set the auto
> disable flag if necessary" into a lower level of the code and only
> expose a single ICE_FEC_AUTO instead of ICE_FEC_AUTO_DIS...
> 
> Thus, I'm happy to respin this, as the new behavior when supported by
> firmware if we have some consensus there.

Hard to get consensus if we still don't know what the FW does...
But if there's no new uAPI, just always enabling OFF with AUTO
then I guess I'd have nothing to complain about as I don't know
what other drivers do either.

> I am happy to drop or
> respin the extack changes if you think thats still valuable as well
> in the event we need to extend that API in the future.

Your call. May be useful to do it sooner rather than later, but 
we should find at least one use for the extack as a justification.

> > There must be _some_ standardization here, because we're talking
> > about <5m cables, so we can safely assume it's linking to a ToR
> > switch.  
> 
> I believe so.

RE: [PATCH net-next 0/2] ice: support FEC automatic disable

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, August 25, 2022 1:34 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Gal Pressman <gal@nvidia.com>; Saeed Mahameed <saeedm@nvidia.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
> 
> On Thu, 25 Aug 2022 17:51:01 +0000 Keller, Jacob E wrote:
> > > Update your FW seems like a reasonable thing to ask customers to do.
> > > Are there cables already qualified for the old FW which would switch
> > > to No FEC under new rules?
> >
> > Not sure I follow what you're asking here.
> 
> IIRC your NICs only work with qualified cables. I was asking if any of
> the qualified cables would actually negotiate to No FEC under new rules.
> Maybe such cables are very rare and there's no need to be super careful?
> 

I am not sure if that would be related

> > > Can you share how your FW picks the mode exactly?
> >
> > I'm not entirely sure how it selects, other than it selects from the
> > table of supported modes. It uses some state machine to go through
> > options until a suitable link is made, but the details of how exactly
> > it does this I'm not quite sure.
> 
> State machine? So you're trying different FEC modes or reading module
> data and picking one?
> 

I believe it is trying different modes, but it is selecting from module data for what to try.

> Various NICs do either or both, but I believe AUTO means the former.
> 
> > The old firmware never picks "No FEC" for some media types, because
> > the standard was that those types would always require FEC of some
> > kind (R or RS). However in practice the modules can work with no FEC
> > anyways, and according to customer reports enabling this has helped
> > with linking issues. That's the sum of my understanding thus far.
> 
> Well, according to the IEEE standard there sure are cables which don't
> need FEC. Maybe your customers had problems linking up because switch
> had a different selection algo?
> 

Right. I believe its because some combinations in their module data don't list No FEC, but still work with No FEC. The understanding I got from the firmware person I've asked was that the list of whats supported is recorded in the module somehow.

> > I would prefer this option of just "auto means also possibly
> > disable", but I wasn't sure how other devices worked and we had some
> > concerns about the change in behavior. Going with this option would
> > significantly simplify things since I could bury the "set the auto
> > disable flag if necessary" into a lower level of the code and only
> > expose a single ICE_FEC_AUTO instead of ICE_FEC_AUTO_DIS...
> >
> > Thus, I'm happy to respin this, as the new behavior when supported by
> > firmware if we have some consensus there.
> 
> Hard to get consensus if we still don't know what the FW does...
> But if there's no new uAPI, just always enabling OFF with AUTO
> then I guess I'd have nothing to complain about as I don't know
> what other drivers do either.
> 

It's been frustratingly difficult to get real answers here.

> > I am happy to drop or
> > respin the extack changes if you think thats still valuable as well
> > in the event we need to extend that API in the future.
> 
> Your call. May be useful to do it sooner rather than later, but
> we should find at least one use for the extack as a justification.
> 

There is at least one error in ice, and I'll update other drivers that print error messages too if I find any.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jacob Keller]

On 8/25/2022 1:34 PM, Jakub Kicinski wrote:
> Hard to get consensus if we still don't know what the FW does...
> But if there's no new uAPI, just always enabling OFF with AUTO
> then I guess I'd have nothing to complain about as I don't know
> what other drivers do either.
> 
Ok. I think I have a basic summary of the situation and whats going on
in the firmware. I'll try to summarize here:

Firmware has a state machine that we call the Link Establishment State
Machine. This is the state machine which will attempt to establish link.
This state machine only applies when auto-negotiation is not used. If
auto-negotation is used it will perform the standard auto-negotiation
flow and set FEC through that method.

The way this works follows this flow:

1) driver sends a set PHY capabilities request. This includes various
bits including whether to automatically select FEC, and which FEC modes
to select from. When we enable ETHTOOL_FEC_AUTO, the driver always sets
all FEC modes with the auto FEC bit.

2) the firmware receives this request and begins the LESM. This starts
with the firmware generating a list of configurations to attempt. Each
configuration is a possible link mode combined with a bitwise AND of the
FEC modes requested above in set PHY capabiltiies and the set of FEC
modes supported by that link mode. The example I gave was if you plugged
in a CA-L cable, it would try:

  100G-CAUI4
  50G-LAUI2
  25G-AUI-C2C
  10G-SFI-DA

I'm still not 100% sure how it decides which link modes to choose for
which cable, but I believe this is in a table stored within the firmware
module we call the netlist.

2a) for older firmware, the set PHY capabiltiies interface does not have
a bit to set for requesting No FEC. Instead, each media type has a
determination made about whether it needed FEC of not. I was told for
example that CA-N cables would enable No FEC as an option, but CA-L
cables would not (even though No FEC is supported for the link modes in
question).

2b) on newer firmware, the set PHY capabilities interface does have a
bit to request No FEC. In this case, if we set the No FEC bit, then the
firmware will be able to select No FEC as an option for cables that
otherwise wouldn't have selected it in the old firmware (such as CA-L
cables mentioned above).

3) once the firmware has generated the list of possible configurations,
it will iterate through them in a loop. Each configuration is applied,
and then we wait some time (the timeout is also stored in the netlist
module). If link establishes at one of these phases, we stop and use
that configuration. Otherwise we move to the next configuration and try
that. Each FEC mode is tried in sequence. (Unless the automatic FEC
selection bit is *not* set. In that case, only one of the FEC modes is
tried instead, and it is expected that software only set one bit to try.
That would perform forced FEC selection instead).

This process will repeat as it iterates through the configurations until
link is established.

As a side note, the first stage is to try auto-negotiation if enabled.
So in the case where auto-negotiation is enabled it will first try
auto-negotiation, then the set of forced configurations, and then loop
back to trying auto-negotiation before trying the forced configs again.

So from the software programming state, we currently translate
ETHTOOL_FEC_AUTO by setting the automatic bit as well as setting every
FEC mode bit, except the "No FEC" bit. This is a new bit which is only
available on newer firmware.

With the proposed change, we would add the "No FEC" bit when user
requested both ETHTOOL_FEC_AUTO and ETHTOOL_FEC_OFF simultaneously.

From reading your previous replies, you would prefer to just have the
driver set the "No FEC" bit always for ETHTOOL_FEC_AUTO when its
available/supported by firmware?

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jakub Kicinski]

On Thu, 25 Aug 2022 17:38:14 -0700 Jacob Keller wrote:
> On 8/25/2022 1:34 PM, Jakub Kicinski wrote:
> > Hard to get consensus if we still don't know what the FW does...
> > But if there's no new uAPI, just always enabling OFF with AUTO
> > then I guess I'd have nothing to complain about as I don't know
> > what other drivers do either.
> >   
> Ok. I think I have a basic summary of the situation and whats going on
> in the firmware. I'll try to summarize here:
> 
> Firmware has a state machine that we call the Link Establishment State
> Machine. This is the state machine which will attempt to establish link.
> This state machine only applies when auto-negotiation is not used. If
> auto-negotation is used it will perform the standard auto-negotiation
> flow and set FEC through that method.
> 
> The way this works follows this flow:
> 
> 1) driver sends a set PHY capabilities request. This includes various
> bits including whether to automatically select FEC, and which FEC modes
> to select from. When we enable ETHTOOL_FEC_AUTO, the driver always sets
> all FEC modes with the auto FEC bit.
> 
> 2) the firmware receives this request and begins the LESM. This starts
> with the firmware generating a list of configurations to attempt. Each
> configuration is a possible link mode combined with a bitwise AND of the
> FEC modes requested above in set PHY capabiltiies and the set of FEC
> modes supported by that link mode. The example I gave was if you plugged
> in a CA-L cable, it would try:
> 
>   100G-CAUI4
>   50G-LAUI2
>   25G-AUI-C2C
>   10G-SFI-DA
> 
> I'm still not 100% sure how it decides which link modes to choose for
> which cable, but I believe this is in a table stored within the firmware
> module we call the netlist.
> 
> 2a) for older firmware, the set PHY capabiltiies interface does not have
> a bit to set for requesting No FEC. Instead, each media type has a
> determination made about whether it needed FEC of not. I was told for
> example that CA-N cables would enable No FEC as an option, but CA-L
> cables would not (even though No FEC is supported for the link modes in
> question).
> 
> 2b) on newer firmware, the set PHY capabilities interface does have a
> bit to request No FEC. In this case, if we set the No FEC bit, then the
> firmware will be able to select No FEC as an option for cables that
> otherwise wouldn't have selected it in the old firmware (such as CA-L
> cables mentioned above).

Oh, but per the IEEE standard No FEC is _not_ an option for CA-L.
From the initial reading of your series I thought that Intel NICs 
would _never_ pick No FEC.

Sounds like we need a bit for "ignore the standard and try everything".

What about BASE-R FEC? Is the FW going to try it on the CA-L cable?

> 3) once the firmware has generated the list of possible configurations,
> it will iterate through them in a loop. Each configuration is applied,
> and then we wait some time (the timeout is also stored in the netlist
> module). If link establishes at one of these phases, we stop and use
> that configuration. Otherwise we move to the next configuration and try
> that. Each FEC mode is tried in sequence. (Unless the automatic FEC
> selection bit is *not* set. In that case, only one of the FEC modes is
> tried instead, and it is expected that software only set one bit to try.
> That would perform forced FEC selection instead).
> 
> This process will repeat as it iterates through the configurations until
> link is established.
> 
> As a side note, the first stage is to try auto-negotiation if enabled.
> So in the case where auto-negotiation is enabled it will first try
> auto-negotiation, then the set of forced configurations, and then loop
> back to trying auto-negotiation before trying the forced configs again.
> 
> So from the software programming state, we currently translate
> ETHTOOL_FEC_AUTO by setting the automatic bit as well as setting every
> FEC mode bit, except the "No FEC" bit. This is a new bit which is only
> available on newer firmware.
> 
> With the proposed change, we would add the "No FEC" bit when user
> requested both ETHTOOL_FEC_AUTO and ETHTOOL_FEC_OFF simultaneously.
> 
> From reading your previous replies, you would prefer to just have the
> driver set the "No FEC" bit always for ETHTOOL_FEC_AUTO when its
> available/supported by firmware?

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jacob Keller]

On 8/25/2022 6:01 PM, Jakub Kicinski wrote:
> On Thu, 25 Aug 2022 17:38:14 -0700 Jacob Keller wrote:
>> 2b) on newer firmware, the set PHY capabilities interface does have a
>> bit to request No FEC. In this case, if we set the No FEC bit, then the
>> firmware will be able to select No FEC as an option for cables that
>> otherwise wouldn't have selected it in the old firmware (such as CA-L
>> cables mentioned above).
> 
> Oh, but per the IEEE standard No FEC is _not_ an option for CA-L.
> From the initial reading of your series I thought that Intel NICs 
> would _never_ pick No FEC.
> 

That was my original interpretation when I was first introduced to this
problem but I was mistaken, hence why the commit message wasn't clear :(

This is rather more complicated than I originally understood and the
names for various bits have not been named very well so their behavior
isn't exactly obvious...


> Sounds like we need a bit for "ignore the standard and try everything".
> 
> What about BASE-R FEC? Is the FW going to try it on the CA-L cable?
> 

Ok I got further clarification on this. We have a bit, "Auto FEC
enable", as well as a bitmask for which FEC modes to try.

If "Auto FEC En" is set, then the Link Establishment State Machine will
try all of the FEC options we list in that bitmask, as long as we can
theoretically support them even if they aren't spec compliant.

For old firmware the bitmask didn't include a bit for "No FEC", where as
the new firmware has a bit for "No FEC".

We were always setting "Auto FEC En" so currently we try all FEC modes
we could theoretically support.

If "Auto FEC En" is disabled, then we only try FEC modes which are spec
compliant. Additionally, only a single FEC mode is tried based on a
priority and the bitmask.

Currently and historically the driver has always set "Auto FEC En", so
we were enabling non-spec compliant FEC modes, but "No FEC" was only
based on spec compliance with the media type.

From this, I think I agree the correct behavior is to add a bit for
"override the spec and try everything", and then on new firmware we'd
set the "No FEC" while on old firmware we'd be limited to only trying
FEC modes.

Does that make sense?

So yea I think we do probably need a "ignore the standard" bit.. but
currently that appears to already be what ice does (excepting No FEC
which didn't previously have a bit to set for it)

Thanks,
Jake

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jakub Kicinski]

On Fri, 26 Aug 2022 10:51:21 -0700 Jacob Keller wrote:
> On 8/25/2022 6:01 PM, Jakub Kicinski wrote:
> > Oh, but per the IEEE standard No FEC is _not_ an option for CA-L.
> > From the initial reading of your series I thought that Intel NICs 
> > would _never_ pick No FEC.
> 
> That was my original interpretation when I was first introduced to this
> problem but I was mistaken, hence why the commit message wasn't clear :(
> 
> This is rather more complicated than I originally understood and the
> names for various bits have not been named very well so their behavior
> isn't exactly obvious...
> 
> > Sounds like we need a bit for "ignore the standard and try everything".
> > 
> > What about BASE-R FEC? Is the FW going to try it on the CA-L cable?
> 
> Ok I got further clarification on this. We have a bit, "Auto FEC
> enable", as well as a bitmask for which FEC modes to try.
> 
> If "Auto FEC En" is set, then the Link Establishment State Machine will
> try all of the FEC options we list in that bitmask, as long as we can
> theoretically support them even if they aren't spec compliant.
> 
> For old firmware the bitmask didn't include a bit for "No FEC", where as
> the new firmware has a bit for "No FEC".
> 
> We were always setting "Auto FEC En" so currently we try all FEC modes
> we could theoretically support.
> 
> If "Auto FEC En" is disabled, then we only try FEC modes which are spec
> compliant. Additionally, only a single FEC mode is tried based on a
> priority and the bitmask.
> 
> Currently and historically the driver has always set "Auto FEC En", so
> we were enabling non-spec compliant FEC modes, but "No FEC" was only
> based on spec compliance with the media type.
> 
> From this, I think I agree the correct behavior is to add a bit for
> "override the spec and try everything", and then on new firmware we'd
> set the "No FEC" while on old firmware we'd be limited to only trying
> FEC modes.
> 
> Does that make sense?
> 
> So yea I think we do probably need a "ignore the standard" bit.. but
> currently that appears to already be what ice does (excepting No FEC
> which didn't previously have a bit to set for it)

Thanks for getting to the bottom of this :)

The "override spec modes" bit sounds like a reasonable addition,
since we possibly have different behavior between vendors letting 
the user know if the device will follow the rules can save someone
debugging time.

But it does sound orthogonal to you adding the No FEC bit to the mask
for ice.

Let me add Simon and Andy so that we have the major vendors on the CC.
(tl;dr the question is whether your FW follows the guidance of 
'Table 110C–1—Host and cable assembly combinations' in AUTO FEC mode).

If all the vendors already ignore the standard (like Intel and AFAIU
nVidia) then we just need to document, no point adding the bit...

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Gal Pressman]

On 27/08/2022 02:57, Jakub Kicinski wrote:
> On Fri, 26 Aug 2022 10:51:21 -0700 Jacob Keller wrote:
>> On 8/25/2022 6:01 PM, Jakub Kicinski wrote:
>>> Oh, but per the IEEE standard No FEC is _not_ an option for CA-L.
>>> From the initial reading of your series I thought that Intel NICs 
>>> would _never_ pick No FEC.
>> That was my original interpretation when I was first introduced to this
>> problem but I was mistaken, hence why the commit message wasn't clear :(
>>
>> This is rather more complicated than I originally understood and the
>> names for various bits have not been named very well so their behavior
>> isn't exactly obvious...
>>
>>> Sounds like we need a bit for "ignore the standard and try everything".
>>>
>>> What about BASE-R FEC? Is the FW going to try it on the CA-L cable?
>> Ok I got further clarification on this. We have a bit, "Auto FEC
>> enable", as well as a bitmask for which FEC modes to try.
>>
>> If "Auto FEC En" is set, then the Link Establishment State Machine will
>> try all of the FEC options we list in that bitmask, as long as we can
>> theoretically support them even if they aren't spec compliant.
>>
>> For old firmware the bitmask didn't include a bit for "No FEC", where as
>> the new firmware has a bit for "No FEC".
>>
>> We were always setting "Auto FEC En" so currently we try all FEC modes
>> we could theoretically support.
>>
>> If "Auto FEC En" is disabled, then we only try FEC modes which are spec
>> compliant. Additionally, only a single FEC mode is tried based on a
>> priority and the bitmask.
>>
>> Currently and historically the driver has always set "Auto FEC En", so
>> we were enabling non-spec compliant FEC modes, but "No FEC" was only
>> based on spec compliance with the media type.
>>
>> From this, I think I agree the correct behavior is to add a bit for
>> "override the spec and try everything", and then on new firmware we'd
>> set the "No FEC" while on old firmware we'd be limited to only trying
>> FEC modes.
>>
>> Does that make sense?
>>
>> So yea I think we do probably need a "ignore the standard" bit.. but
>> currently that appears to already be what ice does (excepting No FEC
>> which didn't previously have a bit to set for it)
> Thanks for getting to the bottom of this :)
>
> The "override spec modes" bit sounds like a reasonable addition,
> since we possibly have different behavior between vendors letting 
> the user know if the device will follow the rules can save someone
> debugging time.
>
> But it does sound orthogonal to you adding the No FEC bit to the mask
> for ice.
>
> Let me add Simon and Andy so that we have the major vendors on the CC.
> (tl;dr the question is whether your FW follows the guidance of 
> 'Table 110C–1—Host and cable assembly combinations' in AUTO FEC mode).
>
> If all the vendors already ignore the standard (like Intel and AFAIU
> nVidia) then we just need to document, no point adding the bit...

I think we misunderstood each other :).
Our implementation definitely *does not* ignore the standard.
When autoneg is disabled, and auto fec is enabled, the firmware chooses
a fec mode according to the spec. If "no FEC" is not in the spec, we
will not pick it (nor do we want to).
It sounds like you're not happy with the spec, then why not change it?
This doesn't sound like an area where we want to be non-compliant.

Regardless, changing our interface because of one device' firmware
bug/behavior change doesn't make any sense. This interface affects all
consumers, and is going to stick with us forever. Firmware will
eventually get updated and it only affects one driver.

RE: [PATCH net-next 0/2] ice: support FEC automatic disable

[Keller, Jacob E]

> -----Original Message-----
> From: Gal Pressman <gal@nvidia.com>
> Sent: Sunday, August 28, 2022 3:43 AM
> To: Jakub Kicinski <kuba@kernel.org>; Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Saeed Mahameed <saeedm@nvidia.com>; netdev@vger.kernel.org; Simon
> Horman <horms@verge.net.au>; Andy Gospodarek <andy@greyhouse.net>
> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
> 
> On 27/08/2022 02:57, Jakub Kicinski wrote:
> > On Fri, 26 Aug 2022 10:51:21 -0700 Jacob Keller wrote:
> >> On 8/25/2022 6:01 PM, Jakub Kicinski wrote:
> >>> Oh, but per the IEEE standard No FEC is _not_ an option for CA-L.
> >>> From the initial reading of your series I thought that Intel NICs
> >>> would _never_ pick No FEC.
> >> That was my original interpretation when I was first introduced to this
> >> problem but I was mistaken, hence why the commit message wasn't clear :(
> >>
> >> This is rather more complicated than I originally understood and the
> >> names for various bits have not been named very well so their behavior
> >> isn't exactly obvious...
> >>
> >>> Sounds like we need a bit for "ignore the standard and try everything".
> >>>
> >>> What about BASE-R FEC? Is the FW going to try it on the CA-L cable?
> >> Ok I got further clarification on this. We have a bit, "Auto FEC
> >> enable", as well as a bitmask for which FEC modes to try.
> >>
> >> If "Auto FEC En" is set, then the Link Establishment State Machine will
> >> try all of the FEC options we list in that bitmask, as long as we can
> >> theoretically support them even if they aren't spec compliant.
> >>
> >> For old firmware the bitmask didn't include a bit for "No FEC", where as
> >> the new firmware has a bit for "No FEC".
> >>
> >> We were always setting "Auto FEC En" so currently we try all FEC modes
> >> we could theoretically support.
> >>
> >> If "Auto FEC En" is disabled, then we only try FEC modes which are spec
> >> compliant. Additionally, only a single FEC mode is tried based on a
> >> priority and the bitmask.
> >>
> >> Currently and historically the driver has always set "Auto FEC En", so
> >> we were enabling non-spec compliant FEC modes, but "No FEC" was only
> >> based on spec compliance with the media type.
> >>
> >> From this, I think I agree the correct behavior is to add a bit for
> >> "override the spec and try everything", and then on new firmware we'd
> >> set the "No FEC" while on old firmware we'd be limited to only trying
> >> FEC modes.
> >>
> >> Does that make sense?
> >>
> >> So yea I think we do probably need a "ignore the standard" bit.. but
> >> currently that appears to already be what ice does (excepting No FEC
> >> which didn't previously have a bit to set for it)
> > Thanks for getting to the bottom of this :)
> >
> > The "override spec modes" bit sounds like a reasonable addition,
> > since we possibly have different behavior between vendors letting
> > the user know if the device will follow the rules can save someone
> > debugging time.
> >
> > But it does sound orthogonal to you adding the No FEC bit to the mask
> > for ice.
> >
> > Let me add Simon and Andy so that we have the major vendors on the CC.
> > (tl;dr the question is whether your FW follows the guidance of
> > 'Table 110C–1—Host and cable assembly combinations' in AUTO FEC mode).
> >

The other engineers I spoke to also wanted to mention that 110C-1 is only a small subset of all of the various link types. They also mentioned something about an SFF standard which describes many more types.

> > If all the vendors already ignore the standard (like Intel and AFAIU
> > nVidia) then we just need to document, no point adding the bit...
> 
> I think we misunderstood each other :).
> Our implementation definitely *does not* ignore the standard.
> When autoneg is disabled, and auto fec is enabled, the firmware chooses
> a fec mode according to the spec. If "no FEC" is not in the spec, we
> will not pick it (nor do we want to).
> It sounds like you're not happy with the spec, then why not change it?
> This doesn't sound like an area where we want to be non-compliant.
> 
> Regardless, changing our interface because of one device' firmware
> bug/behavior change doesn't make any sense. This interface affects all
> consumers, and is going to stick with us forever. Firmware will
> eventually get updated and it only affects one driver.

Well, the current ice behavior for every FEC mode *except* No FEC, we try modes which may be supportable even though they're outside the spec. As far as I understand, the reason we attempt these is because it allows linking in more scenarios, presumably because some combination of things is not fully spec compliant? I don't really know for sure.

For future firmware, this would include No FEC as well. Thus, even with future firmware we'd still be trying some modes outside of the spec. I can try to get some more answers tomorrow about the reasoning and justification for this behavior.

Thanks,
Jake

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Gal Pressman]

On 29/08/2022 10:11, Keller, Jacob E wrote:
>
>> -----Original Message-----
>> From: Gal Pressman <gal@nvidia.com>
>> Sent: Sunday, August 28, 2022 3:43 AM
>> To: Jakub Kicinski <kuba@kernel.org>; Keller, Jacob E <jacob.e.keller@intel.com>
>> Cc: Saeed Mahameed <saeedm@nvidia.com>; netdev@vger.kernel.org; Simon
>> Horman <horms@verge.net.au>; Andy Gospodarek <andy@greyhouse.net>
>> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
>>
>> On 27/08/2022 02:57, Jakub Kicinski wrote:
>>> On Fri, 26 Aug 2022 10:51:21 -0700 Jacob Keller wrote:
>>>> On 8/25/2022 6:01 PM, Jakub Kicinski wrote:
>>>>> Oh, but per the IEEE standard No FEC is _not_ an option for CA-L.
>>>>> From the initial reading of your series I thought that Intel NICs
>>>>> would _never_ pick No FEC.
>>>> That was my original interpretation when I was first introduced to this
>>>> problem but I was mistaken, hence why the commit message wasn't clear :(
>>>>
>>>> This is rather more complicated than I originally understood and the
>>>> names for various bits have not been named very well so their behavior
>>>> isn't exactly obvious...
>>>>
>>>>> Sounds like we need a bit for "ignore the standard and try everything".
>>>>>
>>>>> What about BASE-R FEC? Is the FW going to try it on the CA-L cable?
>>>> Ok I got further clarification on this. We have a bit, "Auto FEC
>>>> enable", as well as a bitmask for which FEC modes to try.
>>>>
>>>> If "Auto FEC En" is set, then the Link Establishment State Machine will
>>>> try all of the FEC options we list in that bitmask, as long as we can
>>>> theoretically support them even if they aren't spec compliant.
>>>>
>>>> For old firmware the bitmask didn't include a bit for "No FEC", where as
>>>> the new firmware has a bit for "No FEC".
>>>>
>>>> We were always setting "Auto FEC En" so currently we try all FEC modes
>>>> we could theoretically support.
>>>>
>>>> If "Auto FEC En" is disabled, then we only try FEC modes which are spec
>>>> compliant. Additionally, only a single FEC mode is tried based on a
>>>> priority and the bitmask.
>>>>
>>>> Currently and historically the driver has always set "Auto FEC En", so
>>>> we were enabling non-spec compliant FEC modes, but "No FEC" was only
>>>> based on spec compliance with the media type.
>>>>
>>>> From this, I think I agree the correct behavior is to add a bit for
>>>> "override the spec and try everything", and then on new firmware we'd
>>>> set the "No FEC" while on old firmware we'd be limited to only trying
>>>> FEC modes.
>>>>
>>>> Does that make sense?
>>>>
>>>> So yea I think we do probably need a "ignore the standard" bit.. but
>>>> currently that appears to already be what ice does (excepting No FEC
>>>> which didn't previously have a bit to set for it)
>>> Thanks for getting to the bottom of this :)
>>>
>>> The "override spec modes" bit sounds like a reasonable addition,
>>> since we possibly have different behavior between vendors letting
>>> the user know if the device will follow the rules can save someone
>>> debugging time.
>>>
>>> But it does sound orthogonal to you adding the No FEC bit to the mask
>>> for ice.
>>>
>>> Let me add Simon and Andy so that we have the major vendors on the CC.
>>> (tl;dr the question is whether your FW follows the guidance of
>>> 'Table 110C–1—Host and cable assembly combinations' in AUTO FEC mode).
>>>
> The other engineers I spoke to also wanted to mention that 110C-1 is only a small subset of all of the various link types. They also mentioned something about an SFF standard which describes many more types.

The firmware folks here claim there's also a difference between TX and RX.

>
>>> If all the vendors already ignore the standard (like Intel and AFAIU
>>> nVidia) then we just need to document, no point adding the bit...
>> I think we misunderstood each other :).
>> Our implementation definitely *does not* ignore the standard.
>> When autoneg is disabled, and auto fec is enabled, the firmware chooses
>> a fec mode according to the spec. If "no FEC" is not in the spec, we
>> will not pick it (nor do we want to).
>> It sounds like you're not happy with the spec, then why not change it?
>> This doesn't sound like an area where we want to be non-compliant.
>>
>> Regardless, changing our interface because of one device' firmware
>> bug/behavior change doesn't make any sense. This interface affects all
>> consumers, and is going to stick with us forever. Firmware will
>> eventually get updated and it only affects one driver.
> Well, the current ice behavior for every FEC mode *except* No FEC, we try modes which may be supportable even though they're outside the spec. As far as I understand, the reason we attempt these is because it allows linking in more scenarios, presumably because some combination of things is not fully spec compliant? I don't really know for sure.
>
> For future firmware, this would include No FEC as well. Thus, even with future firmware we'd still be trying some modes outside of the spec. I can try to get some more answers tomorrow about the reasoning and justification for this behavior.
>

Yea, understood, but respectfully, I don't understand why we should go
along with your requirement to support this non-spec behavior.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jacob Keller]

On 8/29/2022 4:21 AM, Gal Pressman wrote:
> On 29/08/2022 10:11, Keller, Jacob E wrote:
>>> Regardless, changing our interface because of one device' firmware
>>> bug/behavior change doesn't make any sense. This interface affects all
>>> consumers, and is going to stick with us forever. Firmware will
>>> eventually get updated and it only affects one driver.
>> Well, the current ice behavior for every FEC mode *except* No FEC, we try modes which may be supportable even though they're outside the spec. As far as I understand, the reason we attempt these is because it allows linking in more scenarios, presumably because some combination of things is not fully spec compliant? I don't really know for sure.
>>
>> For future firmware, this would include No FEC as well. Thus, even with future firmware we'd still be trying some modes outside of the spec. I can try to get some more answers tomorrow about the reasoning and justification for this behavior.
>>
> 
> Yea, understood, but respectfully, I don't understand why we should go
> along with your requirement to support this non-spec behavior.

My understanding is that this is requested by customers for a few reasons:

1) interopability with legacy switches

2) interopability with modules which don't follow spec

3) low latency applications for which disabling FEC can improve latency
if the module is able to achieve a low enough error rate.

We have a fair number of customer requests to support these
non-compliant modules and modes, including both enabling certain FEC
modes or disabling FEC.

We already have this enabled with existing drivers. Of course, part of
that was caused by confusion due to poor naming scheme and lack of clear
communication to us about what the real behavior was. (Thanks Kuba for
pushing on that...) It probably comes across as a bit disingenuous
because we've implemented and enabled this support without being clear
about the behavior.

I haven't 100% confirmed, but I would be surprised if this only affects
ice. Its likely something that behaves similarly for other Intel products.

The ability to go outside the spec enables some of our customers and
solves real problems. The reality is that we don't always have perfect
hardware, and we want to inter-operate with the existing hardware. Some
switches were designed and built while the standards were still being
developed, and they don't 100% follow the spec because of this.

By extending the interface it becomes clear and obvious that we're going
outside the spec. If this hadn't been brought up this would have more or
less hidden behind a binary firmware blob with almost no way to notice
it, and no way to communicate that is whats happening.

I'm frustrated by the poor communication here because it was not at all
obvious to me until the last week that this is what we were doing.
However, I do see value in supporting the existing hardware available
even when its not quite spec compliant.

Thanks,
Jake

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jacob Keller]

On 8/29/2022 11:10 AM, Jacob Keller wrote:
> 
> 
> On 8/29/2022 4:21 AM, Gal Pressman wrote:
>> On 29/08/2022 10:11, Keller, Jacob E wrote:
>>>> Regardless, changing our interface because of one device' firmware
>>>> bug/behavior change doesn't make any sense. This interface affects all
>>>> consumers, and is going to stick with us forever. Firmware will
>>>> eventually get updated and it only affects one driver.
>>> Well, the current ice behavior for every FEC mode *except* No FEC, we try modes which may be supportable even though they're outside the spec. As far as I understand, the reason we attempt these is because it allows linking in more scenarios, presumably because some combination of things is not fully spec compliant? I don't really know for sure.
>>>
>>> For future firmware, this would include No FEC as well. Thus, even with future firmware we'd still be trying some modes outside of the spec. I can try to get some more answers tomorrow about the reasoning and justification for this behavior.
>>>
>>
>> Yea, understood, but respectfully, I don't understand why we should go
>> along with your requirement to support this non-spec behavior.
> 
> My understanding is that this is requested by customers for a few reasons:
> 
> 1) interopability with legacy switches
> 
> 2) interopability with modules which don't follow spec
> 
> 3) low latency applications for which disabling FEC can improve latency
> if the module is able to achieve a low enough error rate.
> 
> We have a fair number of customer requests to support these
> non-compliant modules and modes, including both enabling certain FEC
> modes or disabling FEC.
> 
> We already have this enabled with existing drivers. Of course, part of
> that was caused by confusion due to poor naming scheme and lack of clear
> communication to us about what the real behavior was. (Thanks Kuba for
> pushing on that...) It probably comes across as a bit disingenuous
> because we've implemented and enabled this support without being clear
> about the behavior.
> 
> I haven't 100% confirmed, but I would be surprised if this only affects
> ice. Its likely something that behaves similarly for other Intel products.
> 
> The ability to go outside the spec enables some of our customers and
> solves real problems. The reality is that we don't always have perfect
> hardware, and we want to inter-operate with the existing hardware. Some
> switches were designed and built while the standards were still being
> developed, and they don't 100% follow the spec because of this.
> 
> By extending the interface it becomes clear and obvious that we're going
> outside the spec. If this hadn't been brought up this would have more or
> less hidden behind a binary firmware blob with almost no way to notice
> it, and no way to communicate that is whats happening.
> 
> I'm frustrated by the poor communication here because it was not at all
> obvious to me until the last week that this is what we were doing.
> However, I do see value in supporting the existing hardware available
> even when its not quite spec compliant.
> 
> Thanks,
> Jake

I'm trying to figure out what my next steps are here.

Jakub, from earlier discussion it sounded like you are ok with accepting
patch to include "No FEC" into our auto override behavior, with no uAPI
changes. Is that still ok given the recent dicussion regarding going
beyond the spec?

I'm also happy to rename the flag in ice so that its not misnamed and
clearly indicates its behavior.

Gal seems against extending uAPI to indicate or support "ignore spec".
To be properly correct that would mean changing ice to stop setting the
AUTO_FEC flag. As explained above, I believe this will lead to breakage
in situations where we used to link and function properly.

I have no way to verify whether other vendors actually follow this or
not, as it essentially requires checking with modules that wouldn't link
otherwise and likely requires a lot of trial and error.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jakub Kicinski]

On Tue, 30 Aug 2022 13:09:20 -0700 Jacob Keller wrote:
> I'm trying to figure out what my next steps are here.
> 
> Jakub, from earlier discussion it sounded like you are ok with accepting
> patch to include "No FEC" into our auto override behavior, with no uAPI
> changes. Is that still ok given the recent dicussion regarding going
> beyond the spec?

Yes, I reserve the right to change my mind :) but AFAIU it doesn't make
things worse, so fine by me.

> I'm also happy to rename the flag in ice so that its not misnamed and
> clearly indicates its behavior.

Which flag? A new ethtool priv flag?

> Gal seems against extending uAPI to indicate or support "ignore spec".
> To be properly correct that would mean changing ice to stop setting the
> AUTO_FEC flag. As explained above, I believe this will lead to breakage
> in situations where we used to link and function properly.

Stop setting the AUTO_FEC flag or start using a new standard compliant
AUTO flag?

Gal, within the spec do you iterate over modes or pick one mode somehow
(the spec gives a set, AFAICT)?

> I have no way to verify whether other vendors actually follow this or
> not, as it essentially requires checking with modules that wouldn't link
> otherwise and likely requires a lot of trial and error.

Getting some input from Broadcom or Netronome would be useful, yes :(

RE: [PATCH net-next 0/2] ice: support FEC automatic disable

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, August 30, 2022 2:45 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Gal Pressman <gal@nvidia.com>; Saeed Mahameed <saeedm@nvidia.com>;
> netdev@vger.kernel.org; Simon Horman <horms@verge.net.au>; Andy
> Gospodarek <andy@greyhouse.net>
> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
> 
> On Tue, 30 Aug 2022 13:09:20 -0700 Jacob Keller wrote:
> > I'm trying to figure out what my next steps are here.
> >
> > Jakub, from earlier discussion it sounded like you are ok with accepting
> > patch to include "No FEC" into our auto override behavior, with no uAPI
> > changes. Is that still ok given the recent dicussion regarding going
> > beyond the spec?
> 
> Yes, I reserve the right to change my mind :) but AFAIU it doesn't make
> things worse, so fine by me.
>

Ok.
 
> > I'm also happy to rename the flag in ice so that its not misnamed and
> > clearly indicates its behavior.
> 
> Which flag? A new ethtool priv flag?
> 

No the flag I am referring to here is for the bit we pass to firmware from the driver. This is confusingly named "EN_AUTO_FEC" but it really means something like "override spec and try all FEC modes".

> > Gal seems against extending uAPI to indicate or support "ignore spec".
> > To be properly correct that would mean changing ice to stop setting the
> > AUTO_FEC flag. As explained above, I believe this will lead to breakage
> > in situations where we used to link and function properly.
> 
> Stop setting the AUTO_FEC flag or start using a new standard compliant
> AUTO flag?

There is only "EN_AUTO_FEC" which means both "try multiple FEC modes, including ones outside the spec". If we disable this flag, then I believe it will only try the highest priority FEC mode for each link type. This is the flag which I think is poorly named and led me to misunderstand the whole behavior.

> 
> Gal, within the spec do you iterate over modes or pick one mode somehow
> (the spec gives a set, AFAICT)?
> 
> > I have no way to verify whether other vendors actually follow this or
> > not, as it essentially requires checking with modules that wouldn't link
> > otherwise and likely requires a lot of trial and error.
> 
> Getting some input from Broadcom or Netronome would be useful, yes :(

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Gal Pressman]

On 31/08/2022 00:44, Jakub Kicinski wrote:
> On Tue, 30 Aug 2022 13:09:20 -0700 Jacob Keller wrote:
>> Gal seems against extending uAPI to indicate or support "ignore spec".
>> To be properly correct that would mean changing ice to stop setting the
>> AUTO_FEC flag. As explained above, I believe this will lead to breakage
>> in situations where we used to link and function properly.
> Stop setting the AUTO_FEC flag or start using a new standard compliant
> AUTO flag?
>
> Gal, within the spec do you iterate over modes or pick one mode somehow
> (the spec gives a set, AFAICT)?

When autoneg is enabled (and auto fec enabled), auto negotiation is done
from the link modes according to spec.
When autoneg is disabled (and auto fec enabled), the firmware chooses
one of the supported modes according to the spec. As far as I
understand, it doesn't try anything, just picks a supported mode.

This whole thing revolves around customers who don't use auto
negotiation, but it sounds like ice is still trying to do auto
negotiation for fec under the hood.
Looking at the reasons Jacob listed here (unspec switches/modules), it
seems like all this can be resolved by simply setting the fec mode
explicitly, and to me it sounds like a reasonable solution for these
special cases.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jakub Kicinski]

On Wed, 31 Aug 2022 14:01:10 +0300 Gal Pressman wrote:
> When autoneg is disabled (and auto fec enabled), the firmware chooses
> one of the supported modes according to the spec. 

Would you be able to provide the pointer in the spec / section which
defines the behavior?  It may be helpful to add that to the kdoc for 
ETHTOOL_FEC_AUTO_BIT.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Jacob Keller]

On 8/31/2022 4:01 AM, Gal Pressman wrote:
> On 31/08/2022 00:44, Jakub Kicinski wrote:
>> On Tue, 30 Aug 2022 13:09:20 -0700 Jacob Keller wrote:
>>> Gal seems against extending uAPI to indicate or support "ignore spec".
>>> To be properly correct that would mean changing ice to stop setting the
>>> AUTO_FEC flag. As explained above, I believe this will lead to breakage
>>> in situations where we used to link and function properly.
>> Stop setting the AUTO_FEC flag or start using a new standard compliant
>> AUTO flag?
>>
>> Gal, within the spec do you iterate over modes or pick one mode somehow
>> (the spec gives a set, AFAICT)?
>> When autoneg is enabled (and auto fec enabled), auto negotiation is done
> from the link modes according to spec.


This is the same for ice, except that the ETHTOOL_FEC_* configuration is
 ignored. The documentation indicates its only intended for when
autonegotiation is disabled.

> When autoneg is disabled (and auto fec enabled), the firmware chooses
> one of the supported modes according to the spec. As far as I
> understand, it doesn't try anything, just picks a supported mode.
> 

This is how ice works if we don't set the ICE_AQC_PHY_EN_FEC_AUTO flag
when configuring our firmware.

> This whole thing revolves around customers who don't use auto
> negotiation, but it sounds like ice is still trying to do auto
> negotiation for fec under the hood.

It's not really auto negotiation as it is more like auto-retry, its a
simple state machine that iterates through a series of possible
configurations. The goal being to reduce cognitive burden on users and
just try to establish link.

I believe that sigh ICE_AQC_PHY_EN_FEC_AUTO set, we still try different
media type settings but only one FEC mode. The exact way it picks such a
FEC mode is unclear to me in this case. With ICE_AQC_PHY_EN_FEC_AUTO,
each media type is re-tried with each FEC mode that is theoretically
possible even if its not in spec.

> Looking at the reasons Jacob listed here (unspec switches/modules), it
> seems like all this can be resolved by simply setting the fec mode
> explicitly, and to me it sounds like a reasonable solution for these
> special cases.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Gal Pressman]

On 31/08/2022 23:15, Jacob Keller wrote:
> On 8/31/2022 4:01 AM, Gal Pressman wrote:
>> When autoneg is disabled (and auto fec enabled), the firmware chooses
>> one of the supported modes according to the spec. As far as I
>> understand, it doesn't try anything, just picks a supported mode.
>>
> This is how ice works if we don't set the ICE_AQC_PHY_EN_FEC_AUTO flag
> when configuring our firmware.

If auto fec is off, whatever mode the user chose explicitly should be used.
What I'm referring to is when auto fec is on, then our firmware picks
one of the spec modes it sees fit (according to spec).

>> This whole thing revolves around customers who don't use auto
>> negotiation, but it sounds like ice is still trying to do auto
>> negotiation for fec under the hood.
> It's not really auto negotiation as it is more like auto-retry, its a
> simple state machine that iterates through a series of possible
> configurations. The goal being to reduce cognitive burden on users and
> just try to establish link.

Cognitive burden can be reduced by using auto negotiation?

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Gal Pressman]

On 31/08/2022 20:36, Jakub Kicinski wrote:
> On Wed, 31 Aug 2022 14:01:10 +0300 Gal Pressman wrote:
>> When autoneg is disabled (and auto fec enabled), the firmware chooses
>> one of the supported modes according to the spec. 
> Would you be able to provide the pointer in the spec / section which
> defines the behavior?  It may be helpful to add that to the kdoc for 
> ETHTOOL_FEC_AUTO_BIT.

Asked our architect for a pointer, hope I'll get it soon (he's OOO,
might take a few days).

RE: [PATCH net-next 0/2] ice: support FEC automatic disable

[Keller, Jacob E]

> -----Original Message-----
> From: Gal Pressman <gal@nvidia.com>
> Sent: Thursday, September 01, 2022 4:52 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>
> Cc: Saeed Mahameed <saeedm@nvidia.com>; netdev@vger.kernel.org; Simon
> Horman <horms@verge.net.au>; Andy Gospodarek <andy@greyhouse.net>
> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
> 
> On 31/08/2022 23:15, Jacob Keller wrote:
> > On 8/31/2022 4:01 AM, Gal Pressman wrote:
> >> When autoneg is disabled (and auto fec enabled), the firmware chooses
> >> one of the supported modes according to the spec. As far as I
> >> understand, it doesn't try anything, just picks a supported mode.
> >>
> > This is how ice works if we don't set the ICE_AQC_PHY_EN_FEC_AUTO flag
> > when configuring our firmware.
> 
> If auto fec is off, whatever mode the user chose explicitly should be used.
> What I'm referring to is when auto fec is on, then our firmware picks
> one of the spec modes it sees fit (according to spec).

Correct. I'm referring to a firwamre bit we set, not the ETHTOOL_FEC_AUTO here. Sorry for the confusion, the names are similar but not strictly related. That is definitely a point of confusion here :(

Currently, when ETHTOOL_FEC_AUTO is  set, we always set ICE_AQC_PHY_EN_FEC_AUTO, but if we opted not to do that, then we would have similar behavior to what you describe.

We would choose a single FEC mode to try for the various media types that get tried. We still perform the link state machine, but with fewer FEC options and none of them outside spec. We don't currently have a driver implemented this way.

> 
> >> This whole thing revolves around customers who don't use auto
> >> negotiation, but it sounds like ice is still trying to do auto
> >> negotiation for fec under the hood.
> > It's not really auto negotiation as it is more like auto-retry, its a
> > simple state machine that iterates through a series of possible
> > configurations. The goal being to reduce cognitive burden on users and
> > just try to establish link.
> 
> Cognitive burden can be reduced by using auto negotiation?

I'm not sure what situations result in autonegotiation vs when we have it disabled. For example, if autonegotiation fails we then the link state machine falls back to the series of forced configurations it tries.

The auto selection makes the device attempt to get link, and by enabling more modes we are more likely to achieve link. By having this happen without need for significant user interaction means that users simply see the device link up and function without the need to make a choice.

Thanks,
Jake

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Gal Pressman]

On 31/08/2022 20:36, Jakub Kicinski wrote:
> On Wed, 31 Aug 2022 14:01:10 +0300 Gal Pressman wrote:
>> When autoneg is disabled (and auto fec enabled), the firmware chooses
>> one of the supported modes according to the spec. 
> Would you be able to provide the pointer in the spec / section which
> defines the behavior?  It may be helpful to add that to the kdoc for 
> ETHTOOL_FEC_AUTO_BIT.

I have been directed to:
https://www.snia.org/technology-communities/sff/specifications?field_doc_status_value=All&combine=8024&field_release_date_value_2%5Bvalue%5D%5Bdate%5D=&field_release_date_value%5Bvalue%5D%5Bdate%5D=&items_per_page=20

SFF-8024 SFF Module Management Reference Code Tables -> Table 4-4
Extended Specification Compliance Codes.

Re: [PATCH net-next 0/2] ice: support FEC automatic disable

[Michael Chan]

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

On Tue, Aug 30, 2022 at 2:45 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 30 Aug 2022 13:09:20 -0700 Jacob Keller wrote:
> > I have no way to verify whether other vendors actually follow this or
> > not, as it essentially requires checking with modules that wouldn't link
> > otherwise and likely requires a lot of trial and error.
>
> Getting some input from Broadcom or Netronome would be useful, yes :(

Sorry for the late comment.  I needed to consult with the firmware
engineer first.  The bnxt driver does not support auto FEC if
auto-negotiation is disabled.  But firmware has a "media detect"
fallback mode if auto-negotiation fails to link up.  It is similar to
the parallel detect mode but done in firmware.  I think this fallback
mode may be relevant to the discussion here.  In this fallback mode,
firmware will cycle through all the supported speeds and supported FEC
combinations (including no FEC) and try to link up.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]