> On Thu, Oct 24, 2024 at 02:53:25PM +0800, Wei Fang wrote:
> > From: Clark Wang <xiaoning.wang@xxxxxxx>
> >
> > Extract enetc_int_vector_init() and enetc_int_vector_destroy() from
> > enetc_alloc_msix() so that the code is more concise and readable. In
> > addition, slightly different from before, the cleanup helper function
> > is used to manage dynamically allocated memory resources.
> >
> > Signed-off-by: Clark Wang <xiaoning.wang@xxxxxxx>
> > Signed-off-by: Wei Fang <wei.fang@xxxxxxx>
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > Reviewed-by: Claudiu Manoil <claudiu.manoil@xxxxxxx>
> > ---
> > v5: no changes
> > ---
> > drivers/net/ethernet/freescale/enetc/enetc.c | 172 ++++++++++---------
> > 1 file changed, 87 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 032d8eadd003..bd725561b8a2 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -2965,6 +2965,87 @@ int enetc_ioctl(struct net_device *ndev, struct
> ifreq *rq, int cmd)
> > }
> > EXPORT_SYMBOL_GPL(enetc_ioctl);
> >
> > +static int enetc_int_vector_init(struct enetc_ndev_priv *priv, int i,
> > + int v_tx_rings)
> > +{
> > + struct enetc_int_vector *v __free(kfree);
>
> Please limit yourself to refactoring the code as-is into a separate function.
Okay
> This function does not benefit in any way from the use of __free() and
> no_free_ptr(). The established norm is that the normal teardown path
> should be identical to the error unwind path, minus the last step.
> Combining normal function calls with devres/scope-based cleanup/whatever
> other "look, you don't get to care about error handling!!!" APIs there may be
> makes that much more difficult to reason about. If the function is really
> simple and you really don't get to care about error handling by using __free(),
> then maybe its usage is tolerable, but that is hardly the general case.
> The more intricate the error handling becomes, the less useful __free() is,
> and the more it starts getting in the way of a human correctness reviewer.
>
> FWIW, Documentation/process/maintainer-netdev.rst, section "Using
> device-managed and cleanup.h constructs", appears to mostly state the
> same position as me here.
>
> Obviously, here, the established cleanup norm isn't followed anyway, but
> a patch which brings it in line would be appreciated. I think that a
> multi-patch approach, where the code is first moved and just moved, and
> then successively transformed in obviously correct and easy to review
> steps, would be best.
>
> Since you're quite close currently to the 15-patch limit, I would try to
> create a patch set just for preparing the enetc drivers, and introduce
> the i.mx95 support in a separate set which has mostly "+" lines in its diff.
> That way, there is also some time to not rush the ongoing IERB/PRB
> dt-binding discussion, while you can progress on pure driver refactoring.
>
Thanks for your suggestion.
