Message ID | 20201009025349.4037-2-andre.guedes@intel.com |
---|---|
State | Superseded |
Headers | show |
Series | igc: Add XDP support | expand |
On Thu, Oct 08, 2020 at 07:53:41PM -0700, Andre Guedes wrote: > The comment describing the timestamps layout in the packet buffer is > wrong and the code is actually retrieving the timestamp in Timer 1 > reference instead of Timer 0. This hasn't been a big issue so far > because hardware is configured to report both timestamps using Timer 0 > (see IGC_SRRCTL register configuration in igc_ptp_enable_rx_timestamp() > helper). This patch fixes the comment and the code so we retrieve the > timestamp in Timer 0 reference as expected. > > This patch also takes the opportunity to get rid of the hw.mac.type check > since it is not required. How is this related to adding XDP support? > > Fixes: 81b055205e8ba ("igc: Add support for RX timestamping") > Signed-off-by: Andre Guedes <andre.guedes@intel.com> > --- > drivers/net/ethernet/intel/igc/igc.h | 2 +- > drivers/net/ethernet/intel/igc/igc_ptp.c | 72 +++++++++++++----------- > 2 files changed, 41 insertions(+), 33 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index 35baae900c1f..b9b924794b22 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -545,7 +545,7 @@ void igc_ptp_init(struct igc_adapter *adapter); > void igc_ptp_reset(struct igc_adapter *adapter); > void igc_ptp_suspend(struct igc_adapter *adapter); > void igc_ptp_stop(struct igc_adapter *adapter); > -void igc_ptp_rx_pktstamp(struct igc_q_vector *q_vector, void *va, > +void igc_ptp_rx_pktstamp(struct igc_q_vector *q_vector, u32 *va, > struct sk_buff *skb); > 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); > diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c > index ac0b9c85da7c..916721e2cd49 100644 > --- a/drivers/net/ethernet/intel/igc/igc_ptp.c > +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c > @@ -152,46 +152,54 @@ static void igc_ptp_systim_to_hwtstamp(struct igc_adapter *adapter, > } > > /** > - * igc_ptp_rx_pktstamp - retrieve Rx per packet timestamp > + * igc_ptp_rx_pktstamp - Retrieve timestamp from rx packet buffer > * @q_vector: Pointer to interrupt specific structure > * @va: Pointer to address containing Rx buffer > * @skb: Buffer containing timestamp and packet > * > - * This function is meant to retrieve the first timestamp from the > - * first buffer of an incoming frame. The value is stored in little > - * endian format starting on byte 0. There's a second timestamp > - * starting on byte 8. > - **/ > -void igc_ptp_rx_pktstamp(struct igc_q_vector *q_vector, void *va, > + * This function retrieves the timestamp saved in the beginning of packet > + * buffer. While two timestamps are available, one in timer0 reference and the > + * other in timer1 reference, this function considers only the timestamp in > + * timer0 reference. > + */ > +void igc_ptp_rx_pktstamp(struct igc_q_vector *q_vector, u32 *va, > struct sk_buff *skb) > { > struct igc_adapter *adapter = q_vector->adapter; > - __le64 *regval = (__le64 *)va; > - int adjust = 0; > - > - /* The timestamp is recorded in little endian format. > - * DWORD: | 0 | 1 | 2 | 3 > - * Field: | Timer0 Low | Timer0 High | Timer1 Low | Timer1 High > + u64 regval; > + int adjust; > + > + /* Timestamps are saved in little endian at the beginning of the packet > + * buffer following the layout: > + * > + * | 0 | 1 | 2 | 3 | > + * | Timer1 SYSTIML | Timer1 SYSTIMH | Timer0 SYSTIML | Timer0 SYSTIMH | > + * > + * SYSTIML holds the nanoseconds part while SYSTIMH holds the seconds > + * part of the timestamp. > */ > - igc_ptp_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), > - le64_to_cpu(regval[0])); > - > - /* adjust timestamp for the RX latency based on link speed */ > - if (adapter->hw.mac.type == igc_i225) { > - switch (adapter->link_speed) { > - case SPEED_10: > - adjust = IGC_I225_RX_LATENCY_10; > - break; > - case SPEED_100: > - adjust = IGC_I225_RX_LATENCY_100; > - break; > - case SPEED_1000: > - adjust = IGC_I225_RX_LATENCY_1000; > - break; > - case SPEED_2500: > - adjust = IGC_I225_RX_LATENCY_2500; > - break; > - } > + regval = le32_to_cpu(va[2]); > + regval |= (u64)le32_to_cpu(va[3]) << 32; > + igc_ptp_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), regval); > + > + /* Adjust timestamp for the RX latency based on link speed */ > + switch (adapter->link_speed) { > + case SPEED_10: > + adjust = IGC_I225_RX_LATENCY_10; > + break; > + case SPEED_100: > + adjust = IGC_I225_RX_LATENCY_100; > + break; > + case SPEED_1000: > + adjust = IGC_I225_RX_LATENCY_1000; > + break; > + case SPEED_2500: > + adjust = IGC_I225_RX_LATENCY_2500; > + break; > + default: > + adjust = 0; > + netdev_warn_once(adapter->netdev, "Imprecise timestamp\n"); > + break; > } > skb_hwtstamps(skb)->hwtstamp = > ktime_sub_ns(skb_hwtstamps(skb)->hwtstamp, adjust); > -- > 2.26.2 > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Hi Maciej, Quoting Maciej Fijalkowski (2020-10-21 06:50:05) > On Thu, Oct 08, 2020 at 07:53:41PM -0700, Andre Guedes wrote: > > The comment describing the timestamps layout in the packet buffer is > > wrong and the code is actually retrieving the timestamp in Timer 1 > > reference instead of Timer 0. This hasn't been a big issue so far > > because hardware is configured to report both timestamps using Timer 0 > > (see IGC_SRRCTL register configuration in igc_ptp_enable_rx_timestamp() > > helper). This patch fixes the comment and the code so we retrieve the > > timestamp in Timer 0 reference as expected. > > > > This patch also takes the opportunity to get rid of the hw.mac.type check > > since it is not required. > > How is this related to adding XDP support? It is not directly related, however patches in this series depends on this patch to apply (see "igc: Refactor rx timestamp handling"). That's why I put it as the first patch in the series. - Andre
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index 35baae900c1f..b9b924794b22 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -545,7 +545,7 @@ void igc_ptp_init(struct igc_adapter *adapter); void igc_ptp_reset(struct igc_adapter *adapter); void igc_ptp_suspend(struct igc_adapter *adapter); void igc_ptp_stop(struct igc_adapter *adapter); -void igc_ptp_rx_pktstamp(struct igc_q_vector *q_vector, void *va, +void igc_ptp_rx_pktstamp(struct igc_q_vector *q_vector, u32 *va, struct sk_buff *skb); 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); diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c index ac0b9c85da7c..916721e2cd49 100644 --- a/drivers/net/ethernet/intel/igc/igc_ptp.c +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c @@ -152,46 +152,54 @@ static void igc_ptp_systim_to_hwtstamp(struct igc_adapter *adapter, } /** - * igc_ptp_rx_pktstamp - retrieve Rx per packet timestamp + * igc_ptp_rx_pktstamp - Retrieve timestamp from rx packet buffer * @q_vector: Pointer to interrupt specific structure * @va: Pointer to address containing Rx buffer * @skb: Buffer containing timestamp and packet * - * This function is meant to retrieve the first timestamp from the - * first buffer of an incoming frame. The value is stored in little - * endian format starting on byte 0. There's a second timestamp - * starting on byte 8. - **/ -void igc_ptp_rx_pktstamp(struct igc_q_vector *q_vector, void *va, + * This function retrieves the timestamp saved in the beginning of packet + * buffer. While two timestamps are available, one in timer0 reference and the + * other in timer1 reference, this function considers only the timestamp in + * timer0 reference. + */ +void igc_ptp_rx_pktstamp(struct igc_q_vector *q_vector, u32 *va, struct sk_buff *skb) { struct igc_adapter *adapter = q_vector->adapter; - __le64 *regval = (__le64 *)va; - int adjust = 0; - - /* The timestamp is recorded in little endian format. - * DWORD: | 0 | 1 | 2 | 3 - * Field: | Timer0 Low | Timer0 High | Timer1 Low | Timer1 High + u64 regval; + int adjust; + + /* Timestamps are saved in little endian at the beginning of the packet + * buffer following the layout: + * + * | 0 | 1 | 2 | 3 | + * | Timer1 SYSTIML | Timer1 SYSTIMH | Timer0 SYSTIML | Timer0 SYSTIMH | + * + * SYSTIML holds the nanoseconds part while SYSTIMH holds the seconds + * part of the timestamp. */ - igc_ptp_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), - le64_to_cpu(regval[0])); - - /* adjust timestamp for the RX latency based on link speed */ - if (adapter->hw.mac.type == igc_i225) { - switch (adapter->link_speed) { - case SPEED_10: - adjust = IGC_I225_RX_LATENCY_10; - break; - case SPEED_100: - adjust = IGC_I225_RX_LATENCY_100; - break; - case SPEED_1000: - adjust = IGC_I225_RX_LATENCY_1000; - break; - case SPEED_2500: - adjust = IGC_I225_RX_LATENCY_2500; - break; - } + regval = le32_to_cpu(va[2]); + regval |= (u64)le32_to_cpu(va[3]) << 32; + igc_ptp_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), regval); + + /* Adjust timestamp for the RX latency based on link speed */ + switch (adapter->link_speed) { + case SPEED_10: + adjust = IGC_I225_RX_LATENCY_10; + break; + case SPEED_100: + adjust = IGC_I225_RX_LATENCY_100; + break; + case SPEED_1000: + adjust = IGC_I225_RX_LATENCY_1000; + break; + case SPEED_2500: + adjust = IGC_I225_RX_LATENCY_2500; + break; + default: + adjust = 0; + netdev_warn_once(adapter->netdev, "Imprecise timestamp\n"); + break; } skb_hwtstamps(skb)->hwtstamp = ktime_sub_ns(skb_hwtstamps(skb)->hwtstamp, adjust);
The comment describing the timestamps layout in the packet buffer is wrong and the code is actually retrieving the timestamp in Timer 1 reference instead of Timer 0. This hasn't been a big issue so far because hardware is configured to report both timestamps using Timer 0 (see IGC_SRRCTL register configuration in igc_ptp_enable_rx_timestamp() helper). This patch fixes the comment and the code so we retrieve the timestamp in Timer 0 reference as expected. This patch also takes the opportunity to get rid of the hw.mac.type check since it is not required. Fixes: 81b055205e8ba ("igc: Add support for RX timestamping") Signed-off-by: Andre Guedes <andre.guedes@intel.com> --- drivers/net/ethernet/intel/igc/igc.h | 2 +- drivers/net/ethernet/intel/igc/igc_ptp.c | 72 +++++++++++++----------- 2 files changed, 41 insertions(+), 33 deletions(-)