| Message ID | 20250120074011.720358-2-jeff.chen_1@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hello Jeff, thanks for the patch. On Mon, Jan 20, 2025 at 03:40:11PM +0800, Jeff Chen wrote: > Add the missing bandwidth configuration for HT40. Can you expand this a little bit? - Is this a regression? - What is the impact of this missing configuration? It's not working at all? It's working in some unexpected way (please explain)? - Should this backported to stable (probably given the answer before it should be obvious the answer to this question)? Anything else worth mentioning? > > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > --- > drivers/net/wireless/marvell/mwifiex/11n.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c > index 66f0f5377ac1..4ae0b4aaa09a 100644 > --- a/drivers/net/wireless/marvell/mwifiex/11n.c > +++ b/drivers/net/wireless/marvell/mwifiex/11n.c > @@ -308,7 +308,7 @@ mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, > int ret_len = 0; > struct ieee80211_supported_band *sband; > struct ieee_types_header *hdr; > - u8 radio_type; > + u8 radio_type, secch_offset; > > if (!buffer || !*buffer) > return ret_len; > @@ -401,13 +401,15 @@ mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, > chan_list->chan_scan_param[0].radio_type = > mwifiex_band_to_radio_type((u8) bss_desc->bss_band); > > - if (sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 && > - bss_desc->bcn_ht_oper->ht_param & > - IEEE80211_HT_PARAM_CHAN_WIDTH_ANY) > - SET_SECONDARYCHAN(chan_list->chan_scan_param[0]. > - radio_type, > - (bss_desc->bcn_ht_oper->ht_param & > - IEEE80211_HT_PARAM_CHA_SEC_OFFSET)); > + if (sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) { > + if (bss_desc->bcn_ht_oper->ht_param & IEEE80211_HT_PARAM_CHAN_WIDTH_ANY) { > + chan_list->chan_scan_param[0].radio_type |= (CHAN_BW_40MHZ << 2); setting `radio_type |= (CHAN_BW_40MHZ << 2)` seems the only real change on this patch, correct? Anything else is cosmetic, correct? would doing just this change be equivalent, right? SET_SECONDARYCHAN(chan_list->chan_scan_param[0]. radio_type | (CHAN_BW_40MHZ << 2), (bss_desc->bcn_ht_oper->ht_param & IEEE80211_HT_PARAM_CHA_SEC_OFFSET)); Francesco
> -----Original Message----- > From: Francesco Dolcini <francesco@dolcini.it> > Sent: Monday, January 20, 2025 7:12 PM > To: Jeff Chen <jeff.chen_1@nxp.com> > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > briannorris@chromium.org; kvalo@kernel.org; francesco@dolcini.it; Pete > Hsieh <tsung-hsien.hsieh@nxp.com>; s.hauer@pengutronix.de > Subject: [EXT] Re: [PATCH 2/2] wifi: mwifiex: Fix the wrong hardware setting for > HT40. > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report > this email' button > > > Hello Jeff, > thanks for the patch. > > On Mon, Jan 20, 2025 at 03:40:11PM +0800, Jeff Chen wrote: > > Add the missing bandwidth configuration for HT40. > > Can you expand this a little bit? > > - Is this a regression? > - What is the impact of this missing configuration? It's not working at all? > It's working in some unexpected way (please explain)? > - Should this backported to stable (probably given the answer before it should > be obvious the answer to this question)? > > Anything else worth mentioning? > > > > > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > > --- > > drivers/net/wireless/marvell/mwifiex/11n.c | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c > > b/drivers/net/wireless/marvell/mwifiex/11n.c > > index 66f0f5377ac1..4ae0b4aaa09a 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/11n.c > > +++ b/drivers/net/wireless/marvell/mwifiex/11n.c > > @@ -308,7 +308,7 @@ mwifiex_cmd_append_11n_tlv(struct > mwifiex_private *priv, > > int ret_len = 0; > > struct ieee80211_supported_band *sband; > > struct ieee_types_header *hdr; > > - u8 radio_type; > > + u8 radio_type, secch_offset; > > > > if (!buffer || !*buffer) > > return ret_len; > > @@ -401,13 +401,15 @@ mwifiex_cmd_append_11n_tlv(struct > mwifiex_private *priv, > > chan_list->chan_scan_param[0].radio_type = > > mwifiex_band_to_radio_type((u8) > > bss_desc->bss_band); > > > > - if (sband->ht_cap.cap & > IEEE80211_HT_CAP_SUP_WIDTH_20_40 && > > - bss_desc->bcn_ht_oper->ht_param & > > - IEEE80211_HT_PARAM_CHAN_WIDTH_ANY) > > - > SET_SECONDARYCHAN(chan_list->chan_scan_param[0]. > > - radio_type, > > - > (bss_desc->bcn_ht_oper->ht_param & > > - > IEEE80211_HT_PARAM_CHA_SEC_OFFSET)); > > + if (sband->ht_cap.cap & > IEEE80211_HT_CAP_SUP_WIDTH_20_40) { > > + if (bss_desc->bcn_ht_oper->ht_param & > IEEE80211_HT_PARAM_CHAN_WIDTH_ANY) { > > + > chan_list->chan_scan_param[0].radio_type > > + |= (CHAN_BW_40MHZ << 2); > > setting `radio_type |= (CHAN_BW_40MHZ << 2)` seems the only real change on > this patch, correct? Anything else is cosmetic, correct? > > would doing just this change be equivalent, right? > > SET_SECONDARYCHAN(chan_list->chan_scan_param[0]. > radio_type | (CHAN_BW_40MHZ << 2), > (bss_desc->bcn_ht_oper->ht_param & > IEEE80211_HT_PARAM_CHA_SEC_OFFSET)); > > > Francesco Hi Francesco, Thank you for the review and advice. I will revise the patch and provide more detailed descriptions. Jeff
Hello Francesco, Thank you for reviewing the patch. > -----Original Message----- > From: Francesco Dolcini <francesco@dolcini.it> > Sent: Monday, January 20, 2025 7:12 PM > To: Jeff Chen <jeff.chen_1@nxp.com> > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > briannorris@chromium.org; kvalo@kernel.org; francesco@dolcini.it; Pete > Hsieh <tsung-hsien.hsieh@nxp.com>; s.hauer@pengutronix.de > Subject: [EXT] Re: [PATCH 2/2] wifi: mwifiex: Fix the wrong hardware setting for > HT40. > > > Can you expand this a little bit? > > - Is this a regression? - No, this is not a regression. > - What is the impact of this missing configuration? It's not working at all? Without this configuration, the connection operates at 20MHz even if the access point supports 40MHz bandwidth. This means that while the device can connect, it does so with reduced bandwidth, potentially affecting performance and throughput. > It's working in some unexpected way (please explain)? As mentioned above, the connection operates at 20MHz even if the access point supports 40MHz bandwidth. > - Should this backported to stable (probably given the answer before it should > be obvious the answer to this question)? Considering that HT40 mode is not commonly used on the 2.4GHz band due to limited bandwidth and potential interference, backporting this fix may not be necessary. > > Anything else worth mentioning? > There is nothing additional to mention. > > > > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > > --- > > drivers/net/wireless/marvell/mwifiex/11n.c | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c > > b/drivers/net/wireless/marvell/mwifiex/11n.c > > index 66f0f5377ac1..4ae0b4aaa09a 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/11n.c > > +++ b/drivers/net/wireless/marvell/mwifiex/11n.c > > @@ -308,7 +308,7 @@ mwifiex_cmd_append_11n_tlv(struct > mwifiex_private *priv, > > int ret_len = 0; > > struct ieee80211_supported_band *sband; > > struct ieee_types_header *hdr; > > - u8 radio_type; > > + u8 radio_type, secch_offset; > > > > if (!buffer || !*buffer) > > return ret_len; > > @@ -401,13 +401,15 @@ mwifiex_cmd_append_11n_tlv(struct > mwifiex_private *priv, > > chan_list->chan_scan_param[0].radio_type = > > mwifiex_band_to_radio_type((u8) > > bss_desc->bss_band); > > > > - if (sband->ht_cap.cap & > IEEE80211_HT_CAP_SUP_WIDTH_20_40 && > > - bss_desc->bcn_ht_oper->ht_param & > > - IEEE80211_HT_PARAM_CHAN_WIDTH_ANY) > > - > SET_SECONDARYCHAN(chan_list->chan_scan_param[0]. > > - radio_type, > > - > (bss_desc->bcn_ht_oper->ht_param & > > - > IEEE80211_HT_PARAM_CHA_SEC_OFFSET)); > > + if (sband->ht_cap.cap & > IEEE80211_HT_CAP_SUP_WIDTH_20_40) { > > + if (bss_desc->bcn_ht_oper->ht_param & > IEEE80211_HT_PARAM_CHAN_WIDTH_ANY) { > > + > chan_list->chan_scan_param[0].radio_type > > + |= (CHAN_BW_40MHZ << 2); > > setting `radio_type |= (CHAN_BW_40MHZ << 2)` seems the only real change > on this patch, correct? Anything else is cosmetic, correct? > > would doing just this change be equivalent, right? > > SET_SECONDARYCHAN(chan_list->chan_scan_param[0]. > radio_type | (CHAN_BW_40MHZ << 2), > (bss_desc->bcn_ht_oper->ht_param & > IEEE80211_HT_PARAM_CHA_SEC_OFFSET)); > > > Francesco
diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c index 66f0f5377ac1..4ae0b4aaa09a 100644 --- a/drivers/net/wireless/marvell/mwifiex/11n.c +++ b/drivers/net/wireless/marvell/mwifiex/11n.c @@ -308,7 +308,7 @@ mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, int ret_len = 0; struct ieee80211_supported_band *sband; struct ieee_types_header *hdr; - u8 radio_type; + u8 radio_type, secch_offset; if (!buffer || !*buffer) return ret_len; @@ -401,13 +401,15 @@ mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, chan_list->chan_scan_param[0].radio_type = mwifiex_band_to_radio_type((u8) bss_desc->bss_band); - if (sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 && - bss_desc->bcn_ht_oper->ht_param & - IEEE80211_HT_PARAM_CHAN_WIDTH_ANY) - SET_SECONDARYCHAN(chan_list->chan_scan_param[0]. - radio_type, - (bss_desc->bcn_ht_oper->ht_param & - IEEE80211_HT_PARAM_CHA_SEC_OFFSET)); + if (sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) { + if (bss_desc->bcn_ht_oper->ht_param & IEEE80211_HT_PARAM_CHAN_WIDTH_ANY) { + chan_list->chan_scan_param[0].radio_type |= (CHAN_BW_40MHZ << 2); + secch_offset = bss_desc->bcn_ht_oper->ht_param & + IEEE80211_HT_PARAM_CHA_SEC_OFFSET; + SET_SECONDARYCHAN(chan_list->chan_scan_param[0].radio_type, + secch_offset); + } + } *buffer += struct_size(chan_list, chan_scan_param, 1); ret_len += struct_size(chan_list, chan_scan_param, 1);
Add the missing bandwidth configuration for HT40. Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> --- drivers/net/wireless/marvell/mwifiex/11n.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)