Hi,
A bunch of patches from our internal tree with mac80211 and
cfg80211 changes. It's the usual developement:
features, cleanups and bugfixes.
Thanks,
Miri
Benjamin Berg (1):
wifi: mac80211: use deflink and fix typo in link ID check
Johannes Berg (7):
wifi: mac80211_hwsim: advertise 15 simultaneous links
wifi: mac80211: simplify ieee80211_config_bw() prototype
wifi: mac80211: remove extra element parsing
wifi: mac80211: simplify HE capability access
wifi: mac80211: disallow drivers with HT wider than HE
wifi: mac80211: fix potential sta-link leak
wifi: mac80211: don't set bss_conf in parsing
drivers/net/wireless/virtual/mac80211_hwsim.c | 8 +-
net/mac80211/main.c | 18 +++
net/mac80211/mlme.c | 107 +++++++-----------
net/mac80211/sta_info.c | 5 +-
net/mac80211/util.c | 21 +---
5 files changed, 72 insertions(+), 87 deletions(-)
--
2.34.1
From: Johannes Berg <[email protected]>
Advertise MLD capabilities and operations in AP mode that
say that up to 15 links are supported simultaneously.
Signed-off-by: Johannes Berg <[email protected]>
Reviewed-by: Gregory Greenman <[email protected]>
Reviewed-by: Ilan Peer <[email protected]>
Signed-off-by: Miri Korenblit <[email protected]>
---
drivers/net/wireless/virtual/mac80211_hwsim.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c
index 304dc96fc3bb..403d892c0468 100644
--- a/drivers/net/wireless/virtual/mac80211_hwsim.c
+++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
@@ -4993,9 +4993,11 @@ static const u8 iftypes_ext_capa_ap[] = {
[9] = WLAN_EXT_CAPA10_TWT_RESPONDER_SUPPORT,
};
-#define MAC80211_HWSIM_MLD_CAPA_OPS FIELD_PREP_CONST( \
- IEEE80211_MLD_CAP_OP_TID_TO_LINK_MAP_NEG_SUPP, \
- IEEE80211_MLD_CAP_OP_TID_TO_LINK_MAP_NEG_SUPP_SAME)
+#define MAC80211_HWSIM_MLD_CAPA_OPS \
+ FIELD_PREP_CONST(IEEE80211_MLD_CAP_OP_TID_TO_LINK_MAP_NEG_SUPP, \
+ IEEE80211_MLD_CAP_OP_TID_TO_LINK_MAP_NEG_SUPP_SAME) | \
+ FIELD_PREP_CONST(IEEE80211_MLD_CAP_OP_MAX_SIMUL_LINKS, \
+ IEEE80211_MLD_MAX_NUM_LINKS - 1)
static const struct wiphy_iftype_ext_capab mac80211_hwsim_iftypes_ext_capa[] = {
{
--
2.34.1
From: Johannes Berg <[email protected]>
The only user of this function passes a lot of pointers
directly from the parsed elements, so it's simpler to
just pass the entire elements parsing struct. This also
shows that the ht_cap is actually unused.
Signed-off-by: Johannes Berg <[email protected]>
Reviewed-by: Gregory Greenman <[email protected]>
Signed-off-by: Miri Korenblit <[email protected]>
---
net/mac80211/mlme.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 576ba6b25db9..6fa69ad3ad4f 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -489,15 +489,15 @@ ieee80211_determine_chantype(struct ieee80211_sub_if_data *sdata,
}
static int ieee80211_config_bw(struct ieee80211_link_data *link,
- const struct ieee80211_ht_cap *ht_cap,
- const struct ieee80211_vht_cap *vht_cap,
- const struct ieee80211_ht_operation *ht_oper,
- const struct ieee80211_vht_operation *vht_oper,
- const struct ieee80211_he_operation *he_oper,
- const struct ieee80211_eht_operation *eht_oper,
- const struct ieee80211_s1g_oper_ie *s1g_oper,
+ struct ieee802_11_elems *elems,
const u8 *bssid, u64 *changed)
{
+ const struct ieee80211_vht_cap *vht_cap = elems->vht_cap_elem;
+ const struct ieee80211_ht_operation *ht_oper = elems->ht_operation;
+ const struct ieee80211_vht_operation *vht_oper = elems->vht_operation;
+ const struct ieee80211_he_operation *he_oper = elems->he_operation;
+ const struct ieee80211_eht_operation *eht_oper = elems->eht_operation;
+ const struct ieee80211_s1g_oper_ie *s1g_oper = elems->s1g_oper;
struct ieee80211_sub_if_data *sdata = link->sdata;
struct ieee80211_local *local = sdata->local;
struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
@@ -6433,11 +6433,7 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_link_data *link,
changed |= ieee80211_recalc_twt_req(sdata, sband, link, link_sta, elems);
- if (ieee80211_config_bw(link, elems->ht_cap_elem,
- elems->vht_cap_elem, elems->ht_operation,
- elems->vht_operation, elems->he_operation,
- elems->eht_operation,
- elems->s1g_oper, bssid, &changed)) {
+ if (ieee80211_config_bw(link, elems, bssid, &changed)) {
sdata_info(sdata,
"failed to follow AP %pM bandwidth change, disconnect\n",
bssid);
--
2.34.1
From: Johannes Berg <[email protected]>
For verifying the required HE capabilities are supported
locally, we access the HE capability element of the AP.
Simplify that access, we've already parsed and validated
it when parsing elements.
Signed-off-by: Johannes Berg <[email protected]>
Reviewed-by: Gregory Greenman <[email protected]>
Signed-off-by: Miri Korenblit <[email protected]>
---
net/mac80211/mlme.c | 32 +++++---------------------------
1 file changed, 5 insertions(+), 27 deletions(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 45be270eaab7..5b1bc84760d5 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4547,41 +4547,17 @@ static u8 ieee80211_max_rx_chains(struct ieee80211_link_data *link,
static bool
ieee80211_verify_peer_he_mcs_support(struct ieee80211_sub_if_data *sdata,
- const struct cfg80211_bss_ies *ies,
+ const struct ieee80211_he_cap_elem *he_cap,
const struct ieee80211_he_operation *he_op)
{
- const struct element *he_cap_elem;
- const struct ieee80211_he_cap_elem *he_cap;
struct ieee80211_he_mcs_nss_supp *he_mcs_nss_supp;
u16 mcs_80_map_tx, mcs_80_map_rx;
u16 ap_min_req_set;
- int mcs_nss_size;
int nss;
- he_cap_elem = cfg80211_find_ext_elem(WLAN_EID_EXT_HE_CAPABILITY,
- ies->data, ies->len);
-
- if (!he_cap_elem)
+ if (!he_cap)
return false;
- /* invalid HE IE */
- if (he_cap_elem->datalen < 1 + sizeof(*he_cap)) {
- sdata_info(sdata,
- "Invalid HE elem, Disable HE\n");
- return false;
- }
-
- /* skip one byte ext_tag_id */
- he_cap = (void *)(he_cap_elem->data + 1);
- mcs_nss_size = ieee80211_he_mcs_nss_size(he_cap);
-
- /* invalid HE IE */
- if (he_cap_elem->datalen < 1 + sizeof(*he_cap) + mcs_nss_size) {
- sdata_info(sdata,
- "Invalid HE elem with nss size, Disable HE\n");
- return false;
- }
-
/* mcs_nss is right after he_cap info */
he_mcs_nss_supp = (void *)(he_cap + 1);
@@ -4946,7 +4922,9 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
}
}
- if (!ieee80211_verify_peer_he_mcs_support(sdata, ies, he_oper) ||
+ if (!ieee80211_verify_peer_he_mcs_support(sdata,
+ (void *)elems->he_cap,
+ he_oper) ||
!ieee80211_verify_sta_he_mcs_support(sdata, sband, he_oper))
*conn_flags |= IEEE80211_CONN_DISABLE_HE |
IEEE80211_CONN_DISABLE_EHT;
--
2.34.1
From: Johannes Berg <[email protected]>
To simplify the code in the next patch, disallow drivers
supporting 40 MHz in HT but not HE, since we'd otherwise
have to track local maximum bandwidth per mode there.
Signed-off-by: Johannes Berg <[email protected]>
Reviewed-by: Gregory Greenman <[email protected]>
Signed-off-by: Miri Korenblit <[email protected]>
---
net/mac80211/main.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index d48fa1147c14..e05bcc35bc1e 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1119,8 +1119,26 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
supp_vht = supp_vht || sband->vht_cap.vht_supported;
for_each_sband_iftype_data(sband, i, iftd) {
+ u8 he_40_mhz_cap;
+
supp_he = supp_he || iftd->he_cap.has_he;
supp_eht = supp_eht || iftd->eht_cap.has_eht;
+
+ if (sband->band == NL80211_BAND_2GHZ)
+ he_40_mhz_cap =
+ IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_IN_2G;
+ else
+ he_40_mhz_cap =
+ IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G;
+
+ /* currently no support for HE client where HT has 40 MHz but not HT */
+ if (iftd->he_cap.has_he &&
+ iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
+ BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
+ sband->ht_cap.ht_supported &&
+ sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
+ !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
+ return -EINVAL;
}
/* HT, VHT, HE require QoS, thus >= 4 queues */
--
2.34.1
From: Johannes Berg <[email protected]>
When parsing 6 GHz operation, don't set the bss_conf
values. We only commit to that later in association,
so move the code there. Also clear it later.
While at it, handle IEEE80211_6GHZ_CTRL_REG_VLP_AP.
Signed-off-by: Johannes Berg <[email protected]>
Signed-off-by: Miri Korenblit <[email protected]>
---
net/mac80211/mlme.c | 27 +++++++++++++++++++++++++++
net/mac80211/util.c | 21 +--------------------
2 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 5b1bc84760d5..99188bd84799 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -3081,6 +3081,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
memset(ifmgd->tx_tspec, 0, sizeof(ifmgd->tx_tspec));
wiphy_delayed_work_cancel(local->hw.wiphy, &ifmgd->tx_tspec_wk);
+ sdata->vif.bss_conf.power_type = IEEE80211_REG_UNSET_AP;
sdata->vif.bss_conf.pwr_reduction = 0;
sdata->vif.bss_conf.tx_pwr_env_num = 0;
memset(sdata->vif.bss_conf.tx_pwr_env, 0,
@@ -4236,12 +4237,38 @@ static bool ieee80211_assoc_config_link(struct ieee80211_link_data *link,
if (elems->he_operation && !(link->u.mgd.conn_flags & IEEE80211_CONN_DISABLE_HE) &&
elems->he_cap) {
+ const struct ieee80211_he_6ghz_oper *he_6ghz_oper;
+
ieee80211_he_cap_ie_to_sta_he_cap(sdata, sband,
elems->he_cap,
elems->he_cap_len,
elems->he_6ghz_capa,
link_sta);
+ he_6ghz_oper = ieee80211_he_6ghz_oper(elems->he_operation);
+
+ if (is_6ghz && he_6ghz_oper) {
+ switch (u8_get_bits(he_6ghz_oper->control,
+ IEEE80211_HE_6GHZ_OPER_CTRL_REG_INFO)) {
+ case IEEE80211_6GHZ_CTRL_REG_LPI_AP:
+ bss_conf->power_type = IEEE80211_REG_LPI_AP;
+ break;
+ case IEEE80211_6GHZ_CTRL_REG_SP_AP:
+ bss_conf->power_type = IEEE80211_REG_SP_AP;
+ break;
+ case IEEE80211_6GHZ_CTRL_REG_VLP_AP:
+ bss_conf->power_type = IEEE80211_REG_VLP_AP;
+ break;
+ default:
+ bss_conf->power_type = IEEE80211_REG_UNSET_AP;
+ break;
+ }
+ } else if (is_6ghz) {
+ link_info(link,
+ "HE 6 GHz operation missing (on %d MHz), expect issues\n",
+ bss_conf->chandef.chan->center_freq);
+ }
+
bss_conf->he_support = link_sta->pub->he_cap.has_he;
if (elems->rsnx && elems->rsnx_len &&
(elems->rsnx[0] & WLAN_RSNX_CAPA_PROTECTED_TWT) &&
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 643c54855be6..685b55a053f3 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -3845,7 +3845,6 @@ bool ieee80211_chandef_he_6ghz_oper(struct ieee80211_sub_if_data *sdata,
const struct ieee80211_sta_eht_cap *eht_cap;
struct cfg80211_chan_def he_chandef = *chandef;
const struct ieee80211_he_6ghz_oper *he_6ghz_oper;
- struct ieee80211_bss_conf *bss_conf = &sdata->vif.bss_conf;
bool support_80_80, support_160, support_320;
u8 he_phy_cap, eht_phy_cap;
u32 freq;
@@ -3881,13 +3880,8 @@ bool ieee80211_chandef_he_6ghz_oper(struct ieee80211_sub_if_data *sdata,
eht_oper = NULL;
he_6ghz_oper = ieee80211_he_6ghz_oper(he_oper);
-
- if (!he_6ghz_oper) {
- sdata_info(sdata,
- "HE 6GHz operation missing (on %d MHz), expect issues\n",
- chandef->chan->center_freq);
+ if (!he_6ghz_oper)
return false;
- }
/*
* The EHT operation IE does not contain the primary channel so the
@@ -3898,19 +3892,6 @@ bool ieee80211_chandef_he_6ghz_oper(struct ieee80211_sub_if_data *sdata,
NL80211_BAND_6GHZ);
he_chandef.chan = ieee80211_get_channel(sdata->local->hw.wiphy, freq);
- switch (u8_get_bits(he_6ghz_oper->control,
- IEEE80211_HE_6GHZ_OPER_CTRL_REG_INFO)) {
- case IEEE80211_6GHZ_CTRL_REG_LPI_AP:
- bss_conf->power_type = IEEE80211_REG_LPI_AP;
- break;
- case IEEE80211_6GHZ_CTRL_REG_SP_AP:
- bss_conf->power_type = IEEE80211_REG_SP_AP;
- break;
- default:
- bss_conf->power_type = IEEE80211_REG_UNSET_AP;
- break;
- }
-
if (!eht_oper ||
!(eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT)) {
switch (u8_get_bits(he_6ghz_oper->control,
--
2.34.1
From: Benjamin Berg <[email protected]>
This does not change anything effectively, but it is closer to what the
code is trying to achieve here. i.e. select the link data if it is an
MLD and fall back to using the deflink otherwise.
Fixes: 0f99f0878350 ("wifi: mac80211: Print local link address during authentication")
Signed-off-by: Benjamin Berg <[email protected]>
Reviewed-by: Ilan Peer <[email protected]>
Signed-off-by: Miri Korenblit <[email protected]>
---
net/mac80211/mlme.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 99188bd84799..cc9a8eaffa6b 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -7869,10 +7869,10 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
if (err)
goto err_clear;
- if (req->link_id > 0)
+ if (req->link_id >= 0)
link = sdata_dereference(sdata->link[req->link_id], sdata);
else
- link = sdata_dereference(sdata->link[0], sdata);
+ link = &sdata->deflink;
if (WARN_ON(!link)) {
err = -ENOLINK;
--
2.34.1
From: Johannes Berg <[email protected]>
We already parse all the BSS elements into elems, there's
really no need to separately find EHT/ML again. Remove the
extra code.
Signed-off-by: Johannes Berg <[email protected]>
Reviewed-by: Gregory Greenman <[email protected]>
Signed-off-by: Miri Korenblit <[email protected]>
---
net/mac80211/mlme.c | 24 ++----------------------
1 file changed, 2 insertions(+), 22 deletions(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 6fa69ad3ad4f..45be270eaab7 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4962,32 +4962,12 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
(IEEE80211_CONN_DISABLE_HE |
IEEE80211_CONN_DISABLE_EHT)) &&
he_oper) {
- const struct cfg80211_bss_ies *cbss_ies;
- const struct element *eht_ml_elem;
- const u8 *eht_oper_ie;
-
- cbss_ies = rcu_dereference(cbss->ies);
- eht_oper_ie = cfg80211_find_ext_ie(WLAN_EID_EXT_EHT_OPERATION,
- cbss_ies->data, cbss_ies->len);
- if (eht_oper_ie && eht_oper_ie[1] >=
- 1 + sizeof(struct ieee80211_eht_operation))
- eht_oper = (void *)(eht_oper_ie + 3);
- else
- eht_oper = NULL;
+ eht_oper = elems->eht_operation;
if (!ieee80211_verify_sta_eht_mcs_support(sdata, sband, eht_oper))
*conn_flags |= IEEE80211_CONN_DISABLE_EHT;
- eht_ml_elem = cfg80211_find_ext_elem(WLAN_EID_EXT_EHT_MULTI_LINK,
- cbss_ies->data, cbss_ies->len);
-
- /* data + 1 / datalen - 1 since it's an extended element */
- if (!(*conn_flags & IEEE80211_CONN_DISABLE_EHT) &&
- eht_ml_elem &&
- ieee80211_mle_type_ok(eht_ml_elem->data + 1,
- IEEE80211_ML_CONTROL_TYPE_BASIC,
- eht_ml_elem->datalen - 1))
- supports_mlo = true;
+ supports_mlo = elems->ml_basic;
}
/* Allow VHT if at least one channel on the sband supports 80 MHz */
--
2.34.1
From: Johannes Berg <[email protected]>
When a station is allocated, links are added but not
set to valid yet (e.g. during connection to an AP MLD),
we might remove the station without ever marking links
valid, and leak them. Fix that.
Fixes: cb71f1d136a6 ("wifi: mac80211: add sta link addition/removal")
Signed-off-by: Johannes Berg <[email protected]>
Reviewed-by: Ilan Peer <[email protected]>
Signed-off-by: Miri Korenblit <[email protected]>
---
net/mac80211/sta_info.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index bf1adcd96b41..92a7ba7c9c9d 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -404,7 +404,10 @@ void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
int i;
for (i = 0; i < ARRAY_SIZE(sta->link); i++) {
- if (!(sta->sta.valid_links & BIT(i)))
+ struct link_sta_info *link_sta;
+
+ link_sta = rcu_access_pointer(sta->link[i]);
+ if (!link_sta)
continue;
sta_remove_link(sta, i, false);
--
2.34.1
On 1/11/24 11:17, Miri Korenblit wrote:
> From: Johannes Berg <[email protected]>
>
> To simplify the code in the next patch, disallow drivers
> supporting 40 MHz in HT but not HE, since we'd otherwise
> have to track local maximum bandwidth per mode there.
>
> Signed-off-by: Johannes Berg <[email protected]>
> Reviewed-by: Gregory Greenman <[email protected]>
> Signed-off-by: Miri Korenblit <[email protected]>
> ---
> net/mac80211/main.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index d48fa1147c14..e05bcc35bc1e 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -1119,8 +1119,26 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> supp_vht = supp_vht || sband->vht_cap.vht_supported;
>
> for_each_sband_iftype_data(sband, i, iftd) {
> + u8 he_40_mhz_cap;
> +
> supp_he = supp_he || iftd->he_cap.has_he;
> supp_eht = supp_eht || iftd->eht_cap.has_eht;
> +
> + if (sband->band == NL80211_BAND_2GHZ)
> + he_40_mhz_cap =
> + IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_IN_2G;
> + else
> + he_40_mhz_cap =
> + IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G;
> +
> + /* currently no support for HE client where HT has 40 MHz but not HT */
where HT has 40 MHz but not HE?
> + if (iftd->he_cap.has_he &&
> + iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
> + BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
> + sband->ht_cap.ht_supported &&
> + sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
> + !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
> + return -EINVAL;
> }
>
> /* HT, VHT, HE require QoS, thus >= 4 queues */
On Thu, 2024-01-11 at 15:39 -0500, Jonathan Bither wrote:
>
> > + /* currently no support for HE client where HT has 40 MHz but not HT */
> where HT has 40 MHz but not HE?
>
Yep, typo, thanks. I'll just fix it when I apply it.
(Btw, it'd help to trim quotes - no need to quote _all_ the context
before and after.)
johannes
Miri Korenblit <[email protected]> writes:
> + /* currently no support for HE client where HT has 40 MHz but not HT */
> + if (iftd->he_cap.has_he &&
> + iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
> + BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
> + sband->ht_cap.ht_supported &&
> + sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
> + !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
> + return -EINVAL;
Should there be a warning message so that this is noticed if it ever
happens? I don't know.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 1/12/24 05:21, Johannes Berg wrote:
> On Thu, 2024-01-11 at 15:39 -0500, Jonathan Bither wrote:
>>> + /* currently no support for HE client where HT has 40 MHz but not HT */
>> where HT has 40 MHz but not HE?
>>
> Yep, typo, thanks. I'll just fix it when I apply it.
No problem.
>
> (Btw, it'd help to trim quotes - no need to quote _all_ the context
> before and after.)
Sure thing, I'll try to be more mindful of this in the future.
>
> johannes
On Fri, 2024-01-12 at 15:10 +0200, Kalle Valo wrote:
> Miri Korenblit <[email protected]> writes:
>
> > + /* currently no support for HE client where HT has 40 MHz but not HT */
> > + if (iftd->he_cap.has_he &&
> > + iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
> > + BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
> > + sband->ht_cap.ht_supported &&
> > + sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
> > + !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
> > + return -EINVAL;
>
> Should there be a warning message so that this is noticed if it ever
> happens? I don't know.
Yeah I don't really know either. I've done that a lot in the past, but
these days I'm kind of thinking that people who develop their drivers
should have some debug story and be able to figure it out? You know
better perhaps ...
Though it'd kind of suck to indent this further with WARN_ON ;-)
johannes
On 1/12/24 10:42, Johannes Berg wrote:
> On Fri, 2024-01-12 at 15:10 +0200, Kalle Valo wrote:
>> Miri Korenblit <[email protected]> writes:
>>
>>> + /* currently no support for HE client where HT has 40 MHz but not HT */
>>> + if (iftd->he_cap.has_he &&
>>> + iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
>>> + BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
>>> + sband->ht_cap.ht_supported &&
>>> + sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
>>> + !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
>>> + return -EINVAL;
>>
>> Should there be a warning message so that this is noticed if it ever
>> happens? I don't know.
>
> Yeah I don't really know either. I've done that a lot in the past, but
> these days I'm kind of thinking that people who develop their drivers
> should have some debug story and be able to figure it out? You know
> better perhaps ...
>
> Though it'd kind of suck to indent this further with WARN_ON ;-)
>
> johannes
I tried backporting this patch into my 6.7 tree. An mtk7915 radio system blows up badly
in this case. Likely this is mt76 bug, but also...it used to work and the crash doesn't
make it very obvious that the above code is to blame.
So, I suggest making this a WARN_ON with appropriate debugging output, let that get some
testing, and then when drivers are fixed, maybe add in the 'return -EINVAL'.
mt7915e 0000:06:00.0: mt7915_register_device failed, ret: -22
mt7915e 0000:06:00.0: mt7915_pci_probe had error on try 3/3, ret: -22
stack segment: 0000 [#1] PREEMPT SMP
CPU: 3 PID: 35 Comm: ksoftirqd/3 Tainted: G WC 6.7.0+ #31
Hardware name: Broachlink NOAH V2 E3845/Aptio CRB, BIOS 5.6.10 08/19/2021
RIP: 0010:tasklet_action_common.constprop.0+0x80/0x220
Code: 00 00 00 00 49 8b 44 24 08 48 89 18 49 89 5c 24 08 66 90 65 66 44 09 35 9e a6 ea 7e fb 0f 1f 44 00 00 48 85 ed 74 53 48 89 eb <48> 8b 6d 000
RSP: 0018:ffffc90000153e50 EFLAGS: 00010202
RAX: ffff8881283271a8 RBX: 005fff800002012c RCX: 0000000000000006
RDX: ffffffff82606ad0 RSI: 0000000000000006 RDI: ffffea0004435488
RBP: 005fff800002012c R08: 0000000000000002 R09: 0000000080000100
R10: ffffffff826060c0 R11: 000000000002e000 R12: ffff88813bd9c230
R13: ffffea0004435488 R14: 0000000000000040 R15: 0000000000000006
FS: 0000000000000000(0000) GS:ffff88813bd80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000565149684ff4 CR3: 000000012176b000 CR4: 00000000001006f0
Call Trace:
<TASK>
? die+0x2d/0x80
? do_trap+0xcd/0xf0
? do_error_trap+0x65/0x80
? exc_stack_segment+0x33/0x50
? asm_exc_stack_segment+0x22/0x30
? tasklet_action_common.constprop.0+0x80/0x220
__do_softirq+0xb4/0x293
? sort_range+0x20/0x20
run_ksoftirqd+0x1f/0x30
smpboot_thread_fn+0xc2/0x1a0
kthread+0xdc/0x110
? kthread_complete_and_exit+0x20/0x20
ret_from_fork+0x28/0x40
? kthread_complete_and_exit+0x20/0x20
ret_from_fork_asm+0x11/0x20
</TASK>
Modules linked in: qrtr ofpart spi_nor mtd intel_rapl_msr at24 spi_intel_platform regmap_i2c spi_intel iTCO_wdt intel_pmc_bxt iTCO_vendor_supports
---[ end trace 0000000000000000 ]---
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
Johannes Berg <[email protected]> writes:
> On Fri, 2024-01-12 at 15:10 +0200, Kalle Valo wrote:
>> Miri Korenblit <[email protected]> writes:
>>
>> > + /* currently no support for HE client where HT has 40 MHz but not HT */
>> > + if (iftd->he_cap.has_he &&
>> > + iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
>> > + BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
>> > + sband->ht_cap.ht_supported &&
>> > + sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
>> > + !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
>> > + return -EINVAL;
>>
>> Should there be a warning message so that this is noticed if it ever
>> happens? I don't know.
>
> Yeah I don't really know either. I've done that a lot in the past, but
> these days I'm kind of thinking that people who develop their drivers
> should have some debug story and be able to figure it out? You know
> better perhaps ...
My worry here is the time needed to figure this all out. Especially if
we have more of these kind silent checks it could get complex quite
quick.
> Though it'd kind of suck to indent this further with WARN_ON ;-)
Yeah, WARN_ON() feels a bit too much. I was more thinking about a small
pr_err() or something like that which give at least some hint what's
happening.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Fri, 2024-01-12 at 11:58 -0800, Ben Greear wrote:
>
> I tried backporting this patch into my 6.7 tree. An mtk7915 radio system blows up badly
> in this case. Likely this is mt76 bug, but also...it used to work and the crash doesn't
> make it very obvious that the above code is to blame.
Yeah, hence my comment about kernel developers hopefully being able to
figure it out :-)
> mt7915e 0000:06:00.0: mt7915_register_device failed, ret: -22
> mt7915e 0000:06:00.0: mt7915_pci_probe had error on try 3/3, ret: -22
Felix says this kind of retry logic doesn't exist upstream, maybe you
have some delta in your tree that's making it crash?
Also, from what he says and looking at the code, it should register with
HE 40 MHz capability set whenever has_he==true, so also here, do you
have any non-upstream changes that could affect it?
johannes
On 1/15/24 14:00, Johannes Berg wrote:
> On Fri, 2024-01-12 at 11:58 -0800, Ben Greear wrote:
>>
>> I tried backporting this patch into my 6.7 tree. An mtk7915 radio system blows up badly
>> in this case. Likely this is mt76 bug, but also...it used to work and the crash doesn't
>> make it very obvious that the above code is to blame.
>
> Yeah, hence my comment about kernel developers hopefully being able to
> figure it out :-)
>
>> mt7915e 0000:06:00.0: mt7915_register_device failed, ret: -22
>> mt7915e 0000:06:00.0: mt7915_pci_probe had error on try 3/3, ret: -22
>
> Felix says this kind of retry logic doesn't exist upstream, maybe you
> have some delta in your tree that's making it crash?
>
> Also, from what he says and looking at the code, it should register with
> HE 40 MHz capability set whenever has_he==true, so also here, do you
> have any non-upstream changes that could affect it?
Yes, I do. Don't think I have anything that adjusts the 40Mhz, but possible.
Anyway, I know how to fix/work around it now, so if it works for Felix
then fine with me.
I still think you should have a pr_err or warn_on_once though, since if that clause
hits, at best case the radio suddenly became un-available.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com