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

[Dan Carpenter]

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