[RFC] rtlwifi: Partial revert of 6539306b

[RFC] rtlwifi: Partial revert of 6539306b

[Larry Finger]

Stanislaw,

When I tested commit 6539306, I did not notice that loading an out-of-tree
module turns off lockdep testing in kernel 3.2. For that reason, I missed
the kernel WARNING shown below:

As I stated earlier, my understanding of locking may not up to the task
of fixing this problem. I first tried using one mutex to replace ips_lock
and a second to replace lps_lock. That still got a warning.

The solution below, which partially reverts 6539306 and restores one of the
spinlocks, does not generate any lockdep warnings. This solution also has
the advantage that it this locking is only used when the NIC is turned on.
I am, however, ready to test any solution that eliminates both spinlocks.

Larry

[   84.168146] ------------[ cut here ]------------
[   84.168155] WARNING: at kernel/mutex.c:198 mutex_lock_nested+0x309/0x310()
[   84.168158] Hardware name: HP Pavilion dv2700 Notebook PC
[   84.168161] Modules linked in: nfs lockd auth_rpcgss nfs_acl sunrpc af_packet cpufreq_conservative cpufreq_userspace cpufreq_powersave powernow_k8 mperf e
xt3 jbd ide_cd_mod cdrom snd_hda_codec_conexant arc4 rtl8192ce ide_pci_generic rtl8192c_common rtlwifi snd_hda_intel mac80211 snd_hda_codec snd_pcm snd_timer
 amd74xx ide_core cfg80211 k8temp snd joydev soundcore hwmon battery forcedeth i2c_nforce2 sg rfkill ac serio_raw snd_page_alloc button video i2c_core ipv6 a
utofs4 ext4 mbcache jbd2 crc16 sd_mod ahci ohci_hcd libahci libata scsi_mod ehci_hcd usbcore usb_common fan processor thermal
[   84.168231] Pid: 1218, comm: kworker/u:2 Not tainted 3.2.0-rc5-wl+ #155
[   84.168234] Call Trace:
[   84.168240]  [<ffffffff81048aaa>] warn_slowpath_common+0x7a/0xb0
[   84.168245]  [<ffffffff81048af5>] warn_slowpath_null+0x15/0x20
[   84.168249]  [<ffffffff813811f9>] mutex_lock_nested+0x309/0x310
[   84.168269]  [<ffffffffa00793f9>] ? rtl_ips_nic_on+0x49/0xb0 [rtlwifi]
[   84.168277]  [<ffffffffa00793f9>] rtl_ips_nic_on+0x49/0xb0 [rtlwifi]
[   84.168284]  [<ffffffffa007ab85>] rtl_pci_tx+0x1b5/0x560 [rtlwifi]
[   84.168291]  [<ffffffffa007635a>] rtl_op_tx+0x9a/0xa0 [rtlwifi]
[   84.168359]  [<ffffffffa043cf51>] __ieee80211_tx+0x181/0x2b0 [mac80211]
[   84.168375]  [<ffffffffa043ef06>] ieee80211_tx+0xf6/0x120 [mac80211]
[   84.168391]  [<ffffffffa043ee49>] ? ieee80211_tx+0x39/0x120 [mac80211]
[   84.168408]  [<ffffffffa043f80b>] ieee80211_xmit+0xdb/0x100 [mac80211]
[   84.168425]  [<ffffffffa043f730>] ? ieee80211_skb_resize.isra.26+0xb0/0xb0 [mac80211]
[   84.168441]  [<ffffffffa0440b2a>] ieee80211_tx_skb_tid+0x5a/0x70 [mac80211]
[   84.168458]  [<ffffffffa0443da2>] ieee80211_send_auth+0x152/0x1b0 [mac80211]
[   84.168474]  [<ffffffffa042e169>] ieee80211_work_work+0x1049/0x1860 [mac80211]
[   84.168489]  [<ffffffffa042d120>] ? free_work+0x20/0x20 [mac80211]
[   84.168504]  [<ffffffffa042d120>] ? free_work+0x20/0x20 [mac80211]
[   84.168510]  [<ffffffff81065ffc>] process_one_work+0x17c/0x530
[   84.168514]  [<ffffffff81065f92>] ? process_one_work+0x112/0x530
[   84.168519]  [<ffffffff81066994>] worker_thread+0x164/0x350
[   84.168524]  [<ffffffff8108420d>] ? trace_hardirqs_on+0xd/0x10
[   84.168528]  [<ffffffff81066830>] ? manage_workers.isra.28+0x220/0x220
[   84.168533]  [<ffffffff8106bc17>] kthread+0x87/0x90
[   84.168539]  [<ffffffff813854b4>] kernel_thread_helper+0x4/0x10
[   84.168543]  [<ffffffff81382bdd>] ? retint_restore_args+0xe/0xe
[   84.168547]  [<ffffffff8106bb90>] ? __init_kthread_worker+0x70/0x70
[   84.168552]  [<ffffffff813854b0>] ? gs_change+0xb/0xb
[   84.168554] ---[ end trace f25a4fdc768c028f ]---

Index: wireless-testing-new/drivers/net/wireless/rtlwifi/base.c
===================================================================
--- wireless-testing-new.orig/drivers/net/wireless/rtlwifi/base.c
+++ wireless-testing-new/drivers/net/wireless/rtlwifi/base.c
@@ -449,6 +449,7 @@ int rtl_init_core(struct ieee80211_hw *h
 	/* <4> locks */
 	mutex_init(&rtlpriv->locks.conf_mutex);
 	mutex_init(&rtlpriv->locks.ps_mutex);
+	spin_lock_init(&rtlpriv->locks.ips_lock);
 	spin_lock_init(&rtlpriv->locks.irq_th_lock);
 	spin_lock_init(&rtlpriv->locks.h2c_lock);
 	spin_lock_init(&rtlpriv->locks.rf_ps_lock);
Index: wireless-testing-new/drivers/net/wireless/rtlwifi/ps.c
===================================================================
--- wireless-testing-new.orig/drivers/net/wireless/rtlwifi/ps.c
+++ wireless-testing-new/drivers/net/wireless/rtlwifi/ps.c
@@ -241,7 +241,7 @@ void rtl_ips_nic_on(struct ieee80211_hw
 	if (mac->opmode != NL80211_IFTYPE_STATION)
 		return;
 
-	mutex_lock(&rtlpriv->locks.ps_mutex);
+	spin_lock(&rtlpriv->locks.ips_lock);
 
 	if (ppsc->inactiveps) {
 		rtstate = ppsc->rfpwr_state;
@@ -257,7 +257,7 @@ void rtl_ips_nic_on(struct ieee80211_hw
 		}
 	}
 
-	mutex_unlock(&rtlpriv->locks.ps_mutex);
+	spin_unlock(&rtlpriv->locks.ips_lock);
 }
 
 /*for FW LPS*/
Index: wireless-testing-new/drivers/net/wireless/rtlwifi/wifi.h
===================================================================
--- wireless-testing-new.orig/drivers/net/wireless/rtlwifi/wifi.h
+++ wireless-testing-new/drivers/net/wireless/rtlwifi/wifi.h
@@ -1547,6 +1547,7 @@ struct rtl_locks {
 	struct mutex ps_mutex;
 
 	/*spin lock */
+	spinlock_t ips_lock;
 	spinlock_t irq_th_lock;
 	spinlock_t h2c_lock;
 	spinlock_t rf_ps_lock;

Re: [RFC] rtlwifi: Partial revert of 6539306b

[Stanislaw Gruszka]

Hi Larry.

On Fri, Dec 16, 2011 at 11:11:08AM -0600, Larry Finger wrote:
> When I tested commit 6539306, I did not notice that loading an out-of-tree
> module turns off lockdep testing in kernel 3.2. For that reason, I missed
> the kernel WARNING shown below:
> 
> As I stated earlier, my understanding of locking may not up to the task
> of fixing this problem. I first tried using one mutex to replace ips_lock
> and a second to replace lps_lock. That still got a warning.
> 
> The solution below, which partially reverts 6539306 and restores one of the
> spinlocks, does not generate any lockdep warnings. This solution also has
> the advantage that it this locking is only used when the NIC is turned on.
> I am, however, ready to test any solution that eliminates both spinlocks.
[snip]

> [   84.168146] ------------[ cut here ]------------
> [   84.168155] WARNING: at kernel/mutex.c:198 mutex_lock_nested+0x309/0x310()
> [   84.168158] Hardware name: HP Pavilion dv2700 Notebook PC
> [   84.168161] Modules linked in: nfs lockd auth_rpcgss nfs_acl sunrpc af_packet cpufreq_conservative cpufreq_userspace cpufreq_powersave powernow_k8 mperf e
> xt3 jbd ide_cd_mod cdrom snd_hda_codec_conexant arc4 rtl8192ce ide_pci_generic rtl8192c_common rtlwifi snd_hda_intel mac80211 snd_hda_codec snd_pcm snd_timer
>  amd74xx ide_core cfg80211 k8temp snd joydev soundcore hwmon battery forcedeth i2c_nforce2 sg rfkill ac serio_raw snd_page_alloc button video i2c_core ipv6 a
> utofs4 ext4 mbcache jbd2 crc16 sd_mod ahci ohci_hcd libahci libata scsi_mod ehci_hcd usbcore usb_common fan processor thermal
> [   84.168231] Pid: 1218, comm: kworker/u:2 Not tainted 3.2.0-rc5-wl+ #155
> [   84.168234] Call Trace:
> [   84.168240]  [<ffffffff81048aaa>] warn_slowpath_common+0x7a/0xb0
> [   84.168245]  [<ffffffff81048af5>] warn_slowpath_null+0x15/0x20
> [   84.168249]  [<ffffffff813811f9>] mutex_lock_nested+0x309/0x310
> [   84.168269]  [<ffffffffa00793f9>] ? rtl_ips_nic_on+0x49/0xb0 [rtlwifi]
> [   84.168277]  [<ffffffffa00793f9>] rtl_ips_nic_on+0x49/0xb0 [rtlwifi]
> [   84.168284]  [<ffffffffa007ab85>] rtl_pci_tx+0x1b5/0x560 [rtlwifi]
> [   84.168291]  [<ffffffffa007635a>] rtl_op_tx+0x9a/0xa0 [rtlwifi]
> [   84.168359]  [<ffffffffa043cf51>] __ieee80211_tx+0x181/0x2b0 [mac80211]

Hugh, good catch, how I missed that? Fix is ok, except
spin_lock_irqsave(, flags) should be used, as we call this function
both from work and from interrupt context.

What brings us to the problems we had before. I think right solution
would be using one spinlock, as we protect the same data, and
change code such there were no delays when holding the spinlocks
(maybe this is done already, not sure).

Anyway for now having two locks is fine, I think, it does not make
things worse than they was before.

Thanks
Stanislaw