netdev - Re: [Intel-wired-lan] [PATCH v3 iwl-next 2/4] 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: <d665834c-3989-4832-8f1c-bf127ce8a652@intel.com>
Date: Fri, 26 Jul 2024 15:22:47 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
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: [Intel-wired-lan] [PATCH v3 iwl-next 2/4] ice: Process TSYN IRQ
 in a separate function

From: Karol Kolacinski <karol.kolacinski@...el.com>
Date: Thu, 25 Jul 2024 11:34:49 +0200

> 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>

[...]

> +/**
> + * ice_ptp_ts_irq - Process the PTP Tx timestamps in IRQ context
> + * @pf: Board private structure
> + *
> + * Returns: IRQ_WAKE_THREAD if Tx timestamp read has to be handled in the bottom

In the previous commit, you used 'Return:', here you have 'Returns:'.
Perhaps you may want to keep the style consistent :)

> + * 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->ptp.phy_model) {
> +	case ICE_PHY_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;
> +			unsigned long flags;
> +			u8 idx;
> +
> +			if (!ice_pf_state_is_nominal(pf))
> +				return IRQ_HANDLED;
> +
> +			spin_lock_irqsave(&tx->lock, flags);

It's top half, right? If so, you don't need _irqsave versions, just
spin_lock/unlock().
_irqsave/restore are only to be used in either BH or process context to
temporarily disable interrupts to avoid concurrent access with TH (e.g.
this block).

> +			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_irqrestore(&tx->lock, flags);
> +
> +			return IRQ_HANDLED;
> +		}
> +		fallthrough; /* non-LL_TS E810 */
> +	case ICE_PHY_E82X:
> +	case ICE_PHY_ETH56G:
> +		/* 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_PHY_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;
> +	default:
> +		return IRQ_HANDLED;
> +	}
> +}

[...]

Thanks,
Olek