netdev - Re: [PATCH v7 iwl-next 4/6] ice: Process TSYN IRQ in a separate function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240823203241.GG2164@kernel.org>
Date: Fri, 23 Aug 2024 21:32:41 +0100
From: Simon Horman <horms@...nel.org>
To: Karol Kolacinski <karol.kolacinski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com
Subject: Re: [PATCH v7 iwl-next 4/6] ice: Process TSYN IRQ in a separate
 function

On Tue, Aug 20, 2024 at 12:21:51PM +0200, Karol Kolacinski wrote:
> Simplify TSYN IRQ processing by moving it to a separate function and
> having appropriate behavior per PHY model, instead of multiple
> conditions not related to HW, but to specific timestamping modes.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@...el.com>

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index cf3b02d14b19..861f6224540a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2760,6 +2760,65 @@ enum ice_tx_tstamp_work ice_ptp_process_ts(struct ice_pf *pf)
>  	}
>  }
>  
> +/**
> + * ice_ptp_ts_irq - Process the PTP Tx timestamps in IRQ context
> + * @pf: Board private structure
> + *
> + * Return: IRQ_WAKE_THREAD if Tx timestamp read has to be handled in the bottom
> + *         half of the interrupt and IRQ_HANDLED otherwise.
> + */
> +irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
> +{
> +	struct ice_hw *hw = &pf->hw;
> +
> +	switch (hw->mac_type) {
> +	case ICE_MAC_E810:
> +		/* E810 capable of low latency timestamping with interrupt can
> +		 * request a single timestamp in the top half and wait for
> +		 * a second LL TS interrupt from the FW when it's ready.
> +		 */
> +		if (hw->dev_caps.ts_dev_info.ts_ll_int_read) {
> +			struct ice_ptp_tx *tx = &pf->ptp.port.tx;
> +			u8 idx;
> +
> +			if (!ice_pf_state_is_nominal(pf))
> +				return IRQ_HANDLED;
> +
> +			spin_lock(&tx->lock);
> +			idx = find_next_bit_wrap(tx->in_use, tx->len,
> +						 tx->last_ll_ts_idx_read + 1);
> +			if (idx != tx->len)
> +				ice_ptp_req_tx_single_tstamp(tx, idx);
> +			spin_unlock(&tx->lock);
> +
> +			return IRQ_HANDLED;
> +		}
> +		fallthrough; /* non-LL_TS E810 */
> +	case ICE_MAC_GENERIC:
> +	case ICE_MAC_GENERIC_3K_E825:
> +		/* All other devices process timestamps in the bottom half due
> +		 * to sleeping or polling.
> +		 */
> +		if (!ice_ptp_pf_handles_tx_interrupt(pf))
> +			return IRQ_HANDLED;
> +
> +		set_bit(ICE_MISC_THREAD_TX_TSTAMP, pf->misc_thread);
> +		return IRQ_WAKE_THREAD;
> +	case ICE_MAC_E830:
> +		/* E830 can read timestamps in the top half using rd32() */
> +		if (ice_ptp_process_ts(pf) == ICE_TX_TSTAMP_WORK_PENDING) {
> +			/* Process outstanding Tx timestamps. If there
> +			 * is more work, re-arm the interrupt to trigger again.
> +			 */
> +			wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
> +			ice_flush(hw);
> +		}
> +		return IRQ_HANDLED;

I think it would be better to split out adding E830 support into a separate
patch, leaving this patch as strictly refactoring of existing code.

> +	default:
> +		return IRQ_HANDLED;
> +	}
> +}
> +
>  /**
>   * ice_ptp_maybe_trigger_tx_interrupt - Trigger Tx timstamp interrupt
>   * @pf: Board private structure
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h

...

> @@ -360,6 +361,11 @@ static inline bool ice_ptp_process_ts(struct ice_pf *pf)
>  	return true;
>  }
>  
> +static inline irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
> +{
> +	return IRQ_HANDLED;
> +}

This no-op implementation is in effect if CONFIG_PTP_1588_CLOCK is not
enabled. Whereas previously the fuller implementation would have run.
I think that deserves some coverage in the patch description.

> +
>  static inline u64
>  ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
>  		    const struct ice_pkt_ctx *pkt_ctx)
> -- 
> 2.46.0
> 
>