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

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 29 Feb 2024 08:43:38 -0800
From: Michael Chan <michael.chan@...adcom.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: davem@...emloft.net, Pavan Chebbi <pavan.chebbi@...adcom.com>, 
	Jakub Kicinski <kuba@...nel.org>, 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 Thu, Feb 29, 2024 at 1:23 AM Vadim Fedorenko
<vadim.fedorenko@...ux.dev> wrote:
>
> 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.

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.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)