* [PATCH] staging: rtl8723bs: protect sleepq_len access by sleep_q.lock @ 2021-09-13 13:03 Fabio Aiuto 2021-09-13 13:24 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Fabio Aiuto @ 2021-09-13 13:03 UTC (permalink / raw) To: gregkh; +Cc: hdegoede, Larry.Finger, linux-staging, linux-kernel, Fabio Aiuto protect sleepq_len access by sleep_q.lock and move pxmitpriv->lock after sleep_q.lock release. This fixes and completes a lockdep warning silencing done in a prevoius commit where accesses to sleep_q related fields were protected by sleep_q.lock instead of pxmitpriv->lock. 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. Fixes: 78a1614a81f0 ("staging: rtl8723bs: remove possible deadlock when disconnect") Reported-by: Hans de Goede <hdegoede@redhat.com> Tested-by: Fabio Aiuto <fabioaiuto83@gmail.com> Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com> --- drivers/staging/rtl8723bs/core/rtw_sta_mgt.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c b/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c index bf090f3b1db6..c98918e02afe 100644 --- a/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c +++ b/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c @@ -294,10 +294,12 @@ u32 rtw_free_stainfo(struct adapter *padapter, struct sta_info *psta) /* list_del_init(&psta->wakeup_list); */ - spin_lock_bh(&pxmitpriv->lock); - rtw_free_xmitframe_queue(pxmitpriv, &psta->sleep_q); + spin_lock_bh(&psta->sleep_q.lock); psta->sleepq_len = 0; + spin_unlock_bh(&psta->sleep_q.lock); + + spin_lock_bh(&pxmitpriv->lock); /* vo */ /* spin_lock_bh(&(pxmitpriv->vo_pending.lock)); */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: protect sleepq_len access by sleep_q.lock 2021-09-13 13:03 [PATCH] staging: rtl8723bs: protect sleepq_len access by sleep_q.lock Fabio Aiuto @ 2021-09-13 13:24 ` Hans de Goede 2021-09-13 13:39 ` Fabio Aiuto 0 siblings, 1 reply; 6+ messages in thread From: Hans de Goede @ 2021-09-13 13:24 UTC (permalink / raw) To: Fabio Aiuto, gregkh; +Cc: Larry.Finger, linux-staging, linux-kernel Hi Fabio, On 9/13/21 3:03 PM, Fabio Aiuto wrote: > protect sleepq_len access by sleep_q.lock and move > pxmitpriv->lock after sleep_q.lock release. > > This fixes and completes a lockdep warning silencing > done in a prevoius commit where accesses to sleep_q > related fields were protected by sleep_q.lock instead > of pxmitpriv->lock. > > 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. > > Fixes: 78a1614a81f0 ("staging: rtl8723bs: remove possible deadlock when disconnect") > Reported-by: Hans de Goede <hdegoede@redhat.com> > Tested-by: Fabio Aiuto <fabioaiuto83@gmail.com> > Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com> > --- > drivers/staging/rtl8723bs/core/rtw_sta_mgt.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c b/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c > index bf090f3b1db6..c98918e02afe 100644 > --- a/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c > +++ b/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c > @@ -294,10 +294,12 @@ u32 rtw_free_stainfo(struct adapter *padapter, struct sta_info *psta) > > /* list_del_init(&psta->wakeup_list); */ > > - spin_lock_bh(&pxmitpriv->lock); > - > 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() ? Regards, Hans > psta->sleepq_len = 0; > + spin_unlock_bh(&psta->sleep_q.lock); > + > + spin_lock_bh(&pxmitpriv->lock); > > /* vo */ > /* spin_lock_bh(&(pxmitpriv->vo_pending.lock)); */ > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: protect sleepq_len access by sleep_q.lock 2021-09-13 13:24 ` Hans de Goede @ 2021-09-13 13:39 ` Fabio Aiuto 2021-09-13 15:12 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Fabio Aiuto @ 2021-09-13 13:39 UTC (permalink / raw) To: Hans de Goede; +Cc: gregkh, Larry.Finger, linux-staging, linux-kernel 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. > > Regards, > > Hans thank you, fabio ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: protect sleepq_len access by sleep_q.lock 2021-09-13 13:39 ` Fabio Aiuto @ 2021-09-13 15:12 ` Hans de Goede 2021-09-17 14:25 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Hans de Goede @ 2021-09-13 15:12 UTC (permalink / raw) To: Fabio Aiuto; +Cc: gregkh, Larry.Finger, linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: protect sleepq_len access by sleep_q.lock 2021-09-13 15:12 ` Hans de Goede @ 2021-09-17 14:25 ` Greg KH 2021-09-17 14:27 ` Fabio Aiuto 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2021-09-17 14:25 UTC (permalink / raw) To: Hans de Goede; +Cc: Fabio Aiuto, Larry.Finger, linux-staging, linux-kernel 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 :) thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: protect sleepq_len access by sleep_q.lock 2021-09-17 14:25 ` Greg KH @ 2021-09-17 14:27 ` Fabio Aiuto 0 siblings, 0 replies; 6+ messages in thread From: Fabio Aiuto @ 2021-09-17 14:27 UTC (permalink / raw) To: Greg KH; +Cc: Hans de Goede, Larry.Finger, linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-17 14:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-13 13:03 [PATCH] staging: rtl8723bs: protect sleepq_len access by sleep_q.lock Fabio Aiuto 2021-09-13 13:24 ` Hans de Goede 2021-09-13 13:39 ` Fabio Aiuto 2021-09-13 15:12 ` Hans de Goede 2021-09-17 14:25 ` Greg KH 2021-09-17 14:27 ` Fabio Aiuto
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.