* [Intel-wired-lan] [PATCH iwl v1 1/5] igc: Fix marking some timestamps as skipped wrongly
2023-05-04 23:52 [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes Vinicius Costa Gomes
@ 2023-05-04 23:52 ` Vinicius Costa Gomes
2023-05-05 9:49 ` Kurt Kanzenbach
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 2/5] igc: Fix race condition in PTP tx code Vinicius Costa Gomes
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Vinicius Costa Gomes @ 2023-05-04 23:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: vladimir.oltean, kurt, anthony.l.nguyen
If a packet was internally forwarded to a igc managed NIC, and it had
the SKBTX_HW_TSTAMP flag set, the driver would mark that timestamp as
skipped.
In reality, that timestamp was "not for us", as TX timestamp could
never be enabled in the NIC.
Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
drivers/net/ethernet/intel/igc/igc_main.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 1c4676882082..0d029041a102 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1578,13 +1578,13 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
}
}
- if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
+ if (unlikely(adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
+ skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
/* 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,
+ if (!test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
&adapter->state)) {
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
tx_flags |= IGC_TX_FLAGS_TSTAMP;
--
2.40.1
_______________________________________________
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] 14+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl v1 1/5] igc: Fix marking some timestamps as skipped wrongly
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 1/5] igc: Fix marking some timestamps as skipped wrongly Vinicius Costa Gomes
@ 2023-05-05 9:49 ` Kurt Kanzenbach
0 siblings, 0 replies; 14+ messages in thread
From: Kurt Kanzenbach @ 2023-05-05 9:49 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan; +Cc: vladimir.oltean, anthony.l.nguyen
[-- Attachment #1.1: Type: text/plain, Size: 544 bytes --]
On Thu May 04 2023, Vinicius Costa Gomes wrote:
> If a packet was internally forwarded to a igc managed NIC, and it had
> the SKBTX_HW_TSTAMP flag set, the driver would mark that timestamp as
> skipped.
>
> In reality, that timestamp was "not for us", as TX timestamp could
> never be enabled in the NIC.
>
> Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
> Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
[-- Attachment #2: Type: text/plain, Size: 162 bytes --]
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Intel-wired-lan] [PATCH iwl v1 2/5] igc: Fix race condition in PTP tx code
2023-05-04 23:52 [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes Vinicius Costa Gomes
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 1/5] igc: Fix marking some timestamps as skipped wrongly Vinicius Costa Gomes
@ 2023-05-04 23:52 ` Vinicius Costa Gomes
2023-05-05 9:51 ` Kurt Kanzenbach
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 3/5] igc: Fix checking for tstamp timeouts TX tstamp is off Vinicius Costa Gomes
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Vinicius Costa Gomes @ 2023-05-04 23:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: Andre Guedes, vladimir.oltean, kurt, anthony.l.nguyen
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.
Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
drivers/net/ethernet/intel/igc/igc.h | 5 +-
drivers/net/ethernet/intel/igc/igc_main.c | 8 ++-
drivers/net/ethernet/intel/igc/igc_ptp.c | 61 ++++++++++++-----------
3 files changed, 43 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 34aebf00a512..7da0657ea48f 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -229,6 +229,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 are 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;
@@ -401,7 +405,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 0d029041a102..b383352651a5 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1584,8 +1584,10 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
* the other timer registers before skipping the
* timestamping request.
*/
- if (!test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
- &adapter->state)) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+ if (!adapter->ptp_tx_skb) {
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
tx_flags |= IGC_TX_FLAGS_TSTAMP;
@@ -1594,6 +1596,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
} else {
adapter->tx_hwtstamp_skipped++;
}
+
+ spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
}
if (skb_vlan_tag_present(skb)) {
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 4e10ced736db..56128e55f5c0 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -603,6 +603,7 @@ 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;
@@ -610,7 +611,6 @@ static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
dev_kfree_skb_any(adapter->ptp_tx_skb);
adapter->ptp_tx_skb = NULL;
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");
@@ -618,20 +618,20 @@ static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
void igc_ptp_tx_hang(struct igc_adapter *adapter)
{
- bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
- IGC_PTP_TX_TIMEOUT);
-
- if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
- return;
-
- /* 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);
- }
+ unsigned long flags;
+
+ spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+
+ if (!adapter->ptp_tx_skb)
+ goto unlock;
+
+ if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
+ goto unlock;
+
+ igc_ptp_tx_timeout(adapter);
+
+unlock:
+ spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
}
/**
@@ -641,6 +641,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)
{
@@ -676,13 +678,7 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
shhwtstamps.hwtstamp =
ktime_add_ns(shhwtstamps.hwtstamp, adjust);
- /* Clear the lock early before calling skb_tstamp_tx so that
- * applications are not woken up before the lock bit is clear. We use
- * a copy of the skb pointer to ensure other threads can't change it
- * while we're notifying the stack.
- */
adapter->ptp_tx_skb = NULL;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
/* Notify the stack and free the skb after we've unlocked */
skb_tstamp_tx(skb, &shhwtstamps);
@@ -693,24 +689,33 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
* igc_ptp_tx_work
* @work: pointer to work struct
*
- * This work function polls the TSYNCTXCTL valid bit to determine when a
- * timestamp has been taken for the current stored skb.
+ * This work function checks the TSYNCTXCTL valid bit to determine when
+ * a timestamp has been taken for the current stored skb.
*/
static void igc_ptp_tx_work(struct work_struct *work)
{
struct igc_adapter *adapter = container_of(work, struct igc_adapter,
ptp_tx_work);
struct igc_hw *hw = &adapter->hw;
+ unsigned long flags;
u32 tsynctxctl;
- if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
- return;
+ spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+
+ if (!adapter->ptp_tx_skb)
+ goto unlock;
tsynctxctl = rd32(IGC_TSYNCTXCTL);
- if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
- return;
+ tsynctxctl &= IGC_TSYNCTXCTL_TXTT_0;
+ if (!tsynctxctl) {
+ WARN_ONCE(1, "Received a TSTAMP interrupt but no TSTAMP is ready.\n");
+ goto unlock;
+ }
igc_ptp_tx_hwtstamp(adapter);
+
+unlock:
+ spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
}
/**
@@ -959,6 +964,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
return;
}
+ spin_lock_init(&adapter->ptp_tx_lock);
spin_lock_init(&adapter->tmreg_lock);
INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
@@ -1023,7 +1029,6 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
cancel_work_sync(&adapter->ptp_tx_work);
dev_kfree_skb_any(adapter->ptp_tx_skb);
adapter->ptp_tx_skb = NULL;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
if (pci_device_is_present(adapter->pdev)) {
igc_ptp_time_save(adapter);
--
2.40.1
_______________________________________________
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] 14+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl v1 2/5] igc: Fix race condition in PTP tx code
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 2/5] igc: Fix race condition in PTP tx code Vinicius Costa Gomes
@ 2023-05-05 9:51 ` Kurt Kanzenbach
0 siblings, 0 replies; 14+ messages in thread
From: Kurt Kanzenbach @ 2023-05-05 9:51 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: Andre Guedes, vladimir.oltean, anthony.l.nguyen
[-- Attachment #1.1: Type: text/plain, Size: 1983 bytes --]
On Thu May 04 2023, Vinicius Costa Gomes wrote:
> 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.
... and you use the _irqsave/restore variants, because patch #4 is going
to retrieve the timestamp directly from the IRQ context.
>
> 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.
>
> Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
[-- Attachment #2: Type: text/plain, Size: 162 bytes --]
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Intel-wired-lan] [PATCH iwl v1 3/5] igc: Fix checking for tstamp timeouts TX tstamp is off
2023-05-04 23:52 [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes Vinicius Costa Gomes
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 1/5] igc: Fix marking some timestamps as skipped wrongly Vinicius Costa Gomes
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 2/5] igc: Fix race condition in PTP tx code Vinicius Costa Gomes
@ 2023-05-04 23:52 ` Vinicius Costa Gomes
2023-05-05 9:51 ` Kurt Kanzenbach
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 4/5] igc: Retrieve TX timestamp during interrupt handling Vinicius Costa Gomes
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Vinicius Costa Gomes @ 2023-05-04 23:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: vladimir.oltean, kurt, anthony.l.nguyen
When TX timestamping is disabled, there's no need to check for
timestamp timeouts.
We should only take care to free any pending timestamp when TX
timestamping is disabled, as that skb would never be released
otherwise.
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
drivers/net/ethernet/intel/igc/igc_ptp.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 56128e55f5c0..4dd0eec5a246 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -536,11 +536,27 @@ static void igc_ptp_enable_rx_timestamp(struct igc_adapter *adapter)
wr32(IGC_TSYNCRXCTL, val);
}
+static void igc_ptp_clear_tx_tstamp(struct igc_adapter *adapter)
+{
+ unsigned long flags;
+
+ cancel_work_sync(&adapter->ptp_tx_work);
+
+ spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+
+ dev_kfree_skb_any(adapter->ptp_tx_skb);
+ adapter->ptp_tx_skb = NULL;
+
+ spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
+}
+
static void igc_ptp_disable_tx_timestamp(struct igc_adapter *adapter)
{
struct igc_hw *hw = &adapter->hw;
wr32(IGC_TSYNCTXCTL, 0);
+
+ igc_ptp_clear_tx_tstamp(adapter);
}
static void igc_ptp_enable_tx_timestamp(struct igc_adapter *adapter)
@@ -620,6 +636,9 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter)
{
unsigned long flags;
+ if (adapter->tstamp_config.tx_type != HWTSTAMP_TX_ON)
+ return;
+
spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
if (!adapter->ptp_tx_skb)
@@ -1026,9 +1045,7 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
return;
- cancel_work_sync(&adapter->ptp_tx_work);
- dev_kfree_skb_any(adapter->ptp_tx_skb);
- adapter->ptp_tx_skb = NULL;
+ igc_ptp_clear_tx_tstamp(adapter);
if (pci_device_is_present(adapter->pdev)) {
igc_ptp_time_save(adapter);
--
2.40.1
_______________________________________________
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] 14+ messages in thread* [Intel-wired-lan] [PATCH iwl v1 4/5] igc: Retrieve TX timestamp during interrupt handling
2023-05-04 23:52 [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes Vinicius Costa Gomes
` (2 preceding siblings ...)
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 3/5] igc: Fix checking for tstamp timeouts TX tstamp is off Vinicius Costa Gomes
@ 2023-05-04 23:52 ` Vinicius Costa Gomes
2023-05-05 9:51 ` Kurt Kanzenbach
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 5/5] igc: Add workaround for missing timestamps Vinicius Costa Gomes
2023-05-08 20:55 ` [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes Tony Nguyen
5 siblings, 1 reply; 14+ messages in thread
From: Vinicius Costa Gomes @ 2023-05-04 23:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: vladimir.oltean, kurt, anthony.l.nguyen
When the interrupt is handled, the TXTT_0 bit in the TSYNCTXCTL
register should already be set and the timestamp value already loaded
in the appropriate register.
This simplifies the handling, and reduces the latency for retrieving
the TX timestamp, which increase the amount of TX timestamps that can
be handled in a given time period.
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
drivers/net/ethernet/intel/igc/igc.h | 2 +-
drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
drivers/net/ethernet/intel/igc/igc_ptp.c | 9 ++-------
3 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 7da0657ea48f..be1a1e67c39b 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -228,7 +228,6 @@ 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 are protected by the
* ptp_tx_lock.
*/
@@ -637,6 +636,7 @@ int igc_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr);
int igc_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr);
void igc_ptp_tx_hang(struct igc_adapter *adapter);
void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts);
+void igc_ptp_tx_work(struct igc_adapter *adapter);
#define igc_rx_pg_size(_ring) (PAGE_SIZE << igc_rx_pg_order(_ring))
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index b383352651a5..e6880b6ea187 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -5216,7 +5216,7 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
if (tsicr & IGC_TSICR_TXTS) {
/* retrieve hardware timestamp */
- schedule_work(&adapter->ptp_tx_work);
+ igc_ptp_tx_work(adapter);
ack |= IGC_TSICR_TXTS;
}
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 4dd0eec5a246..17e8970bd761 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -540,8 +540,6 @@ static void igc_ptp_clear_tx_tstamp(struct igc_adapter *adapter)
{
unsigned long flags;
- cancel_work_sync(&adapter->ptp_tx_work);
-
spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
dev_kfree_skb_any(adapter->ptp_tx_skb);
@@ -706,15 +704,13 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
/**
* igc_ptp_tx_work
- * @work: pointer to work struct
+ * @adapter: board private structure
*
* This work function checks the TSYNCTXCTL valid bit to determine when
* a timestamp has been taken for the current stored skb.
*/
-static void igc_ptp_tx_work(struct work_struct *work)
+void igc_ptp_tx_work(struct igc_adapter *adapter)
{
- struct igc_adapter *adapter = container_of(work, struct igc_adapter,
- ptp_tx_work);
struct igc_hw *hw = &adapter->hw;
unsigned long flags;
u32 tsynctxctl;
@@ -985,7 +981,6 @@ void igc_ptp_init(struct igc_adapter *adapter)
spin_lock_init(&adapter->ptp_tx_lock);
spin_lock_init(&adapter->tmreg_lock);
- INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
--
2.40.1
_______________________________________________
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] 14+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl v1 4/5] igc: Retrieve TX timestamp during interrupt handling
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 4/5] igc: Retrieve TX timestamp during interrupt handling Vinicius Costa Gomes
@ 2023-05-05 9:51 ` Kurt Kanzenbach
0 siblings, 0 replies; 14+ messages in thread
From: Kurt Kanzenbach @ 2023-05-05 9:51 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan; +Cc: vladimir.oltean, anthony.l.nguyen
[-- Attachment #1.1: Type: text/plain, Size: 517 bytes --]
On Thu May 04 2023, Vinicius Costa Gomes wrote:
> When the interrupt is handled, the TXTT_0 bit in the TSYNCTXCTL
> register should already be set and the timestamp value already loaded
> in the appropriate register.
>
> This simplifies the handling, and reduces the latency for retrieving
> the TX timestamp, which increase the amount of TX timestamps that can
> be handled in a given time period.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
[-- Attachment #2: Type: text/plain, Size: 162 bytes --]
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Intel-wired-lan] [PATCH iwl v1 5/5] igc: Add workaround for missing timestamps
2023-05-04 23:52 [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes Vinicius Costa Gomes
` (3 preceding siblings ...)
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 4/5] igc: Retrieve TX timestamp during interrupt handling Vinicius Costa Gomes
@ 2023-05-04 23:52 ` Vinicius Costa Gomes
2023-05-08 20:55 ` [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes Tony Nguyen
5 siblings, 0 replies; 14+ messages in thread
From: Vinicius Costa Gomes @ 2023-05-04 23:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: vladimir.oltean, kurt, anthony.l.nguyen
There's an hardware issue that can cause missing timestamps. The bug
is that the interrupt is only cleared if the IGC_TXSTMPH_0 register is
read.
The bug can cause a race condition if a timestamp is captured at the
wrong time, and we will miss that timestamp. To reduce the time window
that the problem is able to happen, in case no timestamp was ready, we
read the "previous" value of the timestamp registers, and we compare
with the "current" one, if it didn't change we can reasonably sure
that no timestamp was captured. If they are different, we use the new
value as the captured timestamp.
This workaround has more impact when multiple timestamp registers are
used, and the IGC_TXSTMPH_0 register always need to be read, so the
interrupt is cleared.
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
drivers/net/ethernet/intel/igc/igc_ptp.c | 48 ++++++++++++++++++------
1 file changed, 37 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 17e8970bd761..47a2140f9144 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -666,14 +666,49 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
struct sk_buff *skb = adapter->ptp_tx_skb;
struct skb_shared_hwtstamps shhwtstamps;
struct igc_hw *hw = &adapter->hw;
+ u32 tsynctxctl;
int adjust = 0;
u64 regval;
if (WARN_ON_ONCE(!skb))
return;
- regval = rd32(IGC_TXSTMPL);
- regval |= (u64)rd32(IGC_TXSTMPH) << 32;
+ tsynctxctl = rd32(IGC_TSYNCTXCTL);
+ tsynctxctl &= IGC_TSYNCTXCTL_TXTT_0;
+ if (tsynctxctl) {
+ regval = rd32(IGC_TXSTMPL);
+ regval |= (u64)rd32(IGC_TXSTMPH) << 32;
+ } else {
+ /* There's a bug in the hardware that could cause
+ * missing interrupts for TX timestamping. The issue
+ * is that for new interrupts to be triggered, the
+ * IGC_TXSTMPH_0 register must be read.
+ *
+ * To avoid discarding a valid timestamp that just
+ * happened at the "wrong" time, we need to confirm
+ * that there was no timestamp captured, we do that by
+ * assuming that no two timestamps in sequence have
+ * the same nanosecond value.
+ *
+ * So, we read the "low" register, read the "high"
+ * register (to latch a new timestamp) and read the
+ * "low" register again, if "old" and "new" versions
+ * of the "low" register are different, a valid
+ * timestamp was captured, we can read the "high"
+ * register again.
+ */
+ u32 txstmpl_old, txstmpl_new;
+
+ txstmpl_old = rd32(IGC_TXSTMPL);
+ rd32(IGC_TXSTMPH);
+ txstmpl_new = rd32(IGC_TXSTMPL);
+
+ if (txstmpl_old == txstmpl_new)
+ return;
+
+ regval = txstmpl_new;
+ regval |= (u64)rd32(IGC_TXSTMPH) << 32;
+ }
if (igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval))
return;
@@ -711,22 +746,13 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
*/
void igc_ptp_tx_work(struct igc_adapter *adapter)
{
- struct igc_hw *hw = &adapter->hw;
unsigned long flags;
- u32 tsynctxctl;
spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
if (!adapter->ptp_tx_skb)
goto unlock;
- tsynctxctl = rd32(IGC_TSYNCTXCTL);
- tsynctxctl &= IGC_TSYNCTXCTL_TXTT_0;
- if (!tsynctxctl) {
- WARN_ONCE(1, "Received a TSTAMP interrupt but no TSTAMP is ready.\n");
- goto unlock;
- }
-
igc_ptp_tx_hwtstamp(adapter);
unlock:
--
2.40.1
_______________________________________________
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] 14+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes
2023-05-04 23:52 [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes Vinicius Costa Gomes
` (4 preceding siblings ...)
2023-05-04 23:52 ` [Intel-wired-lan] [PATCH iwl v1 5/5] igc: Add workaround for missing timestamps Vinicius Costa Gomes
@ 2023-05-08 20:55 ` Tony Nguyen
2023-05-08 22:18 ` Vinicius Costa Gomes
5 siblings, 1 reply; 14+ messages in thread
From: Tony Nguyen @ 2023-05-08 20:55 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan; +Cc: kurt, vladimir.oltean
On 5/4/2023 4:52 PM, Vinicius Costa Gomes wrote:
> Hi,
>
> Changes from the "for-next-queue" version:
> - As this is intended for the iwl/net-queue tree, removed adding
> support for adding the "extra" tstamp registers;
> - Added "Fixes:" tags to the appropriate patches (Vladimir Oltean);
In most cases, net patches should have Fixes: tags to them. Patches 3
and 5 don't have them and it seems like it would be applicable to them.
Patch 4 seems more like an improvement than a bug fix? If so, -next
would seem a better path for that patch. Based on the 'for-next-queue
version' link, there are still some patches remaining that will go
through -next? Perhaps this can go with them.
> - Improved the check to catch the case that the skb has the
> SKBTX_HW_TSTAMP flag, but TX timestamping is not enabled (Vladimir
> Oltean);
> - Ony check for timestamping timeouts if TX timestamping is enabled
> (Vladimir Oltean);
>
> for-next-queue version link:
> https://lore.kernel.org/intel-wired-lan/20230228054534.1093483-1-vinicius.gomes@intel.com/
...
> BTW: I hope this is the correct usage of the "iwl" subject prefix.
If you could also add -net|-next for the (eventual) target tree
i.e.
net : iwl-net
net-next : iwl-next
in this case 'iwl-net'
Thanks,
Tony
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes
2023-05-08 20:55 ` [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes Tony Nguyen
@ 2023-05-08 22:18 ` Vinicius Costa Gomes
2023-05-09 17:23 ` Tony Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Vinicius Costa Gomes @ 2023-05-08 22:18 UTC (permalink / raw)
To: Tony Nguyen, intel-wired-lan; +Cc: kurt, vladimir.oltean
Tony Nguyen <anthony.l.nguyen@intel.com> writes:
> On 5/4/2023 4:52 PM, Vinicius Costa Gomes wrote:
>> Hi,
>>
>> Changes from the "for-next-queue" version:
>> - As this is intended for the iwl/net-queue tree, removed adding
>> support for adding the "extra" tstamp registers;
>> - Added "Fixes:" tags to the appropriate patches (Vladimir Oltean);
>
> In most cases, net patches should have Fixes: tags to them. Patches 3
> and 5 don't have them and it seems like it would be applicable to them.
>
Patch 3 is directly related to patch 1, but I think it deserved a
separate commit, as it has a bit of refactor. I can squash it into patch
1, if you think it's better I can do that, no worries, I was only afraid
to make the patch harder to follow.
Patch 5, as a hardware issue workaround, I didn't know if adding a
'Fixes:' tag made sense, but as a way to direct patches to the right
stable trees, that would be a good point in favor, even if it's not
fixing a bug in the code. Is this what you had in mind? If so, I can do
that.
> Patch 4 seems more like an improvement than a bug fix? If so, -next
> would seem a better path for that patch. Based on the 'for-next-queue
> version' link, there are still some patches remaining that will go
> through -next? Perhaps this can go with them.
>
On a very loaded system, for example, time synchronization can fail if
something blocks the system workqueue from running, so in a sense, that
patches fixes/helps some user visible issues. But I can see it both
ways, that this is an improvement. What's your preference?
>> - Improved the check to catch the case that the skb has the
>> SKBTX_HW_TSTAMP flag, but TX timestamping is not enabled (Vladimir
>> Oltean);
>> - Ony check for timestamping timeouts if TX timestamping is enabled
>> (Vladimir Oltean);
>>
>> for-next-queue version link:
>> https://lore.kernel.org/intel-wired-lan/20230228054534.1093483-1-vinicius.gomes@intel.com/
>
> ...
>
>> BTW: I hope this is the correct usage of the "iwl" subject prefix.
>
> If you could also add -net|-next for the (eventual) target tree
> i.e.
> net : iwl-net
> net-next : iwl-next
>
> in this case 'iwl-net'
Yeah, I sent this patch a couple minutes before seeing the email about
the subject prefix conventions. Will use the correct one next time.
>
> Thanks,
> Tony
--
Vinicius
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes
2023-05-08 22:18 ` Vinicius Costa Gomes
@ 2023-05-09 17:23 ` Tony Nguyen
2023-05-09 20:51 ` Vinicius Costa Gomes
0 siblings, 1 reply; 14+ messages in thread
From: Tony Nguyen @ 2023-05-09 17:23 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan; +Cc: kurt, vladimir.oltean
On 5/8/2023 3:18 PM, Vinicius Costa Gomes wrote:
> Tony Nguyen <anthony.l.nguyen@intel.com> writes:
>
>> On 5/4/2023 4:52 PM, Vinicius Costa Gomes wrote:
>>> Hi,
>>>
>>> Changes from the "for-next-queue" version:
>>> - As this is intended for the iwl/net-queue tree, removed adding
>>> support for adding the "extra" tstamp registers;
>>> - Added "Fixes:" tags to the appropriate patches (Vladimir Oltean);
>>
>> In most cases, net patches should have Fixes: tags to them. Patches 3
>> and 5 don't have them and it seems like it would be applicable to them.
>>
>
> Patch 3 is directly related to patch 1, but I think it deserved a
> separate commit, as it has a bit of refactor. I can squash it into patch
> 1, if you think it's better I can do that, no worries, I was only afraid
> to make the patch harder to follow.
I understand the reasoning and makes sense, however, I want to say I
recently read on netdev a comment for keeping it in one patch for ease
of backport.
> Patch 5, as a hardware issue workaround, I didn't know if adding a
> 'Fixes:' tag made sense, but as a way to direct patches to the right
> stable trees, that would be a good point in favor, even if it's not
> fixing a bug in the code. Is this what you had in mind? If so, I can do
> that.
Yea, I think a hint on how far back to backport would be valuable. I
believe even though it's a workaround, from user perspective, it would
appear as a bug(?)
>> Patch 4 seems more like an improvement than a bug fix? If so, -next
>> would seem a better path for that patch. Based on the 'for-next-queue
>> version' link, there are still some patches remaining that will go
>> through -next? Perhaps this can go with them.
>>
>
> On a very loaded system, for example, time synchronization can fail if
> something blocks the system workqueue from running, so in a sense, that
> patches fixes/helps some user visible issues. But I can see it both
> ways, that this is an improvement. What's your preference?
I think I'd rather err on the side of fixing and it's already here :)
Thanks,
Tony
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl v1 0/5] igc: TX timestamping fixes
2023-05-09 17:23 ` Tony Nguyen
@ 2023-05-09 20:51 ` Vinicius Costa Gomes
0 siblings, 0 replies; 14+ messages in thread
From: Vinicius Costa Gomes @ 2023-05-09 20:51 UTC (permalink / raw)
To: Tony Nguyen, intel-wired-lan; +Cc: kurt, vladimir.oltean
Hi Tony,
Tony Nguyen <anthony.l.nguyen@intel.com> writes:
> On 5/8/2023 3:18 PM, Vinicius Costa Gomes wrote:
>> Tony Nguyen <anthony.l.nguyen@intel.com> writes:
>>
>>> On 5/4/2023 4:52 PM, Vinicius Costa Gomes wrote:
>>>> Hi,
>>>>
>>>> Changes from the "for-next-queue" version:
>>>> - As this is intended for the iwl/net-queue tree, removed adding
>>>> support for adding the "extra" tstamp registers;
>>>> - Added "Fixes:" tags to the appropriate patches (Vladimir Oltean);
>>>
>>> In most cases, net patches should have Fixes: tags to them. Patches 3
>>> and 5 don't have them and it seems like it would be applicable to them.
>>>
>>
>> Patch 3 is directly related to patch 1, but I think it deserved a
>> separate commit, as it has a bit of refactor. I can squash it into patch
>> 1, if you think it's better I can do that, no worries, I was only afraid
>> to make the patch harder to follow.
>
> I understand the reasoning and makes sense, however, I want to say I
> recently read on netdev a comment for keeping it in one patch for ease
> of backport.
>
Makes sense. Will squash it.
>> Patch 5, as a hardware issue workaround, I didn't know if adding a
>> 'Fixes:' tag made sense, but as a way to direct patches to the right
>> stable trees, that would be a good point in favor, even if it's not
>> fixing a bug in the code. Is this what you had in mind? If so, I can do
>> that.
>
> Yea, I think a hint on how far back to backport would be valuable. I
> believe even though it's a workaround, from user perspective, it would
> appear as a bug(?)
>
Will add the 'Fixes:' tag.
>>> Patch 4 seems more like an improvement than a bug fix? If so, -next
>>> would seem a better path for that patch. Based on the 'for-next-queue
>>> version' link, there are still some patches remaining that will go
>>> through -next? Perhaps this can go with them.
>>>
>>
>> On a very loaded system, for example, time synchronization can fail if
>> something blocks the system workqueue from running, so in a sense, that
>> patches fixes/helps some user visible issues. But I can see it both
>> ways, that this is an improvement. What's your preference?
>
> I think I'd rather err on the side of fixing and it's already here :)
>
Understood. Will keep proposing it here for 'iwl-net'.
Will send the v2 soon.
Thank you,
--
Vinicius
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 14+ messages in thread