On 9/13/2019 8:57 PM, Andrew Rybchenko wrote:
> On 9/13/19 7:34 PM, Ferruh Yigit wrote:
>> 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?
> 
> Not sure that I understand the question, but tap_ioctl() does not use it.
> So, it is safe to move just before tap_flow_implicit_create().

I think 'dev->data->promiscuous' shouldn't be set be PMD dev_ops, and API
already does it. Is there a specific reason in tap to set this in dev_ops? If
not can we remove setting 'dev->data->promiscuous' from dev_ops?

> 
>>>>> +         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;
>>>>>    }
> 

Reply via email to