mbox series

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

Message ID 5c23149c-1487-438d-bb37-69e2dd8173dc@gmail.com (mailing list archive)
Headers show
Series wifi: rtlwifi: Add new rtl8192du driver | expand

Message

Bitterblue Smith March 17, 2024, 6:44 p.m. UTC
These patches add a driver for the RTL8192DU chip.

Patch 1 fixes a bug in rtl8192de. Patch 2 depends on it.

Patch 2 moves a good portion of the code from rtl8192de to a new module
rtl8192d-common.

Patch 3 makes some adjustments to rtl8192d-common.

The remaining patches add the new rtl8192du driver.

Power saving is disabled by default because it's not working yet.

Dual MAC chips are not tested.

Until Ping-Ke Shih will add the firmware to the linux-firmware
repository, the driver can be tested with the firmware from Larry
Finger's repository:
https://github.com/lwfinger/rtl8192du/blob/master/rtl8192dufw.bin

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.

Bitterblue Smith (12):
  wifi: rtlwifi: rtl8192de: Fix 5 GHz TX power
  wifi: rtlwifi: Move code from rtl8192de to rtl8192d-common
  wifi: rtlwifi: Adjust rtl8192d-common for USB
  wifi: rtlwifi: Add rtl8192du/table.{c,h}
  wifi: rtlwifi: Add rtl8192du/hw.{c,h}
  wifi: rtlwifi: Add rtl8192du/phy.{c,h}
  wifi: rtlwifi: Add rtl8192du/trx.{c,h}
  wifi: rtlwifi: Add rtl8192du/rf.{c,h}
  wifi: rtlwifi: Add rtl8192du/fw.{c,h} and rtl8192du/led.{c,h}
  wifi: rtlwifi: Add rtl8192du/dm.{c,h}
  wifi: rtlwifi: Add rtl8192du/sw.{c,h}
  wifi: rtlwifi: Enable the new rtl8192du driver

 drivers/net/wireless/realtek/rtlwifi/Kconfig  |   18 +
 drivers/net/wireless/realtek/rtlwifi/Makefile |    2 +
 drivers/net/wireless/realtek/rtlwifi/efuse.c  |    2 +-
 drivers/net/wireless/realtek/rtlwifi/efuse.h  |    2 +-
 .../realtek/rtlwifi/rtl8192d/Makefile         |   11 +
 .../rtlwifi/{rtl8192de => rtl8192d}/def.h     |    0
 .../realtek/rtlwifi/rtl8192d/dm_common.c      | 1079 ++++++
 .../realtek/rtlwifi/rtl8192d/dm_common.h      |  100 +
 .../realtek/rtlwifi/rtl8192d/fw_common.c      |  390 +++
 .../realtek/rtlwifi/rtl8192d/fw_common.h      |   49 +
 .../realtek/rtlwifi/rtl8192d/hw_common.c      | 1218 +++++++
 .../realtek/rtlwifi/rtl8192d/hw_common.h      |   24 +
 .../wireless/realtek/rtlwifi/rtl8192d/main.c  |    9 +
 .../realtek/rtlwifi/rtl8192d/phy_common.c     |  830 +++++
 .../realtek/rtlwifi/rtl8192d/phy_common.h     |   96 +
 .../rtlwifi/{rtl8192de => rtl8192d}/reg.h     |  155 +-
 .../realtek/rtlwifi/rtl8192d/rf_common.c      |  378 ++
 .../realtek/rtlwifi/rtl8192d/rf_common.h      |   13 +
 .../realtek/rtlwifi/rtl8192d/trx_common.c     |  516 +++
 .../realtek/rtlwifi/rtl8192d/trx_common.h     |  442 +++
 .../wireless/realtek/rtlwifi/rtl8192de/dm.c   | 1072 +-----
 .../wireless/realtek/rtlwifi/rtl8192de/dm.h   |   91 +-
 .../wireless/realtek/rtlwifi/rtl8192de/fw.c   |  375 +-
 .../wireless/realtek/rtlwifi/rtl8192de/fw.h   |   37 -
 .../wireless/realtek/rtlwifi/rtl8192de/hw.c   | 1168 +------
 .../wireless/realtek/rtlwifi/rtl8192de/hw.h   |   11 -
 .../wireless/realtek/rtlwifi/rtl8192de/led.c  |    2 +-
 .../wireless/realtek/rtlwifi/rtl8192de/phy.c  |  918 +----
 .../wireless/realtek/rtlwifi/rtl8192de/phy.h  |   59 +-
 .../wireless/realtek/rtlwifi/rtl8192de/rf.c   |  375 +-
 .../wireless/realtek/rtlwifi/rtl8192de/rf.h   |    5 -
 .../wireless/realtek/rtlwifi/rtl8192de/sw.c   |   12 +-
 .../wireless/realtek/rtlwifi/rtl8192de/trx.c  |  515 +--
 .../wireless/realtek/rtlwifi/rtl8192de/trx.h  |  433 ---
 .../realtek/rtlwifi/rtl8192du/Makefile        |   13 +
 .../wireless/realtek/rtlwifi/rtl8192du/dm.c   |  125 +
 .../wireless/realtek/rtlwifi/rtl8192du/dm.h   |   10 +
 .../wireless/realtek/rtlwifi/rtl8192du/fw.c   |  109 +
 .../wireless/realtek/rtlwifi/rtl8192du/fw.h   |    9 +
 .../wireless/realtek/rtlwifi/rtl8192du/hw.c   | 1244 +++++++
 .../wireless/realtek/rtlwifi/rtl8192du/hw.h   |   24 +
 .../wireless/realtek/rtlwifi/rtl8192du/led.c  |   10 +
 .../wireless/realtek/rtlwifi/rtl8192du/led.h  |    9 +
 .../wireless/realtek/rtlwifi/rtl8192du/phy.c  | 3055 +++++++++++++++++
 .../wireless/realtek/rtlwifi/rtl8192du/phy.h  |   32 +
 .../wireless/realtek/rtlwifi/rtl8192du/rf.c   |  236 ++
 .../wireless/realtek/rtlwifi/rtl8192du/rf.h   |   11 +
 .../wireless/realtek/rtlwifi/rtl8192du/sw.c   |  311 ++
 .../wireless/realtek/rtlwifi/rtl8192du/sw.h   |   12 +
 .../realtek/rtlwifi/rtl8192du/table.c         | 1675 +++++++++
 .../realtek/rtlwifi/rtl8192du/table.h         |   30 +
 .../wireless/realtek/rtlwifi/rtl8192du/trx.c  |  380 ++
 .../wireless/realtek/rtlwifi/rtl8192du/trx.h  |   60 +
 drivers/net/wireless/realtek/rtlwifi/usb.c    |    3 +
 drivers/net/wireless/realtek/rtlwifi/wifi.h   |    6 +
 55 files changed, 12788 insertions(+), 4983 deletions(-)
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/Makefile
 rename drivers/net/wireless/realtek/rtlwifi/{rtl8192de => rtl8192d}/def.h (100%)
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/dm_common.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/dm_common.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/fw_common.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/fw_common.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/hw_common.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/hw_common.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/main.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.h
 rename drivers/net/wireless/realtek/rtlwifi/{rtl8192de => rtl8192d}/reg.h (91%)
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/rf_common.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/rf_common.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/trx_common.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192d/trx_common.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/Makefile
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/dm.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/dm.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/fw.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/fw.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/hw.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/hw.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/led.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/led.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/phy.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/phy.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/rf.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/rf.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/sw.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/sw.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/table.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/table.h
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/trx.c
 create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/trx.h

Comments

Ping-Ke Shih March 19, 2024, 9:18 a.m. UTC | #1
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:

And, sparse/smatch report 

  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/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
  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
  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]
  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]
  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
Bitterblue Smith March 19, 2024, 4:28 p.m. UTC | #2
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 
> 
>
Ping-Ke Shih March 20, 2024, 12:57 a.m. UTC | #3
> >   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;
>         }
> 

Duplicate of main statements 'temp_cck = ....' isn't good. I prefer

bool need_lock = rtlpriv->rtlhal.interfaceindex == 1 &&
                 rtlpriv->rtlhal.interface == INTF_PCI;

if (need_lock)
	spin_lock_irqsave(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);

	temp_cck = rtl_get_bbreg(hw, RCCK0_TXFILTER2, MASKDWORD) & MASKCCK;

if (need_lock)
	spin_unlock_irqrestore(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);


But, I wonder why sparse doesn't complain original code (before your patchset)
that used static inline already. Can we keep original style?

Ping-Ke
Bitterblue Smith March 20, 2024, 1:57 p.m. UTC | #4
On 20/03/2024 02:57, Ping-Ke Shih wrote:
> 
>>>   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;
>>         }
>>
> 
> Duplicate of main statements 'temp_cck = ....' isn't good. I prefer
> 
> bool need_lock = rtlpriv->rtlhal.interfaceindex == 1 &&
>                  rtlpriv->rtlhal.interface == INTF_PCI;
> 
> if (need_lock)
> 	spin_lock_irqsave(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);
> 
> 	temp_cck = rtl_get_bbreg(hw, RCCK0_TXFILTER2, MASKDWORD) & MASKCCK;
> 
> if (need_lock)
> 	spin_unlock_irqrestore(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);
> 

Even this doesn't work. I get the same warning as before.

> 
> But, I wonder why sparse doesn't complain original code (before your patchset)
> that used static inline already. Can we keep original style?
> 
I found the reason. In patch 2/12 I moved the two functions
from rtl8192de/phy.h to rtl8192d/phy_common.h. This should be
harmless. But I also deleted these lines from the end of
rtl8192de/phy.h:

void rtl92d_acquire_cckandrw_pagea_ctl(struct ieee80211_hw *hw,
				       unsigned long *flag);
void rtl92d_release_cckandrw_pagea_ctl(struct ieee80211_hw *hw,
				       unsigned long *flag);

They seemed pointless. If I add them to phy_common.h all the
warnings about locks go away. I will do this for v3.