[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 <arybchenko at 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;
>>>>    }