[dpdk-dev] [PATCH v4] ethdev: allow returning error on VLAN offload configuration

[Hemant Agrawal]

On 9/1/2017 6:24 PM, David Harton (dharton) wrote:
>
>
>> -----Original Message-----
>> From: Hemant Agrawal [mailto:hemant.agrawal at nxp.com]
>> Sent: Friday, September 01, 2017 3:41 AM
>> Subject: Re: [PATCH v4] ethdev: allow returning error on VLAN offload
>> configuration
>>
>> On 9/1/2017 8:06 AM, David Harton wrote:
>>> Some devices may not support or fail setting VLAN offload
>>> configuration based on dynamic circurmstances so the
>>> vlan_offload_set_t vector is modified to return an int so the caller
>>> can determine success or not.
>>>
>>> rte_eth_dev_set_vlan_offload is updated to return the value provided
>>> by the vector when called along with restoring the original offload
>>> configs on failure.
>>>
>>> Existing vlan_offload_set_t vectors are modified to return an int.
>>> Majority of cases return 0 but a few that actually can fail now return
>>> their failure codes.
>>>
>>> Finally, a vlan_offload_set_t vector is added to virtio to facilitate
>>> dynamically turning VLAN strip on or off.
>>>
>>> Signed-off-by: David Harton <dharton at cisco.com>
>>> ---

<snip>....

>>> diff --git a/lib/librte_ether/rte_ethdev.c
>> b/lib/librte_ether/rte_ethdev.c
>>> index 0597641..05e52b8 100644
>>> --- a/lib/librte_ether/rte_ethdev.c
>>> +++ b/lib/librte_ether/rte_ethdev.c
>>> @@ -2048,29 +2048,35 @@ struct rte_eth_dev *
>>>  	struct rte_eth_dev *dev;
>>>  	int ret = 0;
>>>  	int mask = 0;
>>> -	int cur, org = 0;
>>> +	int cur, orig = 0;
>>> +	uint8_t orig_strip, orig_filter, orig_extend;
>>>
>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>  	dev = &rte_eth_devices[port_id];
>>>
>>> +	/* save original values in case of failure */
>>> +	orig_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
>>> +	orig_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
>>> +	orig_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
>>> +
>>>  	/*check which option changed by application*/
>>>  	cur = !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD);
>>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
>>> -	if (cur != org) {
>>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
>>> +	if (cur != orig) {
>>>  		dev->data->dev_conf.rxmode.hw_vlan_strip = (uint8_t)cur;
>>>  		mask |= ETH_VLAN_STRIP_MASK;
>>>  	}
>>>
>>>  	cur = !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD);
>>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
>>> -	if (cur != org) {
>>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
>>> +	if (cur != orig) {
>>>  		dev->data->dev_conf.rxmode.hw_vlan_filter = (uint8_t)cur;
>>>  		mask |= ETH_VLAN_FILTER_MASK;
>>>  	}
>>>
>>>  	cur = !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD);
>>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
>>> -	if (cur != org) {
>>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
>>> +	if (cur != orig) {
>>>  		dev->data->dev_conf.rxmode.hw_vlan_extend = (uint8_t)cur;
>>>  		mask |= ETH_VLAN_EXTEND_MASK;
>>>  	}
>>> @@ -2080,7 +2086,13 @@ struct rte_eth_dev *
>>>  		return ret;
>>>
>>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP);
>>> -	(*dev->dev_ops->vlan_offload_set)(dev, mask);
>>> +	ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
>>> +	if (ret) {
>>> +		/* hit an error restore  original values */
>>> +		dev->data->dev_conf.rxmode.hw_vlan_strip = orig_strip;
>>> +		dev->data->dev_conf.rxmode.hw_vlan_filter = orig_filter;
>>> +		dev->data->dev_conf.rxmode.hw_vlan_extend = orig_extend;
>>> +	}
>>>
>> Currently vlan offload is combining three functions:
>> strip, filter and extend.
>>
>> Not all PMDs in DPDK support all of three.
>> Should not the error we extended to reflect, which of the VLAN offload
>> is not supported?
>
> There are many examples of APIs that not all PMDs support.
> There are also other APIs that manipulate many attributes but do
> not communicate which attribute fails on set.
> Solving these issues I believe it outside the scope of this change
> and it requires a larger community discussion.
>
> This proposed change at least adheres to the existing paradigm.

I agree that solving this issue is outside the scope of this patch.

However this patch may leave the drivers in incorrect state.

Earlier this API was not returning error or failing. With this change, 
the driver may want to implement the error handling if 2nd or 3rd 
attribute failed. Note that you are also assuming and reverting back to 
the original condition.

The change is fine w.r.t dpaa2 driver.