[PATCH v3 0/2] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[PATCH v3 0/2] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Jonas Dreßler]

Third revision of this patch.
v1: https://lore.kernel.org/linux-wireless/20211028073729.24408-1-verdre@v0yd.nl/T/
v2: https://lore.kernel.org/linux-wireless/20211103135529.8537-1-verdre@v0yd.nl/T/

Changes between v2 and v3:
 - Remove redundant sizeof(char) in the second patch

Jonas Dreßler (2):
  mwifiex: Use a define for firmware version string length
  mwifiex: Add quirk to disable deep sleep with certain hardware
    revision

 drivers/net/wireless/marvell/mwifiex/fw.h     |  4 ++-
 drivers/net/wireless/marvell/mwifiex/main.c   | 18 +++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.h   |  3 ++-
 .../wireless/marvell/mwifiex/sta_cmdresp.c    | 25 +++++++++++++++++--
 4 files changed, 46 insertions(+), 4 deletions(-)

-- 
2.33.1

[PATCH v3 1/2] mwifiex: Use a define for firmware version string length

[Jonas Dreßler]

Since the version string we get from the firmware is always 128
characters long, use a define for this size instead of having the number
128 copied all over the place.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/fw.h          | 4 +++-
 drivers/net/wireless/marvell/mwifiex/main.h        | 2 +-
 drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 5 +++--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
index 2ff23ab259ab..63c25c69ed2b 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -2071,9 +2071,11 @@ struct mwifiex_ie_types_robust_coex {
 	__le32 mode;
 } __packed;
 
+#define MWIFIEX_VERSION_STR_LENGTH  128
+
 struct host_cmd_ds_version_ext {
 	u8 version_str_sel;
-	char version_str[128];
+	char version_str[MWIFIEX_VERSION_STR_LENGTH];
 } __packed;
 
 struct host_cmd_ds_mgmt_frame_reg {
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 90012cbcfd15..65609ea2327e 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -646,7 +646,7 @@ struct mwifiex_private {
 	struct wireless_dev wdev;
 	struct mwifiex_chan_freq_power cfp;
 	u32 versionstrsel;
-	char version_str[128];
+	char version_str[MWIFIEX_VERSION_STR_LENGTH];
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dfs_dev_dir;
 #endif
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
index 6b5d35d9e69f..20b69a37f9e1 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
@@ -711,8 +711,9 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
 	if (version_ext) {
 		version_ext->version_str_sel = ver_ext->version_str_sel;
 		memcpy(version_ext->version_str, ver_ext->version_str,
-		       sizeof(char) * 128);
-		memcpy(priv->version_str, ver_ext->version_str, 128);
+		       MWIFIEX_VERSION_STR_LENGTH);
+		memcpy(priv->version_str, ver_ext->version_str,
+		       MWIFIEX_VERSION_STR_LENGTH);
 	}
 	return 0;
 }
-- 
2.33.1

[PATCH v3 2/2] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Jonas Dreßler]

The 88W8897 PCIe+USB card in the hardware revision 20 apparently has a
hardware issue where the card wakes up from deep sleep randomly and very
often, somewhat depending on the card activity, maybe the hardware has a
floating wakeup pin or something. This was found by comparing two MS
Surface Book 2 devices, where one devices wifi card experienced spurious
wakeups, while the other one didn't.

Those continuous wakeups prevent the card from entering host sleep when
the computer suspends. And because the host won't answer to events from
the card anymore while it's suspended, the firmwares internal power
saving state machine seems to get confused and the card can't sleep
anymore at all after that.

Since we can't work around that hardware bug in the firmware, let's
get the hardware revision string from the firmware and match it with
known bad revisions. Then disable auto deep sleep for those revisions,
which makes sure we no longer get those spurious wakeups.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/main.c   | 18 +++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.h   |  1 +
 .../wireless/marvell/mwifiex/sta_cmdresp.c    | 20 +++++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 19b996c6a260..ace7371c4773 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -226,6 +226,23 @@ static int mwifiex_process_rx(struct mwifiex_adapter *adapter)
 	return 0;
 }
 
+static void maybe_quirk_fw_disable_ds(struct mwifiex_adapter *adapter)
+{
+	struct mwifiex_private *priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA);
+	struct mwifiex_ver_ext ver_ext;
+
+	if (test_and_set_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &adapter->work_flags))
+		return;
+
+	memset(&ver_ext, 0, sizeof(ver_ext));
+	ver_ext.version_str_sel = 1;
+	if (mwifiex_send_cmd(priv, HostCmd_CMD_VERSION_EXT,
+			     HostCmd_ACT_GEN_GET, 0, &ver_ext, false)) {
+		mwifiex_dbg(priv->adapter, MSG,
+			    "Checking hardware revision failed.\n");
+	}
+}
+
 /*
  * The main process.
  *
@@ -356,6 +373,7 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 			if (adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE) {
 				adapter->hw_status = MWIFIEX_HW_STATUS_READY;
 				mwifiex_init_fw_complete(adapter);
+				maybe_quirk_fw_disable_ds(adapter);
 			}
 		}
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 65609ea2327e..eabd0e0a9f56 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -524,6 +524,7 @@ enum mwifiex_adapter_work_flags {
 	MWIFIEX_IS_SUSPENDED,
 	MWIFIEX_IS_HS_CONFIGURED,
 	MWIFIEX_IS_HS_ENABLING,
+	MWIFIEX_IS_REQUESTING_FW_VEREXT,
 };
 
 struct mwifiex_band_config {
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
index 20b69a37f9e1..6c7b0b9bc4e9 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
@@ -708,6 +708,26 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
 {
 	struct host_cmd_ds_version_ext *ver_ext = &resp->params.verext;
 
+	if (test_and_clear_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &priv->adapter->work_flags)) {
+		if (strncmp(ver_ext->version_str, "ChipRev:20, BB:9b(10.00), RF:40(21)",
+			    MWIFIEX_VERSION_STR_LENGTH) == 0) {
+			struct mwifiex_ds_auto_ds auto_ds = {
+				.auto_ds = DEEP_SLEEP_OFF,
+			};
+
+			mwifiex_dbg(priv->adapter, MSG,
+				    "Bad HW revision detected, disabling deep sleep\n");
+
+			if (mwifiex_send_cmd(priv, HostCmd_CMD_802_11_PS_MODE_ENH,
+					     DIS_AUTO_PS, BITMAP_AUTO_DS, &auto_ds, false)) {
+				mwifiex_dbg(priv->adapter, MSG,
+					    "Disabling deep sleep failed.\n");
+			}
+		}
+
+		return 0;
+	}
+
 	if (version_ext) {
 		version_ext->version_str_sel = ver_ext->version_str_sel;
 		memcpy(version_ext->version_str, ver_ext->version_str,
-- 
2.33.1

Re: [PATCH v3 1/2] mwifiex: Use a define for firmware version string length

[Brian Norris]

On Wed, Nov 03, 2021 at 06:10:54PM +0100, Jonas Dreßler wrote:
> Since the version string we get from the firmware is always 128
> characters long, use a define for this size instead of having the number
> 128 copied all over the place.
> 
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>

Thanks for this patch. For the series:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> ---
>  drivers/net/wireless/marvell/mwifiex/fw.h          | 4 +++-
>  drivers/net/wireless/marvell/mwifiex/main.h        | 2 +-
>  drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 5 +++--
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 2ff23ab259ab..63c25c69ed2b 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -2071,9 +2071,11 @@ struct mwifiex_ie_types_robust_coex {
>  	__le32 mode;
>  } __packed;
>  
> +#define MWIFIEX_VERSION_STR_LENGTH  128
> +
>  struct host_cmd_ds_version_ext {
>  	u8 version_str_sel;
> -	char version_str[128];
> +	char version_str[MWIFIEX_VERSION_STR_LENGTH];
>  } __packed;
>  
>  struct host_cmd_ds_mgmt_frame_reg {
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 90012cbcfd15..65609ea2327e 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -646,7 +646,7 @@ struct mwifiex_private {
>  	struct wireless_dev wdev;
>  	struct mwifiex_chan_freq_power cfp;
>  	u32 versionstrsel;
> -	char version_str[128];
> +	char version_str[MWIFIEX_VERSION_STR_LENGTH];
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *dfs_dev_dir;
>  #endif
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> index 6b5d35d9e69f..20b69a37f9e1 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> @@ -711,8 +711,9 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
>  	if (version_ext) {
>  		version_ext->version_str_sel = ver_ext->version_str_sel;
>  		memcpy(version_ext->version_str, ver_ext->version_str,
> -		       sizeof(char) * 128);
> -		memcpy(priv->version_str, ver_ext->version_str, 128);
> +		       MWIFIEX_VERSION_STR_LENGTH);
> +		memcpy(priv->version_str, ver_ext->version_str,
> +		       MWIFIEX_VERSION_STR_LENGTH);

Not related to your patch, but this highlights that nobody is ensuring
this string is 0-terminated, and various other places (notably, *not*
your patch 2!) assume that it is.

We should either fix those to use an snprintf()/length-restricted
variant, or else just force:

		priv->version_str[MWIFIEX_VERSION_STR_LENGTH - 1] = '\0';

here.

But that's a separate issue/patch. I can cook one on top of your series
when it gets merged if you don't want to.

Brian

>  	}
>  	return 0;
>  }
> -- 
> 2.33.1
> 

Re: [PATCH v3 2/2] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Andy Shevchenko]

On Wed, Nov 03, 2021 at 06:10:55PM +0100, Jonas Dreßler wrote:
> The 88W8897 PCIe+USB card in the hardware revision 20 apparently has a
> hardware issue where the card wakes up from deep sleep randomly and very
> often, somewhat depending on the card activity, maybe the hardware has a
> floating wakeup pin or something. This was found by comparing two MS
> Surface Book 2 devices, where one devices wifi card experienced spurious
> wakeups, while the other one didn't.
> 
> Those continuous wakeups prevent the card from entering host sleep when
> the computer suspends. And because the host won't answer to events from
> the card anymore while it's suspended, the firmwares internal power
> saving state machine seems to get confused and the card can't sleep
> anymore at all after that.
> 
> Since we can't work around that hardware bug in the firmware, let's
> get the hardware revision string from the firmware and match it with
> known bad revisions. Then disable auto deep sleep for those revisions,
> which makes sure we no longer get those spurious wakeups.
> 
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c   | 18 +++++++++++++++++
>  drivers/net/wireless/marvell/mwifiex/main.h   |  1 +
>  .../wireless/marvell/mwifiex/sta_cmdresp.c    | 20 +++++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 19b996c6a260..ace7371c4773 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -226,6 +226,23 @@ static int mwifiex_process_rx(struct mwifiex_adapter *adapter)
>  	return 0;
>  }
>  
> +static void maybe_quirk_fw_disable_ds(struct mwifiex_adapter *adapter)
> +{
> +	struct mwifiex_private *priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA);
> +	struct mwifiex_ver_ext ver_ext;
> +
> +	if (test_and_set_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &adapter->work_flags))
> +		return;
> +
> +	memset(&ver_ext, 0, sizeof(ver_ext));
> +	ver_ext.version_str_sel = 1;

> +	if (mwifiex_send_cmd(priv, HostCmd_CMD_VERSION_EXT,
> +			     HostCmd_ACT_GEN_GET, 0, &ver_ext, false)) {
> +		mwifiex_dbg(priv->adapter, MSG,
> +			    "Checking hardware revision failed.\n");
> +	}

Checkpatch won't warn you if string literal even > 100. So move it to one line
and drop curly braces. Ditto for the case(s) below.

> +}
> +
>  /*
>   * The main process.
>   *
> @@ -356,6 +373,7 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
>  			if (adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE) {
>  				adapter->hw_status = MWIFIEX_HW_STATUS_READY;
>  				mwifiex_init_fw_complete(adapter);
> +				maybe_quirk_fw_disable_ds(adapter);
>  			}
>  		}
>  
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 65609ea2327e..eabd0e0a9f56 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -524,6 +524,7 @@ enum mwifiex_adapter_work_flags {
>  	MWIFIEX_IS_SUSPENDED,
>  	MWIFIEX_IS_HS_CONFIGURED,
>  	MWIFIEX_IS_HS_ENABLING,
> +	MWIFIEX_IS_REQUESTING_FW_VEREXT,
>  };
>  
>  struct mwifiex_band_config {
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> index 20b69a37f9e1..6c7b0b9bc4e9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> @@ -708,6 +708,26 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
>  {
>  	struct host_cmd_ds_version_ext *ver_ext = &resp->params.verext;
>  
> +	if (test_and_clear_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &priv->adapter->work_flags)) {
> +		if (strncmp(ver_ext->version_str, "ChipRev:20, BB:9b(10.00), RF:40(21)",
> +			    MWIFIEX_VERSION_STR_LENGTH) == 0) {
> +			struct mwifiex_ds_auto_ds auto_ds = {
> +				.auto_ds = DEEP_SLEEP_OFF,
> +			};
> +
> +			mwifiex_dbg(priv->adapter, MSG,
> +				    "Bad HW revision detected, disabling deep sleep\n");
> +
> +			if (mwifiex_send_cmd(priv, HostCmd_CMD_802_11_PS_MODE_ENH,
> +					     DIS_AUTO_PS, BITMAP_AUTO_DS, &auto_ds, false)) {
> +				mwifiex_dbg(priv->adapter, MSG,
> +					    "Disabling deep sleep failed.\n");
> +			}
> +		}
> +
> +		return 0;
> +	}
> +
>  	if (version_ext) {
>  		version_ext->version_str_sel = ver_ext->version_str_sel;
>  		memcpy(version_ext->version_str, ver_ext->version_str,
> -- 
> 2.33.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/2] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Andy Shevchenko]

On Wed, Nov 03, 2021 at 06:10:53PM +0100, Jonas Dreßler wrote:
> Third revision of this patch.
> v1: https://lore.kernel.org/linux-wireless/20211028073729.24408-1-verdre@v0yd.nl/T/
> v2: https://lore.kernel.org/linux-wireless/20211103135529.8537-1-verdre@v0yd.nl/T/

With one comment addressed
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for continuing fixing this crappy SW+FW.

> Changes between v2 and v3:
>  - Remove redundant sizeof(char) in the second patch
> 
> Jonas Dreßler (2):
>   mwifiex: Use a define for firmware version string length
>   mwifiex: Add quirk to disable deep sleep with certain hardware
>     revision
> 
>  drivers/net/wireless/marvell/mwifiex/fw.h     |  4 ++-
>  drivers/net/wireless/marvell/mwifiex/main.c   | 18 +++++++++++++
>  drivers/net/wireless/marvell/mwifiex/main.h   |  3 ++-
>  .../wireless/marvell/mwifiex/sta_cmdresp.c    | 25 +++++++++++++++++--
>  4 files changed, 46 insertions(+), 4 deletions(-)
> 
> -- 
> 2.33.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Bjorn Helgaas]

On Wed, Nov 03, 2021 at 07:38:35PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 03, 2021 at 06:10:55PM +0100, Jonas Dreßler wrote:

> > +	if (mwifiex_send_cmd(priv, HostCmd_CMD_VERSION_EXT,
> > +			     HostCmd_ACT_GEN_GET, 0, &ver_ext, false)) {
> > +		mwifiex_dbg(priv->adapter, MSG,
> > +			    "Checking hardware revision failed.\n");
> > +	}
> 
> Checkpatch won't warn you if string literal even > 100. So move it to one line
> and drop curly braces. Ditto for the case(s) below.

I don't understand the advantage of making this one line.  I *do*
understand the advantage of joining a single string so grep can find
it more easily.  But that does make the code a little bit uglier, and
in a case like this, you don't get the benefit of better grepping, so
I don't see the point.

Re: [PATCH v3 2/2] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Andy Shevchenko]

On Wed, Nov 03, 2021 at 12:45:27PM -0500, Bjorn Helgaas wrote:
> On Wed, Nov 03, 2021 at 07:38:35PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 03, 2021 at 06:10:55PM +0100, Jonas Dreßler wrote:
> 
> > > +	if (mwifiex_send_cmd(priv, HostCmd_CMD_VERSION_EXT,
> > > +			     HostCmd_ACT_GEN_GET, 0, &ver_ext, false)) {
> > > +		mwifiex_dbg(priv->adapter, MSG,
> > > +			    "Checking hardware revision failed.\n");
> > > +	}
> > 
> > Checkpatch won't warn you if string literal even > 100. So move it to one line
> > and drop curly braces. Ditto for the case(s) below.
> 
> I don't understand the advantage of making this one line.  I *do*
> understand the advantage of joining a single string so grep can find
> it more easily.  But that does make the code a little bit uglier, and
> in a case like this, you don't get the benefit of better grepping, so
> I don't see the point.

Then disregard my comment. I've no hard feelings about it :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] mwifiex: Use a define for firmware version string length

[Jonas Dreßler]

On 11/3/21 18:28, Brian Norris wrote:
> On Wed, Nov 03, 2021 at 06:10:54PM +0100, Jonas Dreßler wrote:
>> Since the version string we get from the firmware is always 128
>> characters long, use a define for this size instead of having the number
>> 128 copied all over the place.
>>
>> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> 
> Thanks for this patch. For the series:
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 
>> ---
>>   drivers/net/wireless/marvell/mwifiex/fw.h          | 4 +++-
>>   drivers/net/wireless/marvell/mwifiex/main.h        | 2 +-
>>   drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 5 +++--
>>   3 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
>> index 2ff23ab259ab..63c25c69ed2b 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
>> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
>> @@ -2071,9 +2071,11 @@ struct mwifiex_ie_types_robust_coex {
>>   	__le32 mode;
>>   } __packed;
>>   
>> +#define MWIFIEX_VERSION_STR_LENGTH  128
>> +
>>   struct host_cmd_ds_version_ext {
>>   	u8 version_str_sel;
>> -	char version_str[128];
>> +	char version_str[MWIFIEX_VERSION_STR_LENGTH];
>>   } __packed;
>>   
>>   struct host_cmd_ds_mgmt_frame_reg {
>> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
>> index 90012cbcfd15..65609ea2327e 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/main.h
>> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
>> @@ -646,7 +646,7 @@ struct mwifiex_private {
>>   	struct wireless_dev wdev;
>>   	struct mwifiex_chan_freq_power cfp;
>>   	u32 versionstrsel;
>> -	char version_str[128];
>> +	char version_str[MWIFIEX_VERSION_STR_LENGTH];
>>   #ifdef CONFIG_DEBUG_FS
>>   	struct dentry *dfs_dev_dir;
>>   #endif
>> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>> index 6b5d35d9e69f..20b69a37f9e1 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>> @@ -711,8 +711,9 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
>>   	if (version_ext) {
>>   		version_ext->version_str_sel = ver_ext->version_str_sel;
>>   		memcpy(version_ext->version_str, ver_ext->version_str,
>> -		       sizeof(char) * 128);
>> -		memcpy(priv->version_str, ver_ext->version_str, 128);
>> +		       MWIFIEX_VERSION_STR_LENGTH);
>> +		memcpy(priv->version_str, ver_ext->version_str,
>> +		       MWIFIEX_VERSION_STR_LENGTH);
> 
> Not related to your patch, but this highlights that nobody is ensuring
> this string is 0-terminated, and various other places (notably, *not*
> your patch 2!) assume that it is.
> 
> We should either fix those to use an snprintf()/length-restricted
> variant, or else just force:
> 
> 		priv->version_str[MWIFIEX_VERSION_STR_LENGTH - 1] = '\0';
> 
> here.

Indeed, right now we just trust the firmware to make sure that's the case.

Let me add another patch appending the '\0' to priv->version_str...

> 
> But that's a separate issue/patch. I can cook one on top of your series
> when it gets merged if you don't want to.
> 
> Brian
> 
>>   	}
>>   	return 0;
>>   }
>> -- 
>> 2.33.1
>>