Rafał Miłecki <zajec5@xxxxxxxxx> writes:
> 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.
ath10k does something similar. IIRC we currently support four different,
and incompatible, firmware releases now.
--
Kalle Valo
