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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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