[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()

[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: 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()

[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()

[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()

[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 --]