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

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

[Geert Uytterhoeven]

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

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

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

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.

> +	snprintf(version, sizeof(version), "%u.%u.%u.%u",
> +		 SUPPORTED_FW_MAJOR, SUPPORTED_FW_MINOR,
> +		 SUPPORTED_FW_RELEASE, SUPPORTED_FW_BUILD);
> +
> +	ath10k_warn("WARNING: Firmware version %s is not officially supported.\n",
> +		    ar->hw->wiphy->fw_version);
> +	ath10k_warn("Please upgrade to version %s (or newer)\n", version);
> +
> +	return 0;
> +}

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

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

[Kalle Valo]

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