bnx2x: Prevent ptp_task to be rescheduled indefinitely [Linux 4.9.187]

This Linux kernel change "bnx2x: Prevent ptp_task to be rescheduled indefinitely" is included in the Linux 4.9.187 release. This change is authored by Guilherme G. Piccoli <gpiccoli [at] canonical.com> on Thu Jun 27 13:31:33 2019 -0300. The commit for this change in Linux stable tree is fdd098e (patch) which is from upstream commit 3c91f25. The same Linux upstream change may have been applied to various maintained Linux releases and you can find all Linux releases containing changes from upstream 3c91f25.

bnx2x: Prevent ptp_task to be rescheduled indefinitely

[ Upstream commit 3c91f25c2f72ba6001775a5932857c1d2131c531 ]

Currently bnx2x ptp worker tries to read a register with timestamp
information in case of TX packet timestamping and in case it fails,
the routine reschedules itself indefinitely. This was reported as a
kworker always at 100% of CPU usage, which was narrowed down to be
bnx2x ptp_task.

By following the ioctl handler, we could narrow down the problem to
an NTP tool (chrony) requesting HW timestamping from bnx2x NIC with
RX filter zeroed; this isn't reproducible for example with ptp4l
(from linuxptp) since this tool requests a supported RX filter.
It seems NIC FW timestamp mechanism cannot work well with
RX_FILTER_NONE - driver's PTP filter init routine skips a register
write to the adapter if there's not a supported filter request.

This patch addresses the problem of bnx2x ptp thread's everlasting
reschedule by retrying the register read 10 times; between the read
attempts the thread sleeps for an increasing amount of time starting
in 1ms to give FW some time to perform the timestamping. If it still
fails after all retries, we bail out in order to prevent an unbound
resource consumption from bnx2x.

The patch also adds an ethtool statistic for accounting the skipped
TX timestamp packets and it reduces the priority of timestamping
error messages to prevent log flooding. The code was tested using
both linuxptp and chrony.

Reported-and-tested-by: Przemyslaw Hausman <przemyslaw.hausman@canonical.com>
Suggested-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
Acked-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

There are 45 lines of Linux source code added/deleted in this change. Code changes to Linux kernel are as follows.

 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c    |  5 +++-
 .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c    |  4 ++-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c   | 33 ++++++++++++++++------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h  |  3 ++
 4 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index efe1268..6167bb0 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -3863,9 +3863,12 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)

    if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
        if (!(bp->flags & TX_TIMESTAMPING_EN)) {
+           bp->eth_stats.ptp_skip_tx_ts++;
            BNX2X_ERR("Tx timestamping was not enabled, this packet will not be timestamped\n");
        } else if (bp->ptp_tx_skb) {
-           BNX2X_ERR("The device supports only a single outstanding packet to timestamp, this packet will not be timestamped\n");
+           bp->eth_stats.ptp_skip_tx_ts++;
+           dev_err_once(&bp->dev->dev,
+                   "Device supports only a single outstanding packet to timestamp, this packet won't be timestamped\n");
        } else {
            skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
            /* schedule check for Tx timestamp */
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index 15a0850..b1992f4 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -182,7 +182,9 @@
    { STATS_OFFSET32(driver_filtered_tx_pkt),
                4, false, "driver_filtered_tx_pkt" },
    { STATS_OFFSET32(eee_tx_lpi),
-               4, true, "Tx LPI entry count"}
+               4, true, "Tx LPI entry count"},
+   { STATS_OFFSET32(ptp_skip_tx_ts),
+               4, false, "ptp_skipped_tx_tstamp" },
 };

 #define BNX2X_NUM_STATS        ARRAY_SIZE(bnx2x_stats_arr)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index eeeb4c5..2ef6012 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -15261,11 +15261,24 @@ static void bnx2x_ptp_task(struct work_struct *work)
    u32 val_seq;
    u64 timestamp, ns;
    struct skb_shared_hwtstamps shhwtstamps;
+   bool bail = true;
+   int i;
+
+   /* FW may take a while to complete timestamping; try a bit and if it's
+    * still not complete, may indicate an error state - bail out then.
+    */
+   for (i = 0; i < 10; i++) {
+       /* Read Tx timestamp registers */
+       val_seq = REG_RD(bp, port ? NIG_REG_P1_TLLH_PTP_BUF_SEQID :
+                NIG_REG_P0_TLLH_PTP_BUF_SEQID);
+       if (val_seq & 0x10000) {
+           bail = false;
+           break;
+       }
+       msleep(1 << i);
+   }

-   /* Read Tx timestamp registers */
-   val_seq = REG_RD(bp, port ? NIG_REG_P1_TLLH_PTP_BUF_SEQID :
-            NIG_REG_P0_TLLH_PTP_BUF_SEQID);
-   if (val_seq & 0x10000) {
+   if (!bail) {
        /* There is a valid timestamp value */
        timestamp = REG_RD(bp, port ? NIG_REG_P1_TLLH_PTP_BUF_TS_MSB :
                   NIG_REG_P0_TLLH_PTP_BUF_TS_MSB);
@@ -15280,16 +15293,18 @@ static void bnx2x_ptp_task(struct work_struct *work)
        memset(&shhwtstamps, 0, sizeof(shhwtstamps));
        shhwtstamps.hwtstamp = ns_to_ktime(ns);
        skb_tstamp_tx(bp->ptp_tx_skb, &shhwtstamps);
-       dev_kfree_skb_any(bp->ptp_tx_skb);
-       bp->ptp_tx_skb = NULL;

        DP(BNX2X_MSG_PTP, "Tx timestamp, timestamp cycles = %llu, ns = %llu\n",
           timestamp, ns);
    } else {
-       DP(BNX2X_MSG_PTP, "There is no valid Tx timestamp yet\n");
-       /* Reschedule to keep checking for a valid timestamp value */
-       schedule_work(&bp->ptp_task);
+       DP(BNX2X_MSG_PTP,
+          "Tx timestamp is not recorded (register read=%u)\n",
+          val_seq);
+       bp->eth_stats.ptp_skip_tx_ts++;
    }
+
+   dev_kfree_skb_any(bp->ptp_tx_skb);
+   bp->ptp_tx_skb = NULL;
 }

 void bnx2x_set_rx_ts(struct bnx2x *bp, struct sk_buff *skb)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h
index b2644ed..d55e636 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h
@@ -207,6 +207,9 @@ struct bnx2x_eth_stats {
    u32 driver_filtered_tx_pkt;
    /* src: Clear-on-Read register; Will not survive PMF Migration */
    u32 eee_tx_lpi;
+
+   /* PTP */
+   u32 ptp_skip_tx_ts;
 };

 struct bnx2x_eth_q_stats {

Leave a Reply

Your email address will not be published. Required fields are marked *