diff mbox

[RFC,3/3] mac80211: P2P add NOA settings

Message ID CAFED-jndqjMFjNLBadFV1W9+5QZJF649BhFKy5zSjw3pnSQh1w@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Janusz Dziedzic March 19, 2013, 3:38 p.m. UTC 
2013/3/18 Johannes Berg <johannes@sipsolutions.net>:
>
>> -     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(-)

Comments

Johannes Berg March 19, 2013, 8:33 p.m. UTC | #1 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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, &params->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(