diff mbox series

ACS: decide offload by hw_mode without special cmd

Message ID 1577941739-13527-1-git-send-email-neojou@gmail.com
State Rejected
Headers show
Series ACS: decide offload by hw_mode without special cmd | expand

Commit Message

Neo Jou Jan. 2, 2020, 5:08 a.m. UTC 
From: Neo Jou <neojou@gmail.com>

Would like to propose that not to limit to use special vendor
command/data to decide to do ACS offload, but by "hw_mode=any"
as the general case for all the drivers.

There is a special config, "hw_mode=any", for the case when the
driver supports offloaded ACS, but it also needs the driver to
report the special vendor data, QCA_NL80211_VENDOR_SUBCMD_DO_ACS,
and to support the vendor command,
    OUI-QCA + QCA_NL80211_VENDOR_SUBCMD_GET_FEATURE
to get the feature report
    QCA_WLAN_VENDOR_FEATURE_SUPPORT_HW_MODE_ANY
which are only for QCA driver.

The setting, "hw_mode=any", in conf file, seems already be
able to be used to decide if to offload ACS to the driver or not.
So would like to propose the patch as the first step for the
general approach of ACS offload.

With the patch, in the case like "hw_mode=a or g", we can still
do ACS computing in hostapd, and offload ACS to the driver when
"hw_mode=any", without special vendor data/command support.

This patch can be the first step for the general approach for ACS
offload. Furthermore, for the case, "hw_mode=any", we only need to
think a general approach to do as one vendor command,
    OUI-QCA + QCA_NL80211_VENDOR_SUBCMD_DO_ACS
does, as the second step, without those special vendor command/data
as above.

Signed-off-by: Neo Jou <neojou@gmail.com>
---
 src/ap/acs.c         | 3 ++-
 src/ap/hw_features.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Jouni Malinen Jan. 2, 2020, 8:43 a.m. UTC | #1 
On Thu, Jan 02, 2020 at 01:08:59PM +0800, neojou@gmail.com wrote:
> Would like to propose that not to limit to use special vendor
> command/data to decide to do ACS offload, but by "hw_mode=any"
> as the general case for all the drivers.

Why?

> There is a special config, "hw_mode=any", for the case when the
> driver supports offloaded ACS, but it also needs the driver to
> report the special vendor data, QCA_NL80211_VENDOR_SUBCMD_DO_ACS,
> and to support the vendor command,
>     OUI-QCA + QCA_NL80211_VENDOR_SUBCMD_GET_FEATURE
> to get the feature report
>     QCA_WLAN_VENDOR_FEATURE_SUPPORT_HW_MODE_ANY
> which are only for QCA driver.
> 
> The setting, "hw_mode=any", in conf file, seems already be
> able to be used to decide if to offload ACS to the driver or not.
> So would like to propose the patch as the first step for the
> general approach of ACS offload.

No, hw_mode=any has nothing to say on whether ACS should or should not
be offloaded. Sure, the current implementation might not support that
with anything else than an offload case now, but this could certainly be
covered with the not offloaded ACS.

> With the patch, in the case like "hw_mode=a or g", we can still
> do ACS computing in hostapd, and offload ACS to the driver when
> "hw_mode=any", without special vendor data/command support.

Why would hw_mode=a be handled differently from hw_mode=any as far as
selecting between whether hostapd or the driver takes care of ACS?

> diff --git a/src/ap/acs.c b/src/ap/acs.c
> @@ -971,7 +971,8 @@ enum hostapd_chan_status acs_init(struct hostapd_iface *iface)
>  {
>  	wpa_printf(MSG_INFO, "ACS: Automatic channel selection started, this may take a bit");
>  
> -	if (iface->drv_flags & WPA_DRIVER_FLAGS_ACS_OFFLOAD) {
> +	if ((iface->drv_flags & WPA_DRIVER_FLAGS_ACS_OFFLOAD) ||
> +	    (iface->conf->hw_mode == HOSTAPD_MODE_IEEE80211ANY)) {
>  		wpa_printf(MSG_INFO, "ACS: Offloading to driver");

This would break existing use case of using ACS offload to pick a
channel within a single band. It is not appropriate to break deployed
functionality.
Neo Jou Jan. 3, 2020, 2:23 a.m. UTC | #2 
Thanks for your kind reply.

> Jouni Malinen <j@w1.fi>
>
> On Thu, Jan 02, 2020 at 01:08:59PM +0800, neojou@gmail.com wrote:
> > Would like to propose that not to limit to use special vendor
> > command/data to decide to do ACS offload, but by "hw_mode=any"
> > as the general case for all the drivers.
>
> Why?
>
> > There is a special config, "hw_mode=any", for the case when the
> > driver supports offloaded ACS, but it also needs the driver to
> > report the special vendor data, QCA_NL80211_VENDOR_SUBCMD_DO_ACS,
> > and to support the vendor command,
> >     OUI-QCA + QCA_NL80211_VENDOR_SUBCMD_GET_FEATURE
> > to get the feature report
> >     QCA_WLAN_VENDOR_FEATURE_SUPPORT_HW_MODE_ANY
> > which are only for QCA driver.
> >
> > The setting, "hw_mode=any", in conf file, seems already be
> > able to be used to decide if to offload ACS to the driver or not.
> > So would like to propose the patch as the first step for the
> > general approach of ACS offload.
>
> No, hw_mode=any has nothing to say on whether ACS should or should not
> be offloaded. Sure, the current implementation might not support that
> with anything else than an offload case now, but this could certainly be
> covered with the not offloaded ACS.
>

In the hostapd.conf, it is said that
"a special value "any" can be used to indicate that any support band can be
 used. This special case is currently supported only with drivers with
which offloaded ACS is used.".

And in the current hostapd program, it fails if we set "hw_mode=any"
without QCA_WLAN_VENDOR_FEATURE_SUPPORT_HW_MODE_ANY.

So it seems hw_mode=any is only for ACS offload case.


> > With the patch, in the case like "hw_mode=a or g", we can still
> > do ACS computing in hostapd, and offload ACS to the driver when
> > "hw_mode=any", without special vendor data/command support.
>
> Why would hw_mode=a be handled differently from hw_mode=any as far as
> selecting between whether hostapd or the driver takes care of ACS?
>
> > diff --git a/src/ap/acs.c b/src/ap/acs.c
> > @@ -971,7 +971,8 @@ enum hostapd_chan_status acs_init(struct hostapd_iface *iface)
> >  {
> >       wpa_printf(MSG_INFO, "ACS: Automatic channel selection started, this may take a bit");
> >
> > -     if (iface->drv_flags & WPA_DRIVER_FLAGS_ACS_OFFLOAD) {
> > +     if ((iface->drv_flags & WPA_DRIVER_FLAGS_ACS_OFFLOAD) ||
> > +         (iface->conf->hw_mode == HOSTAPD_MODE_IEEE80211ANY)) {
> >               wpa_printf(MSG_INFO, "ACS: Offloading to driver");
>
> This would break existing use case of using ACS offload to pick a
> channel within a single band. It is not appropriate to break deployed
> functionality.
>

The doubt is that  "hw_mode=any has nothing to say on whether ACS
should or should not be offloaded" as you said. Based on the
description in hostapd.conf and the behavior at the hostapd program,
it seems "hw_mode=any" is only for ACS offloaded.


BR,
Neo Jou
Jouni Malinen Jan. 4, 2020, 9:45 p.m. UTC | #3 
On Fri, Jan 03, 2020 at 10:23:32AM +0800, Neo Jou wrote:
> In the hostapd.conf, it is said that
> "a special value "any" can be used to indicate that any support band can be
>  used. This special case is currently supported only with drivers with
> which offloaded ACS is used.".
> 
> And in the current hostapd program, it fails if we set "hw_mode=any"
> without QCA_WLAN_VENDOR_FEATURE_SUPPORT_HW_MODE_ANY.
> 
> So it seems hw_mode=any is only for ACS offload case.

That is the only currently implemented case, but that hw_mode=any could
certainly be made to work with the not-offloaded ACS case and that would
be the correct approach to take here. At its simplest form, this could
pick any one of the bands that the driver supports and fill in hw_mode
based on that.

> The doubt is that  "hw_mode=any has nothing to say on whether ACS
> should or should not be offloaded" as you said. Based on the
> description in hostapd.conf and the behavior at the hostapd program,
> it seems "hw_mode=any" is only for ACS offloaded.

It is not in any way implying that the configuration item would be
designed to work only with the offloaded case and no such implication
should be made explicit by breaking this for any use case. There is no
point in changing the meaning of hw_mode=any if the goal is to make that
work with a driver that does not use ACS offloading; it would be much
more valuable to make hw_mode=any work with such a driver (i.e., with
the not offloaded ACS version).
Neo Jou Jan. 6, 2020, 4:05 a.m. UTC | #4 
Thanks for your kind reply. As you mentioned as below, I understand
the design  philosophy of hw_mode now, and please abandon this patch.
Will try to find another approach to do it.

>"That is the only currently implemented case, but that hw_mode=any could
 >certainly be made to work with the not-offloaded ACS case "

> it would be much
> more valuable to make hw_mode=any work with such a driver (i.e., with
>  the not offloaded ACS version). "


> Jouni Malinen <j@w1.fi>
>
> On Fri, Jan 03, 2020 at 10:23:32AM +0800, Neo Jou wrote:
> > In the hostapd.conf, it is said that
> > "a special value "any" can be used to indicate that any support band can be
> >  used. This special case is currently supported only with drivers with
> > which offloaded ACS is used.".
> >
> > And in the current hostapd program, it fails if we set "hw_mode=any"
> > without QCA_WLAN_VENDOR_FEATURE_SUPPORT_HW_MODE_ANY.
> >
> > So it seems hw_mode=any is only for ACS offload case.
>
> That is the only currently implemented case, but that hw_mode=any could
> certainly be made to work with the not-offloaded ACS case and that would
> be the correct approach to take here. At its simplest form, this could
> pick any one of the bands that the driver supports and fill in hw_mode
> based on that.
>
> > The doubt is that  "hw_mode=any has nothing to say on whether ACS
> > should or should not be offloaded" as you said. Based on the
> > description in hostapd.conf and the behavior at the hostapd program,
> > it seems "hw_mode=any" is only for ACS offloaded.
>
> It is not in any way implying that the configuration item would be
> designed to work only with the offloaded case and no such implication
> should be made explicit by breaking this for any use case. There is no
> point in changing the meaning of hw_mode=any if the goal is to make that
> work with a driver that does not use ACS offloading; it would be much
> more valuable to make hw_mode=any work with such a driver (i.e., with
> the not offloaded ACS version).
>
> --
> Jouni Malinen                                            PGP id EFC895FA
diff mbox series

Patch

diff --git a/src/ap/acs.c b/src/ap/acs.c
index 232afa8..80fd32b 100644
--- a/src/ap/acs.c
+++ b/src/ap/acs.c
@@ -971,7 +971,8 @@  enum hostapd_chan_status acs_init(struct hostapd_iface *iface)
 {
 	wpa_printf(MSG_INFO, "ACS: Automatic channel selection started, this may take a bit");
 
-	if (iface->drv_flags & WPA_DRIVER_FLAGS_ACS_OFFLOAD) {
+	if ((iface->drv_flags & WPA_DRIVER_FLAGS_ACS_OFFLOAD) ||
+	    (iface->conf->hw_mode == HOSTAPD_MODE_IEEE80211ANY)) {
 		wpa_printf(MSG_INFO, "ACS: Offloading to driver");
 		if (hostapd_drv_do_acs(iface->bss[0]))
 			return HOSTAPD_CHAN_INVALID;
diff --git a/src/ap/hw_features.c b/src/ap/hw_features.c
index ba10752..4f250b7 100644
--- a/src/ap/hw_features.c
+++ b/src/ap/hw_features.c
@@ -1031,7 +1031,8 @@  int hostapd_select_hw_mode(struct hostapd_iface *iface)
 		}
 	}
 
-	if (iface->current_mode == NULL) {
+	if ((iface->current_mode == NULL) &&
+	    (iface->conf->hw_mode != HOSTAPD_MODE_IEEE80211ANY)) {
 		if (!(iface->drv_flags & WPA_DRIVER_FLAGS_ACS_OFFLOAD) ||
 		    !(iface->drv_flags & WPA_DRIVER_FLAGS_SUPPORT_HW_MODE_ANY))
 		{