diff mbox series

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

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

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chengwen Feng Sept. 22, 2022, 7:41 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             | 19 ++++++++++++++++++-
 7 files changed, 30 insertions(+), 1 deletion(-)

Comments

Ferruh Yigit Oct. 3, 2022, 5:35 p.m. UTC | #1
On 9/22/2022 8:41 AM, 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>

<...>

> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index de9e970d4d..930b0a2fff 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1848,6 +1848,19 @@ enum rte_eth_representor_type {
>   	RTE_ETH_REPRESENTOR_PF,   /**< representor of Physical Function. */
>   };
>   
> +/**
> + * Ethernet device error handling mode.

Needs to be experimental, if decides to keep.

> + */
> +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,

Hi Chengwen,

Is the intention of 'PASSIVE' / 'PROACTIVE' mode to let application 
decide which event to register? Like some kind of capability?

If mode == RTE_ETH_ERROR_HANDLE_MODE_PASSIVE
	register RTE_ETH_EVENT_INTR_RESET

if mode == RTE_ETH_ERROR_HANDLE_MODE_PROACTIVE
	register ERR_RECOVERING | RECOVERY_SUCCESS | RECOVERY_FAILED

Can't a PMD support both?
Or is application really needs to know this, what happens if it register 
all events and implements related actions for it?


> +};
> +
>   /**
>    * A structure used to retrieve the contextual information of
>    * an Ethernet device, such as the controlling driver of the
> @@ -1908,8 +1921,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;
>   

I guess 'uint8_t' is used to save space, but 'enum' is mostly integer 
(although as far as I remember compiler can select smaller type is cases 
fit it), so I concern if it case any warning. If not agree to use 
smaller type, since we know possible number of handler type is limited 
and small.

> -	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 */
>   };
>
Chengwen Feng Oct. 5, 2022, 1:56 a.m. UTC | #2
Hi Ferruh,

On 2022/10/4 1:35, Ferruh Yigit wrote:
> On 9/22/2022 8:41 AM, 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>
>
> <...>
>
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index de9e970d4d..930b0a2fff 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -1848,6 +1848,19 @@ enum rte_eth_representor_type {
>>       RTE_ETH_REPRESENTOR_PF,   /**< representor of Physical 
>> Function. */
>>   };
>>   +/**
>> + * Ethernet device error handling mode.
>
> Needs to be experimental, if decides to keep.


will fix in v10


>
>> + */
>> +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,
>
> Hi Chengwen,
>
> Is the intention of 'PASSIVE' / 'PROACTIVE' mode to let application 
> decide which event to register? Like some kind of capability?
>
> If mode == RTE_ETH_ERROR_HANDLE_MODE_PASSIVE
>     register RTE_ETH_EVENT_INTR_RESET
>
> if mode == RTE_ETH_ERROR_HANDLE_MODE_PROACTIVE
>     register ERR_RECOVERING | RECOVERY_SUCCESS | RECOVERY_FAILED
>

It mainly to standardize the two error handling modes to avoid poor 
perception.

In the concept space, the reset mode is separated, so that it is not 
difficult to understand.


> Can't a PMD support both?
Currently, I find that no PMD support both.


If the new PMD supports both the two types, it can be extended well, for 
example, it can be defined as bitmask,

and value should not change because PASSIVE correspond 1(1<<0), and 
PROACTIVE correspond 2(1<<1)


>
> Or is application really needs to know this, what happens if it 
> register all events and implements related actions for it?


For simpler, the application could register all events and do what the 
framework requirements.


>
>
>> +};
>> +
>>   /**
>>    * A structure used to retrieve the contextual information of
>>    * an Ethernet device, such as the controlling driver of the
>> @@ -1908,8 +1921,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;
>
> I guess 'uint8_t' is used to save space, but 'enum' is mostly integer 
> (although as far as I remember compiler can select smaller type is 
> cases fit it), so I concern if it case any warning. If not agree to 
> use smaller type, since we know possible number of handler type is 
> limited and small.


Yes, uint8_t is used to save space. It will depend on compiler if use 
enum here, so I think it's OK to use deterministic type.

as for warning, I have not get such converting warning yeth.


>
>> -    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 */
>>   };
>
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 86054455d2..0c10c663e9 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -922,6 +922,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 a9c18b27e8..dea69c9db1 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 652f0d00a5..b2ef2dc366 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -1178,6 +1178,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 f31bbb7895..7b68b171e6 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 de9e970d4d..930b0a2fff 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -1848,6 +1848,19 @@  enum rte_eth_representor_type {
 	RTE_ETH_REPRESENTOR_PF,   /**< representor of Physical Function. */
 };
 
+/**
+ * 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
@@ -1908,8 +1921,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 */
 };