diff mbox series

[1/2] staging: rtl8192e: Remove empty Array Rtl8192PciERadioC_Array

Message ID d8f0e8c7ee6dd820b56636db8d891012aa3ab263.1678051460.git.philipp.g.hortmann@gmail.com
State New
Headers
Series staging: rtl8192e: Removing empty arrays ..RadioC_A.. and ..ERadioD_A.. |

Commit Message

Philipp Hortmann March 5, 2023, 10:33 p.m. UTC 
  Remove empty array Rtl8192PciERadioC_Array and the code where it is used
because it is dead code.

Signed-off-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>
---
 drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c | 12 ------------
 drivers/staging/rtl8192e/rtl8192e/r8192E_phy.h |  2 --
 drivers/staging/rtl8192e/rtl8192e/table.c      |  3 ---
 drivers/staging/rtl8192e/rtl8192e/table.h      |  2 --
 4 files changed, 19 deletions(-)

Comments

Dan Carpenter March 6, 2023, 9:12 a.m. UTC | #1 
On Sun, Mar 05, 2023 at 11:33:05PM +0100, Philipp Hortmann wrote:
> Remove empty array Rtl8192PciERadioC_Array and the code where it is used
> because it is dead code.
> 
> Signed-off-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>
> ---
>  drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c | 12 ------------
>  drivers/staging/rtl8192e/rtl8192e/r8192E_phy.h |  2 --
>  drivers/staging/rtl8192e/rtl8192e/table.c      |  3 ---
>  drivers/staging/rtl8192e/rtl8192e/table.h      |  2 --
>  4 files changed, 19 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c
> index 35ca01ab65ff..fe0ef52c163a 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c
> @@ -649,18 +649,6 @@ u8 rtl92e_config_rf_path(struct net_device *dev, enum rf90_radio_path eRFPath)
>  					  bMask12Bits,
>  					  Rtl819XRadioB_Array[i+1]);
>  
> -		}
> -		break;
> -	case RF90_PATH_C:
> -		for (i = 0; i < RadioC_ArrayLength; i += 2) {
> -			if (Rtl819XRadioC_Array[i] == 0xfe) {
> -				msleep(100);
> -				continue;
> -			}
> -			rtl92e_set_rf_reg(dev, eRFPath, Rtl819XRadioC_Array[i],
> -					  bMask12Bits,
> -					  Rtl819XRadioC_Array[i+1]);
> -

Why is this dead code?  So far as I can see "== 0xfe" is always false
so this calls rtl92e_set_rf_reg() on every iteration through the loop.
It only does one iteration through the loop.

Is it dead code because case RF90_PATH_C is always false?  If so then
that needs to be explained in the commit message.

regards,
dan carpenter
Philipp Hortmann March 6, 2023, 8:43 p.m. UTC | #2 
On 3/6/23 10:12, Dan Carpenter wrote:
> On Sun, Mar 05, 2023 at 11:33:05PM +0100, Philipp Hortmann wrote:
>> Remove empty array Rtl8192PciERadioC_Array and the code where it is used
>> because it is dead code.
>>
>> Signed-off-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>
>> ---
>>   drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c | 12 ------------
>>   drivers/staging/rtl8192e/rtl8192e/r8192E_phy.h |  2 --
>>   drivers/staging/rtl8192e/rtl8192e/table.c      |  3 ---
>>   drivers/staging/rtl8192e/rtl8192e/table.h      |  2 --
>>   4 files changed, 19 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c
>> index 35ca01ab65ff..fe0ef52c163a 100644
>> --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c
>> +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c
>> @@ -649,18 +649,6 @@ u8 rtl92e_config_rf_path(struct net_device *dev, enum rf90_radio_path eRFPath)
>>   					  bMask12Bits,
>>   					  Rtl819XRadioB_Array[i+1]);
>>   
>> -		}
>> -		break;
>> -	case RF90_PATH_C:
>> -		for (i = 0; i < RadioC_ArrayLength; i += 2) {
>> -			if (Rtl819XRadioC_Array[i] == 0xfe) {
>> -				msleep(100);
>> -				continue;
>> -			}
>> -			rtl92e_set_rf_reg(dev, eRFPath, Rtl819XRadioC_Array[i],
>> -					  bMask12Bits,
>> -					  Rtl819XRadioC_Array[i+1]);
>> -
> 
> Why is this dead code?  So far as I can see "== 0xfe" is always false
> so this calls rtl92e_set_rf_reg() on every iteration through the loop.
> It only does one iteration through the loop.
> 
> Is it dead code because case RF90_PATH_C is always false?  If so then
> that needs to be explained in the commit message.
> 
> regards,
> dan carpenter
> 

Hi Dan,

thanks for the review. Here some answers to your questions:

With patch: "[PATCH] staging: rtl8192e: Change filename r8192E_hwimg.x 
to table.x" I changed the filename of r8192E_hwimg.c to table.c and 
r8192E_hwimg.h to table.h to adapt filenames from 
drivers/net/wireless/realtek/rtlwifi rtl8192ee and rtl8192se. Task is 
from TODO file.

The explanation from the cover letter of this patch series was:

Rtl8192PciERadioC_Array and Rtl8192PciERadioD_Array contain only two
values set to 0. Reviewing the other Arrays in table.c and looking into
other realtek drivers (rtl8192se and rtl8192ee) this arrays are not
containing valid data.

Here some more examples of my thoughts:

A valid filled array is looking like this:
u32 Rtl8192PciERadioA_Array[RadioA_ArrayLengthPciE] = {
	0x019, 0x00000003,
	0x000, 0x000000bf,
	0x001, 0x00000ee0,
	...
	over 100 lines but no 0x000, 0x00000000,
	...
	0x004, 0x00000975,
	0x007, 0x00000700,
};

u32 Rtl8192PciERadioB_Array[RadioB_ArrayLengthPciE] = {
	0x019, 0x00000003,
	0x000, 0x000000bf,
	0x001, 0x000006e0,
	...
	over 30 lines but no 0x000, 0x00000000,
	...
	0x004, 0x00000975,
	0x007, 0x00000700,
};

The empty (it is not empty but compared to the ones filled with data it 
is empty) one is looking like this:
u32 Rtl8192PciERadioC_Array[RadioC_ArrayLengthPciE] = {
	0x0,  };

Looking into other cleaned up drivers from the same family:
Example: drivers/net/wireless/realtek/rtlwifi/rtl8192se
Arrays RadioA and RadioB are filled RadioC and RadioD do not exist.
Example: drivers/net/wireless/realtek/rtlwifi/rtl8192ee
Arrays RadioA and RadioB are filled RadioC and RadioD do not exist.
Example: drivers/net/wireless/realtek/rtlwifi/rtl8192de
Arrays RadioA and RadioB are filled RadioC and RadioD do not exist.


In Example: drivers/net/wireless/realtek/rtlwifi/rtl8192cu
I can find a RadioB Array that is just filled with one 0 which is odd:
#define RTL8192CURADIOB_1TARRAYLENGTH	1

u32 RTL8192CU_RADIOB_1TARRAY[RTL8192CURADIOB_1TARRAYLENGTH] = {
	0x0,
};

Here it is written to a variable:

		rtlphy->hwparam_tables[RADIOB_1T].length =
			RTL8192CURADIOB_1TARRAYLENGTH;
		rtlphy->hwparam_tables[RADIOB_1T].pdata =
			RTL8192CU_RADIOB_1TARRAY;

Written to an another variable:

		radiob_arraylen = rtlphy->hwparam_tables[RADIOB_1T].length;
		radiob_array_table = rtlphy->hwparam_tables[RADIOB_1T].pdata;


And then accessed after with i + 1 the element that is random/undefined.


	case RF90_PATH_B:
		for (i = 0; i < radiob_arraylen; i = i + 2) {
			rtl_rfreg_delay(hw, rfpath, radiob_array_table[i],
					RFREG_OFFSET_MASK,
					radiob_array_table[i + 1]);
		}

May be another patch.

I hope this can convince you that Arrays for RadioX that do only contain 
one or two 0 are not in use.

Please let me know your thoughts.

Thanks

Philipp
Dan Carpenter March 7, 2023, 7:49 a.m. UTC | #3 
On Mon, Mar 06, 2023 at 09:43:50PM +0100, Philipp Hortmann wrote:
> Hi Dan,
> 
> thanks for the review. Here some answers to your questions:
> 
> With patch: "[PATCH] staging: rtl8192e: Change filename r8192E_hwimg.x to
> table.x" I changed the filename of r8192E_hwimg.c to table.c and
> r8192E_hwimg.h to table.h to adapt filenames from
> drivers/net/wireless/realtek/rtlwifi rtl8192ee and rtl8192se. Task is from
> TODO file.
> 
> The explanation from the cover letter of this patch series was:

I never read cover letters.  No one is ever going to see the cover
letter when they look through `git log -p` so I don't see the point...

> 
> Rtl8192PciERadioC_Array and Rtl8192PciERadioD_Array contain only two
> values set to 0. Reviewing the other Arrays in table.c and looking into
> other realtek drivers (rtl8192se and rtl8192ee) this arrays are not
> containing valid data.
> 
> Here some more examples of my thoughts:
> 
> A valid filled array is looking like this:
> u32 Rtl8192PciERadioA_Array[RadioA_ArrayLengthPciE] = {
> 	0x019, 0x00000003,
> 	0x000, 0x000000bf,
> 	0x001, 0x00000ee0,
> 	...
> 	over 100 lines but no 0x000, 0x00000000,
> 	...
> 	0x004, 0x00000975,
> 	0x007, 0x00000700,
> };
> 
> u32 Rtl8192PciERadioB_Array[RadioB_ArrayLengthPciE] = {
> 	0x019, 0x00000003,
> 	0x000, 0x000000bf,
> 	0x001, 0x000006e0,
> 	...
> 	over 30 lines but no 0x000, 0x00000000,
> 	...
> 	0x004, 0x00000975,
> 	0x007, 0x00000700,
> };
> 
> The empty (it is not empty but compared to the ones filled with data it is
> empty) one is looking like this:
> u32 Rtl8192PciERadioC_Array[RadioC_ArrayLengthPciE] = {
> 	0x0,  };
> 
> Looking into other cleaned up drivers from the same family:
> Example: drivers/net/wireless/realtek/rtlwifi/rtl8192se
> Arrays RadioA and RadioB are filled RadioC and RadioD do not exist.
> Example: drivers/net/wireless/realtek/rtlwifi/rtl8192ee
> Arrays RadioA and RadioB are filled RadioC and RadioD do not exist.
> Example: drivers/net/wireless/realtek/rtlwifi/rtl8192de
> Arrays RadioA and RadioB are filled RadioC and RadioD do not exist.
> 
> 
> In Example: drivers/net/wireless/realtek/rtlwifi/rtl8192cu
> I can find a RadioB Array that is just filled with one 0 which is odd:
> #define RTL8192CURADIOB_1TARRAYLENGTH	1
> 
> u32 RTL8192CU_RADIOB_1TARRAY[RTL8192CURADIOB_1TARRAYLENGTH] = {
> 	0x0,
> };
> 
> Here it is written to a variable:
> 
> 		rtlphy->hwparam_tables[RADIOB_1T].length =
> 			RTL8192CURADIOB_1TARRAYLENGTH;
> 		rtlphy->hwparam_tables[RADIOB_1T].pdata =
> 			RTL8192CU_RADIOB_1TARRAY;
> 
> Written to an another variable:
> 
> 		radiob_arraylen = rtlphy->hwparam_tables[RADIOB_1T].length;
> 		radiob_array_table = rtlphy->hwparam_tables[RADIOB_1T].pdata;
> 
> 
> And then accessed after with i + 1 the element that is random/undefined.
> 
> 
> 	case RF90_PATH_B:
> 		for (i = 0; i < radiob_arraylen; i = i + 2) {
> 			rtl_rfreg_delay(hw, rfpath, radiob_array_table[i],
> 					RFREG_OFFSET_MASK,
> 					radiob_array_table[i + 1]);

You make a really compelling argument that the code in rtl8192cu
should be deleted, but that's a different driver from what we're
discussing here.

> 		}
> 
> May be another patch.
> 
> I hope this can convince you that Arrays for RadioX that do only contain one
> or two 0 are not in use.

I can easily be convinced of that, but that's not what the commit
message says.  The commit message says the array is "empty" and that
you are deleting "dead code".  When actually you're deleting a call to
rtl92e_set_rf_reg().  So the commit message is just wrong about the
facts.

What you need to do is figure out what the call to rtl92e_set_rf_reg()
does and why it's not required.

I don't have a problem with this *patch* but the commit message says
things which are false.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c
index 35ca01ab65ff..fe0ef52c163a 100644
--- a/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c
+++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c
@@ -649,18 +649,6 @@  u8 rtl92e_config_rf_path(struct net_device *dev, enum rf90_radio_path eRFPath)
 					  bMask12Bits,
 					  Rtl819XRadioB_Array[i+1]);
 
-		}
-		break;
-	case RF90_PATH_C:
-		for (i = 0; i < RadioC_ArrayLength; i += 2) {
-			if (Rtl819XRadioC_Array[i] == 0xfe) {
-				msleep(100);
-				continue;
-			}
-			rtl92e_set_rf_reg(dev, eRFPath, Rtl819XRadioC_Array[i],
-					  bMask12Bits,
-					  Rtl819XRadioC_Array[i+1]);
-
 		}
 		break;
 	case RF90_PATH_D:
diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.h b/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.h
index 561a4c874fe3..6772ed300591 100644
--- a/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.h
+++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_phy.h
@@ -14,7 +14,6 @@ 
 #define RadioA_ArrayLength			RadioA_ArrayLengthPciE
 #define RadioB_ArrayLength			RadioB_ArrayLengthPciE
 #define MACPHY_Array_PGLength			MACPHY_Array_PGLengthPciE
-#define RadioC_ArrayLength			RadioC_ArrayLengthPciE
 #define RadioD_ArrayLength			RadioD_ArrayLengthPciE
 #define PHY_REG_1T2RArrayLength			PHY_REG_1T2RArrayLengthPciE
 
@@ -22,7 +21,6 @@ 
 #define Rtl819XMACPHY_Array			Rtl8192PciEMACPHY_Array
 #define Rtl819XRadioA_Array			Rtl8192PciERadioA_Array
 #define Rtl819XRadioB_Array			Rtl8192PciERadioB_Array
-#define Rtl819XRadioC_Array			Rtl8192PciERadioC_Array
 #define Rtl819XRadioD_Array			Rtl8192PciERadioD_Array
 #define Rtl819XAGCTAB_Array			Rtl8192PciEAGCTAB_Array
 #define Rtl819XPHY_REG_1T2RArray		Rtl8192PciEPHY_REG_1T2RArray
diff --git a/drivers/staging/rtl8192e/rtl8192e/table.c b/drivers/staging/rtl8192e/rtl8192e/table.c
index d04608d99cbf..4e97ad4b8fd7 100644
--- a/drivers/staging/rtl8192e/rtl8192e/table.c
+++ b/drivers/staging/rtl8192e/rtl8192e/table.c
@@ -325,9 +325,6 @@  u32 Rtl8192PciERadioB_Array[RadioB_ArrayLengthPciE] = {
 	0x007, 0x00000700,
 };
 
-u32 Rtl8192PciERadioC_Array[RadioC_ArrayLengthPciE] = {
-	0x0,  };
-
 u32 Rtl8192PciERadioD_Array[RadioD_ArrayLengthPciE] = {
 	0x0, };
 
diff --git a/drivers/staging/rtl8192e/rtl8192e/table.h b/drivers/staging/rtl8192e/rtl8192e/table.h
index 177fe2173430..981e8b57d3de 100644
--- a/drivers/staging/rtl8192e/rtl8192e/table.h
+++ b/drivers/staging/rtl8192e/rtl8192e/table.h
@@ -17,8 +17,6 @@  extern u32 Rtl8192PciEPHY_REG_1T2RArray[PHY_REG_1T2RArrayLengthPciE];
 extern u32 Rtl8192PciERadioA_Array[RadioA_ArrayLengthPciE];
 #define RadioB_ArrayLengthPciE 78
 extern u32 Rtl8192PciERadioB_Array[RadioB_ArrayLengthPciE];
-#define RadioC_ArrayLengthPciE 2
-extern u32 Rtl8192PciERadioC_Array[RadioC_ArrayLengthPciE];
 #define RadioD_ArrayLengthPciE 2
 extern u32 Rtl8192PciERadioD_Array[RadioD_ArrayLengthPciE];
 #define MACPHY_ArrayLengthPciE 18