| Message ID | 20210210215848.24514-2-vedang.patel@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | igc: Add XDP support | expand |
On Wed, 2021-02-10 at 13:58 -0800, Vedang Patel wrote: > From: Andre Guedes <andre.guedes@intel.com> > > 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") This seems better suited for net. I can split it and send it that route, but is there a reason that it needs to stay in this series? > Signed-off-by: Andre Guedes <andre.guedes@intel.com> > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > Signed-off-by: Vedang Patel <vedang.patel@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 5d2809dfd06a..fbd8c414c3c8 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -547,7 +547,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..b6a640bfc991 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: > + * > + * DWORD: | 0 | 1 | 2 | > 3 | > + * Field: | 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; drivers/net/ethernet/intel/igc/igc_ptp.c:181:18: warning: cast to restricted __le32 drivers/net/ethernet/intel/igc/igc_ptp.c:182:24: warning: cast to restricted __le32
On Thu, 2021-02-11 at 00:30 +0000, Nguyen, Anthony L wrote: > On Wed, 2021-02-10 at 13:58 -0800, Vedang Patel wrote: > > From: Andre Guedes <andre.guedes@intel.com> > > > > 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") > > This seems better suited for net. I can split it and send it that > route, but is there a reason that it needs to stay in this series? > A subsequent patch in this series (f6ddfa4ba2ed - igc: Refactor Rx timestamp handling) is dependent on this patch. Please go ahead and split this patch from this series and send it via the "net route" you mention, if that is more appropriate. Let us know if you want us to resend the rest of the patches in this series (without the first patch) . Jithu
On 10/02/2021 23:58, Vedang Patel wrote: > From: Andre Guedes <andre.guedes@intel.com> > > 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> > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > Signed-off-by: Vedang Patel <vedang.patel@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 5d2809dfd06a..fbd8c414c3c8 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -547,7 +547,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..b6a640bfc991 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: > + * > + * DWORD: | 0 | 1 | 2 | 3 | > + * Field: | 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); > Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index 5d2809dfd06a..fbd8c414c3c8 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -547,7 +547,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..b6a640bfc991 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: + * + * DWORD: | 0 | 1 | 2 | 3 | + * Field: | 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);