> 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
Actually, in order to be consistent with the rest of the code where a
routine undoes changes in case of an internal failure, I would prefer your
approach. Can you please post your solution in the next iteration? Thanks.
Regards,
Lorenzo
>
>
> > +
> > + 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
>
Attachment:
signature.asc
Description: PGP signature
