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

[Vivian Wang <wangruikang@iscas.ac.cn>]

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