Re: [PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

2018-02-28 Thread Tobin C. Harding 
On Wed, Feb 28, 2018 at 09:19:07PM -0800, Quytelda Kahja wrote:
...

I would normally respond to the cover letter but here goes.

Reviewed-by: Tobin C. Harding 

thanks,
Tobin.

Re: [PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

2018-02-28 Thread Tobin C. Harding 
On Wed, Feb 28, 2018 at 09:19:07PM -0800, Quytelda Kahja wrote:
...

I would normally respond to the cover letter but here goes.

Reviewed-by: Tobin C. Harding 

thanks,
Tobin.

Re: [PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

2018-02-28 Thread Tobin C. Harding 
On Wed, Feb 28, 2018 at 09:19:07PM -0800, Quytelda Kahja wrote:
> The case statement in get_ap_information() should not use literal integers
> to parse information element IDs when these values are provided by name
> in 'enum ieee80211_eid' in the header 'linux/ieee80211.h'.

Nice.  Magic number removal, I like it.

> Signed-off-by: Quytelda Kahja 
> ---
>  drivers/staging/ks7010/ks_hostif.c | 31 +++
>  drivers/staging/ks7010/ks_hostif.h |  1 +
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks_hostif.c 
> b/drivers/staging/ks7010/ks_hostif.c
> index 74a08417bd0b..67cf32433023 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -251,9 +251,8 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   offset = 0;
>  
>   while (bsize > offset) {
> - /* DPRINTK(4, "Element ID=%d\n",*bp); */

nit: Probably shouldn't remove debugging output in the same patch as
doing magic number removal.  (super nit-picky though.)

> - switch (*bp) {
> - case 0: /* ssid */
> + switch (*bp) { /* Information Element ID */
> + case WLAN_EID_SSID:
>   if (*(bp + 1) <= SSID_MAX_SIZE) {
>   ap->ssid.size = *(bp + 1);
>   } else {
> @@ -263,8 +262,8 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   }
>   memcpy(ap->ssid.body, bp + 2, ap->ssid.size);
>   break;
> - case 1: /* rate */
> - case 50:/* ext rate */
> + case WLAN_EID_SUPP_RATES:
> + case WLAN_EID_EXT_SUPP_RATES:
>   if ((*(bp + 1) + ap->rate_set.size) <=
>   RATE_SET_MAX_SIZE) {
>   memcpy(>rate_set.body[ap->rate_set.size],
> @@ -280,9 +279,9 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   (RATE_SET_MAX_SIZE - ap->rate_set.size);
>   }
>   break;
> - case 3: /* DS parameter */
> + case WLAN_EID_DS_PARAMS:
>   break;
> - case 48:/* RSN(WPA2) */
> + case WLAN_EID_RSN:
>   ap->rsn_ie.id = *bp;
>   if (*(bp + 1) <= RSN_IE_BODY_MAX) {
>   ap->rsn_ie.size = *(bp + 1);
> @@ -293,8 +292,8 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   }
>   memcpy(ap->rsn_ie.body, bp + 2, ap->rsn_ie.size);
>   break;
> - case 221:   /* WPA */
> - if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) {   
> /* WPA OUI check */
> + case WLAN_EID_VENDOR_SPECIFIC: /* WPA */
> + if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) { /* 
> WPA OUI check */

Shouldn't have this line (unnecessary white space change)

>   ap->wpa_ie.id = *bp;
>   if (*(bp + 1) <= RSN_IE_BODY_MAX) {
>   ap->wpa_ie.size = *(bp + 1);
> @@ -309,18 +308,18 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   }
>   break;
>  
> - case 2: /* FH parameter */
> - case 4: /* CF parameter */
> - case 5: /* TIM */
> - case 6: /* IBSS parameter */
> - case 7: /* Country */
> - case 42:/* ERP information */
> - case 47:/* Reserve ID 47 Broadcom AP */
> + case WLAN_EID_FH_PARAMS:
> + case WLAN_EID_CF_PARAMS:
> + case WLAN_EID_TIM:
> + case WLAN_EID_IBSS_PARAMS:
> + case WLAN_EID_COUNTRY:
> + case WLAN_EID_ERP_INFO:
>   break;
>   default:
>   DPRINTK(4, "unknown Element ID=%d\n", *bp);
>   break;
>   }
> +

FTR try not to include unrelated whitespace changes.  I see you changed
the case statement but the whitespace is after it and not really
related.  Again quite nit-picky but I'm guessing you are in staging to
learn so might as well learn it now than when you are patching the core
kernel :)


Good work,
Tobin.

Re: [PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

2018-02-28 Thread Tobin C. Harding 
On Wed, Feb 28, 2018 at 09:19:07PM -0800, Quytelda Kahja wrote:
> The case statement in get_ap_information() should not use literal integers
> to parse information element IDs when these values are provided by name
> in 'enum ieee80211_eid' in the header 'linux/ieee80211.h'.

Nice.  Magic number removal, I like it.

> Signed-off-by: Quytelda Kahja 
> ---
>  drivers/staging/ks7010/ks_hostif.c | 31 +++
>  drivers/staging/ks7010/ks_hostif.h |  1 +
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks_hostif.c 
> b/drivers/staging/ks7010/ks_hostif.c
> index 74a08417bd0b..67cf32433023 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -251,9 +251,8 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   offset = 0;
>  
>   while (bsize > offset) {
> - /* DPRINTK(4, "Element ID=%d\n",*bp); */

nit: Probably shouldn't remove debugging output in the same patch as
doing magic number removal.  (super nit-picky though.)

> - switch (*bp) {
> - case 0: /* ssid */
> + switch (*bp) { /* Information Element ID */
> + case WLAN_EID_SSID:
>   if (*(bp + 1) <= SSID_MAX_SIZE) {
>   ap->ssid.size = *(bp + 1);
>   } else {
> @@ -263,8 +262,8 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   }
>   memcpy(ap->ssid.body, bp + 2, ap->ssid.size);
>   break;
> - case 1: /* rate */
> - case 50:/* ext rate */
> + case WLAN_EID_SUPP_RATES:
> + case WLAN_EID_EXT_SUPP_RATES:
>   if ((*(bp + 1) + ap->rate_set.size) <=
>   RATE_SET_MAX_SIZE) {
>   memcpy(>rate_set.body[ap->rate_set.size],
> @@ -280,9 +279,9 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   (RATE_SET_MAX_SIZE - ap->rate_set.size);
>   }
>   break;
> - case 3: /* DS parameter */
> + case WLAN_EID_DS_PARAMS:
>   break;
> - case 48:/* RSN(WPA2) */
> + case WLAN_EID_RSN:
>   ap->rsn_ie.id = *bp;
>   if (*(bp + 1) <= RSN_IE_BODY_MAX) {
>   ap->rsn_ie.size = *(bp + 1);
> @@ -293,8 +292,8 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   }
>   memcpy(ap->rsn_ie.body, bp + 2, ap->rsn_ie.size);
>   break;
> - case 221:   /* WPA */
> - if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) {   
> /* WPA OUI check */
> + case WLAN_EID_VENDOR_SPECIFIC: /* WPA */
> + if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) { /* 
> WPA OUI check */

Shouldn't have this line (unnecessary white space change)

>   ap->wpa_ie.id = *bp;
>   if (*(bp + 1) <= RSN_IE_BODY_MAX) {
>   ap->wpa_ie.size = *(bp + 1);
> @@ -309,18 +308,18 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   }
>   break;
>  
> - case 2: /* FH parameter */
> - case 4: /* CF parameter */
> - case 5: /* TIM */
> - case 6: /* IBSS parameter */
> - case 7: /* Country */
> - case 42:/* ERP information */
> - case 47:/* Reserve ID 47 Broadcom AP */
> + case WLAN_EID_FH_PARAMS:
> + case WLAN_EID_CF_PARAMS:
> + case WLAN_EID_TIM:
> + case WLAN_EID_IBSS_PARAMS:
> + case WLAN_EID_COUNTRY:
> + case WLAN_EID_ERP_INFO:
>   break;
>   default:
>   DPRINTK(4, "unknown Element ID=%d\n", *bp);
>   break;
>   }
> +

FTR try not to include unrelated whitespace changes.  I see you changed
the case statement but the whitespace is after it and not really
related.  Again quite nit-picky but I'm guessing you are in staging to
learn so might as well learn it now than when you are patching the core
kernel :)


Good work,
Tobin.

[PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

2018-02-28 Thread Quytelda Kahja 
The case statement in get_ap_information() should not use literal integers
to parse information element IDs when these values are provided by name
in 'enum ieee80211_eid' in the header 'linux/ieee80211.h'.

Signed-off-by: Quytelda Kahja 
---
 drivers/staging/ks7010/ks_hostif.c | 31 +++
 drivers/staging/ks7010/ks_hostif.h |  1 +
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index 74a08417bd0b..67cf32433023 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -251,9 +251,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct 
ap_info_t *ap_info,
offset = 0;
 
while (bsize > offset) {
-   /* DPRINTK(4, "Element ID=%d\n",*bp); */
-   switch (*bp) {
-   case 0: /* ssid */
+   switch (*bp) { /* Information Element ID */
+   case WLAN_EID_SSID:
if (*(bp + 1) <= SSID_MAX_SIZE) {
ap->ssid.size = *(bp + 1);
} else {
@@ -263,8 +262,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct 
ap_info_t *ap_info,
}
memcpy(ap->ssid.body, bp + 2, ap->ssid.size);
break;
-   case 1: /* rate */
-   case 50:/* ext rate */
+   case WLAN_EID_SUPP_RATES:
+   case WLAN_EID_EXT_SUPP_RATES:
if ((*(bp + 1) + ap->rate_set.size) <=
RATE_SET_MAX_SIZE) {
memcpy(>rate_set.body[ap->rate_set.size],
@@ -280,9 +279,9 @@ int get_ap_information(struct ks_wlan_private *priv, struct 
ap_info_t *ap_info,
(RATE_SET_MAX_SIZE - ap->rate_set.size);
}
break;
-   case 3: /* DS parameter */
+   case WLAN_EID_DS_PARAMS:
break;
-   case 48:/* RSN(WPA2) */
+   case WLAN_EID_RSN:
ap->rsn_ie.id = *bp;
if (*(bp + 1) <= RSN_IE_BODY_MAX) {
ap->rsn_ie.size = *(bp + 1);
@@ -293,8 +292,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct 
ap_info_t *ap_info,
}
memcpy(ap->rsn_ie.body, bp + 2, ap->rsn_ie.size);
break;
-   case 221:   /* WPA */
-   if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) {   
/* WPA OUI check */
+   case WLAN_EID_VENDOR_SPECIFIC: /* WPA */
+   if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) { /* 
WPA OUI check */
ap->wpa_ie.id = *bp;
if (*(bp + 1) <= RSN_IE_BODY_MAX) {
ap->wpa_ie.size = *(bp + 1);
@@ -309,18 +308,18 @@ int get_ap_information(struct ks_wlan_private *priv, 
struct ap_info_t *ap_info,
}
break;
 
-   case 2: /* FH parameter */
-   case 4: /* CF parameter */
-   case 5: /* TIM */
-   case 6: /* IBSS parameter */
-   case 7: /* Country */
-   case 42:/* ERP information */
-   case 47:/* Reserve ID 47 Broadcom AP */
+   case WLAN_EID_FH_PARAMS:
+   case WLAN_EID_CF_PARAMS:
+   case WLAN_EID_TIM:
+   case WLAN_EID_IBSS_PARAMS:
+   case WLAN_EID_COUNTRY:
+   case WLAN_EID_ERP_INFO:
break;
default:
DPRINTK(4, "unknown Element ID=%d\n", *bp);
break;
}
+
offset += 2;/* id & size field */
offset += *(bp + 1);/* +size offset */
bp += (*(bp + 1) + 2);  /* pointer update */
diff --git a/drivers/staging/ks7010/ks_hostif.h 
b/drivers/staging/ks7010/ks_hostif.h
index 5bae8d468e23..9ac317e4b507 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -13,6 +13,7 @@
 #define _KS_HOSTIF_H_
 
 #include 
+#include 
 
 /*
  * HOST-MAC I/F events
-- 
2.16.2

[PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

2018-02-28 Thread Quytelda Kahja 
The case statement in get_ap_information() should not use literal integers
to parse information element IDs when these values are provided by name
in 'enum ieee80211_eid' in the header 'linux/ieee80211.h'.

Signed-off-by: Quytelda Kahja 
---
 drivers/staging/ks7010/ks_hostif.c | 31 +++
 drivers/staging/ks7010/ks_hostif.h |  1 +
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index 74a08417bd0b..67cf32433023 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -251,9 +251,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct 
ap_info_t *ap_info,
offset = 0;
 
while (bsize > offset) {
-   /* DPRINTK(4, "Element ID=%d\n",*bp); */
-   switch (*bp) {
-   case 0: /* ssid */
+   switch (*bp) { /* Information Element ID */
+   case WLAN_EID_SSID:
if (*(bp + 1) <= SSID_MAX_SIZE) {
ap->ssid.size = *(bp + 1);
} else {
@@ -263,8 +262,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct 
ap_info_t *ap_info,
}
memcpy(ap->ssid.body, bp + 2, ap->ssid.size);
break;
-   case 1: /* rate */
-   case 50:/* ext rate */
+   case WLAN_EID_SUPP_RATES:
+   case WLAN_EID_EXT_SUPP_RATES:
if ((*(bp + 1) + ap->rate_set.size) <=
RATE_SET_MAX_SIZE) {
memcpy(>rate_set.body[ap->rate_set.size],
@@ -280,9 +279,9 @@ int get_ap_information(struct ks_wlan_private *priv, struct 
ap_info_t *ap_info,
(RATE_SET_MAX_SIZE - ap->rate_set.size);
}
break;
-   case 3: /* DS parameter */
+   case WLAN_EID_DS_PARAMS:
break;
-   case 48:/* RSN(WPA2) */
+   case WLAN_EID_RSN:
ap->rsn_ie.id = *bp;
if (*(bp + 1) <= RSN_IE_BODY_MAX) {
ap->rsn_ie.size = *(bp + 1);
@@ -293,8 +292,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct 
ap_info_t *ap_info,
}
memcpy(ap->rsn_ie.body, bp + 2, ap->rsn_ie.size);
break;
-   case 221:   /* WPA */
-   if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) {   
/* WPA OUI check */
+   case WLAN_EID_VENDOR_SPECIFIC: /* WPA */
+   if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) { /* 
WPA OUI check */
ap->wpa_ie.id = *bp;
if (*(bp + 1) <= RSN_IE_BODY_MAX) {
ap->wpa_ie.size = *(bp + 1);
@@ -309,18 +308,18 @@ int get_ap_information(struct ks_wlan_private *priv, 
struct ap_info_t *ap_info,
}
break;
 
-   case 2: /* FH parameter */
-   case 4: /* CF parameter */
-   case 5: /* TIM */
-   case 6: /* IBSS parameter */
-   case 7: /* Country */
-   case 42:/* ERP information */
-   case 47:/* Reserve ID 47 Broadcom AP */
+   case WLAN_EID_FH_PARAMS:
+   case WLAN_EID_CF_PARAMS:
+   case WLAN_EID_TIM:
+   case WLAN_EID_IBSS_PARAMS:
+   case WLAN_EID_COUNTRY:
+   case WLAN_EID_ERP_INFO:
break;
default:
DPRINTK(4, "unknown Element ID=%d\n", *bp);
break;
}
+
offset += 2;/* id & size field */
offset += *(bp + 1);/* +size offset */
bp += (*(bp + 1) + 2);  /* pointer update */
diff --git a/drivers/staging/ks7010/ks_hostif.h 
b/drivers/staging/ks7010/ks_hostif.h
index 5bae8d468e23..9ac317e4b507 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -13,6 +13,7 @@
 #define _KS_HOSTIF_H_
 
 #include 
+#include 
 
 /*
  * HOST-MAC I/F events
-- 
2.16.2