diff mbox

[RFC,v2] ath10k: move code from parameter list into a function

Message ID 1426100519-20636-1-git-send-email-hofrat@osadl.org (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Nicholas Mc Guire March 11, 2015, 7:01 p.m. UTC
Putting code into the parameter list of wait_event_timeout() might be
legal C-code but not really readable - the "inline" code is simply
moved into a function and that passed to wait_event_timeout() as the
condition. As wait_event_timeout will always return >= 0 the following
timeout check is fixed up to  ret == 0 .

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Thanks to Bjorn Mork <bjorn@mork.no> for clarifying my initial confusion !

v2: Thanks to Pat Erley <pat-lkml@erley.org> for catching the botched skip
    parameter - should have been call by reference not value - fixed here.

As Bjorn Mork <bjorn@mork.no> pointed out, a patch like this is hard to
verify by testing - but I do think that the code readability is much
better (if the patch is correct with respect to code behavior) and helps
simplify/automate API usage checking so its worth the effort.

Comments on if this is functionally equivalent to the original code as well
as if the helper func name is suitable would be appreciated.

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m and the generated mac.i file code reviewed.

Patch is against 4.0-rc3 (localversion-next is -next-20150311)

 drivers/net/wireless/ath/ath10k/mac.c |   34 ++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

Comments

Johannes Berg March 11, 2015, 7:10 p.m. UTC | #1
On Wed, 2015-03-11 at 15:01 -0400, Nicholas Mc Guire wrote:
> Putting code into the parameter list of wait_event_timeout() might be
> legal C-code but not really readable - the "inline" code is simply
> moved into a function and that passed to wait_event_timeout() as the
> condition.

Arguably, that's even more unreadable since if you don't know this macro
well you might assume the function is called only once, which is clearly
not true...

Don't get me wrong, I'm not opposed to this change, but if you ask me
it's not completely clear that this makes it more readable.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas Mc Guire March 11, 2015, 7:15 p.m. UTC | #2
On Wed, 11 Mar 2015, Johannes Berg wrote:

> On Wed, 2015-03-11 at 15:01 -0400, Nicholas Mc Guire wrote:
> > Putting code into the parameter list of wait_event_timeout() might be
> > legal C-code but not really readable - the "inline" code is simply
> > moved into a function and that passed to wait_event_timeout() as the
> > condition.
> 
> Arguably, that's even more unreadable since if you don't know this macro
> well you might assume the function is called only once, which is clearly
> not true...
> 
> Don't get me wrong, I'm not opposed to this change, but if you ask me
> it's not completely clear that this makes it more readable.
>
I'm not into this long enough to say what is better and if the consensus
is that this patch is no more readable than the original code
and no more maintainable either, then it is not worth the effort.

so thanks for your comments!

thx!
hofrat 
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg March 11, 2015, 7:22 p.m. UTC | #3
On Wed, 2015-03-11 at 20:15 +0100, Nicholas Mc Guire wrote:

> I'm not into this long enough to say what is better and if the consensus
> is that this patch is no more readable than the original code
> and no more maintainable either, then it is not worth the effort.

Personally, I like the change, it might even make that function more
reusable etc., and it gets rid of the ({ ... }) expression syntax which
I always find a bit odd :)

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov March 11, 2015, 8:29 p.m. UTC | #4
Hello.

On 03/11/2015 10:01 PM, Nicholas Mc Guire wrote:

> Putting code into the parameter list of wait_event_timeout() might be
> legal C-code but not really readable - the "inline" code is simply
> moved into a function and that passed to wait_event_timeout() as the
> condition. As wait_event_timeout will always return >= 0 the following
> timeout check is fixed up to  ret == 0 .

> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
[...]

> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index e8cc19f..a7a12cc 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);

    Parens not needed here.

[...]

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas Mc Guire March 12, 2015, 7:32 a.m. UTC | #5
On Wed, 11 Mar 2015, Sergei Shtylyov wrote:

> Hello.
>
> On 03/11/2015 10:01 PM, Nicholas Mc Guire wrote:
>
>> Putting code into the parameter list of wait_event_timeout() might be
>> legal C-code but not really readable - the "inline" code is simply
>> moved into a function and that passed to wait_event_timeout() as the
>> condition. As wait_event_timeout will always return >= 0 the following
>> timeout check is fixed up to  ret == 0 .
>
>> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> [...]
>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index e8cc19f..a7a12cc 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);
>
>    Parens not needed here.
>
Thanks - will see what else I forgot and then repost a cleaned up
version.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index e8cc19f..a7a12cc 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,21 +4494,11 @@  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)
+	if (ret == 0 || skip)
 		ath10k_warn(ar, "failed to flush transmit queue (skip %i ar-state %i): %i\n",
 			    skip, ar->state, ret);