diff mbox series

[2/2] net/ice: add basic flow ctrl support for enable/disable/get

Message ID 20220901211350.87631-2-markus.theil@tu-ilmenau.de (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series [1/2] net/ice: add nic blinking support |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/intel-Testing fail Testing issues

Commit Message

Markus Theil Sept. 1, 2022, 9:13 p.m. UTC 
  From: Markus Theil <markus.theil@secunet.com>

Signed-off-by: Markus Theil <markus.theil@secunet.com>
---
 drivers/net/ice/ice_ethdev.c | 108 +++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

Comments

Qi Zhang Sept. 4, 2022, 2:06 a.m. UTC | #1 
> -----Original Message-----
> From: Markus Theil <markus.theil@tu-ilmenau.de>
> Sent: Friday, September 2, 2022 5:14 AM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Theil, Markus <markus.theil@secunet.com>
> Subject: [PATCH 2/2] net/ice: add basic flow ctrl support for enable/disable/get
> 
> From: Markus Theil <markus.theil@secunet.com>

Thanks for contribute this.

As this expose new features of Intel E810 NIC and this is not in Intel's plan on DPDK 22.11. 

To help maintainer to decide if we should accept the patches in 22.11. Could you help to share more background? especially the test cases you performed as well as the test result.

Thanks
Qi

> 
> Signed-off-by: Markus Theil <markus.theil@secunet.com>
> ---
>  drivers/net/ice/ice_ethdev.c | 108 +++++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c index
> f5820d6ccb..ad93ec8155 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -92,6 +92,10 @@ static int ice_dev_set_link_up(struct rte_eth_dev *dev);
> static int ice_dev_set_link_down(struct rte_eth_dev *dev);  static int
> ice_dev_led_on(struct rte_eth_dev *dev);  static int ice_dev_led_off(struct
> rte_eth_dev *dev);
> +static int ice_dev_flow_ctrl_get(struct rte_eth_dev *dev,
> +				 struct rte_eth_fc_conf *fc_conf);
> +static int ice_dev_flow_ctrl_set(struct rte_eth_dev *dev,
> +				 struct rte_eth_fc_conf *fc_conf);
> 
>  static int ice_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);  static int
> ice_vlan_offload_set(struct rte_eth_dev *dev, int mask); @@ -219,6 +223,8
> @@ static const struct eth_dev_ops ice_eth_dev_ops = {
>  	.dev_set_link_down            = ice_dev_set_link_down,
>  	.dev_led_on                   = ice_dev_led_on,
>  	.dev_led_off                  = ice_dev_led_off,
> +	.flow_ctrl_get                = ice_dev_flow_ctrl_get,
> +	.flow_ctrl_set                = ice_dev_flow_ctrl_set,
>  	.rx_queue_start               = ice_rx_queue_start,
>  	.rx_queue_stop                = ice_rx_queue_stop,
>  	.tx_queue_start               = ice_tx_queue_start,
> @@ -4096,6 +4102,108 @@ ice_dev_led_off(struct rte_eth_dev *dev)
>  	return status == ICE_SUCCESS ? 0 : -ENOTSUP;  }
> 
> +static int
> +ice_dev_flow_ctrl_get(struct rte_eth_dev *dev, struct rte_eth_fc_conf
> +*fc_conf) {
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +	struct ice_aqc_get_phy_caps_data *pcaps;
> +	int tx_pause, rx_pause;
> +	int status;
> +
> +	pcaps = (struct ice_aqc_get_phy_caps_data *)
> +		ice_malloc(hw, sizeof(*pcaps));
> +	if (!pcaps)
> +		return -ENOMEM;
> +
> +	status = ice_aq_get_phy_caps(hw->port_info, false,
> +			ICE_AQC_REPORT_ACTIVE_CFG, pcaps, NULL);
> +	if(status)
> +		goto out;
> +
> +	fc_conf->autoneg = ice_is_phy_caps_an_enabled(pcaps);
> +	tx_pause = !!(pcaps->caps & ICE_AQC_PHY_EN_TX_LINK_PAUSE);
> +	rx_pause = !!(pcaps->caps & ICE_AQC_PHY_EN_RX_LINK_PAUSE);
> +
> +	if (rx_pause && tx_pause)
> +		fc_conf->mode = RTE_ETH_FC_FULL;
> +	else if (rx_pause)
> +		fc_conf->mode = RTE_ETH_FC_RX_PAUSE;
> +	else if (tx_pause)
> +		fc_conf->mode = RTE_ETH_FC_TX_PAUSE;
> +	else
> +		fc_conf->mode = RTE_ETH_FC_NONE;
> +
> +out:
> +	ice_free(hw, pcaps);
> +	return status;
> +}
> +
> +static int
> +ice_dev_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf
> +*fc_conf) {
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +	struct ice_port_info *pi = hw->port_info;
> +	struct ice_link_status *ls = &pi->phy.link_info;
> +	bool link_up = !!(ls->link_info & ICE_AQ_LINK_UP);
> +	struct ice_aqc_get_phy_caps_data *pcaps;
> +	u8 aq_failures;
> +	int status;
> +
> +	pcaps = (struct ice_aqc_get_phy_caps_data *)
> +		ice_malloc(hw, sizeof(*pcaps));
> +	if (!pcaps)
> +		return -ENOMEM;
> +
> +	status = ice_aq_get_phy_caps(hw->port_info, false,
> +			ICE_AQC_REPORT_ACTIVE_CFG, pcaps, NULL);
> +	if(status) {
> +		status = -EIO;
> +		goto out;
> +	}
> +	if(fc_conf->autoneg != ice_is_phy_caps_an_enabled(pcaps)) {
> +		status = -ENOTSUP;
> +		goto out;
> +	}
> +
> +	switch(fc_conf->mode) {
> +		case RTE_ETH_FC_FULL:
> +			pi->fc.req_mode = ICE_FC_FULL;
> +			break;
> +		case RTE_ETH_FC_RX_PAUSE:
> +			pi->fc.req_mode = ICE_FC_RX_PAUSE;
> +			break;
> +		case RTE_ETH_FC_TX_PAUSE:
> +			pi->fc.req_mode = ICE_FC_TX_PAUSE;
> +			break;
> +		case RTE_ETH_FC_NONE:
> +			pi->fc.req_mode = ICE_FC_NONE;
> +			break;
> +	}
> +
> +	status = ice_set_fc(pi, &aq_failures, link_up);
> +
> +	if (aq_failures & ICE_SET_FC_AQ_FAIL_GET) {
> +		PMD_DRV_LOG(ERR,
> +			    "port %d fc set failed on get, err %d aq status %i\n",
> +			    dev->data->port_id, status, hw-
> >adminq.sq_last_status);
> +		status = -EIO;
> +	} else if (aq_failures & ICE_SET_FC_AQ_FAIL_SET) {
> +		PMD_DRV_LOG(ERR,
> +			    "port %d fc set failed on set, err %d aq status %i\n",
> +			    dev->data->port_id, status, hw-
> >adminq.sq_last_status);
> +		status = -EIO;
> +	} else if (aq_failures & ICE_SET_FC_AQ_FAIL_UPDATE) {
> +		PMD_DRV_LOG(ERR,
> +			    "port %d fc set failed on update, err %d aq
> status %i\n",
> +			    dev->data->port_id, status, hw-
> >adminq.sq_last_status);
> +		status = -EIO;
> +	}
> +
> +out:
> +	ice_free(hw, pcaps);
> +	return status;
> +}
> +
>  static int
>  ice_mtu_set(struct rte_eth_dev *dev, uint16_t mtu __rte_unused)  {
> --
> 2.37.3
Markus Theil Sept. 14, 2022, 3:49 p.m. UTC | #2 
On 9/4/22 04:06, Zhang, Qi Z wrote:
>
>> -----Original Message-----
>> From: Markus Theil <markus.theil@tu-ilmenau.de>
>> Sent: Friday, September 2, 2022 5:14 AM
>> To: dev@dpdk.org
>> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>; Theil, Markus <markus.theil@secunet.com>
>> Subject: [PATCH 2/2] net/ice: add basic flow ctrl support for enable/disable/get
>>
>> From: Markus Theil <markus.theil@secunet.com>
> Thanks for contribute this.
>
> As this expose new features of Intel E810 NIC and this is not in Intel's plan on DPDK 22.11.
>
> To help maintainer to decide if we should accept the patches in 22.11. Could you help to share more background? especially the test cases you performed as well as the test result.

I only tested, if disabling fc works. After testing this against the 
kernel driver on a multi-port E810 card, I drop the flow control patch 
for now. On my machine with firmware 4.00 I was not able to re-bind the 
port to the kernel ice driver after enabling flow control in DPDK. Even 
the kernel-driven port was not usable after a simple reboot, when I 
enabled flow control. Therefore it seems, like some firmware/NVM work 
has to be also done for enabling this feature.

BR
Markus

> Thanks
> Qi
>
>> Signed-off-by: Markus Theil <markus.theil@secunet.com>
>> ---
>>   drivers/net/ice/ice_ethdev.c | 108 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 108 insertions(+)
>>
>> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c index
>> f5820d6ccb..ad93ec8155 100644
>> --- a/drivers/net/ice/ice_ethdev.c
>> +++ b/drivers/net/ice/ice_ethdev.c
>> @@ -92,6 +92,10 @@ static int ice_dev_set_link_up(struct rte_eth_dev *dev);
>> static int ice_dev_set_link_down(struct rte_eth_dev *dev);  static int
>> ice_dev_led_on(struct rte_eth_dev *dev);  static int ice_dev_led_off(struct
>> rte_eth_dev *dev);
>> +static int ice_dev_flow_ctrl_get(struct rte_eth_dev *dev,
>> +				 struct rte_eth_fc_conf *fc_conf);
>> +static int ice_dev_flow_ctrl_set(struct rte_eth_dev *dev,
>> +				 struct rte_eth_fc_conf *fc_conf);
>>
>>   static int ice_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);  static int
>> ice_vlan_offload_set(struct rte_eth_dev *dev, int mask); @@ -219,6 +223,8
>> @@ static const struct eth_dev_ops ice_eth_dev_ops = {
>>   	.dev_set_link_down            = ice_dev_set_link_down,
>>   	.dev_led_on                   = ice_dev_led_on,
>>   	.dev_led_off                  = ice_dev_led_off,
>> +	.flow_ctrl_get                = ice_dev_flow_ctrl_get,
>> +	.flow_ctrl_set                = ice_dev_flow_ctrl_set,
>>   	.rx_queue_start               = ice_rx_queue_start,
>>   	.rx_queue_stop                = ice_rx_queue_stop,
>>   	.tx_queue_start               = ice_tx_queue_start,
>> @@ -4096,6 +4102,108 @@ ice_dev_led_off(struct rte_eth_dev *dev)
>>   	return status == ICE_SUCCESS ? 0 : -ENOTSUP;  }
>>
>> +static int
>> +ice_dev_flow_ctrl_get(struct rte_eth_dev *dev, struct rte_eth_fc_conf
>> +*fc_conf) {
>> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
>>> dev_private);
>> +	struct ice_aqc_get_phy_caps_data *pcaps;
>> +	int tx_pause, rx_pause;
>> +	int status;
>> +
>> +	pcaps = (struct ice_aqc_get_phy_caps_data *)
>> +		ice_malloc(hw, sizeof(*pcaps));
>> +	if (!pcaps)
>> +		return -ENOMEM;
>> +
>> +	status = ice_aq_get_phy_caps(hw->port_info, false,
>> +			ICE_AQC_REPORT_ACTIVE_CFG, pcaps, NULL);
>> +	if(status)
>> +		goto out;
>> +
>> +	fc_conf->autoneg = ice_is_phy_caps_an_enabled(pcaps);
>> +	tx_pause = !!(pcaps->caps & ICE_AQC_PHY_EN_TX_LINK_PAUSE);
>> +	rx_pause = !!(pcaps->caps & ICE_AQC_PHY_EN_RX_LINK_PAUSE);
>> +
>> +	if (rx_pause && tx_pause)
>> +		fc_conf->mode = RTE_ETH_FC_FULL;
>> +	else if (rx_pause)
>> +		fc_conf->mode = RTE_ETH_FC_RX_PAUSE;
>> +	else if (tx_pause)
>> +		fc_conf->mode = RTE_ETH_FC_TX_PAUSE;
>> +	else
>> +		fc_conf->mode = RTE_ETH_FC_NONE;
>> +
>> +out:
>> +	ice_free(hw, pcaps);
>> +	return status;
>> +}
>> +
>> +static int
>> +ice_dev_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf
>> +*fc_conf) {
>> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
>>> dev_private);
>> +	struct ice_port_info *pi = hw->port_info;
>> +	struct ice_link_status *ls = &pi->phy.link_info;
>> +	bool link_up = !!(ls->link_info & ICE_AQ_LINK_UP);
>> +	struct ice_aqc_get_phy_caps_data *pcaps;
>> +	u8 aq_failures;
>> +	int status;
>> +
>> +	pcaps = (struct ice_aqc_get_phy_caps_data *)
>> +		ice_malloc(hw, sizeof(*pcaps));
>> +	if (!pcaps)
>> +		return -ENOMEM;
>> +
>> +	status = ice_aq_get_phy_caps(hw->port_info, false,
>> +			ICE_AQC_REPORT_ACTIVE_CFG, pcaps, NULL);
>> +	if(status) {
>> +		status = -EIO;
>> +		goto out;
>> +	}
>> +	if(fc_conf->autoneg != ice_is_phy_caps_an_enabled(pcaps)) {
>> +		status = -ENOTSUP;
>> +		goto out;
>> +	}
>> +
>> +	switch(fc_conf->mode) {
>> +		case RTE_ETH_FC_FULL:
>> +			pi->fc.req_mode = ICE_FC_FULL;
>> +			break;
>> +		case RTE_ETH_FC_RX_PAUSE:
>> +			pi->fc.req_mode = ICE_FC_RX_PAUSE;
>> +			break;
>> +		case RTE_ETH_FC_TX_PAUSE:
>> +			pi->fc.req_mode = ICE_FC_TX_PAUSE;
>> +			break;
>> +		case RTE_ETH_FC_NONE:
>> +			pi->fc.req_mode = ICE_FC_NONE;
>> +			break;
>> +	}
>> +
>> +	status = ice_set_fc(pi, &aq_failures, link_up);
>> +
>> +	if (aq_failures & ICE_SET_FC_AQ_FAIL_GET) {
>> +		PMD_DRV_LOG(ERR,
>> +			    "port %d fc set failed on get, err %d aq status %i\n",
>> +			    dev->data->port_id, status, hw-
>>> adminq.sq_last_status);
>> +		status = -EIO;
>> +	} else if (aq_failures & ICE_SET_FC_AQ_FAIL_SET) {
>> +		PMD_DRV_LOG(ERR,
>> +			    "port %d fc set failed on set, err %d aq status %i\n",
>> +			    dev->data->port_id, status, hw-
>>> adminq.sq_last_status);
>> +		status = -EIO;
>> +	} else if (aq_failures & ICE_SET_FC_AQ_FAIL_UPDATE) {
>> +		PMD_DRV_LOG(ERR,
>> +			    "port %d fc set failed on update, err %d aq
>> status %i\n",
>> +			    dev->data->port_id, status, hw-
>>> adminq.sq_last_status);
>> +		status = -EIO;
>> +	}
>> +
>> +out:
>> +	ice_free(hw, pcaps);
>> +	return status;
>> +}
>> +
>>   static int
>>   ice_mtu_set(struct rte_eth_dev *dev, uint16_t mtu __rte_unused)  {
>> --
>> 2.37.3
diff mbox series

Patch

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index f5820d6ccb..ad93ec8155 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -92,6 +92,10 @@  static int ice_dev_set_link_up(struct rte_eth_dev *dev);
 static int ice_dev_set_link_down(struct rte_eth_dev *dev);
 static int ice_dev_led_on(struct rte_eth_dev *dev);
 static int ice_dev_led_off(struct rte_eth_dev *dev);
+static int ice_dev_flow_ctrl_get(struct rte_eth_dev *dev,
+				 struct rte_eth_fc_conf *fc_conf);
+static int ice_dev_flow_ctrl_set(struct rte_eth_dev *dev,
+				 struct rte_eth_fc_conf *fc_conf);
 
 static int ice_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
 static int ice_vlan_offload_set(struct rte_eth_dev *dev, int mask);
@@ -219,6 +223,8 @@  static const struct eth_dev_ops ice_eth_dev_ops = {
 	.dev_set_link_down            = ice_dev_set_link_down,
 	.dev_led_on                   = ice_dev_led_on,
 	.dev_led_off                  = ice_dev_led_off,
+	.flow_ctrl_get                = ice_dev_flow_ctrl_get,
+	.flow_ctrl_set                = ice_dev_flow_ctrl_set,
 	.rx_queue_start               = ice_rx_queue_start,
 	.rx_queue_stop                = ice_rx_queue_stop,
 	.tx_queue_start               = ice_tx_queue_start,
@@ -4096,6 +4102,108 @@  ice_dev_led_off(struct rte_eth_dev *dev)
 	return status == ICE_SUCCESS ? 0 : -ENOTSUP;
 }
 
+static int
+ice_dev_flow_ctrl_get(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_aqc_get_phy_caps_data *pcaps;
+	int tx_pause, rx_pause;
+	int status;
+
+	pcaps = (struct ice_aqc_get_phy_caps_data *)
+		ice_malloc(hw, sizeof(*pcaps));
+	if (!pcaps)
+		return -ENOMEM;
+
+	status = ice_aq_get_phy_caps(hw->port_info, false,
+			ICE_AQC_REPORT_ACTIVE_CFG, pcaps, NULL);
+	if(status)
+		goto out;
+
+	fc_conf->autoneg = ice_is_phy_caps_an_enabled(pcaps);
+	tx_pause = !!(pcaps->caps & ICE_AQC_PHY_EN_TX_LINK_PAUSE);
+	rx_pause = !!(pcaps->caps & ICE_AQC_PHY_EN_RX_LINK_PAUSE);
+
+	if (rx_pause && tx_pause)
+		fc_conf->mode = RTE_ETH_FC_FULL;
+	else if (rx_pause)
+		fc_conf->mode = RTE_ETH_FC_RX_PAUSE;
+	else if (tx_pause)
+		fc_conf->mode = RTE_ETH_FC_TX_PAUSE;
+	else
+		fc_conf->mode = RTE_ETH_FC_NONE;
+
+out:
+	ice_free(hw, pcaps);
+	return status;
+}
+
+static int
+ice_dev_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_port_info *pi = hw->port_info;
+	struct ice_link_status *ls = &pi->phy.link_info;
+	bool link_up = !!(ls->link_info & ICE_AQ_LINK_UP);
+	struct ice_aqc_get_phy_caps_data *pcaps;
+	u8 aq_failures;
+	int status;
+
+	pcaps = (struct ice_aqc_get_phy_caps_data *)
+		ice_malloc(hw, sizeof(*pcaps));
+	if (!pcaps)
+		return -ENOMEM;
+
+	status = ice_aq_get_phy_caps(hw->port_info, false,
+			ICE_AQC_REPORT_ACTIVE_CFG, pcaps, NULL);
+	if(status) {
+		status = -EIO;
+		goto out;
+	}
+	if(fc_conf->autoneg != ice_is_phy_caps_an_enabled(pcaps)) {
+		status = -ENOTSUP;
+		goto out;
+	}
+
+	switch(fc_conf->mode) {
+		case RTE_ETH_FC_FULL:
+			pi->fc.req_mode = ICE_FC_FULL;
+			break;
+		case RTE_ETH_FC_RX_PAUSE:
+			pi->fc.req_mode = ICE_FC_RX_PAUSE;
+			break;
+		case RTE_ETH_FC_TX_PAUSE:
+			pi->fc.req_mode = ICE_FC_TX_PAUSE;
+			break;
+		case RTE_ETH_FC_NONE:
+			pi->fc.req_mode = ICE_FC_NONE;
+			break;
+	}
+
+	status = ice_set_fc(pi, &aq_failures, link_up);
+
+	if (aq_failures & ICE_SET_FC_AQ_FAIL_GET) {
+		PMD_DRV_LOG(ERR,
+			    "port %d fc set failed on get, err %d aq status %i\n",
+			    dev->data->port_id, status, hw->adminq.sq_last_status);
+		status = -EIO;
+	} else if (aq_failures & ICE_SET_FC_AQ_FAIL_SET) {
+		PMD_DRV_LOG(ERR,
+			    "port %d fc set failed on set, err %d aq status %i\n",
+			    dev->data->port_id, status, hw->adminq.sq_last_status);
+		status = -EIO;
+	} else if (aq_failures & ICE_SET_FC_AQ_FAIL_UPDATE) {
+		PMD_DRV_LOG(ERR,
+			    "port %d fc set failed on update, err %d aq status %i\n",
+			    dev->data->port_id, status, hw->adminq.sq_last_status);
+		status = -EIO;
+	}
+
+out:
+	ice_free(hw, pcaps);
+	return status;
+}
+
 static int
 ice_mtu_set(struct rte_eth_dev *dev, uint16_t mtu __rte_unused)
 {