* [Intel-wired-lan] [PATCH iwl-net] ice: use spin_lock_bh for Tx timestamp tracker lock
@ 2023-05-26 22:15 Jacob Keller
2023-05-30 20:51 ` Michal Schmidt
0 siblings, 1 reply; 3+ messages in thread
From: Jacob Keller @ 2023-05-26 22:15 UTC (permalink / raw)
To: Intel Wired LAN, Anthony Nguyen; +Cc: Jesse Brandeburg
The Tx timestamp tracker lock is acquired during ice_ptp_tx_tstamp
which is now called from the ice_misc_intr_thread_fn(). As far as I can
understand, this thread function is run in softirq context, so it is
possible for the thread to interrupt another thread that is currently in
the critical section of this lock when using only spin_lock. This would
result in a deadlock.
The use of spin_lock was historically from when Tx timestamps were
processed within a separate kthread run in process context, thus making
spin_lock safe. This was changed when we moved to the threaded IRQ in
commit 1229b33973c7 ("ice: Add low latency Tx timestamp read").
Fix the lock access to use spin_lock_bh() to disable softirq on the current
CPU during the critical sections.
Fixes: 1229b33973c7 ("ice: Add low latency Tx timestamp read")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 30 ++++++++++++------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index ac6f06f9a2ed..0eaadc53daed 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -751,7 +751,7 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
drop_ts = true;
skip_ts_read:
- spin_lock(&tx->lock);
+ spin_lock_bh(&tx->lock);
if (tx->verify_cached && raw_tstamp)
tx->tstamps[idx].cached_tstamp = raw_tstamp;
clear_bit(idx, tx->in_use);
@@ -759,7 +759,7 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
tx->tstamps[idx].skb = NULL;
if (test_and_clear_bit(idx, tx->stale))
drop_ts = true;
- spin_unlock(&tx->lock);
+ spin_unlock_bh(&tx->lock);
/* It is unlikely but possible that the SKB will have been
* flushed at this point due to link change or teardown.
@@ -786,9 +786,9 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
/* Check if we still have work to do. If so, re-queue this task to
* poll for remaining timestamps.
*/
- spin_lock(&tx->lock);
+ spin_lock_bh(&tx->lock);
more_timestamps = tx->init && !bitmap_empty(tx->in_use, tx->len);
- spin_unlock(&tx->lock);
+ spin_unlock_bh(&tx->lock);
return !more_timestamps;
}
@@ -862,12 +862,12 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
if (!hw->reset_ongoing && (tstamp_ready & BIT_ULL(phy_idx)))
ice_clear_phy_tstamp(hw, tx->block, phy_idx);
- spin_lock(&tx->lock);
+ spin_lock_bh(&tx->lock);
skb = tx->tstamps[idx].skb;
tx->tstamps[idx].skb = NULL;
clear_bit(idx, tx->in_use);
clear_bit(idx, tx->stale);
- spin_unlock(&tx->lock);
+ spin_unlock_bh(&tx->lock);
/* Count the number of Tx timestamps flushed */
pf->ptp.tx_hwtstamp_flushed++;
@@ -891,9 +891,9 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
static void
ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
{
- spin_lock(&tx->lock);
+ spin_lock_bh(&tx->lock);
bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
- spin_unlock(&tx->lock);
+ spin_unlock_bh(&tx->lock);
}
/**
@@ -906,9 +906,9 @@ ice_ptp_mark_tx_tracker_stale(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);
+ spin_lock_bh(&tx->lock);
tx->init = 0;
- spin_unlock(&tx->lock);
+ spin_unlock_bh(&tx->lock);
/* wait for potentially outstanding interrupt to complete */
synchronize_irq(pf->msix_entries[pf->oicr_idx].vector);
@@ -1321,7 +1321,7 @@ 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);
+ spin_lock_bh(&ptp_port->tx.lock);
ptp_port->tx.calibrating = true;
spin_unlock(&ptp_port->tx.lock);
ptp_port->tx_fifo_busy_cnt = 0;
@@ -1332,7 +1332,7 @@ 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);
+ spin_lock_bh(&ptp_port->tx.lock);
ptp_port->tx.calibrating = false;
spin_unlock(&ptp_port->tx.lock);
@@ -2392,11 +2392,11 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb)
{
u8 idx;
- spin_lock(&tx->lock);
+ spin_lock_bh(&tx->lock);
/* Check that this tracker is accepting new timestamp requests */
if (!ice_ptp_is_tx_tracker_up(tx)) {
- spin_unlock(&tx->lock);
+ spin_unlock_bh(&tx->lock);
return -1;
}
@@ -2415,7 +2415,7 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb)
ice_trace(tx_tstamp_request, skb, idx);
}
- spin_unlock(&tx->lock);
+ spin_unlock_bh(&tx->lock);
/* return the appropriate PHY timestamp register index, -1 if no
* indexes were available.
base-commit: 125f965c29f1ba72064f7f6e9c9aad3311bd2eb1
--
2.40.0.471.gbd7f14d9353b
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] ice: use spin_lock_bh for Tx timestamp tracker lock
2023-05-26 22:15 [Intel-wired-lan] [PATCH iwl-net] ice: use spin_lock_bh for Tx timestamp tracker lock Jacob Keller
@ 2023-05-30 20:51 ` Michal Schmidt
2023-05-30 21:12 ` Keller, Jacob E
0 siblings, 1 reply; 3+ messages in thread
From: Michal Schmidt @ 2023-05-30 20:51 UTC (permalink / raw)
To: Jacob Keller; +Cc: Anthony Nguyen, Intel Wired LAN, Jesse Brandeburg
On Sat, May 27, 2023 at 12:16 AM Jacob Keller <jacob.e.keller@intel.com> wrote:
> The Tx timestamp tracker lock is acquired during ice_ptp_tx_tstamp
> which is now called from the ice_misc_intr_thread_fn(). As far as I can
> understand, this thread function is run in softirq context, so it is
> possible for the thread to interrupt another thread that is currently in
> the critical section of this lock when using only spin_lock. This would
> result in a deadlock.
No, the thread_fn of a threaded IRQ does not run in softirq context.
It runs in a kernel thread that has its own PID and everything. You
can see IRQ threads in "ps ax" as "[irq/<number>-<name>]", you can
modify their scheduling priority, etc.
Michal
> The use of spin_lock was historically from when Tx timestamps were
> processed within a separate kthread run in process context, thus making
> spin_lock safe. This was changed when we moved to the threaded IRQ in
> commit 1229b33973c7 ("ice: Add low latency Tx timestamp read").
>
> Fix the lock access to use spin_lock_bh() to disable softirq on the current
> CPU during the critical sections.
>
> Fixes: 1229b33973c7 ("ice: Add low latency Tx timestamp read")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice_ptp.c | 30 ++++++++++++------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index ac6f06f9a2ed..0eaadc53daed 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -751,7 +751,7 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
> drop_ts = true;
>
> skip_ts_read:
> - spin_lock(&tx->lock);
> + spin_lock_bh(&tx->lock);
> if (tx->verify_cached && raw_tstamp)
> tx->tstamps[idx].cached_tstamp = raw_tstamp;
> clear_bit(idx, tx->in_use);
> @@ -759,7 +759,7 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
> tx->tstamps[idx].skb = NULL;
> if (test_and_clear_bit(idx, tx->stale))
> drop_ts = true;
> - spin_unlock(&tx->lock);
> + spin_unlock_bh(&tx->lock);
>
> /* It is unlikely but possible that the SKB will have been
> * flushed at this point due to link change or teardown.
> @@ -786,9 +786,9 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
> /* Check if we still have work to do. If so, re-queue this task to
> * poll for remaining timestamps.
> */
> - spin_lock(&tx->lock);
> + spin_lock_bh(&tx->lock);
> more_timestamps = tx->init && !bitmap_empty(tx->in_use, tx->len);
> - spin_unlock(&tx->lock);
> + spin_unlock_bh(&tx->lock);
>
> return !more_timestamps;
> }
> @@ -862,12 +862,12 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
> if (!hw->reset_ongoing && (tstamp_ready & BIT_ULL(phy_idx)))
> ice_clear_phy_tstamp(hw, tx->block, phy_idx);
>
> - spin_lock(&tx->lock);
> + spin_lock_bh(&tx->lock);
> skb = tx->tstamps[idx].skb;
> tx->tstamps[idx].skb = NULL;
> clear_bit(idx, tx->in_use);
> clear_bit(idx, tx->stale);
> - spin_unlock(&tx->lock);
> + spin_unlock_bh(&tx->lock);
>
> /* Count the number of Tx timestamps flushed */
> pf->ptp.tx_hwtstamp_flushed++;
> @@ -891,9 +891,9 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
> static void
> ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
> {
> - spin_lock(&tx->lock);
> + spin_lock_bh(&tx->lock);
> bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
> - spin_unlock(&tx->lock);
> + spin_unlock_bh(&tx->lock);
> }
>
> /**
> @@ -906,9 +906,9 @@ ice_ptp_mark_tx_tracker_stale(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);
> + spin_lock_bh(&tx->lock);
> tx->init = 0;
> - spin_unlock(&tx->lock);
> + spin_unlock_bh(&tx->lock);
>
> /* wait for potentially outstanding interrupt to complete */
> synchronize_irq(pf->msix_entries[pf->oicr_idx].vector);
> @@ -1321,7 +1321,7 @@ 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);
> + spin_lock_bh(&ptp_port->tx.lock);
> ptp_port->tx.calibrating = true;
> spin_unlock(&ptp_port->tx.lock);
> ptp_port->tx_fifo_busy_cnt = 0;
> @@ -1332,7 +1332,7 @@ 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);
> + spin_lock_bh(&ptp_port->tx.lock);
> ptp_port->tx.calibrating = false;
> spin_unlock(&ptp_port->tx.lock);
>
> @@ -2392,11 +2392,11 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb)
> {
> u8 idx;
>
> - spin_lock(&tx->lock);
> + spin_lock_bh(&tx->lock);
>
> /* Check that this tracker is accepting new timestamp requests */
> if (!ice_ptp_is_tx_tracker_up(tx)) {
> - spin_unlock(&tx->lock);
> + spin_unlock_bh(&tx->lock);
> return -1;
> }
>
> @@ -2415,7 +2415,7 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb)
> ice_trace(tx_tstamp_request, skb, idx);
> }
>
> - spin_unlock(&tx->lock);
> + spin_unlock_bh(&tx->lock);
>
> /* return the appropriate PHY timestamp register index, -1 if no
> * indexes were available.
>
> base-commit: 125f965c29f1ba72064f7f6e9c9aad3311bd2eb1
> --
> 2.40.0.471.gbd7f14d9353b
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] ice: use spin_lock_bh for Tx timestamp tracker lock
2023-05-30 20:51 ` Michal Schmidt
@ 2023-05-30 21:12 ` Keller, Jacob E
0 siblings, 0 replies; 3+ messages in thread
From: Keller, Jacob E @ 2023-05-30 21:12 UTC (permalink / raw)
To: mschmidt; +Cc: Nguyen, Anthony L, Intel Wired LAN, Brandeburg, Jesse
> -----Original Message-----
> From: Michal Schmidt <mschmidt@redhat.com>
> Sent: Tuesday, May 30, 2023 1:51 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net] ice: use spin_lock_bh for Tx
> timestamp tracker lock
>
> On Sat, May 27, 2023 at 12:16 AM Jacob Keller <jacob.e.keller@intel.com> wrote:
> > The Tx timestamp tracker lock is acquired during ice_ptp_tx_tstamp
> > which is now called from the ice_misc_intr_thread_fn(). As far as I can
> > understand, this thread function is run in softirq context, so it is
> > possible for the thread to interrupt another thread that is currently in
> > the critical section of this lock when using only spin_lock. This would
> > result in a deadlock.
>
> No, the thread_fn of a threaded IRQ does not run in softirq context.
> It runs in a kernel thread that has its own PID and everything. You
> can see IRQ threads in "ps ax" as "[irq/<number>-<name>]", you can
> modify their scheduling priority, etc.
>
> Michal
>
Ah.. Hm. I must have misunderstood some of the doc I was reading about in the request_threaded_irq. Ok. We can disregard this patch then.
Thanks,
Jake
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-30 21:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-26 22:15 [Intel-wired-lan] [PATCH iwl-net] ice: use spin_lock_bh for Tx timestamp tracker lock Jacob Keller
2023-05-30 20:51 ` Michal Schmidt
2023-05-30 21:12 ` Keller, Jacob E
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.