Hi Greg, On Fri, Sep 17, 2021 at 04:25:12PM +0200, Greg KH wrote: > On Mon, Sep 13, 2021 at 05:12:19PM +0200, Hans de Goede wrote: > > 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. > > I'm just going to go and revert the original change here until you all > can sort it out :) that's the best thing for now ;) > > thanks, > > greg k-h thank you, fabio
