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

[Andrew Rybchenko]

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 at 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.
Logically it is a separate change and I definitely don't want to
mix it.

>>   
>> -	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.