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;
"
>
> - 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.
