[net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2020-04-20

[net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2020-04-20

[Jeff Kirsher]

This small series contains updates to i40e only.

Takashi Iwai, from SUSE, changes snprintf() to scnprintf() to avoid
potential buffer overflow.

Jesper Dangaard Brouer fixes code comments in the XDP code, which were a
cut and paste error.

Arkadiusz adds support for total port shutdown, which allows completely
shutdown the port on the link-down procedure by physically removing the
link from the port.

Todd adds a check to see if Max Frame Size (MFS) is set to an
unsupported value.

The following are changes since commit 82ebc889091a488b4dd95e682b3c3b889a50713c:
  qed: use true,false for bool variables
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue 40GbE

Arkadiusz Kubalewski (1):
  i40e: Add support for a new feature: Total Port Shutdown

Jesper Dangaard Brouer (1):
  i40e: trivial fixup of comments in i40e_xsk.c

Takashi Iwai (1):
  i40e: Use scnprintf() for avoiding potential buffer overflow

Todd Fujinaka (1):
  i40e: Add a check to see if MFS is set

 drivers/net/ethernet/intel/i40e/i40e.h        |   1 +
 .../net/ethernet/intel/i40e/i40e_ethtool.c    |   8 +
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 158 ++++++++++++++----
 drivers/net/ethernet/intel/i40e/i40e_xsk.c    |   4 +-
 4 files changed, 135 insertions(+), 36 deletions(-)

-- 
2.25.3

[net-next 1/4] i40e: Use scnprintf() for avoiding potential buffer overflow

[Jeff Kirsher]

From: Takashi Iwai <tiwai@suse.de>

Since snprintf() returns the would-be-output size instead of the
actual output size, the succeeding calls may go beyond the given
buffer limit.  Fix it by replacing with scnprintf().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 24 ++++++++++-----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 8c3e753bfb9d..ff431c13f858 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -14478,29 +14478,29 @@ static void i40e_print_features(struct i40e_pf *pf)
 
 	i = snprintf(buf, INFO_STRING_LEN, "Features: PF-id[%d]", hw->pf_id);
 #ifdef CONFIG_PCI_IOV
-	i += snprintf(&buf[i], REMAIN(i), " VFs: %d", pf->num_req_vfs);
+	i += scnprintf(&buf[i], REMAIN(i), " VFs: %d", pf->num_req_vfs);
 #endif
-	i += snprintf(&buf[i], REMAIN(i), " VSIs: %d QP: %d",
+	i += scnprintf(&buf[i], REMAIN(i), " VSIs: %d QP: %d",
 		      pf->hw.func_caps.num_vsis,
 		      pf->vsi[pf->lan_vsi]->num_queue_pairs);
 	if (pf->flags & I40E_FLAG_RSS_ENABLED)
-		i += snprintf(&buf[i], REMAIN(i), " RSS");
+		i += scnprintf(&buf[i], REMAIN(i), " RSS");
 	if (pf->flags & I40E_FLAG_FD_ATR_ENABLED)
-		i += snprintf(&buf[i], REMAIN(i), " FD_ATR");
+		i += scnprintf(&buf[i], REMAIN(i), " FD_ATR");
 	if (pf->flags & I40E_FLAG_FD_SB_ENABLED) {
-		i += snprintf(&buf[i], REMAIN(i), " FD_SB");
-		i += snprintf(&buf[i], REMAIN(i), " NTUPLE");
+		i += scnprintf(&buf[i], REMAIN(i), " FD_SB");
+		i += scnprintf(&buf[i], REMAIN(i), " NTUPLE");
 	}
 	if (pf->flags & I40E_FLAG_DCB_CAPABLE)
-		i += snprintf(&buf[i], REMAIN(i), " DCB");
-	i += snprintf(&buf[i], REMAIN(i), " VxLAN");
-	i += snprintf(&buf[i], REMAIN(i), " Geneve");
+		i += scnprintf(&buf[i], REMAIN(i), " DCB");
+	i += scnprintf(&buf[i], REMAIN(i), " VxLAN");
+	i += scnprintf(&buf[i], REMAIN(i), " Geneve");
 	if (pf->flags & I40E_FLAG_PTP)
-		i += snprintf(&buf[i], REMAIN(i), " PTP");
+		i += scnprintf(&buf[i], REMAIN(i), " PTP");
 	if (pf->flags & I40E_FLAG_VEB_MODE_ENABLED)
-		i += snprintf(&buf[i], REMAIN(i), " VEB");
+		i += scnprintf(&buf[i], REMAIN(i), " VEB");
 	else
-		i += snprintf(&buf[i], REMAIN(i), " VEPA");
+		i += scnprintf(&buf[i], REMAIN(i), " VEPA");
 
 	dev_info(&pf->pdev->dev, "%s\n", buf);
 	kfree(buf);
-- 
2.25.3

[net-next 2/4] i40e: trivial fixup of comments in i40e_xsk.c

[Jeff Kirsher]

From: Jesper Dangaard Brouer <brouer@redhat.com>

The comment above i40e_run_xdp_zc() was clearly copy-pasted from
function i40e_xsk_umem_setup, which is just above.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Björn Töpel <bjorn.topel@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 0b7d29192b2c..30dfb0d3d185 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -184,8 +184,6 @@ int i40e_xsk_umem_setup(struct i40e_vsi *vsi, struct xdp_umem *umem,
  * @rx_ring: Rx ring
  * @xdp: xdp_buff used as input to the XDP program
  *
- * This function enables or disables a UMEM to a certain ring.
- *
  * Returns any of I40E_XDP_{PASS, CONSUMED, TX, REDIR}
  **/
 static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct xdp_buff *xdp)
@@ -474,7 +472,7 @@ void i40e_zca_free(struct zero_copy_allocator *alloc, unsigned long handle)
 }
 
 /**
- * i40e_construct_skb_zc - Create skbufff from zero-copy Rx buffer
+ * i40e_construct_skb_zc - Create skbuff from zero-copy Rx buffer
  * @rx_ring: Rx ring
  * @bi: Rx buffer
  * @xdp: xdp_buff
-- 
2.25.3

[net-next 3/4] i40e: Add support for a new feature: Total Port Shutdown

[Jeff Kirsher]

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

Currently after requesting to down a link on a physical network port,
the traffic is no longer being processed but the physical link
with a link partner is still established.

Total Port Shutdown allows to completely shutdown the port on the
link-down procedure by physically removing the link from the port.

Introduced changes:
- probe NVM if the feature was enabled at initialization of the port
- special handling on link-down procedure to let FW physically
shutdown the port if the feature was enabled

Testing Hints (required if no HSD):
Link up/down, link-down-on-close

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h        |   1 +
 .../net/ethernet/intel/i40e/i40e_ethtool.c    |   8 ++
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 125 +++++++++++++++---
 3 files changed, 113 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index e95b8da45e07..bd9b66ba96d8 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -567,6 +567,7 @@ struct i40e_pf {
 #define I40E_FLAG_DISABLE_FW_LLDP		BIT(24)
 #define I40E_FLAG_RS_FEC			BIT(25)
 #define I40E_FLAG_BASE_R_FEC			BIT(26)
+#define I40E_FLAG_TOTAL_PORT_SHUTDOWN		BIT(27)
 
 	struct i40e_client_instance *cinst;
 	bool stat_offsets_loaded;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index aa8026b1eb81..533dab295720 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -428,6 +428,7 @@ struct i40e_priv_flags {
 static const struct i40e_priv_flags i40e_gstrings_priv_flags[] = {
 	/* NOTE: MFP setting cannot be changed */
 	I40E_PRIV_FLAG("MFP", I40E_FLAG_MFP_ENABLED, 1),
+	I40E_PRIV_FLAG("total-port-shutdown", I40E_FLAG_TOTAL_PORT_SHUTDOWN, 1),
 	I40E_PRIV_FLAG("LinkPolling", I40E_FLAG_LINK_POLLING_ENABLED, 0),
 	I40E_PRIV_FLAG("flow-director-atr", I40E_FLAG_FD_ATR_ENABLED, 0),
 	I40E_PRIV_FLAG("veb-stats", I40E_FLAG_VEB_STATS_ENABLED, 0),
@@ -5006,6 +5007,13 @@ static int i40e_set_priv_flags(struct net_device *dev, u32 flags)
 			dev_warn(&pf->pdev->dev, "Cannot change FEC config\n");
 	}
 
+	if ((changed_flags & I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED) &&
+	    (orig_flags & I40E_FLAG_TOTAL_PORT_SHUTDOWN)) {
+		dev_err(&pf->pdev->dev,
+			"Setting link-down-on-close not supported on this port\n");
+		return -EOPNOTSUPP;
+	}
+
 	if ((changed_flags & new_flags &
 	     I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED) &&
 	    (new_flags & I40E_FLAG_MFP_ENABLED))
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index ff431c13f858..4c414208a22a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -54,7 +54,7 @@ static void i40e_fdir_sb_setup(struct i40e_pf *pf);
 static int i40e_veb_get_bw_info(struct i40e_veb *veb);
 static int i40e_get_capabilities(struct i40e_pf *pf,
 				 enum i40e_admin_queue_opc list_type);
-
+static bool i40e_is_total_port_shutdown_enabled(struct i40e_pf *pf);
 
 /* i40e_pci_tbl - PCI Device ID Table
  *
@@ -6672,21 +6672,6 @@ static void i40e_vsi_reinit_locked(struct i40e_vsi *vsi)
 	clear_bit(__I40E_CONFIG_BUSY, pf->state);
 }
 
-/**
- * i40e_up - Bring the connection back up after being down
- * @vsi: the VSI being configured
- **/
-int i40e_up(struct i40e_vsi *vsi)
-{
-	int err;
-
-	err = i40e_vsi_configure(vsi);
-	if (!err)
-		err = i40e_up_complete(vsi);
-
-	return err;
-}
-
 /**
  * i40e_force_link_state - Force the link status
  * @pf: board private structure
@@ -6697,10 +6682,12 @@ static i40e_status i40e_force_link_state(struct i40e_pf *pf, bool is_up)
 	struct i40e_aq_get_phy_abilities_resp abilities;
 	struct i40e_aq_set_phy_config config = {0};
 	struct i40e_hw *hw = &pf->hw;
+	bool non_zero_phy_type;
 	i40e_status err;
 	u64 mask;
 	u8 speed;
 
+	non_zero_phy_type = is_up;
 	/* Card might've been put in an unstable state by other drivers
 	 * and applications, which causes incorrect speed values being
 	 * set on startup. In order to clear speed registers, we call
@@ -6731,8 +6718,11 @@ static i40e_status i40e_force_link_state(struct i40e_pf *pf, bool is_up)
 
 	/* If link needs to go up, but was not forced to go down,
 	 * and its speed values are OK, no need for a flap
+	 * if non_zero_phy_type was set, still need to force up
 	 */
-	if (is_up && abilities.phy_type != 0 && abilities.link_speed != 0)
+	if (pf->flags & I40E_FLAG_TOTAL_PORT_SHUTDOWN)
+		non_zero_phy_type = true;
+	else if (is_up && abilities.phy_type != 0 && abilities.link_speed != 0)
 		return I40E_SUCCESS;
 
 	/* To force link we need to set bits for all supported PHY types,
@@ -6740,10 +6730,18 @@ static i40e_status i40e_force_link_state(struct i40e_pf *pf, bool is_up)
 	 * across two fields.
 	 */
 	mask = I40E_PHY_TYPES_BITMASK;
-	config.phy_type = is_up ? cpu_to_le32((u32)(mask & 0xffffffff)) : 0;
-	config.phy_type_ext = is_up ? (u8)((mask >> 32) & 0xff) : 0;
+	config.phy_type =
+		non_zero_phy_type ? cpu_to_le32((u32)(mask & 0xffffffff)) : 0;
+	config.phy_type_ext =
+		non_zero_phy_type ? (u8)((mask >> 32) & 0xff) : 0;
 	/* Copy the old settings, except of phy_type */
 	config.abilities = abilities.abilities;
+	if (pf->flags & I40E_FLAG_TOTAL_PORT_SHUTDOWN) {
+		if (is_up)
+			config.abilities |= I40E_AQ_PHY_ENABLE_LINK;
+		else
+			config.abilities &= ~(I40E_AQ_PHY_ENABLE_LINK);
+	}
 	if (abilities.link_speed != 0)
 		config.link_speed = abilities.link_speed;
 	else
@@ -6774,11 +6772,31 @@ static i40e_status i40e_force_link_state(struct i40e_pf *pf, bool is_up)
 		i40e_update_link_info(hw);
 	}
 
-	i40e_aq_set_link_restart_an(hw, true, NULL);
+	i40e_aq_set_link_restart_an(hw, is_up, NULL);
 
 	return I40E_SUCCESS;
 }
 
+/**
+ * i40e_up - Bring the connection back up after being down
+ * @vsi: the VSI being configured
+ **/
+int i40e_up(struct i40e_vsi *vsi)
+{
+	int err;
+
+	if (vsi->type == I40E_VSI_MAIN &&
+	    (vsi->back->flags & I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED ||
+	     vsi->back->flags & I40E_FLAG_TOTAL_PORT_SHUTDOWN))
+		i40e_force_link_state(vsi->back, true);
+
+	err = i40e_vsi_configure(vsi);
+	if (!err)
+		err = i40e_up_complete(vsi);
+
+	return err;
+}
+
 /**
  * i40e_down - Shutdown the connection processing
  * @vsi: the VSI being stopped
@@ -6797,7 +6815,8 @@ void i40e_down(struct i40e_vsi *vsi)
 	i40e_vsi_disable_irq(vsi);
 	i40e_vsi_stop_rings(vsi);
 	if (vsi->type == I40E_VSI_MAIN &&
-	    vsi->back->flags & I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED)
+	   (vsi->back->flags & I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED ||
+	    vsi->back->flags & I40E_FLAG_TOTAL_PORT_SHUTDOWN))
 		i40e_force_link_state(vsi->back, false);
 	i40e_napi_disable_all(vsi);
 
@@ -11837,6 +11856,60 @@ i40e_status i40e_commit_partition_bw_setting(struct i40e_pf *pf)
 	return ret;
 }
 
+/**
+ * i40e_is_total_port_shutdown_enabled - read nvm and return value
+ * if total port shutdown feature is enabled for this pf
+ * @pf: board private structure
+ **/
+static bool i40e_is_total_port_shutdown_enabled(struct i40e_pf *pf)
+{
+#define I40E_TOTAL_PORT_SHUTDOWN_ENABLED	BIT(4)
+#define I40E_FEATURES_ENABLE_PTR		0x2A
+#define I40E_CURRENT_SETTING_PTR		0x2B
+#define I40E_LINK_BEHAVIOR_WORD_OFFSET		0x2D
+#define I40E_LINK_BEHAVIOR_WORD_LENGTH		0x1
+#define I40E_LINK_BEHAVIOR_OS_FORCED_ENABLED	BIT(0)
+#define I40E_LINK_BEHAVIOR_PORT_BIT_LENGTH	4
+	i40e_status read_status = I40E_SUCCESS;
+	u16 sr_emp_sr_settings_ptr = 0;
+	u16 features_enable = 0;
+	u16 link_behavior = 0;
+	bool ret = false;
+
+	read_status = i40e_read_nvm_word(&pf->hw,
+					 I40E_SR_EMP_SR_SETTINGS_PTR,
+					 &sr_emp_sr_settings_ptr);
+	if (read_status)
+		goto err_nvm;
+	read_status = i40e_read_nvm_word(&pf->hw,
+					 sr_emp_sr_settings_ptr +
+					 I40E_FEATURES_ENABLE_PTR,
+					 &features_enable);
+	if (read_status)
+		goto err_nvm;
+	if (I40E_TOTAL_PORT_SHUTDOWN_ENABLED & features_enable) {
+		read_status =
+		i40e_read_nvm_module_data(&pf->hw,
+					  I40E_SR_EMP_SR_SETTINGS_PTR,
+					  I40E_CURRENT_SETTING_PTR,
+					  I40E_LINK_BEHAVIOR_WORD_OFFSET,
+					  I40E_LINK_BEHAVIOR_WORD_LENGTH,
+					  &link_behavior);
+		if (read_status)
+			goto err_nvm;
+		link_behavior >>=
+		(pf->hw.port * I40E_LINK_BEHAVIOR_PORT_BIT_LENGTH);
+		ret = I40E_LINK_BEHAVIOR_OS_FORCED_ENABLED & link_behavior;
+	}
+	return ret;
+
+err_nvm:
+	dev_warn(&pf->pdev->dev,
+		 "Total Port Shutdown feature is off due to read nvm error:%d\n",
+		 read_status);
+	return ret;
+}
+
 /**
  * i40e_sw_init - Initialize general software structures (struct i40e_pf)
  * @pf: board private structure to initialize
@@ -12012,6 +12085,16 @@ static int i40e_sw_init(struct i40e_pf *pf)
 
 	pf->tx_timeout_recovery_level = 1;
 
+	if (pf->hw.mac.type != I40E_MAC_X722 &&
+	    i40e_is_total_port_shutdown_enabled(pf)) {
+		/* Link down on close must be on when total port shutdown
+		 * is enabled for a given port
+		 */
+		pf->flags |= (I40E_FLAG_TOTAL_PORT_SHUTDOWN
+			  | I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED);
+		dev_info(&pf->pdev->dev,
+			 "Total Port Shutdown is enabled, link-down-on-close forced on\n");
+	}
 	mutex_init(&pf->switch_mutex);
 
 sw_init_done:
-- 
2.25.3

[net-next 4/4] i40e: Add a check to see if MFS is set

[Jeff Kirsher]

From: Todd Fujinaka <todd.fujinaka@intel.com>

A customer was chain-booting to provision his systems and one of the
steps was setting MFS. MFS isn't cleared by normal warm reboots
(clearing requires a GLOBR) and there was no indication of why Jumbo
Frame receives were failing.

Add a warning if MFS is set to anything lower than the default.

Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 4c414208a22a..3fdbfede0b87 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -15347,6 +15347,15 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			i40e_stat_str(&pf->hw, err),
 			i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status));
 
+	/* make sure the MFS hasn't been set lower than the default */
+#define MAX_FRAME_SIZE_DEFAULT 0x2600
+	for (i = 0; i < 4; i++) {
+		val = ((rd32(&pf->hw, I40E_PRTGL_SAH) & I40E_PRTGL_SAH_MFS_MASK)
+			>> I40E_PRTGL_SAH_MFS_SHIFT);
+		if (val < MAX_FRAME_SIZE_DEFAULT)
+			dev_warn(&pdev->dev, "MFS for port %x has been set below the default: %x\n", i, val);
+	}
+
 	/* Add a filter to drop all Flow control frames from any VSI from being
 	 * transmitted. By doing so we stop a malicious VF from sending out
 	 * PAUSE or PFC frames and potentially controlling traffic for other
-- 
2.25.3

Re: [net-next 3/4] i40e: Add support for a new feature: Total Port Shutdown

[Jakub Kicinski]

On Mon, 20 Apr 2020 18:49:31 -0700 Jeff Kirsher wrote:
> From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> 
> Currently after requesting to down a link on a physical network port,
> the traffic is no longer being processed but the physical link
> with a link partner is still established.
> 
> Total Port Shutdown allows to completely shutdown the port on the
> link-down procedure by physically removing the link from the port.
> 
> Introduced changes:
> - probe NVM if the feature was enabled at initialization of the port
> - special handling on link-down procedure to let FW physically
> shutdown the port if the feature was enabled

How is this different than link-down-on-close?

Perhaps it'd be good to start documenting the private flags in
Documentation/

> Testing Hints (required if no HSD):
> Link up/down, link-down-on-close
> 
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

> @@ -12012,6 +12085,16 @@ static int i40e_sw_init(struct i40e_pf *pf)
>  
>  	pf->tx_timeout_recovery_level = 1;
>  
> +	if (pf->hw.mac.type != I40E_MAC_X722 &&
> +	    i40e_is_total_port_shutdown_enabled(pf)) {
> +		/* Link down on close must be on when total port shutdown
> +		 * is enabled for a given port
> +		 */
> +		pf->flags |= (I40E_FLAG_TOTAL_PORT_SHUTDOWN
> +			  | I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED);

FWIW this is the correct code style in the kernel:

flags = BLA |
        BLA2;

> +		dev_info(&pf->pdev->dev,
> +			 "Total Port Shutdown is enabled, link-down-on-close forced on\n");
> +	}
>  	mutex_init(&pf->switch_mutex);
>  
>  sw_init_done:

Re: [net-next 4/4] i40e: Add a check to see if MFS is set

[Jakub Kicinski]

On Mon, 20 Apr 2020 18:49:32 -0700 Jeff Kirsher wrote:
> From: Todd Fujinaka <todd.fujinaka@intel.com>
> 
> A customer was chain-booting to provision his systems and one of the
> steps was setting MFS. MFS isn't cleared by normal warm reboots
> (clearing requires a GLOBR) and there was no indication of why Jumbo
> Frame receives were failing.
> 
> Add a warning if MFS is set to anything lower than the default.
> 
> Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 4c414208a22a..3fdbfede0b87 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -15347,6 +15347,15 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  			i40e_stat_str(&pf->hw, err),
>  			i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status));
>  
> +	/* make sure the MFS hasn't been set lower than the default */
> +#define MAX_FRAME_SIZE_DEFAULT 0x2600
> +	for (i = 0; i < 4; i++) {

Why is this a loop? AFAICS @i is only used in the warning message

> +		val = ((rd32(&pf->hw, I40E_PRTGL_SAH) & I40E_PRTGL_SAH_MFS_MASK)
> +			>> I40E_PRTGL_SAH_MFS_SHIFT);

outer parens unnecessary

> +		if (val < MAX_FRAME_SIZE_DEFAULT)
> +			dev_warn(&pdev->dev, "MFS for port %x has been set below the default: %x\n", i, val);

Shouldn't you just reset it to default at this point? If the value is
not reset on warm boot this is not really a surprise.

> +	}
> +
>  	/* Add a filter to drop all Flow control frames from any VSI from being
>  	 * transmitted. By doing so we stop a malicious VF from sending out
>  	 * PAUSE or PFC frames and potentially controlling traffic for other

RE: [net-next 4/4] i40e: Add a check to see if MFS is set

[Fujinaka, Todd]

Sorry for the top-posting. Outlook. I can't seem to find this email on my non-Windows machine.

I don't know why we don't reset the NIC, but I was told there is a reason. And the reason there's an index is because
the datasheet shows four registers. Somehow while I was formatting that long line I lost the math bits that read four
different registers. Let me see if I can fix this patch.

Todd Fujinaka
Software Application Engineer
Data Center Group
Intel Corporation
todd.fujinaka@intel.com

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org> 
Sent: Tuesday, April 21, 2020 11:00 AM
To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
Cc: davem@davemloft.net; Fujinaka, Todd <todd.fujinaka@intel.com>; netdev@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Bowers, AndrewX <andrewx.bowers@intel.com>
Subject: Re: [net-next 4/4] i40e: Add a check to see if MFS is set

On Mon, 20 Apr 2020 18:49:32 -0700 Jeff Kirsher wrote:
> From: Todd Fujinaka <todd.fujinaka@intel.com>
> 
> A customer was chain-booting to provision his systems and one of the 
> steps was setting MFS. MFS isn't cleared by normal warm reboots 
> (clearing requires a GLOBR) and there was no indication of why Jumbo 
> Frame receives were failing.
> 
> Add a warning if MFS is set to anything lower than the default.
> 
> Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 4c414208a22a..3fdbfede0b87 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -15347,6 +15347,15 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  			i40e_stat_str(&pf->hw, err),
>  			i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status));
>  
> +	/* make sure the MFS hasn't been set lower than the default */ 
> +#define MAX_FRAME_SIZE_DEFAULT 0x2600
> +	for (i = 0; i < 4; i++) {

Why is this a loop? AFAICS @i is only used in the warning message

> +		val = ((rd32(&pf->hw, I40E_PRTGL_SAH) & I40E_PRTGL_SAH_MFS_MASK)
> +			>> I40E_PRTGL_SAH_MFS_SHIFT);

outer parens unnecessary

> +		if (val < MAX_FRAME_SIZE_DEFAULT)
> +			dev_warn(&pdev->dev, "MFS for port %x has been set below the 
> +default: %x\n", i, val);

Shouldn't you just reset it to default at this point? If the value is not reset on warm boot this is not really a surprise.

> +	}
> +
>  	/* Add a filter to drop all Flow control frames from any VSI from being
>  	 * transmitted. By doing so we stop a malicious VF from sending out
>  	 * PAUSE or PFC frames and potentially controlling traffic for other

RE: [net-next 3/4] i40e: Add support for a new feature: Total Port Shutdown

[Kirsher, Jeffrey T]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, April 21, 2020 10:56
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: davem@davemloft.net; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@intel.com>; netdev@vger.kernel.org;
> nhorman@redhat.com; sassmann@redhat.com; Kwapulinski, Piotr
> <piotr.kwapulinski@intel.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Bowers, AndrewX
> <andrewx.bowers@intel.com>
> Subject: Re: [net-next 3/4] i40e: Add support for a new feature: Total Port
> Shutdown
> 
> On Mon, 20 Apr 2020 18:49:31 -0700 Jeff Kirsher wrote:
> > From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> >
> > Currently after requesting to down a link on a physical network port,
> > the traffic is no longer being processed but the physical link with a
> > link partner is still established.
> >
> > Total Port Shutdown allows to completely shutdown the port on the
> > link-down procedure by physically removing the link from the port.
> >
> > Introduced changes:
> > - probe NVM if the feature was enabled at initialization of the port
> > - special handling on link-down procedure to let FW physically
> > shutdown the port if the feature was enabled
> 
> How is this different than link-down-on-close?
[Kirsher, Jeffrey T] 

First of all total-port-shutdown is a read only flag, the user cannot set it
from the OS.  It is possible to set it in bios, but only if the motherboard
supports it and the NIC has that capability.  Also, the behavior on both
slightly differs, link-down-on-close brings the link down by sending
(to firmware) phy_type=0, while total-port-shutdown does not, the
phy_type is not changed, instead firmware is using
I40E_AQ_PHY_ENABLE_LINK flag. 

> 
> Perhaps it'd be good to start documenting the private flags in Documentation/
[Kirsher, Jeffrey T] 

We could look at adding that information into our kernel documentation, I am
planning on updating the driver documentation in a follow-up patch set.  Would
a descriptive code comment help in this case? 

> 
> > Testing Hints (required if no HSD):
> > Link up/down, link-down-on-close
> >
> > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> > Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> 
> > @@ -12012,6 +12085,16 @@ static int i40e_sw_init(struct i40e_pf *pf)
> >
> >  	pf->tx_timeout_recovery_level = 1;
> >
> > +	if (pf->hw.mac.type != I40E_MAC_X722 &&
> > +	    i40e_is_total_port_shutdown_enabled(pf)) {
> > +		/* Link down on close must be on when total port shutdown
> > +		 * is enabled for a given port
> > +		 */
> > +		pf->flags |= (I40E_FLAG_TOTAL_PORT_SHUTDOWN
> > +			  | I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED);
> 
> FWIW this is the correct code style in the kernel:
> 
> flags = BLA |
>         BLA2;
[Kirsher, Jeffrey T] 

This will get fixed in v2.

> 
> > +		dev_info(&pf->pdev->dev,
> > +			 "Total Port Shutdown is enabled, link-down-on-close
> forced on\n");
> > +	}
> >  	mutex_init(&pf->switch_mutex);
> >
> >  sw_init_done:

Re: [net-next 3/4] i40e: Add support for a new feature: Total Port Shutdown

[Jakub Kicinski]

On Tue, 21 Apr 2020 22:36:21 +0000 Kirsher, Jeffrey T wrote:
> > -----Original Message-----
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Tuesday, April 21, 2020 10:56
> > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> > Cc: davem@davemloft.net; Kubalewski, Arkadiusz
> > <arkadiusz.kubalewski@intel.com>; netdev@vger.kernel.org;
> > nhorman@redhat.com; sassmann@redhat.com; Kwapulinski, Piotr
> > <piotr.kwapulinski@intel.com>; Loktionov, Aleksandr
> > <aleksandr.loktionov@intel.com>; Bowers, AndrewX
> > <andrewx.bowers@intel.com>
> > Subject: Re: [net-next 3/4] i40e: Add support for a new feature: Total Port
> > Shutdown
> > 
> > On Mon, 20 Apr 2020 18:49:31 -0700 Jeff Kirsher wrote:  
> > > From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> > >
> > > Currently after requesting to down a link on a physical network port,
> > > the traffic is no longer being processed but the physical link with a
> > > link partner is still established.
> > >
> > > Total Port Shutdown allows to completely shutdown the port on the
> > > link-down procedure by physically removing the link from the port.
> > >
> > > Introduced changes:
> > > - probe NVM if the feature was enabled at initialization of the port
> > > - special handling on link-down procedure to let FW physically
> > > shutdown the port if the feature was enabled  
> > 
> > How is this different than link-down-on-close?  
> [Kirsher, Jeffrey T] 
> 
> First of all total-port-shutdown is a read only flag, the user cannot set it
> from the OS.  It is possible to set it in bios, but only if the motherboard
> supports it and the NIC has that capability.  Also, the behavior on both
> slightly differs, link-down-on-close brings the link down by sending
> (to firmware) phy_type=0, while total-port-shutdown does not, the
> phy_type is not changed, instead firmware is using
> I40E_AQ_PHY_ENABLE_LINK flag. 

I see. IOW it's a flag that says the other flag is hard wired to on.

Why is it important to prevent user from performing the configuration?
What if an old kernel is run which won't prevent it?

Let's drill down into what we actually want to express here and then
look at the API. Michal has already converted ethtool link info to
netlink..

> > Perhaps it'd be good to start documenting the private flags in Documentation/  
> [Kirsher, Jeffrey T] 
> 
> We could look at adding that information into our kernel documentation, I am
> planning on updating the driver documentation in a follow-up patch set.  Would
> a descriptive code comment help in this case? 

Documentation should be sufficient, IMHO, if it's coming soon.

RE: [net-next 3/4] i40e: Add support for a new feature: Total Port Shutdown

[Kirsher, Jeffrey T]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, April 21, 2020 19:14
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: davem@davemloft.net; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@intel.com>; netdev@vger.kernel.org;
> nhorman@redhat.com; sassmann@redhat.com; Kwapulinski, Piotr
> <piotr.kwapulinski@intel.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>
> Subject: Re: [net-next 3/4] i40e: Add support for a new feature: Total Port
> Shutdown
> 
> On Tue, 21 Apr 2020 22:36:21 +0000 Kirsher, Jeffrey T wrote:
> > > -----Original Message-----
> > > From: Jakub Kicinski <kuba@kernel.org>
> > > Sent: Tuesday, April 21, 2020 10:56
> > > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> > > Cc: davem@davemloft.net; Kubalewski, Arkadiusz
> > > <arkadiusz.kubalewski@intel.com>; netdev@vger.kernel.org;
> > > nhorman@redhat.com; sassmann@redhat.com; Kwapulinski, Piotr
> > > <piotr.kwapulinski@intel.com>; Loktionov, Aleksandr
> > > <aleksandr.loktionov@intel.com>; Bowers, AndrewX
> > > <andrewx.bowers@intel.com>
> > > Subject: Re: [net-next 3/4] i40e: Add support for a new feature:
> > > Total Port Shutdown
> > >
> > > On Mon, 20 Apr 2020 18:49:31 -0700 Jeff Kirsher wrote:
> > > > From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> > > >
> > > > Currently after requesting to down a link on a physical network
> > > > port, the traffic is no longer being processed but the physical
> > > > link with a link partner is still established.
> > > >
> > > > Total Port Shutdown allows to completely shutdown the port on the
> > > > link-down procedure by physically removing the link from the port.
> > > >
> > > > Introduced changes:
> > > > - probe NVM if the feature was enabled at initialization of the
> > > > port
> > > > - special handling on link-down procedure to let FW physically
> > > > shutdown the port if the feature was enabled
> > >
> > > How is this different than link-down-on-close?
> > [Kirsher, Jeffrey T]
> >
> > First of all total-port-shutdown is a read only flag, the user cannot
> > set it from the OS.  It is possible to set it in bios, but only if the
> > motherboard supports it and the NIC has that capability.  Also, the
> > behavior on both slightly differs, link-down-on-close brings the link
> > down by sending (to firmware) phy_type=0, while total-port-shutdown
> > does not, the phy_type is not changed, instead firmware is using
> > I40E_AQ_PHY_ENABLE_LINK flag.
> 
> I see. IOW it's a flag that says the other flag is hard wired to on.
> 
> Why is it important to prevent user from performing the configuration?
> What if an old kernel is run which won't prevent it?
> 
> Let's drill down into what we actually want to express here and then look at the
> API. Michal has already converted ethtool link info to netlink..
[Kirsher, Jeffrey T] 

I know this feature was driven from a customer request/demand where the
link-down-on-close was not sufficient or caused issues.  I want to confirm what
I believe to be the answers to your questions before I respond to them.

> 
> > > Perhaps it'd be good to start documenting the private flags in
> > > Documentation/
> > [Kirsher, Jeffrey T]
> >
> > We could look at adding that information into our kernel
> > documentation, I am planning on updating the driver documentation in a
> > follow-up patch set.  Would a descriptive code comment help in this case?
> 
> Documentation should be sufficient, IMHO, if it's coming soon.
[Kirsher, Jeffrey T] 

It will be for the 5.8 kernel...  Probably won't be next week, will most likely be
1-2 weeks before I get to those changes/patch.