Re: [PATCH] staging: rtl8188eu: core: find and remove code valid only for 5 HGz.

[Jakub Sitnicki <jsitnicki@gmail.com>]

On Thu, Jul 16, 2015 at 04:04 AM CEST, Sreenath Madasu <sreenath.madasu@gmail.com> wrote:
> This one of the TODO tasks for staging rtl8188eu driver. I have removed
> the code referring to channel > 14 for rtw_ap.c, rtw_ieee80211.c and
> rtw_mlme.c files. Please review.
>
> Signed-off-by: Sreenath Madasu <Sreenath.Madasu@gmail.com>

I would consider rewording the subject and the description to say what
the change does ("remove ...") as opposed to saying what you did ("find
and remove...").

The "Description, description, description" section from the article
below, if you haven't seen it yet, explains it better than I can:

https://github.com/gregkh/kernel-tutorial/blob/master/lxf_article/write_kernel_patch.txt

> ---
>  drivers/staging/rtl8188eu/core/rtw_ap.c        | 31 +++++++-------------------
>  drivers/staging/rtl8188eu/core/rtw_ieee80211.c | 24 ++++++--------------
>  drivers/staging/rtl8188eu/core/rtw_mlme.c      |  5 +----
>  3 files changed, 16 insertions(+), 44 deletions(-)
>

[snip]

> @@ -584,15 +576,8 @@ static void update_bmc_sta(struct adapter *padapter)
>  				tx_ra_bitmap |= rtw_get_bit_value_from_ieee_value(psta->bssrateset[i]&0x7f);
>  		}
>  
> -		if (pcur_network->Configuration.DSConfig > 14) {
> -			/* force to A mode. 5G doesn't support CCK rates */
> -			network_type = WIRELESS_11A;
> -			tx_ra_bitmap = 0x150; /*  6, 12, 24 Mbps */
> -		} else {
> -			/* force to b mode */
> -			network_type = WIRELESS_11B;
> -			tx_ra_bitmap = 0xf;
> -		}
> +		network_type = WIRELESS_11B;
> +		tx_ra_bitmap = 0xf;
>  
>  		raid = networktype_to_raid(network_type);
>  		init_rate = get_highest_rate_idx(tx_ra_bitmap&0x0fffffff)&0x3f;

Is the dropped comment ("force to b mode") worth leaving just to draw
attention?  There is something suspicious going on in update_bmc_sta()
because just a few lines above we determine the network_type:

        network_type = rtw_check_network_type((u8 *)&pcur_network->SupportedRates, supportRateNum, 1);

... only to force to it WIRELESS_11B later on.  Unfortunately I'm not
familiar enough with the driver to know why.

> diff --git a/drivers/staging/rtl8188eu/core/rtw_ieee80211.c b/drivers/staging/rtl8188eu/core/rtw_ieee80211.c
> index 11b780d..f55dae1 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_ieee80211.c
> @@ -113,19 +113,12 @@ uint	rtw_is_cckratesonly_included(u8 *rate)
>  
>  int rtw_check_network_type(unsigned char *rate, int ratelen, int channel)
>  {
> -	if (channel > 14) {
> -		if ((rtw_is_cckrates_included(rate)) == true)
> -			return WIRELESS_INVALID;
> -		else
> -			return WIRELESS_11A;
> -	} else {  /*  could be pure B, pure G, or B/G */
> -		if ((rtw_is_cckratesonly_included(rate)) == true)
> -			return WIRELESS_11B;
> -		else if ((rtw_is_cckrates_included(rate)) == true)
> -			return	WIRELESS_11BG;
> -		else
> -			return WIRELESS_11G;
> -	}
> +	if ((rtw_is_cckratesonly_included(rate)) == true)
> +		return WIRELESS_11B;
> +	else if ((rtw_is_cckrates_included(rate)) == true)
> +		return	WIRELESS_11BG;
> +	else
> +		return WIRELESS_11G;
>  }
>

That makes the 'channel' parameter unused.  A candidate for a clean-up
together with 'ratelen'.

Cheers,
Jakub