On 9/16/19 4:22 PM, Ferruh Yigit wrote:
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?
The problem is the following: right now enable function sets
data->promiscuous after driver callback execution, some drivers
definitely use the variable internally and require it to be set before
some driver operations. That's why these drivers set it on entry.
However, it does not mean that no drivers rely on the fact that
data->promiscuous value is set to 1 after callback. Most likely these
drivers are buggy since it is false in the case of configuration restore
on startup, but anyway it is not that simply.
Yes, I would prefer to avoid setting data->promiscuous from driver
code in enable/disable callbacks, but it should be a separate patch
which change it.
Right now:
net/bnxt sets data->promiscuous=1 on configure if Rx mode is VMDQ_DCB
net/mlx4 sets data->promiscuous always on enable/disable entry
net/octeontx sets data->promiscuous before exit from callback and
I think it can be safely removed since API functions definitely do it
before or after callback.
net/softnic sets data->promiscuous=1 on driver register
net/tap sets data->promiscuous=1 in enable/disable hooks before
tap_flow_implicit_create()
net/nfb sets data->promiscuous in driver init
net/octeontx2 sets data->promiscuous in the middle of processing, but
I guess before it is really used
+ 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;
}
