[v11,1/5] ethdev: support get port error handling mode

Message ID 20221009091009.38978-2-fengchengwen@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series support error handling mode |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

fengchengwen Oct. 9, 2022, 9:10 a.m. UTC
  This patch support gets port's error handling mode by
rte_eth_dev_info_get() API.

Currently, the defined modes include:
1) NONE: it means no error handling modes are supported by this port.
2) PASSIVE: passive error handling, after the PMD detect that a reset
is required, the PMD reports RTE_ETH_EVENT_INTR_RESET event, and
application invoke rte_eth_dev_reset() to recover the port.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test-pmd/config.c               |  2 ++
 drivers/net/e1000/igb_ethdev.c      |  2 ++
 drivers/net/ena/ena_ethdev.c        |  2 ++
 drivers/net/iavf/iavf_ethdev.c      |  2 ++
 drivers/net/ixgbe/ixgbe_ethdev.c    |  2 ++
 drivers/net/txgbe/txgbe_ethdev_vf.c |  2 ++
 lib/ethdev/rte_ethdev.h             | 22 +++++++++++++++++++++-
 7 files changed, 33 insertions(+), 1 deletion(-)
  

Comments

Andrew Rybchenko Oct. 10, 2022, 8:38 a.m. UTC | #1
On 10/9/22 12:10, Chengwen Feng wrote:
> This patch support gets port's error handling mode by
> rte_eth_dev_info_get() API.

Just: "Add error handling mode to device info."

> 
> Currently, the defined modes include:
> 1) NONE: it means no error handling modes are supported by this port.
> 2) PASSIVE: passive error handling, after the PMD detect that a reset
> is required, the PMD reports RTE_ETH_EVENT_INTR_RESET event, and
> application invoke rte_eth_dev_reset() to recover the port.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

With review notes applied (may be except usage of reserved
fields):
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index e8d1e1c658..3443bf20e1 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1629,6 +1629,22 @@ enum rte_eth_representor_type {
>   	RTE_ETH_REPRESENTOR_PF,   /**< representor of Physical Function. */
>   };
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this enumeration may change without prior notice.
> + *
> + * Ethernet device error handling mode.
> + */
> +enum rte_eth_err_handle_mode {
> +	/** No error handling modes are supported. */
> +	RTE_ETH_ERROR_HANDLE_MODE_NONE,
> +	/** Passive error handling, after the PMD detect that a reset is
> +	 * required, the PMD reports @see RTE_ETH_EVENT_INTR_RESET event, and
> +	 * application invoke @see rte_eth_dev_reset to recover the port.
> +	 */
> +	RTE_ETH_ERROR_HANDLE_MODE_PASSIVE,
> +};
> +
>   /**
>    * A structure used to retrieve the contextual information of
>    * an Ethernet device, such as the controlling driver of the
> @@ -1689,8 +1705,12 @@ struct rte_eth_dev_info {
>   	 * embedded managed interconnect/switch.
>   	 */
>   	struct rte_eth_switch_info switch_info;
> +	/** Supported error handling mode. @see enum rte_eth_err_handle_mode */
> +	uint8_t err_handle_mode;

IMHO, it must be
     enum rte_eth_err_handle_mode err_handle_mode;
Yes, it takes a bit more space, but it is a control path and
code clearness is more important here than few extra bytes.

>   
> -	uint64_t reserved_64s[2]; /**< Reserved for future fields */
> +	uint8_t reserved_8;       /**< Reserved for future fields  */
> +	uint16_t reserved_16s[3]; /**< Reserved for future fields  */
> +	uint64_t reserved_64;     /**< Reserved for future fields */

As far as I know it is done as per Stephen review notes, but
I'm not really sure why it is a right way in ABI breaking
release. I'd not touch it and just add a new field.

>   	void *reserved_ptrs[2];   /**< Reserved for future fields */
>   };
>
  
Andrew Rybchenko Oct. 10, 2022, 8:44 a.m. UTC | #2
On 10/9/22 12:10, Chengwen Feng wrote:
> This patch support gets port's error handling mode by
> rte_eth_dev_info_get() API.
> 
> Currently, the defined modes include:
> 1) NONE: it means no error handling modes are supported by this port.
> 2) PASSIVE: passive error handling, after the PMD detect that a reset
> is required, the PMD reports RTE_ETH_EVENT_INTR_RESET event, and
> application invoke rte_eth_dev_reset() to recover the port.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

in fact one more point below

> ---
>   app/test-pmd/config.c               |  2 ++
>   drivers/net/e1000/igb_ethdev.c      |  2 ++
>   drivers/net/ena/ena_ethdev.c        |  2 ++
>   drivers/net/iavf/iavf_ethdev.c      |  2 ++
>   drivers/net/ixgbe/ixgbe_ethdev.c    |  2 ++
>   drivers/net/txgbe/txgbe_ethdev_vf.c |  2 ++
>   lib/ethdev/rte_ethdev.h             | 22 +++++++++++++++++++++-
>   7 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 841e8efe78..bd7f2ba257 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -921,6 +921,8 @@ port_infos_display(portid_t port_id)
>   			printf("Switch Rx domain: %u\n",
>   			       dev_info.switch_info.rx_domain);
>   	}
> +	if (dev_info.err_handle_mode == RTE_ETH_ERROR_HANDLE_MODE_PASSIVE)
> +		printf("Device error handling mode: passive\n");

It should be done using switch/case instead of if/elseif.
Also I'd say that none should be handled as well.

>   }
>   
>   void
  

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 841e8efe78..bd7f2ba257 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -921,6 +921,8 @@  port_infos_display(portid_t port_id)
 			printf("Switch Rx domain: %u\n",
 			       dev_info.switch_info.rx_domain);
 	}
+	if (dev_info.err_handle_mode == RTE_ETH_ERROR_HANDLE_MODE_PASSIVE)
+		printf("Device error handling mode: passive\n");
 }
 
 void
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index d6bcc5bf58..8858f975f8 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -2341,6 +2341,8 @@  eth_igbvf_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->rx_desc_lim = rx_desc_lim;
 	dev_info->tx_desc_lim = tx_desc_lim;
 
+	dev_info->err_handle_mode = RTE_ETH_ERROR_HANDLE_MODE_PASSIVE;
+
 	return 0;
 }
 
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 3e88bcda6c..efcb163027 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -2482,6 +2482,8 @@  static int ena_infos_get(struct rte_eth_dev *dev,
 	dev_info->default_rxportconf.ring_size = ENA_DEFAULT_RING_SIZE;
 	dev_info->default_txportconf.ring_size = ENA_DEFAULT_RING_SIZE;
 
+	dev_info->err_handle_mode = RTE_ETH_ERROR_HANDLE_MODE_PASSIVE;
+
 	return 0;
 }
 
diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index 782be82c7f..b1958e0474 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -1179,6 +1179,8 @@  iavf_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		.nb_align = IAVF_ALIGN_RING_DESC,
 	};
 
+	dev_info->err_handle_mode = RTE_ETH_ERROR_HANDLE_MODE_PASSIVE;
+
 	return 0;
 }
 
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index bf70ee041d..fd06ddbe35 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4056,6 +4056,8 @@  ixgbevf_dev_info_get(struct rte_eth_dev *dev,
 	dev_info->rx_desc_lim = rx_desc_lim;
 	dev_info->tx_desc_lim = tx_desc_lim;
 
+	dev_info->err_handle_mode = RTE_ETH_ERROR_HANDLE_MODE_PASSIVE;
+
 	return 0;
 }
 
diff --git a/drivers/net/txgbe/txgbe_ethdev_vf.c b/drivers/net/txgbe/txgbe_ethdev_vf.c
index f52cd8bc19..3b1f7c913b 100644
--- a/drivers/net/txgbe/txgbe_ethdev_vf.c
+++ b/drivers/net/txgbe/txgbe_ethdev_vf.c
@@ -521,6 +521,8 @@  txgbevf_dev_info_get(struct rte_eth_dev *dev,
 	dev_info->rx_desc_lim = rx_desc_lim;
 	dev_info->tx_desc_lim = tx_desc_lim;
 
+	dev_info->err_handle_mode = RTE_ETH_ERROR_HANDLE_MODE_PASSIVE;
+
 	return 0;
 }
 
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index e8d1e1c658..3443bf20e1 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -1629,6 +1629,22 @@  enum rte_eth_representor_type {
 	RTE_ETH_REPRESENTOR_PF,   /**< representor of Physical Function. */
 };
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this enumeration may change without prior notice.
+ *
+ * Ethernet device error handling mode.
+ */
+enum rte_eth_err_handle_mode {
+	/** No error handling modes are supported. */
+	RTE_ETH_ERROR_HANDLE_MODE_NONE,
+	/** Passive error handling, after the PMD detect that a reset is
+	 * required, the PMD reports @see RTE_ETH_EVENT_INTR_RESET event, and
+	 * application invoke @see rte_eth_dev_reset to recover the port.
+	 */
+	RTE_ETH_ERROR_HANDLE_MODE_PASSIVE,
+};
+
 /**
  * A structure used to retrieve the contextual information of
  * an Ethernet device, such as the controlling driver of the
@@ -1689,8 +1705,12 @@  struct rte_eth_dev_info {
 	 * embedded managed interconnect/switch.
 	 */
 	struct rte_eth_switch_info switch_info;
+	/** Supported error handling mode. @see enum rte_eth_err_handle_mode */
+	uint8_t err_handle_mode;
 
-	uint64_t reserved_64s[2]; /**< Reserved for future fields */
+	uint8_t reserved_8;       /**< Reserved for future fields  */
+	uint16_t reserved_16s[3]; /**< Reserved for future fields  */
+	uint64_t reserved_64;     /**< Reserved for future fields */
 	void *reserved_ptrs[2];   /**< Reserved for future fields */
 };