Re: [PATCH net v1 1/2] net: phy: split phy_probe() error paths

[Maxime Chevallier]

Hi,

On 8/12/26 15:41, Andrew Lunn wrote:
> On Wed, Aug 12, 2026 at 08:51:26PM +0800, Xuanqiang Luo wrote:
>> From: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
>>
>> phy_probe() uses one cleanup path for failures at every initialization
>> stage. This runs cleanup for resources that have not been initialized
>> and leaves phy_setup_ports() relying on its caller to remove an SFP
>> upstream after a partial failure.
>>
>> Make phy_setup_ports() unwind the SFP upstream before removing its ports.
>> Then split the phy_probe() cleanup so each failure path unwinds only the
>> resources that may have been initialized.
>>
>> Signed-off-by: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
>> ---
>>  drivers/net/phy/phy_device.c | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 0615228459ef4..f8e434daab66e 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -3556,6 +3556,9 @@ static int phy_setup_ports(struct phy_device *phydev)
>>  	return 0;
>>  
>>  out:
>> +	sfp_bus_del_upstream(phydev->sfp_bus);
>> +	phydev->sfp_bus = NULL;
>> +
>>  	phy_cleanup_ports(phydev);
>>  	return ret;
> 
> This does not look correct. phy_sfp_probe() may fail, and you then
> call sfp_bus_del_upstream() on something which never happened.
> 
> Also, it is not obvious that sfp_bus_del_upstream() is the correct
> thing to do. You are trying to undo phy_sfp_probe() so i would expect
> you to call a function like phy_sfp_release().

I agree with having phy_sfp_release(), we have even more to do with phy SFP
in the future with phy_port, this is less error prone.

> 
> It also looks like phy_sfp_probe() does not correctly clean up on
> phy_setup_sfp_port() returning an error. But that is a different
> issue.

In practise, this is cleaned in the phy_probe's "out" failure label,
but indeed this is not pretty.

Maxime