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 at vt.edu wrote:

On Wed, 11 Mar 2015 15:17:44 +0100, Nicholas Mc Guire said:

quoted

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 at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150311/8d5ad704/attachment.html