| Message ID | 20230614075848.80536-2-dmantipov@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2,v2] wifi: brcmfmac: handle possible completion timeouts | expand |
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; > } > >
+ 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; >> }
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 >
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
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; }
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(-)