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 <quyte...@tamalin.org>
> ---
> 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(&ap->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.
