Message-ID: <IA3PR11MB8986CC2D4EF48CA3678C668AE53BA@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Thu, 28 Aug 2025 12:22:51 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, Intel Wired LAN
	<intel-wired-lan@...ts.osuosl.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
CC: "Keller, Jacob E" <jacob.e.keller@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH iwl-net v3] i40e: fix Jumbo Frame
 support after iPXE boot



> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf
> Of Jacob Keller
> Sent: Wednesday, August 27, 2025 11:18 PM
> To: Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>; Nguyen,
> Anthony L <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org
> Cc: Keller, Jacob E <jacob.e.keller@...el.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v3] i40e: fix Jumbo Frame
> support after iPXE boot
> 
> The i40e hardware has multiple hardware settings which define the
> Maximum
> Frame Size (MFS) of the physical port. The firmware has an AdminQ
> command
> (0x0603) to configure the MFS, but the i40e Linux driver never
> issues this
> command.
> 
> In most cases this is no problem, as the NVM default value has the
> device
> configured for its maximum value of 9728. Unfortunately, recent
> versions of
> the iPXE intelxl driver now issue the 0x0603 Set Mac Config command,
> modifying the MFS and reducing it from its default value of 9728.
> 
> This occurred as part of iPXE commit 6871a7de705b ("[intelxl] Use
> admin
> queue to set port MAC address and maximum frame size"), a
> prerequisite
> change for supporting the E800 series hardware in iPXE. Both the
> E700 and
> E800 firmware support the AdminQ command, and the iPXE code shares
> much of
> the logic between the two device drivers.
> 
> The ice E800 Linux driver already issues the 0x0603 Set Mac Config
> command
> early during probe, and is thus unaffected by the iPXE change.
> 
> Since commit 3a2c6ced90e1 ("i40e: Add a check to see if MFS is
> set"), the
> i40e driver does check the I40E_PRTGL_SAH register, but it only logs
> a
> warning message if its value is below the 9728 default. This
> register also
> only covers received packets and not transmitted packets. A warning
> can
> inform system administrators, but does not correct the issue. No
> interactions from userspace cause the driver to write to PRTGL_SAH
> or issue
> the 0x0603 AdminQ command. Only a GLOBR reset will restore the value
> to its
> default value. There is no obvious method to trigger a GLOBR reset
> from
> user space.
> 
> To fix this, introduce the i40e_aq_set_mac_config() function,
> similar to
> the one from the ice driver. Call this during early probe to ensure
> that
> the device configuration matches driver expectation. Unlike E800,
> the E700
> firmware also has a bit to control whether the MAC should append CRC
> data.
> It is on by default, but setting a 0 to this bit would disable CRC.
> The
> i40e implementation must set this bit to ensure CRC will be appended
> by the
> MAC.
> 
> In addition to the AQ command, instead of just checking the
> I40E_PRTGL_SAH
> register, update its value to the 9728 default and write it back.
> This
> ensures that the hardware is in the expected state, regardless of
> whether
> the iPXE (or any other early boot driver) has modified this state.
> 
> This is a better user experience, as we now fix the issues with
> larger MTU
> instead of merely warning. It also aligns with the way the ice E800
> series
> driver works.
> 
> A final note: The Fixes tag provided here is not strictly accurate.
> The
> issue occurs as a result of an external entity (the iPXE intelxl
> driver),
> and this is not a regression specifically caused by the mentioned
> change.
> However, I believe the original change to just warn about PRTGL_SAH
> being
> too low was an insufficient fix.
> 
> Fixes: 3a2c6ced90e1 ("i40e: Add a check to see if MFS is set")
> Link:
> https://github.com/ipxe/ipxe/commit/6871a7de705b6f6a4046f0d19da9bcd6
> 89c3bc8e
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> ---
> Changes in v3:
> - Don't disable CRC. Otherwise, Tx traffic will not be accepted
>   appropriately.
> - Link to v2: https://lore.kernel.org/r/20250815-jk-fix-i40e-ice-
> pxe-9k-mtu-v2-1-ce857cdc6488@...el.com
> 
> Changes in v2:
> - Rewrite commit message with feedback from Paul Menzel.
> - Add missing initialization of cmd via libie_aq_raw().
> - Fix the Kdoc comment for i40e_aq_set_mac_config().
> - Move clarification of the Fixes tag to the commit message.
> - Link to v1: https://lore.kernel.org/r/20250814-jk-fix-i40e-ice-
> pxe-9k-mtu-v1-1-c3926287fb78@...el.com
> ---
>  drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h |  1 +
>  drivers/net/ethernet/intel/i40e/i40e_prototype.h  |  2 ++
>  drivers/net/ethernet/intel/i40e/i40e_common.c     | 34
> +++++++++++++++++++++++
>  drivers/net/ethernet/intel/i40e/i40e_main.c       | 17 ++++++++----
>  4 files changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> index 76d872b91a38..cc02a85ad42b 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> @@ -1561,6 +1561,7 @@ I40E_CHECK_CMD_LENGTH(i40e_aq_set_phy_config);
>  struct i40e_aq_set_mac_config {
>  	__le16	max_frame_size;
>  	u8	params;
> +#define I40E_AQ_SET_MAC_CONFIG_CRC_EN	BIT(2)
>  	u8	tx_timer_priority; /* bitmap */
>  	__le16	tx_timer_value;
>  	__le16	fc_refresh_threshold;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> index aef5de53ce3b..26bb7bffe361 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> @@ -98,6 +98,8 @@ int i40e_aq_set_mac_loopback(struct i40e_hw *hw,
>  			     struct i40e_asq_cmd_details *cmd_details);
>  int i40e_aq_set_phy_int_mask(struct i40e_hw *hw, u16 mask,
>  			     struct i40e_asq_cmd_details *cmd_details);
> +int i40e_aq_set_mac_config(struct i40e_hw *hw, u16 max_frame_size,
> +			   struct i40e_asq_cmd_details *cmd_details);
>  int i40e_aq_clear_pxe_mode(struct i40e_hw *hw,
>  			   struct i40e_asq_cmd_details *cmd_details);
>  int i40e_aq_set_link_restart_an(struct i40e_hw *hw,
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
> b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 270e7e8cf9cf..59f5c1e810eb 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -1189,6 +1189,40 @@ int i40e_set_fc(struct i40e_hw *hw, u8
> *aq_failures,
>  	return status;
>  }
> 
> +/**
> + * i40e_aq_set_mac_config - Configure MAC settings
> + * @hw: pointer to the hw struct
> + * @max_frame_size: Maximum Frame Size to be supported by the port
> + * @cmd_details: pointer to command details structure or NULL
> + *
> + * Set MAC configuration (0x0603). Note that max_frame_size must be
> greater
> + * than zero.
> + *
> + * Return: 0 on success, or a negative error code on failure.
> + */
> +int i40e_aq_set_mac_config(struct i40e_hw *hw, u16 max_frame_size,
> +			   struct i40e_asq_cmd_details *cmd_details)
> +{
> +	struct i40e_aq_set_mac_config *cmd;
> +	struct libie_aq_desc desc;
> +
> +	cmd = libie_aq_raw(&desc);
> +
> +	if (max_frame_size == 0)
> +		return -EINVAL;
> +
> +	i40e_fill_default_direct_cmd_desc(&desc,
> i40e_aqc_opc_set_mac_config);
> +
> +	cmd->max_frame_size = cpu_to_le16(max_frame_size);
> +	cmd->params = I40E_AQ_SET_MAC_CONFIG_CRC_EN;
> +
> +#define I40E_AQ_SET_MAC_CONFIG_FC_DEFAULT_THRESHOLD	0x7FFF
> +	cmd->fc_refresh_threshold =
> +
> 	cpu_to_le16(I40E_AQ_SET_MAC_CONFIG_FC_DEFAULT_THRESHOLD);
> +
> +	return i40e_asq_send_command(hw, &desc, NULL, 0,
> cmd_details);
> +}
> +
>  /**
>   * i40e_aq_clear_pxe_mode
>   * @hw: pointer to the hw struct
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b83f823e4917..4796fdd0b966 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -16045,13 +16045,18 @@ static int i40e_probe(struct pci_dev
> *pdev, const struct pci_device_id *ent)
>  		dev_dbg(&pf->pdev->dev, "get supported phy types ret =
> %pe last_status =  %s\n",
>  			ERR_PTR(err), libie_aq_str(pf-
> >hw.aq.asq_last_status));
> 
> -	/* make sure the MFS hasn't been set lower than the default
> */
>  #define MAX_FRAME_SIZE_DEFAULT 0x2600
Can you consider re-name MAX_FRAME_SIZE_DEFAULT into I40E_MAX_FRAME_SIZE_DEFAULT ?

> -	val = FIELD_GET(I40E_PRTGL_SAH_MFS_MASK,
> -			rd32(&pf->hw, I40E_PRTGL_SAH));
> -	if (val < MAX_FRAME_SIZE_DEFAULT)
> -		dev_warn(&pdev->dev, "MFS for port %x (%d) has been set
> below the default (%d)\n",
> -			 pf->hw.port, val, MAX_FRAME_SIZE_DEFAULT);
> +
> +	err = i40e_aq_set_mac_config(hw, MAX_FRAME_SIZE_DEFAULT,
> NULL);
Are you sure it's safe to use hw var here? Because old code used pf->hw.

> +	if (err) {
> +		dev_warn(&pdev->dev, "set mac config ret =  %pe
> last_status =  %s\n",
> +			 ERR_PTR(err), libie_aq_str(pf-
> >hw.aq.asq_last_status));
> +	}
> +
> +	/* Make sure the MFS is set to the expected value */
> +	val = rd32(hw, I40E_PRTGL_SAH);
> +	FIELD_MODIFY(I40E_PRTGL_SAH_MFS_MASK, &val,
> MAX_FRAME_SIZE_DEFAULT);
> +	wr32(hw, I40E_PRTGL_SAH, 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
> 
> ---
> base-commit: ceb9515524046252c522b16f38881e8837ec0d91
> change-id: 20250813-jk-fix-i40e-ice-pxe-9k-mtu-2b6d03621cd9
> 
> Best regards,
> --
> Jacob Keller <jacob.e.keller@...el.com>

Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>

Powered by blists - more mailing lists