* Re: Kernel panic in 3.10.10-rt7 (iwlwifi) [not found] <2CD18B42-2489-4DF7-98D6-4D002329DC63@m3y3r.de> @ 2013-10-04 10:12 ` Sebastian Andrzej Siewior 2013-10-06 6:00 ` Grumbach, Emmanuel 0 siblings, 1 reply; 4+ messages in thread From: Sebastian Andrzej Siewior @ 2013-10-04 10:12 UTC (permalink / raw) To: Thomas Meyer Cc: Johannes Berg, linux-rt-users, Wey-Yi Guy, Intel Linux Wireless, linux-wireless, tglx * 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [Ilw] Re: Kernel panic in 3.10.10-rt7 (iwlwifi) 2013-10-04 10:12 ` Kernel panic in 3.10.10-rt7 (iwlwifi) Sebastian Andrzej Siewior @ 2013-10-06 6:00 ` Grumbach, Emmanuel 0 siblings, 0 replies; 4+ messages in thread From: Grumbach, Emmanuel @ 2013-10-06 6:00 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Thomas Meyer Cc: linux-rt-users@vger.kernel.org, Berg, Johannes, linux-wireless@vger.kernel.org, Intel Linux Wireless, tglx@linutronix.de 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== ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [Ilw] Re: Kernel panic in 3.10.10-rt7 (iwlwifi) @ 2013-10-06 6:00 ` Grumbach, Emmanuel 0 siblings, 0 replies; 4+ messages in thread From: Grumbach, Emmanuel @ 2013-10-06 6:00 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Thomas Meyer Cc: linux-rt-users@vger.kernel.org, Berg, Johannes, linux-wireless@vger.kernel.org, Intel Linux Wireless, tglx@linutronix.de > >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. We have a patch internally that goes into that direction, but one thing though. 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. > > 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 > > _______________________________________________ > ilw mailing list > ilw@linux.intel.com > http://linux.intel.com/mailman/listinfo/ilw ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Ilw] Re: Kernel panic in 3.10.10-rt7 (iwlwifi) 2013-10-06 6:00 ` Grumbach, Emmanuel (?) @ 2013-10-07 7:33 ` Sebastian Andrzej Siewior -1 siblings, 0 replies; 4+ messages in thread From: Sebastian Andrzej Siewior @ 2013-10-07 7:33 UTC (permalink / raw) To: Grumbach, Emmanuel Cc: Thomas Meyer, linux-rt-users@vger.kernel.org, Berg, Johannes, linux-wireless@vger.kernel.org, Intel Linux Wireless, tglx@linutronix.de 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-10-07 7:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <2CD18B42-2489-4DF7-98D6-4D002329DC63@m3y3r.de>
2013-10-04 10:12 ` Kernel panic in 3.10.10-rt7 (iwlwifi) Sebastian Andrzej Siewior
2013-10-06 6:00 ` [Ilw] " Grumbach, Emmanuel
2013-10-06 6:00 ` Grumbach, Emmanuel
2013-10-07 7:33 ` Sebastian Andrzej Siewior
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.