Messages in this thread | | | Subject | Re: [PATCH] staging: rtl8723bs: protect sleepq_len access by sleep_q.lock | From | Hans de Goede <> | Date | Mon, 13 Sep 2021 17:12:19 +0200 |
| |
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
| |