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

[Andrew Rybchenko]

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

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