Re: [PATCH net v2 1/3] net: phy: clean the sfp upstream if phy probing fails

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: clean the sfp upstream if phy probing fails

This commit adds a call to sfp_bus_del_upstream() in the probe failure path
to ensure the sfp-bus is not left with a dangling upstream field that could
be used during subsequent SFP events.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -3775,6 +3778,9 @@ static int phy_probe(struct device *dev)
>  	return 0;
>  
>  out:
> +	sfp_bus_del_upstream(phydev->sfp_bus);
> +	phydev->sfp_bus = NULL;
> +

[Severity: High]
This is a pre-existing issue, but if a subsequent initialization step fails
and we jump to the out label, should we be calling the specific PHY driver's
remove callback?

If phydev->drv->probe() succeeds early in phy_probe(), but something like
phy_setup_ports() or of_phy_leds() fails later, the execution jumps to this
error path. Without calling phydev->drv->remove(phydev), any resources
allocated by the driver are leaked.

Also, drivers like dp83640 register the phydev with external subsystems,
such as adding it to shared lists like clock->phylist during their probe.
Leaving dangling pointers to this unbound phydev in those subsystems could
lead to use-after-free crashes later.

>  	if (!phydev->is_on_sfp_module)
>  		phy_led_triggers_unregister(phydev);
>
-- 
pw-bot: cr