On 9/13/2019 5:05 PM, Andrew Rybchenko wrote:
> On 9/13/19 6:39 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/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index f85458c3cd..41612ce838 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -1100,28 +1100,60 @@ tap_link_update(struct rte_eth_dev *dev, int
>>> wait_to_complete __rte_unused)
>>> return 0;
>>> }
>>>
>>> -static void
>>> +static int
>>> tap_promisc_enable(struct rte_eth_dev *dev)
>>> {
>>> struct pmd_internals *pmd = dev->data->dev_private;
>>> struct ifreq ifr = { .ifr_flags = IFF_PROMISC };
>>> + int ret;
>>>
>>> - dev->data->promiscuous = 1;
>>> - tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
>>> - if (pmd->remote_if_index && !pmd->flow_isolate)
>>> - tap_flow_implicit_create(pmd, TAP_REMOTE_PROMISC);
>>> + ret = tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
>>> + if (ret != 0)
>>> + return ret;
>>> +
>>> + if (pmd->remote_if_index && !pmd->flow_isolate) {
>>> + dev->data->promiscuous = 1;
>> I think PMD shouldn't be setting this variable, it is already set by the API.
>> I quickly checked if an internal function requires this but it looks like it
>> has
>> been set by mistake, I think we can remove this.
>
> It is set after callback in the case of enable.
I see, but do we need it enabled earlier?
>
>>> + ret = tap_flow_implicit_create(pmd, TAP_REMOTE_PROMISC);
>>> + if (ret != 0) {
>>> + /* Rollback promisc flag */
>>> + tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE);
>>> + /*
>>> + * rte_eth_dev_promiscuous_enable() rollback
>>> + * dev->data->promiscuous in the case of failure.
>>> + */
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> }
>>>
>>> -static void
>>> +static int
>>> tap_promisc_disable(struct rte_eth_dev *dev)
>>> {
>>> struct pmd_internals *pmd = dev->data->dev_private;
>>> struct ifreq ifr = { .ifr_flags = IFF_PROMISC };
>>> + int ret;
>>>
>>> - dev->data->promiscuous = 0;
>>> - tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE);
>>> - if (pmd->remote_if_index && !pmd->flow_isolate)
>>> - tap_flow_implicit_destroy(pmd, TAP_REMOTE_PROMISC);
>>> + ret = tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE);
>>> + if (ret != 0)
>>> + return ret;
>>> +
>>> + if (pmd->remote_if_index && !pmd->flow_isolate) {
>>> + dev->data->promiscuous = 0;
>> Ditto
>>
>>> + ret = tap_flow_implicit_destroy(pmd, TAP_REMOTE_PROMISC);
>>> + if (ret != 0) {
>>> + /* Rollback promisc flag */
>>> + tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
>>> + /*
>>> + * rte_eth_dev_promiscuous_disable() rollback
>>> + * dev->data->promiscuous in the case of failure.
>>> + */
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> }
>
>
