Re: [PATCHv2 2/3] mac80211: mesh power save doze scheduling

[Marco Porsch <marco@cozybit.com>]

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