Now I am confused. I thought the code where empty and skip are inside the wait_event_timeout leads to empty beeing evaluated every time that the waiting threads gets awoken.
And since some other thread might change ar->htt.num_pending_tx it is necessary to check this every time we get awoken, rather than once before we go to sleep.

Also the locking part arround empty seems to support my guess (why sync if you do not have multiple threads accessing a variable).

These are only guesses without looking at the surrounding code.
Could you please explain why you think it is sufficient to evaluate the condition only once before sleeping on it? (empty || skip) is a constant if you do not update either empty or skip, at least from my point of view.

On 11/03/15 14:40, Valdis.Kletnieks@vt.edu wrote:
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...



_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies