> -----Original Message----- > From: Guilherme G. Piccoli <gpicc...@canonical.com> > Sent: Thursday, June 27, 2019 10:02 PM > To: GR-everest-linux-l2 <gr-everest-linux...@marvell.com>; > netdev@vger.kernel.org; Sudarsana Reddy Kalluru <skall...@marvell.com> > Cc: Ariel Elior <ael...@marvell.com>; gpicc...@canonical.com; > jay.vosbu...@canonical.com > Subject: [EXT] [PATCH V4] bnx2x: Prevent ptp_task to be rescheduled > indefinitely > > External Email > > ---------------------------------------------------------------------- > 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.haus...@canonical.com> > Suggested-by: Sudarsana Reddy Kalluru <skall...@marvell.com> > Signed-off-by: Guilherme G. Piccoli <gpicc...@canonical.com> > --- > > Thanks again for your review Sudarsana. I've addressed in this V4 your > suggestions about removing some debug messages[0]. > > [0] https://marc.info/?l=linux-netdev&m=156165243804760 > > .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 5 ++- > .../ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 4 ++- > .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 33 ++++++++++++++---- > - .../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 008ad0ca89ba..c12c1bab0fe4 100644 > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c > @@ -3857,9 +3857,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++; > + netdev_err_once(bp->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 51fc845de31a..4a0ba6801c9e 100644 > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c > @@ -182,7 +182,9 @@ static const struct { > { 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 03ac10b1cd1e..2cc14db8f0ec 100644 > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c > @@ -15214,11 +15214,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; > > - /* 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) { > + /* 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); > + } > + > + 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); > @@ -15233,16 +15246,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 b2644ed13d06..d55e63692cf3 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 { > -- > 2.22.0
Thanks for the changes. Acked-by: Sudarsana Reddy Kalluru <skall...@marvell.com>
