Re: [dpdk-dev] [RFC] net/null: add empty promiscuous mode functions

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

On 10/17/2019 11:37 AM, Andrew Rybchenko wrote:
> On 10/16/19 9:07 PM, Ferruh Yigit wrote:
>> On 10/16/2019 4:46 PM, Ciara Power wrote:
>>> Adding promiscuous functions prevents sample applications failing when run
>>> on this virtual PMD. The sample applications call promiscuous functions,
>>> and fail if this function call returns an error, which occurs when the
>>> virtual PMD does not support the promiscuous function being called.
>>>
>>> This change will be implemented for all virtual PMDs that currently do not
>>> have existing promiscuous functions. Multicast functions will also be
>>> added for virtual PMDs to prevent sample application breakages here also.
>> +Andrew
>>
>> With the some ethdev APIs returning error code, some sample applications stop
>> working with virtual interfaces,
>>
>> We can,
>> 1- update sample applications to ignore the errors
>> 2- Add dummy dev_ops support to (almost all) virtual PMDs (what this RFC suggests)
>>
>> (1) puts us back to before the ethdev APIs updated status, and this may be wrong
>> for the physical devices case, so I am for this RFC.
>>
>> Only perhaps we can have some common empty function and keep assigning that one
>> to reduce the dummy code, what do you think?
> 
> I don't like the idea to have common empty/dummy functions.
> If virtual PMD behaves in accordance with enabled promiscuous mode,
> it should initialize it properly on init:
>       eth_dev->data->promiscuous = 1;
> If so, if application requires promiscuous mode, attempt to enable will
> do nothing. If application requires non-promiscuous mode, disable will
> fail and it is good.

It is technically correct that we can't disable promiscuous mode in virtual PMDs
but I think mainly we don't really care so it returning error may make the
applications fail/exit unnecessarily with virtual PMDs.

> 
>>> Signed-off-by: Ciara Power <ciara.power@intel.com>
>>> ---
>>>   drivers/net/null/rte_eth_null.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
>>> index e2ff41a22..b8472a0cf 100644
>>> --- a/drivers/net/null/rte_eth_null.c
>>> +++ b/drivers/net/null/rte_eth_null.c
>>> @@ -441,11 +441,25 @@ eth_mac_address_set(__rte_unused struct rte_eth_dev *dev,
>>>   	return 0;
>>>   }
>>>   
>>> +static int
>>> +eth_dev_promiscuous_enable(__rte_unused struct rte_eth_dev *dev)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static int
>>> +eth_dev_promiscuous_disable(__rte_unused struct rte_eth_dev *dev)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>>   static const struct eth_dev_ops ops = {
>>>   	.dev_start = eth_dev_start,
>>>   	.dev_stop = eth_dev_stop,
>>>   	.dev_configure = eth_dev_configure,
>>>   	.dev_infos_get = eth_dev_info,
>>> +	.promiscuous_enable = eth_dev_promiscuous_enable,
>>> +	.promiscuous_disable = eth_dev_promiscuous_disable,
>>>   	.rx_queue_setup = eth_rx_queue_setup,
>>>   	.tx_queue_setup = eth_tx_queue_setup,
>>>   	.rx_queue_release = eth_queue_release,
>>>
>