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: 3577 bytes --]

Hi Ali,

On Thu Nov 12, 2026 at 1:33 PM GMT, Ali Tariq wrote:
>
>
> On 11/9/26 1:10 PM, Troy Mitchell wrote:
>> 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?
>> 
>> 
>
> Thanks Troy, Vivian and Andrew. I'll drop rtnl_trylock().
>
> I think a simpler approach here will be to go with Vivian's suggestion
> of dropping cancel_work_sync() from emac_down(), and handle Troy's two
> cases by disabling the work where the device is stopped without the core:
>
> 1. Suspend: emac_suspend() calls disable_work_sync() before stopping
>     the device, and emac_resume() calls enable_work(). So the work
>     can't run while the device is suspended.
>
> 2. Remove: emac_remove() uses disable_work_sync() instead of
>     cancel_work_sync(), so the work can't be queued again before the
>     devm unregister_netdev().
>
> For a normal close, the work's existing netif_running() check is
> enough, because the core clears it before calling .ndo_stop.
>
> With this, nothing waits for the work while holding RTNL, and the
> work doesn't wait for itself.
>
> v2 will have two patches. First the TX timeout fix (reported by Troy) 
> then the suspend/resume patch, which will take
> RTNL after disable_work_sync() and before the netif_running() checks.
Sounds good :)


-- 
Troy Mitchell


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