Re: [dpdk-dev] [PATCH v2 04/13] ethdev: change promiscuous callbacks to return status

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

On 9/13/2019 4:54 PM, Andrew Rybchenko wrote:
> On 9/13/19 6:34 PM, Ferruh Yigit wrote:
>> On 9/9/2019 12:58 PM, Andrew Rybchenko wrote:
>>> Enabling/disabling of promiscuous mode is not always successful and
>>> it should be taken into account to be able to handle it properly.
>>>
>>> When correct return status is unclear from driver code, -EAGAIN is used.
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> <...>
>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index b97dd8aa85..f2e6b4c83b 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -1892,30 +1892,38 @@ int
>>>   rte_eth_promiscuous_enable(uint16_t port_id)
>>>   {
>>>   	struct rte_eth_dev *dev;
>>> +	uint8_t old_promiscuous;
>>> +	int diag;
>>>   
>>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>   	dev = &rte_eth_devices[port_id];
>>>   
>>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->promiscuous_enable, -ENOTSUP);
>>> -	(*dev->dev_ops->promiscuous_enable)(dev);
>>> -	dev->data->promiscuous = 1;
>>> +	old_promiscuous = dev->data->promiscuous;
>>> +	diag = (*dev->dev_ops->promiscuous_enable)(dev);
>>> +	dev->data->promiscuous = (diag == 0) ? 1 : old_promiscuous;
>>>   
>>> -	return 0;
>>> +	return eth_err(port_id, diag);
>>>   }
>>>   
>>>   int
>>>   rte_eth_promiscuous_disable(uint16_t port_id)
>>>   {
>>>   	struct rte_eth_dev *dev;
>>> +	uint8_t old_promiscuous;
>>> +	int diag;
>>>   
>>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>   	dev = &rte_eth_devices[port_id];
>>>   
>>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->promiscuous_disable, -ENOTSUP);
>>> +	old_promiscuous = dev->data->promiscuous;
>>>   	dev->data->promiscuous = 0;
>>> -	(*dev->dev_ops->promiscuous_disable)(dev);
>>> +	diag = (*dev->dev_ops->promiscuous_disable)(dev);
>>> +	if (diag != 0)
>>> +		dev->data->promiscuous = old_promiscuous;
>> Not a real issue, but the enable/disable does exact same thing, slightly
>> different way, it makes double check if logic is different,
>> can you adapt one of them for both please.
>>
>> "
>> diag = foo();
>> dev->data->promiscuous = (diag == 0) ? 1 : old_promiscuous;
>> "
>> vs
>>
>> "
>> dev->data->promiscuous = 0;
>> diag = bar();
>> if (diag != 0)
>> 	dev->data->promiscuous = old_promiscuous;
>> "
> 
> I simply did not want to touch the logic in big patch series.
> Don't know why, but enable sets promiscuous=1 after callback,
> but disable does it before callback. That's why the difference.

Ahh, so there is a difference indeed.

> Logically it is a separate change and I definitely don't want to
> mix it.

Ok, fair enough.

> 
>>>   
>>> -	return 0;
>>> +	return eth_err(port_id, diag);
>>>   }
>>>   
>>>   int
>>> diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
>>> index 2394b32c83..6322348d17 100644
>>> --- a/lib/librte_ethdev/rte_ethdev_core.h
>>> +++ b/lib/librte_ethdev/rte_ethdev_core.h
>>> @@ -52,10 +52,10 @@ typedef int (*eth_dev_reset_t)(struct rte_eth_dev *dev);
>>>   typedef int (*eth_is_removed_t)(struct rte_eth_dev *dev);
>>>   /**< @internal Function used to detect an Ethernet device removal. */
>>>   
>>> -typedef void (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev);
>>> +typedef int (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev);
>>>   /**< @internal Function used to enable the RX promiscuous mode of an Ethernet device. */
>>>   
>>> -typedef void (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev);
>>> +typedef int (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev);
>>>   /**< @internal Function used to disable the RX promiscuous mode of an Ethernet device. */
>> Should we add a note what is expected return value? This information is not
>> available anyplace, it may help driver developers.
> 
> Makes sense.
> 
>