| Message ID | 20220923093806.3108119-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [-next] wifi: brcmfmac: pcie: add missing pci_disable_device() in brcmf_pcie_get_resource() | expand |
On Fri, Sep 23, 2022 at 2:42 AM ruanjinjie <ruanjinjie@huawei.com> wrote: > > Add missing pci_disable_device() if brcmf_pcie_get_resource() fails. Did you encounter any issue because of the absensent pci_disable_device? A bit more context will be very helpful. > > Signed-off-by: ruanjinjie <ruanjinjie@huawei.com> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index f98641bb1528..25fa69793d86 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -1725,7 +1725,8 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) > if ((bar1_size == 0) || (bar1_addr == 0)) { > brcmf_err(bus, "BAR1 Not enabled, device size=%ld, addr=%#016llx\n", > bar1_size, (unsigned long long)bar1_addr); > - return -EINVAL; > + err = -EINVAL; > + goto err_disable; > } > > devinfo->regs = ioremap(bar0_addr, BRCMF_PCIE_REG_MAP_SIZE); > @@ -1734,7 +1735,8 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) > if (!devinfo->regs || !devinfo->tcm) { > brcmf_err(bus, "ioremap() failed (%p,%p)\n", devinfo->regs, > devinfo->tcm); > - return -EINVAL; > + err = -EINVAL; > + goto err_disable; > } > brcmf_dbg(PCIE, "Phys addr : reg space = %p base addr %#016llx\n", > devinfo->regs, (unsigned long long)bar0_addr); > @@ -1743,6 +1745,9 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) > (unsigned int)bar1_size); > > return 0; > +err_disable: > + pci_disable_device(pdev); Isn't brcmf_pcie_release_resource() a better choice which also unmap the io if any was mapped? Regards, - Franky > + return err; > } > > > -- > 2.25.1 >
On 2022/9/24 0:50, Franky Lin wrote: > On Fri, Sep 23, 2022 at 2:42 AM ruanjinjie <ruanjinjie@huawei.com> wrote: >> >> Add missing pci_disable_device() if brcmf_pcie_get_resource() fails. > > Did you encounter any issue because of the absensent > pci_disable_device? A bit more context will be very helpful. > We use static analysis via coccinelle to find the above issue. The command we use is below: spatch -I include -timeout 60 -very_quiet -sp_file pci_disable_device_missing.cocci drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> >> Signed-off-by: ruanjinjie <ruanjinjie@huawei.com> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> index f98641bb1528..25fa69793d86 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> @@ -1725,7 +1725,8 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) >> if ((bar1_size == 0) || (bar1_addr == 0)) { >> brcmf_err(bus, "BAR1 Not enabled, device size=%ld, addr=%#016llx\n", >> bar1_size, (unsigned long long)bar1_addr); >> - return -EINVAL; >> + err = -EINVAL; >> + goto err_disable; >> } >> >> devinfo->regs = ioremap(bar0_addr, BRCMF_PCIE_REG_MAP_SIZE); >> @@ -1734,7 +1735,8 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) >> if (!devinfo->regs || !devinfo->tcm) { >> brcmf_err(bus, "ioremap() failed (%p,%p)\n", devinfo->regs, >> devinfo->tcm); >> - return -EINVAL; >> + err = -EINVAL; >> + goto err_disable; >> } >> brcmf_dbg(PCIE, "Phys addr : reg space = %p base addr %#016llx\n", >> devinfo->regs, (unsigned long long)bar0_addr); >> @@ -1743,6 +1745,9 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) >> (unsigned int)bar1_size); >> >> return 0; >> +err_disable: >> + pci_disable_device(pdev); > > Isn't brcmf_pcie_release_resource() a better choice which also unmap > the io if any was mapped? > > Regards, > - Franky > >> + return err; >> } >> >> >> -- >> 2.25.1 >>
On 2022/9/24 0:50, Franky Lin wrote: > On Fri, Sep 23, 2022 at 2:42 AM ruanjinjie <ruanjinjie@huawei.com> wrote: >> >> Add missing pci_disable_device() if brcmf_pcie_get_resource() fails. > > Did you encounter any issue because of the absensent > pci_disable_device? A bit more context will be very helpful. > >> >> Signed-off-by: ruanjinjie <ruanjinjie@huawei.com> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> index f98641bb1528..25fa69793d86 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> @@ -1725,7 +1725,8 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) >> if ((bar1_size == 0) || (bar1_addr == 0)) { >> brcmf_err(bus, "BAR1 Not enabled, device size=%ld, addr=%#016llx\n", >> bar1_size, (unsigned long long)bar1_addr); >> - return -EINVAL; >> + err = -EINVAL; >> + goto err_disable; >> } >> >> devinfo->regs = ioremap(bar0_addr, BRCMF_PCIE_REG_MAP_SIZE); >> @@ -1734,7 +1735,8 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) >> if (!devinfo->regs || !devinfo->tcm) { >> brcmf_err(bus, "ioremap() failed (%p,%p)\n", devinfo->regs, >> devinfo->tcm); >> - return -EINVAL; >> + err = -EINVAL; >> + goto err_disable; >> } >> brcmf_dbg(PCIE, "Phys addr : reg space = %p base addr %#016llx\n", >> devinfo->regs, (unsigned long long)bar0_addr); >> @@ -1743,6 +1745,9 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) >> (unsigned int)bar1_size); >> >> return 0; >> +err_disable: >> + pci_disable_device(pdev); > > Isn't brcmf_pcie_release_resource() a better choice which also unmap > the io if any was mapped? > That is a better choice to call pci_disable_device() in brcmf_pcie_release_resource()! > Regards, > - Franky > >> + return err; >> } >> >> >> -- >> 2.25.1 >>
Ruan Jinjie <ruanjinjie@huawei.com> writes: > On 2022/9/24 0:50, Franky Lin wrote: >> On Fri, Sep 23, 2022 at 2:42 AM ruanjinjie <ruanjinjie@huawei.com> wrote: >>> >>> Add missing pci_disable_device() if brcmf_pcie_get_resource() fails. >> >> Did you encounter any issue because of the absensent >> pci_disable_device? A bit more context will be very helpful. >> > > We use static analysis via coccinelle to find the above issue. The > command we use is below: > > spatch -I include -timeout 60 -very_quiet -sp_file > pci_disable_device_missing.cocci > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c Please include this information to the commit log, it helps to understand the background of the fix.
On 2022/9/26 15:58, Kalle Valo wrote: > Ruan Jinjie <ruanjinjie@huawei.com> writes: > >> On 2022/9/24 0:50, Franky Lin wrote: >>> On Fri, Sep 23, 2022 at 2:42 AM ruanjinjie <ruanjinjie@huawei.com> wrote: >>>> >>>> Add missing pci_disable_device() if brcmf_pcie_get_resource() fails. >>> >>> Did you encounter any issue because of the absensent >>> pci_disable_device? A bit more context will be very helpful. >>> >> >> We use static analysis via coccinelle to find the above issue. The >> command we use is below: >> >> spatch -I include -timeout 60 -very_quiet -sp_file >> pci_disable_device_missing.cocci >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > > Please include this information to the commit log, it helps to > understand the background of the fix. Thank you for your suggestion! I'll include this information in the future commit log. >
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c index f98641bb1528..25fa69793d86 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c @@ -1725,7 +1725,8 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) if ((bar1_size == 0) || (bar1_addr == 0)) { brcmf_err(bus, "BAR1 Not enabled, device size=%ld, addr=%#016llx\n", bar1_size, (unsigned long long)bar1_addr); - return -EINVAL; + err = -EINVAL; + goto err_disable; } devinfo->regs = ioremap(bar0_addr, BRCMF_PCIE_REG_MAP_SIZE); @@ -1734,7 +1735,8 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) if (!devinfo->regs || !devinfo->tcm) { brcmf_err(bus, "ioremap() failed (%p,%p)\n", devinfo->regs, devinfo->tcm); - return -EINVAL; + err = -EINVAL; + goto err_disable; } brcmf_dbg(PCIE, "Phys addr : reg space = %p base addr %#016llx\n", devinfo->regs, (unsigned long long)bar0_addr); @@ -1743,6 +1745,9 @@ static int brcmf_pcie_get_resource(struct brcmf_pciedev_info *devinfo) (unsigned int)bar1_size); return 0; +err_disable: + pci_disable_device(pdev); + return err; }
Add missing pci_disable_device() if brcmf_pcie_get_resource() fails. Signed-off-by: ruanjinjie <ruanjinjie@huawei.com> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)