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