On 9/13/19 6:34 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/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index b97dd8aa85..f2e6b4c83b 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1892,30 +1892,38 @@ int
rte_eth_promiscuous_enable(uint16_t port_id)
{
struct rte_eth_dev *dev;
+ uint8_t old_promiscuous;
+ int diag;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
dev = &rte_eth_devices[port_id];
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->promiscuous_enable, -ENOTSUP);
- (*dev->dev_ops->promiscuous_enable)(dev);
- dev->data->promiscuous = 1;
+ old_promiscuous = dev->data->promiscuous;
+ diag = (*dev->dev_ops->promiscuous_enable)(dev);
+ dev->data->promiscuous = (diag == 0) ? 1 : old_promiscuous;
- return 0;
+ return eth_err(port_id, diag);
}
int
rte_eth_promiscuous_disable(uint16_t port_id)
{
struct rte_eth_dev *dev;
+ uint8_t old_promiscuous;
+ int diag;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
dev = &rte_eth_devices[port_id];
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->promiscuous_disable, -ENOTSUP);
+ old_promiscuous = dev->data->promiscuous;
dev->data->promiscuous = 0;
- (*dev->dev_ops->promiscuous_disable)(dev);
+ diag = (*dev->dev_ops->promiscuous_disable)(dev);
+ if (diag != 0)
+ dev->data->promiscuous = old_promiscuous;
Not a real issue, but the enable/disable does exact same thing, slightly
different way, it makes double check if logic is different,
can you adapt one of them for both please.
"
diag = foo();
dev->data->promiscuous = (diag == 0) ? 1 : old_promiscuous;
"
vs
"
dev->data->promiscuous = 0;
diag = bar();
if (diag != 0)
dev->data->promiscuous = old_promiscuous;
"
I simply did not want to touch the logic in big patch series.
Don't know why, but enable sets promiscuous=1 after callback,
but disable does it before callback. That's why the difference.
Logically it is a separate change and I definitely don't want to
mix it.
- return 0;
+ return eth_err(port_id, diag);
}
int
diff --git a/lib/librte_ethdev/rte_ethdev_core.h
b/lib/librte_ethdev/rte_ethdev_core.h
index 2394b32c83..6322348d17 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -52,10 +52,10 @@ typedef int (*eth_dev_reset_t)(struct rte_eth_dev *dev);
typedef int (*eth_is_removed_t)(struct rte_eth_dev *dev);
/**< @internal Function used to detect an Ethernet device removal. */
-typedef void (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev);
+typedef int (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev);
/**< @internal Function used to enable the RX promiscuous mode of an Ethernet
device. */
-typedef void (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev);
+typedef int (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev);
/**< @internal Function used to disable the RX promiscuous mode of an
Ethernet device. */
Should we add a note what is expected return value? This information is not
available anyplace, it may help driver developers.
Makes sense.
