[PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Ali Tariq]

The emac_resume() calls the emac_open() and emac_suspend() calls
emac_stop() functions directly without holding the lock through
rtnl_lock(). These functions rely on phy_disconnect() which reads pointer
with rtnl_dereference(). But since no lock is being held,
rtnl_dereference throws a suspicious RCU usage on
Orange Pi RV2 (SpacemiT K1) warning when lockdep is enabled.

WARNING: suspicious RCU usage
drivers/net/phy/phy_device.c:1831 suspicious rcu_dereference_protected() usage!
Call Trace:
phy_detach_internal+0x490/0x5f2
phy_disconnect+0x90/0xc0
emac_stop+0x114/0x626
emac_suspend+0xac/0x126
platform_pm_suspend+0x72/0xe4

Hold RTNL lock before emac_open() and emac_stop() with rtnl_lock() and
release the lock when functions return with rtnl_unlock().

Fixes: bfec6d7f2001 ("net: spacemit: Add K1 Ethernet MAC")
Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
---
Warning Log: https://kernelci.cloud-v.co/scheduler/job/549#L2168
Fixed Log: https://kernelci.cloud-v.co/scheduler/job/557
---
 drivers/net/ethernet/spacemit/k1_emac.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
index 71a80838bffd..aca25a9e1e9e 100644
--- a/drivers/net/ethernet/spacemit/k1_emac.c
+++ b/drivers/net/ethernet/spacemit/k1_emac.c
@@ -2033,7 +2033,10 @@ static int emac_resume(struct device *dev)
 	if (!netif_running(ndev))
 		return 0;
 
+	rtnl_lock();
 	ret = emac_open(ndev);
+	rtnl_unlock();
+
 	if (ret) {
 		clk_disable_unprepare(priv->bus_clk);
 		return ret;
@@ -2056,7 +2059,9 @@ static int emac_suspend(struct device *dev)
 		return 0;
 	}
 
+	rtnl_lock();
 	emac_stop(ndev);
+	rtnl_unlock();
 
 	clk_disable_unprepare(priv->bus_clk);
 	netif_device_detach(ndev);
-- 
2.34.1

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Breno Leitao]

On Thu, Nov 05, 2026 at 05:34:37PM +0500, Ali Tariq wrote:
> The emac_resume() calls the emac_open() and emac_suspend() calls
> emac_stop() functions directly without holding the lock through
> rtnl_lock(). These functions rely on phy_disconnect() which reads pointer
> with rtnl_dereference(). But since no lock is being held,
> rtnl_dereference throws a suspicious RCU usage on
> Orange Pi RV2 (SpacemiT K1) warning when lockdep is enabled.
> 
> WARNING: suspicious RCU usage
> drivers/net/phy/phy_device.c:1831 suspicious rcu_dereference_protected() usage!
> Call Trace:
> phy_detach_internal+0x490/0x5f2
> phy_disconnect+0x90/0xc0
> emac_stop+0x114/0x626
> emac_suspend+0xac/0x126
> platform_pm_suspend+0x72/0xe4
> 
> Hold RTNL lock before emac_open() and emac_stop() with rtnl_lock() and
> release the lock when functions return with rtnl_unlock().

This would work, but I am wondering if it would be possible to call rtnl
inside the _open function, when dealing with the PHY? in other words,
shrink the time rtnl is held.

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Vadim Fedorenko]

On 05/11/2026 13:40, Breno Leitao wrote:
> On Thu, Nov 05, 2026 at 05:34:37PM +0500, Ali Tariq wrote:
>> The emac_resume() calls the emac_open() and emac_suspend() calls
>> emac_stop() functions directly without holding the lock through
>> rtnl_lock(). These functions rely on phy_disconnect() which reads pointer
>> with rtnl_dereference(). But since no lock is being held,
>> rtnl_dereference throws a suspicious RCU usage on
>> Orange Pi RV2 (SpacemiT K1) warning when lockdep is enabled.
>>
>> WARNING: suspicious RCU usage
>> drivers/net/phy/phy_device.c:1831 suspicious rcu_dereference_protected() usage!
>> Call Trace:
>> phy_detach_internal+0x490/0x5f2
>> phy_disconnect+0x90/0xc0
>> emac_stop+0x114/0x626
>> emac_suspend+0xac/0x126
>> platform_pm_suspend+0x72/0xe4
>>
>> Hold RTNL lock before emac_open() and emac_stop() with rtnl_lock() and
>> release the lock when functions return with rtnl_unlock().
> 
> This would work, but I am wondering if it would be possible to call rtnl
> inside the _open function, when dealing with the PHY? in other words,
> shrink the time rtnl is held.

+1, especially in emac_open() rtnl is needed only in error path case

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

On Thu, 5 Nov 2026 23:28:31 +0100, Vadim Fedorenko wrote:
> > This would work, but I am wondering if it would be possible to call
> > rtnl inside the _open function, when dealing with the PHY? in other
> > words, shrink the time rtnl is held.
>
> +1, especially in emac_open() rtnl is needed only in error path case

emac_open() and emac_stop() are also the driver's .ndo_open and
.ndo_stop, which the core already calls with RTNL held, see
Documentation/networking/netdevices.rst +214. Taking rtnl_lock()
inside them would deadlock on every open and close, so the lock has to
stay in the PM callbacks.

-- 
Troy Mitchell

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On Thu,  5 Nov 2026 17:34:37 +0500, Ali Tariq wrote:
> @@ -2056,7 +2059,9 @@ static int emac_suspend(struct device *dev)
>  		return 0;
>  	}
>  
> +	rtnl_lock();
>  	emac_stop(ndev);
> +	rtnl_unlock();
>  
>  	clk_disable_unprepare(priv->bus_clk);
>  	netif_device_detach(ndev);
This can deadlock. emac_stop() -> emac_down() calls
cancel_work_sync(&priv->tx_timeout_task) with RTNL now held, while
emac_tx_timeout_task() starts with rtnl_lock(). 

-- 
Troy Mitchell

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Ali Tariq]

On 11/6/26 2:43 PM, Troy Mitchell wrote:
> On Thu,  5 Nov 2026 17:34:37 +0500, Ali Tariq wrote:
>> @@ -2056,7 +2059,9 @@ static int emac_suspend(struct device *dev)
>>   		return 0;
>>   	}
>>   
>> +	rtnl_lock();
>>   	emac_stop(ndev);
>> +	rtnl_unlock();
>>   
>>   	clk_disable_unprepare(priv->bus_clk);
>>   	netif_device_detach(ndev);
> This can deadlock. emac_stop() -> emac_down() calls
> cancel_work_sync(&priv->tx_timeout_task) with RTNL now held, while
> emac_tx_timeout_task() starts with rtnl_lock().
> 

Thanks Troy, that makes sense.

For the deadlock, I think the correct function to call is rtnl_trylock() 
inside the emac_tx_timeout_task(), which would return if the lock is 
already held and will hold the lock if it is not.

What are your thoughts on this? Does a v2 changing rtnl_lock() to 
rtnl_trylock() inside emac_tx_timeout_task() make sense?

Regards,
Ali

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Breno Leitao]

On Fri, Nov 06, 2026 at 09:41:31AM +0000, Troy Mitchell wrote:
> On Thu, 5 Nov 2026 23:28:31 +0100, Vadim Fedorenko wrote:
> > > This would work, but I am wondering if it would be possible to call
> > > rtnl inside the _open function, when dealing with the PHY? in other
> > > words, shrink the time rtnl is held.
> >
> > +1, especially in emac_open() rtnl is needed only in error path case
> 
> emac_open() and emac_stop() are also the driver's .ndo_open and
> .ndo_stop, which the core already calls with RTNL held, see
> Documentation/networking/netdevices.rst +214. Taking rtnl_lock()
> inside them would deadlock on every open and close, so the lock has to
> stay in the PM callbacks.

That makes sense to me.

Reviewed-by: Breno Leitao <leitao@debian.org>

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

On Fri, 6 Nov 2026 15:44:50 +0500, Ali Tariq wrote:
> For the deadlock, I think the correct function to call is
> rtnl_trylock() inside the emac_tx_timeout_task(), which would return
> if the lock is already held and will hold the lock if it is not.
>
> What are your thoughts on this? Does a v2 changing rtnl_lock() to
> rtnl_trylock() inside emac_tx_timeout_task() make sense?
rtnl_trylock() there is fine, it fixes the suspend case.

Looking at it, I found an existing bug though: once the work holds RTNL
it deadlocks on itself, on every TX timeout:

  emac_tx_timeout_task()
    dev_close()
      emac_stop()
        emac_down()
          cancel_work_sync(&priv->tx_timeout_task)  <- waits for itself

rtnl_trylock() does not help with that. Please fix it in a separate
patch ahead of this one, e.g. by resetting in place instead of going
through dev_close()/dev_open(). Feel free to add:
Reported-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>

-- 
Troy Mitchell

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Fri Nov 6, 2026 at 11:54 AM GMT, Breno Leitao wrote:
> On Fri, Nov 06, 2026 at 09:41:31AM +0000, Troy Mitchell wrote:
>> On Thu, 5 Nov 2026 23:28:31 +0100, Vadim Fedorenko wrote:
>> > > This would work, but I am wondering if it would be possible to call
>> > > rtnl inside the _open function, when dealing with the PHY? in other
>> > > words, shrink the time rtnl is held.
>> >
>> > +1, especially in emac_open() rtnl is needed only in error path case
>> 
>> emac_open() and emac_stop() are also the driver's .ndo_open and
>> .ndo_stop, which the core already calls with RTNL held, see
>> Documentation/networking/netdevices.rst +214. Taking rtnl_lock()
>> inside them would deadlock on every open and close, so the lock has to
>> stay in the PM callbacks.
>
> That makes sense to me.
>
> Reviewed-by: Breno Leitao <leitao@debian.org>
This tag is deprecated. Version 2 will use a different approach [1]

Link: https://lore.kernel.org/all/c55c5878-d9c1-4863-bc55-99d334ba2ede@gmail.com/ [1]


-- 
Troy Mitchell


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Andrew Lunn]

On Fri, Nov 06, 2026 at 03:44:50PM +0500, Ali Tariq wrote:
> 
> 
> On 11/6/26 2:43 PM, Troy Mitchell wrote:
> > On Thu,  5 Nov 2026 17:34:37 +0500, Ali Tariq wrote:
> > > @@ -2056,7 +2059,9 @@ static int emac_suspend(struct device *dev)
> > >   		return 0;
> > >   	}
> > > +	rtnl_lock();
> > >   	emac_stop(ndev);
> > > +	rtnl_unlock();
> > >   	clk_disable_unprepare(priv->bus_clk);
> > >   	netif_device_detach(ndev);
> > This can deadlock. emac_stop() -> emac_down() calls
> > cancel_work_sync(&priv->tx_timeout_task) with RTNL now held, while
> > emac_tx_timeout_task() starts with rtnl_lock().
> > 
> 
> Thanks Troy, that makes sense.
> 
> For the deadlock, I think the correct function to call is rtnl_trylock()
> inside the emac_tx_timeout_task(), which would return if the lock is already
> held and will hold the lock if it is not.

rtnl_trylock() is generally a workaround needed for badly written
code. Please try to avoid it.

	Andrew

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

On Sat Nov 7, 2026 at 5:32 PM GMT, Andrew Lunn wrote:
> On Fri, Nov 06, 2026 at 03:44:50PM +0500, Ali Tariq wrote:
>> 
>> 
>> On 11/6/26 2:43 PM, Troy Mitchell wrote:
>> > On Thu,  5 Nov 2026 17:34:37 +0500, Ali Tariq wrote:
>> > > @@ -2056,7 +2059,9 @@ static int emac_suspend(struct device *dev)
>> > >   		return 0;
>> > >   	}
>> > > +	rtnl_lock();
>> > >   	emac_stop(ndev);
>> > > +	rtnl_unlock();
>> > >   	clk_disable_unprepare(priv->bus_clk);
>> > >   	netif_device_detach(ndev);
>> > This can deadlock. emac_stop() -> emac_down() calls
>> > cancel_work_sync(&priv->tx_timeout_task) with RTNL now held, while
>> > emac_tx_timeout_task() starts with rtnl_lock().
>> > 
>> 
>> Thanks Troy, that makes sense.
>> 
>> For the deadlock, I think the correct function to call is rtnl_trylock()
>> inside the emac_tx_timeout_task(), which would return if the lock is already
>> held and will hold the lock if it is not.
>
> rtnl_trylock() is generally a workaround needed for badly written
> code. Please try to avoid it.
Agreed. If the reset is done in place, the work does not need RTNL at all,
so .ndo_stop can keep cancel_work_sync() as is.


-- 
Troy Mitchell


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Vivian Wang]

On 11/8/26 15:19, Troy Mitchell wrote:

> On Sat Nov 7, 2026 at 5:32 PM GMT, Andrew Lunn wrote:
>> On Fri, Nov 06, 2026 at 03:44:50PM +0500, Ali Tariq wrote:
>>>
>>> On 11/6/26 2:43 PM, Troy Mitchell wrote:
>>>> On Thu,  5 Nov 2026 17:34:37 +0500, Ali Tariq wrote:
>>>>> @@ -2056,7 +2059,9 @@ static int emac_suspend(struct device *dev)
>>>>>   		return 0;
>>>>>   	}
>>>>> +	rtnl_lock();
>>>>>   	emac_stop(ndev);
>>>>> +	rtnl_unlock();
>>>>>   	clk_disable_unprepare(priv->bus_clk);
>>>>>   	netif_device_detach(ndev);
>>>> This can deadlock. emac_stop() -> emac_down() calls
>>>> cancel_work_sync(&priv->tx_timeout_task) with RTNL now held, while
>>>> emac_tx_timeout_task() starts with rtnl_lock().
>>>>
>>> Thanks Troy, that makes sense.
>>>
>>> For the deadlock, I think the correct function to call is rtnl_trylock()
>>> inside the emac_tx_timeout_task(), which would return if the lock is already
>>> held and will hold the lock if it is not.
>> rtnl_trylock() is generally a workaround needed for badly written
>> code. Please try to avoid it.
> Agreed. If the reset is done in place, the work does not need RTNL at all,
> so .ndo_stop can keep cancel_work_sync() as is.
>
Thanks for the diagnosis and suggestions.

Although, maybe we could simply avoid cancel_work_sync() in emac_down()?
emac_tx_timeout_task() checks for !netif_running and does nothing if
already down so it should be safe. The task cancel_work_sync() in
emac_remove() does cleanup on unbind so the task is not leaked.

This seems to be the pattern in other drivers as well. What do you think?

Vivian "dramforever" Wang

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]

On Mon Nov 9, 2026 at 7:59 AM GMT, Vivian Wang wrote:
> On 11/8/26 15:19, Troy Mitchell wrote:
>
>> On Sat Nov 7, 2026 at 5:32 PM GMT, Andrew Lunn wrote:
>>> On Fri, Nov 06, 2026 at 03:44:50PM +0500, Ali Tariq wrote:
>>>>
>>>> On 11/6/26 2:43 PM, Troy Mitchell wrote:
>>>>> On Thu,  5 Nov 2026 17:34:37 +0500, Ali Tariq wrote:
>>>>>> @@ -2056,7 +2059,9 @@ static int emac_suspend(struct device *dev)
>>>>>>   		return 0;
>>>>>>   	}
>>>>>> +	rtnl_lock();
>>>>>>   	emac_stop(ndev);
>>>>>> +	rtnl_unlock();
>>>>>>   	clk_disable_unprepare(priv->bus_clk);
>>>>>>   	netif_device_detach(ndev);
>>>>> This can deadlock. emac_stop() -> emac_down() calls
>>>>> cancel_work_sync(&priv->tx_timeout_task) with RTNL now held, while
>>>>> emac_tx_timeout_task() starts with rtnl_lock().
>>>>>
>>>> Thanks Troy, that makes sense.
>>>>
>>>> For the deadlock, I think the correct function to call is rtnl_trylock()
>>>> inside the emac_tx_timeout_task(), which would return if the lock is already
>>>> held and will hold the lock if it is not.
>>> rtnl_trylock() is generally a workaround needed for badly written
>>> code. Please try to avoid it.
>> Agreed. If the reset is done in place, the work does not need RTNL at all,
>> so .ndo_stop can keep cancel_work_sync() as is.
>>
> Thanks for the diagnosis and suggestions.
>
> Although, maybe we could simply avoid cancel_work_sync() in emac_down()?
> emac_tx_timeout_task() checks for !netif_running and does nothing if
> already down so it should be safe. The task cancel_work_sync() in
> emac_remove() does cleanup on unbind so the task is not leaked.
>
> This seems to be the pattern in other drivers as well. What do you think?
That was my first thought too, but I'm not sure netif_running() is
enough here. emac_suspend() calls emac_stop() directly, so
netif_running() is still true and a pending work would stop the device
a second time. And since the netdev is devm-registered, emac_remove()
runs before unregister_netdev(), so the work could be requeued after
the cancel there.

So we'd still need to cancel it in suspend and remove, and at that
point resetting in place looks simpler to me. What do you think?


-- 
Troy Mitchell


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[netdev-bot+sashiko]

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

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

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).

- [Medium] Possible ABBA deadlock between rtnl_mutex and the TX-timeout
  work, added to the suspend path.
- [Medium] The new RTNL section does not cover the netif_running() check
  that decides whether to call emac_stop()/emac_open().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is High
  severity and involves the same work/RTNL pair.
- [High] This problem wasn't introduced by this patch, but it is High
  severity and sits in the function the patch modifies.

--

net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

This patch takes rtnl_lock() around the emac_open() call in emac_resume()
and around the emac_stop() call in emac_suspend(). The aim is to silence
a lockdep "suspicious RCU usage" warning from phy_disconnect(), which
reads a pointer with rtnl_dereference().

> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
> index 71a80838bffd0..aca25a9e1e9ef 100644
> --- a/drivers/net/ethernet/spacemit/k1_emac.c
> +++ b/drivers/net/ethernet/spacemit/k1_emac.c
> @@ -2033,7 +2033,10 @@ static int emac_resume(struct device *dev)
>  	if (!netif_running(ndev))
>  		return 0;
>  
> +	rtnl_lock();
>  	ret = emac_open(ndev);
> +	rtnl_unlock();
> +
>  	if (ret) {
>  		clk_disable_unprepare(priv->bus_clk);
>  		return ret;

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. It sits
in the function being changed, though. What state is the device left
in when emac_open() fails here?

By this point the unwind in emac_open() has freed the TX/RX rings. One
of two things has also happened:

- emac_up() failed, and its error path called phy_disconnect() and
  pm_runtime_put_sync().
- emac_phy_connect() failed, and ndev->phydev is still NULL from the
  previous suspend.

The IRQ is not requested and NAPI is disabled. emac_resume() then gates
bus_clk and returns, but __LINK_STATE_START and IFF_UP are still set.

The next "ip link set down", unregister, or emac_suspend() (which sees
netif_running() as true) would then run:

emac_stop()
  emac_down()
    phy_disconnect(ndev->phydev)   <- phydev is NULL
      phy_is_started(phydev)       <- dereferences phydev->state

Can this oops?

Past that point, emac_down() would:

- write MAC registers with the bus clock gated
- call free_irq() on an IRQ that is not requested
- call napi_disable() on a NAPI that is already disabled
- underflow the runtime PM usage count

Also, emac_clean_rx_desc_ring() would index the NULL rx_desc_buf, and
emac_suspend() would call clk_disable_unprepare(priv->bus_clk) a second
time.

Separately, emac_alloc_tx_resources() and emac_alloc_rx_resources()
kfree() the buffer array when dma_alloc_coherent() fails, but they do
not set the pointer to NULL:

	if (!tx_ring->desc_addr) {
		kfree(tx_ring->tx_desc_buf);
		return -ENOMEM;
	}

Could a later teardown free tx_desc_buf (or rx_desc_buf) a second time
through that stale pointer?

> @@ -2056,7 +2059,9 @@ static int emac_suspend(struct device *dev)
>  		return 0;
>  	}
>  
> +	rtnl_lock();
>  	emac_stop(ndev);
> +	rtnl_unlock();

[Severity: Medium]
emac_suspend() checks netif_running() to decide whether to call
emac_stop(). That check runs before rtnl_lock() and is not repeated
under the lock:

	if (!ndev || !netif_running(ndev)) {
		clk_disable_unprepare(priv->bus_clk);
		return 0;
	}

	rtnl_lock();
	emac_stop(ndev);

Can another RTNL holder close the device in between?

Here is one example. The netdev is moved into a netns, and that netns
is torn down as system sleep starts. cleanup_net runs on netns_wq,
which is not freezable. It holds RTNL and calls dev_close(), which
calls emac_stop().

Meanwhile emac_suspend() has already seen netif_running() as true. It
blocks in rtnl_lock(), then runs emac_stop() a second time. There,
emac_down() calls phy_disconnect(ndev->phydev) with phydev already
NULL, and phy_is_started() dereferences it.

emac_resume() has the mirror case. It also tests netif_running() before
rtnl_lock(), so it could call emac_open() on a device that was just
closed. That would leak the rings, the IRQ and the PHY attach.

Would it make sense to take rtnl_lock() before the netif_running()
test, with netif_device_detach() / netif_device_attach() inside the
same locked section?

[Severity: Medium]
Does holding RTNL here invert the lock order with tx_timeout_task?
emac_stop()->emac_down() does:

	cancel_work_sync(&priv->tx_timeout_task);

and the work handler takes RTNL first:

emac_tx_timeout_task() {
	...
	rtnl_lock();
	...
}

emac_tx_timeout() queues the work with schedule_work(). That uses the
system workqueue, which is not freezable, so the work can run during
system suspend:

CPU1 (worker)                      CPU2 (suspend)
emac_tx_timeout_task()
                                   emac_suspend()
                                     rtnl_lock()
  rtnl_lock()  <- blocks
                                     emac_stop()
                                       emac_down()
                                         cancel_work_sync()
                                           <- waits for CPU1

cancel_work_sync() goes through __flush_work()->start_flush_work().
That finds the executing worker with find_worker_executing_work() and
waits on a barrier queued behind it.

Can this hang suspend? Lockdep may also report a circular dependency
between rtnl_mutex and the work's lockdep_map, which would swap one
splat for another.

The ndo_stop path (dev_close() under RTNL, then emac_stop()) already
has this pattern. This patch adds it to the system sleep path too.

Could the synchronous cancel be kept out of any RTNL-held section?
Options include cancelling the work before taking RTNL, using
rtnl_trylock() and requeueing in the work, or a disable_work() based
scheme.

[Severity: High]
This isn't a bug introduced by this patch, but it involves the same
work and RTNL pair. Can emac_tx_timeout_task() wait on itself?

emac_tx_timeout_task()          <- running as priv->tx_timeout_task
  rtnl_lock()
  dev_close(ndev)
    __dev_close_many()
      emac_stop()               <- ndo_stop
        emac_down()
          cancel_work_sync(&priv->tx_timeout_task)

At this point the work is executing, not queued. start_flush_work()
finds the calling worker itself through find_worker_executing_work().
It then inserts a barrier that can only run after the current work
returns, so wait_for_completion() in __flush_work() never returns.

Would every TX timeout recovery leave the worker stuck while holding
rtnl_mutex? That would block every RTNL user. With this patch, the next
emac_suspend() would also block forever in its new rtnl_lock().

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