From: Emmanuel Grumbach <[email protected]>
This patch adds the dtim_period in ieee80211_bss_conf, this allows the low
level driver to know the dtim_period, and to plan power save accordingly.
Signed-off-by: Emmanuel Grumbach <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
include/linux/ieee80211.h | 13 +++++++++++++
include/net/mac80211.h | 1 +
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/mlme.c | 7 +++++++
4 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index a1630ba..7f4df7c 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -506,6 +506,19 @@ struct ieee80211_channel_sw_ie {
u8 count;
} __attribute__ ((packed));
+/**
+ * struct ieee80211_tim
+ *
+ * This structure refers to "Traffic Indication Map information element"
+ */
+struct ieee80211_tim_ie {
+ u8 dtim_count;
+ u8 dtim_period;
+ u8 bitmap_ctrl;
+ /* variable size: 1 - 251 bytes */
+ u8 virtual_map[0];
+} __attribute__ ((packed));
+
struct ieee80211_mgmt {
__le16 frame_control;
__le16 duration;
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 656442c..f249820 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -191,6 +191,7 @@ struct ieee80211_bss_conf {
/* erp related data */
bool use_cts_prot;
bool use_short_preamble;
+ u8 dtim_period;
u16 beacon_int;
u16 assoc_capability;
u64 timestamp;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c916c2f..c36e1e3 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -81,6 +81,7 @@ struct ieee80211_sta_bss {
u8 bssid[ETH_ALEN];
u8 ssid[IEEE80211_MAX_SSID_LEN];
+ u8 dtim_period;
u16 capability; /* host byte order */
enum ieee80211_band band;
int freq;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 00b7186..22eaf86 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -551,6 +551,7 @@ static void ieee80211_set_associated(struct net_device *dev,
/* set timing information */
sdata->bss_conf.beacon_int = bss->beacon_int;
sdata->bss_conf.timestamp = bss->timestamp;
+ sdata->bss_conf.dtim_period = bss->dtim_period;
changed |= ieee80211_handle_bss_capability(sdata, bss);
@@ -2741,6 +2742,12 @@ static void ieee80211_rx_bss_info(struct net_device *dev,
bss->beacon_int = le16_to_cpu(mgmt->u.beacon.beacon_int);
bss->capability = le16_to_cpu(mgmt->u.beacon.capab_info);
+ if (elems->tim) {
+ struct ieee80211_tim_ie *tim_ie =
+ (struct ieee80211_tim_ie *)elems->tim;
+ bss->dtim_period = tim_ie->dtim_period;
+ }
+
bss->supp_rates_len = 0;
if (elems->supp_rates) {
clen = IEEE80211_MAX_SUPP_RATES - bss->supp_rates_len;
--
1.5.4.1
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
Tomas Winkler <[email protected]> writes:
[listen interval]
> This value for iwlwifi hw is derived from maximal supported beacon
> interval and size of the internal HW timers. This value is hard
> coded in the driver.
Ok, now I understand a bit better. Yes, a value like that would be
good to deliver to mac80211.
Just out of curiosity, what's the current hard coded maximum value?
--
Kalle Valo
On Mon, 2008-07-14 at 15:31 +0300, Tomas Winkler wrote:
> This patch makes listen_interval configurable by low level driver.
I'm not sure we we really want that.
With this, you're again putting more features and more decision making
processes with the driver, I would much prefer this to be worked out in
mac80211 so we can do it across all drivers.
If this is a knob that should be adjusted then we want to enable users
to do it, and if it isn't then we can put a value into mac80211 and fix
it. Having drivers make these kinds of decisions is very bad for uniform
wireless behaviour.
johannes
Kalle Valo <[email protected]> writes:
>> This patch should go in and we can reconsider it when other HW
>> driver will implement PS so we can see the bigger picture.
>
> I guess it's a good compromise for now, but as long as we change it
> later.
Sorry, meant to say:
"but as long as we can change it later"
--
Kalle Valo
> sysfs? register_hw()
> user ----------> driver --------------> mac80211
> > This patch should go in and we can reconsider it when other HW
> > driver will implement PS so we can see the bigger picture.
>
> I guess it's a good compromise for now, but as long as we change it
> later.
I disagree. The whole idea of handing an essentially configuration-
dependent value from the driver to the stack is wrong.
johannes
On Tue, 2008-07-15 at 11:29 +0300, Tomas Winkler wrote:
> We are providing power save user interface reach enough to specify all
> the above requirements.
> I think you are both misinterpreting listen interval meaning.
> Listen interval merely says to AP for how many beacons save direct
> packets for this STA. It doesn't mean
> it's can be shorter and it does say it's won't be longer.
Right, but it does make sense to advertise what we're using, and this
patch just adds a strict "driver tells mac80211 what it's using" flow.
That's mostly what I'm objecting to. If you were calling the variable
"max_listen_interval" and having mac80211 send it back to the driver in
the BSS config as bssconf->listen_interval, and, for now, simply use the
max, I wouldn't have a problem with it.
> It's
> doesn't affect power save dynamics it's just sets upper limit.
> This value for iwlwifi hw is derived from maximal supported beacon
> interval and size of the internal HW timers.
> This value is hard coded in the driver.
Shouldn't it depend on the beacon interval then?
johannes
Tomas Winkler <[email protected]> writes:
> This patch adds the dtim_period in ieee80211_bss_conf, this allows the low
> level driver to know the dtim_period, and to plan power save accordingly.
Very good. This is a needed feature in my opinion.
--
Kalle Valo
> This is only required for sending correct value of listen interval to
> AP in association. Anything else is handled in
> the driver or firmware anyway.
The question, however, is _why_ the driver needs to tell mac80211.
What's wrong with mac80211 just deciding on the value? How are you
deciding on the value in iwlwifi?
> This patch should go in and we can reconsider it when other HW driver
> will implement PS so we can see the bigger picture.
That we can still if the driver doesn't get to decide about the value.
johannes
Tomas Winkler <[email protected]> writes:
>> If this is a knob that should be adjusted then we want to enable users
>> to do it, and if it isn't then we can put a value into mac80211 and fix
>> it. Having drivers make these kinds of decisions is very bad for uniform
>> wireless behaviour.
>
> The same story as with beacon interval. This is for the power
> management. It's really hw dependent for how many beacons we can stay
> dormant.
This stuff is very much hardware dependent, I don't deny that, but
still I would assume that the basic principles are almost the same. We
just need to come up with a good interface for the drivers. Sure, it's
more work now, but if we don't do it, PSM support will eventually get
really messy because all drivers do it somehow differently.
> I really don't know what other vendors do and user should not guess
> this.
Yes, for the user this is too difficult, but I think the driver
shouldn't be making the decision either. So mac80211 stack would be
the most logical choice, and it could be make the decisions based on
user input (or something like that).
--
Kalle Valo
On Mon, Jul 14, 2008 at 4:26 PM, Kalle Valo <[email protected]> wrote:
> Tomas Winkler <[email protected]> writes:
>
>>> If this is a knob that should be adjusted then we want to enable users
>>> to do it, and if it isn't then we can put a value into mac80211 and fix
>>> it. Having drivers make these kinds of decisions is very bad for uniform
>>> wireless behaviour.
>>
>> The same story as with beacon interval. This is for the power
>> management. It's really hw dependent for how many beacons we can stay
>> dormant.
>
> This stuff is very much hardware dependent, I don't deny that, but
> still I would assume that the basic principles are almost the same. We
> just need to come up with a good interface for the drivers. Sure, it's
> more work now, but if we don't do it, PSM support will eventually get
> really messy because all drivers do it somehow differently.
>
This patch only sets value of listen interval from the driver I just
think this is exactly what you are suggesting.
This is only required for sending correct value of listen interval to
AP in association. Anything else is handled in
the driver or firmware anyway.
This patch should go in and we can reconsider it when other HW driver
will implement PS so we can see the bigger picture.
Thanks
Tomas
On Mon, Jul 14, 2008 at 3:38 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-07-14 at 15:31 +0300, Tomas Winkler wrote:
>> This patch makes listen_interval configurable by low level driver.
>
> I'm not sure we we really want that.
>
> With this, you're again putting more features and more decision making
> processes with the driver, I would much prefer this to be worked out in
> mac80211 so we can do it across all drivers.
>
> If this is a knob that should be adjusted then we want to enable users
> to do it, and if it isn't then we can put a value into mac80211 and fix
> it. Having drivers make these kinds of decisions is very bad for uniform
> wireless behaviour.
The same story as with beacon interval. This is for the power
management. It's really hw dependent for how many beacons we can stay
dormant. I really don't know what other vendors do and user should not
guess this.
Thanks
Tomas
> johannes
>
Johannes Berg <[email protected]> writes:
> On Mon, 2008-07-14 at 15:31 +0300, Tomas Winkler wrote:
>> This patch makes listen_interval configurable by low level driver.
>
> I'm not sure we we really want that.
Me neither.
> With this, you're again putting more features and more decision making
> processes with the driver, I would much prefer this to be worked out in
> mac80211 so we can do it across all drivers.
I was just thinking the same. In my opinion the driver should be as
simple as possible, and all the logic should be in mac80211 or, if
possible, even in user space.
> If this is a knob that should be adjusted then we want to enable users
> to do it, and if it isn't then we can put a value into mac80211 and fix
> it.
Listen interval is very much dependent on the DTIM period and for
which beacons firmware wakes up (every beacon, every DTIM beacon,
every second DTIM beacon etc). For user space making this decision is
very difficult because it would need to know the current DTIM setting
and capabilities of the driver/hardware. It will get complicated
pretty quickly and I don't see any advantage to push this decision to
user space.
I think we need to soon come up with an idea what kind of PSM control
interface are we going to offer for the user space. Is it going to be
very low level (eg. based on listen interval and DTIMs and what not),
a higher level (eg. levels from one to five, five being the most
aggressive level from powersaving point of view) or what? We have a
plenty of options and I think we should discuss about this at the
summit.
> Having drivers make these kinds of decisions is very bad for uniform
> wireless behaviour.
I agree.
On the other hand different hardware have different ways to configure,
and support, PSM so the implementation might get complicated. But
still I believe that the driver shouldn't be making this kind of
decisions.
--
Kalle Valo
> This stuff is very much hardware dependent, I don't deny that, but
> still I would assume that the basic principles are almost the same. We
> just need to come up with a good interface for the drivers. Sure, it's
> more work now, but if we don't do it, PSM support will eventually get
> really messy because all drivers do it somehow differently.
I agree.
> > I really don't know what other vendors do and user should not guess
> > this.
>
> Yes, for the user this is too difficult, but I think the driver
> shouldn't be making the decision either. So mac80211 stack would be
> the most logical choice, and it could be make the decisions based on
> user input (or something like that).
Well, the user may want to make a choice based on the latency. Or,
ideally, we would detect that they're using voice and want lower
latency. Or something like that.
johannes
On Mon, 2008-07-14 at 15:38 +0300, Tomas Winkler wrote:
> On Mon, Jul 14, 2008 at 3:36 PM, Johannes Berg
> <[email protected]> wrote:
> >
> >> --- a/include/net/mac80211.h
> >> +++ b/include/net/mac80211.h
> >> @@ -191,6 +191,7 @@ struct ieee80211_bss_conf {
> >> /* erp related data */
> >> bool use_cts_prot;
> >> bool use_short_preamble;
> >> + u8 dtim_period;
> >> u16 beacon_int;
> >> u16 assoc_capability;
> >> u64 timestamp;
> >
> > Can you add documentation please, especially about the fact that it can
> > be zero if the network is buggy / IBSS.
>
> Right, I will also add fallback to 1 if it's 0
Good point, I guess that's the best guess then.
johannes
This patch makes listen_interval configurable by low level driver.
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Emmanuel Grumbach <[email protected]>
---
include/net/mac80211.h | 2 ++
net/mac80211/main.c | 3 +++
net/mac80211/mlme.c | 6 ++++--
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index f249820..677ba29 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -423,6 +423,7 @@ enum ieee80211_conf_flags {
* @radio_enabled: when zero, driver is required to switch off the radio.
* TODO make a flag
* @beacon_int: beacon interval (TODO make interface config)
+ * @listen_interval: listen interval in units of beacon interval
* @flags: configuration flags defined above
* @power_level: requested transmit power (in dBm)
* @max_antenna_gain: maximum antenna gain (in dBi)
@@ -437,6 +438,7 @@ struct ieee80211_conf {
int radio_enabled;
int beacon_int;
+ u16 listen_interval;
u32 flags;
int power_level;
int max_antenna_gain;
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index cf477ad..b4b0c2d 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1750,6 +1750,9 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
if (local->hw.conf.beacon_int < 10)
local->hw.conf.beacon_int = 100;
+ if (local->hw.conf.listen_interval == 0)
+ local->hw.conf.listen_interval = 1;
+
local->wstats_flags |= local->hw.flags & (IEEE80211_HW_SIGNAL_UNSPEC |
IEEE80211_HW_SIGNAL_DB |
IEEE80211_HW_SIGNAL_DBM) ?
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 22eaf86..73566e6 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -778,7 +778,8 @@ static void ieee80211_send_assoc(struct net_device *dev,
mgmt->frame_control = IEEE80211_FC(IEEE80211_FTYPE_MGMT,
IEEE80211_STYPE_REASSOC_REQ);
mgmt->u.reassoc_req.capab_info = cpu_to_le16(capab);
- mgmt->u.reassoc_req.listen_interval = cpu_to_le16(1);
+ mgmt->u.reassoc_req.listen_interval =
+ cpu_to_le16(local->hw.conf.listen_interval);
memcpy(mgmt->u.reassoc_req.current_ap, ifsta->prev_bssid,
ETH_ALEN);
} else {
@@ -786,7 +787,8 @@ static void ieee80211_send_assoc(struct net_device *dev,
mgmt->frame_control = IEEE80211_FC(IEEE80211_FTYPE_MGMT,
IEEE80211_STYPE_ASSOC_REQ);
mgmt->u.assoc_req.capab_info = cpu_to_le16(capab);
- mgmt->u.assoc_req.listen_interval = cpu_to_le16(1);
+ mgmt->u.reassoc_req.listen_interval =
+ cpu_to_le16(local->hw.conf.listen_interval);
}
/* SSID */
--
1.5.4.1
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
On Tue, Jul 15, 2008 at 11:41 AM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2008-07-15 at 11:29 +0300, Tomas Winkler wrote:
>
>> We are providing power save user interface reach enough to specify all
>> the above requirements.
>> I think you are both misinterpreting listen interval meaning.
>> Listen interval merely says to AP for how many beacons save direct
>> packets for this STA. It doesn't mean
>> it's can be shorter and it does say it's won't be longer.
>
> Right, but it does make sense to advertise what we're using, and this
> patch just adds a strict "driver tells mac80211 what it's using" flow.
> That's mostly what I'm objecting to. If you were calling the variable
> "max_listen_interval" and having mac80211 send it back to the driver in
> the BSS config as bssconf->listen_interval, and, for now, simply use the
> max, I wouldn't have a problem with it.
Okay will work for me, although I think it's just overhead in this
stage and I doubt it will ever be utilized.
Will submit V2
>> It's
>> doesn't affect power save dynamics it's just sets upper limit.
>> This value for iwlwifi hw is derived from maximal supported beacon
>> interval and size of the internal HW timers.
>> This value is hard coded in the driver.
>
> Shouldn't it depend on the beacon interval then?
Yes, but why to complicate things if not needed.
Tomas
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -191,6 +191,7 @@ struct ieee80211_bss_conf {
> /* erp related data */
> bool use_cts_prot;
> bool use_short_preamble;
> + u8 dtim_period;
> u16 beacon_int;
> u16 assoc_capability;
> u64 timestamp;
Can you add documentation please, especially about the fact that it can
be zero if the network is buggy / IBSS.
johannes
Johannes Berg <[email protected]> writes:
>> Yes, for the user this is too difficult, but I think the driver
>> shouldn't be making the decision either. So mac80211 stack would be
>> the most logical choice, and it could be make the decisions based on
>> user input (or something like that).
>
> Well, the user may want to make a choice based on the latency.
Yeah, latency might be a good choise for the user. Also it would be
nice to be able to say if packet loss is acceptable. For example, I
have heard cases where DTIMs are skipped just to save power in the
expense of loosing some of the broadcast and multicast frames.
> Or, ideally, we would detect that they're using voice and want lower
> latency. Or something like that.
In that case U-APSD needs to be considered as well.
This PSM stuff will be challenging...
--
Kalle Valo
On Mon, Jul 14, 2008 at 3:36 PM, Johannes Berg
<[email protected]> wrote:
>
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -191,6 +191,7 @@ struct ieee80211_bss_conf {
>> /* erp related data */
>> bool use_cts_prot;
>> bool use_short_preamble;
>> + u8 dtim_period;
>> u16 beacon_int;
>> u16 assoc_capability;
>> u64 timestamp;
>
> Can you add documentation please, especially about the fact that it can
> be zero if the network is buggy / IBSS.
Right, I will also add fallback to 1 if it's 0
Thanks
Tomas
Tomas Winkler <[email protected]> writes:
>>
>> This stuff is very much hardware dependent, I don't deny that, but
>> still I would assume that the basic principles are almost the same. We
>> just need to come up with a good interface for the drivers. Sure, it's
>> more work now, but if we don't do it, PSM support will eventually get
>> really messy because all drivers do it somehow differently.
>>
> This patch only sets value of listen interval from the driver I just
> think this is exactly what you are suggesting.
No, I don't suggest that. I understood that for setting the
listen_interval you are after something like this:
sysfs? register_hw()
user ----------> driver --------------> mac80211
(Please correct me if I misunderstood this.)
But I think that the flow should be like this:
WE/cfg80211 config()
user ----------> mac80211 --------------> driver
So that mac80211 would be one making the choice for the listen value
based on user input. But what the user input would be, that's an
another question.
> This is only required for sending correct value of listen interval
> to AP in association. Anything else is handled in the driver or
> firmware anyway.
Yes, driver will pass the information to the firmware. But I still
think mac80211 should make the decision, and the driver should be only
forwarding mac80211 decisions to the firmware.
> This patch should go in and we can reconsider it when other HW
> driver will implement PS so we can see the bigger picture.
I guess it's a good compromise for now, but as long as we change it
later.
--
Kalle Valo
On Tue, Jul 15, 2008 at 11:12 AM, Johannes Berg
<[email protected]> wrote:
>
>> This stuff is very much hardware dependent, I don't deny that, but
>> still I would assume that the basic principles are almost the same. We
>> just need to come up with a good interface for the drivers. Sure, it's
>> more work now, but if we don't do it, PSM support will eventually get
>> really messy because all drivers do it somehow differently.
>
> I agree.
>
>> > I really don't know what other vendors do and user should not guess
>> > this.
>>
>> Yes, for the user this is too difficult, but I think the driver
>> shouldn't be making the decision either. So mac80211 stack would be
>> the most logical choice, and it could be make the decisions based on
>> user input (or something like that).
>
> Well, the user may want to make a choice based on the latency. Or,
> ideally, we would detect that they're using voice and want lower
> latency. Or something like that.
We are providing power save user interface reach enough to specify all
the above requirements.
I think you are both misinterpreting listen interval meaning.
Listen interval merely says to AP for how many beacons save direct
packets for this STA. It doesn't mean
it's can be shorter and it does say it's won't be longer. It's
doesn't affect power save dynamics it's just sets upper limit.
This value for iwlwifi hw is derived from maximal supported beacon
interval and size of the internal HW timers.
This value is hard coded in the driver.
Tomas
Am Di 15 Jul 2008 10:41:01 CEST schrieb Johannes Berg
<[email protected]>:
> Right, but it does make sense to advertise what we're using, and this
> patch just adds a strict "driver tells mac80211 what it's using" flow.
> That's mostly what I'm objecting to. If you were calling the variable
> "max_listen_interval" and having mac80211 send it back to the driver in
> the BSS config as bssconf->listen_interval, and, for now, simply use the
> max, I wouldn't have a problem with it.
Using "max_listen_interval" as default may possibly result in APs
rejecting the association. At least the 802.11 spec defines a status
code 51 with the description "Association denied because the
ListenInterval is too large".
Maybe retrying the association with a lower listen interval could help
here. Therefore I agree with Johannes that the decision about what
value to actually use should be made in mac80211.
Helmut