Re: ath10k: mac80211 driver for Qualcomm Atheros 802.11ac CQA98xx devices

[Kalle Valo <kvalo@qca.qualcomm.com>]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Wed, 10 Jul 2013, Linux Kernel Mailing List wrote:
>> --- /dev/null
>> +++ b/drivers/net/wireless/ath/ath10k/hw.h
>
>> +#define SUPPORTED_FW_MAJOR	1
>> +#define SUPPORTED_FW_MINOR	0
>> +#define SUPPORTED_FW_RELEASE	0
>> +#define SUPPORTED_FW_BUILD	629
>
>> +static int ath10k_check_fw_version(struct ath10k *ar)
>> +{
>> +	char version[32];
>> +
>> +	if (ar->fw_version_major >= SUPPORTED_FW_MAJOR &&
>> +	    ar->fw_version_minor >= SUPPORTED_FW_MINOR &&
>> +	    ar->fw_version_release >= SUPPORTED_FW_RELEASE &&
>> +	    ar->fw_version_build >= SUPPORTED_FW_BUILD)
>> +		return 0;
>
> My attention got triggered by:
>
> drivers/net/wireless/ath/ath10k/core.c: In function ‘ath10k_check_fw_version’:
> drivers/net/wireless/ath/ath10k/core.c:79: warning: comparison is always true due to limited range of data type

I haven't seen this warning, I guess my compiler is too old.

> as an u16 is always larger or equal than zero. Not much you can do to
> silence that warning, though.

Too bad, I really would like to keep ath10k warning free. Easier to
maintain that way.

Does anyone have any nice trick in their sleeves to make this warning go
away? I guess one ugly option is to change u16 to int.

>
> However, I don't think the version check is correct.
> Shouldn't it stop checking later fields if an exact match is found in an
> earlier field?
>
> I.e.
>
> 	if (ar->fw_version_major > SUPPORTED_FW_MAJOR ||
> 	    (ar->fw_version_major == SUPPORTED_FW_MAJOR &&
> 	     ar->fw_version_minor > SUPPORTED_FW_MINOR) ||
> 	     ...) { ... }
>
> Currently e.g. (major, minor) = (3, 0) is considered older than (2, 1).

Doh, that is indeed wrong. I'll fix that, thanks for spotting this.

> Or perhaps minor is never reset to zero when major is increased? In that
> case, the check is correct, but IMHO it's a bit silly to split the version
> number in seperate fields.

No, the firmware engineers are supposed to reset minor version whenever
major changes.

-- 
Kalle Valo