From: Rafał Miłecki <[email protected]>
Some features supported by firmware aren't advertised and there is no
way for a driver to query them. This includes e.g. monitor mode details.
Some firmwares support tagging monitor frames, some build radiotap
header but there is no way to detect it.
This commit adds table that will allow specifying features like:
{ "01-abcdef01", BIT(BRCMF_FEAT_FOO) }
Signed-off-by: Rafał Miłecki <[email protected]>
---
.../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
index 876731c57bf5..1194d31d3902 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
@@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file *seq, void *data)
}
#endif /* DEBUG */
+struct brcmf_feat_fwfeat {
+ const char * const fwid;
+ u32 flags;
+};
+
+static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
+};
+
+static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
+{
+ const struct brcmf_feat_fwfeat *e;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(brcmf_feat_fwfeat_map); i++) {
+ e = &brcmf_feat_fwfeat_map[i];
+ if (!strcmp(e->fwid, pub->fwver)) {
+ pub->feat_flags |= e->flags;
+ break;
+ }
+ }
+}
+
/**
* brcmf_feat_iovar_int_get() - determine feature through iovar query.
*
@@ -216,6 +238,8 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
}
brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_FWSUP, "sup_wpa");
+ brcmf_feat_firmware_features(drvr);
+
/* set chip related quirks */
switch (drvr->bus_if->chip) {
case BRCM_CC_43236_CHIP_ID:
--
2.13.6
On 2018-05-24 09:52, Arend van Spriel wrote:
> On 5/24/2018 7:27 AM, Rafał Miłecki wrote:
>> On 23.05.2018 09:58, Arend van Spriel wrote:
>>> On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <[email protected]>
>>>>
>>>> Some features supported by firmware aren't advertised and there is
>>>> no
>>>> way for a driver to query them. This includes e.g. monitor mode
>>>> details.
>>>> Some firmwares support tagging monitor frames, some build radiotap
>>>> header but there is no way to detect it.
>>>>
>>>> This commit adds table that will allow specifying features like:
>>>> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }
>>>
>>> I have my reservations taking this route. Full-dongle monitor mode is
>>> not very reliable especially when running it next to regular STA/AP
>>> interface due to memory constraints. So enabling a feature from host
>>> side could cause issues that are hard to debug.
>>
>> I was using this *really* intensively on BCM4366 and didn't notice any
>> problems. My use case is listening to the air traffic for 300 ms every
>> few seconds (and I got that running for months!).
>
> I understand. I am just saying that we have quite a variety of devices
> that we support with brcmfmac and this allows enabling features from
> the host driver. So any patch adding an entry will need to be reviewed
> with more scrutiny.
Sounds OK to me!
>> > So I think it would be better if the cap iovar would get a new flag
>> for this. Please hold this patch and let me discuss internally.
>>
>> That would be a pretty big limitation to have to wait for and use a
>> special firmware for this feature. Also considering time it takes to
>> release brcmfmac4366c-pcie.bin, Broadcom vs. Cypress, licensing issues
>> with Cypress, we will likely never get all firmwares updated properly.
>>
>> As for me (!) it seems rather unacceptable (no offence!). I believe we
>> should have a free choice to use that discovered feature even if
>> Broadcom didn't test it for host machine purposes (just internal fw
>> purposes as I get it).
>
> Not sure if "unacceptable" is the right word here. Having it
> discoverable from the firmware itself seems preferred to me
> regardless. For the 4366 firmware you are right it is taking a lot of
> time. It has passed verification. It needed to pass sensitive topic of
> radar detection for FCC and ETSI. So I am preparing that release.
OT: is your brcmfmac4366c-pcie.bin going to include BCM4366E support?
> So I
> just have to discuss this monitor feature tomorrow. That is not too
> long, is it?
Sure thing, let us know when you know some details :)
>>> some more specific comments below...
>>>
>>>> Signed-off-by: Rafał Miłecki <[email protected]>
>>>> ---
>>>> .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24
>>>> ++++++++++++++++++++++
>>>> 1 file changed, 24 insertions(+)
>>>>
>>>> diff --git
>>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>>> index 876731c57bf5..1194d31d3902 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>>> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct
>>>> seq_file
>>>> *seq, void *data)
>>>> }
>>>> #endif /* DEBUG */
>>>>
>>>> +struct brcmf_feat_fwfeat {
>>>> + const char * const fwid;
>>>> + u32 flags;
>>>
>>> For consistency call this feat_flags as well.
>>
>> Sure.
>>
>>
>>>> +};
>>>> +
>>>> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
>>>> +};
>>>> +
>>>> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
>>>
>>> Try to keep name of the struct brcmf_pub instance 'drvr'.
>>
>> Wait, doesn't "pub" make more sense for "struct brcmf_pub" than
>> "drvr"?
>> :) I'm sure I also saw "pub" variables all over the code, so this
>> isn't
>> some new/mine convention.
>
> We used to have a private and public structure long ago and we
> collapsed it into brcmf_pub. So not all places changed the variable to
> 'drvr' and I do not care that much for existing stuff. Could put the
> rename task on TODO list on wireless wiki, but not sure if newbies
> would look there. For new stuff let's try to stick with 'drvr'.
On 5/24/2018 10:21 AM, Rafał Miłecki wrote:
>
> OT: is your brcmfmac4366c-pcie.bin going to include BCM4366E support?
I would think so. I actually tested with 4366E before passing the
firmware for verification.
Regards,
Arend
Hi Rafał,
I love your patch! Perhaps something to improve:
[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on v4.17-rc6 next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/brcmfmac-allow-specifying-features-per-firmware-version/20180523-160546
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c:304:30: sparse: expression using sizeof(void)
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c:417:34: sparse: incorrect type in assignment (different base types) @@ expected restricted __le16 [usertype] it_len @@ got 6 [usertype] it_len @@
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c:417:34: expected restricted __le16 [usertype] it_len
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c:417:34: got unsigned long
vim +417 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
264
265 static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
266 struct net_device *ndev)
267 {
268 int ret;
269 struct brcmf_if *ifp = netdev_priv(ndev);
270 struct brcmf_pub *drvr = ifp->drvr;
271 struct ethhdr *eh;
272 int head_delta;
273
274 brcmf_dbg(DATA, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx);
275
276 /* Can the device send data? */
277 if (drvr->bus_if->state != BRCMF_BUS_UP) {
278 brcmf_err("xmit rejected state=%d\n", drvr->bus_if->state);
279 netif_stop_queue(ndev);
280 dev_kfree_skb(skb);
281 ret = -ENODEV;
282 goto done;
283 }
284
285 /* Some recent Broadcom's firmwares disassociate STA when they receive
286 * an 802.11f ADD frame. This behavior can lead to a local DoS security
287 * issue. Attacker may trigger disassociation of any STA by sending a
288 * proper Ethernet frame to the wireless interface.
289 *
290 * Moreover this feature may break AP interfaces in some specific
291 * setups. This applies e.g. to the bridge with hairpin mode enabled and
292 * IFLA_BRPORT_MCAST_TO_UCAST set. IAPP packet generated by a firmware
293 * will get passed back to the wireless interface and cause immediate
294 * disassociation of a just-connected STA.
295 */
296 if (!drvr->settings->iapp && brcmf_skb_is_iapp(skb)) {
297 dev_kfree_skb(skb);
298 ret = -EINVAL;
299 goto done;
300 }
301
302 /* Make sure there's enough writeable headroom */
303 if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
> 304 head_delta = max_t(int, drvr->hdrlen - skb_headroom(skb), 0);
305
306 brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
307 brcmf_ifname(ifp), head_delta);
308 atomic_inc(&drvr->bus_if->stats.pktcowed);
309 ret = pskb_expand_head(skb, ALIGN(head_delta, NET_SKB_PAD), 0,
310 GFP_ATOMIC);
311 if (ret < 0) {
312 brcmf_err("%s: failed to expand headroom\n",
313 brcmf_ifname(ifp));
314 atomic_inc(&drvr->bus_if->stats.pktcow_failed);
315 goto done;
316 }
317 }
318
319 /* validate length for ether packet */
320 if (skb->len < sizeof(*eh)) {
321 ret = -EINVAL;
322 dev_kfree_skb(skb);
323 goto done;
324 }
325
326 eh = (struct ethhdr *)(skb->data);
327
328 if (eh->h_proto == htons(ETH_P_PAE))
329 atomic_inc(&ifp->pend_8021x_cnt);
330
331 /* determine the priority */
332 if ((skb->priority == 0) || (skb->priority > 7))
333 skb->priority = cfg80211_classify8021d(skb, NULL);
334
335 ret = brcmf_proto_tx_queue_data(drvr, ifp->ifidx, skb);
336 if (ret < 0)
337 brcmf_txfinalize(ifp, skb, false);
338
339 done:
340 if (ret) {
341 ndev->stats.tx_dropped++;
342 } else {
343 ndev->stats.tx_packets++;
344 ndev->stats.tx_bytes += skb->len;
345 }
346
347 /* Return ok: we always eat the packet */
348 return NETDEV_TX_OK;
349 }
350
351 void brcmf_txflowblock_if(struct brcmf_if *ifp,
352 enum brcmf_netif_stop_reason reason, bool state)
353 {
354 unsigned long flags;
355
356 if (!ifp || !ifp->ndev)
357 return;
358
359 brcmf_dbg(TRACE, "enter: bsscfgidx=%d stop=0x%X reason=%d state=%d\n",
360 ifp->bsscfgidx, ifp->netif_stop, reason, state);
361
362 spin_lock_irqsave(&ifp->netif_stop_lock, flags);
363 if (state) {
364 if (!ifp->netif_stop)
365 netif_stop_queue(ifp->ndev);
366 ifp->netif_stop |= reason;
367 } else {
368 ifp->netif_stop &= ~reason;
369 if (!ifp->netif_stop)
370 netif_wake_queue(ifp->ndev);
371 }
372 spin_unlock_irqrestore(&ifp->netif_stop_lock, flags);
373 }
374
375 void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb)
376 {
377 /* Most of Broadcom's firmwares send 802.11f ADD frame every time a new
378 * STA connects to the AP interface. This is an obsoleted standard most
379 * users don't use, so don't pass these frames up unless requested.
380 */
381 if (!ifp->drvr->settings->iapp && brcmf_skb_is_iapp(skb)) {
382 brcmu_pkt_buf_free_skb(skb);
383 return;
384 }
385
386 if (skb->pkt_type == PACKET_MULTICAST)
387 ifp->ndev->stats.multicast++;
388
389 if (!(ifp->ndev->flags & IFF_UP)) {
390 brcmu_pkt_buf_free_skb(skb);
391 return;
392 }
393
394 ifp->ndev->stats.rx_bytes += skb->len;
395 ifp->ndev->stats.rx_packets++;
396
397 brcmf_dbg(DATA, "rx proto=0x%X\n", ntohs(skb->protocol));
398 if (in_interrupt())
399 netif_rx(skb);
400 else
401 /* If the receive is not processed inside an ISR,
402 * the softirqd must be woken explicitly to service
403 * the NET_RX_SOFTIRQ. This is handled by netif_rx_ni().
404 */
405 netif_rx_ni(skb);
406 }
407
408 void brcmf_netif_mon_rx(struct brcmf_if *ifp, struct sk_buff *skb)
409 {
410 if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MON_FMT_RADIOTAP)) {
411 /* Do nothing */
412 } else {
413 struct ieee80211_radiotap_header *radiotap;
414
415 radiotap = skb_push(skb, sizeof(*radiotap));
416 memset(radiotap, 0, sizeof(*radiotap));
> 417 radiotap->it_len = sizeof(*radiotap);
418
419 /* TODO: what are these extra 4 bytes? */
420 skb->len -= 4;
421 }
422
423 skb->dev = ifp->ndev;
424 skb_reset_mac_header(skb);
425 skb->pkt_type = PACKET_OTHERHOST;
426 skb->protocol = htons(ETH_P_802_2);
427
428 brcmf_netif_rx(ifp, skb);
429 }
430
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 5/27/2018 7:34 AM, Julian Calaby wrote:
> Hi Arend,
> On Fri, May 25, 2018 at 12:38 PM Arend van Spriel <
> [email protected]> wrote:
>
>> On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <[email protected]>
>>>
>>> New Broadcom firmwares mark monitor mode packets using a newly defined
>>> bit in the flags field. Use it to filter them out and pass to the
>>> monitor interface. These defines were found in bcmmsgbuf.h from SDK.
>>>
>>> As not every firmware generates radiotap header this commit introduces
>>> BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
>>> not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 frame
>>> and prepends it with an empty radiotap header.
>>>
>>> It's limited to the msgbuf protocol. Adding support for SDIO/USB devices
>>> will require some extra research.
>
>> So I went looking on my shelf and found the patch I made for SDIO a
>> while back. It relies on firmware change and I did not introduce a
>> firmware flag for it. I will send the patch as RFT so you can have a
>> look at it.
>
>> I checked our internal driver and it turns out the raw vs. radiotap is a
>> compilation option. So depending on customer they would get either
>> firmware doing the radiotap header generation or the host driver,
>> without any run-time way to determine it. Not nice for brcmfmac.
>
> I pretty sure I know what the answer to this is going to be, however I
> still have to ask:
>
> Is it possible to open up _any_ part of the firmware? (Even if it's just
> the host-interface end and the hardware end is a big ugly blob) It'd make
> dealing with stuff like this so much easier if we can just toggle a flag
> and recompile. (I'm also sure that we'd be more than happy to pass some
> flag through telling us/you that it's unofficial firmware and has probably
> broken the hardware, etc.)
Hah. Yeah, with next to nil uncertainty I know the answer to this as
well. If you know how long it took before we committed to supporting the
upstream drivers. Despite my better judgement I will pass your idea, but
it is not something that is going to fall in the timeframe that Rafał
had in mind.
Regards,
Arend
On 23.05.2018 09:58, Arend van Spriel wrote:
> On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <[email protected]>
>>
>> Some features supported by firmware aren't advertised and there is no
>> way for a driver to query them. This includes e.g. monitor mode details.
>> Some firmwares support tagging monitor frames, some build radiotap
>> header but there is no way to detect it.
>>
>> This commit adds table that will allow specifying features like:
>> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }
>
> I have my reservations taking this route. Full-dongle monitor mode is not very reliable especially when running it next to regular STA/AP interface due to memory constraints. So enabling a feature from host side could cause issues that are hard to debug.
I was using this *really* intensively on BCM4366 and didn't notice any
problems. My use case is listening to the air traffic for 300 ms every
few seconds (and I got that running for months!).
> So I think it would be better if the cap iovar would get a new flag for this. Please hold this patch and let me discuss internally.
That would be a pretty big limitation to have to wait for and use a
special firmware for this feature. Also considering time it takes to
release brcmfmac4366c-pcie.bin, Broadcom vs. Cypress, licensing issues
with Cypress, we will likely never get all firmwares updated properly.
As for me (!) it seems rather unacceptable (no offence!). I believe we
should have a free choice to use that discovered feature even if
Broadcom didn't test it for host machine purposes (just internal fw
purposes as I get it).
> some more specific comments below...
>
>> Signed-off-by: Rafał Miłecki <[email protected]>
>> ---
>> .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24 ++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> index 876731c57bf5..1194d31d3902 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file *seq, void *data)
>> }
>> #endif /* DEBUG */
>>
>> +struct brcmf_feat_fwfeat {
>> + const char * const fwid;
>> + u32 flags;
>
> For consistency call this feat_flags as well.
Sure.
>> +};
>> +
>> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
>> +};
>> +
>> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
>
> Try to keep name of the struct brcmf_pub instance 'drvr'.
Wait, doesn't "pub" make more sense for "struct brcmf_pub" than "drvr"?
:) I'm sure I also saw "pub" variables all over the code, so this isn't
some new/mine convention.
> Maybe the function name could be brcmf_feat_firmware_overrides() instead.
Sure!
On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> Some features supported by firmware aren't advertised and there is no
> way for a driver to query them. This includes e.g. monitor mode details.
> Some firmwares support tagging monitor frames, some build radiotap
> header but there is no way to detect it.
>
> This commit adds table that will allow specifying features like:
> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }
Discussed this over here. As we are releasing 4366c1 firmware we could
add a proper flag, but there may be other devices/firmwares that can
support certain features so this approach is an acceptable
alternative.... (more below)
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24 ++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> index 876731c57bf5..1194d31d3902 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file *seq, void *data)
> }
> #endif /* DEBUG */
>
> +struct brcmf_feat_fwfeat {
> + const char * const fwid;
> + u32 flags;
> +};
> +
> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
> +};
> +
> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
> +{
> + const struct brcmf_feat_fwfeat *e;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(brcmf_feat_fwfeat_map); i++) {
> + e = &brcmf_feat_fwfeat_map[i];
> + if (!strcmp(e->fwid, pub->fwver)) {
> + pub->feat_flags |= e->flags;
... However, let's be more verbose about this here.
Regards,
Arend
From: Rafał Miłecki <[email protected]>
New Broadcom firmwares mark monitor mode packets using a newly defined
bit in the flags field. Use it to filter them out and pass to the
monitor interface. These defines were found in bcmmsgbuf.h from SDK.
As not every firmware generates radiotap header this commit introduces
BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 frame
and prepends it with an empty radiotap header.
It's limited to the msgbuf protocol. Adding support for SDIO/USB devices
will require some extra research.
Signed-off-by: Rafał Miłecki <[email protected]>
---
.../wireless/broadcom/brcm80211/brcmfmac/core.c | 24 ++++++++++++++++++++++
.../wireless/broadcom/brcm80211/brcmfmac/core.h | 2 ++
.../wireless/broadcom/brcm80211/brcmfmac/feature.h | 6 +++++-
.../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 17 +++++++++++++++
4 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 8d4511eaa9b9..f58d7f7f1359 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -21,6 +21,7 @@
#include <net/cfg80211.h>
#include <net/rtnetlink.h>
#include <net/addrconf.h>
+#include <net/ieee80211_radiotap.h>
#include <net/ipv6.h>
#include <brcmu_utils.h>
#include <brcmu_wifi.h>
@@ -404,6 +405,29 @@ void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb)
netif_rx_ni(skb);
}
+void brcmf_netif_mon_rx(struct brcmf_if *ifp, struct sk_buff *skb)
+{
+ if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MON_FMT_RADIOTAP)) {
+ /* Do nothing */
+ } else {
+ struct ieee80211_radiotap_header *radiotap;
+
+ radiotap = skb_push(skb, sizeof(*radiotap));
+ memset(radiotap, 0, sizeof(*radiotap));
+ radiotap->it_len = sizeof(*radiotap);
+
+ /* TODO: what are these extra 4 bytes? */
+ skb->len -= 4;
+ }
+
+ skb->dev = ifp->ndev;
+ skb_reset_mac_header(skb);
+ skb->pkt_type = PACKET_OTHERHOST;
+ skb->protocol = htons(ETH_P_802_2);
+
+ brcmf_netif_rx(ifp, skb);
+}
+
static int brcmf_rx_hdrpull(struct brcmf_pub *drvr, struct sk_buff *skb,
struct brcmf_if **ifp)
{
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index 401f50458686..dcf6e27cc16f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -121,6 +121,7 @@ struct brcmf_pub {
struct brcmf_if *iflist[BRCMF_MAX_IFS];
s32 if2bss[BRCMF_MAX_IFS];
+ struct brcmf_if *mon_if;
struct mutex proto_block;
unsigned char proto_buf[BRCMF_DCMD_MAXLEN];
@@ -216,6 +217,7 @@ void brcmf_txflowblock_if(struct brcmf_if *ifp,
enum brcmf_netif_stop_reason reason, bool state);
void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success);
void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb);
+void brcmf_netif_mon_rx(struct brcmf_if *ifp, struct sk_buff *skb);
void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on);
int __init brcmf_core_init(void);
void __exit brcmf_core_exit(void);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
index d1193825e559..6e417d104b7f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
@@ -33,6 +33,8 @@
* MFP: 802.11w Management Frame Protection.
* GSCAN: enhanced scan offload feature.
* FWSUP: Firmware supplicant.
+ * MON_802_11_FLAG: monitor packets flagged as 802.11
+ * MON_FMT_RADIOTAP: monitor packets include radiotap header
*/
#define BRCMF_FEAT_LIST \
BRCMF_FEAT_DEF(MBSS) \
@@ -48,7 +50,9 @@
BRCMF_FEAT_DEF(WOWL_ARP_ND) \
BRCMF_FEAT_DEF(MFP) \
BRCMF_FEAT_DEF(GSCAN) \
- BRCMF_FEAT_DEF(FWSUP)
+ BRCMF_FEAT_DEF(FWSUP) \
+ BRCMF_FEAT_DEF(MON_802_11_FLAG) \
+ BRCMF_FEAT_DEF(MON_FMT_RADIOTAP)
/*
* Quirks:
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
index 49d37ad96958..47a9318cccb8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -69,6 +69,8 @@
#define BRCMF_MSGBUF_MAX_EVENTBUF_POST 8
#define BRCMF_MSGBUF_PKT_FLAGS_FRAME_802_3 0x01
+#define BRCMF_MSGBUF_PKT_FLAGS_FRAME_802_11 0x02
+#define BRCMF_MSGBUF_PKT_FLAGS_FRAME_MASK 0x07
#define BRCMF_MSGBUF_PKT_FLAGS_PRIO_SHIFT 5
#define BRCMF_MSGBUF_TX_FLUSH_CNT1 32
@@ -1128,6 +1130,7 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
struct sk_buff *skb;
u16 data_offset;
u16 buflen;
+ u16 flags;
u32 idx;
struct brcmf_if *ifp;
@@ -1137,6 +1140,7 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
data_offset = le16_to_cpu(rx_complete->data_offset);
buflen = le16_to_cpu(rx_complete->data_len);
idx = le32_to_cpu(rx_complete->msg.request_id);
+ flags = le16_to_cpu(rx_complete->flags);
skb = brcmf_msgbuf_get_pktid(msgbuf->drvr->bus_if->dev,
msgbuf->rx_pktids, idx);
@@ -1150,6 +1154,19 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
skb_trim(skb, buflen);
+ if ((flags & BRCMF_MSGBUF_PKT_FLAGS_FRAME_MASK) ==
+ BRCMF_MSGBUF_PKT_FLAGS_FRAME_802_11) {
+ ifp = msgbuf->drvr->mon_if;
+
+ if (!ifp) {
+ brcmf_err("Received unexpected monitor pkt\n");
+ brcmu_pkt_buf_free_skb(skb);
+ }
+
+ brcmf_netif_mon_rx(ifp, skb);
+ return;
+ }
+
ifp = brcmf_get_ifp(msgbuf->drvr, rx_complete->msg.ifidx);
if (!ifp || !ifp->ndev) {
brcmf_err("Received pkt for invalid ifidx %d\n",
--
2.13.6
From: Rafał Miłecki <[email protected]>
Right now it's limited to firmwares that mark monitor interface packets
with a special flag. It's required to distinguish them from other
interface packets as firmware doesn't use any unique ifidx for monitor
interface.
In the future one may also add support for older firmwares without
support for proper packet flags. That will require limiting interface
combos to allow monitor mode *only* and adjusting condition in the
brcmf_msgbuf_process_rx_complete().
Signed-off-by: Rafał Miłecki <[email protected]>
---
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 107 +++++++++++++++++++--
.../wireless/broadcom/brcm80211/brcmfmac/core.c | 65 ++++++++++++-
.../wireless/broadcom/brcm80211/brcmfmac/core.h | 2 +
.../wireless/broadcom/brcm80211/brcmfmac/fwil.h | 2 +
4 files changed, 168 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index f5b405c98047..bbb4f913eece 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -22,6 +22,7 @@
#include <linux/vmalloc.h>
#include <net/cfg80211.h>
#include <net/netlink.h>
+#include <uapi/linux/if_arp.h>
#include <brcmu_utils.h>
#include <defs.h>
@@ -608,6 +609,82 @@ static bool brcmf_is_ibssmode(struct brcmf_cfg80211_vif *vif)
return vif->wdev.iftype == NL80211_IFTYPE_ADHOC;
}
+/**
+ * brcmf_mon_add_vif() - create monitor mode virtual interface
+ *
+ * @wiphy: wiphy device of new interface.
+ * @name: name of the new interface.
+ */
+static struct wireless_dev *brcmf_mon_add_vif(struct wiphy *wiphy,
+ const char *name)
+{
+ struct brcmf_cfg80211_info *cfg = wiphy_to_cfg(wiphy);
+ struct brcmf_cfg80211_vif *vif;
+ struct net_device *ndev;
+ struct brcmf_if *ifp;
+ int err;
+
+ if (cfg->pub->mon_if) {
+ err = -EEXIST;
+ goto err_out;
+ }
+
+ vif = brcmf_alloc_vif(cfg, NL80211_IFTYPE_MONITOR);
+ if (IS_ERR(vif)) {
+ err = PTR_ERR(vif);
+ goto err_out;
+ }
+
+ ndev = alloc_netdev(sizeof(*ifp), name, NET_NAME_UNKNOWN, ether_setup);
+ if (!ndev) {
+ err = -ENOMEM;
+ goto err_free_vif;
+ }
+ ndev->type = ARPHRD_IEEE80211_RADIOTAP;
+ ndev->ieee80211_ptr = &vif->wdev;
+ ndev->needs_free_netdev = true;
+ ndev->priv_destructor = brcmf_cfg80211_free_netdev;
+ SET_NETDEV_DEV(ndev, wiphy_dev(cfg->wiphy));
+
+ ifp = netdev_priv(ndev);
+ ifp->vif = vif;
+ ifp->ndev = ndev;
+ ifp->drvr = cfg->pub;
+
+ vif->ifp = ifp;
+ vif->wdev.netdev = ndev;
+
+ err = brcmf_net_mon_attach(ifp);
+ if (err) {
+ brcmf_err("Failed to attach %s device\n", ndev->name);
+ free_netdev(ndev);
+ goto err_free_vif;
+ }
+
+ cfg->pub->mon_if = ifp;
+
+ return &vif->wdev;
+
+err_free_vif:
+ brcmf_free_vif(vif);
+err_out:
+ return ERR_PTR(err);
+}
+
+static int brcmf_mon_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
+{
+ struct brcmf_cfg80211_info *cfg = wiphy_to_cfg(wiphy);
+ struct net_device *ndev = wdev->netdev;
+
+ ndev->netdev_ops->ndo_stop(ndev);
+
+ brcmf_net_detach(ndev, true);
+
+ cfg->pub->mon_if = NULL;
+
+ return 0;
+}
+
static struct wireless_dev *brcmf_cfg80211_add_iface(struct wiphy *wiphy,
const char *name,
unsigned char name_assign_type,
@@ -628,9 +705,10 @@ static struct wireless_dev *brcmf_cfg80211_add_iface(struct wiphy *wiphy,
case NL80211_IFTYPE_STATION:
case NL80211_IFTYPE_AP_VLAN:
case NL80211_IFTYPE_WDS:
- case NL80211_IFTYPE_MONITOR:
case NL80211_IFTYPE_MESH_POINT:
return ERR_PTR(-EOPNOTSUPP);
+ case NL80211_IFTYPE_MONITOR:
+ return brcmf_mon_add_vif(wiphy, name);
case NL80211_IFTYPE_AP:
wdev = brcmf_ap_add_vif(wiphy, name, params);
break;
@@ -810,9 +888,10 @@ int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
case NL80211_IFTYPE_STATION:
case NL80211_IFTYPE_AP_VLAN:
case NL80211_IFTYPE_WDS:
- case NL80211_IFTYPE_MONITOR:
case NL80211_IFTYPE_MESH_POINT:
return -EOPNOTSUPP;
+ case NL80211_IFTYPE_MONITOR:
+ return brcmf_mon_del_vif(wiphy, wdev);
case NL80211_IFTYPE_AP:
return brcmf_cfg80211_del_ap_iface(wiphy, wdev);
case NL80211_IFTYPE_P2P_CLIENT:
@@ -6339,9 +6418,10 @@ static int brcmf_setup_ifmodes(struct wiphy *wiphy, struct brcmf_if *ifp)
struct ieee80211_iface_limit *c0_limits = NULL;
struct ieee80211_iface_limit *p2p_limits = NULL;
struct ieee80211_iface_limit *mbss_limits = NULL;
- bool mbss, p2p;
- int i, c, n_combos;
+ bool mon, mbss, p2p;
+ int i, c, n_combos, n_limits;
+ mon = brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MON_802_11_FLAG);
mbss = brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS);
p2p = brcmf_feat_is_enabled(ifp, BRCMF_FEAT_P2P);
@@ -6353,14 +6433,21 @@ static int brcmf_setup_ifmodes(struct wiphy *wiphy, struct brcmf_if *ifp)
wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
BIT(NL80211_IFTYPE_ADHOC) |
BIT(NL80211_IFTYPE_AP);
+ if (mon)
+ wiphy->interface_modes |= BIT(NL80211_IFTYPE_MONITOR);
c = 0;
i = 0;
- c0_limits = kcalloc(p2p ? 3 : 2, sizeof(*c0_limits), GFP_KERNEL);
+ n_limits = 1 + mon + p2p ? 2 : 1;
+ c0_limits = kcalloc(n_limits, sizeof(*c0_limits), GFP_KERNEL);
if (!c0_limits)
goto err;
c0_limits[i].max = 1;
c0_limits[i++].types = BIT(NL80211_IFTYPE_STATION);
+ if (mon) {
+ c0_limits[i].max = 1;
+ c0_limits[i++].types = BIT(NL80211_IFTYPE_MONITOR);
+ }
if (p2p) {
if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MCHAN))
combo[c].num_different_channels = 2;
@@ -6406,14 +6493,20 @@ static int brcmf_setup_ifmodes(struct wiphy *wiphy, struct brcmf_if *ifp)
if (mbss) {
c++;
i = 0;
- mbss_limits = kcalloc(1, sizeof(*mbss_limits), GFP_KERNEL);
+ n_limits = 1 + mon;
+ mbss_limits = kcalloc(n_limits, sizeof(*mbss_limits),
+ GFP_KERNEL);
if (!mbss_limits)
goto err;
mbss_limits[i].max = 4;
mbss_limits[i++].types = BIT(NL80211_IFTYPE_AP);
+ if (mon) {
+ mbss_limits[i].max = 1;
+ mbss_limits[i++].types = BIT(NL80211_IFTYPE_MONITOR);
+ }
combo[c].beacon_int_infra_match = true;
combo[c].num_different_channels = 1;
- combo[c].max_interfaces = 4;
+ combo[c].max_interfaces = 4 + mon;
combo[c].n_limits = i;
combo[c].limits = mbss_limits;
}
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index f58d7f7f1359..3f62e75dfa44 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -623,7 +623,7 @@ int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
return -EBADE;
}
-static void brcmf_net_detach(struct net_device *ndev, bool rtnl_locked)
+void brcmf_net_detach(struct net_device *ndev, bool rtnl_locked)
{
if (ndev->reg_state == NETREG_REGISTERED) {
if (rtnl_locked)
@@ -636,6 +636,69 @@ static void brcmf_net_detach(struct net_device *ndev, bool rtnl_locked)
}
}
+static int brcmf_net_mon_open(struct net_device *ndev)
+{
+ struct brcmf_if *ifp = netdev_priv(ndev);
+ u32 monitor;
+ int err;
+
+ brcmf_dbg(TRACE, "Enter\n");
+
+ err = brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_MONITOR, &monitor);
+ if (err) {
+ brcmf_err("BRCMF_C_GET_MONITOR error (%d)\n", err);
+ return err;
+ } else if (monitor) {
+ brcmf_err("Monitor mode is already enabled\n");
+ return -EEXIST;
+ }
+
+ monitor = 3;
+ err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_MONITOR, monitor);
+ if (err)
+ brcmf_err("BRCMF_C_SET_MONITOR error (%d)\n", err);
+
+ return err;
+}
+
+static int brcmf_net_mon_stop(struct net_device *ndev)
+{
+ struct brcmf_if *ifp = netdev_priv(ndev);
+ u32 monitor;
+ int err;
+
+ brcmf_dbg(TRACE, "Enter\n");
+
+ monitor = 0;
+ err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_MONITOR, monitor);
+ if (err)
+ brcmf_err("BRCMF_C_SET_MONITOR error (%d)\n", err);
+
+ return err;
+}
+
+static const struct net_device_ops brcmf_netdev_ops_mon = {
+ .ndo_open = brcmf_net_mon_open,
+ .ndo_stop = brcmf_net_mon_stop,
+};
+
+int brcmf_net_mon_attach(struct brcmf_if *ifp)
+{
+ struct net_device *ndev;
+ int err;
+
+ brcmf_dbg(TRACE, "Enter\n");
+
+ ndev = ifp->ndev;
+ ndev->netdev_ops = &brcmf_netdev_ops_mon;
+
+ err = register_netdevice(ndev);
+ if (err)
+ brcmf_err("Failed to register %s device\n", ndev->name);
+
+ return err;
+}
+
void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on)
{
struct net_device *ndev;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index dcf6e27cc16f..2d37a2fc6a6f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -218,6 +218,8 @@ void brcmf_txflowblock_if(struct brcmf_if *ifp,
void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success);
void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb);
void brcmf_netif_mon_rx(struct brcmf_if *ifp, struct sk_buff *skb);
+void brcmf_net_detach(struct net_device *ndev, bool rtnl_locked);
+int brcmf_net_mon_attach(struct brcmf_if *ifp);
void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on);
int __init brcmf_core_init(void);
void __exit brcmf_core_exit(void);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h
index 63b1287e2e6d..0d9492fd758d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h
@@ -60,6 +60,8 @@
#define BRCMF_C_GET_PM 85
#define BRCMF_C_SET_PM 86
#define BRCMF_C_GET_REVINFO 98
+#define BRCMF_C_GET_MONITOR 107
+#define BRCMF_C_SET_MONITOR 108
#define BRCMF_C_GET_CURR_RATESET 114
#define BRCMF_C_GET_AP 117
#define BRCMF_C_SET_AP 118
--
2.13.6
On 5/24/2018 7:27 AM, Rafał Miłecki wrote:
> On 23.05.2018 09:58, Arend van Spriel wrote:
>> On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <[email protected]>
>>>
>>> Some features supported by firmware aren't advertised and there is no
>>> way for a driver to query them. This includes e.g. monitor mode details.
>>> Some firmwares support tagging monitor frames, some build radiotap
>>> header but there is no way to detect it.
>>>
>>> This commit adds table that will allow specifying features like:
>>> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }
>>
>> I have my reservations taking this route. Full-dongle monitor mode is
>> not very reliable especially when running it next to regular STA/AP
>> interface due to memory constraints. So enabling a feature from host
>> side could cause issues that are hard to debug.
>
> I was using this *really* intensively on BCM4366 and didn't notice any
> problems. My use case is listening to the air traffic for 300 ms every
> few seconds (and I got that running for months!).
I understand. I am just saying that we have quite a variety of devices
that we support with brcmfmac and this allows enabling features from the
host driver. So any patch adding an entry will need to be reviewed with
more scrutiny.
> > So I think it would be better if the cap iovar would get a new flag
> for this. Please hold this patch and let me discuss internally.
>
> That would be a pretty big limitation to have to wait for and use a
> special firmware for this feature. Also considering time it takes to
> release brcmfmac4366c-pcie.bin, Broadcom vs. Cypress, licensing issues
> with Cypress, we will likely never get all firmwares updated properly.
>
> As for me (!) it seems rather unacceptable (no offence!). I believe we
> should have a free choice to use that discovered feature even if
> Broadcom didn't test it for host machine purposes (just internal fw
> purposes as I get it).
Not sure if "unacceptable" is the right word here. Having it
discoverable from the firmware itself seems preferred to me regardless.
For the 4366 firmware you are right it is taking a lot of time. It has
passed verification. It needed to pass sensitive topic of radar
detection for FCC and ETSI. So I am preparing that release. So I just
have to discuss this monitor feature tomorrow. That is not too long, is it?
>> some more specific comments below...
>>
>>> Signed-off-by: Rafał Miłecki <[email protected]>
>>> ---
>>> .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24
>>> ++++++++++++++++++++++
>>> 1 file changed, 24 insertions(+)
>>>
>>> diff --git
>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> index 876731c57bf5..1194d31d3902 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file
>>> *seq, void *data)
>>> }
>>> #endif /* DEBUG */
>>>
>>> +struct brcmf_feat_fwfeat {
>>> + const char * const fwid;
>>> + u32 flags;
>>
>> For consistency call this feat_flags as well.
>
> Sure.
>
>
>>> +};
>>> +
>>> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
>>> +};
>>> +
>>> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
>>
>> Try to keep name of the struct brcmf_pub instance 'drvr'.
>
> Wait, doesn't "pub" make more sense for "struct brcmf_pub" than "drvr"?
> :) I'm sure I also saw "pub" variables all over the code, so this isn't
> some new/mine convention.
We used to have a private and public structure long ago and we collapsed
it into brcmf_pub. So not all places changed the variable to 'drvr' and
I do not care that much for existing stuff. Could put the rename task on
TODO list on wireless wiki, but not sure if newbies would look there.
For new stuff let's try to stick with 'drvr'.
Regards,
Arend
Hi Arend,
On Fri, May 25, 2018 at 12:38 PM Arend van Spriel <
[email protected]> wrote:
> On 5/22/2018 3:18 PM, Rafa=C5=82 Mi=C5=82ecki wrote:
> > From: Rafa=C5=82 Mi=C5=82ecki <[email protected]>
> >
> > New Broadcom firmwares mark monitor mode packets using a newly defined
> > bit in the flags field. Use it to filter them out and pass to the
> > monitor interface. These defines were found in bcmmsgbuf.h from SDK.
> >
> > As not every firmware generates radiotap header this commit introduces
> > BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
> > not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 fram=
e
> > and prepends it with an empty radiotap header.
> >
> > It's limited to the msgbuf protocol. Adding support for SDIO/USB device=
s
> > will require some extra research.
> So I went looking on my shelf and found the patch I made for SDIO a
> while back. It relies on firmware change and I did not introduce a
> firmware flag for it. I will send the patch as RFT so you can have a
> look at it.
> I checked our internal driver and it turns out the raw vs. radiotap is a
> compilation option. So depending on customer they would get either
> firmware doing the radiotap header generation or the host driver,
> without any run-time way to determine it. Not nice for brcmfmac.
I pretty sure I know what the answer to this is going to be, however I
still have to ask:
Is it possible to open up _any_ part of the firmware? (Even if it's just
the host-interface end and the hardware end is a big ugly blob) It'd make
dealing with stuff like this so much easier if we can just toggle a flag
and recompile. (I'm also sure that we'd be more than happy to pass some
flag through telling us/you that it's unofficial firmware and has probably
broken the hardware, etc.)
Thanks,
--=20
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Hi Arend,
On Wed, May 30, 2018 at 8:05 PM Arend van Spriel <
[email protected]> wrote:
> On 5/27/2018 7:34 AM, Julian Calaby wrote:
> > Hi Arend,
> > On Fri, May 25, 2018 at 12:38 PM Arend van Spriel <
> > [email protected]> wrote:
> >
> >> On 5/22/2018 3:18 PM, Rafa=C5=82 Mi=C5=82ecki wrote:
> >>> From: Rafa=C5=82 Mi=C5=82ecki <[email protected]>
> >>>
> >>> New Broadcom firmwares mark monitor mode packets using a newly define=
d
> >>> bit in the flags field. Use it to filter them out and pass to the
> >>> monitor interface. These defines were found in bcmmsgbuf.h from SDK.
> >>>
> >>> As not every firmware generates radiotap header this commit introduce=
s
> >>> BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version.
If
> >>> not present brcmf_netif_mon_rx() assumed packet being a raw 802.11
frame
> >>> and prepends it with an empty radiotap header.
> >>>
> >>> It's limited to the msgbuf protocol. Adding support for SDIO/USB
devices
> >>> will require some extra research.
> >
> >> So I went looking on my shelf and found the patch I made for SDIO a
> >> while back. It relies on firmware change and I did not introduce a
> >> firmware flag for it. I will send the patch as RFT so you can have a
> >> look at it.
> >
> >> I checked our internal driver and it turns out the raw vs. radiotap is
a
> >> compilation option. So depending on customer they would get either
> >> firmware doing the radiotap header generation or the host driver,
> >> without any run-time way to determine it. Not nice for brcmfmac.
> >
> > I pretty sure I know what the answer to this is going to be, however I
> > still have to ask:
> >
> > Is it possible to open up _any_ part of the firmware? (Even if it's jus=
t
> > the host-interface end and the hardware end is a big ugly blob) It'd
make
> > dealing with stuff like this so much easier if we can just toggle a fla=
g
> > and recompile. (I'm also sure that we'd be more than happy to pass some
> > flag through telling us/you that it's unofficial firmware and has
probably
> > broken the hardware, etc.)
> Hah. Yeah, with next to nil uncertainty I know the answer to this as
> well. If you know how long it took before we committed to supporting the
> upstream drivers. Despite my better judgement I will pass your idea, but
> it is not something that is going to fall in the timeframe that Rafa=C5=
=82
> had in mind.
Fair enough, I can't ask for more than that.
Good Luck!
--=20
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> New Broadcom firmwares mark monitor mode packets using a newly defined
> bit in the flags field. Use it to filter them out and pass to the
> monitor interface. These defines were found in bcmmsgbuf.h from SDK.
>
> As not every firmware generates radiotap header this commit introduces
> BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
> not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 frame
> and prepends it with an empty radiotap header.
>
> It's limited to the msgbuf protocol. Adding support for SDIO/USB devices
> will require some extra research.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> .../wireless/broadcom/brcm80211/brcmfmac/core.c | 24 ++++++++++++++++++++++
> .../wireless/broadcom/brcm80211/brcmfmac/core.h | 2 ++
> .../wireless/broadcom/brcm80211/brcmfmac/feature.h | 6 +++++-
> .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 17 +++++++++++++++
> 4 files changed, 48 insertions(+), 1 deletion(-)
>
[snip]
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
> index d1193825e559..6e417d104b7f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
> @@ -33,6 +33,8 @@
> * MFP: 802.11w Management Frame Protection.
> * GSCAN: enhanced scan offload feature.
> * FWSUP: Firmware supplicant.
> + * MON_802_11_FLAG: monitor packets flagged as 802.11
> + * MON_FMT_RADIOTAP: monitor packets include radiotap header
Hi Rafał,
I am preparing the 4366c0 release. Can you tell me which firmwares that
you have provide 802.11 packets and/or radiotap+802.11? Doing 'strings
fwfile | tail -1' should give me the necessary info.
Regards,
Arend
> */
> #define BRCMF_FEAT_LIST \
> BRCMF_FEAT_DEF(MBSS) \
> @@ -48,7 +50,9 @@
> BRCMF_FEAT_DEF(WOWL_ARP_ND) \
> BRCMF_FEAT_DEF(MFP) \
> BRCMF_FEAT_DEF(GSCAN) \
> - BRCMF_FEAT_DEF(FWSUP)
> + BRCMF_FEAT_DEF(FWSUP) \
> + BRCMF_FEAT_DEF(MON_802_11_FLAG) \
> + BRCMF_FEAT_DEF(MON_FMT_RADIOTAP)
On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> New Broadcom firmwares mark monitor mode packets using a newly defined
> bit in the flags field. Use it to filter them out and pass to the
> monitor interface. These defines were found in bcmmsgbuf.h from SDK.
>
> As not every firmware generates radiotap header this commit introduces
> BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
> not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 frame
> and prepends it with an empty radiotap header.
>
> It's limited to the msgbuf protocol. Adding support for SDIO/USB devices
> will require some extra research.
So I went looking on my shelf and found the patch I made for SDIO a
while back. It relies on firmware change and I did not introduce a
firmware flag for it. I will send the patch as RFT so you can have a
look at it.
I checked our internal driver and it turns out the raw vs. radiotap is a
compilation option. So depending on customer they would get either
firmware doing the radiotap header generation or the host driver,
without any run-time way to determine it. Not nice for brcmfmac.
Regards,
Arend
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> .../wireless/broadcom/brcm80211/brcmfmac/core.c | 24 ++++++++++++++++++++++
> .../wireless/broadcom/brcm80211/brcmfmac/core.h | 2 ++
> .../wireless/broadcom/brcm80211/brcmfmac/feature.h | 6 +++++-
> .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 17 +++++++++++++++
> 4 files changed, 48 insertions(+), 1 deletion(-)
>
On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> Some features supported by firmware aren't advertised and there is no
> way for a driver to query them. This includes e.g. monitor mode details.
> Some firmwares support tagging monitor frames, some build radiotap
> header but there is no way to detect it.
>
> This commit adds table that will allow specifying features like:
> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }
I have my reservations taking this route. Full-dongle monitor mode is
not very reliable especially when running it next to regular STA/AP
interface due to memory constraints. So enabling a feature from host
side could cause issues that are hard to debug. So I think it would be
better if the cap iovar would get a new flag for this. Please hold this
patch and let me discuss internally.
some more specific comments below...
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24 ++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> index 876731c57bf5..1194d31d3902 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file *seq, void *data)
> }
> #endif /* DEBUG */
>
> +struct brcmf_feat_fwfeat {
> + const char * const fwid;
> + u32 flags;
For consistency call this feat_flags as well.
> +};
> +
> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
> +};
> +
> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
Try to keep name of the struct brcmf_pub instance 'drvr'. Maybe the
function name could be brcmf_feat_firmware_overrides() instead.
Regards,
Arend