* [Intel-wired-lan] [PATCH 0/4] igc: PTP tx fixes
@ 2020-07-28 23:37 Andre Guedes
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID macro Andre Guedes
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Andre Guedes @ 2020-07-28 23:37 UTC (permalink / raw)
To: intel-wired-lan
Hi all,
This patch series fixes some issues in the PTP tx code from the IGC driver.
The most relevant one is a race condition between igc_xmit_frame_ring() and
igc_ptp_tx_hang() which is fixed by the last patch.
- Andre
Andre Guedes (4):
igc: Rename IGC_TSYNCTXCTL_VALID macro
igc: Don't reschedule ptp_tx work
igc: Remove timeout check from ptp_tx work
igc: Fix race condition in PTP tx code
drivers/net/ethernet/intel/igc/igc.h | 5 +-
drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
drivers/net/ethernet/intel/igc/igc_main.c | 7 ++-
drivers/net/ethernet/intel/igc/igc_ptp.c | 60 +++++++++++---------
4 files changed, 43 insertions(+), 31 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID macro
2020-07-28 23:37 [Intel-wired-lan] [PATCH 0/4] igc: PTP tx fixes Andre Guedes
@ 2020-07-28 23:37 ` Andre Guedes
2020-08-14 3:07 ` Brown, Aaron F
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work Andre Guedes
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Andre Guedes @ 2020-07-28 23:37 UTC (permalink / raw)
To: intel-wired-lan
This patch renames the IGC_TSYNCTXCTL_VALID macro to IGC_TSYNCTXCTL_
TXTT_0 so it matches the datasheet.
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
drivers/net/ethernet/intel/igc/igc_ptp.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 1c032c1ce4d3..28885be15ee8 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -409,7 +409,7 @@
#define IGC_IMIREXT_SIZE_BP 0x00001000 /* Packet size bypass */
/* Time Sync Transmit Control bit definitions */
-#define IGC_TSYNCTXCTL_VALID 0x00000001 /* Tx timestamp valid */
+#define IGC_TSYNCTXCTL_TXTT_0 0x00000001 /* Tx timestamp reg 0 valid */
#define IGC_TSYNCTXCTL_ENABLED 0x00000010 /* enable Tx timestamping */
#define IGC_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK 0x0000F000 /* max delay */
#define IGC_TSYNCTXCTL_SYNC_COMP_ERR 0x20000000 /* sync err */
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index e67d4655b47e..79802fb4ea83 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -410,7 +410,7 @@ static void igc_ptp_tx_work(struct work_struct *work)
}
tsynctxctl = rd32(IGC_TSYNCTXCTL);
- if (tsynctxctl & IGC_TSYNCTXCTL_VALID)
+ if (tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)
igc_ptp_tx_hwtstamp(adapter);
else
/* reschedule to check later */
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work
2020-07-28 23:37 [Intel-wired-lan] [PATCH 0/4] igc: PTP tx fixes Andre Guedes
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID macro Andre Guedes
@ 2020-07-28 23:37 ` Andre Guedes
2020-08-14 3:08 ` Brown, Aaron F
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from " Andre Guedes
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code Andre Guedes
3 siblings, 1 reply; 11+ messages in thread
From: Andre Guedes @ 2020-07-28 23:37 UTC (permalink / raw)
To: intel-wired-lan
The ptp_tx work is scheduled only if TSICR.TXTS bit is set, therefore
TSYNCTXCTL.TXTT_0 bit is expected to be set when we check it igc_ptp_tx_
work(). If it isn't, something is really off and rescheduling the ptp_tx
work to check it later doesn't help much. This patch changes the code to
WARN_ON_ONCE() if this situation ever happens.
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
drivers/net/ethernet/intel/igc/igc_ptp.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 79802fb4ea83..b912830ea7bb 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -410,11 +410,10 @@ static void igc_ptp_tx_work(struct work_struct *work)
}
tsynctxctl = rd32(IGC_TSYNCTXCTL);
- if (tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)
- igc_ptp_tx_hwtstamp(adapter);
- else
- /* reschedule to check later */
- schedule_work(&adapter->ptp_tx_work);
+ if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
+ return;
+
+ igc_ptp_tx_hwtstamp(adapter);
}
/**
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from ptp_tx work
2020-07-28 23:37 [Intel-wired-lan] [PATCH 0/4] igc: PTP tx fixes Andre Guedes
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID macro Andre Guedes
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work Andre Guedes
@ 2020-07-28 23:37 ` Andre Guedes
2020-08-14 3:09 ` Brown, Aaron F
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code Andre Guedes
3 siblings, 1 reply; 11+ messages in thread
From: Andre Guedes @ 2020-07-28 23:37 UTC (permalink / raw)
To: intel-wired-lan
The tx timestamp timeout is already checked by the watchdog_task work
which runs periodically. In addition to that, from the ptp_tx work
perspective, if __IGC_PTP_TX_IN_PROGRESS flag is set we always want
handle the timestamp stored in hardware and update the skb. So this
patch removes the timeout check in igc_ptp_tx_work() function.
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
drivers/net/ethernet/intel/igc/igc_ptp.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index b912830ea7bb..22b2cf80b22f 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -403,12 +403,6 @@ static void igc_ptp_tx_work(struct work_struct *work)
if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
return;
- if (time_is_before_jiffies(adapter->ptp_tx_start +
- IGC_PTP_TX_TIMEOUT)) {
- igc_ptp_tx_timeout(adapter);
- return;
- }
-
tsynctxctl = rd32(IGC_TSYNCTXCTL);
if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
return;
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code
2020-07-28 23:37 [Intel-wired-lan] [PATCH 0/4] igc: PTP tx fixes Andre Guedes
` (2 preceding siblings ...)
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from " Andre Guedes
@ 2020-07-28 23:37 ` Andre Guedes
2020-08-14 3:09 ` Brown, Aaron F
3 siblings, 1 reply; 11+ messages in thread
From: Andre Guedes @ 2020-07-28 23:37 UTC (permalink / raw)
To: intel-wired-lan
Currently, the igc driver supports timestamping only one tx packet at a
time. During the transmission flow, the skb that requires hardware
timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
adapter->ptp_tx_skb, and notify the network stack.
While the thread executing the transmission flow (the user process
running in kernel mode) and the thread executing ptp_tx_work don't
access adapter->ptp_tx_skb concurrently, there are two other places
where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
igc_ptp_suspend().
igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
thread which runs periodically so it is possible we have two threads
accessing ptp_tx_skb at the same time. Consider the following scenario:
right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
written yet, this is considered a timeout and adapter->ptp_tx_skb is
cleaned up.
This patch fixes the issue described above by adding the ptp_tx_lock to
protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
Since igc_xmit_frame_ring() called in atomic context by the networking
stack, ptp_tx_lock is defined as a spinlock.
With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
flag doesn't provide much of a use anymore so this patch gets rid of it.
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
drivers/net/ethernet/intel/igc/igc.h | 5 ++-
drivers/net/ethernet/intel/igc/igc_main.c | 7 +++-
drivers/net/ethernet/intel/igc/igc_ptp.c | 49 ++++++++++++++---------
3 files changed, 40 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 3070dfdb7eb4..19f88f705ec3 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -207,6 +207,10 @@ struct igc_adapter {
struct ptp_clock *ptp_clock;
struct ptp_clock_info ptp_caps;
struct work_struct ptp_tx_work;
+ /* Access to ptp_tx_skb and ptp_tx_start is protected by the
+ * ptp_tx_lock.
+ */
+ spinlock_t ptp_tx_lock;
struct sk_buff *ptp_tx_skb;
struct hwtstamp_config tstamp_config;
unsigned long ptp_tx_start;
@@ -361,7 +365,6 @@ enum igc_state_t {
__IGC_TESTING,
__IGC_RESETTING,
__IGC_DOWN,
- __IGC_PTP_TX_IN_PROGRESS,
};
enum igc_tx_flags {
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index ab5b302d6655..d6b37d91cad9 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1347,13 +1347,14 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
struct igc_adapter *adapter = netdev_priv(tx_ring->netdev);
+ spin_lock(&adapter->ptp_tx_lock);
+
/* FIXME: add support for retrieving timestamps from
* the other timer registers before skipping the
* timestamping request.
*/
if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
- !test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
- &adapter->state)) {
+ !adapter->ptp_tx_skb) {
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
tx_flags |= IGC_TX_FLAGS_TSTAMP;
@@ -1362,6 +1363,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
} else {
adapter->tx_hwtstamp_skipped++;
}
+
+ spin_unlock(&adapter->ptp_tx_lock);
}
/* record initial flags and protocol */
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 22b2cf80b22f..3dda5bd05cc6 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -320,35 +320,35 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
return 0;
}
+/* Requires adapter->ptp_tx_lock held by caller. */
static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
{
struct igc_hw *hw = &adapter->hw;
dev_kfree_skb_any(adapter->ptp_tx_skb);
adapter->ptp_tx_skb = NULL;
+ adapter->ptp_tx_start = 0;
adapter->tx_hwtstamp_timeouts++;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
rd32(IGC_TXSTMPH);
+
netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
}
void igc_ptp_tx_hang(struct igc_adapter *adapter)
{
- bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
- IGC_PTP_TX_TIMEOUT);
+ spin_lock(&adapter->ptp_tx_lock);
- if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
- return;
+ if (!adapter->ptp_tx_skb)
+ goto unlock;
- /* If we haven't received a timestamp within the timeout, it is
- * reasonable to assume that it will never occur, so we can unlock the
- * timestamp bit when this occurs.
- */
- if (timeout) {
- cancel_work_sync(&adapter->ptp_tx_work);
- igc_ptp_tx_timeout(adapter);
- }
+ if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
+ goto unlock;
+
+ igc_ptp_tx_timeout(adapter);
+
+unlock:
+ spin_unlock(&adapter->ptp_tx_lock);
}
/**
@@ -358,6 +358,8 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter)
* If we were asked to do hardware stamping and such a time stamp is
* available, then it must have been for this skb here because we only
* allow only one such packet into the queue.
+ *
+ * Context: Expects adapter->ptp_tx_lock to be held by caller.
*/
static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
{
@@ -379,7 +381,7 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
* while we're notifying the stack.
*/
adapter->ptp_tx_skb = NULL;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
+ adapter->ptp_tx_start = 0;
/* Notify the stack and free the skb after we've unlocked */
skb_tstamp_tx(skb, &shhwtstamps);
@@ -400,14 +402,19 @@ static void igc_ptp_tx_work(struct work_struct *work)
struct igc_hw *hw = &adapter->hw;
u32 tsynctxctl;
- if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
- return;
+ spin_lock(&adapter->ptp_tx_lock);
+
+ if (!adapter->ptp_tx_skb)
+ goto unlock;
tsynctxctl = rd32(IGC_TSYNCTXCTL);
if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
- return;
+ goto unlock;
igc_ptp_tx_hwtstamp(adapter);
+
+unlock:
+ spin_unlock(&adapter->ptp_tx_lock);
}
/**
@@ -484,6 +491,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
}
spin_lock_init(&adapter->tmreg_lock);
+ spin_lock_init(&adapter->ptp_tx_lock);
INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
@@ -515,9 +523,14 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
return;
cancel_work_sync(&adapter->ptp_tx_work);
+
+ spin_lock(&adapter->ptp_tx_lock);
+
dev_kfree_skb_any(adapter->ptp_tx_skb);
adapter->ptp_tx_skb = NULL;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
+ adapter->ptp_tx_start = 0;
+
+ spin_unlock(&adapter->ptp_tx_lock);
}
/**
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID macro
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID macro Andre Guedes
@ 2020-08-14 3:07 ` Brown, Aaron F
0 siblings, 0 replies; 11+ messages in thread
From: Brown, Aaron F @ 2020-08-14 3:07 UTC (permalink / raw)
To: intel-wired-lan
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Tuesday, July 28, 2020 4:38 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID
> macro
>
> This patch renames the IGC_TSYNCTXCTL_VALID macro to IGC_TSYNCTXCTL_
> TXTT_0 so it matches the datasheet.
>
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work Andre Guedes
@ 2020-08-14 3:08 ` Brown, Aaron F
0 siblings, 0 replies; 11+ messages in thread
From: Brown, Aaron F @ 2020-08-14 3:08 UTC (permalink / raw)
To: intel-wired-lan
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Tuesday, July 28, 2020 4:38 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work
>
> The ptp_tx work is scheduled only if TSICR.TXTS bit is set, therefore
> TSYNCTXCTL.TXTT_0 bit is expected to be set when we check it igc_ptp_tx_
> work(). If it isn't, something is really off and rescheduling the ptp_tx
> work to check it later doesn't help much. This patch changes the code to
> WARN_ON_ONCE() if this situation ever happens.
>
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from ptp_tx work
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from " Andre Guedes
@ 2020-08-14 3:09 ` Brown, Aaron F
0 siblings, 0 replies; 11+ messages in thread
From: Brown, Aaron F @ 2020-08-14 3:09 UTC (permalink / raw)
To: intel-wired-lan
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Tuesday, July 28, 2020 4:38 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from
> ptp_tx work
>
> The tx timestamp timeout is already checked by the watchdog_task work
> which runs periodically. In addition to that, from the ptp_tx work
> perspective, if __IGC_PTP_TX_IN_PROGRESS flag is set we always want
> handle the timestamp stored in hardware and update the skb. So this
> patch removes the timeout check in igc_ptp_tx_work() function.
>
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
> drivers/net/ethernet/intel/igc/igc_ptp.c | 6 ------
> 1 file changed, 6 deletions(-)
>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code Andre Guedes
@ 2020-08-14 3:09 ` Brown, Aaron F
2020-08-22 1:24 ` Andre Guedes
0 siblings, 1 reply; 11+ messages in thread
From: Brown, Aaron F @ 2020-08-14 3:09 UTC (permalink / raw)
To: intel-wired-lan
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Tuesday, July 28, 2020 4:38 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code
>
> Currently, the igc driver supports timestamping only one tx packet at a
> time. During the transmission flow, the skb that requires hardware
> timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
> timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
> scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
> adapter->ptp_tx_skb, and notify the network stack.
>
> While the thread executing the transmission flow (the user process
> running in kernel mode) and the thread executing ptp_tx_work don't
> access adapter->ptp_tx_skb concurrently, there are two other places
> where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
> igc_ptp_suspend().
>
> igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
> thread which runs periodically so it is possible we have two threads
> accessing ptp_tx_skb at the same time. Consider the following scenario:
> right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
> igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
> written yet, this is considered a timeout and adapter->ptp_tx_skb is
> cleaned up.
>
> This patch fixes the issue described above by adding the ptp_tx_lock to
> protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
> Since igc_xmit_frame_ring() called in atomic context by the networking
> stack, ptp_tx_lock is defined as a spinlock.
>
> With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
> flag doesn't provide much of a use anymore so this patch gets rid of it.
>
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
> drivers/net/ethernet/intel/igc/igc.h | 5 ++-
> drivers/net/ethernet/intel/igc/igc_main.c | 7 +++-
> drivers/net/ethernet/intel/igc/igc_ptp.c | 49 ++++++++++++++---------
> 3 files changed, 40 insertions(+), 21 deletions(-)
>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code
2020-08-14 3:09 ` Brown, Aaron F
@ 2020-08-22 1:24 ` Andre Guedes
2020-08-24 18:07 ` Nguyen, Anthony L
0 siblings, 1 reply; 11+ messages in thread
From: Andre Guedes @ 2020-08-22 1:24 UTC (permalink / raw)
To: intel-wired-lan
Hi Jeff/Tony,
Quoting Brown, Aaron F (2020-08-13 20:09:42)
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > Andre Guedes
> > Sent: Tuesday, July 28, 2020 4:38 PM
> > To: intel-wired-lan at lists.osuosl.org
> > Subject: [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code
> >
> > Currently, the igc driver supports timestamping only one tx packet at a
> > time. During the transmission flow, the skb that requires hardware
> > timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
> > timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
> > scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
> > adapter->ptp_tx_skb, and notify the network stack.
> >
> > While the thread executing the transmission flow (the user process
> > running in kernel mode) and the thread executing ptp_tx_work don't
> > access adapter->ptp_tx_skb concurrently, there are two other places
> > where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
> > igc_ptp_suspend().
> >
> > igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
> > thread which runs periodically so it is possible we have two threads
> > accessing ptp_tx_skb at the same time. Consider the following scenario:
> > right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
> > igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
> > written yet, this is considered a timeout and adapter->ptp_tx_skb is
> > cleaned up.
> >
> > This patch fixes the issue described above by adding the ptp_tx_lock to
> > protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
> > Since igc_xmit_frame_ring() called in atomic context by the networking
> > stack, ptp_tx_lock is defined as a spinlock.
> >
> > With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
> > flag doesn't provide much of a use anymore so this patch gets rid of it.
> >
> > Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> > ---
> > drivers/net/ethernet/intel/igc/igc.h | 5 ++-
> > drivers/net/ethernet/intel/igc/igc_main.c | 7 +++-
> > drivers/net/ethernet/intel/igc/igc_ptp.c | 49 ++++++++++++++---------
> > 3 files changed, 40 insertions(+), 21 deletions(-)
> >
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Please hold this patch back. I think I found an issue with it. I'm
investigating it and should send a v2 soon.
Cheers,
Andre
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code
2020-08-22 1:24 ` Andre Guedes
@ 2020-08-24 18:07 ` Nguyen, Anthony L
0 siblings, 0 replies; 11+ messages in thread
From: Nguyen, Anthony L @ 2020-08-24 18:07 UTC (permalink / raw)
To: intel-wired-lan
On Fri, 2020-08-21 at 18:24 -0700, Andre Guedes wrote:
> Hi Jeff/Tony,
>
> Quoting Brown, Aaron F (2020-08-13 20:09:42)
> > > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On
> > > Behalf Of
> > > Andre Guedes
> > > Sent: Tuesday, July 28, 2020 4:38 PM
> > > To: intel-wired-lan at lists.osuosl.org
> > > Subject: [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in
> > > PTP tx code
> > >
> > > Currently, the igc driver supports timestamping only one tx
> > > packet at a
> > > time. During the transmission flow, the skb that requires
> > > hardware
> > > timestamping is saved in adapter->ptp_tx_skb. Once hardware has
> > > the
> > > timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
> > > scheduled. In igc_ptp_tx_work(), we read the timestamp register,
> > > update
> > > adapter->ptp_tx_skb, and notify the network stack.
> > >
> > > While the thread executing the transmission flow (the user
> > > process
> > > running in kernel mode) and the thread executing ptp_tx_work
> > > don't
> > > access adapter->ptp_tx_skb concurrently, there are two other
> > > places
> > > where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
> > > igc_ptp_suspend().
> > >
> > > igc_ptp_tx_hang() is executed by the adapter->watchdog_task
> > > worker
> > > thread which runs periodically so it is possible we have two
> > > threads
> > > accessing ptp_tx_skb at the same time. Consider the following
> > > scenario:
> > > right after __IGC_PTP_TX_IN_PROGRESS is set in
> > > igc_xmit_frame_ring(),
> > > igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't
> > > been
> > > written yet, this is considered a timeout and adapter->ptp_tx_skb
> > > is
> > > cleaned up.
> > >
> > > This patch fixes the issue described above by adding the
> > > ptp_tx_lock to
> > > protect access to ptp_tx_skb and ptp_tx_start fields from
> > > igc_adapter.
> > > Since igc_xmit_frame_ring() called in atomic context by the
> > > networking
> > > stack, ptp_tx_lock is defined as a spinlock.
> > >
> > > With the introduction of the ptp_tx_lock, the
> > > __IGC_PTP_TX_IN_PROGRESS
> > > flag doesn't provide much of a use anymore so this patch gets rid
> > > of it.
> > >
> > > Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> > > ---
> > > drivers/net/ethernet/intel/igc/igc.h | 5 ++-
> > > drivers/net/ethernet/intel/igc/igc_main.c | 7 +++-
> > > drivers/net/ethernet/intel/igc/igc_ptp.c | 49 ++++++++++++++---
> > > ------
> > > 3 files changed, 40 insertions(+), 21 deletions(-)
> > >
> >
> > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>
> Please hold this patch back. I think I found an issue with it. I'm
> investigating it and should send a v2 soon.
Thanks Andre, I've dropped this patch from dev-queue.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-08-24 18:07 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 23:37 [Intel-wired-lan] [PATCH 0/4] igc: PTP tx fixes Andre Guedes
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID macro Andre Guedes
2020-08-14 3:07 ` Brown, Aaron F
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work Andre Guedes
2020-08-14 3:08 ` Brown, Aaron F
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from " Andre Guedes
2020-08-14 3:09 ` Brown, Aaron F
2020-07-28 23:37 ` [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code Andre Guedes
2020-08-14 3:09 ` Brown, Aaron F
2020-08-22 1:24 ` Andre Guedes
2020-08-24 18:07 ` Nguyen, Anthony L
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.