From: Marco Porsch <marco@cozybit.com> To: Johannes Berg <johannes@sipsolutions.net> Cc: mcgrof@qca.qualcomm.com, jouni@qca.qualcomm.com, vthiagar@qca.qualcomm.com, senthilb@qca.qualcomm.com, sleffler@google.com, linux-wireless@vger.kernel.org, devel@lists.open80211s.org, ath9k-devel@lists.ath9k.org Subject: Re: [PATCHv2 2/3] mac80211: mesh power save doze scheduling Date: Mon, 25 Feb 2013 11:06:50 +0100 [thread overview] Message-ID: <512B37BA.6060605@cozybit.com> (raw) In-Reply-To: <1361372480.8629.33.camel@jlt4.sipsolutions.net> 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
WARNING: multiple messages have this Message-ID (diff)
From: Marco Porsch <marco@cozybit.com> To: ath9k-devel@lists.ath9k.org Subject: [ath9k-devel] [PATCHv2 2/3] mac80211: mesh power save doze scheduling Date: Mon, 25 Feb 2013 11:06:50 +0100 [thread overview] Message-ID: <512B37BA.6060605@cozybit.com> (raw) In-Reply-To: <1361372480.8629.33.camel@jlt4.sipsolutions.net> 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
next prev parent reply other threads:[~2013-02-25 10:06 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-02-18 16:08 [PATCHv2 1/3] mac80211: move mesh sync beacon handler into neighbour_update Marco Porsch 2013-02-18 16:08 ` [ath9k-devel] " Marco Porsch 2013-02-18 16:08 ` [PATCHv2 2/3] mac80211: mesh power save doze scheduling Marco Porsch 2013-02-18 16:08 ` [ath9k-devel] " Marco Porsch 2013-02-18 16:41 ` Christian Lamparter 2013-02-18 16:41 ` [ath9k-devel] " Christian Lamparter 2013-02-20 15:01 ` Johannes Berg 2013-02-20 15:01 ` [ath9k-devel] " Johannes Berg 2013-02-25 10:06 ` Marco Porsch [this message] 2013-02-25 10:06 ` Marco Porsch 2013-02-26 20:52 ` Johannes Berg 2013-02-26 20:52 ` [ath9k-devel] " Johannes Berg 2013-02-18 16:08 ` [PATCHv2 3/3] ath9k: mesh powersave support Marco Porsch 2013-02-18 16:08 ` [ath9k-devel] " Marco Porsch 2013-02-20 14:50 ` [PATCHv2 1/3] mac80211: move mesh sync beacon handler into neighbour_update Johannes Berg 2013-02-20 14:50 ` [ath9k-devel] " Johannes Berg 2013-02-20 18:26 ` Thomas Pedersen 2013-02-20 18:26 ` [ath9k-devel] " Thomas Pedersen 2013-02-20 20:00 ` Johannes Berg 2013-02-20 20:00 ` [ath9k-devel] " Johannes Berg
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=512B37BA.6060605@cozybit.com \ --to=marco@cozybit.com \ --cc=ath9k-devel@lists.ath9k.org \ --cc=devel@lists.open80211s.org \ --cc=johannes@sipsolutions.net \ --cc=jouni@qca.qualcomm.com \ --cc=linux-wireless@vger.kernel.org \ --cc=mcgrof@qca.qualcomm.com \ --cc=senthilb@qca.qualcomm.com \ --cc=sleffler@google.com \ --cc=vthiagar@qca.qualcomm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.