[PATCH net v2 0/3] net: phy: some cleanups following phy_port SFP

[PATCH net v2 0/3] net: phy: some cleanups following phy_port SFP

[Maxime Chevallier]

While posting the v11 of phy_port netlink, sashiko found some
pre-existing issues, and following the tentative fix [1], Nicolai found
some more :)

Patch 1 and 2 add some cleanup in the phy_probe error paths, to discard
all SFP and phy_port that have been set-up when probing fails further
down the road.

Patch 3 fixes a sashiko-reported issue that I was able to reproduce,
where we hit a deadlock when trying to setup PHY-driven SFP ports while
using genphy. This is because the sfp init logic for PHYs is now
generic, instead of per-driver, and genphy has the particularity of
running its .probe() under RTNL, which clashes with the SFP code.

One more reason to try and cleanup RTNL handling in PHY/SFP :)

Thanks Nicolai for the feedback on V1,

Maxime

[1] : https://lore.kernel.org/r/20260530072706.3167745-1-maxime.chevallier@bootlin.com

Maxime Chevallier (3):
  net: phy: clean the sfp upstream if phy probing fails
  net: phy: remove phy ports upon probe failure
  net: phy: don't try to setup PHY-driven SFP cages when using genphy

 drivers/net/phy/phy_device.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

-- 
2.54.0

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

[Maxime Chevallier]

Sashiko reported that we don't call sfp_bus_del_upstream() in the probe
failure path, so let's add it, otherwise the sfp-bus is left with a
dangling 'upstream' field, that may be used later on during SFP events.

This issue existed before the generic phylib sfp support, back when
drivers were calling phy_sfp_probe themselves.

Fixes: 298e54fa810e ("net: phy: add core phylib sfp support")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2: Null-ify phydev->sfp_bus upon sfp_bus_add_upstream failure

 drivers/net/phy/phy_device.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3370eb822017..6ccbfacf7d1d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1718,6 +1718,9 @@ static int phy_sfp_probe(struct phy_device *phydev)
 
 		ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
 		sfp_bus_put(bus);
+
+		if (ret)
+			phydev->sfp_bus = NULL;
 	}
 
 	if (!ret && phydev->sfp_bus)
@@ -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;
+
 	if (!phydev->is_on_sfp_module)
 		phy_led_triggers_unregister(phydev);
 
-- 
2.54.0

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

[Nicolai Buchwitz]

On 1.6.2026 10:40, Maxime Chevallier wrote:
> Sashiko reported that we don't call sfp_bus_del_upstream() in the probe
> failure path, so let's add it, otherwise the sfp-bus is left with a
> dangling 'upstream' field, that may be used later on during SFP events.
> 
> This issue existed before the generic phylib sfp support, back when
> drivers were calling phy_sfp_probe themselves.
> 
> Fixes: 298e54fa810e ("net: phy: add core phylib sfp support")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> V2: Null-ify phydev->sfp_bus upon sfp_bus_add_upstream failure
> 
>  drivers/net/phy/phy_device.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c 
> b/drivers/net/phy/phy_device.c
> index 3370eb822017..6ccbfacf7d1d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1718,6 +1718,9 @@ static int phy_sfp_probe(struct phy_device 
> *phydev)
> 
>  		ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
>  		sfp_bus_put(bus);
> +
> +		if (ret)
> +			phydev->sfp_bus = NULL;
>  	}
> 
>  	if (!ret && phydev->sfp_bus)
> @@ -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;
> +
>  	if (!phydev->is_on_sfp_module)
>  		phy_led_triggers_unregister(phydev);

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks
Nicolai

[PATCH net v2 2/3] net: phy: remove phy ports upon probe failure

[Maxime Chevallier]

When phy_probe fails, let's clean the phy_ports that were successfully
added already.

Suggested-by: Nicolai Buchwitz <nb@tipi-net.de>
Fixes: 589e934d2735 ("net: phy: Introduce PHY ports representation")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6ccbfacf7d1d..a171cbe2a74a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3778,6 +3778,8 @@ static int phy_probe(struct device *dev)
 	return 0;
 
 out:
+	phy_cleanup_ports(phydev);
+
 	sfp_bus_del_upstream(phydev->sfp_bus);
 	phydev->sfp_bus = NULL;
 
-- 
2.54.0

Re: [PATCH net v2 2/3] net: phy: remove phy ports upon probe failure

[Nicolai Buchwitz]

On 1.6.2026 10:40, Maxime Chevallier wrote:
> When phy_probe fails, let's clean the phy_ports that were successfully
> added already.
> 
> Suggested-by: Nicolai Buchwitz <nb@tipi-net.de>
> Fixes: 589e934d2735 ("net: phy: Introduce PHY ports representation")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/phy_device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c 
> b/drivers/net/phy/phy_device.c
> index 6ccbfacf7d1d..a171cbe2a74a 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3778,6 +3778,8 @@ static int phy_probe(struct device *dev)
>  	return 0;
> 
>  out:
> +	phy_cleanup_ports(phydev);
> +
>  	sfp_bus_del_upstream(phydev->sfp_bus);
>  	phydev->sfp_bus = NULL;

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks
Nicolai

Re: [PATCH net v2 2/3] net: phy: remove phy ports upon probe failure

[Maxime Chevallier]

Hi Nicolai,

On 6/1/26 11:31, Nicolai Buchwitz wrote:
> On 1.6.2026 10:40, Maxime Chevallier wrote:
>> When phy_probe fails, let's clean the phy_ports that were successfully
>> added already.
>>
>> Suggested-by: Nicolai Buchwitz <nb@tipi-net.de>
>> Fixes: 589e934d2735 ("net: phy: Introduce PHY ports representation")
>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>> ---
>>  drivers/net/phy/phy_device.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 6ccbfacf7d1d..a171cbe2a74a 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -3778,6 +3778,8 @@ static int phy_probe(struct device *dev)
>>      return 0;
>>
>>  out:
>> +    phy_cleanup_ports(phydev);
>> +
>>      sfp_bus_del_upstream(phydev->sfp_bus);
>>      phydev->sfp_bus = NULL;
> 
> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

I'll be moving phy_cleanup_ports(phydev); after the sfp cleanup,
can I keep your review tag ?

Maxime

> 
> Thanks
> Nicolai

Re: [PATCH net v2 2/3] net: phy: remove phy ports upon probe failure

[Nicolai Buchwitz]

On 4.6.2026 10:02, Maxime Chevallier wrote:
> Hi Nicolai,
> 
> On 6/1/26 11:31, Nicolai Buchwitz wrote:
>> On 1.6.2026 10:40, Maxime Chevallier wrote:
>>> When phy_probe fails, let's clean the phy_ports that were 
>>> successfully
>>> added already.
>>> 
>>> Suggested-by: Nicolai Buchwitz <nb@tipi-net.de>
>>> Fixes: 589e934d2735 ("net: phy: Introduce PHY ports representation")
>>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>>> ---
>>>  drivers/net/phy/phy_device.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>> 
>>> diff --git a/drivers/net/phy/phy_device.c 
>>> b/drivers/net/phy/phy_device.c
>>> index 6ccbfacf7d1d..a171cbe2a74a 100644
>>> --- a/drivers/net/phy/phy_device.c
>>> +++ b/drivers/net/phy/phy_device.c
>>> @@ -3778,6 +3778,8 @@ static int phy_probe(struct device *dev)
>>>      return 0;
>>> 
>>>  out:
>>> +    phy_cleanup_ports(phydev);
>>> +
>>>      sfp_bus_del_upstream(phydev->sfp_bus);
>>>      phydev->sfp_bus = NULL;
>> 
>> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
> 
> I'll be moving phy_cleanup_ports(phydev); after the sfp cleanup,
> can I keep your review tag ?

Sure.

Thanks,
Nicolai

> [...]

Re: [PATCH net v2 2/3] net: phy: remove phy ports upon probe failure

[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: remove phy ports upon probe failure

When phy_probe fails, let's clean the phy_ports that were successfully
added already.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 6ccbfacf7d1d2..a171cbe2a74a1 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3778,6 +3778,8 @@ static int phy_probe(struct device *dev)
>  	return 0;
>  
>  out:
> +	phy_cleanup_ports(phydev);
> +
>  	sfp_bus_del_upstream(phydev->sfp_bus);
>  	phydev->sfp_bus = NULL;
>  

[Severity: Critical]
Is it safe to call phy_cleanup_ports() before sfp_bus_del_upstream() here?

sfp_bus_del_upstream() unregisters the upstream SFP callbacks and acts as a
synchronization barrier by acquiring rtnl_lock(). Before it is called, the
SFP bus is still active and can asynchronously fire events like
phy_sfp_module_insert().

Because these SFP events locklessly iterate over the phydev->ports list via
phy_get_sfp_port(), and phy_cleanup_ports() mutates the list and frees port
structures without lock protection:

Could this concurrent access lead to list corruption and a use-after-free?

This same incorrect ordering is a pre-existing issue in the phy_remove()
teardown path, but this patch introduces it into the phy_probe() error path.
-- 
pw-bot: cr

Re: [PATCH net v2 2/3] net: phy: remove phy ports upon probe failure

[Maxime Chevallier]

On 6/4/26 04:27, 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: remove phy ports upon probe failure
> 
> When phy_probe fails, let's clean the phy_ports that were successfully
> added already.
> 
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 6ccbfacf7d1d2..a171cbe2a74a1 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -3778,6 +3778,8 @@ static int phy_probe(struct device *dev)
>>  	return 0;
>>  
>>  out:
>> +	phy_cleanup_ports(phydev);
>> +
>>  	sfp_bus_del_upstream(phydev->sfp_bus);
>>  	phydev->sfp_bus = NULL;
>>  
> 
> [Severity: Critical]
> Is it safe to call phy_cleanup_ports() before sfp_bus_del_upstream() here?
> 
> sfp_bus_del_upstream() unregisters the upstream SFP callbacks and acts as a
> synchronization barrier by acquiring rtnl_lock(). Before it is called, the
> SFP bus is still active and can asynchronously fire events like
> phy_sfp_module_insert().
> 
> Because these SFP events locklessly iterate over the phydev->ports list via
> phy_get_sfp_port(), and phy_cleanup_ports() mutates the list and frees port
> structures without lock protection:
> 
> Could this concurrent access lead to list corruption and a use-after-free?
> 
> This same incorrect ordering is a pre-existing issue in the phy_remove()
> teardown path, but this patch introduces it into the phy_probe() error path.

Heh true indeed... Let me respin then, I'll fix the other callsite that has the
same problem

Thanks,

Maxime

[PATCH net v2 3/3] net: phy: don't try to setup PHY-driven SFP cages when using genphy

[Maxime Chevallier]

We don't have support for PHY-driver SFP cages with the genphy code.

On top of that, it was found by sashiko that running
sfp_bus_add_upstream() for genphy deadlocks, as for genphy the PHY
probing runs under RTNL, which isn't the case for non-genphy drivers.

This problem was reproduced, and does lead to a deadlock on RTNL.

Before the blamed commit, the phy_sfp_probe() call was made by
individual PHY drivers, so there was no way to get to the SFP probing
path when using genphy.

Let's therefore only run phy_sfp_probe when not using genphy.

Fixes: bad869b5e41a ("net: phy: Only rely on phy_port for PHY-driven SFP")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a171cbe2a74a..8eace58e9f12 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3512,9 +3512,14 @@ static int phy_setup_ports(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	ret = phy_sfp_probe(phydev);
-	if (ret)
-		goto out;
+	/* We don't support SFP with genphy drivers. Also, genphy driver binding
+	 * occurs with RTNL help, wich will deadlock the sfp_bus_add_upstream().
+	 */
+	if (!phydev->is_genphy_driven) {
+		ret = phy_sfp_probe(phydev);
+		if (ret)
+			goto out;
+	}
 
 	if (phydev->n_ports < phydev->max_n_ports) {
 		ret = phy_default_setup_single_port(phydev);
-- 
2.54.0

Re: [PATCH net v2 3/3] net: phy: don't try to setup PHY-driven SFP cages when using genphy

[Nicolai Buchwitz]

On 1.6.2026 10:40, Maxime Chevallier wrote:
> We don't have support for PHY-driver SFP cages with the genphy code.
> 
> On top of that, it was found by sashiko that running
> sfp_bus_add_upstream() for genphy deadlocks, as for genphy the PHY
> probing runs under RTNL, which isn't the case for non-genphy drivers.
> 
> This problem was reproduced, and does lead to a deadlock on RTNL.
> 
> Before the blamed commit, the phy_sfp_probe() call was made by
> individual PHY drivers, so there was no way to get to the SFP probing
> path when using genphy.
> 
> Let's therefore only run phy_sfp_probe when not using genphy.
> 
> Fixes: bad869b5e41a ("net: phy: Only rely on phy_port for PHY-driven 
> SFP")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/phy_device.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c 
> b/drivers/net/phy/phy_device.c
> index a171cbe2a74a..8eace58e9f12 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3512,9 +3512,14 @@ static int phy_setup_ports(struct phy_device 
> *phydev)
>  	if (ret)
>  		return ret;
> 
> -	ret = phy_sfp_probe(phydev);
> -	if (ret)
> -		goto out;
> +	/* We don't support SFP with genphy drivers. Also, genphy driver 
> binding
> +	 * occurs with RTNL help, wich will deadlock the 
> sfp_bus_add_upstream().
> +	 */
> +	if (!phydev->is_genphy_driven) {
> +		ret = phy_sfp_probe(phydev);
> +		if (ret)
> +			goto out;
> +	}
> 
>  	if (phydev->n_ports < phydev->max_n_ports) {
>  		ret = phy_default_setup_single_port(phydev);

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks
Nicolai

Re: [PATCH net v2 3/3] net: phy: don't try to setup PHY-driven SFP cages when using genphy

[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: don't try to setup PHY-driven SFP cages when using genphy

This change skips phy_sfp_probe() when the genphy driver is bound,
avoiding an RTNL deadlock in sfp_bus_add_upstream() since genphy
binding runs under RTNL while non-genphy driver binding does not.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index a171cbe2a74a..8eace58e9f12 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3512,9 +3512,14 @@ static int phy_setup_ports(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> -	ret = phy_sfp_probe(phydev);
> -	if (ret)
> -		goto out;
> +	/* We don't support SFP with genphy drivers. Also, genphy driver binding
> +	 * occurs with RTNL help, wich will deadlock the sfp_bus_add_upstream().
                          ^^^^

[Low]
There is a typo in the new comment, "wich" should be "which".

> +	 */
> +	if (!phydev->is_genphy_driven) {
> +		ret = phy_sfp_probe(phydev);
> +		if (ret)
> +			goto out;
> +	}
>  
>  	if (phydev->n_ports < phydev->max_n_ports) {
>  		ret = phy_default_setup_single_port(phydev);

Re: [PATCH net v2 3/3] net: phy: don't try to setup PHY-driven SFP cages when using genphy

[Maxime Chevallier]

On 6/4/26 04:27, 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: don't try to setup PHY-driven SFP cages when using genphy
> 
> This change skips phy_sfp_probe() when the genphy driver is bound,
> avoiding an RTNL deadlock in sfp_bus_add_upstream() since genphy
> binding runs under RTNL while non-genphy driver binding does not.
> 
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index a171cbe2a74a..8eace58e9f12 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -3512,9 +3512,14 @@ static int phy_setup_ports(struct phy_device *phydev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	ret = phy_sfp_probe(phydev);
>> -	if (ret)
>> -		goto out;
>> +	/* We don't support SFP with genphy drivers. Also, genphy driver binding
>> +	 * occurs with RTNL help, wich will deadlock the sfp_bus_add_upstream().
>                           ^^^^
> 
> [Low]
> There is a typo in the new comment, "wich" should be "which".

Sure thing, I need to respin anyway, let me fix this :)

Maxime

> 
>> +	 */
>> +	if (!phydev->is_genphy_driven) {
>> +		ret = phy_sfp_probe(phydev);
>> +		if (ret)
>> +			goto out;
>> +	}
>>  
>>  	if (phydev->n_ports < phydev->max_n_ports) {
>>  		ret = phy_default_setup_single_port(phydev);