ath6kl was incorrectly assuming that IEEE80211_NUM_BANDS will always be 2
and used that also in the firmware WMI interface definitions. But after
the support for 60 GHz was added to cfg80211 IEEE80211_NUM_BANDS changed to 3
and this can cause all sort of problems, possibly even memory corruption.
I only found this during code review and didn't notice any bugs, but I'm
sure there are a few lurking somewhere.
To fix this rename unused A_NUM_BANDS to ATH6KL_NUM_BANDS, which is
always defined to be 2, and use that in WMI.
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/ath/ath6kl/wmi.c | 12 ++++++++----
drivers/net/wireless/ath/ath6kl/wmi.h | 4 ++--
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index 3629e86..b3d5576 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -2636,11 +2636,13 @@ static int ath6kl_set_bitrate_mask64(struct wmi *wmi, u8 if_idx,
{
struct sk_buff *skb;
int ret, mode, band;
- u64 mcsrate, ratemask[IEEE80211_NUM_BANDS];
+ u64 mcsrate, ratemask[ATH6KL_NUM_BANDS];
struct wmi_set_tx_select_rates64_cmd *cmd;
memset(&ratemask, 0, sizeof(ratemask));
- for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
+
+ /* only check 2.4 and 5 GHz bands, skip the rest */
+ for (band = 0; band <= IEEE80211_BAND_5GHZ; band++) {
/* copy legacy rate mask */
ratemask[band] = mask->control[band].legacy;
if (band == IEEE80211_BAND_5GHZ)
@@ -2686,11 +2688,13 @@ static int ath6kl_set_bitrate_mask32(struct wmi *wmi, u8 if_idx,
{
struct sk_buff *skb;
int ret, mode, band;
- u32 mcsrate, ratemask[IEEE80211_NUM_BANDS];
+ u32 mcsrate, ratemask[ATH6KL_NUM_BANDS];
struct wmi_set_tx_select_rates32_cmd *cmd;
memset(&ratemask, 0, sizeof(ratemask));
- for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
+
+ /* only check 2.4 and 5 GHz bands, skip the rest */
+ for (band = 0; band <= IEEE80211_BAND_5GHZ; band++) {
/* copy legacy rate mask */
ratemask[band] = mask->control[band].legacy;
if (band == IEEE80211_BAND_5GHZ)
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
index b5deaff..c8f94e1 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.h
+++ b/drivers/net/wireless/ath/ath6kl/wmi.h
@@ -48,7 +48,7 @@
#define A_BAND_24GHZ 0
#define A_BAND_5GHZ 1
-#define A_NUM_BANDS 2
+#define ATH6KL_NUM_BANDS 2
/* in ms */
#define WMI_IMPLICIT_PSTREAM_INACTIVITY_INT 5000
@@ -847,7 +847,7 @@ struct wmi_begin_scan_cmd {
u8 scan_type;
/* Supported rates to advertise in the probe request frames */
- struct wmi_supp_rates supp_rates[IEEE80211_NUM_BANDS];
+ struct wmi_supp_rates supp_rates[ATH6KL_NUM_BANDS];
/* how many channels follow */
u8 num_ch;
On Thu, 12 Jul 2012 12:13:12 +0300
Kalle Valo <[email protected]> wrote:
> + /* only check 2.4 and 5 GHz bands, skip the rest */
> + for (band = 0; band <= IEEE80211_BAND_5GHZ; band++) {
There is something inelegant here. The code is mixing an integer and
an enum. I'd rather go with one or those:
two enums:
for (band = IEEE80211_BAND_2GHZ; band <= IEEE80211_BAND_5GHZ; band++) {
or two integers:
for (band = 0; band <= ATH6KL_NUM_BANDS; band++) {
--
Regards,
Pavel Roskin
On 07/20/2012 12:11 AM, Pavel Roskin wrote:
> On Thu, 19 Jul 2012 14:08:44 +0300
> Kalle Valo <[email protected]> wrote:
>> On 07/12/2012 08:48 PM, Pavel Roskin wrote:
>>> On Thu, 12 Jul 2012 12:13:12 +0300
>>> Kalle Valo <[email protected]> wrote:
>>>
>>>> + /* only check 2.4 and 5 GHz bands, skip the rest */
>>>> + for (band = 0; band <= IEEE80211_BAND_5GHZ; band++) {
>>>
>>> There is something inelegant here. The code is mixing an integer
>>> and an enum. I'd rather go with one or those:
>>>
>>> two enums:
>>> for (band = IEEE80211_BAND_2GHZ; band <= IEEE80211_BAND_5GHZ;
>>> band++) {
>>
>> I somewhat see your point. But IMHO zero is commonly used when
>> iterating over an enum to denote the first value and I don't see how
>> IEEE80211_BAND_2GHZ helps here.
>
> It's the lowest band we support. What if the 900MHz band is added one
> day?
Then that should be added to the end of the enum, not beginning. I think
it would be bad if we change enum values on the fly.
Kalle
Hi Pavel,
sorry for the late reply, this got accidentally buried inside my todo
folder.
On 07/12/2012 08:48 PM, Pavel Roskin wrote:
> On Thu, 12 Jul 2012 12:13:12 +0300
> Kalle Valo <[email protected]> wrote:
>
>> + /* only check 2.4 and 5 GHz bands, skip the rest */
>> + for (band = 0; band <= IEEE80211_BAND_5GHZ; band++) {
>
> There is something inelegant here. The code is mixing an integer and
> an enum. I'd rather go with one or those:
>
> two enums:
> for (band = IEEE80211_BAND_2GHZ; band <= IEEE80211_BAND_5GHZ; band++) {
I somewhat see your point. But IMHO zero is commonly used when iterating
over an enum to denote the first value and I don't see how
IEEE80211_BAND_2GHZ helps here.
> or two integers:
> for (band = 0; band <= ATH6KL_NUM_BANDS; band++) {
ATH6KL_NUM_BANDS is also an enum so I don't see the difference.
Kalle
On Fri, 20 Jul 2012 09:18:17 +0300
Kalle Valo <[email protected]> wrote:
> On 07/20/2012 12:11 AM, Pavel Roskin wrote:
> > On Thu, 19 Jul 2012 14:08:44 +0300
> > Kalle Valo <[email protected]> wrote:
> >> On 07/12/2012 08:48 PM, Pavel Roskin wrote:
> >>> On Thu, 12 Jul 2012 12:13:12 +0300
> >>> Kalle Valo <[email protected]> wrote:
> >>>
> >>>> + /* only check 2.4 and 5 GHz bands, skip the rest */
> >>>> + for (band = 0; band <= IEEE80211_BAND_5GHZ; band++) {
> >>>
> >>> There is something inelegant here. The code is mixing an integer
> >>> and an enum. I'd rather go with one or those:
> >>>
> >>> two enums:
> >>> for (band = IEEE80211_BAND_2GHZ; band <= IEEE80211_BAND_5GHZ;
> >>> band++) {
> >>
> >> I somewhat see your point. But IMHO zero is commonly used when
> >> iterating over an enum to denote the first value and I don't see
> >> how IEEE80211_BAND_2GHZ helps here.
> >
> > It's the lowest band we support. What if the 900MHz band is added
> > one day?
>
> Then that should be added to the end of the enum, not beginning. I
> think it would be bad if we change enum values on the fly.
Actually, it should be OK. It's not like it would affect the
userspace.
I believe that properly written code should not rely on the numeric
values of enums. Well, if 0 means something very special (like no
error), it should not be changed. The (ab)uses of enums for bitmasks
should be exempted too. But IEEE80211_BAND_2GHZ is just one of the
bands.
--
Regards,
Pavel Roskin
On Thu, 19 Jul 2012 14:08:44 +0300
Kalle Valo <[email protected]> wrote:
> Hi Pavel,
>
> sorry for the late reply, this got accidentally buried inside my todo
> folder.
No problem.
> On 07/12/2012 08:48 PM, Pavel Roskin wrote:
> > On Thu, 12 Jul 2012 12:13:12 +0300
> > Kalle Valo <[email protected]> wrote:
> >
> >> + /* only check 2.4 and 5 GHz bands, skip the rest */
> >> + for (band = 0; band <= IEEE80211_BAND_5GHZ; band++) {
> >
> > There is something inelegant here. The code is mixing an integer
> > and an enum. I'd rather go with one or those:
> >
> > two enums:
> > for (band = IEEE80211_BAND_2GHZ; band <= IEEE80211_BAND_5GHZ;
> > band++) {
>
> I somewhat see your point. But IMHO zero is commonly used when
> iterating over an enum to denote the first value and I don't see how
> IEEE80211_BAND_2GHZ helps here.
It's the lowest band we support. What if the 900MHz band is added one
day?
> > or two integers:
> > for (band = 0; band <= ATH6KL_NUM_BANDS; band++) {
>
> ATH6KL_NUM_BANDS is also an enum so I don't see the difference.
Actually, it should NOT be _seen_ as an enum, even if it is defined as
such. It's a number of bands, not an arbitrary number.
But I don't want to argue further any about it. Anything short of
having a bitmap of supported bands would be "good enough for now", and
having the bitmap may be hard to justify if only three bands are
recognized. Things may change if more bands are added (900MHz, 3.7GHz
etc).
--
Regards,
Pavel Roskin
On 07/12/2012 12:13 PM, Kalle Valo wrote:
> ath6kl was incorrectly assuming that IEEE80211_NUM_BANDS will always be 2
> and used that also in the firmware WMI interface definitions. But after
> the support for 60 GHz was added to cfg80211 IEEE80211_NUM_BANDS changed to 3
> and this can cause all sort of problems, possibly even memory corruption.
> I only found this during code review and didn't notice any bugs, but I'm
> sure there are a few lurking somewhere.
>
> To fix this rename unused A_NUM_BANDS to ATH6KL_NUM_BANDS, which is
> always defined to be 2, and use that in WMI.
>
> Signed-off-by: Kalle Valo <[email protected]>
Applied to ath6kl.git.
Kalle