| Message ID | 20201224131148.300691-3-a.darwish@linutronix.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | chelsio: cxgb: Use threaded irqs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: christophe.jaillet@wanadoo.fr leon@kernel.org apais@linux.microsoft.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 5 this patch: 5 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 52 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 2020-12-24 14:11:47 [+0100], Ahmed S. Darwish wrote: > --- a/drivers/net/ethernet/chelsio/cxgb/cxgb2.c > +++ b/drivers/net/ethernet/chelsio/cxgb/cxgb2.c > @@ -211,9 +211,9 @@ static int cxgb_up(struct adapter *adapter) > t1_interrupts_clear(adapter); > > adapter->params.has_msi = !disable_msi && !pci_enable_msi(adapter->pdev); > - err = request_irq(adapter->pdev->irq, t1_interrupt, > - adapter->params.has_msi ? 0 : IRQF_SHARED, > - adapter->name, adapter); > + err = request_threaded_irq(adapter->pdev->irq, t1_irq, t1_irq_thread, > + adapter->params.has_msi ? 0 : IRQF_SHARED, > + adapter->name, adapter); > if (err) { > if (adapter->params.has_msi) > pci_disable_msi(adapter->pdev); > diff --git a/drivers/net/ethernet/chelsio/cxgb/sge.c b/drivers/net/ethernet/chelsio/cxgb/sge.c > index d6df1a87db0b..f1c402f6b889 100644 > --- a/drivers/net/ethernet/chelsio/cxgb/sge.c > +++ b/drivers/net/ethernet/chelsio/cxgb/sge.c > @@ -1626,11 +1626,10 @@ int t1_poll(struct napi_struct *napi, int budget) > return work_done; > } > > -irqreturn_t t1_interrupt(int irq, void *data) > +irqreturn_t t1_irq(int irq, void *data) > { > struct adapter *adapter = data; > struct sge *sge = adapter->sge; > - int handled; > > if (likely(responses_pending(adapter))) { > writel(F_PL_INTR_SGE_DATA, adapter->regs + A_PL_CAUSE); > @@ -1645,9 +1644,19 @@ irqreturn_t t1_interrupt(int irq, void *data) > napi_enable(&adapter->napi); > } > } > + > return IRQ_HANDLED; > } > > + return IRQ_WAKE_THREAD; > +} > + > +irqreturn_t t1_irq_thread(int irq, void *data) > +{ > + struct adapter *adapter = data; > + struct sge *sge = adapter->sge; > + int handled; > + > spin_lock(&adapter->async_lock); > handled = t1_slow_intr_handler(adapter); > spin_unlock(&adapter->async_lock); This does not work in general, it might work in the MSI case but does not work for the LEVEL interrupt case: The interrupt remains active because it has not been ACKed. Chances are that the threaded handler never gets scheduled because interrupt is still pending and t1_irq() gets invoked right away. For that reason, the primary must either mask the interrupt source or use IRQF_ONESHOT to mask the interrupt line until the threaded handler is done. If you look at t1_elmer0_ext_intr() it disables F_PL_INTR_EXT before the worker scheduled so the interrupt does not trigger again. The worker then does what ever is needed (t1_elmer0_ext_intr_handler) and then ACKs F_PL_INTR_EXT and enables F_PL_INTR_EXT again so it may trigger an interrupt again. Sebastian
diff --git a/drivers/net/ethernet/chelsio/cxgb/cxgb2.c b/drivers/net/ethernet/chelsio/cxgb/cxgb2.c index 7b5a98330ef7..c96bdca4f270 100644 --- a/drivers/net/ethernet/chelsio/cxgb/cxgb2.c +++ b/drivers/net/ethernet/chelsio/cxgb/cxgb2.c @@ -211,9 +211,9 @@ static int cxgb_up(struct adapter *adapter) t1_interrupts_clear(adapter); adapter->params.has_msi = !disable_msi && !pci_enable_msi(adapter->pdev); - err = request_irq(adapter->pdev->irq, t1_interrupt, - adapter->params.has_msi ? 0 : IRQF_SHARED, - adapter->name, adapter); + err = request_threaded_irq(adapter->pdev->irq, t1_irq, t1_irq_thread, + adapter->params.has_msi ? 0 : IRQF_SHARED, + adapter->name, adapter); if (err) { if (adapter->params.has_msi) pci_disable_msi(adapter->pdev); diff --git a/drivers/net/ethernet/chelsio/cxgb/sge.c b/drivers/net/ethernet/chelsio/cxgb/sge.c index d6df1a87db0b..f1c402f6b889 100644 --- a/drivers/net/ethernet/chelsio/cxgb/sge.c +++ b/drivers/net/ethernet/chelsio/cxgb/sge.c @@ -1626,11 +1626,10 @@ int t1_poll(struct napi_struct *napi, int budget) return work_done; } -irqreturn_t t1_interrupt(int irq, void *data) +irqreturn_t t1_irq(int irq, void *data) { struct adapter *adapter = data; struct sge *sge = adapter->sge; - int handled; if (likely(responses_pending(adapter))) { writel(F_PL_INTR_SGE_DATA, adapter->regs + A_PL_CAUSE); @@ -1645,9 +1644,19 @@ irqreturn_t t1_interrupt(int irq, void *data) napi_enable(&adapter->napi); } } + return IRQ_HANDLED; } + return IRQ_WAKE_THREAD; +} + +irqreturn_t t1_irq_thread(int irq, void *data) +{ + struct adapter *adapter = data; + struct sge *sge = adapter->sge; + int handled; + spin_lock(&adapter->async_lock); handled = t1_slow_intr_handler(adapter); spin_unlock(&adapter->async_lock); diff --git a/drivers/net/ethernet/chelsio/cxgb/sge.h b/drivers/net/ethernet/chelsio/cxgb/sge.h index a1ba591b3431..4072b3fb312b 100644 --- a/drivers/net/ethernet/chelsio/cxgb/sge.h +++ b/drivers/net/ethernet/chelsio/cxgb/sge.h @@ -74,7 +74,8 @@ struct sge *t1_sge_create(struct adapter *, struct sge_params *); int t1_sge_configure(struct sge *, struct sge_params *); int t1_sge_set_coalesce_params(struct sge *, struct sge_params *); void t1_sge_destroy(struct sge *); -irqreturn_t t1_interrupt(int irq, void *cookie); +irqreturn_t t1_irq(int irq, void *cookie); +irqreturn_t t1_irq_thread(int irq, void *cookie); int t1_poll(struct napi_struct *, int); netdev_tx_t t1_start_xmit(struct sk_buff *skb, struct net_device *dev);
The t1_interrupt() irq handler calls del_timer_sync() down the chain: sge.c: t1_interrupt() -> subr.c: t1_slow_intr_handler() -> asic_slow_intr() || fpga_slow_intr() -> t1_pci_intr_handler() -> cxgb2.c: t1_fatal_err() # Cont. at [*] -> fpga_slow_intr() -> sge.c: t1_sge_intr_error_handler() -> cxgb2.c: t1_fatal_err() # Cont. at [*] [*] cxgb2.c: t1_fatal_err() -> sge.c: t1_sge_stop() -> timer.c: del_timer_sync() This is invalid: if an irq handler calls del_timer_sync() on a timer it has already interrupted, it will loop forever. Move the slow t1 interrupt handling path, t1_slow_intr_handler(), to a threaded-irq task context. Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> --- drivers/net/ethernet/chelsio/cxgb/cxgb2.c | 6 +++--- drivers/net/ethernet/chelsio/cxgb/sge.c | 13 +++++++++++-- drivers/net/ethernet/chelsio/cxgb/sge.h | 3 ++- 3 files changed, 16 insertions(+), 6 deletions(-)