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

["Troy Mitchell" <troy.mitchell@linux.spacemit.com>]

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