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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 16 Jul 2015 17:42:37 +0200
From:	Jakub Sitnicki <jsitnicki@...il.com>
To:	Sreenath Madasu <sreenath.madasu@...il.com>
Cc:	gregkh@...uxfoundation.org, somyaanand214@...il.com,
	devel@...verdev.osuosl.org, tapaswenipathak@...il.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8188eu: core: find and remove code valid only for 5 HGz.

On Thu, Jul 16, 2015 at 04:04 AM CEST, Sreenath Madasu <sreenath.madasu@...il.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@...il.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/