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

["Rafał Miłecki" <rafal@milecki.pl>]

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@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.

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@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'.