[PATCH 0/4] dynamically update puncturing bitmap

[PATCH 0/4] dynamically update puncturing bitmap

[Kang Yang]

After connection, puncturing bitmap maybe change. AP will include the
related changes in beacon.

So update the related changes dynamically.

Kang Yang (4):
  wifi: mac80211: mlme: fix verification of puncturing bitmap obtained
    from AP
  wifi: mac80211: mlme: correct the verification of extracted bitmap
  wifi: mac80211: mlme: enable tracking bandwidth changes for 6 GHz band
  wifi: ath12k: dynamically update puncturing bitmap

 drivers/net/wireless/ath/ath12k/mac.c | 50 +++++++++++++++++++
 drivers/net/wireless/ath/ath12k/wmi.h | 17 +++++++
 net/mac80211/mlme.c                   | 72 ++++++++++++++++++++-------
 3 files changed, 121 insertions(+), 18 deletions(-)


base-commit: 73e13f6a439b75a9dbc84bbfa0b0d6624b354853
-- 
2.34.1


-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

[PATCH 1/4] wifi: mac80211: mlme: fix verification of puncturing bitmap obtained from AP

[Kang Yang]

Puncturing bitmap and bandwidth is included in beacon's EHT Operation
element. After receiving beacon, mac80211 will verify if they are match.
But the bandwidth used for verification is incorrect. Because bandwidth
in link->conf->chandef is a negotiated bandwidth, it may be downgraded.
Should use bandwidth included in beacon. Otherwise when bandwidth
downgrade occurs, even if the received values match, an error may be
returned.

Also, checking if bitmap and bandwidth match should be done before
extraction. But here extract first and then check.

So fix these two issues.

Fixes: aa87cd8b3573 ("wifi: mac80211: mlme: handle EHT channel puncturing")
Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
---
 net/mac80211/mlme.c | 54 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index f93eb38ae0b8..16e15ced28a5 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -5670,11 +5670,33 @@ static bool ieee80211_rx_our_beacon(const u8 *tx_bssid,
 	return ether_addr_equal(tx_bssid, bss->transmitted_bss->bssid);
 }
 
+static enum nl80211_chan_width
+ieee80211_rx_bw_to_nlwidth(enum ieee80211_sta_rx_bandwidth bw)
+{
+	switch (bw) {
+	case IEEE80211_STA_RX_BW_20:
+		return NL80211_CHAN_WIDTH_20;
+	case IEEE80211_STA_RX_BW_40:
+		return NL80211_CHAN_WIDTH_40;
+	case IEEE80211_STA_RX_BW_80:
+		return NL80211_CHAN_WIDTH_80;
+	case IEEE80211_STA_RX_BW_160:
+		return NL80211_CHAN_WIDTH_160;
+	case IEEE80211_STA_RX_BW_320:
+		return NL80211_CHAN_WIDTH_320;
+	default:
+		WARN_ON(1);
+		return NL80211_CHAN_WIDTH_20;
+	}
+}
+
 static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
 					const struct ieee80211_eht_operation *eht_oper,
 					u64 *changed)
 {
+	struct cfg80211_chan_def rx_chandef = link->conf->chandef;
 	u16 bitmap = 0, extracted;
+	u8 bw = 0;
 
 	if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) &&
 	    (eht_oper->params &
@@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
 		const u8 *disable_subchannel_bitmap = info->optional;
 
 		bitmap = get_unaligned_le16(disable_subchannel_bitmap);
+		bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH);
+		rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw);
+
+		if (rx_chandef.width == NL80211_CHAN_WIDTH_80)
+			rx_chandef.center_freq1 =
+				ieee80211_channel_to_frequency(info->ccfs0,
+							       rx_chandef.chan->band);
+		else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 ||
+			 rx_chandef.width == NL80211_CHAN_WIDTH_320)
+			rx_chandef.center_freq1 =
+				ieee80211_channel_to_frequency(info->ccfs1,
+							       rx_chandef.chan->band);
+	}
+
+	if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
+						      &rx_chandef)) {
+		link_info(link,
+			  "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n",
+			  link->u.mgd.bssid,
+			  bitmap,
+			  rx_chandef.width);
+		return false;
 	}
 
 	extracted = ieee80211_extract_dis_subch_bmap(eht_oper,
@@ -5695,16 +5739,6 @@ static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
 	    extracted == link->conf->eht_puncturing)
 		return true;
 
-	if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
-						      &link->conf->chandef)) {
-		link_info(link,
-			  "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n",
-			  link->u.mgd.bssid,
-			  bitmap,
-			  link->conf->chandef.width);
-		return false;
-	}
-
 	ieee80211_handle_puncturing_bitmap(link, eht_oper, bitmap, changed);
 	return true;
 }
-- 
2.34.1


-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

[PATCH 2/4] wifi: mac80211: mlme: correct the verification of extracted bitmap

[Kang Yang]

Mac80211 will extract puncturing bitmap according to the negotiated
bandwidth. After extraction, should check the new puncturing bitmap
according to the negotiated bandwidth, but here input the original bitmap.

When bandwidth downgrade occurs, this function may return an error even
though negotiated bandwidth and puncturing bitmap match correctly.

So correct it.

Fixes: aa87cd8b3573 ("wifi: mac80211: mlme: handle EHT channel puncturing")
Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
---
 net/mac80211/mlme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 16e15ced28a5..13fba1f1cd89 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -145,7 +145,7 @@ ieee80211_handle_puncturing_bitmap(struct ieee80211_link_data *link,
 			ieee80211_extract_dis_subch_bmap(eht_oper, chandef,
 							 bitmap);
 
-		if (cfg80211_valid_disable_subchannel_bitmap(&bitmap,
+		if (cfg80211_valid_disable_subchannel_bitmap(&extracted,
 							     chandef))
 			break;
 		link->u.mgd.conn_flags |=
-- 
2.34.1


-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

[PATCH 3/4] wifi: mac80211: mlme: enable tracking bandwidth changes for 6 GHz band

[Kang Yang]

After connection, mac80211 will track bandwidth changes upon
beacon's CRC is changed. But it will stop tracking bandwidth
changes when these is no HT Operation element.

According to the section 9.4.2.311 in IEEE P802.11be draft 4.0:
The operation of EHT STAs in an EHT BSS is controlled by the following:
        -'The HT Operation element, HE Operation element, and EHT
          Operation element if operating in the 2.4 GHz band'
        -'The HT Operation element, VHT Operation element (if present),
          HE Operation element, and EHT Operation element if operating
          in the 5 GHz band'
        -'The HE Operation element and EHT Operation element if
          operating in the 6 GHz band'

For 6 GHz band, beacon doesn't contain HT Operation element. Therefore,
current checking code needs to be changed.

Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
---
 net/mac80211/mlme.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 13fba1f1cd89..f55d677f9ace 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -501,9 +501,9 @@ static int ieee80211_config_bw(struct ieee80211_link_data *link,
 	u32 vht_cap_info = 0;
 	int ret;
 
-	/* if HT was/is disabled, don't track any bandwidth changes */
-	if (link->u.mgd.conn_flags & IEEE80211_CONN_DISABLE_HT || !ht_oper)
-		return 0;
+	/* don't check HT if we associated as non-HT station */
+	if (link->u.mgd.conn_flags & IEEE80211_CONN_DISABLE_HT)
+		ht_oper = NULL;
 
 	/* don't check VHT if we associated as non-VHT station */
 	if (link->u.mgd.conn_flags & IEEE80211_CONN_DISABLE_VHT)
@@ -525,10 +525,12 @@ static int ieee80211_config_bw(struct ieee80211_link_data *link,
 	 * if bss configuration changed store the new one -
 	 * this may be applicable even if channel is identical
 	 */
-	ht_opmode = le16_to_cpu(ht_oper->operation_mode);
-	if (link->conf->ht_operation_mode != ht_opmode) {
-		*changed |= BSS_CHANGED_HT;
-		link->conf->ht_operation_mode = ht_opmode;
+	if (ht_oper) {
+		ht_opmode = le16_to_cpu(ht_oper->operation_mode);
+		if (link->conf->ht_operation_mode != ht_opmode) {
+			*changed |= BSS_CHANGED_HT;
+			link->conf->ht_operation_mode = ht_opmode;
+		}
 	}
 
 	if (vht_cap)
-- 
2.34.1


-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

[PATCH 4/4] wifi: ath12k: dynamically update puncturing bitmap

[Kang Yang]

Every time EHT Operation element changed, mac80211 will parse it and
extract the valid puncturing bitmap according to the negotiated bandwidth.

So add updates of bandwidth and puncturing bitmap in ath12k. Then
process and send them to the firmware by WMI event.

Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4

Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 50 +++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath12k/wmi.h | 17 +++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 85602d64b607..24968ef02978 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -2486,6 +2486,32 @@ static int ath12k_mac_fils_discovery(struct ath12k_vif *arvif,
 	return ret;
 }
 
+static enum wmi_channel_width
+ath12k_mac_nlwidth_to_wmiwidth(enum nl80211_chan_width width)
+{
+	switch (width) {
+	case NL80211_CHAN_WIDTH_20:
+		return WMI_CHAN_WIDTH_20;
+	case NL80211_CHAN_WIDTH_40:
+		return WMI_CHAN_WIDTH_40;
+	case NL80211_CHAN_WIDTH_80:
+		return WMI_CHAN_WIDTH_80;
+	case NL80211_CHAN_WIDTH_160:
+		return WMI_CHAN_WIDTH_160;
+	case NL80211_CHAN_WIDTH_80P80:
+		return WMI_CHAN_WIDTH_80P80;
+	case NL80211_CHAN_WIDTH_5:
+		return WMI_CHAN_WIDTH_5;
+	case NL80211_CHAN_WIDTH_10:
+		return WMI_CHAN_WIDTH_10;
+	case NL80211_CHAN_WIDTH_320:
+		return WMI_CHAN_WIDTH_320;
+	default:
+		WARN_ON(1);
+		return WMI_CHAN_WIDTH_20;
+	}
+}
+
 static void ath12k_mac_op_bss_info_changed(struct ieee80211_hw *hw,
 					   struct ieee80211_vif *vif,
 					   struct ieee80211_bss_conf *info,
@@ -2507,6 +2533,27 @@ static void ath12k_mac_op_bss_info_changed(struct ieee80211_hw *hw,
 
 	mutex_lock(&ar->conf_mutex);
 
+	if (changed & BSS_CHANGED_EHT_PUNCTURING) {
+		param_id = WMI_PEER_CHWIDTH_PUNCTURE_20MHZ_BITMAP;
+		param_value = ath12k_mac_nlwidth_to_wmiwidth(info->chandef.width) |
+			      u32_encode_bits((~info->eht_puncturing),
+					      WMI_PEER_PUNCTURE_BITMAP);
+
+		ret = ath12k_wmi_set_peer_param(ar, arvif->bssid,
+						arvif->vdev_id, param_id,
+						param_value);
+		if (ret)
+			ath12k_warn(ar->ab,
+				    "Failed to set puncturing bitmap %04x "
+				    "and bandwidth %d for VDEV: %d\n",
+				    info->eht_puncturing, info->chandef.width,
+				    arvif->vdev_id);
+		else
+			ath12k_dbg(ar->ab, ATH12K_DBG_MAC,
+				   "Set puncturing bitmap %04x and and bandwidth %d for VDEV: %d\n",
+				   info->eht_puncturing, info->chandef.width, arvif->vdev_id);
+	}
+
 	if (changed & BSS_CHANGED_BEACON_INT) {
 		arvif->beacon_interval = info->beacon_int;
 
@@ -3725,6 +3772,9 @@ static u32 ath12k_mac_ieee80211_sta_bw_to_wmi(struct ath12k *ar,
 	case IEEE80211_STA_RX_BW_160:
 		bw = WMI_PEER_CHWIDTH_160MHZ;
 		break;
+	case IEEE80211_STA_RX_BW_320:
+		bw = WMI_PEER_CHWIDTH_320MHZ;
+		break;
 	default:
 		ath12k_warn(ar->ab, "Invalid bandwidth %d in rc update for %pM\n",
 			    sta->deflink.bandwidth, sta->addr);
diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
index c75a6fa1f7e0..55ef3443fcc5 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.h
+++ b/drivers/net/wireless/ath/ath12k/wmi.h
@@ -2194,8 +2194,11 @@ enum wmi_peer_param {
 	WMI_PEER_SET_MAX_TX_RATE = 17,
 	WMI_PEER_SET_MIN_TX_RATE = 18,
 	WMI_PEER_SET_DEFAULT_ROUTING = 19,
+	WMI_PEER_CHWIDTH_PUNCTURE_20MHZ_BITMAP = 39,
 };
 
+#define WMI_PEER_PUNCTURE_BITMAP		GENMASK(23, 8)
+
 enum wmi_slot_time {
 	WMI_VDEV_SLOT_TIME_LONG = 1,
 	WMI_VDEV_SLOT_TIME_SHORT = 2,
@@ -2217,6 +2220,7 @@ enum wmi_peer_chwidth {
 	WMI_PEER_CHWIDTH_40MHZ = 1,
 	WMI_PEER_CHWIDTH_80MHZ = 2,
 	WMI_PEER_CHWIDTH_160MHZ = 3,
+	WMI_PEER_CHWIDTH_320MHZ = 4,
 };
 
 enum wmi_beacon_gen_mode {
@@ -2265,6 +2269,19 @@ struct ath12k_wmi_hal_reg_capabilities_ext_arg {
 	u32 high_5ghz_chan;
 };
 
+enum wmi_channel_width {
+	WMI_CHAN_WIDTH_20 = 0,
+	WMI_CHAN_WIDTH_40 = 1,
+	WMI_CHAN_WIDTH_80 = 2,
+	WMI_CHAN_WIDTH_160 = 3,
+	WMI_CHAN_WIDTH_80P80 = 4,
+	WMI_CHAN_WIDTH_5 = 5,
+	WMI_CHAN_WIDTH_10 = 6,
+	WMI_CHAN_WIDTH_165 = 7,
+	WMI_CHAN_WIDTH_160P160 = 8,
+	WMI_CHAN_WIDTH_320 = 9,
+};
+
 #define WMI_HOST_MAX_PDEV 3
 
 struct ath12k_wmi_host_mem_chunk_params {
-- 
2.34.1


-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 3/4] wifi: mac80211: mlme: enable tracking bandwidth changes for 6 GHz band

[Jeff Johnson]

On 9/27/2023 10:50 PM, Kang Yang wrote:
> After connection, mac80211 will track bandwidth changes upon
> beacon's CRC is changed. But it will stop tracking bandwidth
> changes when these is no HT Operation element.

nit: s/these/there/


-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 1/4] wifi: mac80211: mlme: fix verification of puncturing bitmap obtained from AP

[Johannes Berg]

On Thu, 2023-09-28 at 13:50 +0800, Kang Yang wrote:
> 
> +static enum nl80211_chan_width
> +ieee80211_rx_bw_to_nlwidth(enum ieee80211_sta_rx_bandwidth bw)
> +{
> +	switch (bw) {
> +	case IEEE80211_STA_RX_BW_20:
> +		return NL80211_CHAN_WIDTH_20;

So for a while now I was actually not responding to this because I was
scratching my head over how this function ever could be needed or make
sense ...


>  static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
>  					const struct ieee80211_eht_operation *eht_oper,
>  					u64 *changed)
>  {
> +	struct cfg80211_chan_def rx_chandef = link->conf->chandef;
>  	u16 bitmap = 0, extracted;
> +	u8 bw = 0;
>  
>  	if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) &&
>  	    (eht_oper->params &
> @@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
>  		const u8 *disable_subchannel_bitmap = info->optional;
>  
>  		bitmap = get_unaligned_le16(disable_subchannel_bitmap);
> +		bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH);
> +		rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw);

But looking here, it clearly _doesn't_ make sense. IEEE80211_STA_RX_BW_*
is a purely internal API, has nothing to do with the spec.

All this might even be "accidentally correct", but it really isn't right
at all - the values in IEEE80211_EHT_OPER_CHAN_WIDTH are
IEEE80211_EHT_OPER_CHAN_WIDTH_*, not IEEE80211_STA_RX_BW_*.



More generally though, I don't even understand the change.

> +		if (rx_chandef.width == NL80211_CHAN_WIDTH_80)
> +			rx_chandef.center_freq1 =
> +				ieee80211_channel_to_frequency(info->ccfs0,
> +							       rx_chandef.chan->band);
> +		else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 ||
> +			 rx_chandef.width == NL80211_CHAN_WIDTH_320)
> +			rx_chandef.center_freq1 =
> +				ieee80211_channel_to_frequency(info->ccfs1,
> +							       rx_chandef.chan->band);
> +	}
> +
> +	if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
> +						      &rx_chandef)) {
> +		link_info(link,
> +			  "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n",
> +			  link->u.mgd.bssid,
> +			  bitmap,
> +			  rx_chandef.width);
> +		return false;
>  	}
>  
>  	extracted = ieee80211_extract_dis_subch_bmap(eht_oper,
// I've filled in the context here in the patch
>                                                      &link->conf->chandef,
>                                                      bitmap);
> 
>         /* accept if there are no changes */
>         if (!(*changed & BSS_CHANGED_BANDWIDTH) &&
>             extracted == link->conf->eht_puncturing)
>                 return true;

but ... ieee80211_extract_dis_subch_bmap actually already takes the
bandwidth from eht_oper into account!
 
> -	if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
> -						      &link->conf->chandef)) {

So are you saying that the real bug is that we're missing to update the
link->conf->chandef with the EHT operation from the assoc response?

But you didn't fix that issue ... so not sure?

johannes


-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 1/4] wifi: mac80211: mlme: fix verification of puncturing bitmap obtained from AP

[Kang Yang]

On 10/18/2023 7:39 PM, Johannes Berg wrote:
>>   static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
>>   					const struct ieee80211_eht_operation *eht_oper,
>>   					u64 *changed)
>>   {
>> +	struct cfg80211_chan_def rx_chandef = link->conf->chandef;
>>   	u16 bitmap = 0, extracted;
>> +	u8 bw = 0;
>>   
>>   	if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) &&
>>   	    (eht_oper->params &
>> @@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
>>   		const u8 *disable_subchannel_bitmap = info->optional;
>>   
>>   		bitmap = get_unaligned_le16(disable_subchannel_bitmap);
>> +		bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH);
>> +		rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw);
> 
> But looking here, it clearly _doesn't_ make sense. IEEE80211_STA_RX_BW_*
> is a purely internal API, has nothing to do with the spec.
> 
> All this might even be "accidentally correct", but it really isn't right
> at all - the values in IEEE80211_EHT_OPER_CHAN_WIDTH are
> IEEE80211_EHT_OPER_CHAN_WIDTH_*, not IEEE80211_STA_RX_BW_*.
> 



Oh, sorry that i didn't notice IEEE80211_EHT_OPER_CHAN_WIDTH_*, will 
replace them.


> 
> 
> More generally though, I don't even understand the change.


During assoc, downgrade may happen in func ieee80211_config_bw(). In 
this situation, the bandwidth in beacon and the bandwidth after 
downgrade(chandef->width, maybe i should call it local bandwidth during 
below context, will use this name in next version) during assoc will be 
different.

The change is based on this point.


> 
>> +		if (rx_chandef.width == NL80211_CHAN_WIDTH_80)
>> +			rx_chandef.center_freq1 =
>> +				ieee80211_channel_to_frequency(info->ccfs0,
>> +							       rx_chandef.chan->band);
>> +		else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 ||
>> +			 rx_chandef.width == NL80211_CHAN_WIDTH_320)
>> +			rx_chandef.center_freq1 =
>> +				ieee80211_channel_to_frequency(info->ccfs1,
>> +							       rx_chandef.chan->band);
>> +	}
>> +
>> +	if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
>> +						      &rx_chandef)) {


Here i change you code 
cfg80211_valid_disable_subchannel_bitmap(&bitmap,&link->conf->chandef) to
cfg80211_valid_disable_subchannel_bitmap(&bitmap,&rx_chandef)

As described above, downgrade may happen before enter 
ieee80211_config_puncturing(), so the bandwidth in link->conf->chandef 
may be different with bandwidth in beacon.

Here, the bitmap you used is from eht_oper in beacon, but the bandwidth 
you used is local bandwidth. They are not match. This is the first issue.


>> +		link_info(link,
>> +			  "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n",
>> +			  link->u.mgd.bssid,
>> +			  bitmap,
>> +			  rx_chandef.width);
>> +		return false;
>>   	}
>>   
>>   	extracted = ieee80211_extract_dis_subch_bmap(eht_oper,
> // I've filled in the context here in the patch


Here is move the cfg80211_valid_disable_subchannel_bitmap() before the 
ieee80211_extract_dis_subch_bmap(), cause i think check should done 
before extract.

This is the second issue i said(perhaps not a issue).



>>                                                       &link->conf->chandef,
>>                                                       bitmap);
>>
>>          /* accept if there are no changes */
>>          if (!(*changed & BSS_CHANGED_BANDWIDTH) &&
>>              extracted == link->conf->eht_puncturing)
>>                  return true;
> 
> but ... ieee80211_extract_dis_subch_bmap actually already takes the
> bandwidth from eht_oper into account!
>   

Yes, the ieee80211_extract_dis_subch_bmap realy takes the bandwidth from 
eht_oper into account, and the local_bw in this func is the local 
bandwidth i discuss.

You already notice the difference between bandwidth from eht_oper and 
local bandwidth in ieee80211_extract_dis_subch_bmap(). I think you 
should also take it into account when you use 
cfg80211_valid_disable_subchannel_bitmap(), right?

BTW, do you want to verify the bitmap from eht_oper, or the extracted 
bitmap?

Anyway, whether you want to verify the bitmap from eht_oper or extracted 
bitmap in cfg80211_valid_disable_subchannel_bitmap(), the bitmap and 
bandwidth must correspond.



>> -	if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
>> -						      &link->conf->chandef)) {
> 
> So are you saying that the real bug is that we're missing to update the
> link->conf->chandef with the EHT operation from the assoc response?
> 
> But you didn't fix that issue ... so not sure?


I have described my patch with the comments above, maybe i should make 
my commit log more coherent.


Besides, this is you first version, i made some comments on Nov. 21, 
2022, 7:29 a.m. Maybe you already forget them.
https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/


> 
> johannes
> 

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 1/4] wifi: mac80211: mlme: fix verification of puncturing bitmap obtained from AP

[Kang Yang]

Due to Johannes's refactor of Puncturing bitmap, this patchset can be 
abandoned now.


Will prepare a new patch about puncturing bitmap for ath12k.


On 10/19/2023 11:25 AM, Kang Yang wrote:
> 
> 
> On 10/18/2023 7:39 PM, Johannes Berg wrote:
>>>   static bool ieee80211_config_puncturing(struct ieee80211_link_data 
>>> *link,
>>>                       const struct ieee80211_eht_operation *eht_oper,
>>>                       u64 *changed)
>>>   {
>>> +    struct cfg80211_chan_def rx_chandef = link->conf->chandef;
>>>       u16 bitmap = 0, extracted;
>>> +    u8 bw = 0;
>>>       if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) &&
>>>           (eht_oper->params &
>>> @@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct 
>>> ieee80211_link_data *link,
>>>           const u8 *disable_subchannel_bitmap = info->optional;
>>>           bitmap = get_unaligned_le16(disable_subchannel_bitmap);
>>> +        bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH);
>>> +        rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw);
>>
>> But looking here, it clearly _doesn't_ make sense. IEEE80211_STA_RX_BW_*
>> is a purely internal API, has nothing to do with the spec.
>>
>> All this might even be "accidentally correct", but it really isn't right
>> at all - the values in IEEE80211_EHT_OPER_CHAN_WIDTH are
>> IEEE80211_EHT_OPER_CHAN_WIDTH_*, not IEEE80211_STA_RX_BW_*.
>>
> 
> 
> 
> Oh, sorry that i didn't notice IEEE80211_EHT_OPER_CHAN_WIDTH_*, will 
> replace them.
> 
> 
>>
>>
>> More generally though, I don't even understand the change.
> 
> 
> During assoc, downgrade may happen in func ieee80211_config_bw(). In 
> this situation, the bandwidth in beacon and the bandwidth after 
> downgrade(chandef->width, maybe i should call it local bandwidth during 
> below context, will use this name in next version) during assoc will be 
> different.
> 
> The change is based on this point.
> 
> 
>>
>>> +        if (rx_chandef.width == NL80211_CHAN_WIDTH_80)
>>> +            rx_chandef.center_freq1 =
>>> +                ieee80211_channel_to_frequency(info->ccfs0,
>>> +                                   rx_chandef.chan->band);
>>> +        else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 ||
>>> +             rx_chandef.width == NL80211_CHAN_WIDTH_320)
>>> +            rx_chandef.center_freq1 =
>>> +                ieee80211_channel_to_frequency(info->ccfs1,
>>> +                                   rx_chandef.chan->band);
>>> +    }
>>> +
>>> +    if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
>>> +                              &rx_chandef)) {
> 
> 
> Here i change you code 
> cfg80211_valid_disable_subchannel_bitmap(&bitmap,&link->conf->chandef) to
> cfg80211_valid_disable_subchannel_bitmap(&bitmap,&rx_chandef)
> 
> As described above, downgrade may happen before enter 
> ieee80211_config_puncturing(), so the bandwidth in link->conf->chandef 
> may be different with bandwidth in beacon.
> 
> Here, the bitmap you used is from eht_oper in beacon, but the bandwidth 
> you used is local bandwidth. They are not match. This is the first issue.
> 
> 
>>> +        link_info(link,
>>> +              "Got an invalid disable subchannel bitmap from AP %pM: 
>>> bitmap = 0x%x, bw = 0x%x. disconnect\n",
>>> +              link->u.mgd.bssid,
>>> +              bitmap,
>>> +              rx_chandef.width);
>>> +        return false;
>>>       }
>>>       extracted = ieee80211_extract_dis_subch_bmap(eht_oper,
>> // I've filled in the context here in the patch
> 
> 
> Here is move the cfg80211_valid_disable_subchannel_bitmap() before the 
> ieee80211_extract_dis_subch_bmap(), cause i think check should done 
> before extract.
> 
> This is the second issue i said(perhaps not a issue).
> 
> 
> 
>>>                                                       
>>> &link->conf->chandef,
>>>                                                       bitmap);
>>>
>>>          /* accept if there are no changes */
>>>          if (!(*changed & BSS_CHANGED_BANDWIDTH) &&
>>>              extracted == link->conf->eht_puncturing)
>>>                  return true;
>>
>> but ... ieee80211_extract_dis_subch_bmap actually already takes the
>> bandwidth from eht_oper into account!
> 
> Yes, the ieee80211_extract_dis_subch_bmap realy takes the bandwidth from 
> eht_oper into account, and the local_bw in this func is the local 
> bandwidth i discuss.
> 
> You already notice the difference between bandwidth from eht_oper and 
> local bandwidth in ieee80211_extract_dis_subch_bmap(). I think you 
> should also take it into account when you use 
> cfg80211_valid_disable_subchannel_bitmap(), right?
> 
> BTW, do you want to verify the bitmap from eht_oper, or the extracted 
> bitmap?
> 
> Anyway, whether you want to verify the bitmap from eht_oper or extracted 
> bitmap in cfg80211_valid_disable_subchannel_bitmap(), the bitmap and 
> bandwidth must correspond.
> 
> 
> 
>>> -    if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
>>> -                              &link->conf->chandef)) {
>>
>> So are you saying that the real bug is that we're missing to update the
>> link->conf->chandef with the EHT operation from the assoc response?
>>
>> But you didn't fix that issue ... so not sure?
> 
> 
> I have described my patch with the comments above, maybe i should make 
> my commit log more coherent.
> 
> 
> Besides, this is you first version, i made some comments on Nov. 21, 
> 2022, 7:29 a.m. Maybe you already forget them.
> https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/
> 
> 
>>
>> johannes
>>
>