Re: [PATCH] brcmfmac: include missing AP scan feature

[James Prestwood <prestwoj@gmail.com>]

Hi Alvin,

On Sat, 2022-02-26 at 11:27 +0000, Alvin Šipraga wrote:
> Hi James,
> 
> James Prestwood <prestwoj@gmail.com> writes:
> 
> > Hi Alvin,
> > 
> > On Fri, 2022-02-25 at 22:55 +0000, Alvin Šipraga wrote:
> > > Hi James,
> > > 
> > > James Prestwood <prestwoj@gmail.com> writes:
> > > 
> > > > This driver does not advertise this feature yet scanning with
> > > > on an
> > > > AP interface appears to work just fine.
> > > > ---
> > > >  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c |
> > > > 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > I've submitted this patch mainly to start a discussion about
> > > > it. I
> > > > find it hard to believe that ALL brcmfmac devices support AP
> > > > scanning
> > > > in which case this feature needs to be limited to those devices
> > > > only. Trouble is there is no FW feature for AP scanning AFAIK.
> > > > 
> > > > In any case I think this driver needs to sort out if it
> > > > supports
> > > > this
> > > > feature or not, and advertise as such rather than leaving
> > > > userspace
> > > > in the dark.
> > > 
> > > By the way, what are the typical use-cases for AP scanning?
> > > 
> > > I know that hostapd does a passive scan on the AP interface on
> > > the
> > > assumption that the driver/firmware will gather channel survey
> > > data,
> > > but
> > > that's not a universally applicable assumption. Not all
> > > implementations
> > > will do that.
> > 
> > We have someone wanting it for onboarding/configuring a new
> > headless
> > device. Where on boot, if it is unconfigured, it starts an AP and
> > waits
> > for a client to configure it.
> > 
> > A client would connect, have the device scan and present available
> > networks. The client then selects a network and provides
> > credentials.
> > The new device can then switch back to station mode and connect.
> > 
> > This is a relatively common practice I've seen with IoT devices.
> 
> Ah OK! Actually we do pretty much the same here at B&O with
> brcmfmac. But rather than starting the AP on the primary interface
> (the
> one default created by the driver), we add a separate AP interface
> with
> the equivalent of the following command:
> 
> # iw dev wlan0 add uap0 type __ap
> 
> Here wlan0 is the primary interface, and uap0 is what I call my AP.
> 
> In that case you can start the AP on uap0, but still do scanning on
> wlan0 (which remains in station mode). Don't quote me on it, but I
> think
> this is the canonical approach with this driver. Is it something you
> have considered?

Thanks, this does seem to work on brcmfmac. I had tried this on other
hardware without any luck. I mentioned this to the user but since the
AP scanning feature has been implemented they may want to still use
that, but who knows. I think finding out if brcmfmac is intended to
scan on the AP interface would still be good to know.

> > 
> > Other than this I cant see much else of a use case besides, like
> > you
> > mentioned, gathering survey data to choose a low load channel (ACS
> > its
> > called I think?)
> 
> Yes, see hostap/src/ap/acs.c.
> 
> See also
> https://lore.kernel.org/linux-wireless/96652669-0cad-8cdb-fbe1-4df0f7161102@bang-olufsen.dk/
> for some grumblings of mine on this API.
> 
> > 
> > Sadly this onboarding use case is quite perfect for DPP, but since
> > Apple came up with their own protocol DPP won't work for any
> > products
> > that want cross compatibility...
> 
> Yes, this is a real shame. And I highly doubt that Apple will abandon
> their own protocol.
> 
> > 
> > > 
> > > Kind regards,
> > > Alvin
> > > 
> > > > 
> > > > diff --git
> > > > a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > > > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > > > index fb727778312c..b6a50e65dbf6 100644
> > > > ---
> > > > a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > > > +++
> > > > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > > > @@ -7729,6 +7729,8 @@ struct brcmf_cfg80211_info
> > > > *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
> > > >  #endif
> > > >         }
> > > >  
> > > > +       wiphy->features |= NL80211_FEATURE_AP_SCAN;
> > > > +
> > > >         return cfg;
> > > >  
> > > >  detach: