[PATCH 0/4] Improvments for wlcore irq and resume for v5.9

[PATCH 0/4] Improvments for wlcore irq and resume for v5.9

[Tony Lindgren]

Hi,

Here are few improvments for wlcore interrupt handling and resume ELP
handling for v5.9.

Regards,

Tony


Tony Lindgren (4):
  wlcore: Use spin_trylock in wlcore_irq_locked() for running the queue
  wlcore: Use spin_trylock in wlcore_irq() to see if we need to queue tx
  wlcore: Simplify runtime resume ELP path
  wlcore: Remove pointless spinlock

 drivers/net/wireless/ti/wlcore/main.c | 84 +++++++++++++--------------
 1 file changed, 39 insertions(+), 45 deletions(-)

-- 
2.27.0

[PATCH 1/4] wlcore: Use spin_trylock in wlcore_irq_locked() for running the queue

[Tony Lindgren]

We need the spinlock to check if we need to run the queue. Let's use
spin_trylock instead and always run the queue unless we know there's
nothing to do.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/wireless/ti/wlcore/main.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -521,6 +521,7 @@ static int wlcore_irq_locked(struct wl1271 *wl)
 	int ret = 0;
 	u32 intr;
 	int loopcount = WL1271_IRQ_MAX_LOOPS;
+	bool run_tx_queue = true;
 	bool done = false;
 	unsigned int defer_count;
 	unsigned long flags;
@@ -586,19 +587,22 @@ static int wlcore_irq_locked(struct wl1271 *wl)
 				goto err_ret;
 
 			/* Check if any tx blocks were freed */
-			spin_lock_irqsave(&wl->wl_lock, flags);
-			if (!test_bit(WL1271_FLAG_FW_TX_BUSY, &wl->flags) &&
-			    wl1271_tx_total_queue_count(wl) > 0) {
-				spin_unlock_irqrestore(&wl->wl_lock, flags);
+			if (!test_bit(WL1271_FLAG_FW_TX_BUSY, &wl->flags)) {
+				if (spin_trylock_irqsave(&wl->wl_lock, flags)) {
+					if (!wl1271_tx_total_queue_count(wl))
+						run_tx_queue = false;
+					spin_unlock_irqrestore(&wl->wl_lock, flags);
+				}
+
 				/*
 				 * In order to avoid starvation of the TX path,
 				 * call the work function directly.
 				 */
-				ret = wlcore_tx_work_locked(wl);
-				if (ret < 0)
-					goto err_ret;
-			} else {
-				spin_unlock_irqrestore(&wl->wl_lock, flags);
+				if (run_tx_queue) {
+					ret = wlcore_tx_work_locked(wl);
+					if (ret < 0)
+						goto err_ret;
+				}
 			}
 
 			/* check for tx results */
-- 
2.27.0

[PATCH 2/4] wlcore: Use spin_trylock in wlcore_irq() to see if we need to queue tx

[Tony Lindgren]

We need the spinlock to check if we need to queue tx in wlcore_irq().
Let's use spin_trylock instead and always queue tx unless we know there's
nothing to do.

Let's also update the comment a bit while at it.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/wireless/ti/wlcore/main.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -652,6 +652,7 @@ static irqreturn_t wlcore_irq(int irq, void *cookie)
 	int ret;
 	unsigned long flags;
 	struct wl1271 *wl = cookie;
+	bool queue_tx_work = true;
 
 	/* complete the ELP completion */
 	spin_lock_irqsave(&wl->wl_lock, flags);
@@ -682,13 +683,17 @@ static irqreturn_t wlcore_irq(int irq, void *cookie)
 	if (ret)
 		wl12xx_queue_recovery_work(wl);
 
-	spin_lock_irqsave(&wl->wl_lock, flags);
-	/* In case TX was not handled here, queue TX work */
+	/* In case TX was not handled in wlcore_irq_locked(), queue TX work */
 	clear_bit(WL1271_FLAG_TX_PENDING, &wl->flags);
-	if (!test_bit(WL1271_FLAG_FW_TX_BUSY, &wl->flags) &&
-	    wl1271_tx_total_queue_count(wl) > 0)
-		ieee80211_queue_work(wl->hw, &wl->tx_work);
-	spin_unlock_irqrestore(&wl->wl_lock, flags);
+	if (!test_bit(WL1271_FLAG_FW_TX_BUSY, &wl->flags)) {
+		if (spin_trylock_irqsave(&wl->wl_lock, flags)) {
+			if (!wl1271_tx_total_queue_count(wl))
+				queue_tx_work = false;
+			spin_unlock_irqrestore(&wl->wl_lock, flags);
+		}
+		if (queue_tx_work)
+			ieee80211_queue_work(wl->hw, &wl->tx_work);
+	}
 
 	mutex_unlock(&wl->mutex);
 
-- 
2.27.0

[PATCH 3/4] wlcore: Simplify runtime resume ELP path

[Tony Lindgren]

We can simplify the runtime resume ELP path by always setting and
clearing the completion in runtime resume. This way we can test for
WL1271_FLAG_IRQ_RUNNING after the resume write to see if we need
completion at all.

And in wlcore_irq(), we only need to take spinlock for running the
completion.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/wireless/ti/wlcore/main.c | 43 ++++++++++-----------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -654,24 +654,26 @@ static irqreturn_t wlcore_irq(int irq, void *cookie)
 	struct wl1271 *wl = cookie;
 	bool queue_tx_work = true;
 
-	/* 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;
+
+	/* complete the ELP completion */
+	if (test_bit(WL1271_FLAG_IN_ELP, &wl->flags)) {
+		spin_lock_irqsave(&wl->wl_lock, flags);
+		if (wl->elp_compl)
+			complete(wl->elp_compl);
+		spin_unlock_irqrestore(&wl->wl_lock, flags);
 	}
 
 	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");
+		spin_lock_irqsave(&wl->wl_lock, flags);
 		disable_irq_nosync(wl->irq);
 		pm_wakeup_event(wl->dev, 0);
 		spin_unlock_irqrestore(&wl->wl_lock, flags);
 		goto out_handled;
 	}
-	spin_unlock_irqrestore(&wl->wl_lock, flags);
 
 	/* TX might be handled here, avoid redundant work */
 	set_bit(WL1271_FLAG_TX_PENDING, &wl->flags);
@@ -6741,7 +6743,6 @@ static int __maybe_unused wlcore_runtime_resume(struct device *dev)
 	unsigned long flags;
 	int ret;
 	unsigned long start_time = jiffies;
-	bool pending = false;
 	bool recovery = false;
 
 	/* Nothing to do if no ELP mode requested */
@@ -6751,49 +6752,35 @@ static int __maybe_unused wlcore_runtime_resume(struct device *dev)
 	wl1271_debug(DEBUG_PSM, "waking up chip from elp");
 
 	spin_lock_irqsave(&wl->wl_lock, flags);
-	if (test_bit(WL1271_FLAG_IRQ_RUNNING, &wl->flags))
-		pending = true;
-	else
-		wl->elp_compl = &compl;
+	wl->elp_compl = &compl;
 	spin_unlock_irqrestore(&wl->wl_lock, flags);
 
 	ret = wlcore_raw_write32(wl, HW_ACCESS_ELP_CTRL_REG, ELPCTRL_WAKE_UP);
 	if (ret < 0) {
 		recovery = true;
-		goto err;
-	}
-
-	if (!pending) {
+	} else if (!test_bit(WL1271_FLAG_IRQ_RUNNING, &wl->flags)) {
 		ret = wait_for_completion_timeout(&compl,
 			msecs_to_jiffies(WL1271_WAKEUP_TIMEOUT));
 		if (ret == 0) {
 			wl1271_warning("ELP wakeup timeout!");
-
-			/* Return no error for runtime PM for recovery */
-			ret = 0;
 			recovery = true;
-			goto err;
 		}
 	}
 
-	clear_bit(WL1271_FLAG_IN_ELP, &wl->flags);
-
-	wl1271_debug(DEBUG_PSM, "wakeup time: %u ms",
-		     jiffies_to_msecs(jiffies - start_time));
-
-	return 0;
-
-err:
 	spin_lock_irqsave(&wl->wl_lock, flags);
 	wl->elp_compl = NULL;
 	spin_unlock_irqrestore(&wl->wl_lock, flags);
+	clear_bit(WL1271_FLAG_IN_ELP, &wl->flags);
 
 	if (recovery) {
 		set_bit(WL1271_FLAG_INTENDED_FW_RECOVERY, &wl->flags);
 		wl12xx_queue_recovery_work(wl);
+	} else {
+		wl1271_debug(DEBUG_PSM, "wakeup time: %u ms",
+			     jiffies_to_msecs(jiffies - start_time));
 	}
 
-	return ret;
+	return 0;
 }
 
 static const struct dev_pm_ops wlcore_pm_ops = {
-- 
2.27.0

[PATCH 4/4] wlcore: Remove pointless spinlock

[Tony Lindgren]

No need to take a spinlock here.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/wireless/ti/wlcore/main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -700,9 +700,7 @@ static irqreturn_t wlcore_irq(int irq, void *cookie)
 	mutex_unlock(&wl->mutex);
 
 out_handled:
-	spin_lock_irqsave(&wl->wl_lock, flags);
 	clear_bit(WL1271_FLAG_IRQ_RUNNING, &wl->flags);
-	spin_unlock_irqrestore(&wl->wl_lock, flags);
 
 	return IRQ_HANDLED;
 }
-- 
2.27.0

Re: [PATCH 1/4] wlcore: Use spin_trylock in wlcore_irq_locked() for running the queue

[Kalle Valo]

Tony Lindgren <tony@atomide.com> writes:

> We need the spinlock to check if we need to run the queue. Let's use
> spin_trylock instead and always run the queue unless we know there's
> nothing to do.

Why? What's the problem you are solving here?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 2/4] wlcore: Use spin_trylock in wlcore_irq() to see if we need to queue tx

[Kalle Valo]

Tony Lindgren <tony@atomide.com> writes:

> We need the spinlock to check if we need to queue tx in wlcore_irq().
> Let's use spin_trylock instead and always queue tx unless we know there's
> nothing to do.
>
> Let's also update the comment a bit while at it.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Same question as in patch 1, the background for this is not clear.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/4] wlcore: Use spin_trylock in wlcore_irq_locked() for running the queue

[Tony Lindgren]

* Kalle Valo <kvalo@codeaurora.org> [200622 14:15]:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > We need the spinlock to check if we need to run the queue. Let's use
> > spin_trylock instead and always run the queue unless we know there's
> > nothing to do.
> 
> Why? What's the problem you are solving here?

To simplify the flags and locking use between the threaded irq
and tx work.

While chasing an occasional hang with an idle wlan doing just a
periodic network scans, I noticed we can start simplifying the
locking between the threaded irq and tx work for the driver.

No luck so far figuring out what the occasional idle wlan hang is,
but I suspect we end up somewhere in a deadlock between tx work
and the threaded irq.

We currently have a collection of flags and locking between the
threaded irq and tx work:

- wl->flags bitops
- wl->mutex
- wl->wl_lock spinlock

The bitops flags do not need a spinlock around them, and
wlcore_irq() already holds the mutex calling wlcore_irq_locked().
And we only need the spinlock to see if we need to run the queue
or not.

So I think eventually we can remove most of the spinlock use in
favor of the mutex. I guess I could leave out the trylock changes
here if this is too many changes at once.

Or do you see some problem in general with this approach?

Regards,

Tony

Re: [PATCH 1/4] wlcore: Use spin_trylock in wlcore_irq_locked() for running the queue

[Kalle Valo]

Tony Lindgren <tony@atomide.com> writes:

> * Kalle Valo <kvalo@codeaurora.org> [200622 14:15]:
>> Tony Lindgren <tony@atomide.com> writes:
>> 
>> > We need the spinlock to check if we need to run the queue. Let's use
>> > spin_trylock instead and always run the queue unless we know there's
>> > nothing to do.
>> 
>> Why? What's the problem you are solving here?
>
> To simplify the flags and locking use between the threaded irq
> and tx work.
>
> While chasing an occasional hang with an idle wlan doing just a
> periodic network scans, I noticed we can start simplifying the
> locking between the threaded irq and tx work for the driver.
>
> No luck so far figuring out what the occasional idle wlan hang is,
> but I suspect we end up somewhere in a deadlock between tx work
> and the threaded irq.
>
> We currently have a collection of flags and locking between the
> threaded irq and tx work:
>
> - wl->flags bitops
> - wl->mutex
> - wl->wl_lock spinlock
>
> The bitops flags do not need a spinlock around them, and
> wlcore_irq() already holds the mutex calling wlcore_irq_locked().
> And we only need the spinlock to see if we need to run the queue
> or not.
>
> So I think eventually we can remove most of the spinlock use in
> favor of the mutex. I guess I could leave out the trylock changes
> here if this is too many changes at once.
>
> Or do you see some problem in general with this approach?

My only problem was lack of background information in the commit logs.
Conditional locking is tricky and I didn't figure out why you are doing
that and why it's safe to do. So if you could send v2 with the
information above in the commit log I would be happy.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/4] wlcore: Use spin_trylock in wlcore_irq_locked() for running the queue

[Tony Lindgren]

* Kalle Valo <kvalo@codeaurora.org> [200623 06:46]:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > * Kalle Valo <kvalo@codeaurora.org> [200622 14:15]:
> >> Tony Lindgren <tony@atomide.com> writes:
> >> 
> >> > We need the spinlock to check if we need to run the queue. Let's use
> >> > spin_trylock instead and always run the queue unless we know there's
> >> > nothing to do.
> >> 
> >> Why? What's the problem you are solving here?
> >
> > To simplify the flags and locking use between the threaded irq
> > and tx work.
> >
> > While chasing an occasional hang with an idle wlan doing just a
> > periodic network scans, I noticed we can start simplifying the
> > locking between the threaded irq and tx work for the driver.
> >
> > No luck so far figuring out what the occasional idle wlan hang is,
> > but I suspect we end up somewhere in a deadlock between tx work
> > and the threaded irq.
> >
> > We currently have a collection of flags and locking between the
> > threaded irq and tx work:
> >
> > - wl->flags bitops
> > - wl->mutex
> > - wl->wl_lock spinlock
> >
> > The bitops flags do not need a spinlock around them, and
> > wlcore_irq() already holds the mutex calling wlcore_irq_locked().
> > And we only need the spinlock to see if we need to run the queue
> > or not.
> >
> > So I think eventually we can remove most of the spinlock use in
> > favor of the mutex. I guess I could leave out the trylock changes
> > here if this is too many changes at once.
> >
> > Or do you see some problem in general with this approach?
> 
> My only problem was lack of background information in the commit logs.
> Conditional locking is tricky and I didn't figure out why you are doing
> that and why it's safe to do. So if you could send v2 with the
> information above in the commit log I would be happy.

OK. I'll update the description for the patches and resend.

Thanks,

Tony