Hi Fabio, On 9/13/21 3:39 PM, Fabio Aiuto wrote: > Hello Hans, > > On Mon, Sep 13, 2021 at 03:24:44PM +0200, Hans de Goede wrote: >> Hi Fabio, >> > >>> Note that sleep_q.lock is already taken inside >>> rtw_free_xmitframe_queue so we just wrap sleepq_len >>> access. >>> >>> Moved pxmitpriv->lock after sleep_q.lock release to >>> avoid locks nesting. > >>> rtw_free_xmitframe_queue(pxmitpriv, &psta->sleep_q); >>> + spin_lock_bh(&psta->sleep_q.lock); >> >> AFAICT this needs to be above the rtw_free_xmitframe_queue() ? > > as I wrote on the changelog, the sleep_q.lock is already > taken inside rtw_free_xmitframe_queue. If I put the > sleep_q.lock above that function a soft lock occurs when > I disconnect. > > So I put it just below rtw_free_xmitframe_queue. > > Things works fine this way. > > Please tell me if there's a best way to do it. Hmm I see, this may work, but the sleepq_len access really should be protected by the same lock as the freeing of the queue is without dropping it in between. That rtw_free_xmitframe_queue() takes the sleep_q.lock then to me that signals that other (higher-level) functions should not take sleep_q.lock at all, since this is then private to the functions operating on the sleep_q. I've an idea how we we can possibly tackle this, but I'm not sure yet I will try to make some time to look into this tomorrow or the day after. Regards, Hans
