| Message ID | 1346063114-30361-3-git-send-email-wni@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 08/27/2012 12:25 PM, Wei Ni wrote: > In case of inband interrupts, if we handle the interrupt in dpc thread, > two level of thread switching takes place to process wifi interrupts. > One in SDHCI driver and the other in Wifi driver. This may cause the system > instability. Looking into the sdhci/mmc code indeed shows that the brcmfmac irq handler is not called in true IRQ context. So the dpc thread may add unnecessary complexity, but to me there is not indication that there is a stability issue. > Because the SDHCI calls sdio_irq_thread() to handle the irq, this thread locks > mmc host and calls wifi handler. It expects WiFi handler to be quick and > enables sdio interrupt from card at end. If wifi handler defers this work for > a different thread, sdio_irq_thread() will be stuck on next wifi interrupt > since mmc lock is not freed. Not sure if I can follow this explanation. The isr is called with host claimed (by sdio_irq_thread) and all it does is at a linked list member and signal the dpc thread. After doing this the host is released. > Handling the interrupt in ISR directly will prevent thread context switching in > wifi driver. It can fix the instability problems. This basically increases the duration of the isr in brcmfmac. > Signed-off-by: Wei Ni <wni@nvidia.com> > --- > drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 2 ++ > drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c | 8 +++++++- > 2 files changed, 9 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c > index 8e7e692..5cf6c3b 100644 > --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c > @@ -121,7 +121,9 @@ static void brcmf_sdio_irqhandler(struct sdio_func *func) > > brcmf_dbg(INTR, "ib intr triggered\n"); > > + sdio_release_host(sdiodev->func[1]); > brcmf_sdbrcm_isr(sdiodev->bus); > + sdio_claim_host(sdiodev->func[1]); This is probably needed because the ISR now locks the host to long because brcmf_sdbrcm_dpc() is called directly. > } > > /* dummy handler for SDIO function 2 interrupt */ > diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c > index 472f2ef..4576d59 100644 > --- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c > +++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c > @@ -2347,7 +2347,7 @@ static bool brcmf_sdbrcm_dpc(struct brcmf_sdio *bus) > uint framecnt = 0; /* Temporary counter of tx/rx frames */ > bool rxdone = true; /* Flag for no more read data */ > bool resched = false; /* Flag indicating resched wanted */ > - int err; > + int err = 0; > > brcmf_dbg(TRACE, "Enter\n"); > > @@ -3786,11 +3786,17 @@ void brcmf_sdbrcm_isr(void *arg) > if (!bus->intr) > brcmf_dbg(ERROR, "isr w/o interrupt configured!\n"); > > + > +#ifndef CONFIG_BRCMFMAC_SDIO_OOB > + while (brcmf_sdbrcm_dpc(bus)) > + ; > +#else > bus->dpc_sched = true; > if (bus->dpc_tsk) { > brcmf_sdbrcm_adddpctsk(bus); > complete(&bus->dpc_wait); > } > +#endif > } > > static bool brcmf_sdbrcm_bus_watchdog(struct brcmf_sdio *bus) > I would really like to know what issue is solved by this change. Could you provide more details. Franky, Do you have anything to add here? Gr. AvS -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/27/2012 09:24 AM, Arend van Spriel wrote: > On 08/27/2012 12:25 PM, Wei Ni wrote: >> In case of inband interrupts, if we handle the interrupt in dpc thread, >> two level of thread switching takes place to process wifi interrupts. >> One in SDHCI driver and the other in Wifi driver. This may cause the >> system >> instability. > > Looking into the sdhci/mmc code indeed shows that the brcmfmac irq > handler is not called in true IRQ context. So the dpc thread may add > unnecessary complexity, but to me there is not indication that there is > a stability issue. > >> Because the SDHCI calls sdio_irq_thread() to handle the irq, this >> thread locks >> mmc host and calls wifi handler. It expects WiFi handler to be quick and >> enables sdio interrupt from card at end. If wifi handler defers this >> work for >> a different thread, sdio_irq_thread() will be stuck on next wifi >> interrupt >> since mmc lock is not freed. > > Not sure if I can follow this explanation. The isr is called with host > claimed (by sdio_irq_thread) and all it does is at a linked list member > and signal the dpc thread. After doing this the host is released. Is the issue something like the ISR handler or first level of threading does: * Trigger DPC * Re-enable interrupt So that the interrupt then fires again before the triggered DPC can run to handle/clear it, thus causing an interrupt storm? Whereas handling the interrupt directly prevents this race condition? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-08-28 at 00:24 +0800, Arend van Spriel wrote: > On 08/27/2012 12:25 PM, Wei Ni wrote: > > In case of inband interrupts, if we handle the interrupt in dpc thread, > > two level of thread switching takes place to process wifi interrupts. > > One in SDHCI driver and the other in Wifi driver. This may cause the system > > instability. > > Looking into the sdhci/mmc code indeed shows that the brcmfmac irq > handler is not called in true IRQ context. So the dpc thread may add > unnecessary complexity, but to me there is not indication that there is > a stability issue. > > > Because the SDHCI calls sdio_irq_thread() to handle the irq, this thread locks > > mmc host and calls wifi handler. It expects WiFi handler to be quick and > > enables sdio interrupt from card at end. If wifi handler defers this work for > > a different thread, sdio_irq_thread() will be stuck on next wifi interrupt > > since mmc lock is not freed. > > Not sure if I can follow this explanation. The isr is called with host > claimed (by sdio_irq_thread) and all it does is at a linked list member > and signal the dpc thread. After doing this the host is released. > > > Handling the interrupt in ISR directly will prevent thread context switching in > > wifi driver. It can fix the instability problems. > > This basically increases the duration of the isr in brcmfmac. > > > Signed-off-by: Wei Ni <wni@nvidia.com> > > --- > > drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 2 ++ > > drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c | 8 +++++++- > > 2 files changed, 9 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c > > index 8e7e692..5cf6c3b 100644 > > --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c > > +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c > > @@ -121,7 +121,9 @@ static void brcmf_sdio_irqhandler(struct sdio_func *func) > > > > brcmf_dbg(INTR, "ib intr triggered\n"); > > > > + sdio_release_host(sdiodev->func[1]); > > brcmf_sdbrcm_isr(sdiodev->bus); > > + sdio_claim_host(sdiodev->func[1]); > > This is probably needed because the ISR now locks the host to long > because brcmf_sdbrcm_dpc() is called directly. > > > } > > > > /* dummy handler for SDIO function 2 interrupt */ > > diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c > > index 472f2ef..4576d59 100644 > > --- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c > > +++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c > > @@ -2347,7 +2347,7 @@ static bool brcmf_sdbrcm_dpc(struct brcmf_sdio *bus) > > uint framecnt = 0; /* Temporary counter of tx/rx frames */ > > bool rxdone = true; /* Flag for no more read data */ > > bool resched = false; /* Flag indicating resched wanted */ > > - int err; > > + int err = 0; > > > > brcmf_dbg(TRACE, "Enter\n"); > > > > @@ -3786,11 +3786,17 @@ void brcmf_sdbrcm_isr(void *arg) > > if (!bus->intr) > > brcmf_dbg(ERROR, "isr w/o interrupt configured!\n"); > > > > + > > +#ifndef CONFIG_BRCMFMAC_SDIO_OOB > > + while (brcmf_sdbrcm_dpc(bus)) > > + ; > > +#else > > bus->dpc_sched = true; > > if (bus->dpc_tsk) { > > brcmf_sdbrcm_adddpctsk(bus); > > complete(&bus->dpc_wait); > > } > > +#endif > > } > > > > static bool brcmf_sdbrcm_bus_watchdog(struct brcmf_sdio *bus) > > > > I would really like to know what issue is solved by this change. Could > you provide more details. If without this fix, the system is instability, we observed interrupt from Wi-Fi cards pilling up in the system. We observed following issue because of this: 1. Device is slow while downloading file through WiFi 2. WiFi performance is bad 3. WiFi does not turn on single CPU 4. Sometimes it will show following spew from kernel, and system will hang up, it was caused by the semaphore which didn't be released. INFO: task brcmf_watchdog:248 blocked for more than 120 seconds. "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. brcmf_watchdog D c041aeac 0 248 2 0x00000000 [<c041aeac>] (__schedule+0x3c4/0x6d0) from [<c0418fe0>] (schedule_timeout+0x1b0/0x218) [<c0418fe0>] (schedule_timeout+0x1b0/0x218) from [<c041a748>] (__down +0x68/0x98) [<c041a748>] (__down+0x68/0x98) from [<c004c13c>] (down+0x44/0x4c) [<c004c13c>] (down+0x44/0x4c) from [<bf017210>] (brcmf_sdbrcm_bus_watchdog+0x28/0x1d0 [brcmfmac]) [<bf017210>] (brcmf_sdbrcm_bus_watchdog+0x28/0x1d0 [brcmfmac]) from [<bf0173e4>] (brcmf_sdbrcm_watchdog_thread+0x2c/0x50 [brcmfmac]) [<bf0173e4>] (brcmf_sdbrcm_watchdog_thread+0x2c/0x50 [brcmfmac]) from [<c0046328>] (kthread+0x8c/0x98) [<c0046328>] (kthread+0x8c/0x98) from [<c000f5e4>] (kernel_thread_exit +0x0/0x8) After add this fix, everything looks ok. I noticed that in the old version driver, it also has this fix, but removed in http://git.kernel.org/?p=linux/kernel/git/torvalds/linux- 2.6.git;a=commit;h=b61c23c846978a4381fdc499055bd66b7bd24120 Thanks. Wei. > > Franky, > > Do you have anything to add here? > > Gr. AvS > > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-08-28 at 04:06 +0800, Stephen Warren wrote: > On 08/27/2012 09:24 AM, Arend van Spriel wrote: > > On 08/27/2012 12:25 PM, Wei Ni wrote: > >> In case of inband interrupts, if we handle the interrupt in dpc thread, > >> two level of thread switching takes place to process wifi interrupts. > >> One in SDHCI driver and the other in Wifi driver. This may cause the > >> system > >> instability. > > > > Looking into the sdhci/mmc code indeed shows that the brcmfmac irq > > handler is not called in true IRQ context. So the dpc thread may add > > unnecessary complexity, but to me there is not indication that there is > > a stability issue. The brcmfmac irq handler is called in the thread sdio_irq_thread(), this thread indeed is driven by the sdhci irq, although it's not the true IRQ context. If the brcmfmac doesn't clear the IRQ condition ASAP, the sdio_irq_thread will be triggered again and again, and in this condition it's too difficult to run the brcmfmac dpc thread, more and more interrupt can't be handled. > > > >> Because the SDHCI calls sdio_irq_thread() to handle the irq, this > >> thread locks > >> mmc host and calls wifi handler. It expects WiFi handler to be quick and > >> enables sdio interrupt from card at end. If wifi handler defers this > >> work for > >> a different thread, sdio_irq_thread() will be stuck on next wifi > >> interrupt > >> since mmc lock is not freed. > > > > Not sure if I can follow this explanation. The isr is called with host > > claimed (by sdio_irq_thread) and all it does is at a linked list member > > and signal the dpc thread. After doing this the host is released. > > Is the issue something like the ISR handler or first level of threading > does: > > * Trigger DPC > * Re-enable interrupt > > So that the interrupt then fires again before the triggered DPC can run > to handle/clear it, thus causing an interrupt storm? > > Whereas handling the interrupt directly prevents this race condition? Above is my understanding. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/28/2012 04:13 AM, Wei Ni wrote: > On Tue, 2012-08-28 at 04:06 +0800, Stephen Warren wrote: >> On 08/27/2012 09:24 AM, Arend van Spriel wrote: >>> On 08/27/2012 12:25 PM, Wei Ni wrote: >>>> In case of inband interrupts, if we handle the interrupt in dpc thread, >>>> two level of thread switching takes place to process wifi interrupts. >>>> One in SDHCI driver and the other in Wifi driver. This may cause the >>>> system >>>> instability. >>> >>> Looking into the sdhci/mmc code indeed shows that the brcmfmac irq >>> handler is not called in true IRQ context. So the dpc thread may add >>> unnecessary complexity, but to me there is not indication that there is >>> a stability issue. > > The brcmfmac irq handler is called in the thread sdio_irq_thread(), this > thread indeed is driven by the sdhci irq, although it's not the true IRQ > context. If the brcmfmac doesn't clear the IRQ condition ASAP, the > sdio_irq_thread will be triggered again and again, and in this condition > it's too difficult to run the brcmfmac dpc thread, more and more > interrupt can't be handled. > >>> >>>> Because the SDHCI calls sdio_irq_thread() to handle the irq, this >>>> thread locks >>>> mmc host and calls wifi handler. It expects WiFi handler to be quick and >>>> enables sdio interrupt from card at end. If wifi handler defers this >>>> work for >>>> a different thread, sdio_irq_thread() will be stuck on next wifi >>>> interrupt >>>> since mmc lock is not freed. >>> >>> Not sure if I can follow this explanation. The isr is called with host >>> claimed (by sdio_irq_thread) and all it does is at a linked list member >>> and signal the dpc thread. After doing this the host is released. >> >> Is the issue something like the ISR handler or first level of threading >> does: >> >> * Trigger DPC >> * Re-enable interrupt >> >> So that the interrupt then fires again before the triggered DPC can run >> to handle/clear it, thus causing an interrupt storm? >> >> Whereas handling the interrupt directly prevents this race condition? > > Above is my understanding. > Hi Wei, I understand the issue here and totally agree that we should treat in-band and out-band interrupts differently. But my concern is that the behavior of releasing the host before calling brcmf_sdbrcm_isr and grab it after is likely error prone. Also we are restructuring the dpc routine internally and it's almost done. I will find a better solution for in-band interrupt and get it the queue as well. So I suggest dropping this patch. Thanks, Franky -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/28/2012 09:45 AM, Franky Lin wrote: > On 08/28/2012 04:13 AM, Wei Ni wrote: >> On Tue, 2012-08-28 at 04:06 +0800, Stephen Warren wrote: >>> On 08/27/2012 09:24 AM, Arend van Spriel wrote: >>>> On 08/27/2012 12:25 PM, Wei Ni wrote: >>>>> In case of inband interrupts, if we handle the interrupt in dpc >>>>> thread, >>>>> two level of thread switching takes place to process wifi interrupts. >>>>> One in SDHCI driver and the other in Wifi driver. This may cause the >>>>> system >>>>> instability. ... >>>> Not sure if I can follow this explanation. The isr is called with host >>>> claimed (by sdio_irq_thread) and all it does is at a linked list member >>>> and signal the dpc thread. After doing this the host is released. >>> >>> Is the issue something like the ISR handler or first level of threading >>> does: >>> >>> * Trigger DPC >>> * Re-enable interrupt >>> >>> So that the interrupt then fires again before the triggered DPC can run >>> to handle/clear it, thus causing an interrupt storm? >>> >>> Whereas handling the interrupt directly prevents this race condition? >> >> Above is my understanding. > > I understand the issue here and totally agree that we should treat > in-band and out-band interrupts differently. But my concern is that the > behavior of releasing the host before calling brcmf_sdbrcm_isr and grab > it after is likely error prone. Also we are restructuring the dpc > routine internally and it's almost done. I will find a better solution > for in-band interrupt and get it the queue as well. So I suggest > dropping this patch. Franky, do you know which kernel release the DPC restructuring will make it into? I ask because I can't apply the rest of the patches in this series without first resolving the stability issues with the Broadcom WiFi enabled, since that'd de-stabilize the Tegra platform significantly, and I'd like to plan when we can apply these patches to Tegra. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/28/2012 03:39 PM, Stephen Warren wrote: > On 08/28/2012 09:45 AM, Franky Lin wrote: >> On 08/28/2012 04:13 AM, Wei Ni wrote: >>> On Tue, 2012-08-28 at 04:06 +0800, Stephen Warren wrote: >>>> On 08/27/2012 09:24 AM, Arend van Spriel wrote: >>>>> On 08/27/2012 12:25 PM, Wei Ni wrote: >>>>>> In case of inband interrupts, if we handle the interrupt in dpc >>>>>> thread, >>>>>> two level of thread switching takes place to process wifi interrupts. >>>>>> One in SDHCI driver and the other in Wifi driver. This may cause the >>>>>> system >>>>>> instability. > ... >>>>> Not sure if I can follow this explanation. The isr is called with host >>>>> claimed (by sdio_irq_thread) and all it does is at a linked list member >>>>> and signal the dpc thread. After doing this the host is released. >>>> >>>> Is the issue something like the ISR handler or first level of threading >>>> does: >>>> >>>> * Trigger DPC >>>> * Re-enable interrupt >>>> >>>> So that the interrupt then fires again before the triggered DPC can run >>>> to handle/clear it, thus causing an interrupt storm? >>>> >>>> Whereas handling the interrupt directly prevents this race condition? >>> >>> Above is my understanding. >> >> I understand the issue here and totally agree that we should treat >> in-band and out-band interrupts differently. But my concern is that the >> behavior of releasing the host before calling brcmf_sdbrcm_isr and grab >> it after is likely error prone. Also we are restructuring the dpc >> routine internally and it's almost done. I will find a better solution >> for in-band interrupt and get it the queue as well. So I suggest >> dropping this patch. > > Franky, do you know which kernel release the DPC restructuring will make > it into? I ask because I can't apply the rest of the patches in this > series without first resolving the stability issues with the Broadcom > WiFi enabled, since that'd de-stabilize the Tegra platform > significantly, and I'd like to plan when we can apply these patches to > Tegra. Thanks! > Hi Stephen, Since we submit patches through linux-wireless tree, you may only be able to pick it up at 3.7-rc1. It's quite a big change so I don't think it will qualify as a bug fix to get into 3.6-rcX. Regards, Franky -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/28/2012 04:01 PM, Franky Lin wrote: > On 08/28/2012 03:39 PM, Stephen Warren wrote: >> On 08/28/2012 09:45 AM, Franky Lin wrote: >>> On 08/28/2012 04:13 AM, Wei Ni wrote: >>>> On Tue, 2012-08-28 at 04:06 +0800, Stephen Warren wrote: >>>>> On 08/27/2012 09:24 AM, Arend van Spriel wrote: >>>>>> On 08/27/2012 12:25 PM, Wei Ni wrote: >>>>>>> In case of inband interrupts, if we handle the interrupt in dpc >>>>>>> thread, >>>>>>> two level of thread switching takes place to process wifi >>>>>>> interrupts. >>>>>>> One in SDHCI driver and the other in Wifi driver. This may cause the >>>>>>> system >>>>>>> instability. >> ... >>>>>> Not sure if I can follow this explanation. The isr is called with >>>>>> host >>>>>> claimed (by sdio_irq_thread) and all it does is at a linked list >>>>>> member >>>>>> and signal the dpc thread. After doing this the host is released. >>>>> >>>>> Is the issue something like the ISR handler or first level of >>>>> threading >>>>> does: >>>>> >>>>> * Trigger DPC >>>>> * Re-enable interrupt >>>>> >>>>> So that the interrupt then fires again before the triggered DPC can >>>>> run >>>>> to handle/clear it, thus causing an interrupt storm? >>>>> >>>>> Whereas handling the interrupt directly prevents this race condition? >>>> >>>> Above is my understanding. >>> >>> I understand the issue here and totally agree that we should treat >>> in-band and out-band interrupts differently. But my concern is that the >>> behavior of releasing the host before calling brcmf_sdbrcm_isr and grab >>> it after is likely error prone. Also we are restructuring the dpc >>> routine internally and it's almost done. I will find a better solution >>> for in-band interrupt and get it the queue as well. So I suggest >>> dropping this patch. >> >> Franky, do you know which kernel release the DPC restructuring will make >> it into? I ask because I can't apply the rest of the patches in this >> series without first resolving the stability issues with the Broadcom >> WiFi enabled, since that'd de-stabilize the Tegra platform >> significantly, and I'd like to plan when we can apply these patches to >> Tegra. Thanks! >> > > Hi Stephen, > > Since we submit patches through linux-wireless tree, you may only be > able to pick it up at 3.7-rc1. It's quite a big change so I don't think > it will qualify as a bug fix to get into 3.6-rcX. That's as quick as I expected it to show up, so that's great. I don't suppose you could mail Wei and myself once the patch gets into the linux-wireless tree, so we can test it out on Tegra. If the patch could possibly go into a topic branch in the wireless tree so it can be merged into the Tegra tree before this series, that would be awesome. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/28/2012 04:04 PM, Stephen Warren wrote: > On 08/28/2012 04:01 PM, Franky Lin wrote: >> On 08/28/2012 03:39 PM, Stephen Warren wrote: >>> On 08/28/2012 09:45 AM, Franky Lin wrote: >>>> On 08/28/2012 04:13 AM, Wei Ni wrote: >>>>> On Tue, 2012-08-28 at 04:06 +0800, Stephen Warren wrote: >>>>>> On 08/27/2012 09:24 AM, Arend van Spriel wrote: >>>>>>> On 08/27/2012 12:25 PM, Wei Ni wrote: >>>>>>>> In case of inband interrupts, if we handle the interrupt in dpc >>>>>>>> thread, >>>>>>>> two level of thread switching takes place to process wifi >>>>>>>> interrupts. >>>>>>>> One in SDHCI driver and the other in Wifi driver. This may cause the >>>>>>>> system >>>>>>>> instability. >>> ... >>>>>>> Not sure if I can follow this explanation. The isr is called with >>>>>>> host >>>>>>> claimed (by sdio_irq_thread) and all it does is at a linked list >>>>>>> member >>>>>>> and signal the dpc thread. After doing this the host is released. >>>>>> >>>>>> Is the issue something like the ISR handler or first level of >>>>>> threading >>>>>> does: >>>>>> >>>>>> * Trigger DPC >>>>>> * Re-enable interrupt >>>>>> >>>>>> So that the interrupt then fires again before the triggered DPC can >>>>>> run >>>>>> to handle/clear it, thus causing an interrupt storm? >>>>>> >>>>>> Whereas handling the interrupt directly prevents this race condition? >>>>> >>>>> Above is my understanding. >>>> >>>> I understand the issue here and totally agree that we should treat >>>> in-band and out-band interrupts differently. But my concern is that the >>>> behavior of releasing the host before calling brcmf_sdbrcm_isr and grab >>>> it after is likely error prone. Also we are restructuring the dpc >>>> routine internally and it's almost done. I will find a better solution >>>> for in-band interrupt and get it the queue as well. So I suggest >>>> dropping this patch. >>> >>> Franky, do you know which kernel release the DPC restructuring will make >>> it into? I ask because I can't apply the rest of the patches in this >>> series without first resolving the stability issues with the Broadcom >>> WiFi enabled, since that'd de-stabilize the Tegra platform >>> significantly, and I'd like to plan when we can apply these patches to >>> Tegra. Thanks! >>> >> >> Hi Stephen, >> >> Since we submit patches through linux-wireless tree, you may only be >> able to pick it up at 3.7-rc1. It's quite a big change so I don't think >> it will qualify as a bug fix to get into 3.6-rcX. > > That's as quick as I expected it to show up, so that's great. I don't > suppose you could mail Wei and myself once the patch gets into the > linux-wireless tree, so we can test it out on Tegra. If the patch could > possibly go into a topic branch in the wireless tree so it can be merged > into the Tegra tree before this series, that would be awesome. Thanks. > Will keep you posted. Franky -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c index 8e7e692..5cf6c3b 100644 --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c @@ -121,7 +121,9 @@ static void brcmf_sdio_irqhandler(struct sdio_func *func) brcmf_dbg(INTR, "ib intr triggered\n"); + sdio_release_host(sdiodev->func[1]); brcmf_sdbrcm_isr(sdiodev->bus); + sdio_claim_host(sdiodev->func[1]); } /* dummy handler for SDIO function 2 interrupt */ diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c index 472f2ef..4576d59 100644 --- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c +++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c @@ -2347,7 +2347,7 @@ static bool brcmf_sdbrcm_dpc(struct brcmf_sdio *bus) uint framecnt = 0; /* Temporary counter of tx/rx frames */ bool rxdone = true; /* Flag for no more read data */ bool resched = false; /* Flag indicating resched wanted */ - int err; + int err = 0; brcmf_dbg(TRACE, "Enter\n"); @@ -3786,11 +3786,17 @@ void brcmf_sdbrcm_isr(void *arg) if (!bus->intr) brcmf_dbg(ERROR, "isr w/o interrupt configured!\n"); + +#ifndef CONFIG_BRCMFMAC_SDIO_OOB + while (brcmf_sdbrcm_dpc(bus)) + ; +#else bus->dpc_sched = true; if (bus->dpc_tsk) { brcmf_sdbrcm_adddpctsk(bus); complete(&bus->dpc_wait); } +#endif } static bool brcmf_sdbrcm_bus_watchdog(struct brcmf_sdio *bus)
In case of inband interrupts, if we handle the interrupt in dpc thread, two level of thread switching takes place to process wifi interrupts. One in SDHCI driver and the other in Wifi driver. This may cause the system instability. Because the SDHCI calls sdio_irq_thread() to handle the irq, this thread locks mmc host and calls wifi handler. It expects WiFi handler to be quick and enables sdio interrupt from card at end. If wifi handler defers this work for a different thread, sdio_irq_thread() will be stuck on next wifi interrupt since mmc lock is not freed. Handling the interrupt in ISR directly will prevent thread context switching in wifi driver. It can fix the instability problems. Signed-off-by: Wei Ni <wni@nvidia.com> --- drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 2 ++ drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletions(-)