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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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