LinuxLists.cc - [PATCH 1/2] [v2] wifi: brcmfmac: handle possible completion timeouts

[permalink] [raw]

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

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 <[email protected]>
---
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

2024-01-18 11:22:45

by Kalle Valo

[permalink] [raw]

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

Dmitry Antipov <[email protected]> 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 <[email protected]>

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/[email protected]/

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

2024-01-18 11:57:50

by Arend Van Spriel

[permalink] [raw]

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

On 1/18/2024 12:22 PM, Kalle Valo wrote:
> Dmitry Antipov <[email protected]> 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 <[email protected]>
>
> 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
>

2024-01-18 12:22:51

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> 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);

Attachments:

2024-01-18 12:26:58

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> 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;
> }
>
>

Attachments:

2024-01-18 12:31:39

[permalink] [raw]

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

+ 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 <[email protected]>
>> Signed-off-by: Dmitry Antipov <[email protected]>
>> ---
>> 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;
>> }

Attachments:

2024-01-19 21:24:17

by Bjorn Helgaas

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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
>

2024-01-20 13:24:37

[permalink] [raw]

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

On January 19, 2024 10:24:07 PM Bjorn Helgaas <[email protected]> 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 <[email protected]>
>> ---
>> 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

Attachments:

2024-01-22 11:59:15

[permalink] [raw]

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

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 <[email protected]>
---
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

2024-01-22 11:59:22

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Dmitry Antipov <[email protected]>
---
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

2024-01-22 19:05:44

by Bjorn Helgaas

[permalink] [raw]

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

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

2024-01-22 20:44:26

[permalink] [raw]

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

On January 22, 2024 12:59:03 PM Dmitry Antipov <[email protected]> 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<[email protected]>
> Suggested-by: Bjorn Helgaas <[email protected]>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> 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

Attachments:

2024-01-24 06:29:36

[permalink] [raw]

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

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

2024-01-24 07:03:35

[permalink] [raw]

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

On January 24, 2024 7:22:58 AM Dmitry Antipov <[email protected]> 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

Attachments:

2024-01-24 09:35:14

[permalink] [raw]

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

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 <[email protected]>
---
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

2024-01-24 09:35:20

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Dmitry Antipov <[email protected]>
---
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

2024-01-24 14:14:42

by Bjorn Helgaas

[permalink] [raw]

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

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/

2024-01-25 12:08:39

[permalink] [raw]

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

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

2024-01-25 12:16:26

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Dmitry Antipov <[email protected]>
---
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

2024-01-25 12:37:51

[permalink] [raw]

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

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 <[email protected]>
---
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

2024-01-25 17:32:33

[permalink] [raw]

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

On January 25, 2024 1:08:25 PM Dmitry Antipov <[email protected]> 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 <[email protected]>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> 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(-)

Attachments:

2024-01-25 17:34:02

[permalink] [raw]

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

Attachments:

2024-01-25 18:57:09

[permalink] [raw]

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

On January 25, 2024 1:08:26 PM Dmitry Antipov <[email protected]> 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 <[email protected]>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> 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;
> }

Attachments: