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

["Tobin C. Harding" <me@tobin.cc>]

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 <quytelda@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.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel