Received-SPF: pass (google.com: domain of linux-wireless+bounces-8964-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 13 Jun 2024 09:29:50 +0200
From: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
To: Arend van Spriel <arend.vanspriel@broadcom.com>,
	brcm80211@lists.linux.dev, linux-wireless@vger.kernel.org
Subject: brcmfmac: implement basic AP-follow-STA
Message-ID: <Zmqf7jCqwlQNGM_j@x1.vandijck-laurijssen.be>
Mail-Followup-To: Arend van Spriel <arend.vanspriel@broadcom.com>,
	brcm80211@lists.linux.dev, linux-wireless@vger.kernel.org
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline

Hi,

/* context */
We (Yamabiko) have an application where we migrated to a bcm4339 sdio wifi chip.
We use it in AP+STA mode: when the chip is detected (wlan+ add uevent),
we call 'run iw dev wlan0 interface add wap0 type __ap' and start
wpa_supplicant on wlan0 and hostapd on wap0.
The STA is more important than the AP.
We have 'roamoff' parameter set. We observed problems with the firmware roaming
before and switched to wpa_supplicant roaming.

We run a linux v5.4.24 derivative.

/* problem */
We observed that the chip is able to switch channel for wpa_supplicant to
connect to a different channel, but it soon looses connection because hostapd
does not change channel too.

This did work with our previous wifi chip (realtek 88x2 something), which notifies
hostapd that it switched.

/* patch description */
I went down and ended up modifying the brcmfmac driver, patch appended below.
For contributing on these mailing lists, I ported it to yesterday's master.
The idea is that whenever a STA issues a connect with channel info, the AP's
will switch to it too. This implies a small glitch in the AP radio, which already
occurred before my patch. it seems that the wifi chip cannot modify radio settings
per virtual interface, although the API to the wifi chip suggests it can (that is
most probable a more generic communication used for other chips that can do this).
The channel switch is also reported to userspace.

To be less invasive, this new behaviour is put behind
a module parameter 'ap_follow_sta'.

/* questions */
1. do you consider this new behaviour an improvement to include in mainline kernel?
2. if not, I still want to ask for a quick review of my implementation,
   for major mistakes. We at Yamabiko have other expertise than wifi.
3. In order to switch channel, I need to provide a frequency and a bandwidth.
   I found it hard to discover if the wifi chip supports different bandwidths
   per virtual interface. Nor did I really discover where in the driver I could
   retrieve the currently used bandwidth. So I hardcoded 20MHz bandwidth
   in brcmf_cfg80211_connect() to use for my new brcmf_ap_follow_sta().
   Any suggestion how I could improve that?

Kind regards,
Kurt Van Dijck

commit df8e547cc1ae24e2a780aec548098fa25d24dbdc
Author: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
Date:   Tue Jun 11 15:46:53 2024

    brcmfmac: implement basic AP-follow-STA
    
    By experience, a bcm4339 maintains differenct channel parameters per vif,
    while it can only handle 1 channel at a time.
    This commit adds a module parameter, ap_follow_sta, that when enabled,
    will switch active AP's to the new channel when a STAtion connects
    and provides channel information.
    
    I use the chip with roamoff=1 set.
    In my setup, using 1 AP and 1 STA on the wireless device,
    the STA constantly looses connection to it AP, unless the AP is manually
    configured to use the same channel.
    With this patch and module parameter activated, the AP switches automatically.
    
    Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 5fe0e671ecb36..c0d7f11cdb9a8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -895,6 +895,79 @@ static bool brcmf_is_ibssmode(struct brcmf_cfg80211_vif *vif)
 	return vif->wdev.iftype == NL80211_IFTYPE_ADHOC;
 }
 
+/* by experience, bcm4339 keeps a channel per VIF, but it can only track 1 channel
+ * If a station connects, try to move the AP's to the same channel
+ * We could also modify the AP channel when AP starts, but that does
+ * not avoid station disconnect, nor speed things up, so leave it like that
+ */
+static inline int brcmf_switch_channel(struct brcmf_if *ifp, u16 chanspec,
+		struct cfg80211_chan_def *chandef)
+{
+	int err, err2;
+
+	/* notify intent to userspace */
+	cfg80211_ch_switch_started_notify(ifp->ndev, chandef, 0);
+
+	/* Firmware 10.x requires setting channel after enabling
+	 * AP and before bringing interface up.
+	 */
+	err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_DOWN, 1);
+	if (err < 0) {
+		bphy_err(ifp->drvr, "BRCMF_C_DOWN 1 error (%d)\n", err);
+		return err;
+	}
+	err = brcmf_fil_iovar_int_set(ifp, "chanspec", chanspec);
+	if (err < 0)
+		bphy_err(ifp->drvr, "Set Channel failed: chspec=%d, %d\n",
+				chanspec, err);
+	/* still proceed and bring iface up */
+	err2 = brcmf_fil_cmd_int_set(ifp, BRCMF_C_UP, 1);
+	if (err2 < 0)
+		bphy_err(ifp->drvr, "BRCMF_C_UP 1 error (%d)\n", err2);
+
+	if (!err)
+		/* notify userspace */
+		cfg80211_ch_switch_notify(ifp->ndev, chandef);
+	return err;
+}
+
+static int brcmf_ap_follow_sta(struct brcmf_if *ifp,
+		struct cfg80211_chan_def *chandef)
+{
+	struct brcmf_pub *drvr = ifp->drvr;
+	struct brcmf_cfg80211_info *cfg = wiphy_to_cfg(drvr->wiphy);
+	u16 chanspec = chandef_to_chanspec(&cfg->d11inf, chandef);
+	struct brcmf_if *lifp;
+	int j;
+	int channel;
+	int ochannel;
+	int err = 0, err2;
+
+	channel = ieee80211_frequency_to_channel(chandef->center_freq1);
+	for (j = 0; j < ARRAY_SIZE(drvr->iflist); ++j) {
+		lifp = drvr->iflist[j];
+		if (!lifp || lifp == ifp)
+			continue;
+
+		if (!test_bit(BRCMF_VIF_STATUS_AP_CREATED,
+					&lifp->vif->sme_state))
+			/* no AP yet */
+			continue;
+		/* find old channel for this VIF */
+		ochannel = ieee80211_frequency_to_channel(
+				lifp->vif->wdev.chandef.center_freq1);
+		if (ochannel != channel) {
+			bphy_err(drvr, "%s ch %i..%i", netdev_name(lifp->ndev),
+					ochannel, channel);
+			/* notify intent to userspace */
+			err2 = brcmf_switch_channel(lifp, chanspec, chandef);
+			if (err2 && !err)
+				err = err2;
+		}
+	}
+	return err;
+}
+
 /**
  * brcmf_mon_add_vif() - create monitor mode virtual interface
  *
@@ -2551,6 +2624,13 @@ brcmf_cfg80211_connect(struct wiphy *wiphy, struct net_device *ndev,
 		ext_join_params->scan_le.passive_time = cpu_to_le32(-1);
 		ext_join_params->scan_le.nprobes = cpu_to_le32(-1);
 	}
+	if (sme->channel && brcmf_want_ap_follow_sta) {
+		struct cfg80211_chan_def chandef = {};
+
+		cfg80211_chandef_create(&chandef, sme->channel,
+				NL80211_CHAN_NO_HT /*?*/);
+		brcmf_ap_follow_sta(ifp, &chandef);
+	}
 
 	brcmf_set_join_pref(ifp, &sme->bss_select);
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index b24faae35873d..d0c0b23a95c46 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -63,6 +63,10 @@ static int brcmf_roamoff;
 module_param_named(roamoff, brcmf_roamoff, int, 0400);
 MODULE_PARM_DESC(roamoff, "Do not use internal roaming engine");
 
+int brcmf_want_ap_follow_sta;
+module_param_named(ap_follow_sta, brcmf_want_ap_follow_sta, int, 0600);
+MODULE_PARM_DESC(ap_follow_sta, "AP follows STA channel in AP+STA mode");
+
 static int brcmf_iapp_enable;
 module_param_named(iapp, brcmf_iapp_enable, int, 0);
 MODULE_PARM_DESC(iapp, "Enable partial support for the obsoleted Inter-Access Point Protocol");
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
index 2be2986d2110a..287c055c4ec84 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
@@ -61,6 +61,11 @@ struct brcmf_mp_device {
 	} bus;
 };
 
+/* ad-hoc module parameter
+ * it affects runtime behaviour, and may safely changed after init
+ */
+extern int brcmf_want_ap_follow_sta;
+
 void brcmf_c_set_joinpref_default(struct brcmf_if *ifp);
 
 struct brcmf_mp_device *brcmf_get_module_param(struct device *dev,