Hi Johannes,
On 02/20/2013 04:01 PM, Johannes Berg wrote:
> On Mon, 2013-02-18 at 17:08 +0100, Marco Porsch wrote:
>
>> +/**
>> + * ieee80211_mps_init - register callbacks for mesh powersave mode
>> + *
>> + * @hw: the hardware
>> + * @ops: callbacks for this device
>> + *
>> + * called by driver on mesh interface add/remove
>> + */
>> +#ifdef CONFIG_MAC80211_MESH
>> +void ieee80211_mps_init(struct ieee80211_hw *hw,
>> + const struct ieee80211_mps_ops *ops);
>> +#else
>> +static inline void ieee80211_mps_init(struct ieee80211_hw *hw,
>> + const struct ieee80211_mps_ops *ops)
>> +{ return; }
>> +#endif
>
> The "return" there is spurious. Is it really worth providing a static
> inline? It seems drivers might want to #ifdef it anyway so they don't
> have carry around the ops struct and the called functions if mesh isn't
> compiled in.
Ok, that seems reasonable. And that one function would be gone if I just
use additional ieee80211_ops (see below).
>> +static bool mps_doze_check_sta(struct ieee80211_local *local, u64 *nexttbtt)
>> +{
>> + struct sta_info *sta;
>> + bool allow = true;
>> + u64 nexttbtt_min = ULLONG_MAX;
>> +
>> + mutex_lock(&local->sta_mtx);
>> + list_for_each_entry(sta, &local->sta_list, list) {
>> + if (!ieee80211_vif_is_mesh(&sta->sdata->vif) ||
>> + !ieee80211_sdata_running(sta->sdata) ||
>> + sta->plink_state != NL80211_PLINK_ESTAB) {
>> + continue;
>
> This is strange, why bother with the else if there's a continue?
I don't quite get this comment. The current logic is like this:
if (unrelated cases) {
continue;
} else if (related and blocking) {
allow = false;
break;
} else if (related, non-blocking and new minimum) {
min = sta->nexttbtt;
}
>> + } else if (test_sta_flag(sta, WLAN_STA_MPS_WAIT_FOR_CAB) ||
>> + test_sta_flag(sta, WLAN_STA_MPSP_OWNER) ||
>> + test_sta_flag(sta, WLAN_STA_MPSP_RECIPIENT) ||
>> + !timer_pending(&sta->nexttbtt_timer) ||
>> + time_after(jiffies, sta->nexttbtt_jiffies)) {
>
> Are you sure jiffies are good enough? Some systems have HZ=33 or so I
> think, which makes a jiffy like 30ms.
Hm, jiffies is what I have available easily. Using the TSF would be
obvious but may suffer from delay when obtaining it. Umm... hrtimers again?
>> + allow = false;
>> + break;
>> + } else if (sta->nexttbtt_tsf < nexttbtt_min) {
>> + nexttbtt_min = sta->nexttbtt_tsf;
>> + }
>
> ditto, why bother with else after break?
>
>> + if (nexttbtt_min != ULLONG_MAX)
>> + *nexttbtt = nexttbtt_min;
>
> The API of this function is very strange. Sometimes it might set it,
> sometimes it might leave it, but that's not even consistent with the
> "allow" return value ... It seems it'd be better to always set it.
Alright. Will just have to make sure to hand out zero as invalid value,
not ULLONG_MAX.
>> +/**
>> + * ieee80211_mps_doze - trigger radio doze state after checking conditions
>> + *
>> + * @local: local interface data
>
> "interface"? hardly.
* @local: mac80211 hw info struct
>> +void ieee80211_mps_doze(struct ieee80211_local *local)
>> +{
>> + u64 nexttbtt = 0;
>
> and get rid of the assignment here.
>
>> +
>> +void ieee80211_mps_init(struct ieee80211_hw *hw,
>> + const struct ieee80211_mps_ops *ops)
>> +{
>> + struct ieee80211_local *local = hw_to_local(hw);
>> +
>> + if (!ops)
>> + local->mps_enabled = false;
>
> Allowing that seems pointless ... in fact, why is there this assignment
> function anyway? It seems these are pretty normal, if #ifdef MESH,
> driver callbacks?
So may I just add them to the ieee80211_ops under #ifdef? That would
certainly make things easier. I would add a HW capability flag for MPS
doze as well.
--Marco
_______________________________________________
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
