Message-ID: <Y5Ejgb2P2f/PX0ym@x130>
Date:   Wed, 7 Dec 2022 15:36:33 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        edumazet@...gle.com, Jacob Keller <jacob.e.keller@...el.com>,
        netdev@...r.kernel.org, richardcochran@...il.com, leon@...nel.org,
        Gurucharan G <gurucharanx.g@...el.com>
Subject: Re: [PATCH net-next v2 09/15] ice: protect init and calibrating
 check in ice_ptp_request_ts

On 07 Dec 13:09, Tony Nguyen wrote:
>From: Jacob Keller <jacob.e.keller@...el.com>
>
>When requesting a new timestamp, the ice_ptp_request_ts function does not
>hold the Tx tracker lock while checking init and calibrating. This means
>that we might issue a new timestamp request just after the Tx timestamp
>tracker starts being deinitialized. This could lead to incorrect access of
>the timestamp structures. Correct this by moving the init and calibrating
>checks under the lock, and updating the flows which modify these fields to
>use the lock.
>
>Note that we do not need to hold the lock while checking for tx->init in
>ice_ptp_tstamp_tx. This is because the teardown function will use
>synchronize_irq after clearing the flag to ensure that the threaded

FYI: couldn't find any ice_ptp_tstamp_tx(), and if it's running in xmit
path sofritrq then sync_irq won't help you.

>interrupt completes. Either a) the tx->init flag will be cleared before the
>ice_ptp_tx_tstamp function starts, thus it will exit immediately, or b) the
>threaded interrupt will be executing and the synchronize_irq will wait
>until the threaded interrupt has completed at which point we know the init
>field has definitely been set and new interrupts will not execute the Tx
>timestamp thread function.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>Tested-by: Gurucharan G <gurucharanx.g@...el.com> (A Contingent worker at Intel)
>Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>---
> drivers/net/ethernet/intel/ice/ice_ptp.c | 36 ++++++++++++++++++++----
> drivers/net/ethernet/intel/ice/ice_ptp.h |  2 +-
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>index 0282ccc55819..481492d84e0e 100644
>--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>@@ -599,6 +599,23 @@ static u64 ice_ptp_extend_40b_ts(struct ice_pf *pf, u64 in_tstamp)
> 				     (in_tstamp >> 8) & mask);
> }
>
>+/**
>+ * ice_ptp_is_tx_tracker_up - Check if Tx tracker is ready for new timestamps
>+ * @tx: the PTP Tx timestamp tracker to check
>+ *
>+ * Check that a given PTP Tx timestamp tracker is up, i.e. that it is ready
>+ * to accept new timestamp requests.
>+ *
>+ * Assumes the tx->lock spinlock is already held.
>+ */
>+static bool
>+ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx)
>+{
>+	lockdep_assert_held(&tx->lock);
>+
>+	return tx->init && !tx->calibrating;
>+}
>+
> /**
>  * ice_ptp_tx_tstamp - Process Tx timestamps for a port
>  * @tx: the PTP Tx timestamp tracker
>@@ -788,10 +805,10 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)
> 		return -ENOMEM;
> 	}
>
>-	spin_lock_init(&tx->lock);
>-
> 	tx->init = 1;
>
>+	spin_lock_init(&tx->lock);
>+

this change is pointless. 

> 	return 0;
> }
>
>@@ -833,7 +850,9 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
> static void
> ice_ptp_release_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
> {
>+	spin_lock(&tx->lock);
> 	tx->init = 0;
>+	spin_unlock(&tx->lock);
>
> 	/* wait for potentially outstanding interrupt to complete */
> 	synchronize_irq(pf->msix_entries[pf->oicr_idx].vector);
>@@ -1327,7 +1346,9 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
> 	kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
>
> 	/* temporarily disable Tx timestamps while calibrating PHY offset */
>+	spin_lock(&ptp_port->tx.lock);
> 	ptp_port->tx.calibrating = true;
>+	spin_unlock(&ptp_port->tx.lock);
> 	ptp_port->tx_fifo_busy_cnt = 0;
>
> 	/* Start the PHY timer in Vernier mode */
>@@ -1336,7 +1357,9 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
> 		goto out_unlock;
>
> 	/* Enable Tx timestamps right away */
>+	spin_lock(&ptp_port->tx.lock);
> 	ptp_port->tx.calibrating = false;
>+	spin_unlock(&ptp_port->tx.lock);
>
> 	kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work, 0);
>
>@@ -2330,11 +2353,14 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb)
> {
> 	u8 idx;
>
>-	/* Check if this tracker is initialized */
>-	if (!tx->init || tx->calibrating)
>+	spin_lock(&tx->lock);
>+
>+	/* Check that this tracker is accepting new timestamp requests */
>+	if (!ice_ptp_is_tx_tracker_up(tx)) {
>+		spin_unlock(&tx->lock);
> 		return -1;
>+	}
>
>-	spin_lock(&tx->lock);
> 	/* Find and set the first available index */
> 	idx = find_first_zero_bit(tx->in_use, tx->len);
> 	if (idx < tx->len) {
>diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
>index 5052fc41bed3..0bfafaaab6c7 100644
>--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
>+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
>@@ -110,7 +110,7 @@ struct ice_tx_tstamp {
>
> /**
>  * struct ice_ptp_tx - Tracking structure for all Tx timestamp requests on a port
>- * @lock: lock to prevent concurrent write to in_use bitmap
>+ * @lock: lock to prevent concurrent access to fields of this struct
>  * @tstamps: array of len to store outstanding requests
>  * @in_use: bitmap of len to indicate which slots are in use
>  * @block: which memory block (quad or port) the timestamps are captured in
>-- 
>2.35.1
>

Powered by blists - more mailing lists