On Wed, 11 Mar 2015, Bj??rn Mork wrote:
> Valdis.Kletnieks@xxxxxx writes:
>
> > On Wed, 11 Mar 2015 15:17:44 +0100, Nicholas Mc Guire said:
> >
> >> So the wait_event_timeout condition here ends up being (empty || skip)
> >> but what is the point of puting this code into the parameter list of
> >> wait_event_timeout() ?
> >>
> >> Would it not be equivalent to:
> >>
> >> bool empty;
> >> ...
> >>
> >> spin_lock_bh(&ar->htt.tx_lock);
> >> empty = (ar->htt.num_pending_tx == 0);
> >> spin_unlock_bh(&ar->htt.tx_lock);
> >>
> >> skip = (ar->state == ATH10K_STATE_WEDGED) ||
> >> test_bit(ATH10K_FLAG_CRASH_FLUSH,
> >> &ar->dev_flags);
> >>
> >> ret = wait_event_timeout(ar->htt.empty_tx_wq, (empty || skip),
> >> ATH10K_FLUSH_TIMEOUT_HZ);
> >>
> >> What am I missing here ?
> >
> > Umm... a Signed-off-by: and formatting it as an actual patch? :)
> >
> > Seriously - you're right, it's ugly code that needs fixing...
>
> Huh?
>
> The condition needs to be re-evaluated every time the process wakes up.
> Evaluating it once and then reusing that result is not the same.
> Something elseis supposed to modify ar->htt.num_pending_tx, ar->state or
> ar->dev_flags while we are waiting.
>
after picking appart the mac.i file I see what you mean
(a bit reformated to make it kind of readable)
ret = ({ long __ret = (5*250);
do { _cond_resched(); } while (0);
if (!({
bool __cond = (({
bool empty;
spin_lock_bh(&ar->htt.tx_lock);
empty = (ar->htt.num_pending_tx == 0);
spin_unlock_bh(&ar->htt.tx_lock);
skip = (ar->state == ATH10K_STATE_WEDGED) ||
(__builtin_constant_p((ATH10K_FLAG_CRASH_FLUSH)) ?
constant_test_bit((ATH10K_FLAG_CRASH_FLUSH),
(&ar->dev_flags)) :
variable_test_bit((ATH10K_FLAG_CRASH_FLUSH),
(&ar->dev_flags)));
(empty || skip);
}));
if (__cond && !__ret)
__ret = 1;
__cond || !__ret;
}))
__ret = ({ __label__ __out;
wait_queue_t __wait;
long __ret = (5*250);
INIT_LIST_HEAD(&__wait.task_list);
if (0)
__wait.flags = 0x01;
else
__wait.flags = 0;
for (;;) {
long __int = prepare_to_wait_event(&ar->htt.empty_tx_wq,
&__wait, 2);
if (({
bool __cond = (({
bool empty;
spin_lock_bh(&ar->htt.tx_lock);
empty = (ar->htt.num_pending_tx == 0);
spin_unlock_bh(&ar->htt.tx_lock);
skip = (ar->state == ATH10K_STATE_WEDGED) || (__builtin_constant_p((ATH10K_FLAG_CRASH_FLUSH)) ? constant_test_bit((ATH10K_FLAG_CRASH_FLUSH), (&ar->dev_flags)) : variable_test_bit((ATH10K_FLAG_CRASH_FLUSH), (&ar->dev_flags)));
(empty || skip);
}));
i if (__cond && !__ret)
__ret = 1;
__cond || !__ret;
}))
break;
if ((!__builtin_constant_p(2) || 2 == 1 || 2 == (128 | 2)) && __int) {
__ret = __int;
if (0) {
abort_exclusive_wait(&ar->htt.empty_tx_wq, &__wait, 2, ((void *)0));
goto __out;
}
break;
}
__ret = schedule_timeout(__ret);
}
finish_wait(&ar->htt.empty_tx_wq, &__wait);
__out:
__ret;
});
__ret;
})
So the for(;;){ ... } is the part I missed at first.
but never the less the code should then pack up the inner block into a
static function and pass it to wait_event_timeout rather than
putting the basic block into the parameter list e.g. like in
drivers/gpu/drm/drm_dp_mst_topology.c:drm_dp_mst_wait_tx_reply()
static bool check_txmsg_state(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_sideband_msg_tx *txmsg)
{
bool ret;
mutex_lock(&mgr->qlock);
ret = (txmsg->state == DRM_DP_SIDEBAND_TX_RX ||
txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT);
mutex_unlock(&mgr->qlock);
return ret;
}
drm_dp_mst_wait_tx_reply()
<snip>
ret = wait_event_timeout(mgr->tx_waitq,
check_txmsg_state(mgr, txmsg),
(4 * HZ));
<snip>
which is readable and achieves the same purpose. something like
the below quick shot:
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index e8cc19f..7b27d99 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4463,11 +4463,25 @@ static int ath10k_set_rts_threshold(struct ieee80211_hw *hw, u32 value)
return ret;
}
+static bool check_htt_state (struct ath10k *ar, bool skip)
+{
+ bool empty;
+
+ spin_lock_bh(&ar->htt.tx_lock);
+ empty = (ar->htt.num_pending_tx == 0);
+ spin_unlock_bh(&ar->htt.tx_lock);
+
+ skip = (ar->state == ATH10K_STATE_WEDGED) ||
+ test_bit(ATH10K_FLAG_CRASH_FLUSH,
+ &ar->dev_flags);
+ return (empty || skip);
+}
+
static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
u32 queues, bool drop)
{
struct ath10k *ar = hw->priv;
- bool skip;
+ bool skip = false;
int ret;
/* mac80211 doesn't care if we really xmit queued frames or not
@@ -4480,19 +4494,9 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
if (ar->state == ATH10K_STATE_WEDGED)
goto skip;
- ret = wait_event_timeout(ar->htt.empty_tx_wq, ({
- bool empty;
-
- spin_lock_bh(&ar->htt.tx_lock);
- empty = (ar->htt.num_pending_tx == 0);
- spin_unlock_bh(&ar->htt.tx_lock);
-
- skip = (ar->state == ATH10K_STATE_WEDGED) ||
- test_bit(ATH10K_FLAG_CRASH_FLUSH,
- &ar->dev_flags);
-
- (empty || skip);
- }), ATH10K_FLUSH_TIMEOUT_HZ);
+ ret = wait_event_timeout(ar->htt.empty_tx_wq,
+ check_htt_state(ar, skip),
+ ATH10K_FLUSH_TIMEOUT_HZ);
if (ret <= 0 || skip)
ath10k_warn(ar, "failed to flush transmit queue (skip %i ar-state %i): %i\n",
--
1.7.10.4
not yet sure if its correct though just wrapped it into a function and
compile checked it... and the if (ret <= 0 ... also needs fixing as
wait_event_timeout returns >= 0 always.
thanks for the clarification - think I got it this time.
thx!
hofrat
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@xxxxxxxxxxxxxxxxx
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
