[PATCH net-next 0/3] nfp: support FEC mode reporting and auto-neg

[PATCH net-next 0/3] nfp: support FEC mode reporting and auto-neg

[Simon Horman]

Hi,

this series adds support for the following features to the nfp driver:

* Patch 1/3: Support active FEC mode
* Patch 2/3: Support link auto negotiation
* Patch 3/3: Support restart of link auto negotiation

Fei Qin (1):
  nfp: add support restart of link auto-negotiation

Yinjun Zhang (2):
  nfp: add support for reporting active FEC mode
  nfp: add support for link auto negotiation

 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 78 +++++++++++++++++--
 .../net/ethernet/netronome/nfp/nfp_net_main.c | 23 +++++-
 .../ethernet/netronome/nfp/nfpcore/nfp_nsp.h  |  4 +
 .../netronome/nfp/nfpcore/nfp_nsp_eth.c       | 13 +++-
 4 files changed, 107 insertions(+), 11 deletions(-)

-- 
2.30.2

[PATCH net-next 1/3] nfp: add support for reporting active FEC mode

[Simon Horman]

From: Yinjun Zhang <yinjun.zhang@corigine.com>

The latest management firmware can now report the active FEC
mode. Adapt driver accordingly so that user can get the active
FEC mode by running command:

  # ethtool --show-fec <intf>

Also correct use of `fec` field.

Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Reviewed-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c     | 2 +-
 drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h     | 2 ++
 drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c | 9 ++++++++-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index db58532364b6..d50af23642a2 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -996,7 +996,7 @@ nfp_port_get_fecparam(struct net_device *netdev,
 		return 0;
 
 	param->fec = nfp_port_fec_nsp_to_ethtool(eth_port->fec_modes_supported);
-	param->active_fec = nfp_port_fec_nsp_to_ethtool(eth_port->fec);
+	param->active_fec = nfp_port_fec_nsp_to_ethtool(BIT(eth_port->act_fec));
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
index 77d66855be42..52465670a01e 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
@@ -132,6 +132,7 @@ enum nfp_eth_fec {
  * @ports.interface:	interface (module) plugged in
  * @ports.media:	media type of the @interface
  * @ports.fec:		forward error correction mode
+ * @ports.act_fec:	active forward error correction mode
  * @ports.aneg:		auto negotiation mode
  * @ports.mac_addr:	interface MAC address
  * @ports.label_port:	port id
@@ -162,6 +163,7 @@ struct nfp_eth_table {
 		enum nfp_eth_media media;
 
 		enum nfp_eth_fec fec;
+		enum nfp_eth_fec act_fec;
 		enum nfp_eth_aneg aneg;
 
 		u8 mac_addr[ETH_ALEN];
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
index 4cc38799eabc..18ba7629cdc2 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
@@ -40,6 +40,7 @@
 #define NSP_ETH_STATE_OVRD_CHNG		BIT_ULL(22)
 #define NSP_ETH_STATE_ANEG		GENMASK_ULL(25, 23)
 #define NSP_ETH_STATE_FEC		GENMASK_ULL(27, 26)
+#define NSP_ETH_STATE_ACT_FEC		GENMASK_ULL(29, 28)
 
 #define NSP_ETH_CTRL_CONFIGURED		BIT_ULL(0)
 #define NSP_ETH_CTRL_ENABLED		BIT_ULL(1)
@@ -170,7 +171,13 @@ nfp_eth_port_translate(struct nfp_nsp *nsp, const union eth_table_entry *src,
 	if (dst->fec_modes_supported)
 		dst->fec_modes_supported |= NFP_FEC_AUTO | NFP_FEC_DISABLED;
 
-	dst->fec = 1 << FIELD_GET(NSP_ETH_STATE_FEC, state);
+	dst->fec = FIELD_GET(NSP_ETH_STATE_FEC, state);
+	dst->act_fec = dst->fec;
+
+	if (nfp_nsp_get_abi_ver_minor(nsp) < 33)
+		return;
+
+	dst->act_fec = FIELD_GET(NSP_ETH_STATE_ACT_FEC, state);
 }
 
 static void
-- 
2.30.2

[PATCH net-next 2/3] nfp: add support for link auto negotiation

[Simon Horman]

From: Yinjun Zhang <yinjun.zhang@corigine.com>

Report the auto negotiation capability if it's supported
in management firmware, and advertise it if it's enabled.

Changing FEC to mode other than auto or changing port
speed is not allowed when autoneg is enabled. And FEC mode
is enforced into auto mode when enabling link autoneg.

The ethtool <intf> command displays the auto-neg capability:

  # ethtool enp1s0np0
  Settings for enp1s0np0:
          Supported ports: [ FIBRE ]
          Supported link modes:   Not reported
          Supported pause frame use: Symmetric
          Supports auto-negotiation: Yes
          Supported FEC modes: None        RS      BASER
          Advertised link modes:  Not reported
          Advertised pause frame use: Symmetric
          Advertised auto-negotiation: Yes
          Advertised FEC modes: None       RS      BASER
          Speed: 25000Mb/s
          Duplex: Full
          Auto-negotiation: on
          Port: FIBRE
          PHYAD: 0
          Transceiver: internal
          Link detected: yes

Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Reviewed-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 43 ++++++++++++++++---
 .../net/ethernet/netronome/nfp/nfp_net_main.c | 23 +++++++++-
 .../ethernet/netronome/nfp/nfpcore/nfp_nsp.h  |  2 +
 .../netronome/nfp/nfpcore/nfp_nsp_eth.c       |  4 +-
 4 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index d50af23642a2..00aacc48a7a2 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -290,8 +290,13 @@ nfp_net_get_link_ksettings(struct net_device *netdev,
 	if (eth_port) {
 		ethtool_link_ksettings_add_link_mode(cmd, supported, Pause);
 		ethtool_link_ksettings_add_link_mode(cmd, advertising, Pause);
-		cmd->base.autoneg = eth_port->aneg != NFP_ANEG_DISABLED ?
-			AUTONEG_ENABLE : AUTONEG_DISABLE;
+		if (eth_port->supp_aneg) {
+			ethtool_link_ksettings_add_link_mode(cmd, supported, Autoneg);
+			if (eth_port->aneg == NFP_ANEG_AUTO) {
+				ethtool_link_ksettings_add_link_mode(cmd, advertising, Autoneg);
+				cmd->base.autoneg = AUTONEG_ENABLE;
+			}
+		}
 		nfp_net_set_fec_link_mode(eth_port, cmd);
 	}
 
@@ -327,6 +332,7 @@ static int
 nfp_net_set_link_ksettings(struct net_device *netdev,
 			   const struct ethtool_link_ksettings *cmd)
 {
+	bool req_aneg = (cmd->base.autoneg == AUTONEG_ENABLE);
 	struct nfp_eth_table_port *eth_port;
 	struct nfp_port *port;
 	struct nfp_nsp *nsp;
@@ -346,16 +352,36 @@ nfp_net_set_link_ksettings(struct net_device *netdev,
 	if (IS_ERR(nsp))
 		return PTR_ERR(nsp);
 
-	err = __nfp_eth_set_aneg(nsp, cmd->base.autoneg == AUTONEG_ENABLE ?
-				 NFP_ANEG_AUTO : NFP_ANEG_DISABLED);
+	if (req_aneg && !eth_port->supp_aneg) {
+		netdev_warn(netdev, "Autoneg is not supported.\n");
+		err = -EOPNOTSUPP;
+		goto err_bad_set;
+	}
+
+	err = __nfp_eth_set_aneg(nsp, req_aneg ? NFP_ANEG_AUTO : NFP_ANEG_DISABLED);
 	if (err)
 		goto err_bad_set;
+
 	if (cmd->base.speed != SPEED_UNKNOWN) {
-		u32 speed = cmd->base.speed / eth_port->lanes;
+		if (req_aneg) {
+			netdev_err(netdev, "Speed changing is not allowed when working on autoneg mode.\n");
+			err = -EINVAL;
+			goto err_bad_set;
+		} else {
+			u32 speed = cmd->base.speed / eth_port->lanes;
 
-		err = __nfp_eth_set_speed(nsp, speed);
+			err = __nfp_eth_set_speed(nsp, speed);
+			if (err)
+				goto err_bad_set;
+		}
+	}
+
+	if (req_aneg && nfp_eth_can_support_fec(eth_port) && eth_port->fec != NFP_FEC_AUTO_BIT) {
+		err = __nfp_eth_set_fec(nsp, NFP_FEC_AUTO_BIT);
 		if (err)
 			goto err_bad_set;
+
+		netdev_info(netdev, "FEC is enforced into auto mode when autoneg is enabled.\n");
 	}
 
 	err = nfp_eth_config_commit_end(nsp);
@@ -1021,6 +1047,11 @@ nfp_port_set_fecparam(struct net_device *netdev,
 	if (fec < 0)
 		return fec;
 
+	if (eth_port->supp_aneg && eth_port->aneg == NFP_ANEG_AUTO && fec != NFP_FEC_AUTO_BIT) {
+		netdev_err(netdev, "Only auto mode is allowed when link autoneg is enabled.\n");
+		return -EINVAL;
+	}
+
 	err = nfp_eth_set_fec(port->app->cpp, eth_port->index, fec);
 	if (!err)
 		/* Only refresh if we did something */
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
index e2d4c487e8de..2c0279dcf299 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
@@ -322,8 +322,11 @@ static int nfp_net_pf_cfg_nsp(struct nfp_pf *pf, bool sp_indiff)
 
 	snprintf(hwinfo, sizeof(hwinfo), "sp_indiff=%d", sp_indiff);
 	err = nfp_nsp_hwinfo_set(nsp, hwinfo, sizeof(hwinfo));
-	if (err)
+	if (err) {
+		/* Not a fatal error, no need to return error to stop driver from loading */
 		nfp_warn(pf->cpp, "HWinfo(sp_indiff=%d) set failed: %d\n", sp_indiff, err);
+		err = 0;
+	}
 
 	nfp_nsp_close(nsp);
 	return err;
@@ -331,7 +334,23 @@ static int nfp_net_pf_cfg_nsp(struct nfp_pf *pf, bool sp_indiff)
 
 static int nfp_net_pf_init_nsp(struct nfp_pf *pf)
 {
-	return nfp_net_pf_cfg_nsp(pf, pf->sp_indiff);
+	int err;
+
+	err = nfp_net_pf_cfg_nsp(pf, pf->sp_indiff);
+	if (!err) {
+		struct nfp_port *port;
+
+		/* The eth ports need be refreshed after nsp is configured,
+		 * since the eth table state may change, e.g. aneg_supp field.
+		 * Only `CHANGED` bit is set here in case nsp needs some time
+		 * to process the configuration.
+		 */
+		list_for_each_entry(port, &pf->ports, port_list)
+			if (__nfp_port_get_eth_port(port))
+				set_bit(NFP_PORT_CHANGED, &port->flags);
+	}
+
+	return err;
 }
 
 static void nfp_net_pf_clean_nsp(struct nfp_pf *pf)
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
index 52465670a01e..e045b6fb5fde 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
@@ -174,6 +174,7 @@ struct nfp_eth_table {
 		bool enabled;
 		bool tx_enabled;
 		bool rx_enabled;
+		bool supp_aneg;
 
 		bool override_changed;
 
@@ -218,6 +219,7 @@ void nfp_eth_config_cleanup_end(struct nfp_nsp *nsp);
 int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode);
 int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed);
 int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes);
+int __nfp_eth_set_fec(struct nfp_nsp *nsp, enum nfp_eth_fec mode);
 
 /**
  * struct nfp_nsp_identify - NSP static information
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
index 18ba7629cdc2..8084d52ade46 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
@@ -27,6 +27,7 @@
 #define NSP_ETH_PORT_PHYLABEL		GENMASK_ULL(59, 54)
 #define NSP_ETH_PORT_FEC_SUPP_BASER	BIT_ULL(60)
 #define NSP_ETH_PORT_FEC_SUPP_RS	BIT_ULL(61)
+#define NSP_ETH_PORT_SUPP_ANEG		BIT_ULL(63)
 
 #define NSP_ETH_PORT_LANES_MASK		cpu_to_le64(NSP_ETH_PORT_LANES)
 
@@ -178,6 +179,7 @@ nfp_eth_port_translate(struct nfp_nsp *nsp, const union eth_table_entry *src,
 		return;
 
 	dst->act_fec = FIELD_GET(NSP_ETH_STATE_ACT_FEC, state);
+	dst->supp_aneg = FIELD_GET(NSP_ETH_PORT_SUPP_ANEG, port);
 }
 
 static void
@@ -564,7 +566,7 @@ int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode)
  *
  * Return: 0 or -ERRNO.
  */
-static int __nfp_eth_set_fec(struct nfp_nsp *nsp, enum nfp_eth_fec mode)
+int __nfp_eth_set_fec(struct nfp_nsp *nsp, enum nfp_eth_fec mode)
 {
 	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
 				      NSP_ETH_STATE_FEC, mode,
-- 
2.30.2

[PATCH net-next 3/3] nfp: add support restart of link auto-negotiation

[Simon Horman]

From: Fei Qin <fei.qin@corigine.com>

Add support restart of link auto-negotiation.
This may be initiated using:

  # ethtool -r <intf>

Signed-off-by: Fei Qin <fei.qin@corigine.com>
Reviewed-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index 00aacc48a7a2..ceda4d0b98bf 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -228,6 +228,37 @@ nfp_net_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo)
 	nfp_get_drvinfo(nn->app, nn->pdev, vnic_version, drvinfo);
 }
 
+static int
+nfp_net_nway_reset(struct net_device *netdev)
+{
+	struct nfp_eth_table_port *eth_port;
+	struct nfp_port *port;
+	int err;
+
+	port = nfp_port_from_netdev(netdev);
+	eth_port = nfp_port_get_eth_port(port);
+	if (!eth_port)
+		return -EOPNOTSUPP;
+
+	if (!netif_running(netdev))
+		return 0;
+
+	err = nfp_port_configure(netdev, false);
+	if (err) {
+		netdev_info(netdev, "Link down failed: %d\n", err);
+		return err;
+	}
+
+	err = nfp_port_configure(netdev, true);
+	if (err) {
+		netdev_info(netdev, "Link up failed: %d\n", err);
+		return err;
+	}
+
+	netdev_info(netdev, "Link reset succeeded\n");
+	return 0;
+}
+
 static void
 nfp_app_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo)
 {
@@ -1854,6 +1885,7 @@ static const struct ethtool_ops nfp_net_ethtool_ops = {
 				     ETHTOOL_COALESCE_MAX_FRAMES |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE,
 	.get_drvinfo		= nfp_net_get_drvinfo,
+	.nway_reset             = nfp_net_nway_reset,
 	.get_link		= ethtool_op_get_link,
 	.get_ringparam		= nfp_net_get_ringparam,
 	.set_ringparam		= nfp_net_set_ringparam,
@@ -1891,6 +1923,7 @@ static const struct ethtool_ops nfp_net_ethtool_ops = {
 
 const struct ethtool_ops nfp_port_ethtool_ops = {
 	.get_drvinfo		= nfp_app_get_drvinfo,
+	.nway_reset             = nfp_net_nway_reset,
 	.get_link		= ethtool_op_get_link,
 	.get_strings		= nfp_port_get_strings,
 	.get_ethtool_stats	= nfp_port_get_stats,
-- 
2.30.2

Re: [PATCH net-next 3/3] nfp: add support restart of link auto-negotiation

[Jakub Kicinski]

On Wed, 21 Sep 2022 14:12:35 +0200 Simon Horman wrote:
> +	err = nfp_port_configure(netdev, false);
> +	if (err) {
> +		netdev_info(netdev, "Link down failed: %d\n", err);
> +		return err;
> +	}
> +
> +	err = nfp_port_configure(netdev, true);
> +	if (err) {
> +		netdev_info(netdev, "Link up failed: %d\n", err);
> +		return err;
> +	}
> +
> +	netdev_info(netdev, "Link reset succeeded\n");
> +	return 0;

This will not do anything if the port is forced up by multi host
or NC-SI.

Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Jakub Kicinski]

On Wed, 21 Sep 2022 14:12:34 +0200 Simon Horman wrote:
> From: Yinjun Zhang <yinjun.zhang@corigine.com>
> 
> Report the auto negotiation capability if it's supported
> in management firmware, and advertise it if it's enabled.
> 
> Changing FEC to mode other than auto or changing port
> speed is not allowed when autoneg is enabled. And FEC mode
> is enforced into auto mode when enabling link autoneg.
> 
> The ethtool <intf> command displays the auto-neg capability:

>  	if (cmd->base.speed != SPEED_UNKNOWN) {
> -		u32 speed = cmd->base.speed / eth_port->lanes;
> +		if (req_aneg) {
> +			netdev_err(netdev, "Speed changing is not allowed when working on autoneg mode.\n");
> +			err = -EINVAL;
> +			goto err_bad_set;
> +		} else {
> +			u32 speed = cmd->base.speed / eth_port->lanes;
>  
> -		err = __nfp_eth_set_speed(nsp, speed);
> +			err = __nfp_eth_set_speed(nsp, speed);
> +			if (err)
> +				goto err_bad_set;
> +		}

Please refactor this to avoid the extra indentation

> +	}
> +
> +	if (req_aneg && nfp_eth_can_support_fec(eth_port) && eth_port->fec != NFP_FEC_AUTO_BIT) {
> +		err = __nfp_eth_set_fec(nsp, NFP_FEC_AUTO_BIT);
>  		if (err)
>  			goto err_bad_set;

> +	if (eth_port->supp_aneg && eth_port->aneg == NFP_ANEG_AUTO && fec != NFP_FEC_AUTO_BIT) {
> +		netdev_err(netdev, "Only auto mode is allowed when link autoneg is enabled.\n");
> +		return -EINVAL;
> +	}

Autoneg and AUTO fec are two completely different things.
There was a long thread on AUTO recently.. :(

>  	snprintf(hwinfo, sizeof(hwinfo), "sp_indiff=%d", sp_indiff);
>  	err = nfp_nsp_hwinfo_set(nsp, hwinfo, sizeof(hwinfo));
> -	if (err)
> +	if (err) {
> +		/* Not a fatal error, no need to return error to stop driver from loading */
>  		nfp_warn(pf->cpp, "HWinfo(sp_indiff=%d) set failed: %d\n", sp_indiff, err);
> +		err = 0;

This should be a separate commit, it seems

>  
>  	nfp_nsp_close(nsp);
>  	return err;
> @@ -331,7 +334,23 @@ static int nfp_net_pf_cfg_nsp(struct nfp_pf *pf, bool sp_indiff)
>  
>  static int nfp_net_pf_init_nsp(struct nfp_pf *pf)
>  {
> -	return nfp_net_pf_cfg_nsp(pf, pf->sp_indiff);
> +	int err;
> +
> +	err = nfp_net_pf_cfg_nsp(pf, pf->sp_indiff);
> +	if (!err) {
> +		struct nfp_port *port;
> +
> +		/* The eth ports need be refreshed after nsp is configured,
> +		 * since the eth table state may change, e.g. aneg_supp field.

No idea why, tho

> +		 * Only `CHANGED` bit is set here in case nsp needs some time
> +		 * to process the configuration.

I can't parse what this is saying but doesn't look good

> +		 */
> +		list_for_each_entry(port, &pf->ports, port_list)
> +			if (__nfp_port_get_eth_port(port))
> +				set_bit(NFP_PORT_CHANGED, &port->flags);
> +	}
> +
> +	return err;
>  }

RE: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Yinjun Zhang]

On Thu, 22 Sep 2022 18:00:40 -0700 Jakub Kicinski wrote:
> On Wed, 21 Sep 2022 14:12:34 +0200 Simon Horman wrote:
> > From: Yinjun Zhang <yinjun.zhang@corigine.com>
> >
> > Report the auto negotiation capability if it's supported
> > in management firmware, and advertise it if it's enabled.
> >
> > Changing FEC to mode other than auto or changing port
> > speed is not allowed when autoneg is enabled. And FEC mode
> > is enforced into auto mode when enabling link autoneg.
> >
> > The ethtool <intf> command displays the auto-neg capability:
> 
> > +				goto err_bad_set;
> > +		}
> 
> Please refactor this to avoid the extra indentation

Will do.

> 
> > +	if (eth_port->supp_aneg && eth_port->aneg == NFP_ANEG_AUTO &&
> fec != NFP_FEC_AUTO_BIT) {
> > +		netdev_err(netdev, "Only auto mode is allowed when link
> autoneg is enabled.\n");
> > +		return -EINVAL;
> > +	}
> 
> Autoneg and AUTO fec are two completely different things.
> There was a long thread on AUTO recently.. :(

Yes, you're right, will remove this limitation.

> 
> > +		/* Not a fatal error, no need to return error to stop driver
> from loading */
> >  		nfp_warn(pf->cpp, "HWinfo(sp_indiff=%d) set failed: %d\n",
> sp_indiff, err);
> > +		err = 0;
> 
> This should be a separate commit, it seems

Will separate it.

> 
> >
> >  	nfp_nsp_close(nsp);
> >  	return err;
> > @@ -331,7 +334,23 @@ static int nfp_net_pf_cfg_nsp(struct nfp_pf *pf,
> bool sp_indiff)
> >
> >  static int nfp_net_pf_init_nsp(struct nfp_pf *pf)
> >  {
> > -	return nfp_net_pf_cfg_nsp(pf, pf->sp_indiff);
> > +	int err;
> > +
> > +	err = nfp_net_pf_cfg_nsp(pf, pf->sp_indiff);
> > +	if (!err) {
> > +		struct nfp_port *port;
> > +
> > +		/* The eth ports need be refreshed after nsp is configured,
> > +		 * since the eth table state may change, e.g. aneg_supp field.
> 
> No idea why, tho
> 
> > +		 * Only `CHANGED` bit is set here in case nsp needs some
> time
> > +		 * to process the configuration.
> 
> I can't parse what this is saying but doesn't look good

I think this comment is clear enough. In previous ` nfp_net_pf_cfg_nsp`, hwinfo "sp_indiff" is 
configured into Management firmware(NSP), and it decides if autoneg is supported or not and
updates eth table accordingly. And only `CHANGED` flag is set here so that with some delay driver
can get the updated eth table instead of stale info.

> 
> > +		 */
> > +		list_for_each_entry(port, &pf->ports, port_list)
> > +			if (__nfp_port_get_eth_port(port))
> > +				set_bit(NFP_PORT_CHANGED, &port->flags);
> > +	}
> > +
> > +	return err;
> >  }

RE: [PATCH net-next 3/3] nfp: add support restart of link auto-negotiation

[Yinjun Zhang]

On Thu, 22 Sep 2022 17:54:53 -0700 Jakub Kicinski wrote:
> On Wed, 21 Sep 2022 14:12:35 +0200 Simon Horman wrote:
> > +
> > +	netdev_info(netdev, "Link reset succeeded\n");
> > +	return 0;
> 
> This will not do anything if the port is forced up by multi host
> or NC-SI.

OK, we're going to do the reset thing as long as it's physically
configured up.

RE: [PATCH net-next 3/3] nfp: add support restart of link auto-negotiation

[Yinjun Zhang]

> On Thu, 22 Sep 2022 17:54:53 -0700 Jakub Kicinski wrote:
> > On Wed, 21 Sep 2022 14:12:35 +0200 Simon Horman wrote:
> > > +
> > > +	netdev_info(netdev, "Link reset succeeded\n");
> > > +	return 0;
> >
> > This will not do anything if the port is forced up by multi host
> > or NC-SI.
> 
> OK, we're going to do the reset thing as long as it's physically
> configured up.

Sorry, I may misunderstand, will use ` nfp_eth_set_configured` directly.

Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Jakub Kicinski]

On Fri, 23 Sep 2022 04:37:58 +0000 Yinjun Zhang wrote:
> > I can't parse what this is saying but doesn't look good  
> 
> I think this comment is clear enough. In previous `
> nfp_net_pf_cfg_nsp`, hwinfo "sp_indiff" is configured into Management
> firmware(NSP), and it decides if autoneg is supported or not and
> updates eth table accordingly. And only `CHANGED` flag is set here so
> that with some delay driver can get the updated eth table instead of
> stale info.

Why is the sp_indif thing configured at the nfp_main layer, before 
the eth table is read? Doing this inside nfp_net_main seems like 
the wrong layering to me.

Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Yinjun Zhang]

On Fri, Sep 23, 2022 at 06:21:14AM -0700, Jakub Kicinski wrote:
> On Fri, 23 Sep 2022 04:37:58 +0000 Yinjun Zhang wrote:
> > > I can't parse what this is saying but doesn't look good  
> > 
> > I think this comment is clear enough. In previous `
> > nfp_net_pf_cfg_nsp`, hwinfo "sp_indiff" is configured into Management
> > firmware(NSP), and it decides if autoneg is supported or not and
> > updates eth table accordingly. And only `CHANGED` flag is set here so
> > that with some delay driver can get the updated eth table instead of
> > stale info.
> 
> Why is the sp_indif thing configured at the nfp_main layer, before 
> the eth table is read? Doing this inside nfp_net_main seems like 
> the wrong layering to me.

Because the value of sp_indiff depends on the loaded application
firmware, please ref to previous commit:
2b88354d37ca ("nfp: check if application firmware is indifferent to port speed")

Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Jakub Kicinski]

On Fri, 23 Sep 2022 23:41:57 +0800 Yinjun Zhang wrote:
> > Why is the sp_indif thing configured at the nfp_main layer, before 
> > the eth table is read? Doing this inside nfp_net_main seems like 
> > the wrong layering to me.  
> 
> Because the value of sp_indiff depends on the loaded application
> firmware, please ref to previous commit:
> 2b88354d37ca ("nfp: check if application firmware is indifferent to port speed")

AFAICT you check if it's flower, you can check that in the main code,
the app id is just a symbol, right?

Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Yinjun Zhang]

On Fri, Sep 23, 2022 at 05:24:10PM -0700, Jakub Kicinski wrote:
> On Fri, 23 Sep 2022 23:41:57 +0800 Yinjun Zhang wrote:
> > > Why is the sp_indif thing configured at the nfp_main layer, before 
> > > the eth table is read? Doing this inside nfp_net_main seems like 
> > > the wrong layering to me.  
> > 
> > Because the value of sp_indiff depends on the loaded application
> > firmware, please ref to previous commit:
> > 2b88354d37ca ("nfp: check if application firmware is indifferent to port speed")
> 
> AFAICT you check if it's flower, you can check that in the main code,
> the app id is just a symbol, right?

Not only check if it's flower, but also check if it's sp_indiff when
it's not flower by parsing the tlv caps.

Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Jakub Kicinski]

On Sat, 24 Sep 2022 10:45:30 +0800 Yinjun Zhang wrote:
> On Fri, Sep 23, 2022 at 05:24:10PM -0700, Jakub Kicinski wrote:
> > > Because the value of sp_indiff depends on the loaded application
> > > firmware, please ref to previous commit:
> > > 2b88354d37ca ("nfp: check if application firmware is indifferent to port speed")  
> > 
> > AFAICT you check if it's flower, you can check that in the main code,
> > the app id is just a symbol, right?  
> 
> Not only check if it's flower, but also check if it's sp_indiff when
> it's not flower by parsing the tlv caps.

Seems bogus. The speed independence is a property of the whole FW image,
you record it in the pf structure.

Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Yinjun Zhang]

On Mon, Sep 26, 2022 at 09:25:47AM -0700, Jakub Kicinski wrote:
> On Sat, 24 Sep 2022 10:45:30 +0800 Yinjun Zhang wrote:
> > On Fri, Sep 23, 2022 at 05:24:10PM -0700, Jakub Kicinski wrote:
> > > > Because the value of sp_indiff depends on the loaded application
> > > > firmware, please ref to previous commit:
> > > > 2b88354d37ca ("nfp: check if application firmware is indifferent to port speed")  
> > > 
> > > AFAICT you check if it's flower, you can check that in the main code,
> > > the app id is just a symbol, right?  
> > 
> > Not only check if it's flower, but also check if it's sp_indiff when
> > it's not flower by parsing the tlv caps.
> 
> Seems bogus. The speed independence is a property of the whole FW image,
> you record it in the pf structure.

It's indeed a per-fw property, but we don't have existing way to expose
per-fw capabilities to driver currently, so use per-vnic tlv caps here.
Maybe define a new fw symbol is a choice, but my concern is it's not
visible to netvf driver.
Any suggestion is welcomed.

Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Jakub Kicinski]

On Tue, 27 Sep 2022 09:13:53 +0800 Yinjun Zhang wrote:
> On Mon, Sep 26, 2022 at 09:25:47AM -0700, Jakub Kicinski wrote:
> > On Sat, 24 Sep 2022 10:45:30 +0800 Yinjun Zhang wrote:  
> > > Not only check if it's flower, but also check if it's sp_indiff when
> > > it's not flower by parsing the tlv caps.  
> > 
> > Seems bogus. The speed independence is a property of the whole FW image,
> > you record it in the pf structure.  
> 
> It's indeed a per-fw property, but we don't have existing way to expose
> per-fw capabilities to driver currently, so use per-vnic tlv caps here.
> Maybe define a new fw symbol is a choice, but my concern is it's not
> visible to netvf driver.
> Any suggestion is welcomed.

Why not put an rtsym with the value in the FW? That'd be my first
go-to way of communicating information about the FW as a whole.

Re: [PATCH net-next 2/3] nfp: add support for link auto negotiation

[Yinjun Zhang]

On Mon, Sep 26, 2022 at 06:38:40PM -0700, Jakub Kicinski wrote:
> On Tue, 27 Sep 2022 09:13:53 +0800 Yinjun Zhang wrote:
> > On Mon, Sep 26, 2022 at 09:25:47AM -0700, Jakub Kicinski wrote:
> > > On Sat, 24 Sep 2022 10:45:30 +0800 Yinjun Zhang wrote:  
> > > > Not only check if it's flower, but also check if it's sp_indiff when
> > > > it's not flower by parsing the tlv caps.  
> > > 
> > > Seems bogus. The speed independence is a property of the whole FW image,
> > > you record it in the pf structure.  
> > 
> > It's indeed a per-fw property, but we don't have existing way to expose
> > per-fw capabilities to driver currently, so use per-vnic tlv caps here.
> > Maybe define a new fw symbol is a choice, but my concern is it's not
> > visible to netvf driver.
> > Any suggestion is welcomed.
> 
> Why not put an rtsym with the value in the FW? That'd be my first
> go-to way of communicating information about the FW as a whole.

Like I said, the VF driver cannot read the rtsym, so it's not a perfect
way as exposing a per-FW property. But for this case, it's OK since VF
doesn't need this info. I'll try this way. Thanks.