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

[fengchengwen <fengchengwen@huawei.com>]

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