diff mbox series

[net-next,2/2] bnxt_en: Retry for TX timestamp from FW until timeout specified

Message ID 20240229070202.107488-3-michael.chan@broadcom.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: Support configurable PTP TX timeout | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 940 this patch: 940
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 2 maintainers not CCed: linux-doc@vger.kernel.org corbet@lwn.net
netdev/build_clang success Errors and warnings before: 957 this patch: 957
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 960 this patch: 960
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 85 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Michael Chan Feb. 29, 2024, 7:02 a.m. UTC 
From: Pavan Chebbi <pavan.chebbi@broadcom.com>

Use the ptp_tx_timeout devlink parameter introduced in the previous
patch to retry querying TX timestamp, up to the timeout specified.
Firmware supports timeout values up to 65535 microseconds.  The
driver will set this firmware timeout value according to the
ptp_tx_timeout parameter.  If the ptp_tx_timeout value exceeds
the maximum firmware value, the driver will retry in the context
of bnxt_ptp_ts_aux_work().

Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/devlink/bnxt.rst     |  7 +++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 19 ++++++++++++++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  4 +++-
 3 files changed, 26 insertions(+), 4 deletions(-)

Comments

Vadim Fedorenko Feb. 29, 2024, 9:23 a.m. UTC | #1 
On 29/02/2024 07:02, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> Use the ptp_tx_timeout devlink parameter introduced in the previous
> patch to retry querying TX timestamp, up to the timeout specified.
> Firmware supports timeout values up to 65535 microseconds.  The
> driver will set this firmware timeout value according to the
> ptp_tx_timeout parameter.  If the ptp_tx_timeout value exceeds
> the maximum firmware value, the driver will retry in the context
> of bnxt_ptp_ts_aux_work().
> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>   Documentation/networking/devlink/bnxt.rst     |  7 +++++++
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 19 ++++++++++++++++---
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  4 +++-
>   3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/networking/devlink/bnxt.rst b/Documentation/networking/devlink/bnxt.rst
> index a4fb27663cd6..48833c190c5b 100644
> --- a/Documentation/networking/devlink/bnxt.rst
> +++ b/Documentation/networking/devlink/bnxt.rst
> @@ -41,6 +41,13 @@ parameters.
>        - Generic Routing Encapsulation (GRE) version check will be enabled in
>          the device. If disabled, the device will skip the version check for
>          incoming packets.
> +   * - ``ptp_tx_timeout``
> +     - u32
> +     - Runtime
> +     - PTP Transmit timestamp timeout value in milliseconds. The default
> +       value is 1000 and the maximum value is 5000. Use a higher value
> +       on a busy network to prevent timeout retrieving the PTP Transmit
> +       timestamp.
>   
>   Info versions
>   =============
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> index 4b50b07b9771..a05b50162e9e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> @@ -122,10 +122,14 @@ static int bnxt_hwrm_port_ts_query(struct bnxt *bp, u32 flags, u64 *ts)
>   	req->flags = cpu_to_le32(flags);
>   	if ((flags & PORT_TS_QUERY_REQ_FLAGS_PATH) ==
>   	    PORT_TS_QUERY_REQ_FLAGS_PATH_TX) {
> +		struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> +		u32 tmo_us = ptp->txts_tmo * 1000;
> +
>   		req->enables = cpu_to_le16(BNXT_PTP_QTS_TX_ENABLES);
> -		req->ptp_seq_id = cpu_to_le32(bp->ptp_cfg->tx_seqid);
> -		req->ptp_hdr_offset = cpu_to_le16(bp->ptp_cfg->tx_hdr_off);
> -		req->ts_req_timeout = cpu_to_le16(BNXT_PTP_QTS_TIMEOUT);
> +		req->ptp_seq_id = cpu_to_le32(ptp->tx_seqid);
> +		req->ptp_hdr_offset = cpu_to_le16(ptp->tx_hdr_off);
> +		tmo_us = min(tmo_us, BNXT_PTP_QTS_MAX_TMO_US);

With this logic the request will stay longer than expected. With
BNXT_PTP_QTS_MAX_TMO_US hardcoded to 65ms (it's later in the patch),
and TXT timestamp timeout set for 270ms, the request will wait for 325ms
in total. It doesn't look like a blocker, but it's definitely area to
improve given that only one TX timestamp request can be in-flight.

> +		req->ts_req_timeout = cpu_to_le16(tmo_us);
>   	}
>   	resp = hwrm_req_hold(bp, req);
>   
> @@ -675,6 +679,8 @@ static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
>   	u64 ts = 0, ns = 0;
>   	int rc;
>   
> +	if (!ptp->txts_pending)
> +		ptp->abs_txts_tmo = jiffies + msecs_to_jiffies(ptp->txts_tmo);
>   	rc = bnxt_hwrm_port_ts_query(bp, PORT_TS_QUERY_REQ_FLAGS_PATH_TX, &ts);
>   	if (!rc) {
>   		memset(&timestamp, 0, sizeof(timestamp));
> @@ -684,6 +690,10 @@ static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
>   		timestamp.hwtstamp = ns_to_ktime(ns);
>   		skb_tstamp_tx(ptp->tx_skb, &timestamp);
>   	} else {
> +		if (!time_after_eq(jiffies, ptp->abs_txts_tmo)) {
> +			ptp->txts_pending = true;
> +			return;
> +		}
>   		netdev_warn_once(bp->dev,
>   				 "TS query for TX timer failed rc = %x\n", rc);
>   	}
> @@ -691,6 +701,7 @@ static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
>   	dev_kfree_skb_any(ptp->tx_skb);
>   	ptp->tx_skb = NULL;
>   	atomic_inc(&ptp->tx_avail);
> +	ptp->txts_pending = false;
>   }
>   
>   static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
> @@ -714,6 +725,8 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
>   		spin_unlock_bh(&ptp->ptp_lock);
>   		ptp->next_overflow_check = now + BNXT_PHC_OVERFLOW_PERIOD;
>   	}
> +	if (ptp->txts_pending)
> +		return 0;
>   	return HZ;
>   }
>   
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> index ee977620d33e..bfb165d2b365 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> @@ -24,7 +24,7 @@
>   
>   #define BNXT_PTP_DFLT_TX_TMO	1000 /* ms */
>   #define BNXT_PTP_MAX_TX_TMO	5000 /* ms */
> -#define BNXT_PTP_QTS_TIMEOUT	1000
> +#define BNXT_PTP_QTS_MAX_TMO_US	65535
             ^^^^
This is the request timeout definition

>   #define BNXT_PTP_QTS_TX_ENABLES	(PORT_TS_QUERY_REQ_ENABLES_PTP_SEQ_ID |	\
>   				 PORT_TS_QUERY_REQ_ENABLES_TS_REQ_TIMEOUT | \
>   				 PORT_TS_QUERY_REQ_ENABLES_PTP_HDR_OFFSET)
> @@ -117,12 +117,14 @@ struct bnxt_ptp_cfg {
>   					 BNXT_PTP_MSG_PDELAY_REQ |	\
>   					 BNXT_PTP_MSG_PDELAY_RESP)
>   	u8			tx_tstamp_en:1;
> +	u8			txts_pending:1;
>   	int			rx_filter;
>   	u32			tstamp_filters;
>   
>   	u32			refclk_regs[2];
>   	u32			refclk_mapped_regs[2];
>   	u32			txts_tmo;
> +	unsigned long		abs_txts_tmo;
>   };
>   
>   #if BITS_PER_LONG == 32

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Michael Chan Feb. 29, 2024, 4:43 p.m. UTC | #2 
On Thu, Feb 29, 2024 at 1:23 AM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 29/02/2024 07:02, Michael Chan wrote:
> > From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> >
> > Use the ptp_tx_timeout devlink parameter introduced in the previous
> > patch to retry querying TX timestamp, up to the timeout specified.
> > Firmware supports timeout values up to 65535 microseconds.  The
> > driver will set this firmware timeout value according to the
> > ptp_tx_timeout parameter.  If the ptp_tx_timeout value exceeds
> > the maximum firmware value, the driver will retry in the context
> > of bnxt_ptp_ts_aux_work().
> >
> > Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> > Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > ---
> >   Documentation/networking/devlink/bnxt.rst     |  7 +++++++
> >   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 19 ++++++++++++++++---
> >   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  4 +++-
> >   3 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/networking/devlink/bnxt.rst b/Documentation/networking/devlink/bnxt.rst
> > index a4fb27663cd6..48833c190c5b 100644
> > --- a/Documentation/networking/devlink/bnxt.rst
> > +++ b/Documentation/networking/devlink/bnxt.rst
> > @@ -41,6 +41,13 @@ parameters.
> >        - Generic Routing Encapsulation (GRE) version check will be enabled in
> >          the device. If disabled, the device will skip the version check for
> >          incoming packets.
> > +   * - ``ptp_tx_timeout``
> > +     - u32
> > +     - Runtime
> > +     - PTP Transmit timestamp timeout value in milliseconds. The default
> > +       value is 1000 and the maximum value is 5000. Use a higher value
> > +       on a busy network to prevent timeout retrieving the PTP Transmit
> > +       timestamp.
> >
> >   Info versions
> >   =============
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> > index 4b50b07b9771..a05b50162e9e 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> > @@ -122,10 +122,14 @@ static int bnxt_hwrm_port_ts_query(struct bnxt *bp, u32 flags, u64 *ts)
> >       req->flags = cpu_to_le32(flags);
> >       if ((flags & PORT_TS_QUERY_REQ_FLAGS_PATH) ==
> >           PORT_TS_QUERY_REQ_FLAGS_PATH_TX) {
> > +             struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> > +             u32 tmo_us = ptp->txts_tmo * 1000;
> > +
> >               req->enables = cpu_to_le16(BNXT_PTP_QTS_TX_ENABLES);
> > -             req->ptp_seq_id = cpu_to_le32(bp->ptp_cfg->tx_seqid);
> > -             req->ptp_hdr_offset = cpu_to_le16(bp->ptp_cfg->tx_hdr_off);
> > -             req->ts_req_timeout = cpu_to_le16(BNXT_PTP_QTS_TIMEOUT);
> > +             req->ptp_seq_id = cpu_to_le32(ptp->tx_seqid);
> > +             req->ptp_hdr_offset = cpu_to_le16(ptp->tx_hdr_off);
> > +             tmo_us = min(tmo_us, BNXT_PTP_QTS_MAX_TMO_US);
>
> With this logic the request will stay longer than expected. With
> BNXT_PTP_QTS_MAX_TMO_US hardcoded to 65ms (it's later in the patch),
> and TXT timestamp timeout set for 270ms, the request will wait for 325ms
> in total. It doesn't look like a blocker, but it's definitely area to
> improve given that only one TX timestamp request can be in-flight.

Note that the firmware will return the timestamp as soon as it is
available or wait up to the timeout value.  Yes, this firmware timeout
value will be set to 65ms if the devlink parameter is > 65 ms.  And
yes, the worst case wait time can potentially be +65 msec in this
case.

We can set this FW timeout value to something smaller than 65 ms
(maybe something like 25 ms) when the devlink parameter is > 65 ms.
Thanks.
diff mbox series

Patch

diff --git a/Documentation/networking/devlink/bnxt.rst b/Documentation/networking/devlink/bnxt.rst
index a4fb27663cd6..48833c190c5b 100644
--- a/Documentation/networking/devlink/bnxt.rst
+++ b/Documentation/networking/devlink/bnxt.rst
@@ -41,6 +41,13 @@  parameters.
      - Generic Routing Encapsulation (GRE) version check will be enabled in
        the device. If disabled, the device will skip the version check for
        incoming packets.
+   * - ``ptp_tx_timeout``
+     - u32
+     - Runtime
+     - PTP Transmit timestamp timeout value in milliseconds. The default
+       value is 1000 and the maximum value is 5000. Use a higher value
+       on a busy network to prevent timeout retrieving the PTP Transmit
+       timestamp.
 
 Info versions
 =============
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index 4b50b07b9771..a05b50162e9e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -122,10 +122,14 @@  static int bnxt_hwrm_port_ts_query(struct bnxt *bp, u32 flags, u64 *ts)
 	req->flags = cpu_to_le32(flags);
 	if ((flags & PORT_TS_QUERY_REQ_FLAGS_PATH) ==
 	    PORT_TS_QUERY_REQ_FLAGS_PATH_TX) {
+		struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
+		u32 tmo_us = ptp->txts_tmo * 1000;
+
 		req->enables = cpu_to_le16(BNXT_PTP_QTS_TX_ENABLES);
-		req->ptp_seq_id = cpu_to_le32(bp->ptp_cfg->tx_seqid);
-		req->ptp_hdr_offset = cpu_to_le16(bp->ptp_cfg->tx_hdr_off);
-		req->ts_req_timeout = cpu_to_le16(BNXT_PTP_QTS_TIMEOUT);
+		req->ptp_seq_id = cpu_to_le32(ptp->tx_seqid);
+		req->ptp_hdr_offset = cpu_to_le16(ptp->tx_hdr_off);
+		tmo_us = min(tmo_us, BNXT_PTP_QTS_MAX_TMO_US);
+		req->ts_req_timeout = cpu_to_le16(tmo_us);
 	}
 	resp = hwrm_req_hold(bp, req);
 
@@ -675,6 +679,8 @@  static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 	u64 ts = 0, ns = 0;
 	int rc;
 
+	if (!ptp->txts_pending)
+		ptp->abs_txts_tmo = jiffies + msecs_to_jiffies(ptp->txts_tmo);
 	rc = bnxt_hwrm_port_ts_query(bp, PORT_TS_QUERY_REQ_FLAGS_PATH_TX, &ts);
 	if (!rc) {
 		memset(&timestamp, 0, sizeof(timestamp));
@@ -684,6 +690,10 @@  static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 		timestamp.hwtstamp = ns_to_ktime(ns);
 		skb_tstamp_tx(ptp->tx_skb, &timestamp);
 	} else {
+		if (!time_after_eq(jiffies, ptp->abs_txts_tmo)) {
+			ptp->txts_pending = true;
+			return;
+		}
 		netdev_warn_once(bp->dev,
 				 "TS query for TX timer failed rc = %x\n", rc);
 	}
@@ -691,6 +701,7 @@  static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 	dev_kfree_skb_any(ptp->tx_skb);
 	ptp->tx_skb = NULL;
 	atomic_inc(&ptp->tx_avail);
+	ptp->txts_pending = false;
 }
 
 static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
@@ -714,6 +725,8 @@  static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 		spin_unlock_bh(&ptp->ptp_lock);
 		ptp->next_overflow_check = now + BNXT_PHC_OVERFLOW_PERIOD;
 	}
+	if (ptp->txts_pending)
+		return 0;
 	return HZ;
 }
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index ee977620d33e..bfb165d2b365 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -24,7 +24,7 @@ 
 
 #define BNXT_PTP_DFLT_TX_TMO	1000 /* ms */
 #define BNXT_PTP_MAX_TX_TMO	5000 /* ms */
-#define BNXT_PTP_QTS_TIMEOUT	1000
+#define BNXT_PTP_QTS_MAX_TMO_US	65535
 #define BNXT_PTP_QTS_TX_ENABLES	(PORT_TS_QUERY_REQ_ENABLES_PTP_SEQ_ID |	\
 				 PORT_TS_QUERY_REQ_ENABLES_TS_REQ_TIMEOUT | \
 				 PORT_TS_QUERY_REQ_ENABLES_PTP_HDR_OFFSET)
@@ -117,12 +117,14 @@  struct bnxt_ptp_cfg {
 					 BNXT_PTP_MSG_PDELAY_REQ |	\
 					 BNXT_PTP_MSG_PDELAY_RESP)
 	u8			tx_tstamp_en:1;
+	u8			txts_pending:1;
 	int			rx_filter;
 	u32			tstamp_filters;
 
 	u32			refclk_regs[2];
 	u32			refclk_mapped_regs[2];
 	u32			txts_tmo;
+	unsigned long		abs_txts_tmo;
 };
 
 #if BITS_PER_LONG == 32