[PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

Current firmware doesn't support AP and STA running on different
channels simultaneously.
FW crash would occur in such case.
This patch avoids the issue by disabling AP and STA to run on
different channels.

Signed-off-by: David Lin <yu-hao.lin@nxp.com>
---

v2:
   - clean up code.

---
 .../net/wireless/marvell/mwifiex/cfg80211.c   | 17 ++++---
 drivers/net/wireless/marvell/mwifiex/util.c   | 44 +++++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/util.h   | 13 ++++++
 3 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 722ead51e912..3dbcab463445 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -781,11 +781,9 @@ mwifiex_cfg80211_set_wiphy_params(struct wiphy *wiphy, u32 changed)
 		break;
 
 	case MWIFIEX_BSS_ROLE_STA:
-		if (priv->media_connected) {
-			mwifiex_dbg(adapter, ERROR,
-				    "cannot change wiphy params when connected");
-			return -EINVAL;
-		}
+		if (priv->media_connected)
+			break;
+
 		if (changed & WIPHY_PARAM_RTS_THRESHOLD) {
 			ret = mwifiex_set_rts(priv,
 					      wiphy->rts_threshold);
@@ -2069,6 +2067,9 @@ static int mwifiex_cfg80211_start_ap(struct wiphy *wiphy,
 	if (GET_BSS_ROLE(priv) != MWIFIEX_BSS_ROLE_UAP)
 		return -1;
 
+	if (!mwifiex_is_channel_setting_allowable(priv, params->chandef.chan))
+		return -EOPNOTSUPP;
+
 	bss_cfg = kzalloc(sizeof(struct mwifiex_uap_bss_param), GFP_KERNEL);
 	if (!bss_cfg)
 		return -ENOMEM;
@@ -2463,6 +2464,9 @@ mwifiex_cfg80211_connect(struct wiphy *wiphy, struct net_device *dev,
 		return -EFAULT;
 	}
 
+	if (!mwifiex_is_channel_setting_allowable(priv, sme->channel))
+		return -EOPNOTSUPP;
+
 	mwifiex_dbg(adapter, INFO,
 		    "info: Trying to associate to bssid %pM\n", sme->bssid);
 
@@ -4298,6 +4302,9 @@ mwifiex_cfg80211_authenticate(struct wiphy *wiphy,
 		return -EINVAL;
 	}
 
+	if (!mwifiex_is_channel_setting_allowable(priv, req->bss->channel))
+		return -EOPNOTSUPP;
+
 	if (priv->auth_alg != WLAN_AUTH_SAE &&
 	    (priv->auth_flag & HOST_MLME_AUTH_PENDING)) {
 		mwifiex_dbg(priv->adapter, ERROR, "Pending auth on going\n");
diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
index 42c04bf858da..6512562c9fb4 100644
--- a/drivers/net/wireless/marvell/mwifiex/util.c
+++ b/drivers/net/wireless/marvell/mwifiex/util.c
@@ -323,6 +323,50 @@ int mwifiex_debug_info_to_buffer(struct mwifiex_private *priv, char *buf,
 	return p - buf;
 }
 
+bool mwifiex_is_channel_setting_allowable(struct mwifiex_private *priv,
+					  struct ieee80211_channel *check_chan)
+{
+	struct mwifiex_adapter *adapter = priv->adapter;
+	int i;
+	struct mwifiex_private *tmp_priv;
+	u8 bss_role = GET_BSS_ROLE(priv);
+	struct ieee80211_channel *set_chan;
+
+	for (i = 0; i < MWIFIEX_MAX_BSS_NUM; i++) {
+		tmp_priv = adapter->priv[i];
+		if (tmp_priv == priv)
+			continue;
+
+		set_chan = NULL;
+		if (bss_role == MWIFIEX_BSS_ROLE_STA) {
+			if (GET_BSS_ROLE(tmp_priv) == MWIFIEX_BSS_ROLE_UAP &&
+			    netif_carrier_ok(tmp_priv->netdev) &&
+			    cfg80211_chandef_valid(&tmp_priv->bss_chandef))
+				set_chan = tmp_priv->bss_chandef.chan;
+		} else if (bss_role == MWIFIEX_BSS_ROLE_UAP) {
+			struct mwifiex_current_bss_params *bss_params =
+				&tmp_priv->curr_bss_params;
+			int channel = bss_params->bss_descriptor.channel;
+			enum nl80211_band band =
+				mwifiex_band_to_radio_type(bss_params->band);
+			int freq =
+				ieee80211_channel_to_frequency(channel, band);
+
+			if (GET_BSS_ROLE(tmp_priv) == MWIFIEX_BSS_ROLE_STA &&
+			    tmp_priv->media_connected)
+				set_chan = ieee80211_get_channel(adapter->wiphy, freq);
+		}
+
+		if (set_chan && !ieee80211_channel_equal(check_chan, set_chan)) {
+			mwifiex_dbg(adapter, ERROR,
+				    "AP/STA must run on the same channel\n");
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static int
 mwifiex_parse_mgmt_packet(struct mwifiex_private *priv, u8 *payload, u16 len,
 			  struct rxpd *rx_pd)
diff --git a/drivers/net/wireless/marvell/mwifiex/util.h b/drivers/net/wireless/marvell/mwifiex/util.h
index 4699c505c0a0..16f092bb0823 100644
--- a/drivers/net/wireless/marvell/mwifiex/util.h
+++ b/drivers/net/wireless/marvell/mwifiex/util.h
@@ -86,4 +86,17 @@ static inline void le16_unaligned_add_cpu(__le16 *var, u16 val)
 	put_unaligned_le16(get_unaligned_le16(var) + val, var);
 }
 
+/* Current firmware doesn't support AP and STA running on different
+ * channels simultaneously in normal mode.
+ * FW crash would occur in such case.
+ * This function is used to check if check_chan can be set to FW or not.
+ *
+ * Return:
+ * %true if check_chan can be set to FW without issues.
+ * %false there is already other channel is set to FW, setting of
+ * check_chan is not allowable.
+ */
+bool mwifiex_is_channel_setting_allowable(struct mwifiex_private *priv,
+					  struct ieee80211_channel *check_chan);
+
 #endif /* !_MWIFIEX_UTIL_H_ */

base-commit: ae98f5c9fd8ba84cd408b41faa77e65bf1b4cdfa
-- 
2.34.1

Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[Sascha Hauer]

On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> Current firmware doesn't support AP and STA running on different
> channels simultaneously.

As mentioned here:

https://lore.kernel.org/all/ZtGnWC7SPHt7Vbbp@pengutronix.de/

AP and STA can indeed have different channels when DRCS is enabled, so I
think you have to check this in your patch.

Maybe the same question here again: Wouldn't it make sense to enable
DRCS by default?

Sascha

> FW crash would occur in such case.
> This patch avoids the issue by disabling AP and STA to run on
> different channels.
> 
> Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> ---
> 
> v2:
>    - clean up code.
> 
> ---
>  .../net/wireless/marvell/mwifiex/cfg80211.c   | 17 ++++---
>  drivers/net/wireless/marvell/mwifiex/util.c   | 44 +++++++++++++++++++
>  drivers/net/wireless/marvell/mwifiex/util.h   | 13 ++++++
>  3 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 722ead51e912..3dbcab463445 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -781,11 +781,9 @@ mwifiex_cfg80211_set_wiphy_params(struct wiphy *wiphy, u32 changed)
>  		break;
>  
>  	case MWIFIEX_BSS_ROLE_STA:
> -		if (priv->media_connected) {
> -			mwifiex_dbg(adapter, ERROR,
> -				    "cannot change wiphy params when connected");
> -			return -EINVAL;
> -		}
> +		if (priv->media_connected)
> +			break;
> +
>  		if (changed & WIPHY_PARAM_RTS_THRESHOLD) {
>  			ret = mwifiex_set_rts(priv,
>  					      wiphy->rts_threshold);
> @@ -2069,6 +2067,9 @@ static int mwifiex_cfg80211_start_ap(struct wiphy *wiphy,
>  	if (GET_BSS_ROLE(priv) != MWIFIEX_BSS_ROLE_UAP)
>  		return -1;
>  
> +	if (!mwifiex_is_channel_setting_allowable(priv, params->chandef.chan))
> +		return -EOPNOTSUPP;
> +
>  	bss_cfg = kzalloc(sizeof(struct mwifiex_uap_bss_param), GFP_KERNEL);
>  	if (!bss_cfg)
>  		return -ENOMEM;
> @@ -2463,6 +2464,9 @@ mwifiex_cfg80211_connect(struct wiphy *wiphy, struct net_device *dev,
>  		return -EFAULT;
>  	}
>  
> +	if (!mwifiex_is_channel_setting_allowable(priv, sme->channel))
> +		return -EOPNOTSUPP;
> +
>  	mwifiex_dbg(adapter, INFO,
>  		    "info: Trying to associate to bssid %pM\n", sme->bssid);
>  
> @@ -4298,6 +4302,9 @@ mwifiex_cfg80211_authenticate(struct wiphy *wiphy,
>  		return -EINVAL;
>  	}
>  
> +	if (!mwifiex_is_channel_setting_allowable(priv, req->bss->channel))
> +		return -EOPNOTSUPP;
> +
>  	if (priv->auth_alg != WLAN_AUTH_SAE &&
>  	    (priv->auth_flag & HOST_MLME_AUTH_PENDING)) {
>  		mwifiex_dbg(priv->adapter, ERROR, "Pending auth on going\n");
> diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
> index 42c04bf858da..6512562c9fb4 100644
> --- a/drivers/net/wireless/marvell/mwifiex/util.c
> +++ b/drivers/net/wireless/marvell/mwifiex/util.c
> @@ -323,6 +323,50 @@ int mwifiex_debug_info_to_buffer(struct mwifiex_private *priv, char *buf,
>  	return p - buf;
>  }
>  
> +bool mwifiex_is_channel_setting_allowable(struct mwifiex_private *priv,
> +					  struct ieee80211_channel *check_chan)
> +{
> +	struct mwifiex_adapter *adapter = priv->adapter;
> +	int i;
> +	struct mwifiex_private *tmp_priv;
> +	u8 bss_role = GET_BSS_ROLE(priv);
> +	struct ieee80211_channel *set_chan;
> +
> +	for (i = 0; i < MWIFIEX_MAX_BSS_NUM; i++) {
> +		tmp_priv = adapter->priv[i];
> +		if (tmp_priv == priv)
> +			continue;
> +
> +		set_chan = NULL;
> +		if (bss_role == MWIFIEX_BSS_ROLE_STA) {
> +			if (GET_BSS_ROLE(tmp_priv) == MWIFIEX_BSS_ROLE_UAP &&
> +			    netif_carrier_ok(tmp_priv->netdev) &&
> +			    cfg80211_chandef_valid(&tmp_priv->bss_chandef))
> +				set_chan = tmp_priv->bss_chandef.chan;
> +		} else if (bss_role == MWIFIEX_BSS_ROLE_UAP) {
> +			struct mwifiex_current_bss_params *bss_params =
> +				&tmp_priv->curr_bss_params;
> +			int channel = bss_params->bss_descriptor.channel;
> +			enum nl80211_band band =
> +				mwifiex_band_to_radio_type(bss_params->band);
> +			int freq =
> +				ieee80211_channel_to_frequency(channel, band);
> +
> +			if (GET_BSS_ROLE(tmp_priv) == MWIFIEX_BSS_ROLE_STA &&
> +			    tmp_priv->media_connected)
> +				set_chan = ieee80211_get_channel(adapter->wiphy, freq);
> +		}
> +
> +		if (set_chan && !ieee80211_channel_equal(check_chan, set_chan)) {
> +			mwifiex_dbg(adapter, ERROR,
> +				    "AP/STA must run on the same channel\n");
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  static int
>  mwifiex_parse_mgmt_packet(struct mwifiex_private *priv, u8 *payload, u16 len,
>  			  struct rxpd *rx_pd)
> diff --git a/drivers/net/wireless/marvell/mwifiex/util.h b/drivers/net/wireless/marvell/mwifiex/util.h
> index 4699c505c0a0..16f092bb0823 100644
> --- a/drivers/net/wireless/marvell/mwifiex/util.h
> +++ b/drivers/net/wireless/marvell/mwifiex/util.h
> @@ -86,4 +86,17 @@ static inline void le16_unaligned_add_cpu(__le16 *var, u16 val)
>  	put_unaligned_le16(get_unaligned_le16(var) + val, var);
>  }
>  
> +/* Current firmware doesn't support AP and STA running on different
> + * channels simultaneously in normal mode.
> + * FW crash would occur in such case.
> + * This function is used to check if check_chan can be set to FW or not.
> + *
> + * Return:
> + * %true if check_chan can be set to FW without issues.
> + * %false there is already other channel is set to FW, setting of
> + * check_chan is not allowable.
> + */
> +bool mwifiex_is_channel_setting_allowable(struct mwifiex_private *priv,
> +					  struct ieee80211_channel *check_chan);
> +
>  #endif /* !_MWIFIEX_UTIL_H_ */
> 
> base-commit: ae98f5c9fd8ba84cd408b41faa77e65bf1b4cdfa
> -- 
> 2.34.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Monday, September 2, 2024 5:38 PM
> To: David Lin <yu-hao.lin@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>
> Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> different channel
>
> On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > Current firmware doesn't support AP and STA running on different
> > channels simultaneously.
>
> As mentioned here:
>
> https://lore.kern/
> el.org%2Fall%2FZtGnWC7SPHt7Vbbp%40pengutronix.de%2F&data=05%7C02%
> 7Cyu-hao.lin%40nxp.com%7Cce9b7d4e417c41113c7d08dccb32fc49%7C686ea
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638608667089710854%7CUnkn
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=nMZO565xCUO%2BwxD4tIfi
> w6cGyYrinaEsi7XLfqyxgXg%3D&reserved=0
>
> AP and STA can indeed have different channels when DRCS is enabled, so I
> think you have to check this in your patch.
>
> Maybe the same question here again: Wouldn't it make sense to enable DRCS
> by default?
>
> Sascha
>

I will look into DRCS support later after current tasks on hand.
This patch is a quick fix to avoid firmware crash in the specific scenario.

David

Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[Sascha Hauer]

On Mon, Sep 02, 2024 at 10:35:01AM +0000, David Lin wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Sent: Monday, September 2, 2024 5:38 PM
> > To: David Lin <yu-hao.lin@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>
> > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> > different channel
> >
> > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > Current firmware doesn't support AP and STA running on different
> > > channels simultaneously.
> >
> > As mentioned here:
> >
> > https://lore.kern/
> > el.org%2Fall%2FZtGnWC7SPHt7Vbbp%40pengutronix.de%2F&data=05%7C02%
> > 7Cyu-hao.lin%40nxp.com%7Cce9b7d4e417c41113c7d08dccb32fc49%7C686ea
> > 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638608667089710854%7CUnkn
> > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> > aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=nMZO565xCUO%2BwxD4tIfi
> > w6cGyYrinaEsi7XLfqyxgXg%3D&reserved=0
> >
> > AP and STA can indeed have different channels when DRCS is enabled, so I
> > think you have to check this in your patch.
> >
> > Maybe the same question here again: Wouldn't it make sense to enable DRCS
> > by default?
> >
> > Sascha
> >
> 
> I will look into DRCS support later after current tasks on hand.
> This patch is a quick fix to avoid firmware crash in the specific scenario.

With DRCS support enabled AP and STA actually can run on different
channels with the current code. You are breaking this scenario with this
patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, September 10, 2024 4:33 AM
> To: David Lin <yu-hao.lin@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>
> Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> different channel
> 
> 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
> 
> 
> On Mon, Sep 02, 2024 at 10:35:01AM +0000, David Lin wrote:
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > Sent: Monday, September 2, 2024 5:38 PM
> > > To: David Lin <yu-hao.lin@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>
> > > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > running on different channel
> > >
> > > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > > Current firmware doesn't support AP and STA running on different
> > > > channels simultaneously.
> > >
> > > As mentioned here:
> > >
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > >
> re.kern%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7C7712df39ac37414fd
> a7
> > >
> e08dcd10eac35%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6386
> 15108
> > >
> 157502805%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> 2luMz
> > >
> IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=URNJPJE17iRY
> Tu4i
> > > rx7eQAC97tE5OE6a4kUfjUwuaVU%3D&reserved=0
> > >
> el.org%2Fall%2FZtGnWC7SPHt7Vbbp%40pengutronix.de%2F&data=05%7C02%
> > >
> 7Cyu-hao.lin%40nxp.com%7Cce9b7d4e417c41113c7d08dccb32fc49%7C686ea
> > >
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638608667089710854%7CUnkn
> > >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> > >
> aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=nMZO565xCUO%2BwxD4tIfi
> > > w6cGyYrinaEsi7XLfqyxgXg%3D&reserved=0
> > >
> > > AP and STA can indeed have different channels when DRCS is enabled,
> > > so I think you have to check this in your patch.
> > >
> > > Maybe the same question here again: Wouldn't it make sense to enable
> > > DRCS by default?
> > >
> > > Sascha
> > >
> >
> > I will look into DRCS support later after current tasks on hand.
> > This patch is a quick fix to avoid firmware crash in the specific scenario.
> 
> With DRCS support enabled AP and STA actually can run on different channels
> with the current code. You are breaking this scenario with this patch.
> 
> Sascha
> 

DRCS will be checked in the future.

David

Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[Sascha Hauer]

On Tue, Sep 10, 2024 at 01:55:14AM +0000, David Lin wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Sent: Tuesday, September 10, 2024 4:33 AM
> > To: David Lin <yu-hao.lin@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>
> > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> > different channel
> > 
> > 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
> > 
> > 
> > On Mon, Sep 02, 2024 at 10:35:01AM +0000, David Lin wrote:
> > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Sent: Monday, September 2, 2024 5:38 PM
> > > > To: David Lin <yu-hao.lin@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>
> > > > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > > running on different channel
> > > >
> > > > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > > > Current firmware doesn't support AP and STA running on different
> > > > > channels simultaneously.
> > > >
> > > > As mentioned here:
> > > >
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > > >
> > re.kern%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7C7712df39ac37414fd
> > a7
> > > >
> > e08dcd10eac35%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6386
> > 15108
> > > >
> > 157502805%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> > 2luMz
> > > >
> > IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=URNJPJE17iRY
> > Tu4i
> > > > rx7eQAC97tE5OE6a4kUfjUwuaVU%3D&reserved=0
> > > >
> > el.org%2Fall%2FZtGnWC7SPHt7Vbbp%40pengutronix.de%2F&data=05%7C02%
> > > >
> > 7Cyu-hao.lin%40nxp.com%7Cce9b7d4e417c41113c7d08dccb32fc49%7C686ea
> > > >
> > 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638608667089710854%7CUnkn
> > > >
> > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> > > >
> > aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=nMZO565xCUO%2BwxD4tIfi
> > > > w6cGyYrinaEsi7XLfqyxgXg%3D&reserved=0
> > > >
> > > > AP and STA can indeed have different channels when DRCS is enabled,
> > > > so I think you have to check this in your patch.
> > > >
> > > > Maybe the same question here again: Wouldn't it make sense to enable
> > > > DRCS by default?
> > > >
> > > > Sascha
> > > >
> > >
> > > I will look into DRCS support later after current tasks on hand.
> > > This patch is a quick fix to avoid firmware crash in the specific scenario.
> > 
> > With DRCS support enabled AP and STA actually can run on different channels
> > with the current code. You are breaking this scenario with this patch.
> > 
> > Sascha
> > 
> 
> DRCS will be checked in the future.

By future you mean v3 of this patch?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, September 10, 2024 1:56 PM
> To: David Lin <yu-hao.lin@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>
> Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> different channel
> 
> 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
> 
> 
> On Tue, Sep 10, 2024 at 01:55:14AM +0000, David Lin wrote:
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > Sent: Tuesday, September 10, 2024 4:33 AM
> > > To: David Lin <yu-hao.lin@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>
> > > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > running on different channel
> > >
> > > 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
> > >
> > >
> > > On Mon, Sep 02, 2024 at 10:35:01AM +0000, David Lin wrote:
> > > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > Sent: Monday, September 2, 2024 5:38 PM
> > > > > To: David Lin <yu-hao.lin@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>
> > > > > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > > > running on different channel
> > > > >
> > > > > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > > > > Current firmware doesn't support AP and STA running on
> > > > > > different channels simultaneously.
> > > > >
> > > > > As mentioned here:
> > > > >
> > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > >
> 2Flo%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7Cd0a1e3797a0d4acd7a
> > > > >
> 9c08dcd15d3ddb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> 6
> > > > >
> 15445620612026%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QI
> > > > >
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=5yC
> > > > > jjIqxpV%2BJxlPdrCH0gRTQ%2FCQZKr%2FDc0S1cjkaIA8%3D&reserved=0
> > > > >
> > >
> re.kern%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7C7712df39ac37414fd
> > > a7
> > > > >
> > >
> e08dcd10eac35%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6386
> > > 15108
> > > > >
> > >
> 157502805%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> > > 2luMz
> > > > >
> > >
> IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=URNJPJE17iRY
> > > Tu4i
> > > > > rx7eQAC97tE5OE6a4kUfjUwuaVU%3D&reserved=0
> > > > >
> > >
> el.org%2Fall%2FZtGnWC7SPHt7Vbbp%40pengutronix.de%2F&data=05%7C02%
> > > > >
> > >
> 7Cyu-hao.lin%40nxp.com%7Cce9b7d4e417c41113c7d08dccb32fc49%7C686ea
> > > > >
> > >
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638608667089710854%7CUnkn
> > > > >
> > >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> > > > >
> > >
> aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=nMZO565xCUO%2BwxD4tIfi
> > > > > w6cGyYrinaEsi7XLfqyxgXg%3D&reserved=0
> > > > >
> > > > > AP and STA can indeed have different channels when DRCS is
> > > > > enabled, so I think you have to check this in your patch.
> > > > >
> > > > > Maybe the same question here again: Wouldn't it make sense to
> > > > > enable DRCS by default?
> > > > >
> > > > > Sascha
> > > > >
> > > >
> > > > I will look into DRCS support later after current tasks on hand.
> > > > This patch is a quick fix to avoid firmware crash in the specific scenario.
> > >
> > > With DRCS support enabled AP and STA actually can run on different
> > > channels with the current code. You are breaking this scenario with this
> patch.
> > >
> > > Sascha
> > >
> >
> > DRCS will be checked in the future.
> 
> By future you mean v3 of this patch?
> 
> Sascha
> 

No schedule now.

David

Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[Sascha Hauer]

On Tue, Sep 10, 2024 at 05:56:58AM +0000, David Lin wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Sent: Tuesday, September 10, 2024 1:56 PM
> > To: David Lin <yu-hao.lin@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>
> > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> > different channel
> > 
> > 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
> > 
> > 
> > On Tue, Sep 10, 2024 at 01:55:14AM +0000, David Lin wrote:
> > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Sent: Tuesday, September 10, 2024 4:33 AM
> > > > To: David Lin <yu-hao.lin@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>
> > > > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > > running on different channel
> > > >
> > > > 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
> > > >
> > > >
> > > > On Mon, Sep 02, 2024 at 10:35:01AM +0000, David Lin wrote:
> > > > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > Sent: Monday, September 2, 2024 5:38 PM
> > > > > > To: David Lin <yu-hao.lin@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>
> > > > > > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > > > > running on different channel
> > > > > >
> > > > > > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > > > > > Current firmware doesn't support AP and STA running on
> > > > > > > different channels simultaneously.
> > > > > >
> > > > > > As mentioned here:
> > > > > >
> > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > > >
> > 2Flo%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7Cd0a1e3797a0d4acd7a
> > > > > >
> > 9c08dcd15d3ddb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> > 6
> > > > > >
> > 15445620612026%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > QI
> > > > > >
> > joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=5yC
> > > > > > jjIqxpV%2BJxlPdrCH0gRTQ%2FCQZKr%2FDc0S1cjkaIA8%3D&reserved=0
> > > > > >
> > > >
> > re.kern%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7C7712df39ac37414fd
> > > > a7
> > > > > >
> > > >
> > e08dcd10eac35%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6386
> > > > 15108
> > > > > >
> > > >
> > 157502805%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> > > > 2luMz
> > > > > >
> > > >
> > IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=URNJPJE17iRY
> > > > Tu4i
> > > > > > rx7eQAC97tE5OE6a4kUfjUwuaVU%3D&reserved=0
> > > > > >
> > > >
> > el.org%2Fall%2FZtGnWC7SPHt7Vbbp%40pengutronix.de%2F&data=05%7C02%
> > > > > >
> > > >
> > 7Cyu-hao.lin%40nxp.com%7Cce9b7d4e417c41113c7d08dccb32fc49%7C686ea
> > > > > >
> > > >
> > 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638608667089710854%7CUnkn
> > > > > >
> > > >
> > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> > > > > >
> > > >
> > aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=nMZO565xCUO%2BwxD4tIfi
> > > > > > w6cGyYrinaEsi7XLfqyxgXg%3D&reserved=0
> > > > > >
> > > > > > AP and STA can indeed have different channels when DRCS is
> > > > > > enabled, so I think you have to check this in your patch.
> > > > > >
> > > > > > Maybe the same question here again: Wouldn't it make sense to
> > > > > > enable DRCS by default?
> > > > > >
> > > > > > Sascha
> > > > > >
> > > > >
> > > > > I will look into DRCS support later after current tasks on hand.
> > > > > This patch is a quick fix to avoid firmware crash in the specific scenario.
> > > >
> > > > With DRCS support enabled AP and STA actually can run on different
> > > > channels with the current code. You are breaking this scenario with this
> > patch.
> > > >
> > > > Sascha
> > > >
> > >
> > > DRCS will be checked in the future.
> > 
> > By future you mean v3 of this patch?
> > 
> > Sascha
> > 
> 
> No schedule now.

I am getting confused now. You want us to abandon my patch in favour of
yours, but you have no plans to update your patch to avoid a regression
that you introduce with your patch?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, September 10, 2024 2:07 PM
> To: David Lin <yu-hao.lin@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>
> Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> different channel
> 
> 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
> 
> 
> On Tue, Sep 10, 2024 at 05:56:58AM +0000, David Lin wrote:
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > Sent: Tuesday, September 10, 2024 1:56 PM
> > > To: David Lin <yu-hao.lin@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>
> > > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > running on different channel
> > >
> > > 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
> > >
> > >
> > > On Tue, Sep 10, 2024 at 01:55:14AM +0000, David Lin wrote:
> > > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > Sent: Tuesday, September 10, 2024 4:33 AM
> > > > > To: David Lin <yu-hao.lin@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>
> > > > > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and
> > > > > STA running on different channel
> > > > >
> > > > > 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
> > > > >
> > > > >
> > > > > On Mon, Sep 02, 2024 at 10:35:01AM +0000, David Lin wrote:
> > > > > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > > Sent: Monday, September 2, 2024 5:38 PM
> > > > > > > To: David Lin <yu-hao.lin@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>
> > > > > > > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and
> > > > > > > STA running on different channel
> > > > > > >
> > > > > > > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > > > > > > Current firmware doesn't support AP and STA running on
> > > > > > > > different channels simultaneously.
> > > > > > >
> > > > > > > As mentioned here:
> > > > > > >
> > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A
> > > > > > > %2F%
> > > > > > >
> > >
> 2Flo%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7Cd0a1e3797a0d4acd7a
> > > > > > >
> > >
> 9c08dcd15d3ddb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> > > 6
> > > > > > >
> > >
> 15445620612026%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > QI
> > > > > > >
> > >
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=5yC
> > > > > > >
> jjIqxpV%2BJxlPdrCH0gRTQ%2FCQZKr%2FDc0S1cjkaIA8%3D&reserved=0
> > > > > > >
> > > > >
> > >
> re.kern%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7C7712df39ac37414fd
> > > > > a7
> > > > > > >
> > > > >
> > >
> e08dcd10eac35%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6386
> > > > > 15108
> > > > > > >
> > > > >
> > >
> 157502805%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> > > > > 2luMz
> > > > > > >
> > > > >
> > >
> IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=URNJPJE17iRY
> > > > > Tu4i
> > > > > > > rx7eQAC97tE5OE6a4kUfjUwuaVU%3D&reserved=0
> > > > > > >
> > > > >
> > >
> el.org%2Fall%2FZtGnWC7SPHt7Vbbp%40pengutronix.de%2F&data=05%7C02%
> > > > > > >
> > > > >
> > >
> 7Cyu-hao.lin%40nxp.com%7Cce9b7d4e417c41113c7d08dccb32fc49%7C686ea
> > > > > > >
> > > > >
> > >
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638608667089710854%7CUnkn
> > > > > > >
> > > > >
> > >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> > > > > > >
> > > > >
> > >
> aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=nMZO565xCUO%2BwxD4tIfi
> > > > > > > w6cGyYrinaEsi7XLfqyxgXg%3D&reserved=0
> > > > > > >
> > > > > > > AP and STA can indeed have different channels when DRCS is
> > > > > > > enabled, so I think you have to check this in your patch.
> > > > > > >
> > > > > > > Maybe the same question here again: Wouldn't it make sense
> > > > > > > to enable DRCS by default?
> > > > > > >
> > > > > > > Sascha
> > > > > > >
> > > > > >
> > > > > > I will look into DRCS support later after current tasks on hand.
> > > > > > This patch is a quick fix to avoid firmware crash in the specific
> scenario.
> > > > >
> > > > > With DRCS support enabled AP and STA actually can run on
> > > > > different channels with the current code. You are breaking this
> > > > > scenario with this
> > > patch.
> > > > >
> > > > > Sascha
> > > > >
> > > >
> > > > DRCS will be checked in the future.
> > >
> > > By future you mean v3 of this patch?
> > >
> > > Sascha
> > >
> >
> > No schedule now.
> 
> I am getting confused now. You want us to abandon my patch in favour of yours,
> but you have no plans to update your patch to avoid a regression that you
> introduce with your patch?
> 
> Sascha
> 

My patch resolves the same issue as your patch. But your patch can't let AP and STA run on the same channel if some wiphy parameters are set.

I wonder did you test your patch?

David

Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[Sascha Hauer]

On Tue, Sep 10, 2024 at 06:18:57AM +0000, David Lin wrote:
> > > > > > With DRCS support enabled AP and STA actually can run on
> > > > > > different channels with the current code. You are breaking this
> > > > > > scenario with this
> > > > patch.
> > > > > >
> > > > > > Sascha
> > > > > >
> > > > >
> > > > > DRCS will be checked in the future.
> > > >
> > > > By future you mean v3 of this patch?
> > > >
> > > > Sascha
> > > >
> > >
> > > No schedule now.
> > 
> > I am getting confused now. You want us to abandon my patch in favour of yours,
> > but you have no plans to update your patch to avoid a regression that you
> > introduce with your patch?
> > 
> > Sascha
> > 
> 
> My patch resolves the same issue as your patch. But your patch can't
> let AP and STA run on the same channel if some wiphy parameters are
> set.
> 
> I wonder did you test your patch?

I finally see what you mean with "some wiphy parameters are set".
I did test my patch and I didn't run into this issue, because I haven't
set anything like rts_threshold in my config.

Nevertheless what I am trying to tell you in this thread is: Your patch
introduces a regression and needs an update.

It's not about my patch or your patch, both are currently not suitable
for inclusion and the question is: will you update your patch?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, September 10, 2024 3:22 PM
> To: David Lin <yu-hao.lin@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>
> Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> different channel
> 
> 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
> 
> 
> On Tue, Sep 10, 2024 at 06:18:57AM +0000, David Lin wrote:
> > > > > > > With DRCS support enabled AP and STA actually can run on
> > > > > > > different channels with the current code. You are breaking
> > > > > > > this scenario with this
> > > > > patch.
> > > > > > >
> > > > > > > Sascha
> > > > > > >
> > > > > >
> > > > > > DRCS will be checked in the future.
> > > > >
> > > > > By future you mean v3 of this patch?
> > > > >
> > > > > Sascha
> > > > >
> > > >
> > > > No schedule now.
> > >
> > > I am getting confused now. You want us to abandon my patch in favour
> > > of yours, but you have no plans to update your patch to avoid a
> > > regression that you introduce with your patch?
> > >
> > > Sascha
> > >
> >
> > My patch resolves the same issue as your patch. But your patch can't
> > let AP and STA run on the same channel if some wiphy parameters are
> > set.
> >
> > I wonder did you test your patch?
> 
> I finally see what you mean with "some wiphy parameters are set".
> I did test my patch and I didn't run into this issue, because I haven't set
> anything like rts_threshold in my config.
> 
> Nevertheless what I am trying to tell you in this thread is: Your patch
> introduces a regression and needs an update.
> 
> It's not about my patch or your patch, both are currently not suitable for
> inclusion and the question is: will you update your patch?
> 
> Sascha
> 

Without DRCS, this patch can avoid firmware crash and this is the current plan for Mwifiex.
It won't affect any existing features.

David

Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[Sascha Hauer]

On Tue, Sep 10, 2024 at 07:28:29AM +0000, David Lin wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Sent: Tuesday, September 10, 2024 3:22 PM
> > To: David Lin <yu-hao.lin@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>
> > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> > different channel
> > 
> > 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
> > 
> > 
> > On Tue, Sep 10, 2024 at 06:18:57AM +0000, David Lin wrote:
> > > > > > > > With DRCS support enabled AP and STA actually can run on
> > > > > > > > different channels with the current code. You are breaking
> > > > > > > > this scenario with this
> > > > > > patch.
> > > > > > > >
> > > > > > > > Sascha
> > > > > > > >
> > > > > > >
> > > > > > > DRCS will be checked in the future.
> > > > > >
> > > > > > By future you mean v3 of this patch?
> > > > > >
> > > > > > Sascha
> > > > > >
> > > > >
> > > > > No schedule now.
> > > >
> > > > I am getting confused now. You want us to abandon my patch in favour
> > > > of yours, but you have no plans to update your patch to avoid a
> > > > regression that you introduce with your patch?
> > > >
> > > > Sascha
> > > >
> > >
> > > My patch resolves the same issue as your patch. But your patch can't
> > > let AP and STA run on the same channel if some wiphy parameters are
> > > set.
> > >
> > > I wonder did you test your patch?
> > 
> > I finally see what you mean with "some wiphy parameters are set".
> > I did test my patch and I didn't run into this issue, because I haven't set
> > anything like rts_threshold in my config.
> > 
> > Nevertheless what I am trying to tell you in this thread is: Your patch
> > introduces a regression and needs an update.
> > 
> > It's not about my patch or your patch, both are currently not suitable for
> > inclusion and the question is: will you update your patch?
> > 
> > Sascha
> > 
> 
> Without DRCS, this patch can avoid firmware crash and this is the
> current plan for Mwifiex.  It won't affect any existing features.

What I am trying to tell you: Your patch fixes one thing and breaks
another.

The current mwifiex driver *does* support DRCS when enabled with the
drcs=1 module parameter. With DRCS enabled the current driver *can*
successfully run an Accesspoint on one channel and a station on another
channel. You are breaking this with your patch.

Your mwifiex_is_channel_setting_allowable() needs this:

        if (adapter->drcs_enabled)
		return true;

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, September 10, 2024 4:08 PM
> To: David Lin <yu-hao.lin@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>
> Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> different channel
> 
> 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
> 
> 
> On Tue, Sep 10, 2024 at 07:28:29AM +0000, David Lin wrote:
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > Sent: Tuesday, September 10, 2024 3:22 PM
> > > To: David Lin <yu-hao.lin@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>
> > > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > running on different channel
> > >
> > > 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
> > >
> > >
> > > On Tue, Sep 10, 2024 at 06:18:57AM +0000, David Lin wrote:
> > > > > > > > > With DRCS support enabled AP and STA actually can run on
> > > > > > > > > different channels with the current code. You are
> > > > > > > > > breaking this scenario with this
> > > > > > > patch.
> > > > > > > > >
> > > > > > > > > Sascha
> > > > > > > > >
> > > > > > > >
> > > > > > > > DRCS will be checked in the future.
> > > > > > >
> > > > > > > By future you mean v3 of this patch?
> > > > > > >
> > > > > > > Sascha
> > > > > > >
> > > > > >
> > > > > > No schedule now.
> > > > >
> > > > > I am getting confused now. You want us to abandon my patch in
> > > > > favour of yours, but you have no plans to update your patch to
> > > > > avoid a regression that you introduce with your patch?
> > > > >
> > > > > Sascha
> > > > >
> > > >
> > > > My patch resolves the same issue as your patch. But your patch
> > > > can't let AP and STA run on the same channel if some wiphy
> > > > parameters are set.
> > > >
> > > > I wonder did you test your patch?
> > >
> > > I finally see what you mean with "some wiphy parameters are set".
> > > I did test my patch and I didn't run into this issue, because I
> > > haven't set anything like rts_threshold in my config.
> > >
> > > Nevertheless what I am trying to tell you in this thread is: Your
> > > patch introduces a regression and needs an update.
> > >
> > > It's not about my patch or your patch, both are currently not
> > > suitable for inclusion and the question is: will you update your patch?
> > >
> > > Sascha
> > >
> >
> > Without DRCS, this patch can avoid firmware crash and this is the
> > current plan for Mwifiex.  It won't affect any existing features.
> 
> What I am trying to tell you: Your patch fixes one thing and breaks another.
> 
> The current mwifiex driver *does* support DRCS when enabled with the
> drcs=1 module parameter. With DRCS enabled the current driver *can*
> successfully run an Accesspoint on one channel and a station on another
> channel. You are breaking this with your patch.
> 
> Your mwifiex_is_channel_setting_allowable() needs this:
> 
>         if (adapter->drcs_enabled)
>                 return true;
> 
> Sascha
> 

Thanks. Because the code is ported from nxpwifi which removes DRCS. I will add this check in patch v3.

David

Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[Sascha Hauer]

On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> Current firmware doesn't support AP and STA running on different
> channels simultaneously.
> FW crash would occur in such case.
> This patch avoids the issue by disabling AP and STA to run on
> different channels.
> 
> Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> ---
> 
> v2:
>    - clean up code.
> 
> ---
>  .../net/wireless/marvell/mwifiex/cfg80211.c   | 17 ++++---
>  drivers/net/wireless/marvell/mwifiex/util.c   | 44 +++++++++++++++++++
>  drivers/net/wireless/marvell/mwifiex/util.h   | 13 ++++++
>  3 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 722ead51e912..3dbcab463445 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -781,11 +781,9 @@ mwifiex_cfg80211_set_wiphy_params(struct wiphy *wiphy, u32 changed)
>  		break;
>  
>  	case MWIFIEX_BSS_ROLE_STA:
> -		if (priv->media_connected) {
> -			mwifiex_dbg(adapter, ERROR,
> -				    "cannot change wiphy params when connected");
> -			return -EINVAL;
> -		}
> +		if (priv->media_connected)
> +			break;

This hunk seems unrelated to this patch. If this is needed then it
deserves an extra patch along with an explanation why this is necessary.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, September 10, 2024 5:05 AM
> To: David Lin <yu-hao.lin@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>
> Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> different channel
> 
> 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
> 
> 
> On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > Current firmware doesn't support AP and STA running on different
> > channels simultaneously.
> > FW crash would occur in such case.
> > This patch avoids the issue by disabling AP and STA to run on
> > different channels.
> >
> > Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> > ---
> >
> > v2:
> >    - clean up code.
> >
> > ---
> >  .../net/wireless/marvell/mwifiex/cfg80211.c   | 17 ++++---
> >  drivers/net/wireless/marvell/mwifiex/util.c   | 44 +++++++++++++++++++
> >  drivers/net/wireless/marvell/mwifiex/util.h   | 13 ++++++
> >  3 files changed, 69 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > index 722ead51e912..3dbcab463445 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > @@ -781,11 +781,9 @@ mwifiex_cfg80211_set_wiphy_params(struct wiphy
> *wiphy, u32 changed)
> >               break;
> >
> >       case MWIFIEX_BSS_ROLE_STA:
> > -             if (priv->media_connected) {
> > -                     mwifiex_dbg(adapter, ERROR,
> > -                                 "cannot change wiphy params
> when connected");
> > -                     return -EINVAL;
> > -             }
> > +             if (priv->media_connected)
> > +                     break;
> 
> This hunk seems unrelated to this patch. If this is needed then it deserves an
> extra patch along with an explanation why this is necessary.
> 
> Sascha
> 

Without this hunk, AP and STA can't run on the same channel if some wiphy parameters are setting.

David

Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[Sascha Hauer]

On Tue, Sep 10, 2024 at 01:52:02AM +0000, David Lin wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Sent: Tuesday, September 10, 2024 5:05 AM
> > To: David Lin <yu-hao.lin@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>
> > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> > different channel
> > 
> > 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
> > 
> > 
> > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > Current firmware doesn't support AP and STA running on different
> > > channels simultaneously.
> > > FW crash would occur in such case.
> > > This patch avoids the issue by disabling AP and STA to run on
> > > different channels.
> > >
> > > Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> > > ---
> > >
> > > v2:
> > >    - clean up code.
> > >
> > > ---
> > >  .../net/wireless/marvell/mwifiex/cfg80211.c   | 17 ++++---
> > >  drivers/net/wireless/marvell/mwifiex/util.c   | 44 +++++++++++++++++++
> > >  drivers/net/wireless/marvell/mwifiex/util.h   | 13 ++++++
> > >  3 files changed, 69 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > index 722ead51e912..3dbcab463445 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > @@ -781,11 +781,9 @@ mwifiex_cfg80211_set_wiphy_params(struct wiphy
> > *wiphy, u32 changed)
> > >               break;
> > >
> > >       case MWIFIEX_BSS_ROLE_STA:
> > > -             if (priv->media_connected) {
> > > -                     mwifiex_dbg(adapter, ERROR,
> > > -                                 "cannot change wiphy params
> > when connected");
> > > -                     return -EINVAL;
> > > -             }
> > > +             if (priv->media_connected)
> > > +                     break;
> > 
> > This hunk seems unrelated to this patch. If this is needed then it deserves an
> > extra patch along with an explanation why this is necessary.
> > 
> > Sascha
> > 
> 
> Without this hunk, AP and STA can't run on the same channel if some
> wiphy parameters are setting.

Ok, I now see where you are aiming at. Here's the problematic function:

> static int
> mwifiex_cfg80211_set_wiphy_params(struct wiphy *wiphy, u32 changed)
> {
> 	...
> 
> 	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> 
> 	switch (priv->bss_role) {
> 	case MWIFIEX_BSS_ROLE_UAP:
> 		if (priv->bss_started) {
> 			mwifiex_dbg(adapter, ERROR,
> 				    "cannot change wiphy params when bss started");
> 			return -EINVAL;
> 		}
> 
> 		...
> 		mwifiex_send_cmd(priv, HostCmd_CMD_UAP_SYS_CONFIG, ...);
> 
> 		break;
> 	case MWIFIEX_BSS_ROLE_STA:
> 		if (priv->media_connected) {
> 			mwifiex_dbg(adapter, ERROR,
> 				    "cannot change wiphy params when connected");
> 			return -EINVAL;
> 		}
> 
> 		...
> 		mwifiex_send_cmd(priv, HostCmd_CMD_802_11_SNMP_MIB, ...);
> 
> 		break;
> 	}
> 
> 	return 0;
> }

This function is for setting wiphy params like rts_threshold and others.

mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY) returns the first
priv which by default is in station mode. Now if you start priv0 in
station mode, then afterwards start priv1 in AP mode *and* have
rts_threshold = xy in your config, then you run into the
"cannot change wiphy params when connected" case.

I really wonder if the settings done in this function are per priv or
per adapter. Is there one rts_threshold setting in a mwifiex chip or are
there multiple (per vif/priv)?

If it's a global setting, then why are we interested in the
media_connected state of one specific priv? Shouldn't we check all
privs?

If it's a setting per priv, then why do we choose the same priv
everytime in this function?

Either way, this function looks fishy and changing it should be done
with an explanation, just dropping the error message and returning
success is not enough.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, September 10, 2024 4:00 PM
> To: David Lin <yu-hao.lin@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>
> Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> different channel
> 
> 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
> 
> 
> On Tue, Sep 10, 2024 at 01:52:02AM +0000, David Lin wrote:
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > Sent: Tuesday, September 10, 2024 5:05 AM
> > > To: David Lin <yu-hao.lin@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>
> > > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > running on different channel
> > >
> > > 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
> > >
> > >
> > > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > > Current firmware doesn't support AP and STA running on different
> > > > channels simultaneously.
> > > > FW crash would occur in such case.
> > > > This patch avoids the issue by disabling AP and STA to run on
> > > > different channels.
> > > >
> > > > Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> > > > ---
> > > >
> > > > v2:
> > > >    - clean up code.
> > > >
> > > > ---
> > > >  .../net/wireless/marvell/mwifiex/cfg80211.c   | 17 ++++---
> > > >  drivers/net/wireless/marvell/mwifiex/util.c   | 44
> +++++++++++++++++++
> > > >  drivers/net/wireless/marvell/mwifiex/util.h   | 13 ++++++
> > > >  3 files changed, 69 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > index 722ead51e912..3dbcab463445 100644
> > > > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > @@ -781,11 +781,9 @@ mwifiex_cfg80211_set_wiphy_params(struct
> > > > wiphy
> > > *wiphy, u32 changed)
> > > >               break;
> > > >
> > > >       case MWIFIEX_BSS_ROLE_STA:
> > > > -             if (priv->media_connected) {
> > > > -                     mwifiex_dbg(adapter, ERROR,
> > > > -                                 "cannot change wiphy params
> > > when connected");
> > > > -                     return -EINVAL;
> > > > -             }
> > > > +             if (priv->media_connected)
> > > > +                     break;
> > >
> > > This hunk seems unrelated to this patch. If this is needed then it
> > > deserves an extra patch along with an explanation why this is necessary.
> > >
> > > Sascha
> > >
> >
> > Without this hunk, AP and STA can't run on the same channel if some
> > wiphy parameters are setting.
> 
> Ok, I now see where you are aiming at. Here's the problematic function:
> 
> > static int
> > mwifiex_cfg80211_set_wiphy_params(struct wiphy *wiphy, u32 changed) {
> >       ...
> >
> >       priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> >
> >       switch (priv->bss_role) {
> >       case MWIFIEX_BSS_ROLE_UAP:
> >               if (priv->bss_started) {
> >                       mwifiex_dbg(adapter, ERROR,
> >                                   "cannot change wiphy params
> when bss started");
> >                       return -EINVAL;
> >               }
> >
> >               ...
> >               mwifiex_send_cmd(priv,
> HostCmd_CMD_UAP_SYS_CONFIG, ...);
> >
> >               break;
> >       case MWIFIEX_BSS_ROLE_STA:
> >               if (priv->media_connected) {
> >                       mwifiex_dbg(adapter, ERROR,
> >                                   "cannot change wiphy params
> when connected");
> >                       return -EINVAL;
> >               }
> >
> >               ...
> >               mwifiex_send_cmd(priv,
> HostCmd_CMD_802_11_SNMP_MIB,
> > ...);
> >
> >               break;
> >       }
> >
> >       return 0;
> > }
> 
> This function is for setting wiphy params like rts_threshold and others.
> 
> mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY) returns the first priv
> which by default is in station mode. Now if you start priv0 in station mode,
> then afterwards start priv1 in AP mode *and* have rts_threshold = xy in your
> config, then you run into the "cannot change wiphy params when connected"
> case.
> 
> I really wonder if the settings done in this function are per priv or per adapter.
> Is there one rts_threshold setting in a mwifiex chip or are there multiple (per
> vif/priv)?
> 
> If it's a global setting, then why are we interested in the media_connected
> state of one specific priv? Shouldn't we check all privs?
> 
> If it's a setting per priv, then why do we choose the same priv everytime in this
> function?
> 
> Either way, this function looks fishy and changing it should be done with an
> explanation, just dropping the error message and returning success is not
> enough.
> 
> Sascha
> 
> --

O.K. I will add comment in patch v3.

David

Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[Sascha Hauer]

On Tue, Sep 10, 2024 at 08:19:59AM +0000, David Lin wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Sent: Tuesday, September 10, 2024 4:00 PM
> > To: David Lin <yu-hao.lin@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>
> > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> > different channel
> > 
> > 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
> > 
> > 
> > On Tue, Sep 10, 2024 at 01:52:02AM +0000, David Lin wrote:
> > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Sent: Tuesday, September 10, 2024 5:05 AM
> > > > To: David Lin <yu-hao.lin@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>
> > > > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > > running on different channel
> > > >
> > > > 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
> > > >
> > > >
> > > > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > > > Current firmware doesn't support AP and STA running on different
> > > > > channels simultaneously.
> > > > > FW crash would occur in such case.
> > > > > This patch avoids the issue by disabling AP and STA to run on
> > > > > different channels.
> > > > >
> > > > > Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> > > > > ---
> > > > >
> > > > > v2:
> > > > >    - clean up code.
> > > > >
> > > > > ---
> > > > >  .../net/wireless/marvell/mwifiex/cfg80211.c   | 17 ++++---
> > > > >  drivers/net/wireless/marvell/mwifiex/util.c   | 44
> > +++++++++++++++++++
> > > > >  drivers/net/wireless/marvell/mwifiex/util.h   | 13 ++++++
> > > > >  3 files changed, 69 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > > index 722ead51e912..3dbcab463445 100644
> > > > > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > > @@ -781,11 +781,9 @@ mwifiex_cfg80211_set_wiphy_params(struct
> > > > > wiphy
> > > > *wiphy, u32 changed)
> > > > >               break;
> > > > >
> > > > >       case MWIFIEX_BSS_ROLE_STA:
> > > > > -             if (priv->media_connected) {
> > > > > -                     mwifiex_dbg(adapter, ERROR,
> > > > > -                                 "cannot change wiphy params
> > > > when connected");
> > > > > -                     return -EINVAL;
> > > > > -             }
> > > > > +             if (priv->media_connected)
> > > > > +                     break;
> > > >
> > > > This hunk seems unrelated to this patch. If this is needed then it
> > > > deserves an extra patch along with an explanation why this is necessary.
> > > >
> > > > Sascha
> > > >
> > >
> > > Without this hunk, AP and STA can't run on the same channel if some
> > > wiphy parameters are setting.
> > 
> > Ok, I now see where you are aiming at. Here's the problematic function:
> > 
> > > static int
> > > mwifiex_cfg80211_set_wiphy_params(struct wiphy *wiphy, u32 changed) {
> > >       ...
> > >
> > >       priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> > >
> > >       switch (priv->bss_role) {
> > >       case MWIFIEX_BSS_ROLE_UAP:
> > >               if (priv->bss_started) {
> > >                       mwifiex_dbg(adapter, ERROR,
> > >                                   "cannot change wiphy params
> > when bss started");
> > >                       return -EINVAL;
> > >               }
> > >
> > >               ...
> > >               mwifiex_send_cmd(priv,
> > HostCmd_CMD_UAP_SYS_CONFIG, ...);
> > >
> > >               break;
> > >       case MWIFIEX_BSS_ROLE_STA:
> > >               if (priv->media_connected) {
> > >                       mwifiex_dbg(adapter, ERROR,
> > >                                   "cannot change wiphy params
> > when connected");
> > >                       return -EINVAL;
> > >               }
> > >
> > >               ...
> > >               mwifiex_send_cmd(priv,
> > HostCmd_CMD_802_11_SNMP_MIB,
> > > ...);
> > >
> > >               break;
> > >       }
> > >
> > >       return 0;
> > > }
> > 
> > This function is for setting wiphy params like rts_threshold and others.
> > 
> > mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY) returns the first priv
> > which by default is in station mode. Now if you start priv0 in station mode,
> > then afterwards start priv1 in AP mode *and* have rts_threshold = xy in your
> > config, then you run into the "cannot change wiphy params when connected"
> > case.
> > 
> > I really wonder if the settings done in this function are per priv or per adapter.
> > Is there one rts_threshold setting in a mwifiex chip or are there multiple (per
> > vif/priv)?
> > 
> > If it's a global setting, then why are we interested in the media_connected
> > state of one specific priv? Shouldn't we check all privs?
> > 
> > If it's a setting per priv, then why do we choose the same priv everytime in this
> > function?
> > 
> > Either way, this function looks fishy and changing it should be done with an
> > explanation, just dropping the error message and returning success is not
> > enough.
> > 
> > Sascha
> > 
> > --
> 
> O.K. I will add comment in patch v3.

Really I hoped that you could clarify a bit how this function works and
answer some of the questions I raised above. That could help us to get
a better driver over time. The same code is in the nxpwifi driver as
well, so it's not only a matter of which driver we decide to maintain
in the future.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

[David Lin]

> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, September 10, 2024 4:32 PM
> To: David Lin <yu-hao.lin@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>
> Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> different channel
> 
> 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
> 
> 
> On Tue, Sep 10, 2024 at 08:19:59AM +0000, David Lin wrote:
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > Sent: Tuesday, September 10, 2024 4:00 PM
> > > To: David Lin <yu-hao.lin@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>
> > > Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > running on different channel
> > >
> > > 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
> > >
> > >
> > > On Tue, Sep 10, 2024 at 01:52:02AM +0000, David Lin wrote:
> > > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > Sent: Tuesday, September 10, 2024 5:05 AM
> > > > > To: David Lin <yu-hao.lin@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>
> > > > > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA
> > > > > running on different channel
> > > > >
> > > > > 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
> > > > >
> > > > >
> > > > > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > > > > Current firmware doesn't support AP and STA running on
> > > > > > different channels simultaneously.
> > > > > > FW crash would occur in such case.
> > > > > > This patch avoids the issue by disabling AP and STA to run on
> > > > > > different channels.
> > > > > >
> > > > > > Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> > > > > > ---
> > > > > >
> > > > > > v2:
> > > > > >    - clean up code.
> > > > > >
> > > > > > ---
> > > > > >  .../net/wireless/marvell/mwifiex/cfg80211.c   | 17 ++++---
> > > > > >  drivers/net/wireless/marvell/mwifiex/util.c   | 44
> > > +++++++++++++++++++
> > > > > >  drivers/net/wireless/marvell/mwifiex/util.h   | 13 ++++++
> > > > > >  3 files changed, 69 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > > > index 722ead51e912..3dbcab463445 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > > > > @@ -781,11 +781,9 @@
> mwifiex_cfg80211_set_wiphy_params(struct
> > > > > > wiphy
> > > > > *wiphy, u32 changed)
> > > > > >               break;
> > > > > >
> > > > > >       case MWIFIEX_BSS_ROLE_STA:
> > > > > > -             if (priv->media_connected) {
> > > > > > -                     mwifiex_dbg(adapter, ERROR,
> > > > > > -                                 "cannot change wiphy
> params
> > > > > when connected");
> > > > > > -                     return -EINVAL;
> > > > > > -             }
> > > > > > +             if (priv->media_connected)
> > > > > > +                     break;
> > > > >
> > > > > This hunk seems unrelated to this patch. If this is needed then
> > > > > it deserves an extra patch along with an explanation why this is
> necessary.
> > > > >
> > > > > Sascha
> > > > >
> > > >
> > > > Without this hunk, AP and STA can't run on the same channel if
> > > > some wiphy parameters are setting.
> > >
> > > Ok, I now see where you are aiming at. Here's the problematic function:
> > >
> > > > static int
> > > > mwifiex_cfg80211_set_wiphy_params(struct wiphy *wiphy, u32 changed)
> {
> > > >       ...
> > > >
> > > >       priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> > > >
> > > >       switch (priv->bss_role) {
> > > >       case MWIFIEX_BSS_ROLE_UAP:
> > > >               if (priv->bss_started) {
> > > >                       mwifiex_dbg(adapter, ERROR,
> > > >                                   "cannot change wiphy params
> > > when bss started");
> > > >                       return -EINVAL;
> > > >               }
> > > >
> > > >               ...
> > > >               mwifiex_send_cmd(priv,
> > > HostCmd_CMD_UAP_SYS_CONFIG, ...);
> > > >
> > > >               break;
> > > >       case MWIFIEX_BSS_ROLE_STA:
> > > >               if (priv->media_connected) {
> > > >                       mwifiex_dbg(adapter, ERROR,
> > > >                                   "cannot change wiphy params
> > > when connected");
> > > >                       return -EINVAL;
> > > >               }
> > > >
> > > >               ...
> > > >               mwifiex_send_cmd(priv,
> > > HostCmd_CMD_802_11_SNMP_MIB,
> > > > ...);
> > > >
> > > >               break;
> > > >       }
> > > >
> > > >       return 0;
> > > > }
> > >
> > > This function is for setting wiphy params like rts_threshold and others.
> > >
> > > mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY) returns the first
> > > priv which by default is in station mode. Now if you start priv0 in
> > > station mode, then afterwards start priv1 in AP mode *and* have
> > > rts_threshold = xy in your config, then you run into the "cannot change
> wiphy params when connected"
> > > case.
> > >
> > > I really wonder if the settings done in this function are per priv or per
> adapter.
> > > Is there one rts_threshold setting in a mwifiex chip or are there
> > > multiple (per vif/priv)?
> > >
> > > If it's a global setting, then why are we interested in the
> > > media_connected state of one specific priv? Shouldn't we check all privs?
> > >
> > > If it's a setting per priv, then why do we choose the same priv
> > > everytime in this function?
> > >
> > > Either way, this function looks fishy and changing it should be done
> > > with an explanation, just dropping the error message and returning
> > > success is not enough.
> > >
> > > Sascha
> > >
> > > --
> >
> > O.K. I will add comment in patch v3.
> 
> Really I hoped that you could clarify a bit how this function works and answer
> some of the questions I raised above. That could help us to get a better driver
> over time. The same code is in the nxpwifi driver as well, so it's not only a
> matter of which driver we decide to maintain in the future.
> 
> Sascha
> 

I will add the comment in patch v3. I am busy on other stuffs.

David