Re: [PATCH] net: bgmac: fix refcount leak in bgmac_phy_connect_direct()

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says:

Although the issues reported below are flagged as pre-existing
I wonder if they should be considered in the scope of this patch (set)
as they appear to be similar issues to the one this patch addresses.
---
net: bgmac: fix refcount leak in bgmac_phy_connect_direct()

In bgmac_phy_connect_direct(), fixed_phy_register() returns a
phy_device with an incremented refcount. If the subsequent
phy_connect_direct() call fails, the function returns the error
without releasing the phy_device reference. Add the missing
fixed_phy_unregister() in the error path to prevent the leak.

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 3e9c57196a397..c97cf7ebad7f1 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1456,6 +1456,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
>  				 PHY_INTERFACE_MODE_MII);
>  	if (err) {
>  		dev_err(bgmac->dev, "Connecting PHY failed\n");
> +		fixed_phy_unregister(phy_dev);

[Severity: High]
This isn't a bug introduced by this patch, but does this driver still leak
the fixed phy identifier during driver teardown and in other error paths?

In bgmac_enet_remove(), the driver calls phy_disconnect() but does not
appear to unregister the fixed phy:

drivers/net/ethernet/broadcom/bgmac.c:bgmac_enet_remove() {
	unregister_netdev(bgmac->net_dev);
	phy_disconnect(bgmac->net_dev->phydev);
	netif_napi_del(&bgmac->napi);
	bgmac_dma_free(bgmac);
}

Similarly, in the error path of bgmac_enet_probe():

drivers/net/ethernet/broadcom/bgmac.c:bgmac_enet_probe() {
...
err_phy_disconnect:
	phy_disconnect(net_dev->phydev);
...
}

Since fixed_phy_register() allocates an identifier from fixed_phy_ids (which
has a hard limit of 8 slots), failing to unregister it means the identifier
is never released. After 8 unbind or bind cycles or probe failures, the
fixed_phy_ids bitmap will be exhausted, permanently failing subsequent probes.

Should fixed_phy_unregister() also be called in these paths to prevent
exhausting the fixed phy identifiers?

>  		return err;
>  	}
>