| Message ID | 20131004101216.GA17043@linutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
> >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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }