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