Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device

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

On 5/11/2022 3:16 AM, Min Hu (Connor) wrote:

<...>
>>>>>> @@ -609,8 +658,25 @@ eth_dev_start_mp(uint16_t port_id)
>>>>>>   static int
>>>>>>   eth_dev_stop_mp(uint16_t port_id)
>>>>>>   {
>>>>>> -    if (is_proc_primary())
>>>>>> -        return rte_eth_dev_stop(port_id);
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (is_proc_primary()) {
>>>>>> +        ret = rte_eth_dev_stop(port_id);
>>>>>> +        if (ret != 0)
>>>>>> +            return ret;
>>>>>> +
>>>>>> +#ifdef RTE_NET_BOND
>>>>>
>>>>> Here and in other places - probably no need to pollute the code
>>>>> with all these 'ifdef RTE_NET_BOND'.
>>>>> I suppose this logic (for checking is bonding API present or not)
>>>>> can be hidden inside change_bonding_slave_port_status() itself.
>>>>>
>>>> I think it does not pollute the code. anyone can tell according to
>>>> the flag 'ifdef RTE_NET_BOND'.
>>>> if hiddle inside 'change_bonding_slave_port_status', it will be weird.
>>>> For example, if the port is not bonding port, It will also invoke 
>>>> 'change_bonding_slave_port_status'. That is unreasonable.
>>>>
>>>
>>> Hi Konstantin,
>>>
>>> I also was not happy to have bonding (or any PMD) ifdef in the 
>>> generic (start()/stop()) functions, but the ifdef blocks updates 
>>> testpmd (application) level status. So that can't be handled in the 
>>> PMD and need to be in the application level.
>>> Which is enforcing to have same PMD specific code in the testpmd level,
>>> if you have any suggestion to prevent this, I am for it.
>>
>>
>> I am not aking to move it to PMD.
>> What I am saying that this ifdef logic could be grouped in one place
>> (inside change_bonding_slave_port_status()) instead of spreading it
>> around multiple places.
>>
 >
 > Hi, Konstantin,
 >      fixed in v4, thanks.

Hi Conor,

What do you think to apply same on patch 4/5?