diff mbox series

net/i40e: fix link speed issue

Message ID 1530508681-50504-1-git-send-email-xiaoyun.li@intel.com (mailing list archive)
State Superseded, archived
Headers show
Series net/i40e: fix link speed issue | expand

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Li, Xiaoyun July 2, 2018, 5:18 a.m. UTC
When link needs to go up, I40E_AQ_PHY_AN_ENABLED is always be set in DPDK.
So all speeds are always set. This causes speed config never works.

This patch fixes this issue and only allows to set available speeds. If
link needs to go up and speed setting is not supported, it will print
warning and set default available speeds. And when link needs to go down,
link speed field should be set to non-zero to avoid link down issue when
binding back to kernel driver.

Fixes: ca7e599d4506 ("net/i40e: fix link management")
Fixes: 1bb8f661168d ("net/i40e: fix link down and negotiation")
Cc: stable@dpdk.org

Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 58 ++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 22 deletions(-)

Comments

Qi Zhang July 10, 2018, 7:57 a.m. UTC | #1
Hi Xiaoyun:

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xiaoyun Li
> Sent: Monday, July 2, 2018 1:18 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] net/i40e: fix link speed issue
> 
> When link needs to go up, I40E_AQ_PHY_AN_ENABLED is always be set in
> DPDK.
> So all speeds are always set. This causes speed config never works.
> 
> This patch fixes this issue and only allows to set available speeds. If link needs
> to go up and speed setting is not supported, it will print warning and set
> default available speeds. And when link needs to go down, link speed field
> should be set to non-zero to avoid link down issue when binding back to kernel
> driver.
> 
> Fixes: ca7e599d4506 ("net/i40e: fix link management")
> Fixes: 1bb8f661168d ("net/i40e: fix link down and negotiation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 58
> ++++++++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 13c5d32..272a975 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -2026,27 +2026,38 @@ i40e_phy_conf_link(struct i40e_hw *hw,
>  	struct i40e_aq_get_phy_abilities_resp phy_ab;
>  	struct i40e_aq_set_phy_config phy_conf;
>  	enum i40e_aq_phy_type cnt;
> +	uint8_t avail_speed;
>  	uint32_t phy_type_mask = 0;
> 
>  	const uint8_t mask = I40E_AQ_PHY_FLAG_PAUSE_TX |
>  			I40E_AQ_PHY_FLAG_PAUSE_RX |
>  			I40E_AQ_PHY_FLAG_PAUSE_RX |
>  			I40E_AQ_PHY_FLAG_LOW_POWER;
> -	const uint8_t advt = I40E_LINK_SPEED_40GB |
> -			I40E_LINK_SPEED_25GB |
> -			I40E_LINK_SPEED_10GB |
> -			I40E_LINK_SPEED_1GB |
> -			I40E_LINK_SPEED_100MB;
>  	int ret = -ENOTSUP;
> 
> +	/* To get phy capabilities of available speeds. */
> +	status = i40e_aq_get_phy_capabilities(hw, false, true, &phy_ab,
> +					      NULL);
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
> +				status);
> +		return ret;
> +	}
> +	avail_speed = phy_ab.link_speed;
> 
> +	/* To get the current phy config. */
>  	status = i40e_aq_get_phy_capabilities(hw, false, false, &phy_ab,
>  					      NULL);
> -	if (status)
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to get the current PHY config: %d\n",
> +				status);
>  		return ret;
> +	}
> 
> -	/* If link already up, no need to set up again */
> -	if (is_up && phy_ab.phy_type != 0)
> +	/* If link needs to go up and its speed values are OK, no need
> +	 * to set up again.
> +	 */
> +	if (is_up && phy_ab.phy_type != 0 && phy_ab.link_speed != 0)
>  		return I40E_SUCCESS;
Why phy_ab.link_speed !=0 means speed values are OK?
What if dev->data->dev_conf->link_speed != ETH_LINK_SPEED_AUTONEG and user want a specific speed?
Should we also add "&& capability & I40E_AQ_PHY_AN_ENABLED" here ?

> 
>  	memset(&phy_conf, 0, sizeof(phy_conf)); @@ -2055,15 +2066,17 @@
> i40e_phy_conf_link(struct i40e_hw *hw,
>  	abilities &= ~mask;
>  	abilities |= phy_ab.abilities & mask;
> 
> -	/* update ablities and speed */
> -	if (abilities & I40E_AQ_PHY_AN_ENABLED)
> -		phy_conf.link_speed = advt;
> -	else
> -		phy_conf.link_speed = is_up ? force_speed : phy_ab.link_speed;
> -
>  	phy_conf.abilities = abilities;
> 
> -
> +	/* If link needs to go up, but the force speed is not supported,
> +	 * Warn users and config the default available speeds.
> +	 */
> +	if (is_up && !(force_speed & avail_speed)) {
> +		PMD_DRV_LOG(WARNING, "Invalid speed setting, set to
> default!\n");
> +		phy_conf.link_speed = avail_speed;
> +	} else {
> +		phy_conf.link_speed = is_up ? force_speed : avail_speed;
> +	}
> 
>  	/* To enable link, phy_type mask needs to include each type */
>  	for (cnt = I40E_PHY_TYPE_SGMII; cnt < I40E_PHY_TYPE_MAX; cnt++) @@
> -2099,6 +2112,14 @@ i40e_apply_link_speed(struct rte_eth_dev *dev)
>  	struct i40e_hw *hw =
> I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  	struct rte_eth_conf *conf = &dev->data->dev_conf;
> 
> +	if (conf->link_speeds == ETH_LINK_SPEED_AUTONEG) {
> +		conf->link_speeds = ETH_LINK_SPEED_40G |
> +				    ETH_LINK_SPEED_25G |
> +				    ETH_LINK_SPEED_20G |
> +				    ETH_LINK_SPEED_10G |
> +				    ETH_LINK_SPEED_1G |
> +				    ETH_LINK_SPEED_100M;
> +	}
>  	speed = i40e_parse_link_speeds(conf->link_speeds);
>  	abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
>  	if (!(conf->link_speeds & ETH_LINK_SPEED_FIXED)) @@ -2220,13 +2241,6
> @@ i40e_dev_start(struct rte_eth_dev *dev)
>  	}
> 
>  	/* Apply link configure */
> -	if (dev->data->dev_conf.link_speeds & ~(ETH_LINK_SPEED_100M |
> -				ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G |
> -				ETH_LINK_SPEED_20G | ETH_LINK_SPEED_25G |
> -				ETH_LINK_SPEED_40G)) {
> -		PMD_DRV_LOG(ERR, "Invalid link setting");
> -		goto err_up;
> -	}
>  	ret = i40e_apply_link_speed(dev);
>  	if (I40E_SUCCESS != ret) {
>  		PMD_DRV_LOG(ERR, "Fail to apply link setting");
> --
> 2.7.4
diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 13c5d32..272a975 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2026,27 +2026,38 @@  i40e_phy_conf_link(struct i40e_hw *hw,
 	struct i40e_aq_get_phy_abilities_resp phy_ab;
 	struct i40e_aq_set_phy_config phy_conf;
 	enum i40e_aq_phy_type cnt;
+	uint8_t avail_speed;
 	uint32_t phy_type_mask = 0;
 
 	const uint8_t mask = I40E_AQ_PHY_FLAG_PAUSE_TX |
 			I40E_AQ_PHY_FLAG_PAUSE_RX |
 			I40E_AQ_PHY_FLAG_PAUSE_RX |
 			I40E_AQ_PHY_FLAG_LOW_POWER;
-	const uint8_t advt = I40E_LINK_SPEED_40GB |
-			I40E_LINK_SPEED_25GB |
-			I40E_LINK_SPEED_10GB |
-			I40E_LINK_SPEED_1GB |
-			I40E_LINK_SPEED_100MB;
 	int ret = -ENOTSUP;
 
+	/* To get phy capabilities of available speeds. */
+	status = i40e_aq_get_phy_capabilities(hw, false, true, &phy_ab,
+					      NULL);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				status);
+		return ret;
+	}
+	avail_speed = phy_ab.link_speed;
 
+	/* To get the current phy config. */
 	status = i40e_aq_get_phy_capabilities(hw, false, false, &phy_ab,
 					      NULL);
-	if (status)
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to get the current PHY config: %d\n",
+				status);
 		return ret;
+	}
 
-	/* If link already up, no need to set up again */
-	if (is_up && phy_ab.phy_type != 0)
+	/* If link needs to go up and its speed values are OK, no need
+	 * to set up again.
+	 */
+	if (is_up && phy_ab.phy_type != 0 && phy_ab.link_speed != 0)
 		return I40E_SUCCESS;
 
 	memset(&phy_conf, 0, sizeof(phy_conf));
@@ -2055,15 +2066,17 @@  i40e_phy_conf_link(struct i40e_hw *hw,
 	abilities &= ~mask;
 	abilities |= phy_ab.abilities & mask;
 
-	/* update ablities and speed */
-	if (abilities & I40E_AQ_PHY_AN_ENABLED)
-		phy_conf.link_speed = advt;
-	else
-		phy_conf.link_speed = is_up ? force_speed : phy_ab.link_speed;
-
 	phy_conf.abilities = abilities;
 
-
+	/* If link needs to go up, but the force speed is not supported,
+	 * Warn users and config the default available speeds.
+	 */
+	if (is_up && !(force_speed & avail_speed)) {
+		PMD_DRV_LOG(WARNING, "Invalid speed setting, set to default!\n");
+		phy_conf.link_speed = avail_speed;
+	} else {
+		phy_conf.link_speed = is_up ? force_speed : avail_speed;
+	}
 
 	/* To enable link, phy_type mask needs to include each type */
 	for (cnt = I40E_PHY_TYPE_SGMII; cnt < I40E_PHY_TYPE_MAX; cnt++)
@@ -2099,6 +2112,14 @@  i40e_apply_link_speed(struct rte_eth_dev *dev)
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct rte_eth_conf *conf = &dev->data->dev_conf;
 
+	if (conf->link_speeds == ETH_LINK_SPEED_AUTONEG) {
+		conf->link_speeds = ETH_LINK_SPEED_40G |
+				    ETH_LINK_SPEED_25G |
+				    ETH_LINK_SPEED_20G |
+				    ETH_LINK_SPEED_10G |
+				    ETH_LINK_SPEED_1G |
+				    ETH_LINK_SPEED_100M;
+	}
 	speed = i40e_parse_link_speeds(conf->link_speeds);
 	abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
 	if (!(conf->link_speeds & ETH_LINK_SPEED_FIXED))
@@ -2220,13 +2241,6 @@  i40e_dev_start(struct rte_eth_dev *dev)
 	}
 
 	/* Apply link configure */
-	if (dev->data->dev_conf.link_speeds & ~(ETH_LINK_SPEED_100M |
-				ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G |
-				ETH_LINK_SPEED_20G | ETH_LINK_SPEED_25G |
-				ETH_LINK_SPEED_40G)) {
-		PMD_DRV_LOG(ERR, "Invalid link setting");
-		goto err_up;
-	}
 	ret = i40e_apply_link_speed(dev);
 	if (I40E_SUCCESS != ret) {
 		PMD_DRV_LOG(ERR, "Fail to apply link setting");