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

[Arend Van Spriel <arend.vanspriel@xxxxxxxxxxxx>]

On January 19, 2024 10:24:07 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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: smime.p7s
Description: S/MIME Cryptographic Signature