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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

On 6/5/26 03:24, Jakub Kicinski wrote:
> 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.

Good point too. I've sent a V3 already, so if V3 is accepted I'll send a
standalone patch for that, or include that in V4 should it be needed :)

Maxime

> 
>>  	if (!phydev->is_on_sfp_module)
>>  		phy_led_triggers_unregister(phydev);
>>