[PATCH net-next] bnxt_en: add timestamping statistics support

[PATCH net-next] bnxt_en: add timestamping statistics support

[Vadim Fedorenko]

The ethtool_ts_stats structure was introduced earlier this year. Now
it's time to support this group of counters in more drivers.
This patch adds support to bnxt driver.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c      | 18 +++++++++++++-----
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 18 ++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c  | 18 ++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h  |  8 ++++++++
 4 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c437ca1c0fd3..f3db8b0fbbe6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -512,8 +512,11 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
 		struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 
-		if (ptp && ptp->tx_tstamp_en && !skb_is_gso(skb) &&
-		    atomic_dec_if_positive(&ptp->tx_avail) >= 0) {
+		if (ptp && ptp->tx_tstamp_en && !skb_is_gso(skb)) {
+			if (!atomic_dec_if_positive(&ptp->tx_avail)) {
+				atomic64_inc(&ptp->stats->ts_err);
+				goto tx_no_ts;
+			}
 			if (!bnxt_ptp_parse(skb, &ptp->tx_seqid,
 					    &ptp->tx_hdr_off)) {
 				if (vlan_tag_flags)
@@ -526,6 +529,7 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	}
 
+tx_no_ts:
 	if (unlikely(skb->no_fcs))
 		lflags |= cpu_to_le32(TX_BD_FLAGS_NO_CRC);
 
@@ -732,8 +736,10 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 
 tx_dma_error:
-	if (BNXT_TX_PTP_IS_SET(lflags))
+	if (BNXT_TX_PTP_IS_SET(lflags)) {
+		atomic64_inc(&bp->ptp_cfg->stats->ts_err);
 		atomic_inc(&bp->ptp_cfg->tx_avail);
+	}
 
 	last_frag = i;
 
@@ -812,10 +818,12 @@ static void __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) {
 			if (BNXT_CHIP_P5(bp)) {
 				/* PTP worker takes ownership of the skb */
-				if (!bnxt_get_tx_ts_p5(bp, skb))
+				if (!bnxt_get_tx_ts_p5(bp, skb)) {
 					skb = NULL;
-				else
+				} else {
+					atomic64_inc(&bp->ptp_cfg->stats->ts_err);
 					atomic_inc(&bp->ptp_cfg->tx_avail);
+				}
 			}
 		}
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 8763f8a01457..594e9967c591 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -5233,6 +5233,23 @@ static void bnxt_get_rmon_stats(struct net_device *dev,
 	*ranges = bnxt_rmon_ranges;
 }
 
+static void bnxt_get_ptp_stats(struct net_device *dev,
+			       struct ethtool_ts_stats *ts_stats)
+{
+	struct bnxt *bp = netdev_priv(dev);
+	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
+
+	if (ptp) {
+		ts_stats->pkts = ptp->stats->ts_pkts;
+		ts_stats->lost = ptp->stats->ts_lost;
+		ts_stats->err = atomic64_read(&ptp->stats->ts_err);
+	} else {
+		ts_stats->pkts = 0;
+		ts_stats->err = 0;
+		ts_stats->lost = 0;
+	}
+}
+
 static void bnxt_get_link_ext_stats(struct net_device *dev,
 				    struct ethtool_link_ext_stats *stats)
 {
@@ -5316,4 +5333,5 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 	.get_eth_mac_stats	= bnxt_get_eth_mac_stats,
 	.get_eth_ctrl_stats	= bnxt_get_eth_ctrl_stats,
 	.get_rmon_stats		= bnxt_get_rmon_stats,
+	.get_ts_stats		= bnxt_get_ptp_stats,
 };
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index e661ab154d6b..f75bcb8d43c0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -696,11 +696,13 @@ static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 		spin_unlock_bh(&ptp->ptp_lock);
 		timestamp.hwtstamp = ns_to_ktime(ns);
 		skb_tstamp_tx(ptp->tx_skb, &timestamp);
+		ptp->stats->ts_pkts++;
 	} else {
 		if (!time_after_eq(jiffies, ptp->abs_txts_tmo)) {
 			ptp->txts_pending = true;
 			return;
 		}
+		ptp->stats->ts_lost++;
 		netdev_warn_once(bp->dev,
 				 "TS query for TX timer failed rc = %x\n", rc);
 	}
@@ -979,6 +981,18 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
 		rc = err;
 		goto out;
 	}
+	if (!ptp->stats) {
+		ptp->stats = kzalloc(sizeof(*ptp->stats), GFP_KERNEL);
+		if (!ptp->stats) {
+			rc = -ENOMEM;
+			goto out;
+		}
+		atomic64_set(&ptp->stats->ts_err, 0);
+	} else {
+		ptp->stats->ts_pkts = 0;
+		ptp->stats->ts_lost = 0;
+		atomic64_set(&ptp->stats->ts_err, 0);
+	}
 	if (BNXT_CHIP_P5(bp)) {
 		spin_lock_bh(&ptp->ptp_lock);
 		bnxt_refclk_read(bp, NULL, &ptp->current_time);
@@ -1013,5 +1027,9 @@ void bnxt_ptp_clear(struct bnxt *bp)
 		dev_kfree_skb_any(ptp->tx_skb);
 		ptp->tx_skb = NULL;
 	}
+
+	kfree(ptp->stats);
+	ptp->stats = NULL;
+
 	bnxt_unmap_ptp_regs(bp);
 }
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index 2c3415c8fc03..589e093b1608 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -79,6 +79,12 @@ struct bnxt_pps {
 	struct pps_pin pins[BNXT_MAX_TSIO_PINS];
 };
 
+struct bnxt_ptp_stats {
+	u64		ts_pkts;
+	u64		ts_lost;
+	atomic64_t	ts_err;
+};
+
 struct bnxt_ptp_cfg {
 	struct ptp_clock_info	ptp_info;
 	struct ptp_clock	*ptp_clock;
@@ -125,6 +131,8 @@ struct bnxt_ptp_cfg {
 	u32			refclk_mapped_regs[2];
 	u32			txts_tmo;
 	unsigned long		abs_txts_tmo;
+
+	struct bnxt_ptp_stats	*stats;
 };
 
 #if BITS_PER_LONG == 32
-- 
2.43.0

Re: [PATCH net-next] bnxt_en: add timestamping statistics support

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]

On Wed, May 29, 2024 at 10:19 AM Vadim Fedorenko <vadfed@meta.com> wrote:
>
> The ethtool_ts_stats structure was introduced earlier this year. Now
> it's time to support this group of counters in more drivers.
> This patch adds support to bnxt driver.
>
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c      | 18 +++++++++++++-----
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 18 ++++++++++++++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c  | 18 ++++++++++++++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h  |  8 ++++++++
>  4 files changed, 57 insertions(+), 5 deletions(-)
>

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> index 2c3415c8fc03..589e093b1608 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> @@ -79,6 +79,12 @@ struct bnxt_pps {
>         struct pps_pin pins[BNXT_MAX_TSIO_PINS];
>  };
>
> +struct bnxt_ptp_stats {
> +       u64             ts_pkts;
> +       u64             ts_lost;
> +       atomic64_t      ts_err;
> +};
> +
>  struct bnxt_ptp_cfg {
>         struct ptp_clock_info   ptp_info;
>         struct ptp_clock        *ptp_clock;
> @@ -125,6 +131,8 @@ struct bnxt_ptp_cfg {
>         u32                     refclk_mapped_regs[2];
>         u32                     txts_tmo;
>         unsigned long           abs_txts_tmo;
> +
> +       struct bnxt_ptp_stats   *stats;

I think there is no need to allocate this small stats structure
separately.  It can just be:

struct bnxt_ptp_stats    stats;

The struct bnxt_ptp_cfg will only be allocated if the device supports
PTP.  So the stats can always be a part of struct bnxt_ptp_cfg.

Other than that, it looks good to me.  Thanks.

>  };
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next] bnxt_en: add timestamping statistics support

[Vadim Fedorenko]

On 29/05/2024 18:48, Michael Chan wrote:
> On Wed, May 29, 2024 at 10:19 AM Vadim Fedorenko <vadfed@meta.com> wrote:
>>
>> The ethtool_ts_stats structure was introduced earlier this year. Now
>> it's time to support this group of counters in more drivers.
>> This patch adds support to bnxt driver.
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>> ---
>>   drivers/net/ethernet/broadcom/bnxt/bnxt.c      | 18 +++++++++++++-----
>>   .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 18 ++++++++++++++++++
>>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c  | 18 ++++++++++++++++++
>>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h  |  8 ++++++++
>>   4 files changed, 57 insertions(+), 5 deletions(-)
>>
> 
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>> index 2c3415c8fc03..589e093b1608 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>> @@ -79,6 +79,12 @@ struct bnxt_pps {
>>          struct pps_pin pins[BNXT_MAX_TSIO_PINS];
>>   };
>>
>> +struct bnxt_ptp_stats {
>> +       u64             ts_pkts;
>> +       u64             ts_lost;
>> +       atomic64_t      ts_err;
>> +};
>> +
>>   struct bnxt_ptp_cfg {
>>          struct ptp_clock_info   ptp_info;
>>          struct ptp_clock        *ptp_clock;
>> @@ -125,6 +131,8 @@ struct bnxt_ptp_cfg {
>>          u32                     refclk_mapped_regs[2];
>>          u32                     txts_tmo;
>>          unsigned long           abs_txts_tmo;
>> +
>> +       struct bnxt_ptp_stats   *stats;
> 
> I think there is no need to allocate this small stats structure
> separately.  It can just be:
> 
> struct bnxt_ptp_stats    stats;
> 
> The struct bnxt_ptp_cfg will only be allocated if the device supports
> PTP.  So the stats can always be a part of struct bnxt_ptp_cfg.

Yeah, I was thinking about embedding the struct into bnxt_ptp_cfg.
Ok, I'll make v2 in 24hr then.

Thanks!

> Other than that, it looks good to me.  Thanks.
> 
>>   };
>>