[PATCH] wifi: mwifiex: Resolve the failure in downloading calibration data.

[PATCH] wifi: mwifiex: Resolve the failure in downloading calibration data.

[Jeff Chen]

Correct the command format for downloading calibration data.

Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com>
---
 drivers/net/wireless/marvell/mwifiex/fw.h     |  7 +++++++
 drivers/net/wireless/marvell/mwifiex/main.c   |  4 ----
 .../net/wireless/marvell/mwifiex/sta_cmd.c    | 19 +++++++++++++------
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
index 4a96281792cc..0c75a574a7ee 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -2352,6 +2352,12 @@ struct host_cmd_ds_add_station {
 	u8 tlv[];
 } __packed;
 
+struct host_cmd_ds_802_11_cfg_data {
+	__le16 action;
+	__le16 type;
+	__le16 data_len;
+} __packed;
+
 struct host_cmd_ds_command {
 	__le16 command;
 	__le16 size;
@@ -2431,6 +2437,7 @@ struct host_cmd_ds_command {
 		struct host_cmd_ds_pkt_aggr_ctrl pkt_aggr_ctrl;
 		struct host_cmd_ds_sta_configure sta_cfg;
 		struct host_cmd_ds_add_station sta_info;
+		struct host_cmd_ds_802_11_cfg_data cfg_data;
 	} params;
 } __packed;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 855019fe5485..80fc6d5afe86 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -691,10 +691,6 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, void *context)
 
 	init_failed = true;
 done:
-	if (adapter->cal_data) {
-		release_firmware(adapter->cal_data);
-		adapter->cal_data = NULL;
-	}
 	if (adapter->firmware) {
 		release_firmware(adapter->firmware);
 		adapter->firmware = NULL;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index e2800a831c8e..027555211a99 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -1500,18 +1500,19 @@ int mwifiex_dnld_dt_cfgdata(struct mwifiex_private *priv,
 
 /* This function prepares command of set_cfg_data. */
 static int mwifiex_cmd_cfg_data(struct mwifiex_private *priv,
-				struct host_cmd_ds_command *cmd, void *data_buf)
+				struct host_cmd_ds_command *cmd, void *data_buf, u16 cmd_action)
 {
 	struct mwifiex_adapter *adapter = priv->adapter;
 	struct property *prop = data_buf;
 	u32 len;
 	u8 *data = (u8 *)cmd + S_DS_GEN;
 	int ret;
+	struct host_cmd_ds_802_11_cfg_data *pcfg_data = &cmd->params.cfg_data;
 
 	if (prop) {
 		len = prop->length;
 		ret = of_property_read_u8_array(adapter->dt_node, prop->name,
-						data, len);
+						data + sizeof(*pcfg_data), len);
 		if (ret)
 			return ret;
 		mwifiex_dbg(adapter, INFO,
@@ -1519,15 +1520,18 @@ static int mwifiex_cmd_cfg_data(struct mwifiex_private *priv,
 			    prop->name);
 	} else if (adapter->cal_data->data && adapter->cal_data->size > 0) {
 		len = mwifiex_parse_cal_cfg((u8 *)adapter->cal_data->data,
-					    adapter->cal_data->size, data);
+					    adapter->cal_data->size, data + sizeof(*pcfg_data));
 		mwifiex_dbg(adapter, INFO,
 			    "download cfg_data from config file\n");
 	} else {
 		return -1;
 	}
 
+	pcfg_data->action = cpu_to_le16(cmd_action);
+	pcfg_data->type = cpu_to_le16(2);
+	pcfg_data->data_len = cpu_to_le16(len);
 	cmd->command = cpu_to_le16(HostCmd_CMD_CFG_DATA);
-	cmd->size = cpu_to_le16(S_DS_GEN + len);
+	cmd->size = cpu_to_le16(S_DS_GEN + sizeof(*pcfg_data) + len);
 
 	return 0;
 }
@@ -1949,7 +1953,7 @@ int mwifiex_sta_prepare_cmd(struct mwifiex_private *priv, uint16_t cmd_no,
 		ret = mwifiex_cmd_get_hw_spec(priv, cmd_ptr);
 		break;
 	case HostCmd_CMD_CFG_DATA:
-		ret = mwifiex_cmd_cfg_data(priv, cmd_ptr, data_buf);
+		ret = mwifiex_cmd_cfg_data(priv, cmd_ptr, data_buf, cmd_action);
 		break;
 	case HostCmd_CMD_MAC_CONTROL:
 		ret = mwifiex_cmd_mac_control(priv, cmd_ptr, cmd_action,
@@ -2293,9 +2297,12 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
 						"marvell,caldata");
 		}
 
-		if (adapter->cal_data)
+		if (adapter->cal_data) {
 			mwifiex_send_cmd(priv, HostCmd_CMD_CFG_DATA,
 					 HostCmd_ACT_GEN_SET, 0, NULL, true);
+			release_firmware(adapter->cal_data);
+			adapter->cal_data = NULL;
+		}
 
 		/* Read MAC address from HW */
 		ret = mwifiex_send_cmd(priv, HostCmd_CMD_GET_HW_SPEC,
-- 
2.34.1

[PATCH 2/2] wifi: mwifiex: Fix the wrong hardware setting for HT40.

[Jeff Chen]

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(-)

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);
-- 
2.34.1

Re: [PATCH 2/2] wifi: mwifiex: Fix the wrong hardware setting for HT40.

[Francesco Dolcini]

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

Re: [PATCH] wifi: mwifiex: Resolve the failure in downloading calibration data.

[Kalle Valo]

Jeff Chen <jeff.chen_1@nxp.com> writes:

> Correct the command format for downloading calibration data.
>
> Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com>

Like Francesco mentioned in patch 2, please also expand here why you are
doing all this. One sentence commit message is usually a bad idea,
unless the patch is really trivial.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

RE: [EXT] Re: [PATCH 2/2] wifi: mwifiex: Fix the wrong hardware setting for HT40.

[Jeff Chen]

> -----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

RE: [EXT] Re: [PATCH] wifi: mwifiex: Resolve the failure in downloading calibration data.

[Jeff Chen]

> -----Original Message-----
> From: Kalle Valo <kvalo@kernel.org>
> Sent: Monday, January 20, 2025 8:31 PM
> To: Jeff Chen <jeff.chen_1@nxp.com>
> Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org;
> briannorris@chromium.org; francesco@dolcini.it; Pete Hsieh
> <tsung-hsien.hsieh@nxp.com>; s.hauer@pengutronix.de
> Subject: [EXT] Re: [PATCH] wifi: mwifiex: Resolve the failure in downloading
> calibration data.
> 
> 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
> 
> 
> Jeff Chen <jeff.chen_1@nxp.com> writes:
> 
> > Correct the command format for downloading calibration data.
> >
> > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com>
> 
> Like Francesco mentioned in patch 2, please also expand here why you are
> doing all this. One sentence commit message is usually a bad idea, unless the
> patch is really trivial.
> 
> --

Hi Kalle,

Thanks for review. I will add more detailed descriptions.

Jeff

RE: [EXT] Re: [PATCH 2/2] wifi: mwifiex: Fix the wrong hardware setting for HT40.

[Jeff Chen]

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