Add P2P NOA settings for STA mode.
Signed-off-by: Janusz Dziedzic <[email protected]>
---
include/net/mac80211.h | 19 +++++++++++++++++++
net/mac80211/mlme.c | 40 +++++++++++++++++++++++++++++++---------
2 files changed, 50 insertions(+), 9 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cdd7cea..4e13f2a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -264,6 +264,24 @@ enum ieee80211_rssi_event {
RSSI_EVENT_LOW,
};
+/* struct p2p_noa_desc - holds Notice of Absence parameters
+ *
+ * This structure keeps parameters used in case of Notice
+ * of Absence - P2P PS.
+ *
+ * @count: 1-255 - number of absence intervals.
+ * 0 - special meaning - descriptor not used
+ * @duration: maximum duration in units of microseconds
+ * @interval: length of NOA in microseconds
+ * @start_time: lower 4 bytes of TSF timer
+ */
+struct p2p_noa_desc {
+ u8 count;
+ u32 duration;
+ u32 interval;
+ u32 start_time;
+};
+
/**
* struct ieee80211_bss_conf - holds the BSS's changing parameters
*
@@ -365,6 +383,7 @@ struct ieee80211_bss_conf {
int txpower;
u8 p2p_ctwindow;
bool p2p_oppps;
+ struct p2p_noa_desc p2p_noa_desc[IEEE80211_P2P_NOA_DESC_MAX];
};
/**
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 4fecfb8..7e282ea 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1629,6 +1629,30 @@ static u32 ieee80211_handle_bss_capability(struct ieee80211_sub_if_data *sdata,
return changed;
}
+static u8 ieee80211_setup_noa_attr(struct ieee80211_bss_conf *bss_conf,
+ struct ieee80211_p2p_noa_attr *noa)
+{
+ u8 noa_index;
+ int i;
+
+ noa_index = noa->index;
+ bss_conf->p2p_oppps = noa->oppps_ctwindow & 0x80;
+ bss_conf->p2p_ctwindow = noa->oppps_ctwindow & 0x7f;
+
+ for (i = 0; i < IEEE80211_P2P_NOA_DESC_MAX; i++) {
+ bss_conf->p2p_noa_desc[i].count =
+ noa->desc[i].count;
+ bss_conf->p2p_noa_desc[i].duration =
+ le32_to_cpu(noa->desc[i].duration);
+ bss_conf->p2p_noa_desc[i].interval =
+ le32_to_cpu(noa->desc[i].interval);
+ bss_conf->p2p_noa_desc[i].start_time =
+ le32_to_cpu(noa->desc[i].start_time);
+ }
+
+ return noa_index;
+}
+
static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
struct cfg80211_bss *cbss,
u32 bss_info_changed)
@@ -1655,7 +1679,7 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
rcu_read_lock();
ies = rcu_dereference(cbss->ies);
if (ies) {
- struct ieee80211_p2p_noa_attr noa;
+ struct ieee80211_p2p_noa_attr noa = {0};
int ret;
ret = cfg80211_get_p2p_attr(
@@ -1663,10 +1687,9 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
(u8 *) &noa, sizeof(noa));
if (ret >= 2) {
- bss_conf->p2p_oppps = noa.oppps_ctwindow & 0x80;
- bss_conf->p2p_ctwindow = noa.oppps_ctwindow & 0x7f;
+ sdata->u.mgd.p2p_noa_index =
+ ieee80211_setup_noa_attr(bss_conf, &noa);
bss_info_changed |= BSS_CHANGED_P2P_PS;
- sdata->u.mgd.p2p_noa_index = noa.index;
}
}
rcu_read_unlock();
@@ -2953,18 +2976,17 @@ ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
}
if (sdata->vif.p2p) {
- struct ieee80211_p2p_noa_attr noa;
+ struct ieee80211_p2p_noa_attr noa = {0};
int ret;
ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
len - baselen,
IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
(u8 *) &noa, sizeof(noa));
- if (ret >= 2 && sdata->u.mgd.p2p_noa_index != noa.index) {
- bss_conf->p2p_oppps = noa.oppps_ctwindow & 0x80;
- bss_conf->p2p_ctwindow = noa.oppps_ctwindow & 0x7f;
+ if (sdata->u.mgd.p2p_noa_index != noa.index) {
+ sdata->u.mgd.p2p_noa_index =
+ ieee80211_setup_noa_attr(bss_conf, &noa);
changed |= BSS_CHANGED_P2P_PS;
- sdata->u.mgd.p2p_noa_index = noa.index;
/*
* make sure we update all information, the CRC
* mechanism doesn't look at P2P attributes.
--
1.7.9.5
> - u8 p2p_ctwindow;
> - bool p2p_oppps;
> + struct ieee80211_p2p_noa_attr p2p_noa_attr;
Shouldn't you also remove sdata.u.mgd.p2p_noa_index? Actually, maybe
not, since that needs to be -1 in some cases, so n/m.
> - if (params->p2p_opp_ps >= 0) {
> - sdata->vif.bss_conf.p2p_oppps = params->p2p_opp_ps;
> + if (params->p2p_opp_ps > 0) {
> + sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |= BIT(8);
> + changed |= BSS_CHANGED_P2P_PS;
> + } else if (params->p2p_opp_ps == 0) {
> + sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~BIT(8);
> changed |= BSS_CHANGED_P2P_PS;
BIT(8) can't be right -- you mean 7?
Maybe also time to introduce a constant for this so drivers can use it
as well?
> if (sdata->vif.p2p) {
> - struct ieee80211_p2p_noa_attr noa;
> int ret;
> -
> + struct ieee80211_p2p_noa_attr noa = {};
> + /* We have to handle here such cases:
> + - p2p_attr disapear
got catch, I guess I never noticed that -- also typo ("disappears" or
"disappeared")
wrong comment style though
> + /* We can memcmp() here in case will find broken GO */
> + if (sdata->u.mgd.p2p_noa_index != noa.index) {
> sdata->u.mgd.p2p_noa_index = noa.index;
> + memcpy(&bss_conf->p2p_noa_attr, &noa, sizeof(noa));
> + changed |= BSS_CHANGED_P2P_PS;
> /*
> * make sure we update all information, the CRC
> * mechanism doesn't look at P2P attributes.
> */
> ifmgd->beacon_crc_valid = false;
> }
> +
> + /* Or different handling:
> +
> + // Version 2:
Are you asking me to pick version? I guess the first one above doesn't
handle the disappearance case very well?
> + int ret;
> + struct ieee80211_p2p_noa_attr noa = {};
> + ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
> + len - baselen,
> + IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
> + (u8 *) &noa, 2);
why 2 now?
> + if (ret <= 0) {
> + if (sdata->u.mgd.p2p_noa_index) {
> + // NOA attr disapear
> + sdata->u.mgd.p2p_noa_index = 0;
> + memset(&bss_conf->p2p_noa_attr, 0,
> + sizeof(bss_conf->p2p_noa_attr));
> + changed |= BSS_CHANGED_P2P_PS;
> + ifmgd->beacon_crc_valid = false;
> + }
> + } else if (sdata->u.mgd.p2p_noa_index != noa.index) {
> + // NOA changed, noa desc(s) could disapear
> + memset(&bss_conf->p2p_noa_attr, 0,
> + sizeof(bss_conf->p2p_noa_attr);
> + ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
> + len - baselen,
> + IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
> + (u8 *) &bss_conf->p2p_noa_attr,
> + sizeof(bss_conf->p2p_noa_attr));
I don't see why you'd want to retrieve it twice? It will only copy as
much as is present and return the data length.
> + // Version 3:
> + int ret;
> + memset(&bss_conf->p2p_noa_attr, 0, sizeof(bss_conf->p2p_noa_attr));
> + ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
> + len - baselen,
> + IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
> + (u8 *) &bss_conf->p2p_noa_attr,
> + sizeof(bss_conf->p2p_noa_attr));
> + if (sdata->u.mgd.p2p_noa_index != noa.index) {
> + sdata->u.mgd.p2p_noa_index = noa.index;
> + changed |= BSS_CHANGED_P2P_PS;
> + ifmgd->beacon_crc_valid = false;
> + }
Here you didn't check "ret", so the "disappeared entirely" case isn't
handled?
Seems like it should be like this:
ret = get_p2p_attr(...)
if (sdata->u.mgd.p2p_noa_index != noa.index || !ret) {
if (ret)
p2p_noa_index = noa.index
else
p2p_noa_index = -1;
changed |= ...
crc_valid = false;
}
to handle all the cases, including dis- and reappearance?
johannes
2013/3/18 Johannes Berg <[email protected]>:
>
>> - u8 p2p_ctwindow;
>> - bool p2p_oppps;
>> + struct ieee80211_p2p_noa_attr p2p_noa_attr;
>
> Shouldn't you also remove sdata.u.mgd.p2p_noa_index? Actually, maybe
> not, since that needs to be -1 in some cases, so n/m.
>
>> - if (params->p2p_opp_ps >= 0) {
>> - sdata->vif.bss_conf.p2p_oppps = params->p2p_opp_ps;
>> + if (params->p2p_opp_ps > 0) {
>> + sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |= BIT(8);
>> + changed |= BSS_CHANGED_P2P_PS;
>> + } else if (params->p2p_opp_ps == 0) {
>> + sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~BIT(8);
>> changed |= BSS_CHANGED_P2P_PS;
>
> BIT(8) can't be right -- you mean 7?
>
> Maybe also time to introduce a constant for this so drivers can use it
> as well?
>
>> if (sdata->vif.p2p) {
>> - struct ieee80211_p2p_noa_attr noa;
>> int ret;
>> -
>> + struct ieee80211_p2p_noa_attr noa = {};
>> + /* We have to handle here such cases:
>> + - p2p_attr disapear
>
> got catch, I guess I never noticed that -- also typo ("disappears" or
> "disappeared")
>
> wrong comment style though
>
>> + /* We can memcmp() here in case will find broken GO */
>> + if (sdata->u.mgd.p2p_noa_index != noa.index) {
>> sdata->u.mgd.p2p_noa_index = noa.index;
>> + memcpy(&bss_conf->p2p_noa_attr, &noa, sizeof(noa));
>> + changed |= BSS_CHANGED_P2P_PS;
>> /*
>> * make sure we update all information, the CRC
>> * mechanism doesn't look at P2P attributes.
>> */
>> ifmgd->beacon_crc_valid = false;
>> }
>> +
>> + /* Or different handling:
>> +
>> + // Version 2:
>
> Are you asking me to pick version? I guess the first one above doesn't
> handle the disappearance case very well?
>
>> + int ret;
>> + struct ieee80211_p2p_noa_attr noa = {};
>> + ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
>> + len - baselen,
>> + IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
>> + (u8 *) &noa, 2);
>
> why 2 now?
>
>> + if (ret <= 0) {
>> + if (sdata->u.mgd.p2p_noa_index) {
>> + // NOA attr disapear
>> + sdata->u.mgd.p2p_noa_index = 0;
>> + memset(&bss_conf->p2p_noa_attr, 0,
>> + sizeof(bss_conf->p2p_noa_attr));
>> + changed |= BSS_CHANGED_P2P_PS;
>> + ifmgd->beacon_crc_valid = false;
>> + }
>> + } else if (sdata->u.mgd.p2p_noa_index != noa.index) {
>> + // NOA changed, noa desc(s) could disapear
>> + memset(&bss_conf->p2p_noa_attr, 0,
>> + sizeof(bss_conf->p2p_noa_attr);
>> + ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
>> + len - baselen,
>> + IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
>> + (u8 *) &bss_conf->p2p_noa_attr,
>> + sizeof(bss_conf->p2p_noa_attr));
>
> I don't see why you'd want to retrieve it twice? It will only copy as
> much as is present and return the data length.
>
>> + // Version 3:
>> + int ret;
>> + memset(&bss_conf->p2p_noa_attr, 0, sizeof(bss_conf->p2p_noa_attr));
>> + ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
>> + len - baselen,
>> + IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
>> + (u8 *) &bss_conf->p2p_noa_attr,
>> + sizeof(bss_conf->p2p_noa_attr));
>> + if (sdata->u.mgd.p2p_noa_index != noa.index) {
>> + sdata->u.mgd.p2p_noa_index = noa.index;
>> + changed |= BSS_CHANGED_P2P_PS;
>> + ifmgd->beacon_crc_valid = false;
>> + }
>
> Here you didn't check "ret", so the "disappeared entirely" case isn't
> handled?
>
> Seems like it should be like this:
>
> ret = get_p2p_attr(...)
> if (sdata->u.mgd.p2p_noa_index != noa.index || !ret) {
> if (ret)
> p2p_noa_index = noa.index
> else
> p2p_noa_index = -1;
> changed |= ...
> crc_valid = false;
> }
>
> to handle all the cases, including dis- and reappearance?
>
Added changes. Seems sdata->u.mgd.p2p_noa_index is required while
noa.index valid range is 0-255 and decide base on index.
I think code cover now all cases.
---
include/net/mac80211.h | 3 +--
net/mac80211/cfg.c | 19 ++++++++++++++-----
net/mac80211/ieee80211_i.h | 2 +-
net/mac80211/mlme.c | 42 ++++++++++++++++++++++++++----------------
net/mac80211/trace.h | 6 ++----
5 files changed, 44 insertions(+), 28 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cdd7cea..582e743 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -363,8 +363,7 @@ struct ieee80211_bss_conf {
size_t ssid_len;
bool hidden_ssid;
int txpower;
- u8 p2p_ctwindow;
- bool p2p_oppps;
+ struct ieee80211_p2p_noa_attr p2p_noa_attr;
};
/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 1d1ddab..6dc592b 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -960,8 +960,12 @@ static int ieee80211_start_ap(struct wiphy
*wiphy, struct net_device *dev,
sdata->vif.bss_conf.hidden_ssid =
(params->hidden_ssid != NL80211_HIDDEN_SSID_NOT_IN_USE);
- sdata->vif.bss_conf.p2p_ctwindow = params->p2p_ctwindow;
- sdata->vif.bss_conf.p2p_oppps = params->p2p_opp_ps;
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow =
+ params->p2p_ctwindow & 0x7F;
+ if (params->p2p_opp_ps)
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |= BIT(7);
+ else
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~BIT(7);
err = ieee80211_assign_beacon(sdata, ¶ms->beacon);
if (err < 0)
@@ -1956,12 +1960,17 @@ static int ieee80211_change_bss(struct wiphy *wiphy,
}
if (params->p2p_ctwindow >= 0) {
- sdata->vif.bss_conf.p2p_ctwindow = params->p2p_ctwindow;
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~0x7f;
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |=
+ params->p2p_ctwindow & 0x7f;
changed |= BSS_CHANGED_P2P_PS;
}
- if (params->p2p_opp_ps >= 0) {
- sdata->vif.bss_conf.p2p_oppps = params->p2p_opp_ps;
+ if (params->p2p_opp_ps > 0) {
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |= BIT(7);
+ changed |= BSS_CHANGED_P2P_PS;
+ } else if (params->p2p_opp_ps == 0) {
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~BIT(7);
changed |= BSS_CHANGED_P2P_PS;
}
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index f4433f0..11451ee 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -442,7 +442,7 @@ struct ieee80211_if_managed {
u8 use_4addr;
- u8 p2p_noa_index;
+ s16 p2p_noa_index;
/* Signal strength from the last Beacon frame in the current BSS. */
int last_beacon_signal;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 4fecfb8..252f4a0 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1655,18 +1655,17 @@ static void ieee80211_set_associated(struct
ieee80211_sub_if_data *sdata,
rcu_read_lock();
ies = rcu_dereference(cbss->ies);
if (ies) {
- struct ieee80211_p2p_noa_attr noa;
int ret;
ret = cfg80211_get_p2p_attr(
ies->data, ies->len,
IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
- (u8 *) &noa, sizeof(noa));
+ (u8 *) &bss_conf->p2p_noa_attr,
+ sizeof(bss_conf->p2p_noa_attr));
if (ret >= 2) {
- bss_conf->p2p_oppps = noa.oppps_ctwindow & 0x80;
- bss_conf->p2p_ctwindow = noa.oppps_ctwindow & 0x7f;
+ sdata->u.mgd.p2p_noa_index =
+ bss_conf->p2p_noa_attr.index;
bss_info_changed |= BSS_CHANGED_P2P_PS;
- sdata->u.mgd.p2p_noa_index = noa.index;
}
}
rcu_read_unlock();
@@ -1791,8 +1790,9 @@ static void ieee80211_set_disassoc(struct
ieee80211_sub_if_data *sdata,
changed |= BSS_CHANGED_ASSOC;
sdata->vif.bss_conf.assoc = false;
- sdata->vif.bss_conf.p2p_ctwindow = 0;
- sdata->vif.bss_conf.p2p_oppps = false;
+ ifmgd->p2p_noa_index = -1;
+ memset(&sdata->vif.bss_conf.p2p_noa_attr, 0,
+ sizeof(sdata->vif.bss_conf.p2p_noa_attr));
/* on the next assoc, re-program HT/VHT parameters */
memset(&ifmgd->ht_capa, 0, sizeof(ifmgd->ht_capa));
@@ -2953,22 +2953,31 @@ ieee80211_rx_mgmt_beacon(struct
ieee80211_sub_if_data *sdata,
}
if (sdata->vif.p2p) {
- struct ieee80211_p2p_noa_attr noa;
+ struct ieee80211_p2p_noa_attr noa = {};
int ret;
ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
len - baselen,
IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
(u8 *) &noa, sizeof(noa));
- if (ret >= 2 && sdata->u.mgd.p2p_noa_index != noa.index) {
- bss_conf->p2p_oppps = noa.oppps_ctwindow & 0x80;
- bss_conf->p2p_ctwindow = noa.oppps_ctwindow & 0x7f;
+
+ if (ret >= 2) {
+ if (sdata->u.mgd.p2p_noa_index != noa.index) {
+ /* valid noa_attr and index changed */
+ sdata->u.mgd.p2p_noa_index = noa.index;
+ memcpy(&bss_conf->p2p_noa_attr, &noa, sizeof(noa));
+ changed |= BSS_CHANGED_P2P_PS;
+ /*
+ * make sure we update all information, the CRC
+ * mechanism doesn't look at P2P attributes.
+ */
+ ifmgd->beacon_crc_valid = false;
+ }
+ } else if (sdata->u.mgd.p2p_noa_index != -1) {
+ /* noa_attr not found and we had valid noa_attr before */
+ sdata->u.mgd.p2p_noa_index = -1;
+ memset(&bss_conf->p2p_noa_attr, 0, sizeof(bss_conf->p2p_noa_attr));
changed |= BSS_CHANGED_P2P_PS;
- sdata->u.mgd.p2p_noa_index = noa.index;
- /*
- * make sure we update all information, the CRC
- * mechanism doesn't look at P2P attributes.
- */
ifmgd->beacon_crc_valid = false;
}
}
@@ -3511,6 +3520,7 @@ void ieee80211_sta_setup_sdata(struct
ieee80211_sub_if_data *sdata)
ifmgd->powersave = sdata->wdev.ps;
ifmgd->uapsd_queues = IEEE80211_DEFAULT_UAPSD_QUEUES;
ifmgd->uapsd_max_sp_len = IEEE80211_DEFAULT_MAX_SP_LEN;
+ ifmgd->p2p_noa_index = -1;
mutex_init(&ifmgd->mtx);
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index e7db2b8..b9322b5 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -359,8 +359,7 @@ TRACE_EVENT(drv_bss_info_changed,
__dynamic_array(u8, ssid, info->ssid_len);
__field(bool, hidden_ssid);
__field(int, txpower)
- __field(u8, p2p_ctwindow)
- __field(bool, p2p_oppps)
+ __field(u8, p2p_oppps_ctwindow)
),
TP_fast_assign(
@@ -400,8 +399,7 @@ TRACE_EVENT(drv_bss_info_changed,
memcpy(__get_dynamic_array(ssid), info->ssid, info->ssid_len);
__entry->hidden_ssid = info->hidden_ssid;
__entry->txpower = info->txpower;
- __entry->p2p_ctwindow = info->p2p_ctwindow;
- __entry->p2p_oppps = info->p2p_oppps;
+ __entry->p2p_oppps_ctwindow = info->p2p_noa_attr.oppps_ctwindow;
),
TP_printk(
--
1.7.9.5
On Thu, 2013-03-14 at 09:33 +0200, [email protected] wrote:
> Add P2P NOA settings for STA mode.
First two patches are fine, here I have some concerns.
> +/* struct p2p_noa_desc - holds Notice of Absence parameters
> + *
This isn't valid kernel-doc.
> +struct p2p_noa_desc {
However, in any case, I'd rather you remove this struct.
> + struct p2p_noa_desc p2p_noa_desc[IEEE80211_P2P_NOA_DESC_MAX];
and use ieee80211_p2p_noa_desc here
> +static u8 ieee80211_setup_noa_attr(struct ieee80211_bss_conf *bss_conf,
> + struct ieee80211_p2p_noa_attr *noa)
indentation is wrong, but you'll get rid of the function anyway :-)
> + bss_conf->p2p_noa_desc[i].duration =
> + le32_to_cpu(noa->desc[i].duration);
I don't see any need to do endian conversion here in mac80211 -- the
driver can do it. Most drivers won't need it anyway, so this is just
extra code/time spent.
> - struct ieee80211_p2p_noa_attr noa;
> + struct ieee80211_p2p_noa_attr noa = {0};
Please just use = {}. Although ... see below
> @@ -2953,18 +2976,17 @@ ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
> }
>
> if (sdata->vif.p2p) {
> - struct ieee80211_p2p_noa_attr noa;
> + struct ieee80211_p2p_noa_attr noa = {0};
> int ret;
>
> ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
> len - baselen,
> IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
> (u8 *) &noa, sizeof(noa));
> - if (ret >= 2 && sdata->u.mgd.p2p_noa_index != noa.index) {
> - bss_conf->p2p_oppps = noa.oppps_ctwindow & 0x80;
> - bss_conf->p2p_ctwindow = noa.oppps_ctwindow & 0x7f;
> + if (sdata->u.mgd.p2p_noa_index != noa.index) {
> + sdata->u.mgd.p2p_noa_index =
> + ieee80211_setup_noa_attr(bss_conf, &noa);
> changed |= BSS_CHANGED_P2P_PS;
> - sdata->u.mgd.p2p_noa_index = noa.index;
Now that we pretty much have *all* fields of the NoA attribute in
bss_conf (except the index), why not just put the full struct there, and
have cfg80211 read into that buffer directly? That way, we save all the
copying. Yes, it would mean changing all the drivers (just one I guess)
using it, but maybe that'd be better?
Or do we expect broken GOs that change the contents w/o updating the
index, and then things break or something?
johanes
On Tue, 2013-03-19 at 16:38 +0100, Janusz Dziedzic wrote:
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -960,8 +960,12 @@ static int ieee80211_start_ap(struct wiphy
> *wiphy, struct net_device *dev,
> sdata->vif.bss_conf.hidden_ssid =
> (params->hidden_ssid != NL80211_HIDDEN_SSID_NOT_IN_USE);
>
> - sdata->vif.bss_conf.p2p_ctwindow = params->p2p_ctwindow;
> - sdata->vif.bss_conf.p2p_oppps = params->p2p_opp_ps;
> + sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow =
> + params->p2p_ctwindow & 0x7F;
> + if (params->p2p_opp_ps)
> + sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |= BIT(7);
> + else
> + sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~BIT(7);
That else branch isn't really needed.
> @@ -1956,12 +1960,17 @@ static int ieee80211_change_bss(struct wiphy *wiphy,
> }
>
> if (params->p2p_ctwindow >= 0) {
> - sdata->vif.bss_conf.p2p_ctwindow = params->p2p_ctwindow;
> + sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~0x7f;
> + sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |=
> + params->p2p_ctwindow & 0x7f;
You have this BIT(7) and 0x7f a lot now, I think you should add
constants next to the struct definition and use them everywhere.
Otherwise looks fine, thanks for fixing my bugs too :-)
One more thing -- you need to update the iwlwifi/mvm driver that uses
this already. I'd prefer to also use the constants I asked for above
there.
johannes
2013/3/15 Johannes Berg <[email protected]>:
> On Thu, 2013-03-14 at 09:33 +0200, [email protected] wrote:
>> Add P2P NOA settings for STA mode.
>
> First two patches are fine, here I have some concerns.
>
>> +/* struct p2p_noa_desc - holds Notice of Absence parameters
>> + *
>
> This isn't valid kernel-doc.
>
>> +struct p2p_noa_desc {
>
> However, in any case, I'd rather you remove this struct.
>
>> + struct p2p_noa_desc p2p_noa_desc[IEEE80211_P2P_NOA_DESC_MAX];
>
> and use ieee80211_p2p_noa_desc here
>
>> +static u8 ieee80211_setup_noa_attr(struct ieee80211_bss_conf *bss_conf,
>> + struct ieee80211_p2p_noa_attr *noa)
>
> indentation is wrong, but you'll get rid of the function anyway :-)
>
>> + bss_conf->p2p_noa_desc[i].duration =
>> + le32_to_cpu(noa->desc[i].duration);
>
> I don't see any need to do endian conversion here in mac80211 -- the
> driver can do it. Most drivers won't need it anyway, so this is just
> extra code/time spent.
>
>> - struct ieee80211_p2p_noa_attr noa;
>> + struct ieee80211_p2p_noa_attr noa = {0};
>
> Please just use = {}. Although ... see below
>
>> @@ -2953,18 +2976,17 @@ ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
>> }
>>
>> if (sdata->vif.p2p) {
>> - struct ieee80211_p2p_noa_attr noa;
>> + struct ieee80211_p2p_noa_attr noa = {0};
>> int ret;
>>
>> ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
>> len - baselen,
>> IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
>> (u8 *) &noa, sizeof(noa));
>> - if (ret >= 2 && sdata->u.mgd.p2p_noa_index != noa.index) {
>> - bss_conf->p2p_oppps = noa.oppps_ctwindow & 0x80;
>> - bss_conf->p2p_ctwindow = noa.oppps_ctwindow & 0x7f;
>> + if (sdata->u.mgd.p2p_noa_index != noa.index) {
>> + sdata->u.mgd.p2p_noa_index =
>> + ieee80211_setup_noa_attr(bss_conf, &noa);
>> changed |= BSS_CHANGED_P2P_PS;
>> - sdata->u.mgd.p2p_noa_index = noa.index;
>
> Now that we pretty much have *all* fields of the NoA attribute in
> bss_conf (except the index), why not just put the full struct there, and
> have cfg80211 read into that buffer directly? That way, we save all the
> copying. Yes, it would mean changing all the drivers (just one I guess)
> using it, but maybe that'd be better?
>
> Or do we expect broken GOs that change the contents w/o updating the
> index, and then things break or something?
>
Hello Johannes,
Added changes you suggest.
Added some comments/questions in the code.
Please review.
---
include/net/mac80211.h | 3 +-
net/mac80211/cfg.c | 19 ++++++++----
net/mac80211/mlme.c | 76 +++++++++++++++++++++++++++++++++++++++---------
3 files changed, 78 insertions(+), 20 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cdd7cea..582e743 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -363,8 +363,7 @@ struct ieee80211_bss_conf {
size_t ssid_len;
bool hidden_ssid;
int txpower;
- u8 p2p_ctwindow;
- bool p2p_oppps;
+ struct ieee80211_p2p_noa_attr p2p_noa_attr;
};
/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 1d1ddab..2a8be822 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -960,8 +960,12 @@ static int ieee80211_start_ap(struct wiphy
*wiphy, struct net_device *dev,
sdata->vif.bss_conf.hidden_ssid =
(params->hidden_ssid != NL80211_HIDDEN_SSID_NOT_IN_USE);
- sdata->vif.bss_conf.p2p_ctwindow = params->p2p_ctwindow;
- sdata->vif.bss_conf.p2p_oppps = params->p2p_opp_ps;
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow =
+ params->p2p_ctwindow & 0x7F;
+ if (params->p2p_opp_ps)
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |= BIT(8);
+ else
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~BIT(8);
err = ieee80211_assign_beacon(sdata, ¶ms->beacon);
if (err < 0)
@@ -1956,12 +1960,17 @@ static int ieee80211_change_bss(struct wiphy *wiphy,
}
if (params->p2p_ctwindow >= 0) {
- sdata->vif.bss_conf.p2p_ctwindow = params->p2p_ctwindow;
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~0x7f;
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |=
+ params->p2p_ctwindow & 0x7f;
changed |= BSS_CHANGED_P2P_PS;
}
- if (params->p2p_opp_ps >= 0) {
- sdata->vif.bss_conf.p2p_oppps = params->p2p_opp_ps;
+ if (params->p2p_opp_ps > 0) {
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |= BIT(8);
+ changed |= BSS_CHANGED_P2P_PS;
+ } else if (params->p2p_opp_ps == 0) {
+ sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow &= ~BIT(8);
changed |= BSS_CHANGED_P2P_PS;
}
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 4fecfb8..2681429 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1655,18 +1655,17 @@ static void ieee80211_set_associated(struct
ieee80211_sub_if_data *sdata,
rcu_read_lock();
ies = rcu_dereference(cbss->ies);
if (ies) {
- struct ieee80211_p2p_noa_attr noa;
int ret;
ret = cfg80211_get_p2p_attr(
ies->data, ies->len,
IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
- (u8 *) &noa, sizeof(noa));
+ (u8 *) &bss_conf->p2p_noa_attr,
+ sizeof(bss_conf->p2p_noa_attr));
if (ret >= 2) {
- bss_conf->p2p_oppps = noa.oppps_ctwindow & 0x80;
- bss_conf->p2p_ctwindow = noa.oppps_ctwindow & 0x7f;
+ sdata->u.mgd.p2p_noa_index =
+ bss_conf->p2p_noa_attr.index;
bss_info_changed |= BSS_CHANGED_P2P_PS;
- sdata->u.mgd.p2p_noa_index = noa.index;
}
}
rcu_read_unlock();
@@ -1791,8 +1790,8 @@ static void ieee80211_set_disassoc(struct
ieee80211_sub_if_data *sdata,
changed |= BSS_CHANGED_ASSOC;
sdata->vif.bss_conf.assoc = false;
- sdata->vif.bss_conf.p2p_ctwindow = 0;
- sdata->vif.bss_conf.p2p_oppps = false;
+ memset(&sdata->vif.bss_conf.p2p_noa_attr, 0,
+ sizeof(sdata->vif.bss_conf.p2p_noa_attr));
/* on the next assoc, re-program HT/VHT parameters */
memset(&ifmgd->ht_capa, 0, sizeof(ifmgd->ht_capa));
@@ -2953,24 +2952,75 @@ ieee80211_rx_mgmt_beacon(struct
ieee80211_sub_if_data *sdata,
}
if (sdata->vif.p2p) {
- struct ieee80211_p2p_noa_attr noa;
int ret;
-
+ struct ieee80211_p2p_noa_attr noa = {};
+ /* We have to handle here such cases:
+ - p2p_attr disapear
+ - index changed, noa desc disapear but left oppps
+ */
ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
len - baselen,
IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
(u8 *) &noa, sizeof(noa));
- if (ret >= 2 && sdata->u.mgd.p2p_noa_index != noa.index) {
- bss_conf->p2p_oppps = noa.oppps_ctwindow & 0x80;
- bss_conf->p2p_ctwindow = noa.oppps_ctwindow & 0x7f;
- changed |= BSS_CHANGED_P2P_PS;
+
+ /* We can memcmp() here in case will find broken GO */
+ if (sdata->u.mgd.p2p_noa_index != noa.index) {
sdata->u.mgd.p2p_noa_index = noa.index;
+ memcpy(&bss_conf->p2p_noa_attr, &noa, sizeof(noa));
+ changed |= BSS_CHANGED_P2P_PS;
/*
* make sure we update all information, the CRC
* mechanism doesn't look at P2P attributes.
*/
ifmgd->beacon_crc_valid = false;
}
+
+ /* Or different handling:
+
+ // Version 2:
+ int ret;
+ struct ieee80211_p2p_noa_attr noa = {};
+ ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
+ len - baselen,
+ IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
+ (u8 *) &noa, 2);
+ if (ret <= 0) {
+ if (sdata->u.mgd.p2p_noa_index) {
+ // NOA attr disapear
+ sdata->u.mgd.p2p_noa_index = 0;
+ memset(&bss_conf->p2p_noa_attr, 0,
+ sizeof(bss_conf->p2p_noa_attr));
+ changed |= BSS_CHANGED_P2P_PS;
+ ifmgd->beacon_crc_valid = false;
+ }
+ } else if (sdata->u.mgd.p2p_noa_index != noa.index) {
+ // NOA changed, noa desc(s) could disapear
+ memset(&bss_conf->p2p_noa_attr, 0,
+ sizeof(bss_conf->p2p_noa_attr);
+ ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
+ len - baselen,
+ IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
+ (u8 *) &bss_conf->p2p_noa_attr,
+ sizeof(bss_conf->p2p_noa_attr));
+ sdata->u.mgd.p2p_noa_index = noa.index;
+ changed |= BSS_CHANGED_P2P_PS;
+ ifmgd->beacon_crc_valid = false;
+ }
+
+ // Version 3:
+ int ret;
+ memset(&bss_conf->p2p_noa_attr, 0, sizeof(bss_conf->p2p_noa_attr));
+ ret = cfg80211_get_p2p_attr(mgmt->u.beacon.variable,
+ len - baselen,
+ IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
+ (u8 *) &bss_conf->p2p_noa_attr,
+ sizeof(bss_conf->p2p_noa_attr));
+ if (sdata->u.mgd.p2p_noa_index != noa.index) {
+ sdata->u.mgd.p2p_noa_index = noa.index;
+ changed |= BSS_CHANGED_P2P_PS;
+ ifmgd->beacon_crc_valid = false;
+ }
+ */
}
if (ncrc == ifmgd->beacon_crc && ifmgd->beacon_crc_valid)
--
1.7.9.5
BR
Janusz