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

[Ali Tariq <alitariq45892@gmail.com>]

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.

If you still prefer resetting in place, please let me know.

Regards,
Ali

pw-bot: cr