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 <rafal@xxxxxxxxxx>
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 <rafal@xxxxxxxxxx>
---
.../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'.
