* [PATCH] wlcore: move handling from hardirq to the irq thread function
@ 2013-03-25 10:53 Luciano Coelho
2013-03-25 11:11 ` Felipe Balbi
2013-03-25 15:08 ` Luciano Coelho
0 siblings, 2 replies; 5+ messages in thread
From: Luciano Coelho @ 2013-03-25 10:53 UTC (permalink / raw)
To: linux-wireless, ido, balbi; +Cc: coelho, gregoire
Spin locks and completions are expensive in hard IRQ context and cause
problems with RT kernels. In RT kernels, both spin locks and
completions can schedule(), so we can't use them in hard irq context.
Move handling code into the irq thread function to avoid that.
Reported-by: Gregoire Gentil <gregoire@alwaysinnovating.com>
Signed-off-by: Luciano Coelho <coelho@ti.com>
---
drivers/net/wireless/ti/wlcore/main.c | 53 +++++++++++++--------------------
1 file changed, 21 insertions(+), 32 deletions(-)
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index 248daa9..c2730a7 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -651,6 +651,25 @@ static irqreturn_t wlcore_irq(int irq, void *cookie)
unsigned long flags;
struct wl1271 *wl = cookie;
+ /* complete the ELP completion */
+ spin_lock_irqsave(&wl->wl_lock, flags);
+ set_bit(WL1271_FLAG_IRQ_RUNNING, &wl->flags);
+ if (wl->elp_compl) {
+ complete(wl->elp_compl);
+ wl->elp_compl = NULL;
+ }
+
+ if (test_bit(WL1271_FLAG_SUSPENDED, &wl->flags)) {
+ /* don't enqueue a work right now. mark it as pending */
+ set_bit(WL1271_FLAG_PENDING_WORK, &wl->flags);
+ wl1271_debug(DEBUG_IRQ, "should not enqueue work");
+ disable_irq_nosync(wl->irq);
+ pm_wakeup_event(wl->dev, 0);
+ spin_unlock_irqrestore(&wl->wl_lock, flags);
+ return IRQ_HANDLED;
+ }
+ spin_unlock_irqrestore(&wl->wl_lock, flags);
+
/* TX might be handled here, avoid redundant work */
set_bit(WL1271_FLAG_TX_PENDING, &wl->flags);
cancel_work_sync(&wl->tx_work);
@@ -5998,35 +6017,6 @@ int wlcore_free_hw(struct wl1271 *wl)
}
EXPORT_SYMBOL_GPL(wlcore_free_hw);
-static irqreturn_t wl12xx_hardirq(int irq, void *cookie)
-{
- struct wl1271 *wl = cookie;
- unsigned long flags;
-
- wl1271_debug(DEBUG_IRQ, "IRQ");
-
- /* complete the ELP completion */
- spin_lock_irqsave(&wl->wl_lock, flags);
- set_bit(WL1271_FLAG_IRQ_RUNNING, &wl->flags);
- if (wl->elp_compl) {
- complete(wl->elp_compl);
- wl->elp_compl = NULL;
- }
-
- if (test_bit(WL1271_FLAG_SUSPENDED, &wl->flags)) {
- /* don't enqueue a work right now. mark it as pending */
- set_bit(WL1271_FLAG_PENDING_WORK, &wl->flags);
- wl1271_debug(DEBUG_IRQ, "should not enqueue work");
- disable_irq_nosync(wl->irq);
- pm_wakeup_event(wl->dev, 0);
- spin_unlock_irqrestore(&wl->wl_lock, flags);
- return IRQ_HANDLED;
- }
- spin_unlock_irqrestore(&wl->wl_lock, flags);
-
- return IRQ_WAKE_THREAD;
-}
-
static void wlcore_nvs_cb(const struct firmware *fw, void *context)
{
struct wl1271 *wl = context;
@@ -6068,9 +6058,8 @@ static void wlcore_nvs_cb(const struct firmware *fw, void *context)
else
irqflags = IRQF_TRIGGER_HIGH | IRQF_ONESHOT;
- ret = request_threaded_irq(wl->irq, wl12xx_hardirq, wlcore_irq,
- irqflags,
- pdev->name, wl);
+ ret = request_threaded_irq(wl->irq, NULL, wlcore_irq,
+ irqflags, pdev->name, wl);
if (ret < 0) {
wl1271_error("request_irq() failed: %d", ret);
goto out_free_nvs;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] wlcore: move handling from hardirq to the irq thread function
2013-03-25 10:53 [PATCH] wlcore: move handling from hardirq to the irq thread function Luciano Coelho
@ 2013-03-25 11:11 ` Felipe Balbi
2013-03-25 11:33 ` Luciano Coelho
2013-03-25 15:08 ` Luciano Coelho
1 sibling, 1 reply; 5+ messages in thread
From: Felipe Balbi @ 2013-03-25 11:11 UTC (permalink / raw)
To: Luciano Coelho; +Cc: linux-wireless, ido, balbi, gregoire
[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]
On Mon, Mar 25, 2013 at 12:53:44PM +0200, Luciano Coelho wrote:
> Spin locks and completions are expensive in hard IRQ context and cause
> problems with RT kernels. In RT kernels, both spin locks and
> completions can schedule(), so we can't use them in hard irq context.
>
> Move handling code into the irq thread function to avoid that.
>
> Reported-by: Gregoire Gentil <gregoire@alwaysinnovating.com>
> Signed-off-by: Luciano Coelho <coelho@ti.com>
> ---
> drivers/net/wireless/ti/wlcore/main.c | 53 +++++++++++++--------------------
> 1 file changed, 21 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
> index 248daa9..c2730a7 100644
> --- a/drivers/net/wireless/ti/wlcore/main.c
> +++ b/drivers/net/wireless/ti/wlcore/main.c
> @@ -651,6 +651,25 @@ static irqreturn_t wlcore_irq(int irq, void *cookie)
> unsigned long flags;
> struct wl1271 *wl = cookie;
>
> + /* complete the ELP completion */
> + spin_lock_irqsave(&wl->wl_lock, flags);
> + set_bit(WL1271_FLAG_IRQ_RUNNING, &wl->flags);
> + if (wl->elp_compl) {
> + complete(wl->elp_compl);
> + wl->elp_compl = NULL;
> + }
> +
> + if (test_bit(WL1271_FLAG_SUSPENDED, &wl->flags)) {
> + /* don't enqueue a work right now. mark it as pending */
> + set_bit(WL1271_FLAG_PENDING_WORK, &wl->flags);
> + wl1271_debug(DEBUG_IRQ, "should not enqueue work");
> + disable_irq_nosync(wl->irq);
> + pm_wakeup_event(wl->dev, 0);
> + spin_unlock_irqrestore(&wl->wl_lock, flags);
> + return IRQ_HANDLED;
> + }
> + spin_unlock_irqrestore(&wl->wl_lock, flags);
I still think _irqrestore() here is wrong, since it will reenable the
IRQ line... other than that, it looks alright.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] wlcore: move handling from hardirq to the irq thread function
2013-03-25 11:11 ` Felipe Balbi
@ 2013-03-25 11:33 ` Luciano Coelho
2013-03-25 11:38 ` Felipe Balbi
0 siblings, 1 reply; 5+ messages in thread
From: Luciano Coelho @ 2013-03-25 11:33 UTC (permalink / raw)
To: balbi; +Cc: linux-wireless, ido, gregoire
On Mon, 2013-03-25 at 13:11 +0200, Felipe Balbi wrote:
> On Mon, Mar 25, 2013 at 12:53:44PM +0200, Luciano Coelho wrote:
> > Spin locks and completions are expensive in hard IRQ context and cause
> > problems with RT kernels. In RT kernels, both spin locks and
> > completions can schedule(), so we can't use them in hard irq context.
> >
> > Move handling code into the irq thread function to avoid that.
> >
> > Reported-by: Gregoire Gentil <gregoire@alwaysinnovating.com>
> > Signed-off-by: Luciano Coelho <coelho@ti.com>
> > ---
> > drivers/net/wireless/ti/wlcore/main.c | 53 +++++++++++++--------------------
> > 1 file changed, 21 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
> > index 248daa9..c2730a7 100644
> > --- a/drivers/net/wireless/ti/wlcore/main.c
> > +++ b/drivers/net/wireless/ti/wlcore/main.c
> > @@ -651,6 +651,25 @@ static irqreturn_t wlcore_irq(int irq, void *cookie)
> > unsigned long flags;
> > struct wl1271 *wl = cookie;
> >
> > + /* complete the ELP completion */
> > + spin_lock_irqsave(&wl->wl_lock, flags);
> > + set_bit(WL1271_FLAG_IRQ_RUNNING, &wl->flags);
> > + if (wl->elp_compl) {
> > + complete(wl->elp_compl);
> > + wl->elp_compl = NULL;
> > + }
> > +
> > + if (test_bit(WL1271_FLAG_SUSPENDED, &wl->flags)) {
> > + /* don't enqueue a work right now. mark it as pending */
> > + set_bit(WL1271_FLAG_PENDING_WORK, &wl->flags);
> > + wl1271_debug(DEBUG_IRQ, "should not enqueue work");
> > + disable_irq_nosync(wl->irq);
> > + pm_wakeup_event(wl->dev, 0);
> > + spin_unlock_irqrestore(&wl->wl_lock, flags);
> > + return IRQ_HANDLED;
> > + }
> > + spin_unlock_irqrestore(&wl->wl_lock, flags);
>
> I still think _irqrestore() here is wrong, since it will reenable the
> IRQ line... other than that, it looks alright.
As we discussed earlier, it won't re-enable, since it was not enabled
when we saved. But, in any case, as we agreed, I'll send a separate
patch removing it. Don't want to mix two things in the same patch.
--
Luca.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] wlcore: move handling from hardirq to the irq thread function
2013-03-25 11:33 ` Luciano Coelho
@ 2013-03-25 11:38 ` Felipe Balbi
0 siblings, 0 replies; 5+ messages in thread
From: Felipe Balbi @ 2013-03-25 11:38 UTC (permalink / raw)
To: Luciano Coelho; +Cc: balbi, linux-wireless, ido, gregoire
[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]
On Mon, Mar 25, 2013 at 01:33:25PM +0200, Luciano Coelho wrote:
> On Mon, 2013-03-25 at 13:11 +0200, Felipe Balbi wrote:
> > On Mon, Mar 25, 2013 at 12:53:44PM +0200, Luciano Coelho wrote:
> > > Spin locks and completions are expensive in hard IRQ context and cause
> > > problems with RT kernels. In RT kernels, both spin locks and
> > > completions can schedule(), so we can't use them in hard irq context.
> > >
> > > Move handling code into the irq thread function to avoid that.
> > >
> > > Reported-by: Gregoire Gentil <gregoire@alwaysinnovating.com>
> > > Signed-off-by: Luciano Coelho <coelho@ti.com>
> > > ---
> > > drivers/net/wireless/ti/wlcore/main.c | 53 +++++++++++++--------------------
> > > 1 file changed, 21 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
> > > index 248daa9..c2730a7 100644
> > > --- a/drivers/net/wireless/ti/wlcore/main.c
> > > +++ b/drivers/net/wireless/ti/wlcore/main.c
> > > @@ -651,6 +651,25 @@ static irqreturn_t wlcore_irq(int irq, void *cookie)
> > > unsigned long flags;
> > > struct wl1271 *wl = cookie;
> > >
> > > + /* complete the ELP completion */
> > > + spin_lock_irqsave(&wl->wl_lock, flags);
> > > + set_bit(WL1271_FLAG_IRQ_RUNNING, &wl->flags);
> > > + if (wl->elp_compl) {
> > > + complete(wl->elp_compl);
> > > + wl->elp_compl = NULL;
> > > + }
> > > +
> > > + if (test_bit(WL1271_FLAG_SUSPENDED, &wl->flags)) {
> > > + /* don't enqueue a work right now. mark it as pending */
> > > + set_bit(WL1271_FLAG_PENDING_WORK, &wl->flags);
> > > + wl1271_debug(DEBUG_IRQ, "should not enqueue work");
> > > + disable_irq_nosync(wl->irq);
> > > + pm_wakeup_event(wl->dev, 0);
> > > + spin_unlock_irqrestore(&wl->wl_lock, flags);
> > > + return IRQ_HANDLED;
> > > + }
> > > + spin_unlock_irqrestore(&wl->wl_lock, flags);
> >
> > I still think _irqrestore() here is wrong, since it will reenable the
> > IRQ line... other than that, it looks alright.
>
> As we discussed earlier, it won't re-enable, since it was not enabled
> when we saved. But, in any case, as we agreed, I'll send a separate
> patch removing it. Don't want to mix two things in the same patch.
alright...
Reviewed-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] wlcore: move handling from hardirq to the irq thread function
2013-03-25 10:53 [PATCH] wlcore: move handling from hardirq to the irq thread function Luciano Coelho
2013-03-25 11:11 ` Felipe Balbi
@ 2013-03-25 15:08 ` Luciano Coelho
1 sibling, 0 replies; 5+ messages in thread
From: Luciano Coelho @ 2013-03-25 15:08 UTC (permalink / raw)
To: linux-wireless; +Cc: ido, balbi, gregoire
On Mon, 2013-03-25 at 12:53 +0200, Luciano Coelho wrote:
> Spin locks and completions are expensive in hard IRQ context and cause
> problems with RT kernels. In RT kernels, both spin locks and
> completions can schedule(), so we can't use them in hard irq context.
>
> Move handling code into the irq thread function to avoid that.
>
> Reported-by: Gregoire Gentil <gregoire@alwaysinnovating.com>
> Signed-off-by: Luciano Coelho <coelho@ti.com>
> ---
Applied.
--
Luca.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-03-25 15:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-25 10:53 [PATCH] wlcore: move handling from hardirq to the irq thread function Luciano Coelho
2013-03-25 11:11 ` Felipe Balbi
2013-03-25 11:33 ` Luciano Coelho
2013-03-25 11:38 ` Felipe Balbi
2013-03-25 15:08 ` Luciano Coelho
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.