Re: [PATCH v5 net-next 10/13] net: enetc: extract enetc_int_vector_init/destroy() from enetc_alloc_msix()

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Thu, Oct 24, 2024 at 02:53:25PM +0800, Wei Fang wrote:
> From: Clark Wang <xiaoning.wang@nxp.com>
> 
> 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@nxp.com>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>
> ---
> 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.
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.

> +	struct enetc_bdr *bdr;
> +	int j, err;
> +
> +	v = kzalloc(struct_size(v, tx_ring, v_tx_rings), GFP_KERNEL);
> +	if (!v)
> +		return -ENOMEM;
> +
> +	bdr = &v->rx_ring;
> +	bdr->index = i;
> +	bdr->ndev = priv->ndev;
> +	bdr->dev = priv->dev;
> +	bdr->bd_count = priv->rx_bd_count;
> +	bdr->buffer_offset = ENETC_RXB_PAD;
> +	priv->rx_ring[i] = bdr;
> +
> +	err = xdp_rxq_info_reg(&bdr->xdp.rxq, priv->ndev, i, 0);
> +	if (err)
> +		return err;
> +
> +	err = xdp_rxq_info_reg_mem_model(&bdr->xdp.rxq,
> +					 MEM_TYPE_PAGE_SHARED, NULL);

MEM_TYPE_PAGE_SHARED fits on the previous line.

> +	if (err) {
> +		xdp_rxq_info_unreg(&bdr->xdp.rxq);
> +		return err;
> +	}
> +
> +	/* init defaults for adaptive IC */
> +	if (priv->ic_mode & ENETC_IC_RX_ADAPTIVE) {
> +		v->rx_ictt = 0x1;
> +		v->rx_dim_en = true;
> +	}
> +
> +	INIT_WORK(&v->rx_dim.work, enetc_rx_dim_work);
> +	netif_napi_add(priv->ndev, &v->napi, enetc_poll);
> +	v->count_tx_rings = v_tx_rings;
> +
> +	for (j = 0; j < v_tx_rings; j++) {
> +		int idx;
> +
> +		/* default tx ring mapping policy */
> +		idx = priv->bdr_int_num * j + i;
> +		__set_bit(idx, &v->tx_rings_map);
> +		bdr = &v->tx_ring[j];
> +		bdr->index = idx;
> +		bdr->ndev = priv->ndev;
> +		bdr->dev = priv->dev;
> +		bdr->bd_count = priv->tx_bd_count;
> +		priv->tx_ring[idx] = bdr;
> +	}
> +
> +	priv->int_vector[i] = no_free_ptr(v);
> +
> +	return 0;
> +}