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!). > 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 <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. > Maybe the function name could be brcmf_feat_firmware_overrides() instead. Sure!
