Re: Kernel panic in 3.10.10-rt7 (iwlwifi)

Re: Kernel panic in 3.10.10-rt7 (iwlwifi)

[Sebastian Andrzej Siewior]

* Thomas Meyer | 2013-09-15 23:49:29 [+0200]:

>Hi,
Hi,

>My system did lockup after short time of usage. I was only able to capture this screenshot:
>
>Any ideas?

the problem seems to be that they use sleeping locks in the primary irq
handler.

iwl_pcie_rx_replenish() takes the same lock (->irq_lock) and additionaly
->lock so I think tunrning everything into raw locks isn't very wise for
the latency.

The threaded handler takes for a very short time irq_lock lock. It also
takes the ->lock via (iwl_pcie_rxq_inc_wr_ptr()) with irqs off (that one
looks short, too) and others for instance via iwl_pcie_rx_handle().
Ideally the driver should hold one spinlock to synchronize the primary
and threaded irq handler while disabling the interrupt in the iwl
hardware. Everything else would then hold one or two mutex(es) and could
even allocate memory with GFP_KERNEL.

For now I think the simply thing would be just to let both handlers run
in the thread. Does this patch solve your problem?

diff --git a/drivers/net/wireless/iwlwifi/pcie/trans.c b/drivers/net/wireless/iwlwifi/pcie/trans.c
index aeb70e1..42567fc 100644
--- a/drivers/net/wireless/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/iwlwifi/pcie/trans.c
@@ -1456,6 +1456,20 @@ static const struct iwl_trans_ops trans_ops_pcie = {
 	.set_bits_mask = iwl_trans_pcie_set_bits_mask,
 };
 
+#ifdef CONFIG_PREEMPT_RT_BASE
+static irqreturn_t iwl_rt_irq_handler(int irq, void *dev_id)
+{
+	irqreturn_t ret;
+
+	local_bh_disable();
+	ret = iwl_pcie_isr_ict(irq, dev_id);
+	local_bh_enable();
+	if (ret == IRQ_WAKE_THREAD)
+		ret = iwl_pcie_irq_handler(irq, dev_id);
+	return ret;
+}
+#endif
+
 struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev,
 				       const struct pci_device_id *ent,
 				       const struct iwl_cfg *cfg)
@@ -1566,9 +1580,14 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev,
 	if (iwl_pcie_alloc_ict(trans))
 		goto out_free_cmd_pool;
 
+#ifdef CONFIG_PREEMPT_RT_BASE
+	if (request_threaded_irq(pdev->irq, NULL, iwl_rt_irq_handler,
+				 IRQF_SHARED | IRQF_ONESHOT, DRV_NAME, trans)) {
+#else
 	if (request_threaded_irq(pdev->irq, iwl_pcie_isr_ict,
 				 iwl_pcie_irq_handler,
 				 IRQF_SHARED, DRV_NAME, trans)) {
+#endif
 		IWL_ERR(trans, "Error allocating IRQ %d\n", pdev->irq);
 		goto out_free_ict;
 	}
-- 
1.8.4.rc3


>With kind regards
>Thomas

Sebastian

RE: [Ilw] Re: Kernel panic in 3.10.10-rt7 (iwlwifi)

[Grumbach, Emmanuel]

PiA+TXkgc3lzdGVtIGRpZCBsb2NrdXAgYWZ0ZXIgc2hvcnQgdGltZSBvZiB1c2FnZS4gSSB3YXMg
b25seSBhYmxlIHRvIGNhcHR1cmUNCj4gdGhpcyBzY3JlZW5zaG90Og0KPiA+DQo+ID5BbnkgaWRl
YXM/DQo+IA0KPiB0aGUgcHJvYmxlbSBzZWVtcyB0byBiZSB0aGF0IHRoZXkgdXNlIHNsZWVwaW5n
IGxvY2tzIGluIHRoZSBwcmltYXJ5IGlycQ0KPiBoYW5kbGVyLg0KPiANCj4gaXdsX3BjaWVfcnhf
cmVwbGVuaXNoKCkgdGFrZXMgdGhlIHNhbWUgbG9jayAoLT5pcnFfbG9jaykgYW5kIGFkZGl0aW9u
YWx5DQo+IC0+bG9jayBzbyBJIHRoaW5rIHR1bnJuaW5nIGV2ZXJ5dGhpbmcgaW50byByYXcgbG9j
a3MgaXNuJ3QgdmVyeSB3aXNlIGZvcg0KPiB0aGUgbGF0ZW5jeS4NCj4gDQo+IFRoZSB0aHJlYWRl
ZCBoYW5kbGVyIHRha2VzIGZvciBhIHZlcnkgc2hvcnQgdGltZSBpcnFfbG9jayBsb2NrLiBJdCBh
bHNvIHRha2VzDQo+IHRoZSAtPmxvY2sgdmlhIChpd2xfcGNpZV9yeHFfaW5jX3dyX3B0cigpKSB3
aXRoIGlycXMgb2ZmICh0aGF0IG9uZSBsb29rcyBzaG9ydCwNCj4gdG9vKSBhbmQgb3RoZXJzIGZv
ciBpbnN0YW5jZSB2aWEgaXdsX3BjaWVfcnhfaGFuZGxlKCkuDQo+IElkZWFsbHkgdGhlIGRyaXZl
ciBzaG91bGQgaG9sZCBvbmUgc3BpbmxvY2sgdG8gc3luY2hyb25pemUgdGhlIHByaW1hcnkgYW5k
DQo+IHRocmVhZGVkIGlycSBoYW5kbGVyIHdoaWxlIGRpc2FibGluZyB0aGUgaW50ZXJydXB0IGlu
IHRoZSBpd2wgaGFyZHdhcmUuDQo+IEV2ZXJ5dGhpbmcgZWxzZSB3b3VsZCB0aGVuIGhvbGQgb25l
IG9yIHR3byBtdXRleChlcykgYW5kIGNvdWxkIGV2ZW4NCj4gYWxsb2NhdGUgbWVtb3J5IHdpdGgg
R0ZQX0tFUk5FTC4NCg0KV2UgaGF2ZSBhIHBhdGNoIGludGVybmFsbHkgdGhhdCBnb2VzIGludG8g
dGhhdCBkaXJlY3Rpb24sIGJ1dCBvbmUgdGhpbmcgdGhvdWdoLiBJZiB5b3Ugc3RpbGwgaGF2ZSBh
IHNwaW5sb2NrIGluIHRoZSBwcmltYXJ5IGhhbmRsZXIsIHdvdWxkbid0IHRoYXQgc2xlZXAgaW4g
YSBub24tc2xlZXBhYmxlIGNvbnRleHQ/IEZvcmdpdmUgbXkgLXJ0IGlnbm9yYW5jZSwgYnV0IEkg
dW5kZXJzdG9vZCB0aGF0IGluIC1ydCwgYWxsIHRoZSBzcGlubG9jayBnbyB0byBzbGVlcCB3aGlj
aCBiYXNpY2FsbHkgbWVhbiB0aGF0IHdlIGNhbid0IHRha2UgYW55IHNwaW5sb2NrIGluIHRoZSBw
cmltYXJ5IGlycSBoYW5kbGVyIHNvIEkgYW0gYSBiaXQgY29uZnVzZWQgaGVyZS4NCg0KPiANCj4g
Rm9yIG5vdyBJIHRoaW5rIHRoZSBzaW1wbHkgdGhpbmcgd291bGQgYmUganVzdCB0byBsZXQgYm90
aCBoYW5kbGVycyBydW4gaW4gdGhlDQo+IHRocmVhZC4gRG9lcyB0aGlzIHBhdGNoIHNvbHZlIHlv
dXIgcHJvYmxlbT8NCj4gDQo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL25ldC93aXJlbGVzcy9pd2x3
aWZpL3BjaWUvdHJhbnMuYw0KPiBiL2RyaXZlcnMvbmV0L3dpcmVsZXNzL2l3bHdpZmkvcGNpZS90
cmFucy5jDQo+IGluZGV4IGFlYjcwZTEuLjQyNTY3ZmMgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMv
bmV0L3dpcmVsZXNzL2l3bHdpZmkvcGNpZS90cmFucy5jDQo+ICsrKyBiL2RyaXZlcnMvbmV0L3dp
cmVsZXNzL2l3bHdpZmkvcGNpZS90cmFucy5jDQo+IEBAIC0xNDU2LDYgKzE0NTYsMjAgQEAgc3Rh
dGljIGNvbnN0IHN0cnVjdCBpd2xfdHJhbnNfb3BzIHRyYW5zX29wc19wY2llID0NCj4gew0KPiAg
CS5zZXRfYml0c19tYXNrID0gaXdsX3RyYW5zX3BjaWVfc2V0X2JpdHNfbWFzaywgIH07DQo+IA0K
PiArI2lmZGVmIENPTkZJR19QUkVFTVBUX1JUX0JBU0UNCj4gK3N0YXRpYyBpcnFyZXR1cm5fdCBp
d2xfcnRfaXJxX2hhbmRsZXIoaW50IGlycSwgdm9pZCAqZGV2X2lkKSB7DQo+ICsJaXJxcmV0dXJu
X3QgcmV0Ow0KPiArDQo+ICsJbG9jYWxfYmhfZGlzYWJsZSgpOw0KPiArCXJldCA9IGl3bF9wY2ll
X2lzcl9pY3QoaXJxLCBkZXZfaWQpOw0KPiArCWxvY2FsX2JoX2VuYWJsZSgpOw0KPiArCWlmIChy
ZXQgPT0gSVJRX1dBS0VfVEhSRUFEKQ0KPiArCQlyZXQgPSBpd2xfcGNpZV9pcnFfaGFuZGxlcihp
cnEsIGRldl9pZCk7DQo+ICsJcmV0dXJuIHJldDsNCj4gK30NCj4gKyNlbmRpZg0KPiArDQo+ICBz
dHJ1Y3QgaXdsX3RyYW5zICppd2xfdHJhbnNfcGNpZV9hbGxvYyhzdHJ1Y3QgcGNpX2RldiAqcGRl
diwNCj4gIAkJCQkgICAgICAgY29uc3Qgc3RydWN0IHBjaV9kZXZpY2VfaWQgKmVudCwNCj4gIAkJ
CQkgICAgICAgY29uc3Qgc3RydWN0IGl3bF9jZmcgKmNmZykNCj4gQEAgLTE1NjYsOSArMTU4MCwx
NCBAQCBzdHJ1Y3QgaXdsX3RyYW5zICppd2xfdHJhbnNfcGNpZV9hbGxvYyhzdHJ1Y3QNCj4gcGNp
X2RldiAqcGRldiwNCj4gIAlpZiAoaXdsX3BjaWVfYWxsb2NfaWN0KHRyYW5zKSkNCj4gIAkJZ290
byBvdXRfZnJlZV9jbWRfcG9vbDsNCj4gDQo+ICsjaWZkZWYgQ09ORklHX1BSRUVNUFRfUlRfQkFT
RQ0KPiArCWlmIChyZXF1ZXN0X3RocmVhZGVkX2lycShwZGV2LT5pcnEsIE5VTEwsIGl3bF9ydF9p
cnFfaGFuZGxlciwNCj4gKwkJCQkgSVJRRl9TSEFSRUQgfCBJUlFGX09ORVNIT1QsDQo+IERSVl9O
QU1FLCB0cmFucykpIHsgI2Vsc2UNCj4gIAlpZiAocmVxdWVzdF90aHJlYWRlZF9pcnEocGRldi0+
aXJxLCBpd2xfcGNpZV9pc3JfaWN0LA0KPiAgCQkJCSBpd2xfcGNpZV9pcnFfaGFuZGxlciwNCj4g
IAkJCQkgSVJRRl9TSEFSRUQsIERSVl9OQU1FLCB0cmFucykpIHsNCj4gKyNlbmRpZg0KPiAgCQlJ
V0xfRVJSKHRyYW5zLCAiRXJyb3IgYWxsb2NhdGluZyBJUlEgJWRcbiIsIHBkZXYtPmlycSk7DQo+
ICAJCWdvdG8gb3V0X2ZyZWVfaWN0Ow0KPiAgCX0NCj4gLS0NCj4gMS44LjQucmMzDQo+IA0KPiAN
Cj4gPldpdGgga2luZCByZWdhcmRzDQo+ID5UaG9tYXMNCj4gDQo+IFNlYmFzdGlhbg0KPiANCj4g
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18NCj4gaWx3IG1h
aWxpbmcgbGlzdA0KPiBpbHdAbGludXguaW50ZWwuY29tDQo+IGh0dHA6Ly9saW51eC5pbnRlbC5j
b20vbWFpbG1hbi9saXN0aW5mby9pbHcNCg==

Re: [Ilw] Re: Kernel panic in 3.10.10-rt7 (iwlwifi)

[Sebastian Andrzej Siewior]

On 10/06/2013 08:00 AM, Grumbach, Emmanuel wrote:
> 
> We have a patch internally that goes into that direction, but one thing though.

Sounds great.

> If you still have a spinlock in the primary handler, wouldn't that sleep in a non-sleepable context? Forgive my -rt ignorance, but I understood that in -rt, all the spinlock go to sleep which basically mean that we can't take any spinlock in the primary irq handler so I am a bit confused here.

After my change there is no primary handler anymore, just the threaded
where you can take the (sleeping) spinlock.
Everything what you wrote is correct:
- you can't take a spinlock in the primary handler
- all spinlocks may sleep

Sebastian