From 63c349cafbacebc9f4a4d826c172c70381926853 Mon Sep 17 00:00:00 2001 From: Martin Pecka Date: Wed, 26 Jun 2024 01:22:06 +0200 Subject: [PATCH] Fixed PTP software timestamping. The TX path ignored whether the SKB wanted a SW or HW stamp, and once HW timestamping was enabled for the whole NIC, it always returned HW stamps. With this fix, if the TX packet requests SW stamp, it correctly only gets SW stamps. On the RX path, the driver has started in an inconsistent state. By default, no timestamping should be done for RX packets. However, the driver automatically enabled RX timestamping on initialization. This fix correctly initializes the NIC with no timestamping and it needs to be set up via SIOCSHWTSTAMP. There was also a problem with the RX path on init. The NIC did not put PTP packets on the PTP ring, but it did append timestamps to them. That resulted in wrong RX packets which were 12/14 bytes longer, because the timestamp length is only subtracted from PTP ring packets. This was worked around by adding a "flipping" enable_ptp() call that first sets the opposite value and then the actually wanted one. This does not seem to be needed for ATL2 chips. Signed-off-by: Martin Pecka --- aq_main.c | 5 ++-- aq_nic.h | 1 + aq_ptp.c | 59 ++++++++++++++++++++++++++++++++++++---------- hw_atl/hw_atl_a0.c | 4 ++++ hw_atl/hw_atl_b0.c | 8 +++++-- 5 files changed, 60 insertions(+), 17 deletions(-) diff --git a/aq_main.c b/aq_main.c index 4d2f0de..48def4b 100644 --- a/aq_main.c +++ b/aq_main.c @@ -127,8 +127,9 @@ static netdev_tx_t aq_ndev_start_xmit(struct sk_buff *skb, struct net_device *nd #if IS_REACHABLE(CONFIG_PTP_1588_CLOCK) if (unlikely(aq_utils_obj_test(&aq_nic->flags, AQ_NIC_PTP_DPATH_UP))) { /* Hardware adds the Timestamp for PTPv2 802.AS1 - * and PTPv2 IPv4 UDP. - * We have to push even general 320 port messages to the ptp + * and PTPv2 IPv4 UDP. Opposed to what is needed, + * it timestamps both event and general messages. That is why + * we have to push even port 320 (general) messages to the ptp * queue explicitly. This is a limitation of current firmware * and hardware PTP design of the chip. Otherwise ptp stream * will fail to sync diff --git a/aq_nic.h b/aq_nic.h index 3d2e6bf..3845cad 100644 --- a/aq_nic.h +++ b/aq_nic.h @@ -96,6 +96,7 @@ struct aq_nic_cfg_s { #define AQ_NIC_FLAG_ERR_HW 0x80000000U #define AQ_NIC_QUIRK_BAD_PTP BIT(0) +#define AQ_NIC_QUIRK_NEED_PTP_FLIP BIT(1) #ifdef PCI_DEBUG #define AQ_NIC_PCI_RESOURCE_BUSY 0x00800000U diff --git a/aq_ptp.c b/aq_ptp.c index f78f5fc..0a3db99 100644 --- a/aq_ptp.c +++ b/aq_ptp.c @@ -1075,15 +1075,32 @@ void aq_ptp_tx_hwtstamp(struct aq_nic_s *aq_nic, u64 timestamp) netdev_err(aq_nic->ndev, "have timestamp but tx_queues empty\n"); return; } - - timestamp += atomic_read(&aq_ptp->offset_egress); - aq_ptp_convert_to_hwtstamp(aq_ptp, &hwtstamp, timestamp); - skb_tstamp_tx(skb, &hwtstamp); + // Only report the HW timestamp if it was requested. Otherwise, + // SW timestamping would be broken. + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) { + timestamp += atomic_read(&aq_ptp->offset_egress); + aq_ptp_convert_to_hwtstamp(aq_ptp, &hwtstamp, timestamp); + skb_tstamp_tx(skb, &hwtstamp); + } dev_kfree_skb_any(skb); aq_ptp_tx_timeout_update(aq_ptp); } +static void aq_ptp_enable_ptp(struct aq_ptp_s *aq_ptp, int enable) +{ + struct aq_nic_s *aq_nic = aq_ptp->aq_nic; + if (aq_ptp->a1_ptp) { + mutex_lock(&aq_nic->fwreq_mutex); + aq_nic->aq_fw_ops->enable_ptp(aq_nic->aq_hw, enable); + mutex_unlock(&aq_nic->fwreq_mutex); + } + + if (aq_ptp->a2_ptp) + aq_nic->aq_hw_ops->enable_ptp(aq_nic->aq_hw, + aq_ptp->ptp_clock_sel, enable); +} + static int aq_ptp_dpath_enable(struct aq_ptp_s *aq_ptp, int enable_flags, u16 rx_queue) { @@ -1099,6 +1116,10 @@ static int aq_ptp_dpath_enable(struct aq_ptp_s *aq_ptp, "%sable ptp filters: %x.\n", enable_flags ? "En" : "Dis", enable_flags); + // Some NICs need to flip the bit twice to actually start/stop PTP + if (aq_nic->aq_nic_cfg.aq_hw_caps->quirks & AQ_NIC_QUIRK_NEED_PTP_FLIP) + aq_ptp_enable_ptp(aq_ptp, enable_flags ? 0 : 1); + if (enable_flags) { if (enable_flags & (AQ_HW_PTP_L4_ENABLE)) { if (aq_ptp->a1_ptp) { @@ -1260,6 +1281,11 @@ static int aq_ptp_dpath_enable(struct aq_ptp_s *aq_ptp, "Set L2 filter complete. Location: %d\n", aq_ptp->eth_type_filter.location); } + + if (!err) { + aq_ptp_enable_ptp(aq_ptp, 1); + netdev_info(aq_nic->ndev, "PTP filters enabled.\n"); + } } else { /* PTP disabled, clear all UDP/L2 filters */ for (i = 0; i < PTP_UDP_FILTERS_CNT; i++) { @@ -1273,6 +1299,9 @@ static int aq_ptp_dpath_enable(struct aq_ptp_s *aq_ptp, if (!err && hw_ops->hw_filter_l2_clear) err = hw_ops->hw_filter_l2_clear(aq_nic->aq_hw, &aq_ptp->eth_type_filter); + + aq_ptp_enable_ptp(aq_ptp, 0); + netdev_info(aq_nic->ndev, "PTP filters disabled.\n"); } return err; @@ -1345,6 +1374,13 @@ int aq_ptp_hwtstamp_config_set(struct aq_ptp_s *aq_ptp, return -ERANGE; } + if (ptp_en_flags != AQ_HW_PTP_DISABLE && + !aq_utils_obj_test(&aq_nic->aq_hw->flags, AQ_HW_PTP_AVAILABLE)) { + config->rx_filter = HWTSTAMP_FILTER_NONE; + netdev_err(aq_nic->ndev, "PTP timestamping requested but not supported.\n"); + return -EOPNOTSUPP; + } + if (aq_ptp->hwtstamp_config.rx_filter != config->rx_filter) err = aq_ptp_dpath_enable(aq_ptp, ptp_en_flags, @@ -1537,7 +1573,10 @@ int aq_ptp_xmit(struct aq_nic_s *aq_nic, struct sk_buff *skb) ring->size); return NETDEV_TX_BUSY; } - skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; + // SKBTX_IN_PROGRESS would report SW timestamp reporting in case only + // SW timestamps are requested. + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; aq_ptp_tx_timeout_start(aq_ptp); skb_tx_timestamp(skb); @@ -2013,7 +2052,7 @@ void aq_ptp_clock_init(struct aq_nic_s *aq_nic, enum aq_ptp_state state) } } - if (!aq_ptp->a1_ptp && state != AQ_PTP_FIRST_INIT) { + if (state != AQ_PTP_FIRST_INIT) { int ptp_en_flags = aq_ptp_parse_rx_filters(state == AQ_PTP_LINK_UP ? aq_ptp->hwtstamp_config.rx_filter : @@ -2138,13 +2177,7 @@ int aq_ptp_init(struct aq_nic_s *aq_nic, unsigned int idx_ptp_vec, unsigned int /* enable ptp counter */ aq_ptp->ptp_clock_sel = ATL_TSG_CLOCK_SEL_0; //mbox.info.caps_ex.; aq_utils_obj_set(&aq_nic->aq_hw->flags, AQ_HW_PTP_AVAILABLE); - if (a1_ptp) { - mutex_lock(&aq_nic->fwreq_mutex); - aq_nic->aq_fw_ops->enable_ptp(aq_nic->aq_hw, 1); - mutex_unlock(&aq_nic->fwreq_mutex); - } - if (a2_ptp) - aq_nic->aq_hw_ops->enable_ptp(aq_nic->aq_hw, aq_ptp->ptp_clock_sel, 1); + aq_ptp_enable_ptp(aq_ptp, 1); INIT_DELAYED_WORK(&aq_ptp->poll_sync, &aq_ptp_poll_sync_work_cb); diff --git a/hw_atl/hw_atl_a0.c b/hw_atl/hw_atl_a0.c index 4625ccb..0b6b97e 100644 --- a/hw_atl/hw_atl_a0.c +++ b/hw_atl/hw_atl_a0.c @@ -54,6 +54,7 @@ const struct aq_hw_caps_s hw_atl_a0_caps_aqc100 = { AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, + .quirks = AQ_NIC_QUIRK_NEED_PTP_FLIP, }; const struct aq_hw_caps_s hw_atl_a0_caps_aqc107 = { @@ -64,6 +65,7 @@ const struct aq_hw_caps_s hw_atl_a0_caps_aqc107 = { AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, + .quirks = AQ_NIC_QUIRK_NEED_PTP_FLIP, }; const struct aq_hw_caps_s hw_atl_a0_caps_aqc108 = { @@ -73,6 +75,7 @@ const struct aq_hw_caps_s hw_atl_a0_caps_aqc108 = { AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, + .quirks = AQ_NIC_QUIRK_NEED_PTP_FLIP, }; const struct aq_hw_caps_s hw_atl_a0_caps_aqc109 = { @@ -81,6 +84,7 @@ const struct aq_hw_caps_s hw_atl_a0_caps_aqc109 = { .link_speed_msk = AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, + .quirks = AQ_NIC_QUIRK_NEED_PTP_FLIP, }; static int hw_atl_a0_hw_reset(struct aq_hw_s *self) diff --git a/hw_atl/hw_atl_b0.c b/hw_atl/hw_atl_b0.c index 650c1db..1de6718 100644 --- a/hw_atl/hw_atl_b0.c +++ b/hw_atl/hw_atl_b0.c @@ -69,6 +69,7 @@ const struct aq_hw_caps_s hw_atl_b0_caps_aqc100 = { AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, + .quirks = AQ_NIC_QUIRK_NEED_PTP_FLIP, .fw_image_name = AQ_FW_AQC100X, }; @@ -80,6 +81,7 @@ const struct aq_hw_caps_s hw_atl_b0_caps_aqc107 = { AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, + .quirks = AQ_NIC_QUIRK_NEED_PTP_FLIP, .fw_image_name = AQ_FW_AQC10XX, }; @@ -90,6 +92,7 @@ const struct aq_hw_caps_s hw_atl_b0_caps_aqc108 = { AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, + .quirks = AQ_NIC_QUIRK_NEED_PTP_FLIP, .fw_image_name = AQ_FW_AQC10XX, }; @@ -99,6 +102,7 @@ const struct aq_hw_caps_s hw_atl_b0_caps_aqc109 = { .link_speed_msk = AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, + .quirks = AQ_NIC_QUIRK_NEED_PTP_FLIP, .fw_image_name = AQ_FW_AQC10XX, }; @@ -109,7 +113,7 @@ const struct aq_hw_caps_s hw_atl_b0_caps_aqc111 = { AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, - .quirks = AQ_NIC_QUIRK_BAD_PTP, + .quirks = AQ_NIC_QUIRK_BAD_PTP | AQ_NIC_QUIRK_NEED_PTP_FLIP, .fw_image_name = AQ_FW_AQC11XX, }; @@ -119,7 +123,7 @@ const struct aq_hw_caps_s hw_atl_b0_caps_aqc112 = { .link_speed_msk = AQ_NIC_RATE_2G5 | AQ_NIC_RATE_1G | AQ_NIC_RATE_100M, - .quirks = AQ_NIC_QUIRK_BAD_PTP, + .quirks = AQ_NIC_QUIRK_BAD_PTP | AQ_NIC_QUIRK_NEED_PTP_FLIP, .fw_image_name = AQ_FW_AQC11XX, };