[1/2] ap: Remove opt-out DisableHT for opt-in EnableHT

Commit Message

James Prestwood June 2, 2023, 2:58 p.m. UTC

HT support was added to AP mode which was not widely tested
across many different wireless adapters. The adapters tested
did not seem to mind the channel width setting but recently
several users have experienced AP mode breaking using the
default/no configuration. Disabling HT works around the issue
and allows AP mode to start in these cases.

This deserves further investigation but its looking like more
often than not users need to add DisableHT. This isn't so bad
for AP profiles since the user has to create a profile anyways,
but for the Start command its best if it "just works". To error
on the side of caution DisableHT is being renamed to EnableHT
making it opt-in rather than opt-out.
---
 src/ap.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Denis Kenzior June 6, 2023, 3:40 p.m. UTC | #1

Hi James,

> This deserves further investigation but its looking like more
> often than not users need to add DisableHT. This isn't so bad
> for AP profiles since the user has to create a profile anyways,
> but for the Start command its best if it "just works". To error
> on the side of caution DisableHT is being renamed to EnableHT
> making it opt-in rather than opt-out.

Our philosophy has always been to have the user perform the least amount of work 
for the best possible result.  Most modern hardware should support HT these 
days, so the 'just works' approach is to enable HT whenever possible and not 
make it 'opt-in'.  The current settings naming is just fine in my opinion.

The ideal solution to the above problem would be to figure out why HT isn't 
working on certain drivers and fix that.

If not possible, the next best approach would be to create a list of drivers we 
know do not work (or vice-versa) and do not attempt HT on the problematic ones 
and print a warning to the log.

Do we know which drivers don't work by the way?  I might take a look when / if I 
have time.

Regards,
-Denis

James Prestwood June 6, 2023, 4:11 p.m. UTC | #2

On 6/6/23 8:40 AM, Denis Kenzior wrote:
> Hi James,
> 
>> This deserves further investigation but its looking like more
>> often than not users need to add DisableHT. This isn't so bad
>> for AP profiles since the user has to create a profile anyways,
>> but for the Start command its best if it "just works". To error
>> on the side of caution DisableHT is being renamed to EnableHT
>> making it opt-in rather than opt-out.
> 
> Our philosophy has always been to have the user perform the least amount 
> of work for the best possible result.  Most modern hardware should 
> support HT these days, so the 'just works' approach is to enable HT 
> whenever possible and not make it 'opt-in'.  The current settings naming 
> is just fine in my opinion.
> 
> The ideal solution to the above problem would be to figure out why HT 
> isn't working on certain drivers and fix that.

I agree.

> 
> If not possible, the next best approach would be to create a list of 
> drivers we know do not work (or vice-versa) and do not attempt HT on the 
> problematic ones and print a warning to the log.
> 
> Do we know which drivers don't work by the way?  I might take a look 
> when / if I have time.

So far complaints have been with brcmfmac (raspi), mwifiex (NXP driver 
out of tree), and one other I'm not sure of (I'll ask).

I did dig into the NXP driver somewhat. Its out of tree so take with a 
grain of salt, but I was able to get it more or less working by first 
issuing NL80211_CMD_SET_WIPHY with the freq/channel width before 
START_AP (and leave both attributes out of START_AP). This is how 
hostapd behaves as well depending on the configuration file. Since this 
seemed like a one-off or maybe legacy behavior so I didn't even bother 
sending a patch, but maybe more investigation is needed.

I tested on iwlwifi, realtek and ath9k.

When I have some time I'll get my raspi set up and see if I can 
reproduce and compare with hostapd.

> 
> Regards,
> -Denis

Patch

diff --git a/src/ap.c b/src/ap.c
index 398e469a..7e6c868e 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -3668,19 +3668,22 @@  static int ap_load_config(struct ap_state *ap, const struct l_settings *config,
 		ap->band = BAND_FREQ_2_4_GHZ;
 	}
 
-	if (l_settings_has_key(config, "General", "DisableHT")) {
+	if (l_settings_has_key(config, "General", "EnableHT")) {
 		bool boolval;
 
-		if (!l_settings_get_bool(config, "General", "DisableHT",
+		if (!l_settings_get_bool(config, "General", "EnableHT",
 						&boolval)) {
-			l_error("AP [General].DisableHT not a valid boolean");
+			l_error("AP [General].EnableHT not a valid boolean");
 			return -EINVAL;
 		}
 
-		ap->supports_ht = !boolval;
-	} else
-		ap->supports_ht = wiphy_get_ht_capabilities(wiphy, ap->band,
-							NULL) != NULL;
+		if (!wiphy_get_ht_capabilities(wiphy, ap->band, NULL)) {
+			l_error("AP hardware does not support HT");
+			return -EINVAL;
+		}
+
+		ap->supports_ht = boolval;
+	}
 
 	if (!ap_validate_band_channel(ap)) {
 		l_error("AP Band and Channel combination invalid");