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: sta...@dpdk.org
> >
> > Signed-off-by: Sunyang Wu <sunyang...@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?
