* [PATCH] ethdev: keep promiscuous/allmulti value before enabling
@ 2025-07-21 11:51 Sunyang Wu
2025-07-21 12:07 ` Thomas Monjalon
0 siblings, 1 reply; 4+ messages in thread
From: Sunyang Wu @ 2025-07-21 11:51 UTC (permalink / raw)
To: dev; +Cc: thomas, ferruh.yigit, andrew.rybchenko, stable
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ethdev: keep promiscuous/allmulti value before enabling
2025-07-21 11:51 [PATCH] ethdev: keep promiscuous/allmulti value before enabling Sunyang Wu
@ 2025-07-21 12:07 ` Thomas Monjalon
2025-07-21 12:13 ` 回复: " Sunyang Wu
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Monjalon @ 2025-07-21 12:07 UTC (permalink / raw)
To: Sunyang Wu; +Cc: dev, ferruh.yigit, andrew.rybchenko, stable
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* 回复: [PATCH] ethdev: keep promiscuous/allmulti value before enabling
2025-07-21 12:07 ` Thomas Monjalon
@ 2025-07-21 12:13 ` Sunyang Wu
2025-07-21 12:28 ` Thomas Monjalon
0 siblings, 1 reply; 4+ messages in thread
From: Sunyang Wu @ 2025-07-21 12:13 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, ferruh.yigit, andrew.rybchenko, stable
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: 回复: [PATCH] ethdev: keep promiscuous/allmulti value before enabling
2025-07-21 12:13 ` 回复: " Sunyang Wu
@ 2025-07-21 12:28 ` Thomas Monjalon
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Monjalon @ 2025-07-21 12:28 UTC (permalink / raw)
To: Sunyang Wu; +Cc: dev, ferruh.yigit, andrew.rybchenko, stable
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-07-21 12:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-07-21 11:51 [PATCH] ethdev: keep promiscuous/allmulti value before enabling Sunyang Wu
2025-07-21 12:07 ` Thomas Monjalon
2025-07-21 12:13 ` 回复: " Sunyang Wu
2025-07-21 12:28 ` Thomas Monjalon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).