On 9/1/2017 6:24 PM, David Harton (dharton) wrote:
-----Original Message-----
From: Hemant Agrawal [mailto:hemant.agra...@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 <dhar...@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.
