Re: [Intel-wired-lan] [PATCH iwl-net v2] ixgbevf: fix link speed reporting for Hyper-V E610 VFs

Dear Tomasz,


Thank you for your patch.

Am 04.08.26 um 17:39 schrieb Tomasz Lichwala:
> On Hyper-V E610 VFs the VFLINKS register does not carry valid link speed.
> Instead, link status is exposed through emulated PCI config space at offset
> 0x209 in VFLINKS register format. Read and decode it when checking link on
> E610 VFs.

Is that emulated space documented somewhere? It’d be great if you added 
a reference.

Also, it’d be great if you documented the commands and output without 
and with your patch in the commit message, and mention the Hyper-V 
environment.

> Fixes: 4c44b450c69b ("ixgbevf: Add support for Intel(R) E610 device")
> Reviewed-by: Marcin Szycik <[email protected]>
> Signed-off-by: Tomasz Lichwala <[email protected]>
> ---
>   drivers/net/ethernet/intel/ixgbevf/vf.c | 79 ++++++++++++++++++++++---
>   1 file changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
> index f6df86d124b9..4c460ec62de3 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
> @@ -1,14 +1,17 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /* Copyright(c) 1999 - 2024 Intel Corporation. */
>   
> +#include <linux/unaligned.h>
> +
>   #include "vf.h"
>   #include "ixgbevf.h"
>   
> -/* On Hyper-V, to reset, we need to read from this offset
> - * from the PCI config space. This is the mechanism used on
> - * Hyper-V to support PF/VF communication.
> +/* On Hyper-V the PF/VF communication is through emulated PCI config
> + * space. The reset and link status are exposed at the offsets below.
>    */
> -#define IXGBE_HV_RESET_OFFSET           0x201
> +#define IXGBE_HV_RESET_OFFSET		0x201
> +#define IXGBE_HV_LINK_STATUS_OFFSET	0x209
> +#define IXGBE_HV_LINK_STATUS_SIZE	4
>   
>   static inline s32 ixgbevf_write_msg_read_ack(struct ixgbe_hw *hw, u32 *msg,
>   					     u32 *retmsg, u16 size)
> @@ -901,6 +904,40 @@ static s32 ixgbevf_check_mac_link_vf(struct ixgbe_hw *hw,
>   	return ret_val;
>   }
>   
> +/**
> + * ixgbevf_hv_read_links_e610 - read link status from PCI config space
> + * @hw: pointer to hardware structure
> + * @links_reg: pointer to store read value
> + *
> + * On Hyper-V E610 VFs the VFLINKS register does not carry valid link speed.
> + * Instead, link status is exposed through emulated PCI config space at offset
> + * 0x209 in VFLINKS register format.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static s32 ixgbevf_hv_read_links_e610(struct ixgbe_hw *hw, u32 *links_reg)
> +{
> +	struct ixgbevf_adapter *adapter = hw->back;
> +
> +#if IS_ENABLED(CONFIG_PCI_MMCONFIG)

Can the check be done in C code, and the linker will remove the unused 
stuff? This way everything would be seen by the compiler and compile 
checked.

> +	u8 data[IXGBE_HV_LINK_STATUS_SIZE];
> +
> +	for (int i = 0; i < IXGBE_HV_LINK_STATUS_SIZE; i++) {
> +		int ret = pci_read_config_byte(adapter->pdev,
> +					       IXGBE_HV_LINK_STATUS_OFFSET + i,
> +					       &data[i]);
> +		if (ret)
> +			return pcibios_err_to_errno(ret);
> +	}
> +
> +	*links_reg = get_unaligned_le32(data);
> +	return 0;
> +#else
> +	dev_err_once(&adapter->pdev->dev, "cannot read link status, PCI_MMCONFIG is required for Hyper-V\n");
> +	return -EOPNOTSUPP;
> +#endif
> +}
> +
>   /**
>    * ixgbevf_hv_check_mac_link_vf - check link
>    * @hw: pointer to private hardware struct
> @@ -909,6 +946,7 @@ static s32 ixgbevf_check_mac_link_vf(struct ixgbe_hw *hw,
>    * @autoneg_wait_to_complete: unused
>    *
>    * Hyper-V variant; there is no mailbox communication.
> + * For E610 VFs, link status is read from emulated PCI config space.
>    */
>   static s32 ixgbevf_hv_check_mac_link_vf(struct ixgbe_hw *hw,
>   					ixgbe_link_speed *speed,
> @@ -923,13 +961,30 @@ static s32 ixgbevf_hv_check_mac_link_vf(struct ixgbe_hw *hw,
>   	if (!mbx->ops.check_for_rst(hw) || !mbx->timeout)
>   		mac->get_link_status = true;
>   
> +	/* E610 VFs always read link status from emulated PCI config space
> +	 * because VFLINKS does not carry valid speed for these devices.
> +	 * Skip get_link_status caching since PCI config reads are cheap.
> +	 */
> +	if (mac->type == ixgbe_mac_e610_vf) {
> +		if (ixgbevf_hv_read_links_e610(hw, &links_reg)) {
> +			*link_up = false;
> +			*speed = IXGBE_LINK_SPEED_UNKNOWN;
> +			return 0;
> +		}
> +		goto decode;
> +	}
> +
>   	if (!mac->get_link_status)
>   		goto out;
>   
> -	/* if link status is down no point in checking to see if pf is up */
>   	links_reg = IXGBE_READ_REG(hw, IXGBE_VFLINKS);
> -	if (!(links_reg & IXGBE_LINKS_UP))
> -		goto out;
> +
> +decode:

How would the implementation without goto look like?

> +	if (!(links_reg & IXGBE_LINKS_UP)) {
> +		*link_up = false;
> +		*speed = IXGBE_LINK_SPEED_UNKNOWN;
> +		return 0;
> +	}
>   
>   	/* for SFP+ modules and DA cables on 82599 it can take up to 500usecs
>   	 * before the link status is correct
> @@ -941,8 +996,11 @@ static s32 ixgbevf_hv_check_mac_link_vf(struct ixgbe_hw *hw,
>   			udelay(100);
>   			links_reg = IXGBE_READ_REG(hw, IXGBE_VFLINKS);
>   
> -			if (!(links_reg & IXGBE_LINKS_UP))
> -				goto out;
> +			if (!(links_reg & IXGBE_LINKS_UP)) {
> +				*link_up = false;
> +				*speed = IXGBE_LINK_SPEED_UNKNOWN;
> +				return 0;
> +			}
>   		}
>   	}
>   
> @@ -956,6 +1014,9 @@ static s32 ixgbevf_hv_check_mac_link_vf(struct ixgbe_hw *hw,
>   	case IXGBE_LINKS_SPEED_100_82599:
>   		*speed = IXGBE_LINK_SPEED_100_FULL;
>   		break;
> +	default:
> +		*speed = IXGBE_LINK_SPEED_UNKNOWN;
> +		break;
>   	}
>   
>   	/* if we passed all the tests above then the link is up and we no


Kind regards,

Paul