[PATCH] ethdev: keep promiscuous/allmulti value before enabling

[PATCH] ethdev: keep promiscuous/allmulti value before enabling

[Sunyang Wu]

The values of the promiscuous and allmulticast variables
are set after calling the driver, according to the return value.

Fixes: 400d75818266 ("ethdev: check device promiscuous state")
de5ccf0775ae ("ethdev:do nothing if all-multicast mode is applied
again")
Cc: stable@dpdk.org

Signed-off-by: Sunyang Wu <sunyang.wu@jaguarmicro.com>
---
 lib/ethdev/rte_ethdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index dd7c00bc94..41f96071e2 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -3018,7 +3018,8 @@ rte_eth_promiscuous_enable(uint16_t port_id)
 		return -ENOTSUP;
 
 	diag = dev->dev_ops->promiscuous_enable(dev);
-	dev->data->promiscuous = (diag == 0) ? 1 : 0;
+	if (diag == 0)
+		dev->data->promiscuous = 1;
 
 	diag = eth_err(port_id, diag);
 
@@ -3086,7 +3087,8 @@ rte_eth_allmulticast_enable(uint16_t port_id)
 	if (dev->dev_ops->allmulticast_enable == NULL)
 		return -ENOTSUP;
 	diag = dev->dev_ops->allmulticast_enable(dev);
-	dev->data->all_multicast = (diag == 0) ? 1 : 0;
+	if (diag == 0)
+		dev->data->all_multicast = 1;
 
 	diag = eth_err(port_id, diag);
 
-- 
2.19.0.rc0.windows.1

Re: [PATCH] ethdev: keep promiscuous/allmulti value before enabling

[Thomas Monjalon]

21/07/2025 13:51, Sunyang Wu:
> The values of the promiscuous and allmulticast variables
> are set after calling the driver, according to the return value.
> 
> Fixes: 400d75818266 ("ethdev: check device promiscuous state")
> de5ccf0775ae ("ethdev:do nothing if all-multicast mode is applied
> again")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Sunyang Wu <sunyang.wu@jaguarmicro.com>
[...]
>  	diag = dev->dev_ops->promiscuous_enable(dev);
> -	dev->data->promiscuous = (diag == 0) ? 1 : 0;
> +	if (diag == 0)
> +		dev->data->promiscuous = 1;

I remember seeing this strange behavior of resetting the value if failed.
And it is done differently in the "disable" functions.

But it is not so wrong, because if it was enabled, the function returns early.
So the value changes only if it is successful.

What is the issue you observe?
Is it a rework to make the code easier to understand?

回复: [PATCH] ethdev: keep promiscuous/allmulti value before enabling

[Sunyang Wu]

Hi, Thomas:
Thanks for your note. The main purpose of this modification is to align the handling logic with the "disable" functions,
aiming to enhance the overall consistency and maintainability of the code.

Previously, the handling of failure scenarios in the "enable" related logic differed from that in the "disable" functions. 
With this adjustment, both will behave more uniformly in exceptional cases, which should reduce potential confusion
during future development or maintenance caused by inconsistent logic.

Best regards,
Sunyang

21/07/2025 13:51, Sunyang Wu:
> The values of the promiscuous and allmulticast variables are set after 
> calling the driver, according to the return value.
>
> Fixes: 400d75818266 ("ethdev: check device promiscuous state") 
> de5ccf0775ae ("ethdev:do nothing if all-multicast mode is applied
> again")
> Cc: stable@dpdk.org
>
> Signed-off-by: Sunyang Wu <sunyang.wu@jaguarmicro.com>
[...]
>       diag = dev->dev_ops->promiscuous_enable(dev);
> -     dev->data->promiscuous = (diag == 0) ? 1 : 0;
> +     if (diag == 0)
> +             dev->data->promiscuous = 1;

I remember seeing this strange behavior of resetting the value if failed.
And it is done differently in the "disable" functions.

But it is not so wrong, because if it was enabled, the function returns early.
So the value changes only if it is successful.

What is the issue you observe?
Is it a rework to make the code easier to understand?

Re: 回复: [PATCH] ethdev: keep promiscuous/allmulti value before enabling

[Thomas Monjalon]

21/07/2025 14:13, Sunyang Wu:
> Hi, Thomas:
> Thanks for your note. The main purpose of this modification is to align the handling logic with the "disable" functions,
> aiming to enhance the overall consistency and maintainability of the code.
> 
> Previously, the handling of failure scenarios in the "enable" related logic differed from that in the "disable" functions. 
> With this adjustment, both will behave more uniformly in exceptional cases, which should reduce potential confusion
> during future development or maintenance caused by inconsistent logic.

In this case, please send a v2 explaining the intent is to align with disable functions,
and saying there is no behavior change.


> 21/07/2025 13:51, Sunyang Wu:
> > The values of the promiscuous and allmulticast variables are set after 
> > calling the driver, according to the return value.
> >
> > Fixes: 400d75818266 ("ethdev: check device promiscuous state") 
> > de5ccf0775ae ("ethdev:do nothing if all-multicast mode is applied
> > again")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Sunyang Wu <sunyang.wu@jaguarmicro.com>
> [...]
> >       diag = dev->dev_ops->promiscuous_enable(dev);
> > -     dev->data->promiscuous = (diag == 0) ? 1 : 0;
> > +     if (diag == 0)
> > +             dev->data->promiscuous = 1;
> 
> I remember seeing this strange behavior of resetting the value if failed.
> And it is done differently in the "disable" functions.
> 
> But it is not so wrong, because if it was enabled, the function returns early.
> So the value changes only if it is successful.
> 
> What is the issue you observe?
> Is it a rework to make the code easier to understand?