[PATCH net] net: emac: release DT PHY resources on teardown

[PATCH net] net: emac: release DT PHY resources on teardown

[Rosen Penev]

Disconnect PHYLIB PHYs and deregister fixed PHYs during both probe
unwind and driver removal. Without this cleanup, unbinding a DT-based
EMAC can leave its PHY attached to the freed netdev, while fixed-link
instances retain their registered fixed PHY.

Fixes: a577ca6badb5 ("net: emac: add support for device-tree based PHY discovery and setup")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index e7043523457c..4124a90b18c4 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -2810,6 +2810,16 @@ static int emac_init_phy(struct emac_instance *dev)
 	return 0;
 }
 
+static void emac_cleanup_phy(struct emac_instance *dev)
+{
+	struct device_node *np = dev->ofdev->dev.of_node;
+
+	if (dev->ndev->phydev && dev->phy.address >= 0)
+		phy_disconnect(dev->ndev->phydev);
+	if (of_phy_is_fixed_link(np))
+		of_phy_deregister_fixed_link(np);
+}
+
 static int emac_init_config(struct emac_instance *dev)
 {
 	struct device_node *np = dev->ofdev->dev.of_node;
@@ -3197,6 +3207,7 @@ static int emac_probe(struct platform_device *ofdev)
 	if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII))
 		zmii_detach(dev->zmii_dev, dev->zmii_port);
  err_unreg_commac:
+	emac_cleanup_phy(dev);
 	mal_unregister_commac(dev->mal, &dev->commac);
  err_rel_deps:
 	emac_put_deps(dev);
@@ -3220,6 +3231,7 @@ static void emac_remove(struct platform_device *ofdev)
 	unregister_netdev(dev->ndev);
 
 	cancel_work_sync(&dev->reset_work);
+	emac_cleanup_phy(dev);
 
 	if (emac_has_feature(dev, EMAC_FTR_HAS_TAH))
 		tah_detach(dev->tah_dev, dev->tah_port);
-- 
2.56.0

Re: [PATCH net] net: emac: release DT PHY resources on teardown

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: emac: release DT PHY resources on teardown

[Rosen Penev]

On Tue, Oct 20, 2026 at 3:33 PM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
LLM
>
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.
>
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Re: [PATCH net] net: emac: release DT PHY resources on teardown

[Andrew Lunn]

On Tue, Oct 20, 2026 at 03:37:57PM -0700, Rosen Penev wrote:
> On Tue, Oct 20, 2026 at 3:33 PM <netdev-bot+sinfo@kernel.org> wrote:
> >
> > Hi!
> >
> > This is an automated message. This series looks like a fix, but its
> > commit messages seem to be missing some information:
> >
> >  - How the issue was discovered, e.g. hit in production, hit during
> >    development, syzbot report, manual code inspection, LLM or static
> >    analysis tool scan.
> LLM

Ideally, you should learn from these emails, and put it into the
commit message.

       Andrew

Re: [PATCH net] net: emac: release DT PHY resources on teardown

[Andrew Lunn]

On Tue, Oct 20, 2026 at 03:30:02PM -0700, Rosen Penev wrote:
> Disconnect PHYLIB PHYs and deregister fixed PHYs during both probe
> unwind and driver removal. Without this cleanup, unbinding a DT-based
> EMAC can leave its PHY attached to the freed netdev, while fixed-link
> instances retain their registered fixed PHY.

This driver is quite convoluted with its PHY handling.

Please split this into two, phy_disconnect() and
of_phy_deregister_fixed_link() and add better commit messages.

	if ((dev->phy_address == 0xffffffff && dev->phy_map == 0xffffffff) ||
	    of_phy_is_fixed_link(np)) {

		/* PHY-less configuration. */
		dev->phy.address = -1;
...
		if (of_phy_is_fixed_link(np)) {
			int res = emac_dt_mdio_probe(dev);

			if (res)
				return res;

			res = of_phy_register_fixed_link(np);

The logic here is more complex than 

> +	if (of_phy_is_fixed_link(np))
> +		of_phy_deregister_fixed_link(np);

So you need to explain in the commit message why it is correct.

> +static void emac_cleanup_phy(struct emac_instance *dev)
> +{
> +	struct device_node *np = dev->ofdev->dev.of_node;
> +
> +	if (dev->ndev->phydev && dev->phy.address >= 0)
> +		phy_disconnect(dev->ndev->phydev);

It is also not clear this is correct, since a fixed-link should be
connected to, yet a fixed link has dev->phy.address == -1?

	Andrew

Re: [PATCH net] net: emac: release DT PHY resources on teardown

[Rosen Penev]

On Tue Oct 20, 2026 at 6:05 PM PDT, Andrew Lunn wrote:
> On Tue, Oct 20, 2026 at 03:30:02PM -0700, Rosen Penev wrote:
>> Disconnect PHYLIB PHYs and deregister fixed PHYs during both probe
>> unwind and driver removal. Without this cleanup, unbinding a DT-based
>> EMAC can leave its PHY attached to the freed netdev, while fixed-link
>> instances retain their registered fixed PHY.
>
> This driver is quite convoluted with its PHY handling.
I agree. I got this LLM to convert the whole thing to phylink. It tested
a bunch of stuff over serial and ethernet, which works. That's net-next
material though. It'll probably get rejected.
>
> Please split this into two, phy_disconnect() and
> of_phy_deregister_fixed_link() and add better commit messages.
>
> 	if ((dev->phy_address == 0xffffffff && dev->phy_map == 0xffffffff) ||
> 	    of_phy_is_fixed_link(np)) {
>
> 		/* PHY-less configuration. */
> 		dev->phy.address = -1;
> ...
> 		if (of_phy_is_fixed_link(np)) {
> 			int res = emac_dt_mdio_probe(dev);
>
> 			if (res)
> 				return res;
>
> 			res = of_phy_register_fixed_link(np);
>
> The logic here is more complex than
>
>> +	if (of_phy_is_fixed_link(np))
>> +		of_phy_deregister_fixed_link(np);
>
> So you need to explain in the commit message why it is correct.
>
>> +static void emac_cleanup_phy(struct emac_instance *dev)
>> +{
>> +	struct device_node *np = dev->ofdev->dev.of_node;
>> +
>> +	if (dev->ndev->phydev && dev->phy.address >= 0)
>> +		phy_disconnect(dev->ndev->phydev);
>
> It is also not clear this is correct, since a fixed-link should be
> connected to, yet a fixed link has dev->phy.address == -1?
>
> 	Andrew

Re: [PATCH net] net: emac: release DT PHY resources on teardown

[Andrew Lunn]

On Tue, Oct 20, 2026 at 08:11:02PM -0700, Rosen Penev wrote:
> On Tue Oct 20, 2026 at 6:05 PM PDT, Andrew Lunn wrote:
> > On Tue, Oct 20, 2026 at 03:30:02PM -0700, Rosen Penev wrote:
> >> Disconnect PHYLIB PHYs and deregister fixed PHYs during both probe
> >> unwind and driver removal. Without this cleanup, unbinding a DT-based
> >> EMAC can leave its PHY attached to the freed netdev, while fixed-link
> >> instances retain their registered fixed PHY.
> >
> > This driver is quite convoluted with its PHY handling.
> I agree. I got this LLM to convert the whole thing to phylink. It tested
> a bunch of stuff over serial and ethernet, which works. That's net-next
> material though. It'll probably get rejected.

What is important here is you have the hardware and you have tested
it. That is better than most LLM generated patches which sometime
don't even build, and most of the time are never run on hardware
before submission.

If your patchset for phylink is up to mainline quality, post it.

We push back on LLM generated patches which are of obvious bad
quality. But we have no problems with good quality patches tested on
real hardware.

     Andrew

Re: [PATCH net] net: emac: release DT PHY resources on teardown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] emac_remove() calls the new emac_cleanup_phy() only after
  unregister_netdev() has returned, and the phylib PHY is still running…
- [Low] The new phy_disconnect() call in emac_cleanup_phy() runs without
  RTNL in both callers.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but busy_phy_map
  reservations are unbalanced.

--

net: emac: release DT PHY resources on teardown

This adds emac_cleanup_phy(), which disconnects phylib PHYs and
deregisters fixed-link PHYs. It is called from the err_unreg_commac
unwind in emac_probe() and from emac_remove() after unregister_netdev().

> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index e7043523457c1..4124a90b18c49 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -2810,6 +2810,16 @@ static int emac_init_phy(struct emac_instance *dev)
>  	return 0;
>  }
>  
> +static void emac_cleanup_phy(struct emac_instance *dev)
> +{
> +	struct device_node *np = dev->ofdev->dev.of_node;
> +
> +	if (dev->ndev->phydev && dev->phy.address >= 0)
> +		phy_disconnect(dev->ndev->phydev);

[Severity: Low]

Should RTNL be held around this phy_disconnect()? Neither caller takes
it. emac_remove() calls this after unregister_netdev() has already dropped
RTNL, and the err_unreg_commac unwind in emac_probe() never takes it.

For a PHY attached via of_phy_connect(), phy_disconnect() -> phy_detach()
reaches:

drivers/net/phy/phy_device.c:phy_detach() {
    ...
		hwprov = rtnl_dereference(dev->hwprov);
    ...
}

rtnl_dereference() is rcu_dereference_protected(p, lockdep_rtnl_is_held()).
With CONFIG_PROVE_RCU this would print a "suspicious
rcu_dereference_protected() usage" splat on every unbind of a phy-handle
EMAC. It would also print one on probe unwind once the PHY is attached,
for example when register_netdev() fails.

The netdev is unregistered or not yet registered at this point, so no
concurrent hwprov writer can exist. That makes this a lockdep splat
rather than a real race. smsc95xx_unbind() follows the same pattern.

Could emac_cleanup_phy() take rtnl_lock() around phy_disconnect()?

> +	if (of_phy_is_fixed_link(np))
> +		of_phy_deregister_fixed_link(np);
> +}
> +

[ ... ]

> @@ -3220,6 +3231,7 @@ static void emac_remove(struct platform_device *ofdev)
>  	unregister_netdev(dev->ndev);
>  
>  	cancel_work_sync(&dev->reset_work);
> +	emac_cleanup_phy(dev);

[Severity: High]

Can this leave ndev on the linkwatch event list after it has been freed?

unregister_netdev() has returned by now, but the phylib PHY attached by
emac_dt_phy_connect() is still running. emac_mdio_init_phy() starts it at
probe time:

static int emac_mdio_init_phy(struct mii_phy *phy)
{
	struct net_device *ndev = phy->dev;

	phy_start(ndev->phydev);
	return phy_init_hw(ndev->phydev);
}

The driver never calls phy_stop(), either from emac_close() or anywhere
else.

Suppose the interface is down at unbind time and the PHY link is up. This
happens if the interface was never opened, or if it was closed and the
link then toggled. Carrier is on, and unregister_netdev() does not clear
it. The new call then does:

emac_cleanup_phy()
  phy_disconnect()
    phy_stop()
      _phy_state_machine()     /* PHY_HALTED, phydev->link set */
        phy_link_down()
          phy_link_change(false)
            netif_carrier_off(ndev)
              linkwatch_fire_event()
                linkwatch_add_event()

netif_carrier_off() only returns early for NETREG_UNINITIALIZED.
linkwatch_add_event() does not check reg_state. So the unregistered ndev
is added to lweventlist, a netdev_hold() is taken, and linkwatch_work is
scheduled.

After emac_remove() returns, devres (devm_alloc_etherdev) calls
free_netdev(), and free_netdev() does not check link_watch_list. Within
about a second, linkwatch_event() -> __linkwatch_run_queue() would walk
lweventlist and dereference the freed net_device.

Before this patch, the PHY was torn down through the devm mdiobus
unregistration. phy_remove() only cancels state_queue and fires no
carrier events.

Would it help to call phy_stop() before unregister_netdev(), so the
carrier event gets flushed by netdev_run_todo()/linkwatch_sync_dev()?
Another option is to move phy_start()/phy_stop() into ndo_open/ndo_stop,
so the PHY is already halted with link down when phy_disconnect() runs.

>  
>  	if (emac_has_feature(dev, EMAC_FTR_HAS_TAH))
>  		tah_detach(dev->tah_dev, dev->tah_port);

[Severity: Medium]

This isn't a bug introduced by this patch, but it is in the same teardown
path. emac_cleanup_phy() guards on dev->phy.address >= 0, yet the
busy_phy_map update a few lines later in emac_remove() has no guard and
takes no lock:

drivers/net/ethernet/ibm/emac/core.c:emac_remove() {
    ...
	busy_phy_map &= ~(1 << dev->phy.address);
    ...
}

PHY-less and fixed-link instances set dev->phy.address = -1 in
emac_init_phy(), so this evaluates 1 << -1. That shift is undefined
behaviour, and UBSAN would report it as shift-out-of-bounds.

DT phy-handle instances get dev->phy.address = phy_dev->mdio.addr in
emac_dt_phy_connect() but never set a bit in busy_phy_map. Could this
clear a bit that another EMAC instance reserved during its own legacy
scan?

Bits are only set in the legacy scan loop in emac_init_phy():

			busy_phy_map |= 1 << i;

None of the emac_probe() error labels clear them. Do those addresses stay
reserved after a failed probe?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020223002.1749284-1-rosenp%40gmail.com