[lvc-project] [PATCH 1/2] [v2] wifi: brcmfmac: handle possible completion timeouts

[Arend van Spriel]

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 at broadcom.com>
> Signed-off-by: Dmitry Antipov <dmantipov at 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);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4219 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://linuxtesting.org/pipermail/lvc-project/attachments/20240118/87990ea1/attachment.bin>