Re: [Intel-wired-lan] [PATCH iwl-next 2/5] ice: schedule service task in IRQ thread_fn

[Michal Schmidt <mschmidt@redhat.com>]

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;

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan