| Message ID | 20220411215402.31939-1-greearb@candelatech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hostapd: allow disabling background radar | expand |
pon., 11 kwi 2022 o 23:59 <greearb@candelatech.com> napisał(a): > > From: Ben Greear <greearb@candelatech.com> > > To work around buggy drivers and/or for any other user preference. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > hostapd/config_file.c | 2 ++ > hostapd/hostapd.conf | 5 +++++ > src/ap/ap_config.c | 1 + > src/ap/ap_config.h | 1 + > src/ap/dfs.c | 3 ++- > 5 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/hostapd/config_file.c b/hostapd/config_file.c > index 81bfb6ef7..998529bcc 100644 > --- a/hostapd/config_file.c > +++ b/hostapd/config_file.c > @@ -3210,6 +3210,8 @@ static int hostapd_config_fill(struct hostapd_config *conf, > conf->acs_freq_list_present = 1; > } else if (os_strcmp(buf, "acs_exclude_6ghz_non_psc") == 0) { > conf->acs_exclude_6ghz_non_psc = atoi(pos); > + } else if (os_strcmp(buf, "disable_background_radar") == 0) { > + conf->disable_background_radar = atoi(pos); > } else if (os_strcmp(buf, "min_tx_power") == 0) { > int val = atoi(pos); > > diff --git a/hostapd/hostapd.conf b/hostapd/hostapd.conf > index fb2a3e83e..07fc625cb 100644 > --- a/hostapd/hostapd.conf > +++ b/hostapd/hostapd.conf > @@ -225,6 +225,11 @@ channel=1 > # Default behavior is to include all PSC and non-PSC channels. > #acs_exclude_6ghz_non_psc=1 > > +# Disable background radar feature, even if radio supports it. > +# 0: Leave active (default) > +# 1: Disable it. > +#disable_background_radar=1 > + > # Set minimum permitted max TX power (in dBm) for ACS and DFS channel selection. > # (default 0, i.e., not constraint) > #min_tx_power=20 > diff --git a/src/ap/ap_config.c b/src/ap/ap_config.c > index f33a25a18..3cae18626 100644 > --- a/src/ap/ap_config.c > +++ b/src/ap/ap_config.c > @@ -250,6 +250,7 @@ struct hostapd_config * hostapd_config_defaults(void) > conf->ap_table_max_size = 255; > conf->ap_table_expiration_time = 60; > conf->track_sta_max_age = 180; > + conf->disable_background_radar = 0; > > #ifdef CONFIG_TESTING_OPTIONS > conf->ignore_probe_probability = 0.0; > diff --git a/src/ap/ap_config.h b/src/ap/ap_config.h > index d1f387221..4ff0382c4 100644 > --- a/src/ap/ap_config.h > +++ b/src/ap/ap_config.h > @@ -974,6 +974,7 @@ struct hostapd_config { > u8 min_tx_power; > enum hostapd_hw_mode hw_mode; /* HOSTAPD_MODE_IEEE80211A, .. */ > int acs_exclude_6ghz_non_psc; > + int disable_background_radar; > enum { > LONG_PREAMBLE = 0, > SHORT_PREAMBLE = 1 > diff --git a/src/ap/dfs.c b/src/ap/dfs.c > index 2c92e1dd3..9384b0bca 100644 > --- a/src/ap/dfs.c > +++ b/src/ap/dfs.c > @@ -34,7 +34,8 @@ dfs_downgrade_bandwidth(struct hostapd_iface *iface, int *secondary_channel, > > static bool dfs_use_radar_background(struct hostapd_iface *iface) > { > - return iface->drv_flags2 & WPA_DRIVER_RADAR_BACKGROUND; > + return (iface->drv_flags2 & WPA_DRIVER_RADAR_BACKGROUND) && > + !iface->conf->disable_background_radar; > } > Thanks Ben. Looks good :) Maybe we should also add param to cli chan_switch command: - use bgcac / use legacy (then we can decide in runtime) - don't switch after bgcac completed - simple EU DFS channels cleanup BR Janusz
On Mon, Apr 11, 2022 at 02:54:02PM -0700, greearb@candelatech.com wrote:
> To work around buggy drivers and/or for any other user preference.
Are there any known issues with drivers advertising this capability but
not working? Why would a user want to disable this? This type of a
configuration parameter was there originally when this capability was
introduced, but it got removed since there was no clear justification
for adding the extra complexity. Has something changed recently to
reconsider that earlier conclusion?
On 4/16/22 6:51 AM, Jouni Malinen wrote: > On Mon, Apr 11, 2022 at 02:54:02PM -0700, greearb@candelatech.com wrote: >> To work around buggy drivers and/or for any other user preference. > > Are there any known issues with drivers advertising this capability but > not working? Why would a user want to disable this? This type of a > configuration parameter was there originally when this capability was > introduced, but it got removed since there was no clear justification > for adding the extra complexity. Has something changed recently to > reconsider that earlier conclusion? > The mt7915 driver mistakenly enables this feature for mt7915 radios from asia-rf that do not have the hardware support for this feature. And there is very little complexity added by my patch. There are often unforseen bugs in new features, I think it is good practice to allow them to be disabled. Thanks, Ben
On Sat, Apr 16, 2022 at 07:18:08AM -0700, Ben Greear wrote: > The mt7915 driver mistakenly enables this feature for mt7915 radios from > asia-rf that do not have the hardware support for this feature. Has that been fixed in the driver already or is this a known, but not fixed issue in an actually released kernel version? > And there is very little complexity added by my patch. There are often > unforseen bugs in new features, I think it is good practice to allow > them to be disabled. Every new configuration parameter makes things more complex. I don't know how a normal user could easily figure out when to disable this particular feature as an example. I don't think I would agree with it being a good practice to provide an explicit user configuration parameter for all new features. In fact, I'd highly prefer not to have to do that for any new feature without a good justification. Whether this particular case is a good justification depends on whether that identified driver issue has made its way into commonly used kernel releases. The most appropriate fix for this would obviously be to fix the driver instead of providing user space workarounds that users would need to manually figure out when to use.
On 4/16/22 7:36 AM, Jouni Malinen wrote: > On Sat, Apr 16, 2022 at 07:18:08AM -0700, Ben Greear wrote: >> The mt7915 driver mistakenly enables this feature for mt7915 radios from >> asia-rf that do not have the hardware support for this feature. > > Has that been fixed in the driver already or is this a known, but not > fixed issue in an actually released kernel version? It is known to the developers and the mailing list, I told both. But there is no fix proposed yet that I have seen. I know how to make my system work by both the hostapd change and by removing the offending patch from the kernel. That doesn't help other users though. > >> And there is very little complexity added by my patch. There are often >> unforseen bugs in new features, I think it is good practice to allow >> them to be disabled. > > Every new configuration parameter makes things more complex. I don't > know how a normal user could easily figure out when to disable this > particular feature as an example. I don't think I would agree with it > being a good practice to provide an explicit user configuration > parameter for all new features. In fact, I'd highly prefer not to have > to do that for any new feature without a good justification. Whether > this particular case is a good justification depends on whether that > identified driver issue has made its way into commonly used kernel > releases. The most appropriate fix for this would obviously be to fix > the driver instead of providing user space workarounds that users would > need to manually figure out when to use. So far, there is one driver that supports this feature, and it is broken. Makes me think it won't be the last. In the case bugs are found in features like this, it is a whole lot easier to tell the user to enable some obscure feature in their hostapd conf file vs have them patch and compile hostapd and/or the kernel. That is why I like enable/disable options for this sort of thing. The end users can have work arounds while proper fixes are completed and fully tested. Thanks, Ben > >
On Sat, Apr 16, 2022 at 07:43:40AM -0700, Ben Greear wrote: > It is known to the developers and the mailing list, I told both. > But there is no fix proposed yet that I have seen. I know how > to make my system work by both the hostapd change and by removing > the offending patch from the kernel. That doesn't help other > users though. OK, that is not convenient. > So far, there is one driver that supports this feature, and it is broken. > Makes me think it won't be the last. This type of capability should really not be enabled in the driver before it has been properly tested.. I was under the impression that this had been tested, but apparently not sufficiently. > In the case bugs are found in features like this, it is a whole lot easier > to tell the user to enable some obscure feature in their hostapd conf file > vs have them patch and compile hostapd and/or the kernel. That is why I > like enable/disable options for this sort of thing. The end users can > have work arounds while proper fixes are completed and fully tested. It would be much more understandable to make this disabled by default rather than providing an obscure configuration parameter that the user would somehow need to figure out when to use to disable it if things don't work. I don't think it would be a good approach to introduce this type of parameters to manually disable something that might not work under some conditions. I would be much more likely to accept a change that would disable this recently added capability by default and provide a new option to enable it. Should the driver side functionality become more robust in the future, this new configuration option in hostapd could then be change to default to having this capability enabled or even better, remove that configuration parameter completely once there are no known issues in commonly used kernel versions.
On Sat, 2022-04-16 at 18:02 +0300, Jouni Malinen wrote: > I would be much more likely to accept a change that would disable this > recently added capability by default and provide a new option to enable > it. Should the driver side functionality become more robust in the > future, this new configuration option in hostapd could then be change to > default to having this capability enabled or even better, remove that > configuration parameter completely once there are no known issues in > commonly used kernel versions. > This was a pretty recent driver capability, arguably it should just be disabled in the driver (CC stable) and then we don't need to have these discussions in userspace ... johannes
On 4/17/22 1:03 PM, Johannes Berg wrote: > On Sat, 2022-04-16 at 18:02 +0300, Jouni Malinen wrote: >> I would be much more likely to accept a change that would disable this >> recently added capability by default and provide a new option to enable >> it. Should the driver side functionality become more robust in the >> future, this new configuration option in hostapd could then be change to >> default to having this capability enabled or even better, remove that >> configuration parameter completely once there are no known issues in >> commonly used kernel versions. >> > > This was a pretty recent driver capability, arguably it should just be > disabled in the driver (CC stable) and then we don't need to have these > discussions in userspace ... The feature might work fine with many radios supported by this driver, just not the asia-rf. Seems the pci id for the asia-rf is same as other cards that do support the feature, but hopefully the mtk folks can find some way to differentiate the hardware automatically. At least with user-space change, those wanting to use the feature on supported hardware can do so with a config change in hostapd w/out having to hack yet another out-of-tree patch into the driver. Thanks, Ben > > johannes >
diff --git a/hostapd/config_file.c b/hostapd/config_file.c index 81bfb6ef7..998529bcc 100644 --- a/hostapd/config_file.c +++ b/hostapd/config_file.c @@ -3210,6 +3210,8 @@ static int hostapd_config_fill(struct hostapd_config *conf, conf->acs_freq_list_present = 1; } else if (os_strcmp(buf, "acs_exclude_6ghz_non_psc") == 0) { conf->acs_exclude_6ghz_non_psc = atoi(pos); + } else if (os_strcmp(buf, "disable_background_radar") == 0) { + conf->disable_background_radar = atoi(pos); } else if (os_strcmp(buf, "min_tx_power") == 0) { int val = atoi(pos); diff --git a/hostapd/hostapd.conf b/hostapd/hostapd.conf index fb2a3e83e..07fc625cb 100644 --- a/hostapd/hostapd.conf +++ b/hostapd/hostapd.conf @@ -225,6 +225,11 @@ channel=1 # Default behavior is to include all PSC and non-PSC channels. #acs_exclude_6ghz_non_psc=1 +# Disable background radar feature, even if radio supports it. +# 0: Leave active (default) +# 1: Disable it. +#disable_background_radar=1 + # Set minimum permitted max TX power (in dBm) for ACS and DFS channel selection. # (default 0, i.e., not constraint) #min_tx_power=20 diff --git a/src/ap/ap_config.c b/src/ap/ap_config.c index f33a25a18..3cae18626 100644 --- a/src/ap/ap_config.c +++ b/src/ap/ap_config.c @@ -250,6 +250,7 @@ struct hostapd_config * hostapd_config_defaults(void) conf->ap_table_max_size = 255; conf->ap_table_expiration_time = 60; conf->track_sta_max_age = 180; + conf->disable_background_radar = 0; #ifdef CONFIG_TESTING_OPTIONS conf->ignore_probe_probability = 0.0; diff --git a/src/ap/ap_config.h b/src/ap/ap_config.h index d1f387221..4ff0382c4 100644 --- a/src/ap/ap_config.h +++ b/src/ap/ap_config.h @@ -974,6 +974,7 @@ struct hostapd_config { u8 min_tx_power; enum hostapd_hw_mode hw_mode; /* HOSTAPD_MODE_IEEE80211A, .. */ int acs_exclude_6ghz_non_psc; + int disable_background_radar; enum { LONG_PREAMBLE = 0, SHORT_PREAMBLE = 1 diff --git a/src/ap/dfs.c b/src/ap/dfs.c index 2c92e1dd3..9384b0bca 100644 --- a/src/ap/dfs.c +++ b/src/ap/dfs.c @@ -34,7 +34,8 @@ dfs_downgrade_bandwidth(struct hostapd_iface *iface, int *secondary_channel, static bool dfs_use_radar_background(struct hostapd_iface *iface) { - return iface->drv_flags2 & WPA_DRIVER_RADAR_BACKGROUND; + return (iface->drv_flags2 & WPA_DRIVER_RADAR_BACKGROUND) && + !iface->conf->disable_background_radar; }