| Message ID | 1426100519-20636-1-git-send-email-hofrat@osadl.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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
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
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
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 --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);
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(-)