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

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

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

> +};
> +
> +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