Re: [PATCH v2 1/4] net: airoha: Fix an error handling path in airoha_alloc_gdm_port()

[Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>]

Le 08/05/2025 à 17:31, Lorenzo Bianconi a écrit :
> > If register_netdev() fails, the error handling path of the probe will not
> > free the memory allocated by the previous airoha_metadata_dst_alloc() call
> > because port->dev->reg_state will not be NETREG_REGISTERED.
> >
> > So, an explicit airoha_metadata_dst_free() call is needed in this case to
> > avoid a memory leak.
> >
> > Fixes: af3cf757d5c9 ("net: airoha: Move DSA tag in DMA descriptor")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> > ---
> > Changes in v2:
> >    - New patch
> >
> > Compile tested only.
> > ---
> >   drivers/net/ethernet/airoha/airoha_eth.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 16c7896f931f..af8c4015938c 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -2873,7 +2873,15 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> >   	if (err)
> >   		return err;
> >   
> > -	return register_netdev(dev);
> > +	err = register_netdev(dev);
> > +	if (err)
> > +		goto free_metadata_dst;
> > +
> > +	return 0;
> > +
> > +free_metadata_dst:
> > +	airoha_metadata_dst_free(port);
> > +	return err;
> >   }
> >   
> >   static int airoha_probe(struct platform_device *pdev)
> > --
> > 2.49.0
>
> I have not tested it but I think the right fix here would be something like:
>
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index b1ca8322d4eb..33f8926bba25 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2996,10 +2996,12 @@ static int airoha_probe(struct platform_device *pdev)
>   	for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
>   		struct airoha_gdm_port *port = eth->ports[i];
>   
> -		if (port && port->dev->reg_state == NETREG_REGISTERED) {
> +		if (!port)
> +			continue;

I think it works.

We can still have port non NULL and airoha_metadata_dst_alloc() which 
fails, but airoha_metadata_dst_free() seems to handle it correctly.

CJ


> +
> +		if (port->dev->reg_state == NETREG_REGISTERED)
>   			unregister_netdev(port->dev);
> -			airoha_metadata_dst_free(port);
> -		}
> +		airoha_metadata_dst_free(port);
>   	}
>   	free_netdev(eth->napi_dev);
>   	platform_set_drvdata(pdev, NULL);
>
> Regards,
> Lorenzo