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