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