On 3 January 2017 at 15:14, Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
> On 3 January 2017 at 14:19, Arend Van Spriel
> <arend.vanspriel@xxxxxxxxxxxx> wrote:
>> On 3-1-2017 12:31, Rafał Miłecki wrote:
>>>>> + if (!channel) {
>>>>> + brcmf_err("Firmware reported unexpected channel %d\n",
>>>>> + ch.control_ch_num);
>>>>> + continue;
>>>>> + }
>>>> As stated above something is really off when this happens so should we
>>>> continue and try to make sense of what firmware provides or simply fail.
>>> Well, I could image something like this happening and not being critical.
>>> The simplest case: Broadcom team releases a new firmware which
>>> supports extra 5 GHz channels (e.g. due to the IEEE standard change).
>>> Why should we refuse to run & support all "old" channel just because of that?
>>
>> Fair enough. I was assuming we keep __wl_{2,5}ghz_channels up to date
>> with IEEE standard.
>>
>>> What do you mean by "make sense of what firmware provides"? Would kind
>>> of solution would you suggest?
>>
>> When the above assumption can be assured (by us) the only other scenario
>> would be a change in the firmware API where we wrongly interpret the
>> information retrieved. In this case all subsequent channels will likely
>> result in bogus or accidental matches hence it seems better to bail out
>> early.
>
> Good point, this actually gave me an idea for a small nice
> improvement. I remember I saw something like WL_VER in wl ioctl
> protocol, it was already bumped at some point by Broadcom, when
> chanspec format has changed. We should probably read this number from
> firmware and maybe refuse to run if version is newer than the one we
> know.
I was thinking about WLC_GET_VERSION and you seem to already have it
in brcmfmac under nam BRCMF_C_GET_VERSION. If you wish to be prepared
for firmware API change, I guess you should check version. It seems
brcmfmac supports 1 and 2.
On the other hand if adding firmware with incompatible API you may
want to have different directory or file names. I think this is what
Intel does. This allows one to have multiple firmwares in
/lib/firmware/ and switching between kernels freely.
--
Rafał
