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

[Arend Van Spriel]

On January 19, 2024 10:24:07 PM Bjorn Helgaas <helgaas at 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 at 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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4219 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://linuxtesting.org/pipermail/lvc-project/attachments/20240120/c85f34ca/attachment.bin>