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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>