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

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

[Dmitry Antipov]

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(-)

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);
-- 
2.40.1

[PATCH 2/2] [v2] wifi: brcmfmac: handle possible MSI enabling error

[Dmitry Antipov]

Handle possible 'pci_enable_msi()' error in
'brcmf_pcie_request_irq()', 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
---
 .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c  | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 80220685f5e4..f7d9f2cbd60b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -965,6 +965,7 @@ static irqreturn_t brcmf_pcie_isr_thread(int irq, void *arg)
 
 static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
 {
+	int ret;
 	struct pci_dev *pdev = devinfo->pdev;
 	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
 
@@ -972,16 +973,19 @@ static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
 
 	brcmf_dbg(PCIE, "Enter\n");
 
-	pci_enable_msi(pdev);
+	ret = pci_enable_msi(pdev);
+	if (ret)
+		return ret;
 	if (request_threaded_irq(pdev->irq, brcmf_pcie_quick_check_isr,
 				 brcmf_pcie_isr_thread, IRQF_SHARED,
 				 "brcmf_pcie_intr", devinfo)) {
 		pci_disable_msi(pdev);
 		brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);
-		return -EIO;
+		ret = -EIO;
+	} else {
+		devinfo->irq_allocated = true;
 	}
-	devinfo->irq_allocated = true;
-	return 0;
+	return ret;
 }
 
 
-- 
2.40.1

Re: [PATCH 1/2] [v2] wifi: brcmfmac: handle possible completion timeouts

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230614075848.80536-1-dmantipov@yandex.ru/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/2] [v2] wifi: brcmfmac: handle possible completion timeouts

[Arend Van Spriel]

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
> 

Re: [PATCH 1/2] [v2] wifi: brcmfmac: handle possible completion timeouts

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 4503 bytes --]

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);

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 2/2] [v2] wifi: brcmfmac: handle possible MSI enabling error

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

On 6/14/2023 9:58 AM, Dmitry Antipov wrote:
> Handle possible 'pci_enable_msi()' error in
> 'brcmf_pcie_request_irq()', adjust related code.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v2: rebase against wireless-next tree
> ---
>   .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c  | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index 80220685f5e4..f7d9f2cbd60b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -965,6 +965,7 @@ static irqreturn_t brcmf_pcie_isr_thread(int irq, void *arg)
>   
>   static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
>   {
> +	int ret;
>   	struct pci_dev *pdev = devinfo->pdev;
>   	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
>   
> @@ -972,16 +973,19 @@ static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
>   
>   	brcmf_dbg(PCIE, "Enter\n");
>   
> -	pci_enable_msi(pdev);
> +	ret = pci_enable_msi(pdev);
> +	if (ret)
> +		return ret;

The above is sufficient. Further adjustments not needed.

>   	if (request_threaded_irq(pdev->irq, brcmf_pcie_quick_check_isr,
>   				 brcmf_pcie_isr_thread, IRQF_SHARED,
>   				 "brcmf_pcie_intr", devinfo)) {
>   		pci_disable_msi(pdev);
>   		brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);
> -		return -EIO;
> +		ret = -EIO;
> +	} else {
> +		devinfo->irq_allocated = true;
>   	}
> -	devinfo->irq_allocated = true;
> -	return 0;
> +	return ret;
>   }
>   
>   

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 2/2] [v2] wifi: brcmfmac: handle possible MSI enabling error

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

+ Bjorn

On 1/18/2024 1:26 PM, Arend van Spriel wrote:
> On 6/14/2023 9:58 AM, Dmitry Antipov wrote:
>> Handle possible 'pci_enable_msi()' error in
>> 'brcmf_pcie_request_irq()', adjust related code.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
>> ---
>> v2: rebase against wireless-next tree
>> ---
>>   .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c  | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
>> index 80220685f5e4..f7d9f2cbd60b 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
>> @@ -965,6 +965,7 @@ static irqreturn_t brcmf_pcie_isr_thread(int irq, 
>> void *arg)
>>   static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
>>   {
>> +    int ret;
>>       struct pci_dev *pdev = devinfo->pdev;
>>       struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
>> @@ -972,16 +973,19 @@ static int brcmf_pcie_request_irq(struct 
>> brcmf_pciedev_info *devinfo)
>>       brcmf_dbg(PCIE, "Enter\n");
>> -    pci_enable_msi(pdev);
>> +    ret = pci_enable_msi(pdev);
>> +    if (ret)
>> +        return ret;
> 
> The above is sufficient. Further adjustments not needed.

Actually I am not sure about this. I think there is no harm in silently 
ignoring the failure.

>>       if (request_threaded_irq(pdev->irq, brcmf_pcie_quick_check_isr,
>>                    brcmf_pcie_isr_thread, IRQF_SHARED,
>>                    "brcmf_pcie_intr", devinfo)) {
>>           pci_disable_msi(pdev);
>>           brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);
>> -        return -EIO;
>> +        ret = -EIO;
>> +    } else {
>> +        devinfo->irq_allocated = true;
>>       }
>> -    devinfo->irq_allocated = true;
>> -    return 0;
>> +    return ret;
>>   }

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 2/2] [v2] wifi: brcmfmac: handle possible MSI enabling error

[Bjorn Helgaas]

On Wed, Jun 14, 2023 at 10:58:48AM +0300, Dmitry Antipov wrote:
> Handle possible 'pci_enable_msi()' error in
> 'brcmf_pcie_request_irq()', 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
> ---
>  .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c  | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index 80220685f5e4..f7d9f2cbd60b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -965,6 +965,7 @@ static irqreturn_t brcmf_pcie_isr_thread(int irq, void *arg)
>  
>  static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
>  {
> +	int ret;
>  	struct pci_dev *pdev = devinfo->pdev;
>  	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
>  
> @@ -972,16 +973,19 @@ static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
>  
>  	brcmf_dbg(PCIE, "Enter\n");
>  
> -	pci_enable_msi(pdev);
> +	ret = pci_enable_msi(pdev);
> +	if (ret)
> +		return ret;

If the device supports INTx and MSI is disabled for some reason
(booted with "pci=nomsi", ACPI_FADT_NO_MSI is set, etc), this change
means the driver would no longer be able to fall back to using INTx.

If you want to go to the trouble of changing this code, you could
consider using the new APIs:

  ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
  if (ret < 0)
    return ret;

  if (request_threaded_irq(pdev->irq, ...)) {
    pci_free_irq_vectors(pdev);
    return -EIO;
  }

plus the appropriate pci_free_irq_vectors() calls in other failure and
remove paths.

See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/PCI/msi-howto.rst?v6.7#n93

>  	if (request_threaded_irq(pdev->irq, brcmf_pcie_quick_check_isr,
>  				 brcmf_pcie_isr_thread, IRQF_SHARED,
>  				 "brcmf_pcie_intr", devinfo)) {
>  		pci_disable_msi(pdev);
>  		brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);
> -		return -EIO;
> +		ret = -EIO;

IMHO this part ("ret = -EIO" and "return ret" below) makes the code
harder to read for no benefit.  The existing code returns -EIO
immediately here and returns 0 below.  It's easier to read that than
to follow the use of "ret".

I guess that's just repeating what Arend already said; sorry, I hadn't
read the whole thread yet.

> +	} else {
> +		devinfo->irq_allocated = true;
>  	}
> -	devinfo->irq_allocated = true;
> -	return 0;
> +	return ret;
>  }
>  
>  
> -- 
> 2.40.1
> 

Re: [PATCH 2/2] [v2] wifi: brcmfmac: handle possible MSI enabling error

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 3087 bytes --]

On January 19, 2024 10:24:07 PM Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Wed, Jun 14, 2023 at 10:58:48AM +0300, Dmitry Antipov wrote:
>> Handle possible 'pci_enable_msi()' error in
>> 'brcmf_pcie_request_irq()', 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
>> ---
>> .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c  | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
>> index 80220685f5e4..f7d9f2cbd60b 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
>> @@ -965,6 +965,7 @@ static irqreturn_t brcmf_pcie_isr_thread(int irq, void 
>> *arg)
>>
>> static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
>> {
>> + int ret;
>> struct pci_dev *pdev = devinfo->pdev;
>> struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
>>
>> @@ -972,16 +973,19 @@ static int brcmf_pcie_request_irq(struct 
>> brcmf_pciedev_info *devinfo)
>>
>> brcmf_dbg(PCIE, "Enter\n");
>>
>> - pci_enable_msi(pdev);
>> + ret = pci_enable_msi(pdev);
>> + if (ret)
>> + return ret;
>
> If the device supports INTx and MSI is disabled for some reason
> (booted with "pci=nomsi", ACPI_FADT_NO_MSI is set, etc), this change
> means the driver would no longer be able to fall back to using INTx.

Thanks, Bjorn

This was indeed my concern.

> If you want to go to the trouble of changing this code, you could
> consider using the new APIs:

I saw this mentioned in pci_enable_msi() kerneldoc, but didn't bring it up yet.

>
>  ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
>  if (ret < 0)
>    return ret;
>
>  if (request_threaded_irq(pdev->irq, ...)) {
>    pci_free_irq_vectors(pdev);
>    return -EIO;
>  }
>
> plus the appropriate pci_free_irq_vectors() calls in other failure and
> remove paths.
>
> See 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/PCI/msi-howto.rst?v6.7#n93
>
>> if (request_threaded_irq(pdev->irq, brcmf_pcie_quick_check_isr,
>> brcmf_pcie_isr_thread, IRQF_SHARED,
>> "brcmf_pcie_intr", devinfo)) {
>> pci_disable_msi(pdev);
>> brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);
>> - return -EIO;
>> + ret = -EIO;
>
> IMHO this part ("ret = -EIO" and "return ret" below) makes the code
> harder to read for no benefit.  The existing code returns -EIO
> immediately here and returns 0 below.  It's easier to read that than
> to follow the use of "ret".
>
> I guess that's just repeating what Arend already said; sorry, I hadn't
> read the whole thread yet.

No need to be sorry. Thanks for taking time to look at it and give your 
feedback.

Regards,
Arend

>
>
>> + } else {
>> + devinfo->irq_allocated = true;
>> }
>> - devinfo->irq_allocated = true;
>> - return 0;
>> + return ret;
>> }
>>
>>
>> --
>> 2.40.1




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

[PATCH 1/2] [v3] wifi: brcmfmac: handle possible completion timeouts

[Dmitry Antipov]

Handle possible 'wait_for_completion_timeout()' errors in
'brcmf_p2p_af_searching_channel()' and 'brcmf_p2p_del_vif()',
fix spelling and add comments where appropriate. Compile
tested only.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: adjust per Arend's review
v2: rebase against wireless-next tree
---
 .../broadcom/brcm80211/brcmfmac/p2p.c         | 36 +++++++++++--------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index 6e0c90f4718b..a346c5a6e602 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,10 @@ 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);
+		timeout = !wait_for_completion_timeout
+			(&afx_hdl->act_frm_scan, P2P_AF_FRM_SCAN_MAX_WAIT);
+		if (timeout)
+			break;
 		if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) ||
 		    (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL,
 			       &p2p->status)))
@@ -1186,8 +1189,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);
+			timeout = !wait_for_completion_timeout
+				(&afx_hdl->act_frm_scan,
+				 P2P_AF_FRM_SCAN_MAX_WAIT);
+			if (timeout)
+				break;
 		}
 		if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) ||
 		    (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL,
@@ -1209,7 +1215,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,10 +1586,11 @@ static s32 brcmf_p2p_tx_action_frame(struct brcmf_p2p_info *p2p,
 		  (p2p->wait_for_offchan_complete) ?
 		   "off-channel" : "on-channel");
 
+	/* timeout would cause the code to proceed in the else branch below */
 	wait_for_completion_timeout(&p2p->send_af_done, P2P_AF_MAX_WAIT_TIME);
 
 	if (test_bit(BRCMF_P2P_STATUS_ACTION_TX_COMPLETED, &p2p->status)) {
-		brcmf_dbg(TRACE, "TX action frame operation is success\n");
+		brcmf_dbg(TRACE, "TX action frame operation has succeeded\n");
 	} else {
 		err = -EIO;
 		brcmf_dbg(TRACE, "TX action frame operation has failed\n");
@@ -2371,7 +2378,7 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 	struct brcmf_cfg80211_vif *vif;
 	enum nl80211_iftype iftype;
 	bool wait_for_disable = false;
-	int err;
+	int err = 0;
 
 	brcmf_dbg(TRACE, "delete P2P vif\n");
 	vif = container_of(wdev, struct brcmf_cfg80211_vif, wdev);
@@ -2403,14 +2410,15 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 	clear_bit(BRCMF_P2P_STATUS_GO_NEG_PHASE, &p2p->status);
 	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 = 0;
 	if (iftype != NL80211_IFTYPE_P2P_DEVICE) {
-		brcmf_vif_clear_mgmt_ies(vif);
-		err = brcmf_p2p_release_p2p_if(vif);
+		if (wait_for_disable)
+			err = (wait_for_completion_timeout
+			       (&cfg->vif_disabled,
+				BRCMF_P2P_DISABLE_TIMEOUT) ? 0 : -ETIMEDOUT);
+		if (!err) {
+			brcmf_vif_clear_mgmt_ies(vif);
+			err = brcmf_p2p_release_p2p_if(vif);
+		}
 	}
 	if (!err) {
 		/* wait for firmware event */
-- 
2.43.0

[PATCH 2/2] [v3] wifi: brcmfmac: handle possible PCIE irq handling errors

[Dmitry Antipov]

Switch to newer 'pci_{alloc,feee}_irq_vectors()' API and handle
possible errors in 'brcmf_pcie_request_irq()'. Compile tested only.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: switch to 'pci_{alloc,feee}_irq_vectors()' per Bjorn's review
v2: rebase against wireless-next tree
---
 .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c    | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 80220685f5e4..0f77d94f34a3 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -965,6 +965,7 @@ static irqreturn_t brcmf_pcie_isr_thread(int irq, void *arg)
 
 static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
 {
+	int ret;
 	struct pci_dev *pdev = devinfo->pdev;
 	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
 
@@ -972,11 +973,14 @@ static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
 
 	brcmf_dbg(PCIE, "Enter\n");
 
-	pci_enable_msi(pdev);
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		return ret;
+
 	if (request_threaded_irq(pdev->irq, brcmf_pcie_quick_check_isr,
 				 brcmf_pcie_isr_thread, IRQF_SHARED,
 				 "brcmf_pcie_intr", devinfo)) {
-		pci_disable_msi(pdev);
+		pci_free_irq_vectors(pdev);
 		brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);
 		return -EIO;
 	}
@@ -997,7 +1001,7 @@ static void brcmf_pcie_release_irq(struct brcmf_pciedev_info *devinfo)
 
 	brcmf_pcie_intr_disable(devinfo);
 	free_irq(pdev->irq, devinfo);
-	pci_disable_msi(pdev);
+	pci_free_irq_vectors(pdev);
 
 	msleep(50);
 	count = 0;
-- 
2.43.0

Re: [PATCH 2/2] [v3] wifi: brcmfmac: handle possible PCIE irq handling errors

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

On January 22, 2024 12:59:03 PM Dmitry Antipov <dmantipov@yandex.ru> wrote:

> Switch to newer 'pci_{alloc,feee}_irq_vectors()' API and handle
> possible errors in 'brcmf_pcie_request_irq()'. Compile tested only.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

See comment below...

Reviewed-by: Arend van Spriel<arend.vanspriel@broadcom.com>
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v3: switch to 'pci_{alloc,feee}_irq_vectors()' per Bjorn's review
> v2: rebase against wireless-next tree
> ---
> .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c    | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index 80220685f5e4..0f77d94f34a3 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -965,6 +965,7 @@ static irqreturn_t brcmf_pcie_isr_thread(int irq, void 
> *arg)
>
> static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
> {
> + int ret;
>  struct pci_dev *pdev = devinfo->pdev;
>  struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
>
> @@ -972,11 +973,14 @@ static int brcmf_pcie_request_irq(struct 
> brcmf_pciedev_info *devinfo)
>
>  brcmf_dbg(PCIE, "Enter\n");
>
> - pci_enable_msi(pdev);
> + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> + if (ret < 0)
> + return ret;
> +
>  if (request_threaded_irq(pdev->irq,

Reading the kerneldoc for pci_alloc_irq_vectors() you should use the helper 
function pci_irq_vector() instead of pdev->irq


brcmf_pcie_quick_check_isr,
>  brcmf_pcie_isr_thread, IRQF_SHARED,
>  "brcmf_pcie_intr", devinfo)) {
> - pci_disable_msi(pdev);
> + pci_free_irq_vectors(pdev);
>  brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);
>  return -EIO;
>  }
> @@ -997,7 +1001,7 @@ static void brcmf_pcie_release_irq(struct 
> brcmf_pciedev_info *devinfo)
>
>  brcmf_pcie_intr_disable(devinfo);
>  free_irq(pdev->irq, devinfo);
> - pci_disable_msi(pdev);
> + pci_free_irq_vectors(pdev);
>
>  msleep(50);
>  count = 0;
> --
> 2.43.0




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 2/2] [v3] wifi: brcmfmac: handle possible PCIE irq handling errors

[Bjorn Helgaas]

On Mon, Jan 22, 2024 at 02:57:25PM +0300, Dmitry Antipov wrote:
> Switch to newer 'pci_{alloc,feee}_irq_vectors()' API and handle
> possible errors in 'brcmf_pcie_request_irq()'. Compile tested only.

If you have occasion to update this, possibly s/PCIE/PCI/ in the
subject, since this is generic to PCI and PCIe.

Bjorn

Re: [PATCH 2/2] [v3] wifi: brcmfmac: handle possible PCIE irq handling errors

[Dmitry Antipov]

On 1/22/24 21:20, Bjorn Helgaas wrote:

> If you have occasion to update this, possibly s/PCIE/PCI/ in the
> subject, since this is generic to PCI and PCIe.

OK. BTW if we're touching this anyway, shouldn't we hook into the generic device
framework and switch to devm_request_threaded_irq()/devm_free_irq() as well?

Dmitry

Re: [PATCH 2/2] [v3] wifi: brcmfmac: handle possible PCIE irq handling errors

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On January 24, 2024 7:22:58 AM Dmitry Antipov <dmantipov@yandex.ru> wrote:

> On 1/22/24 21:20, Bjorn Helgaas wrote:
>
>> If you have occasion to update this, possibly s/PCIE/PCI/ in the
>> subject, since this is generic to PCI and PCIe.
>
> OK. BTW if we're touching this anyway, shouldn't we hook into the generic 
> device
> framework and switch to devm_request_threaded_irq()/devm_free_irq() as well?

Hi Dmitry,

Those are not generic device functions. They create device managed resource 
so devm_free_irq() is implicitly invoked upon device detach. So replacing 
free_irq() with devm_free_irq() is not very meaningful. There are some 
devm_*() usages in the driver, but it's not used commonly. However, feel 
free to add it.

Regards,
Arend

> Dmitry




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

[PATCH 1/2] [v4] wifi: brcmfmac: handle possible completion timeouts

[Dmitry Antipov]

Handle possible 'wait_for_completion_timeout()' errors in
'brcmf_p2p_af_searching_channel()' and 'brcmf_p2p_del_vif()',
fix spelling and add comments where appropriate. Compile
tested only.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: unchanged since v3
v3: adjust per Arend's review
v2: rebase against wireless-next tree
---
 .../broadcom/brcm80211/brcmfmac/p2p.c         | 36 +++++++++++--------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index 6e0c90f4718b..a346c5a6e602 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,10 @@ 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);
+		timeout = !wait_for_completion_timeout
+			(&afx_hdl->act_frm_scan, P2P_AF_FRM_SCAN_MAX_WAIT);
+		if (timeout)
+			break;
 		if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) ||
 		    (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL,
 			       &p2p->status)))
@@ -1186,8 +1189,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);
+			timeout = !wait_for_completion_timeout
+				(&afx_hdl->act_frm_scan,
+				 P2P_AF_FRM_SCAN_MAX_WAIT);
+			if (timeout)
+				break;
 		}
 		if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) ||
 		    (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL,
@@ -1209,7 +1215,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,10 +1586,11 @@ static s32 brcmf_p2p_tx_action_frame(struct brcmf_p2p_info *p2p,
 		  (p2p->wait_for_offchan_complete) ?
 		   "off-channel" : "on-channel");
 
+	/* timeout would cause the code to proceed in the else branch below */
 	wait_for_completion_timeout(&p2p->send_af_done, P2P_AF_MAX_WAIT_TIME);
 
 	if (test_bit(BRCMF_P2P_STATUS_ACTION_TX_COMPLETED, &p2p->status)) {
-		brcmf_dbg(TRACE, "TX action frame operation is success\n");
+		brcmf_dbg(TRACE, "TX action frame operation has succeeded\n");
 	} else {
 		err = -EIO;
 		brcmf_dbg(TRACE, "TX action frame operation has failed\n");
@@ -2371,7 +2378,7 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 	struct brcmf_cfg80211_vif *vif;
 	enum nl80211_iftype iftype;
 	bool wait_for_disable = false;
-	int err;
+	int err = 0;
 
 	brcmf_dbg(TRACE, "delete P2P vif\n");
 	vif = container_of(wdev, struct brcmf_cfg80211_vif, wdev);
@@ -2403,14 +2410,15 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 	clear_bit(BRCMF_P2P_STATUS_GO_NEG_PHASE, &p2p->status);
 	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 = 0;
 	if (iftype != NL80211_IFTYPE_P2P_DEVICE) {
-		brcmf_vif_clear_mgmt_ies(vif);
-		err = brcmf_p2p_release_p2p_if(vif);
+		if (wait_for_disable)
+			err = (wait_for_completion_timeout
+			       (&cfg->vif_disabled,
+				BRCMF_P2P_DISABLE_TIMEOUT) ? 0 : -ETIMEDOUT);
+		if (!err) {
+			brcmf_vif_clear_mgmt_ies(vif);
+			err = brcmf_p2p_release_p2p_if(vif);
+		}
 	}
 	if (!err) {
 		/* wait for firmware event */
-- 
2.43.0

[PATCH 2/2] [v4] wifi: brcmfmac: handle possible PCI irq handling errors

[Dmitry Antipov]

Switch to newer 'pci_{alloc,feee}_irq_vectors()' API and handle
possible errors in 'brcmf_pcie_request_irq()'. Compile tested only.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: prefer devm_{devm_request_threaded_irq,free_irq}()', use
'pci_irq_vector()' and fix title (Arend, Bjorn)
v3: switch to 'pci_{alloc,feee}_irq_vectors()' per Bjorn's review
v2: rebase against wireless-next tree
---
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 20 ++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 80220685f5e4..17b855164025 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -965,6 +965,7 @@ static irqreturn_t brcmf_pcie_isr_thread(int irq, void *arg)
 
 static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
 {
+	int ret;
 	struct pci_dev *pdev = devinfo->pdev;
 	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
 
@@ -972,11 +973,16 @@ static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
 
 	brcmf_dbg(PCIE, "Enter\n");
 
-	pci_enable_msi(pdev);
-	if (request_threaded_irq(pdev->irq, brcmf_pcie_quick_check_isr,
-				 brcmf_pcie_isr_thread, IRQF_SHARED,
-				 "brcmf_pcie_intr", devinfo)) {
-		pci_disable_msi(pdev);
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		return ret;
+
+	if (devm_request_threaded_irq(&pdev->dev,
+				      pci_irq_vector(pdev, 0),
+				      brcmf_pcie_quick_check_isr,
+				      brcmf_pcie_isr_thread, IRQF_SHARED,
+				      "brcmf_pcie_intr", devinfo)) {
+		pci_free_irq_vectors(pdev);
 		brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);
 		return -EIO;
 	}
@@ -996,8 +1002,8 @@ static void brcmf_pcie_release_irq(struct brcmf_pciedev_info *devinfo)
 		return;
 
 	brcmf_pcie_intr_disable(devinfo);
-	free_irq(pdev->irq, devinfo);
-	pci_disable_msi(pdev);
+	devm_free_irq(&pdev->dev, pdev->irq, devinfo);
+	pci_free_irq_vectors(pdev);
 
 	msleep(50);
 	count = 0;
-- 
2.43.0

Re: [PATCH 2/2] [v4] wifi: brcmfmac: handle possible PCI irq handling errors

[Bjorn Helgaas]

On Wed, Jan 24, 2024 at 12:27:10PM +0300, Dmitry Antipov wrote:
> Switch to newer 'pci_{alloc,feee}_irq_vectors()' API and handle
> possible errors in 'brcmf_pcie_request_irq()'. Compile tested only.

s/feee/free/

[PATCH 0/2] Re: Re: [lvc-project] [PATCH 2/2] [v4] wifi: brcmfmac: handle possible PCI irq handling errors

[Dmitry Antipov]

Re-sending v4 with spelling fixes as noticed by Bjorn.

Dmitry Antipov (2):
  wifi: brcmfmac: handle possible completion timeouts
  wifi: brcmfmac: handle possible PCI irq handling errors

 .../broadcom/brcm80211/brcmfmac/p2p.c         | 36 +++++++++++--------
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 20 +++++++----
 2 files changed, 35 insertions(+), 21 deletions(-)

-- 
2.43.0

[PATCH 1/2] [v4] wifi: brcmfmac: handle possible completion timeouts

[Dmitry Antipov]

Handle possible 'wait_for_completion_timeout()' errors in
'brcmf_p2p_af_searching_channel()' and 'brcmf_p2p_del_vif()',
fix spelling and add comments where appropriate. Compile
tested only.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: unchanged since v3
v3: adjust per Arend's review
v2: rebase against wireless-next tree
---
 .../broadcom/brcm80211/brcmfmac/p2p.c         | 36 +++++++++++--------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index 6e0c90f4718b..a346c5a6e602 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,10 @@ 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);
+		timeout = !wait_for_completion_timeout
+			(&afx_hdl->act_frm_scan, P2P_AF_FRM_SCAN_MAX_WAIT);
+		if (timeout)
+			break;
 		if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) ||
 		    (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL,
 			       &p2p->status)))
@@ -1186,8 +1189,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);
+			timeout = !wait_for_completion_timeout
+				(&afx_hdl->act_frm_scan,
+				 P2P_AF_FRM_SCAN_MAX_WAIT);
+			if (timeout)
+				break;
 		}
 		if ((afx_hdl->peer_chan != P2P_INVALID_CHANNEL) ||
 		    (!test_bit(BRCMF_P2P_STATUS_FINDING_COMMON_CHANNEL,
@@ -1209,7 +1215,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,10 +1586,11 @@ static s32 brcmf_p2p_tx_action_frame(struct brcmf_p2p_info *p2p,
 		  (p2p->wait_for_offchan_complete) ?
 		   "off-channel" : "on-channel");
 
+	/* timeout would cause the code to proceed in the else branch below */
 	wait_for_completion_timeout(&p2p->send_af_done, P2P_AF_MAX_WAIT_TIME);
 
 	if (test_bit(BRCMF_P2P_STATUS_ACTION_TX_COMPLETED, &p2p->status)) {
-		brcmf_dbg(TRACE, "TX action frame operation is success\n");
+		brcmf_dbg(TRACE, "TX action frame operation has succeeded\n");
 	} else {
 		err = -EIO;
 		brcmf_dbg(TRACE, "TX action frame operation has failed\n");
@@ -2371,7 +2378,7 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 	struct brcmf_cfg80211_vif *vif;
 	enum nl80211_iftype iftype;
 	bool wait_for_disable = false;
-	int err;
+	int err = 0;
 
 	brcmf_dbg(TRACE, "delete P2P vif\n");
 	vif = container_of(wdev, struct brcmf_cfg80211_vif, wdev);
@@ -2403,14 +2410,15 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 	clear_bit(BRCMF_P2P_STATUS_GO_NEG_PHASE, &p2p->status);
 	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 = 0;
 	if (iftype != NL80211_IFTYPE_P2P_DEVICE) {
-		brcmf_vif_clear_mgmt_ies(vif);
-		err = brcmf_p2p_release_p2p_if(vif);
+		if (wait_for_disable)
+			err = (wait_for_completion_timeout
+			       (&cfg->vif_disabled,
+				BRCMF_P2P_DISABLE_TIMEOUT) ? 0 : -ETIMEDOUT);
+		if (!err) {
+			brcmf_vif_clear_mgmt_ies(vif);
+			err = brcmf_p2p_release_p2p_if(vif);
+		}
 	}
 	if (!err) {
 		/* wait for firmware event */
-- 
2.43.0

[PATCH 2/2] [v4] wifi: brcmfmac: handle possible PCI irq handling errors

[Dmitry Antipov]

Switch to newer 'pci_{alloc,free}_irq_vectors()' API and handle
possible errors in 'brcmf_pcie_request_irq()'. Compile tested only.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: prefer devm_{devm_request_threaded_irq,free_irq}()', use
'pci_irq_vector()' and fix title (Arend, Bjorn)
v3: switch to 'pci_{alloc,free}_irq_vectors()' per Bjorn's review
v2: rebase against wireless-next tree
---
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 20 ++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 80220685f5e4..17b855164025 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -965,6 +965,7 @@ static irqreturn_t brcmf_pcie_isr_thread(int irq, void *arg)
 
 static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
 {
+	int ret;
 	struct pci_dev *pdev = devinfo->pdev;
 	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
 
@@ -972,11 +973,16 @@ static int brcmf_pcie_request_irq(struct brcmf_pciedev_info *devinfo)
 
 	brcmf_dbg(PCIE, "Enter\n");
 
-	pci_enable_msi(pdev);
-	if (request_threaded_irq(pdev->irq, brcmf_pcie_quick_check_isr,
-				 brcmf_pcie_isr_thread, IRQF_SHARED,
-				 "brcmf_pcie_intr", devinfo)) {
-		pci_disable_msi(pdev);
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		return ret;
+
+	if (devm_request_threaded_irq(&pdev->dev,
+				      pci_irq_vector(pdev, 0),
+				      brcmf_pcie_quick_check_isr,
+				      brcmf_pcie_isr_thread, IRQF_SHARED,
+				      "brcmf_pcie_intr", devinfo)) {
+		pci_free_irq_vectors(pdev);
 		brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);
 		return -EIO;
 	}
@@ -996,8 +1002,8 @@ static void brcmf_pcie_release_irq(struct brcmf_pciedev_info *devinfo)
 		return;
 
 	brcmf_pcie_intr_disable(devinfo);
-	free_irq(pdev->irq, devinfo);
-	pci_disable_msi(pdev);
+	devm_free_irq(&pdev->dev, pdev->irq, devinfo);
+	pci_free_irq_vectors(pdev);
 
 	msleep(50);
 	count = 0;
-- 
2.43.0

Re: [PATCH 1/2] [v4] wifi: brcmfmac: handle possible completion timeouts

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On January 25, 2024 1:08:25 PM Dmitry Antipov <dmantipov@yandex.ru> wrote:

> Handle possible 'wait_for_completion_timeout()' errors in
> 'brcmf_p2p_af_searching_channel()' and 'brcmf_p2p_del_vif()',
> fix spelling and add comments where appropriate. Compile
> tested only.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v4: unchanged since v3
> v3: adjust per Arend's review
> v2: rebase against wireless-next tree
> ---
> .../broadcom/brcm80211/brcmfmac/p2p.c         | 36 +++++++++++--------
> 1 file changed, 22 insertions(+), 14 deletions(-)



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 2/2] [v4] wifi: brcmfmac: handle possible PCI irq handling errors

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

On January 25, 2024 1:08:26 PM Dmitry Antipov <dmantipov@yandex.ru> wrote:

> Switch to newer 'pci_{alloc,free}_irq_vectors()' API and handle
> possible errors in 'brcmf_pcie_request_irq()'. Compile tested only.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v4: prefer devm_{devm_request_threaded_irq,free_irq}()', use
> 'pci_irq_vector()' and fix title (Arend, Bjorn)
> v3: switch to 'pci_{alloc,free}_irq_vectors()' per Bjorn's review
> v2: rebase against wireless-next tree
> ---
> .../broadcom/brcm80211/brcmfmac/pcie.c        | 20 ++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 2/2] [v4] wifi: brcmfmac: handle possible PCI irq handling errors

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]

On January 25, 2024 1:08:26 PM Dmitry Antipov <dmantipov@yandex.ru> wrote:

> Switch to newer 'pci_{alloc,free}_irq_vectors()' API and handle
> possible errors in 'brcmf_pcie_request_irq()'. Compile tested only.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

Crap. Did notice something else...

> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v4: prefer devm_{devm_request_threaded_irq,free_irq}()', use
> 'pci_irq_vector()' and fix title (Arend, Bjorn)
> v3: switch to 'pci_{alloc,free}_irq_vectors()' per Bjorn's review
> v2: rebase against wireless-next tree
> ---
> .../broadcom/brcm80211/brcmfmac/pcie.c        | 20 ++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index 80220685f5e4..17b855164025 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c

[...]

> @@ -972,11 +973,16 @@ static int brcmf_pcie_request_irq(struct 
> brcmf_pciedev_info *devinfo)
>
>  brcmf_dbg(PCIE, "Enter\n");
>
> - pci_enable_msi(pdev);
> - if (request_threaded_irq(pdev->irq, brcmf_pcie_quick_check_isr,
> - brcmf_pcie_isr_thread, IRQF_SHARED,
> - "brcmf_pcie_intr", devinfo)) {
> - pci_disable_msi(pdev);
> + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> + if (ret < 0)
> + return ret;
> +
> + if (devm_request_threaded_irq(&pdev->dev,
> +      pci_irq_vector(pdev, 0),
> +      brcmf_pcie_quick_check_isr,
> +      brcmf_pcie_isr_thread, IRQF_SHARED,
> +      "brcmf_pcie_intr", devinfo)) {
> + pci_free_irq_vectors(pdev);
>  brcmf_err(bus, "Failed to request IRQ %d\n", pdev->irq);

Maybe better to use pci_irq_vector() here as well.

>  return -EIO;
>  }




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]