| Message ID | 20230526222158.2685796-3-jacob.e.keller@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ice: Improve miscellaneous interrupt code | expand |
Den 27.05.2023 kl. 00.21 skrev Jacob Keller: > From: Karol Kolacinski <karol.kolacinski@intel.com> > > If the kernel is configured with CONFIG_PREEMPT_RT, scheduling the service > task in interrupt context can result in a kernel panic. This is a result of > ice_service_task_schedule calling queue_work. Is it really the case on current kernels that one cannot use queue_work in that context? The previous posting of this patch showed a stack trace from a 3.10-based vendor kernel. Has the crash been seen on anything recent? I think the workqueue code has been safe to use in atomic context on even on PREEMPT_RT since commit fe3bc8a988a4 ("Merge branch 'for-5.8' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq"). That said, the patch looks OK to me. It makes the code cleaner. I object only to the description. Michal > Move the ice_service_task_schedule() call into the miscellaneous IRQ thread > that functions as the interrupt bottom half. > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_main.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > index 9e4d7d884115..8b59632ec6b1 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -3058,7 +3058,6 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data) > { > struct ice_pf *pf = (struct ice_pf *)data; > struct ice_hw *hw = &pf->hw; > - irqreturn_t ret = IRQ_NONE; > struct device *dev; > u32 oicr, ena_mask; > > @@ -3139,10 +3138,8 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data) > > if (oicr & PFINT_OICR_TSYN_TX_M) { > ena_mask &= ~PFINT_OICR_TSYN_TX_M; > - if (!hw->reset_ongoing) { > + if (!hw->reset_ongoing) > pf->oicr_misc |= PFINT_OICR_TSYN_TX_M; > - ret = IRQ_WAKE_THREAD; > - } > } > > if (oicr & PFINT_OICR_TSYN_EVNT_M) { > @@ -3159,7 +3156,6 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data) > GLTSYN_STAT_EVENT2_M); > > pf->oicr_misc |= PFINT_OICR_TSYN_EVNT_M; > - ret = IRQ_WAKE_THREAD; > } > } > > @@ -3180,16 +3176,12 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data) > if (oicr & (PFINT_OICR_PCI_EXCEPTION_M | > PFINT_OICR_ECC_ERR_M)) { > set_bit(ICE_PFR_REQ, pf->state); > - ice_service_task_schedule(pf); > } > } > - if (!ret) > - ret = IRQ_HANDLED; > > - ice_service_task_schedule(pf); > ice_irq_dynamic_ena(hw, NULL, NULL); > > - return ret; > + return IRQ_WAKE_THREAD; > } > > /** > @@ -3204,6 +3196,8 @@ static irqreturn_t ice_misc_intr_thread_fn(int __always_unused irq, void *data) > if (ice_is_reset_in_progress(pf->state)) > return IRQ_HANDLED; > > + ice_service_task_schedule(pf); > + > if (pf->oicr_misc & PFINT_OICR_TSYN_EVNT_M) { > ice_ptp_extts_event(pf); > pf->oicr_misc &= ~PFINT_OICR_TSYN_EVNT_M;
> -----Original Message----- > From: Michal Schmidt <mschmidt@redhat.com> > Sent: Monday, May 29, 2023 3:43 AM > To: Keller, Jacob E <jacob.e.keller@intel.com>; Intel Wired LAN <intel-wired- > lan@lists.osuosl.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Cc: Kolacinski, Karol <karol.kolacinski@intel.com> > Subject: Re: [Intel-wired-lan] [PATCH iwl-next 2/5] ice: schedule service task in > IRQ thread_fn > > Den 27.05.2023 kl. 00.21 skrev Jacob Keller: > > From: Karol Kolacinski <karol.kolacinski@intel.com> > > > > If the kernel is configured with CONFIG_PREEMPT_RT, scheduling the service > > task in interrupt context can result in a kernel panic. This is a result of > > ice_service_task_schedule calling queue_work. > > Is it really the case on current kernels that one cannot use queue_work > in that context? > The previous posting of this patch showed a stack trace from a > 3.10-based vendor kernel. Has the crash been seen on anything recent? > I think the workqueue code has been safe to use in atomic context on > even on PREEMPT_RT since commit fe3bc8a988a4 ("Merge branch 'for-5.8' of > git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq"). > > That said, the patch looks OK to me. It makes the code cleaner. I object > only to the description. > > Michal > Hmm... we had developed this fix some time ago for a customer who was on an older stable kernel that didn't have this fix. I wasn't aware of it, so I assumed it was still a problem when writing this message for upstream. I think I prefer the overall code design here especially since we're moving processing of external timestamps. I'll re-write the commit message.
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 9e4d7d884115..8b59632ec6b1 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -3058,7 +3058,6 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data) { struct ice_pf *pf = (struct ice_pf *)data; struct ice_hw *hw = &pf->hw; - irqreturn_t ret = IRQ_NONE; struct device *dev; u32 oicr, ena_mask; @@ -3139,10 +3138,8 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data) if (oicr & PFINT_OICR_TSYN_TX_M) { ena_mask &= ~PFINT_OICR_TSYN_TX_M; - if (!hw->reset_ongoing) { + if (!hw->reset_ongoing) pf->oicr_misc |= PFINT_OICR_TSYN_TX_M; - ret = IRQ_WAKE_THREAD; - } } if (oicr & PFINT_OICR_TSYN_EVNT_M) { @@ -3159,7 +3156,6 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data) GLTSYN_STAT_EVENT2_M); pf->oicr_misc |= PFINT_OICR_TSYN_EVNT_M; - ret = IRQ_WAKE_THREAD; } } @@ -3180,16 +3176,12 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data) if (oicr & (PFINT_OICR_PCI_EXCEPTION_M | PFINT_OICR_ECC_ERR_M)) { set_bit(ICE_PFR_REQ, pf->state); - ice_service_task_schedule(pf); } } - if (!ret) - ret = IRQ_HANDLED; - ice_service_task_schedule(pf); ice_irq_dynamic_ena(hw, NULL, NULL); - return ret; + return IRQ_WAKE_THREAD; } /** @@ -3204,6 +3196,8 @@ static irqreturn_t ice_misc_intr_thread_fn(int __always_unused irq, void *data) if (ice_is_reset_in_progress(pf->state)) return IRQ_HANDLED; + ice_service_task_schedule(pf); + if (pf->oicr_misc & PFINT_OICR_TSYN_EVNT_M) { ice_ptp_extts_event(pf); pf->oicr_misc &= ~PFINT_OICR_TSYN_EVNT_M;