Re: [PATCH 1/3] brcmfmac: allow specifying features per firmware version

[Arend van Spriel <arend.vanspriel@broadcom.com>]

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@milecki.pl>
>>>
>>> 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 <rafal@milecki.pl>
>>> ---
>>>   .../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