Re: [PATCH v2 00/12] wifi: rtlwifi: Add new rtl8192du driver

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 19/03/2024 11:18, Ping-Ke Shih wrote:
> On Sun, 2024-03-17 at 20:44 +0200, Bitterblue Smith wrote:
>>
>> v2:
>>  - Add cover letter.
>>  - Implement feedback.
>>  - Fix more problems reported by checkpatch.
>>  - Split the new driver into several patches (4-12) for easier
>>    reviewing.
>>  - More details about the changes can be found in each patch.
>>
>>
> 
> I have not started reviewing yet, but compiler reports errors/warnings:
> 
>   drivers/net/wireless/realtek/rtlwifi/rtl8192du/phy.c: In function 'rtl92d_phy_set_poweron':
>   drivers/net/wireless/realtek/rtlwifi/rtl8192du/phy.c:3055:1: error: expected declaration or 
> statement at end of input
>    3055 | }
>         | ^
>   At top level:
> 

Ahh, that's embarrassing. checkpatch said "else" after "break"
is not useful, so I removed the else. I didn't notice the open
brace after the if... and forgot to compile again before
format-patch. Sorry about that.

> And, sparse/smatch report 
> 

I installed sparse and smatch now.

>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.h:30:17: warning: 'channel_all' defined
> but not used [-Wunused-const-variable=]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192de/../rtl8192d/phy_common.h:30:17: warning:
> 'channel_all' defined but not used [-Wunused-const-variable=]

I see now that channel_all is only used in phy_common.c.
I will move it there.

>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.h:60:39: warning: context imbalance in
> 'rtl92d_bandtype_2_4G' - unexpected unlock
>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.h:60:39: warning: context imbalance in
> 'rtl92d_dm_false_alarm_counter_statistics' - unexpected unlock
>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.h:60:39: warning: context imbalance in
> 'rtl92d_dm_cck_packet_detection_thresh' - unexpected unlock

These look like false positives. Every unlock is preceded by
a lock. I found a suggestion to annotate the functions with
"__acquires(...)" and "__releases(...)" to quiet these warnings,
but that didn't do anything. I can only fix it by copying the
contents of rtl92d_acquire_cckandrw_pagea_ctl() and
rtl92d_release_cckandrw_pagea_ctl() to the eight places where
they are called, and duplicating the code that needs locking:

	if (rtlpriv->rtlhal.interfaceindex == 1 &&
	    rtlpriv->rtlhal.interface == INTF_PCI) {
		spin_lock_irqsave(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);
		temp_cck = rtl_get_bbreg(hw, RCCK0_TXFILTER2,
					 MASKDWORD) & MASKCCK;
		spin_unlock_irqrestore(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);
	} else {
		temp_cck = rtl_get_bbreg(hw, RCCK0_TXFILTER2,
					 MASKDWORD) & MASKCCK;
	}

>   drivers/net/wireless/realtek/rtlwifi/rtl8192de/../rtl8192d/phy_common.h:30:17: warning:
> 'channel_all' defined but not used [-Wunused-const-variable=]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192du/../rtl8192d/phy_common.h:30:17: warning:
> 'channel_all' defined but not used [-Wunused-const-variable=]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.h:30:17: warning: 'channel_all' defined
> but not used [-Wunused-const-variable=]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192de/../rtl8192d/phy_common.h:30:17: warning:
> 'channel_all' defined but not used [-Wunused-const-variable=]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192de/../rtl8192d/phy_common.h:60:39: warning: context
> imbalance in 'rtl92d_phy_set_bw_mode' - unexpected unlock
>   drivers/net/wireless/realtek/rtlwifi/rtl8192de/../rtl8192d/phy_common.h:60:39: warning: context
> imbalance in '_rtl92d_phy_reload_imr_setting' - unexpected unlock
>   drivers/net/wireless/realtek/rtlwifi/rtl8192de/../rtl8192d/phy_common.h:60:39: warning: context
> imbalance in 'rtl92d_phy_iq_calibrate' - unexpected unlock
>   drivers/net/wireless/realtek/rtlwifi/rtl8192du/../rtl8192d/phy_common.h:30:17: warning:
> 'channel_all' defined but not used [-Wunused-const-variable=]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192de/../rtl8192d/phy_common.h:30:17: warning:
> 'channel_all' defined but not used [-Wunused-const-variable=]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192de/../rtl8192d/phy_common.h:30:17: warning:
> 'channel_all' defined but not used [-Wunused-const-variable=]

>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.c:91:16: warning: context imbalance in
> 'rtl92d_phy_query_rf_reg' - different lock contexts for basic block
>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.c:98:6: warning: context imbalance in
> 'rtl92d_phy_set_rf_reg' - different lock contexts for basic block

This looks like sparse is getting confused. I fixed it
by putting both lock and unlock inside the same if,
like above.

>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/rf_common.c:8:6: warning: no previous prototype for
> 'rtl92d_phy_rf6052_set_bandwidth' [-Wmissing-prototypes]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/rf_common.c:46:6: warning: no previous prototype for
> 'rtl92d_phy_rf6052_set_cck_txpower' [-Wmissing-prototypes]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/rf_common.c:362:6: warning: no previous prototype
> for 'rtl92d_phy_rf6052_set_ofdm_txpower' [-Wmissing-prototypes]

It was missing #include "rf_common.h".

>   drivers/net/wireless/realtek/rtlwifi/rtl8192du/phy.c:3055:1: error: expected declaration or
> statement at end of input
>   drivers/net/wireless/realtek/rtlwifi/rtl8192du/phy.c:2957:6: warning: 'rtl92du_phy_init_pa_bias'
> defined but not used [-Wunused-function]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192du/phy.c:2923:6: warning: 'rtl92d_phy_check_poweroff'
> defined but not used [-Wunused-function]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192du/phy.c:2694:6: warning:
> 'rtl92d_update_bbrf_configuration' defined but not used [-Wunused-function]

These warnings seem to be caused by that stray open brace I mentioned.

>   drivers/net/wireless/realtek/rtlwifi/rtl8192du/../rtl8192d/phy_common.h:30:17: warning:
> 'channel_all' defined but not used [-Wunused-const-variable=]
>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/rf_common.c:8:6: warning: symbol
> 'rtl92d_phy_rf6052_set_bandwidth' was not declared. Should it be static?
>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/rf_common.c:46:6: warning: symbol
> 'rtl92d_phy_rf6052_set_cck_txpower' was not declared. Should it be static?
>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/rf_common.c:362:6: warning: symbol
> 'rtl92d_phy_rf6052_set_ofdm_txpower' was not declared. Should it be static?
>   drivers/net/wireless/realtek/rtlwifi/rtl8192de/../rtl8192d/phy_common.h:30:17: warning:
> 'channel_all' defined but not used [-Wunused-const-variable=]
> 
> Please correct them. I will wait for your v3. 
> 
> Ping-Ke 
> 
>