| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3c38627e-b9b6-43a3-ae66-628976c41247@linux.dev>
Date: Thu, 29 Feb 2024 09:23:08 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Michael Chan <michael.chan@...adcom.com>, davem@...emloft.net,
Pavan Chebbi <pavan.chebbi@...adcom.com>, Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
andrew.gospodarek@...adcom.com, jiri@...nulli.us, richardcochran@...il.com
Subject: Re: [PATCH net-next 2/2] bnxt_en: Retry for TX timestamp from FW
until timeout specified
On 29/02/2024 07:02, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@...adcom.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@...adcom.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@...adcom.com>
> Signed-off-by: Michael Chan <michael.chan@...adcom.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(×tamp, 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, ×tamp);
> } 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@...ux.dev>
Powered by blists - more mailing lists