diff mbox

Research + questions on brcmfmac and support for monitor mode

Message ID 5B28B68F.8070505@broadcom.com (mailing list archive)
State RFC
Delegated to: Kalle Valo
Headers show

Commit Message

Arend van Spriel June 19, 2018, 7:53 a.m. UTC
On 6/19/2018 9:27 AM, Rafał Miłecki wrote:
> On Mon, 11 Jun 2018 at 12:48, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 5/30/2018 1:52 PM, Rafał Miłecki wrote:
>>> I'm providing extra version info of tested firmware images as requested
>>> by Arend in another e-mail thread.
>>
>> Looking into our firmware repo it there are two flags, ie. WL_MONITOR
>> and WL_RADIOTAP. It seems both are set for firmware containing -stamon-
>> feature. Your list below confirms that. I still plan to add indication
>> for WL_RADIOTAP in the "cap" iovar, but a stamon feature check could be
>> used for older firmwares.
>
> I just checked wl.mk (it's an open source file) and found following line:
> WLFILES_SRC_HI += src/wl/sys/wlc_stamon.c
> not guarded by any ifeq.

wl.mk is used for NIC driver (softmac) so it is not used for fullmac 
firmware.

> It appears wlc_stamon.c is always being compiled in. Are you 100% sure
> that wlc_stamon.c depends & uses radiotap? Are you sure it's
> impossible to include stamon support without radiotap support?
>
> I'm asking because we're going to check "sta_monitor" iovar to find
> out if radiotap support is included. I'd like to be sure it's 100%
> reliable.

I have already created a patch for this (see below). I will submit it 
this week.

Regards,
Arend

  #endif /* FWIL_TYPES_H_ */

Comments

Rafał Miłecki June 19, 2018, 8:32 a.m. UTC | #1
On 19.06.2018 09:53, Arend van Spriel wrote:
> On 6/19/2018 9:27 AM, Rafał Miłecki wrote:
>> On Mon, 11 Jun 2018 at 12:48, Arend van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 5/30/2018 1:52 PM, Rafał Miłecki wrote:
>>>> I'm providing extra version info of tested firmware images as requested
>>>> by Arend in another e-mail thread.
>>>
>>> Looking into our firmware repo it there are two flags, ie. WL_MONITOR
>>> and WL_RADIOTAP. It seems both are set for firmware containing -stamon-
>>> feature. Your list below confirms that. I still plan to add indication
>>> for WL_RADIOTAP in the "cap" iovar, but a stamon feature check could be
>>> used for older firmwares.
>>
>> I just checked wl.mk (it's an open source file) and found following line:
>> WLFILES_SRC_HI += src/wl/sys/wlc_stamon.c
>> not guarded by any ifeq.
> 
> wl.mk is used for NIC driver (softmac) so it is not used for fullmac firmware.

Weird. I see a few rte references in wl.mk which suggests it's used for
both (softmac & fullmac firmware).


>> It appears wlc_stamon.c is always being compiled in. Are you 100% sure
>> that wlc_stamon.c depends & uses radiotap? Are you sure it's
>> impossible to include stamon support without radiotap support?
>>
>> I'm asking because we're going to check "sta_monitor" iovar to find
>> out if radiotap support is included. I'd like to be sure it's 100%
>> reliable.
> 
> I have already created a patch for this (see below). I will submit it this week.

Just to be clear could you also answer my above question, please?

Did you check if it's impossible to build firmware *with* stamon.c (and
sta_monitor iovar) and *without WL_RADIOTAP?


> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> index f70fec6..67d7fc7 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> @@ -207,6 +207,7 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
>       struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0);
>       struct brcmf_pno_macaddr_le pfn_mac;
>       struct brcmf_gscan_config gscan_cfg;
> +    struct brcmf_stamon_sta_config stamon_cfg;
>       u32 wowl_cap;
>       s32 err;
> 
> @@ -217,6 +218,20 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
>           brcmf_feat_iovar_data_set(ifp, BRCMF_FEAT_GSCAN,
>                         "pfn_gscan_cfg",
>                         &gscan_cfg, sizeof(gscan_cfg));
> +
> +    if (!brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MONITOR) ||
> +        !brcmf_feat_is_enabled(ifp, BRCMF_FEAT_RADIOTAP)) {
> +        ifp->fwil_fwerr = true;
> +        memset(&stamon_cfg, 0, sizeof(stamon_cfg));
> +        /* iovar requires IOVF_SET_UP so this fails either way */
> +        err = brcmf_fil_iovar_data_set(ifp, "sta_monitor", &stamon_cfg,
> +                           sizeof(stamon_cfg));

I think it may be simpler (and maybe less invasive) to use
brcmf_fil_iovar_data_get.


> +        if (err != BRCMF_FW_UNSUPPORTED) {
> +            ifp->drvr->feat_flags |= BRCMF_FEAT_MONITOR;
> +            ifp->drvr->feat_flags |= BRCMF_FEAT_RADIOTAP;
> +        }
> +        ifp->fwil_fwerr = false;
> +    }
Arend van Spriel June 19, 2018, 10:49 a.m. UTC | #2
On 6/19/2018 10:32 AM, Rafał Miłecki wrote:
> On 19.06.2018 09:53, Arend van Spriel wrote:
>> On 6/19/2018 9:27 AM, Rafał Miłecki wrote:
>>> On Mon, 11 Jun 2018 at 12:48, Arend van Spriel
>>> <arend.vanspriel@broadcom.com> wrote:
>>>> On 5/30/2018 1:52 PM, Rafał Miłecki wrote:
>>>>> I'm providing extra version info of tested firmware images as
>>>>> requested
>>>>> by Arend in another e-mail thread.
>>>>
>>>> Looking into our firmware repo it there are two flags, ie. WL_MONITOR
>>>> and WL_RADIOTAP. It seems both are set for firmware containing -stamon-
>>>> feature. Your list below confirms that. I still plan to add indication
>>>> for WL_RADIOTAP in the "cap" iovar, but a stamon feature check could be
>>>> used for older firmwares.
>>>
>>> I just checked wl.mk (it's an open source file) and found following
>>> line:
>>> WLFILES_SRC_HI += src/wl/sys/wlc_stamon.c
>>> not guarded by any ifeq.
>>
>> wl.mk is used for NIC driver (softmac) so it is not used for fullmac
>> firmware.
>
> Weird. I see a few rte references in wl.mk which suggests it's used for
> both (softmac & fullmac firmware).

yeah. my mistake.

>>> It appears wlc_stamon.c is always being compiled in. Are you 100% sure
>>> that wlc_stamon.c depends & uses radiotap? Are you sure it's
>>> impossible to include stamon support without radiotap support?
>>>
>>> I'm asking because we're going to check "sta_monitor" iovar to find
>>> out if radiotap support is included. I'd like to be sure it's 100%
>>> reliable.
>>
>> I have already created a patch for this (see below). I will submit it
>> this week.
>
> Just to be clear could you also answer my above question, please?
>
> Did you check if it's impossible to build firmware *with* stamon.c (and
> sta_monitor iovar) and *without WL_RADIOTAP?

Yes. The functions in stamon.c, most importantly wlc_stamon_attach, are 
only invoked in firmware when WL_STA_MONITOR is defined.

>> diff --git
>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> index f70fec6..67d7fc7 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> @@ -207,6 +207,7 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
>>       struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0);
>>       struct brcmf_pno_macaddr_le pfn_mac;
>>       struct brcmf_gscan_config gscan_cfg;
>> +    struct brcmf_stamon_sta_config stamon_cfg;
>>       u32 wowl_cap;
>>       s32 err;
>>
>> @@ -217,6 +218,20 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
>>           brcmf_feat_iovar_data_set(ifp, BRCMF_FEAT_GSCAN,
>>                         "pfn_gscan_cfg",
>>                         &gscan_cfg, sizeof(gscan_cfg));
>> +
>> +    if (!brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MONITOR) ||
>> +        !brcmf_feat_is_enabled(ifp, BRCMF_FEAT_RADIOTAP)) {
>> +        ifp->fwil_fwerr = true;
>> +        memset(&stamon_cfg, 0, sizeof(stamon_cfg));
>> +        /* iovar requires IOVF_SET_UP so this fails either way */
>> +        err = brcmf_fil_iovar_data_set(ifp, "sta_monitor", &stamon_cfg,
>> +                           sizeof(stamon_cfg));
>
> I think it may be simpler (and maybe less invasive) to use
> brcmf_fil_iovar_data_get.

Thanks for the comment, but looking at the firmware I can not concur. In 
the get-path there is yet another compile flag that requires different 
query for different firmwares. For the set there is a prerequisite that 
the firmware stack is up and we know it is not upon executing 
brcmf_feat_attach() so it fails before entering the specific iovar 
handler in firmware.

Regards,
Arend
diff mbox

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
index f70fec6..67d7fc7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
@@ -207,6 +207,7 @@  void brcmf_feat_attach(struct brcmf_pub *drvr)
  	struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0);
  	struct brcmf_pno_macaddr_le pfn_mac;
  	struct brcmf_gscan_config gscan_cfg;
+	struct brcmf_stamon_sta_config stamon_cfg;
  	u32 wowl_cap;
  	s32 err;

@@ -217,6 +218,20 @@  void brcmf_feat_attach(struct brcmf_pub *drvr)
  		brcmf_feat_iovar_data_set(ifp, BRCMF_FEAT_GSCAN,
  					  "pfn_gscan_cfg",
  					  &gscan_cfg, sizeof(gscan_cfg));
+
+	if (!brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MONITOR) ||
+	    !brcmf_feat_is_enabled(ifp, BRCMF_FEAT_RADIOTAP)) {
+		ifp->fwil_fwerr = true;
+		memset(&stamon_cfg, 0, sizeof(stamon_cfg));
+		/* iovar requires IOVF_SET_UP so this fails either way */
+		err = brcmf_fil_iovar_data_set(ifp, "sta_monitor", &stamon_cfg,
+					       sizeof(stamon_cfg));
+		if (err != BRCMF_FW_UNSUPPORTED) {
+			ifp->drvr->feat_flags |= BRCMF_FEAT_MONITOR;
+			ifp->drvr->feat_flags |= BRCMF_FEAT_RADIOTAP;
+		}
+		ifp->fwil_fwerr = false;
+	}
  	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_PNO, "pfn");
  	if (drvr->bus_if->wowl_supported)
  		brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_WOWL, "wowl");
diff --git 
a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
index 4b29070..e42d296 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
@@ -963,4 +963,36 @@  struct brcmf_gscan_config {
  	struct brcmf_gscan_bucket_config bucket[1];
  };

+enum brcmf_stamon_cfg_cmd {
+        STAMON_CFG_CMD_DEL = 0,
+        STAMON_CFG_CMD_ADD = 1,
+        STAMON_CFG_CMD_ENB = 2,
+        STAMON_CFG_CMD_DSB = 3,
+        STAMON_CFG_CMD_CNT = 4,
+        STAMON_CFG_CMD_RSTCNT = 5,
+        STAMON_CFG_CMD_GET_STATS = 6,
+        STAMON_CFG_CMD_SET_MONTIME = 7
+};
+
+/**
+ * struct brcmf_stamon_sta_config - configuration data for sta monitor.
+ *
+ * @cmd: subcommand for this configuration (see @enum 
brcmf_stamon_cfg_cmd).
+ * @mac: mac address of sta for which @cmd is intended.
+ * @version: version of this configuration structure.
+ * @length: number of bytes following this field.
+ * @chanspec: not used ?
+ * @mon_time: time for which STA's are monitored (ms).
+ * @offchan_time: timer for which off-channel STA's are monitored.
+ */
+struct brcmf_stamon_sta_config {
+        __le32 cmd;
+        u8 mac[ETH_ALEN];
+        __le16 version;
+        __le16 length;
+        __le16 chanspec;
+        __le32 monitor_time;
+        __le32 offchan_time;
+};
+