diff mbox series

brcmfmac: implement basic AP-follow-STA

Message ID Zmqf7jCqwlQNGM_j@x1.vandijck-laurijssen.be
State New
Headers show
Series brcmfmac: implement basic AP-follow-STA | expand

Commit Message

Kurt Van Dijck June 13, 2024, 7:29 a.m. UTC 
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>

Comments

Kalle Valo June 17, 2024, 1:19 p.m. UTC | #1 
Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> writes:

> /* 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'.

FWIW module parameters should be avoided, especially for 802.11 protocol
level functionality.
Arend Van Spriel June 17, 2024, 2:57 p.m. UTC | #2 
On June 17, 2024 3:19:56 PM Kalle Valo <kvalo@kernel.org> wrote:

> Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> writes:
>
>> /* 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'.
>
> FWIW module parameters should be avoided, especially for 802.11 protocol
> level functionality.

Right. Only had taken a quick glance and noticed this. The 802.11 spec does 
not really cover the concept of virtual interfaces, but still having this 
solved through a module parameter for a specific driver is definitely not a 
first choice.

Anyway, I do have some thoughts about this patch. Hopefully I can dive into 
this in coming days.

Regards,
Arend

> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kurt Van Dijck June 17, 2024, 5:27 p.m. UTC | #3 
On ma, 17 jun 2024 16:19:53 +0300, Kalle Valo wrote:
> Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> writes:
> 
> > To be less invasive, this new behaviour is put behind
> > a module parameter 'ap_follow_sta'.
> 
> FWIW module parameters should be avoided, especially for 802.11 protocol
> level functionality.

The module parameter does not add functionality in this case either.
If it's a problem for merging this patch, I'll be more than happy to send a v2
without the parameter, with the functionality always active.

Kurt
diff mbox series

Patch

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,