confusing code....whats the point of this construct ?

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Bj??rn Mork wrote:

> Valdis.Kletnieks at vt.edu 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