| Message ID | 20230614075848.80536-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/2,v2] wifi: brcmfmac: handle possible completion timeouts | expand |
Dmitry Antipov <dmantipov@yandex.ru> wrote: > Handle possible 'wait_for_completion_timeout()' errors in > 'brcmf_p2p_af_searching_channel()', 'brcmf_p2p_tx_action_frame()' > and 'brcmf_p2p_del_vif()', adjust related code. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> This is not simple cleanup and I feel that these should be tested on a real device. 2 patches set to Changes Requested. 13279707 [1/2,v2] wifi: brcmfmac: handle possible completion timeouts 13279708 [2/2,v2] wifi: brcmfmac: handle possible MSI enabling error
On 1/18/2024 12:22 PM, Kalle Valo wrote: > Dmitry Antipov <dmantipov@yandex.ru> wrote: > >> Handle possible 'wait_for_completion_timeout()' errors in >> 'brcmf_p2p_af_searching_channel()', 'brcmf_p2p_tx_action_frame()' >> and 'brcmf_p2p_del_vif()', adjust related code. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > > This is not simple cleanup and I feel that these should be tested on a > real device. P2P testing. Ouch. Let me first have a closer look at the patches ;-) > 2 patches set to Changes Requested. > > 13279707 [1/2,v2] wifi: brcmfmac: handle possible completion timeouts > 13279708 [2/2,v2] wifi: brcmfmac: handle possible MSI enabling error >
On 6/14/2023 9:58 AM, Dmitry Antipov wrote: > Handle possible 'wait_for_completion_timeout()' errors in > 'brcmf_p2p_af_searching_channel()', 'brcmf_p2p_tx_action_frame()' > and 'brcmf_p2p_del_vif()', adjust related code. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. Thanks for adding this exception handling. Please consider suggestions below. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > v2: rebase against wireless-next tree > --- > .../broadcom/brcm80211/brcmfmac/p2p.c | 31 +++++++++++++------ > 1 file changed, 21 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c > index d4492d02e4ea..e43dabdaeb0b 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c > @@ -1151,6 +1151,7 @@ static s32 brcmf_p2p_af_searching_channel(struct brcmf_p2p_info *p2p) > { > struct afx_hdl *afx_hdl = &p2p->afx_hdl; > struct brcmf_cfg80211_vif *pri_vif; > + bool timeout = false; > s32 retry; > > brcmf_dbg(TRACE, "Enter\n"); > @@ -1173,8 +1174,11 @@ static s32 brcmf_p2p_af_searching_channel(struct brcmf_p2p_info *p2p) > retry); > /* search peer on peer's listen channel */ > schedule_work(&afx_hdl->afx_work); > - wait_for_completion_timeout(&afx_hdl->act_frm_scan, > - P2P_AF_FRM_SCAN_MAX_WAIT); > + if (!wait_for_completion_timeout(&afx_hdl->act_frm_scan, > + P2P_AF_FRM_SCAN_MAX_WAIT)) { > + timeout = true; > + break; > + } Instead could do: timeout = !wait_for_completion_timeout(...); if (timeout) break; > if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) || > (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL, > &p2p->status))) > @@ -1186,8 +1190,11 @@ static s32 brcmf_p2p_af_searching_channel(struct brcmf_p2p_info *p2p) > /* listen on my listen channel */ > afx_hdl->is_listen = true; > schedule_work(&afx_hdl->afx_work); > - wait_for_completion_timeout(&afx_hdl->act_frm_scan, > - P2P_AF_FRM_SCAN_MAX_WAIT); > + if (!wait_for_completion_timeout > + (&afx_hdl->act_frm_scan, P2P_AF_FRM_SCAN_MAX_WAIT)) { > + timeout = true; > + break; > + } dito > } > if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) || > (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL, > @@ -1209,7 +1216,7 @@ static s32 brcmf_p2p_af_searching_channel(struct brcmf_p2p_info *p2p) > > clear_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL, &p2p->status); > > - return afx_hdl->peer_chan; > + return timeout ? P2P_INVALID_CHANNEL : afx_hdl->peer_chan; > } > > > @@ -1580,14 +1587,18 @@ static s32 brcmf_p2p_tx_action_frame(struct brcmf_p2p_info *p2p, > (p2p->wait_for_offchan_complete) ? > "off-channel" : "on-channel"); > > - wait_for_completion_timeout(&p2p->send_af_done, P2P_AF_MAX_WAIT_TIME); > - > + if (!wait_for_completion_timeout(&p2p->send_af_done, > + P2P_AF_MAX_WAIT_TIME)) { > + err = -ETIMEDOUT; > + goto clear; > + } Not really needed as timeout would cause the code to proceed in the else branch below. > if (test_bit(BRCMF_P2P_STATUS_ACTION_TX_COMPLETED, &p2p->status)) { > brcmf_dbg(TRACE, "TX action frame operation is success\n"); > } else { > err = -EIO; > brcmf_dbg(TRACE, "TX action frame operation has failed\n"); > } > +clear: > /* clear status bit for action tx */ > clear_bit(BRCMF_P2P_STATUS_ACTION_TX_COMPLETED, &p2p->status); > clear_bit(BRCMF_P2P_STATUS_ACTION_TX_NOACK, &p2p->status); > @@ -2404,10 +2415,10 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev) > brcmf_dbg(INFO, "P2P: GO_NEG_PHASE status cleared\n"); > > if (wait_for_disable) > - wait_for_completion_timeout(&cfg->vif_disabled, > - BRCMF_P2P_DISABLE_TIMEOUT); > + err = (wait_for_completion_timeout(&cfg->vif_disabled, > + BRCMF_P2P_DISABLE_TIMEOUT) > + ? 0 : -ETIMEDOUT); > > - err = 0; For P2P_DEVICE wait_for_disable is false so err would be uninitialized here when removing the line above. Looking at the function wait_for_disable is set to true for non-P2P_DEVICE so the wait can move inside the if statement below. > if (iftype != NL80211_IFTYPE_P2P_DEVICE) { > brcmf_vif_clear_mgmt_ies(vif); > err = brcmf_p2p_release_p2p_if(vif);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c index d4492d02e4ea..e43dabdaeb0b 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c @@ -1151,6 +1151,7 @@ static s32 brcmf_p2p_af_searching_channel(struct brcmf_p2p_info *p2p) { struct afx_hdl *afx_hdl = &p2p->afx_hdl; struct brcmf_cfg80211_vif *pri_vif; + bool timeout = false; s32 retry; brcmf_dbg(TRACE, "Enter\n"); @@ -1173,8 +1174,11 @@ static s32 brcmf_p2p_af_searching_channel(struct brcmf_p2p_info *p2p) retry); /* search peer on peer's listen channel */ schedule_work(&afx_hdl->afx_work); - wait_for_completion_timeout(&afx_hdl->act_frm_scan, - P2P_AF_FRM_SCAN_MAX_WAIT); + if (!wait_for_completion_timeout(&afx_hdl->act_frm_scan, + P2P_AF_FRM_SCAN_MAX_WAIT)) { + timeout = true; + break; + } if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) || (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL, &p2p->status))) @@ -1186,8 +1190,11 @@ static s32 brcmf_p2p_af_searching_channel(struct brcmf_p2p_info *p2p) /* listen on my listen channel */ afx_hdl->is_listen = true; schedule_work(&afx_hdl->afx_work); - wait_for_completion_timeout(&afx_hdl->act_frm_scan, - P2P_AF_FRM_SCAN_MAX_WAIT); + if (!wait_for_completion_timeout + (&afx_hdl->act_frm_scan, P2P_AF_FRM_SCAN_MAX_WAIT)) { + timeout = true; + break; + } } if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) || (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL, @@ -1209,7 +1216,7 @@ static s32 brcmf_p2p_af_searching_channel(struct brcmf_p2p_info *p2p) clear_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL, &p2p->status); - return afx_hdl->peer_chan; + return timeout ? P2P_INVALID_CHANNEL : afx_hdl->peer_chan; } @@ -1580,14 +1587,18 @@ static s32 brcmf_p2p_tx_action_frame(struct brcmf_p2p_info *p2p, (p2p->wait_for_offchan_complete) ? "off-channel" : "on-channel"); - wait_for_completion_timeout(&p2p->send_af_done, P2P_AF_MAX_WAIT_TIME); - + if (!wait_for_completion_timeout(&p2p->send_af_done, + P2P_AF_MAX_WAIT_TIME)) { + err = -ETIMEDOUT; + goto clear; + } if (test_bit(BRCMF_P2P_STATUS_ACTION_TX_COMPLETED, &p2p->status)) { brcmf_dbg(TRACE, "TX action frame operation is success\n"); } else { err = -EIO; brcmf_dbg(TRACE, "TX action frame operation has failed\n"); } +clear: /* clear status bit for action tx */ clear_bit(BRCMF_P2P_STATUS_ACTION_TX_COMPLETED, &p2p->status); clear_bit(BRCMF_P2P_STATUS_ACTION_TX_NOACK, &p2p->status); @@ -2404,10 +2415,10 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev) brcmf_dbg(INFO, "P2P: GO_NEG_PHASE status cleared\n"); if (wait_for_disable) - wait_for_completion_timeout(&cfg->vif_disabled, - BRCMF_P2P_DISABLE_TIMEOUT); + err = (wait_for_completion_timeout(&cfg->vif_disabled, + BRCMF_P2P_DISABLE_TIMEOUT) + ? 0 : -ETIMEDOUT); - err = 0; if (iftype != NL80211_IFTYPE_P2P_DEVICE) { brcmf_vif_clear_mgmt_ies(vif); err = brcmf_p2p_release_p2p_if(vif);
Handle possible 'wait_for_completion_timeout()' errors in 'brcmf_p2p_af_searching_channel()', 'brcmf_p2p_tx_action_frame()' and 'brcmf_p2p_del_vif()', adjust related code. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- v2: rebase against wireless-next tree --- .../broadcom/brcm80211/brcmfmac/p2p.c | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-)