[PATCH 0/3] staging: r8188eu: clean up rtw_rf.c

[PATCH 0/3] staging: r8188eu: clean up rtw_rf.c

[Michael Straube]

This set removes 5 GHz frequencies from the channel-frequency map in
rtw_rf.c and simplifies the rtw_ch2freq() function.

Tested on x86_64 with Inter-Tech DMG-02.

Michael Straube (3):
  staging: r8188eu: remove 5 GHz channels from ch_freq_map
  staging: r8188eu: refactor rtw_ch2freq()
  staging: r8188eu: clean up rtw_rf.c

 drivers/staging/r8188eu/core/rtw_rf.c | 61 +++++++--------------------
 1 file changed, 16 insertions(+), 45 deletions(-)

-- 
2.35.1

[PATCH 1/3] staging: r8188eu: remove 5 GHz channels from ch_freq_map

[Michael Straube]

This driver is for chips that operate only in the 2.4 GHz band.
Frequencies for 5 GHz channels can be removed from the ch_freq_map
array.

Signed-off-by: Michael Straube <straube.linux@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_rf.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_rf.c b/drivers/staging/r8188eu/core/rtw_rf.c
index e704092d31d0..51425971782b 100644
--- a/drivers/staging/r8188eu/core/rtw_rf.c
+++ b/drivers/staging/r8188eu/core/rtw_rf.c
@@ -17,22 +17,6 @@ static struct ch_freq ch_freq_map[] = {
 	{1, 2412}, {2, 2417}, {3, 2422}, {4, 2427}, {5, 2432},
 	{6, 2437}, {7, 2442}, {8, 2447}, {9, 2452}, {10, 2457},
 	{11, 2462}, {12, 2467}, {13, 2472}, {14, 2484},
-	/*  UNII */
-	{36, 5180}, {40, 5200}, {44, 5220}, {48, 5240}, {52, 5260},
-	{56, 5280}, {60, 5300}, {64, 5320}, {149, 5745}, {153, 5765},
-	{157, 5785}, {161, 5805}, {165, 5825}, {167, 5835}, {169, 5845},
-	{171, 5855}, {173, 5865},
-	/* HiperLAN2 */
-	{100, 5500}, {104, 5520}, {108, 5540}, {112, 5560}, {116, 5580},
-	{120, 5600}, {124, 5620}, {128, 5640}, {132, 5660}, {136, 5680},
-	{140, 5700},
-	/* Japan MMAC */
-	{34, 5170}, {38, 5190}, {42, 5210}, {46, 5230},
-	/*  Japan */
-	{184, 4920}, {188, 4940}, {192, 4960}, {196, 4980},
-	{208, 5040},/* Japan, means J08 */
-	{212, 5060},/* Japan, means J12 */
-	{216, 5080},/* Japan, means J16 */
 };
 
 static int ch_freq_map_num = ARRAY_SIZE(ch_freq_map);
-- 
2.35.1

[PATCH 2/3] staging: r8188eu: refactor rtw_ch2freq()

[Michael Straube]

Since we have only channel values from 1 to 14 we can convert the
array ch_freq_map to a simple integer array and use the indices as
channel numbers. This simplifies the code and avoids looping through
the array to get the frequency.

Signed-off-by: Michael Straube <straube.linux@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_rf.c | 40 +++++++++++----------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_rf.c b/drivers/staging/r8188eu/core/rtw_rf.c
index 51425971782b..945a40e6511a 100644
--- a/drivers/staging/r8188eu/core/rtw_rf.c
+++ b/drivers/staging/r8188eu/core/rtw_rf.c
@@ -8,32 +8,24 @@
 #include "../include/recv_osdep.h"
 #include "../include/xmit_osdep.h"
 
-struct ch_freq {
-	u32 channel;
-	u32 frequency;
+static const u32 ch_freq_map[] = {
+	2412,
+	2417,
+	2422,
+	2427,
+	2432,
+	2437,
+	2442,
+	2447,
+	2452,
+	2457,
+	2462,
+	2467,
+	2472,
+	2484
 };
 
-static struct ch_freq ch_freq_map[] = {
-	{1, 2412}, {2, 2417}, {3, 2422}, {4, 2427}, {5, 2432},
-	{6, 2437}, {7, 2442}, {8, 2447}, {9, 2452}, {10, 2457},
-	{11, 2462}, {12, 2467}, {13, 2472}, {14, 2484},
-};
-
-static int ch_freq_map_num = ARRAY_SIZE(ch_freq_map);
-
 u32 rtw_ch2freq(u32 channel)
 {
-	u8	i;
-	u32	freq = 0;
-
-	for (i = 0; i < ch_freq_map_num; i++) {
-		if (channel == ch_freq_map[i].channel) {
-			freq = ch_freq_map[i].frequency;
-				break;
-		}
-	}
-	if (i == ch_freq_map_num)
-		freq = 2412;
-
-	return freq;
+	return ch_freq_map[channel - 1];
 }
-- 
2.35.1

[PATCH 3/3] staging: r8188eu: clean up rtw_rf.c

[Michael Straube]

Remove unnecessary includes and the unused define _RTW_RF_C_ from
rtw_rf.c.

Signed-off-by: Michael Straube <straube.linux@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_rf.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_rf.c b/drivers/staging/r8188eu/core/rtw_rf.c
index 945a40e6511a..ac1c06a60735 100644
--- a/drivers/staging/r8188eu/core/rtw_rf.c
+++ b/drivers/staging/r8188eu/core/rtw_rf.c
@@ -1,12 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright(c) 2007 - 2011 Realtek Corporation. */
 
-#define _RTW_RF_C_
-
-#include "../include/osdep_service.h"
 #include "../include/drv_types.h"
-#include "../include/recv_osdep.h"
-#include "../include/xmit_osdep.h"
 
 static const u32 ch_freq_map[] = {
 	2412,
-- 
2.35.1

Re: [PATCH 2/3] staging: r8188eu: refactor rtw_ch2freq()

[Pavel Skripkin]

Hi Michael,

On 2/20/22 18:48, Michael Straube wrote:
> -static int ch_freq_map_num = ARRAY_SIZE(ch_freq_map);
> -
>   u32 rtw_ch2freq(u32 channel)
>   {
> -	u8	i;
> -	u32	freq = 0;
> -
> -	for (i = 0; i < ch_freq_map_num; i++) {
> -		if (channel == ch_freq_map[i].channel) {
> -			freq = ch_freq_map[i].frequency;
> -				break;
> -		}
> -	}
> -	if (i == ch_freq_map_num)
> -		freq = 2412;
> -
> -	return freq;
> +	return ch_freq_map[channel - 1];
>   }

What if channel has wrong value? The old code returned some default 
value, but with new one we will hit OOB.





With regards,
Pavel Skripkin

Re: [PATCH 2/3] staging: r8188eu: refactor rtw_ch2freq()

[Michael Straube]

On 2/20/22 17:20, Pavel Skripkin wrote:
> Hi Michael,
> 
> On 2/20/22 18:48, Michael Straube wrote:
>> -static int ch_freq_map_num = ARRAY_SIZE(ch_freq_map);
>> -
>>   u32 rtw_ch2freq(u32 channel)
>>   {
>> -    u8    i;
>> -    u32    freq = 0;
>> -
>> -    for (i = 0; i < ch_freq_map_num; i++) {
>> -        if (channel == ch_freq_map[i].channel) {
>> -            freq = ch_freq_map[i].frequency;
>> -                break;
>> -        }
>> -    }
>> -    if (i == ch_freq_map_num)
>> -        freq = 2412;
>> -
>> -    return freq;
>> +    return ch_freq_map[channel - 1];
>>   }
> 
> What if channel has wrong value? The old code returned some default 
> value, but with new one we will hit OOB.
> 

Hi Pavel,

thanks for reviewing. Yeah, I thought about adding a check for channel
value between 1 and 14. But I did not add it because I think if this
function will ever be called with channel < 1 or channel > 14, then the
calling code must be wrong.

Would be nice to see what others think about this.

Thanks,
Michael

Re: [PATCH 2/3] staging: r8188eu: refactor rtw_ch2freq()

[Dan Carpenter]

On Sun, Feb 20, 2022 at 05:30:08PM +0100, Michael Straube wrote:
> On 2/20/22 17:20, Pavel Skripkin wrote:
> > Hi Michael,
> > 
> > On 2/20/22 18:48, Michael Straube wrote:
> > > -static int ch_freq_map_num = ARRAY_SIZE(ch_freq_map);
> > > -
> > >   u32 rtw_ch2freq(u32 channel)
> > >   {
> > > -    u8    i;
> > > -    u32    freq = 0;
> > > -
> > > -    for (i = 0; i < ch_freq_map_num; i++) {
> > > -        if (channel == ch_freq_map[i].channel) {
> > > -            freq = ch_freq_map[i].frequency;
> > > -                break;
> > > -        }
> > > -    }
> > > -    if (i == ch_freq_map_num)
> > > -        freq = 2412;
> > > -
> > > -    return freq;
> > > +    return ch_freq_map[channel - 1];
> > >   }
> > 
> > What if channel has wrong value? The old code returned some default
> > value, but with new one we will hit OOB.
> > 
> 
> Hi Pavel,
> 
> thanks for reviewing. Yeah, I thought about adding a check for channel
> value between 1 and 14. But I did not add it because I think if this
> function will ever be called with channel < 1 or channel > 14, then the
> calling code must be wrong.
> 
> Would be nice to see what others think about this.

I'm glad that Pavel noticed this change.  This is a risky thing and
should have been noted in the commit message.

Just from a review stand point it would be best to leave the original
behavior.

I have audited this change and I do not think it is safe.  It seems to
me that one way this can be controlled is via
module_param(rtw_channel, int, 0644); in
drivers/staging/r8188eu/os_dep/os_intfs.c.  I don't see any checking on
that.

regards,
dan carpenter

Re: [PATCH 1/3] staging: r8188eu: remove 5 GHz channels from ch_freq_map

[Greg KH]

On Sun, Feb 20, 2022 at 04:48:45PM +0100, Michael Straube wrote:
> This driver is for chips that operate only in the 2.4 GHz band.

How do we know that?

thanks,

greg k-h

Re: [PATCH 1/3] staging: r8188eu: remove 5 GHz channels from ch_freq_map

[Michael Straube]

On 2/21/22 18:13, Greg KH wrote:
> On Sun, Feb 20, 2022 at 04:48:45PM +0100, Michael Straube wrote:
>> This driver is for chips that operate only in the 2.4 GHz band.
> 
> How do we know that?
> 

The documentation that I've found for RTL8188EU and RTL8188ETV says
that both are 2.4 Ghz only chips.

Also the old rtl8188eu driver had a TODO item to remove code that is
only valid for 5 GHz.

So I thought this can be removed. Did I miss something?

regards,

Michael

Re: [PATCH 2/3] staging: r8188eu: refactor rtw_ch2freq()

[Michael Straube]

On 2/21/22 13:22, Dan Carpenter wrote:
> On Sun, Feb 20, 2022 at 05:30:08PM +0100, Michael Straube wrote:
>> On 2/20/22 17:20, Pavel Skripkin wrote:
>>> Hi Michael,
>>>
>>> On 2/20/22 18:48, Michael Straube wrote:
>>>> -static int ch_freq_map_num = ARRAY_SIZE(ch_freq_map);
>>>> -
>>>>    u32 rtw_ch2freq(u32 channel)
>>>>    {
>>>> -    u8    i;
>>>> -    u32    freq = 0;
>>>> -
>>>> -    for (i = 0; i < ch_freq_map_num; i++) {
>>>> -        if (channel == ch_freq_map[i].channel) {
>>>> -            freq = ch_freq_map[i].frequency;
>>>> -                break;
>>>> -        }
>>>> -    }
>>>> -    if (i == ch_freq_map_num)
>>>> -        freq = 2412;
>>>> -
>>>> -    return freq;
>>>> +    return ch_freq_map[channel - 1];
>>>>    }
>>>
>>> What if channel has wrong value? The old code returned some default
>>> value, but with new one we will hit OOB.
>>>
>>
>> Hi Pavel,
>>
>> thanks for reviewing. Yeah, I thought about adding a check for channel
>> value between 1 and 14. But I did not add it because I think if this
>> function will ever be called with channel < 1 or channel > 14, then the
>> calling code must be wrong.
>>
>> Would be nice to see what others think about this.
> 
> I'm glad that Pavel noticed this change.  This is a risky thing and
> should have been noted in the commit message.
> 
> Just from a review stand point it would be best to leave the original
> behavior.
>

Do you mean to leave the whole original code including the 5 GHz 
frequencies? Or returning a default value if we have a channel value < 1
or > 14?

I'm a bit confused now, because Greg asked how we know that the driver
is only for 2.4 GHz chips.

> I have audited this change and I do not think it is safe.  It seems to
> me that one way this can be controlled is via
> module_param(rtw_channel, int, 0644); in
> drivers/staging/r8188eu/os_dep/os_intfs.c.  I don't see any checking on
> that.
> 

Thank you Dan!

I missed that and blindly assumed the function will never be called
with channel values OOB. That was not good, sorry.

regards,

Michael

Re: [PATCH 1/3] staging: r8188eu: remove 5 GHz channels from ch_freq_map

[Larry Finger]

On 2/21/22 11:13, Greg KH wrote:
> On Sun, Feb 20, 2022 at 04:48:45PM +0100, Michael Straube wrote:
>> This driver is for chips that operate only in the 2.4 GHz band.
> 
> How do we know that?
> 
> thanks,
Realtek 
(https://www.realtek.com/en/products/communications-network-ics/item/rtl8188eus) 
states the following:

"The Realtek RTL8188EUS is an 802.11bgn 2.4G single-chip that integrates 
Wireless LAN (WLAN) and a network USB interface (USB 1.0/1.1/2.0 compatible) 
controller. It combines a WLAN MAC, a 1T1R capable WLAN baseband, and WLAN RF in 
a single chip. The RTL8188EUS provides a complete solution for high-throughput 
performance and low power consumption integrated wireless LAN devices."

The reason there are 5G snippets in the driver is because Realtek uses the base 
code to generate drivers for other chip families such as RTL8192DU that have 
radios for both 2.4 and 5G bands. The RTL81288EU and variants have only a single 
radio.

Larry

Re: [PATCH 2/3] staging: r8188eu: refactor rtw_ch2freq()

[Pavel Skripkin]

Hi Michael,

On 2/21/22 22:20, Michael Straube wrote:
>> I'm glad that Pavel noticed this change.  This is a risky thing and
>> should have been noted in the commit message.
>> 
>> Just from a review stand point it would be best to leave the original
>> behavior.
>>
> 
> Do you mean to leave the whole original code including the 5 GHz
> frequencies? Or returning a default value if we have a channel value < 1
> or > 14?
> 

IMO, your version is much cleaner than previous one. This table walk 
seems really unreasonable, since 5 GHz support is really redundant (I 
saw it in other thread)

I'd put just sanity check and return the default value from previous 
version. Maybe even wrapped with unlikely() if we sure, that in normal 
state we won't hit it ;)




With regards,
Pavel Skripkin

Re: [PATCH 2/3] staging: r8188eu: refactor rtw_ch2freq()

[Dan Carpenter]

On Mon, Feb 21, 2022 at 08:20:14PM +0100, Michael Straube wrote:
> On 2/21/22 13:22, Dan Carpenter wrote:
> > On Sun, Feb 20, 2022 at 05:30:08PM +0100, Michael Straube wrote:
> > > On 2/20/22 17:20, Pavel Skripkin wrote:

> > Just from a review stand point it would be best to leave the original
> > behavior.
> > 
> 
> Do you mean to leave the whole original code including the 5 GHz
> frequencies? Or returning a default value if we have a channel value < 1
> or > 14?

I meant what Pavel said:

	if (channel == 0 || channel > ARRAY_SIZE(ch_freq_map))
		return 2412;

	return ch_freq_map[channel - 1];

> 
> I'm a bit confused now, because Greg asked how we know that the driver
> is only for 2.4 GHz chips.
> 

Haven't you been deleting all the 5GHz code for some time now?  Greg
isn't saying you have to keep that if there is no reason.  Just put the
reason in the commit message and resend.

regards,
dan carpenter

Re: [PATCH 2/3] staging: r8188eu: refactor rtw_ch2freq()

[Dan Carpenter]

On Mon, Feb 21, 2022 at 11:54:50PM +0300, Pavel Skripkin wrote:
> Hi Michael,
> 
> On 2/21/22 22:20, Michael Straube wrote:
> > > I'm glad that Pavel noticed this change.  This is a risky thing and
> > > should have been noted in the commit message.
> > > 
> > > Just from a review stand point it would be best to leave the original
> > > behavior.
> > > 
> > 
> > Do you mean to leave the whole original code including the 5 GHz
> > frequencies? Or returning a default value if we have a channel value < 1
> > or > 14?
> > 
> 
> IMO, your version is much cleaner than previous one. This table walk seems
> really unreasonable, since 5 GHz support is really redundant (I saw it in
> other thread)
> 
> I'd put just sanity check and return the default value from previous
> version. Maybe even wrapped with unlikely() if we sure, that in normal state
> we won't hit it ;)

Adding likely()/unlikely() annotations hurt readability.  Do not do that
unless you have benchmark data to prove that it is worth it.  (Hint: it
is not worth it here).

regards,
dan carpenter

Re: [PATCH 1/3] staging: r8188eu: remove 5 GHz channels from ch_freq_map

[Greg KH]

On Mon, Feb 21, 2022 at 01:41:22PM -0600, Larry Finger wrote:
> On 2/21/22 11:13, Greg KH wrote:
> > On Sun, Feb 20, 2022 at 04:48:45PM +0100, Michael Straube wrote:
> > > This driver is for chips that operate only in the 2.4 GHz band.
> > 
> > How do we know that?
> > 
> > thanks,
> Realtek (https://www.realtek.com/en/products/communications-network-ics/item/rtl8188eus)
> states the following:
> 
> "The Realtek RTL8188EUS is an 802.11bgn 2.4G single-chip that integrates
> Wireless LAN (WLAN) and a network USB interface (USB 1.0/1.1/2.0 compatible)
> controller. It combines a WLAN MAC, a 1T1R capable WLAN baseband, and WLAN
> RF in a single chip. The RTL8188EUS provides a complete solution for
> high-throughput performance and low power consumption integrated wireless
> LAN devices."
> 
> The reason there are 5G snippets in the driver is because Realtek uses the
> base code to generate drivers for other chip families such as RTL8192DU that
> have radios for both 2.4 and 5G bands. The RTL81288EU and variants have only
> a single radio.

Ah, ok, that's good to know.  And should belong in the changelog text so
that we are not confused :)

thanks,

greg k-h