Received-SPF: pass (google.com: domain of linux-wireless+bounces-4916-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f19a1e6c-fd56-45b7-9936-a1a72d1988ad@gmail.com>
Date: Tue, 19 Mar 2024 18:28:41 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 00/12] wifi: rtlwifi: Add new rtl8192du driver
Content-Language: en-US
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
 "s.l-h@gmx.de" <s.l-h@gmx.de>, "chewitt@libreelec.tv" <chewitt@libreelec.tv>
References: <5c23149c-1487-438d-bb37-69e2dd8173dc@gmail.com>
 <2280b6c991fa09e66506088441f63790d092e343.camel@realtek.com>
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <2280b6c991fa09e66506088441f63790d092e343.camel@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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