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

[Simon Horman <horms@kernel.org>]

On Tue, Oct 15, 2024 at 08:58:38PM +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.
> 
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
> v2 changes:
> This patch is separated from v1 patch 9 ("net: enetc: optimize the
> allocation of tx_bdr"). Separate enetc_int_vector_init() from the
> original patch. In addition, add new help function
> enetc_int_vector_destroy().
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 174 +++++++++----------
>  1 file changed, 87 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 032d8eadd003..d36af3f8ba31 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);
> +	struct enetc_bdr *bdr;
> +	int j, err;
> +
> +	v = kzalloc(struct_size(v, tx_ring, v_tx_rings), GFP_KERNEL);
> +	if (!v)
> +		return -ENOMEM;

...

>  int enetc_alloc_msix(struct enetc_ndev_priv *priv)
>  {
>  	struct pci_dev *pdev = priv->si->pdev;

...

> @@ -2986,64 +3067,9 @@ int enetc_alloc_msix(struct enetc_ndev_priv *priv)
>  	/* # of tx rings per int vector */
>  	v_tx_rings = priv->num_tx_rings / priv->bdr_int_num;
>  
> -	for (i = 0; i < priv->bdr_int_num; i++) {
> -		struct enetc_int_vector *v;
> -		struct enetc_bdr *bdr;
> -		int j;
> -
> -		v = kzalloc(struct_size(v, tx_ring, v_tx_rings), GFP_KERNEL);
> -		if (!v) {
> -			err = -ENOMEM;
> +	for (i = 0; i < priv->bdr_int_num; i++)
> +		if (enetc_int_vector_init(priv, i, v_tx_rings))
>  			goto fail;

Hi Wei Fang,

It looks like, if we reach this error handling during the first iteration
of the for loop then err, which will be return value returned by the function,
is ininitialised. Perhaps this would be better expressed as follows?
(Completely untested!)

		err = enetc_int_vector_init(priv, i, v_tx_rings);
		if (err)
			goto fail;

Flagged by Smatch.

...