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

[Philipp Hortmann]

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@xxxxxxxxx>
> > ---
> >   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