[PATCH net-next 0/2] bnxt_en: Support configurable PTP TX timeout

[PATCH net-next 0/2] bnxt_en: Support configurable PTP TX timeout

[Michael Chan]

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

The driver needs to call firmware to retrieve the PTP hardware TX
timestamp for PTP packets.  The PTP packet is timestamped by hardware
right before packet transmission on the wire.  So there is variable
delay on when the timestamp is available.  The current fixed timeout
value of 1 msec passed to the firmware may not be long enough on
busy networks.

Add a devlink driver runtime parameter to allow the user to configure
this timeout value.  Increase the default to 1 second and allow a
maximum of 5 seconds.

Pavan Chebbi (2):
  bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout
  bnxt_en: Retry for TX timestamp from FW until timeout specified

 Documentation/networking/devlink/bnxt.rst     |  7 ++++
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 42 +++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 20 +++++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  7 +++-
 4 files changed, 72 insertions(+), 4 deletions(-)

-- 
2.30.1


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

[PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Michael Chan]

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

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

Sometimes, the current 1ms value that driver waits for firmware
to obtain a tx timestamp for a PTP packet may not be sufficient.
User may want the driver to wait for a longer custom period before
timing out.

Introduce a new runtime driver param for devlink "ptp_tx_timeout".
Using this parameter the driver can wait for up to the specified
time, when it is querying for a TX timestamp from firmware.  By
default the value is set to 1s.

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>
---
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 42 +++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c |  1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  3 ++
 3 files changed, 46 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index ae4529c043f0..0df0baa9d18c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -652,6 +652,7 @@ static const struct devlink_ops bnxt_vf_dl_ops;
 enum bnxt_dl_param_id {
 	BNXT_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
 	BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK,
+	BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO,
 };
 
 static const struct bnxt_dl_nvm_param nvm_params[] = {
@@ -1077,6 +1078,42 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
 	return rc;
 }
 
+static int bnxt_dl_ptp_param_get(struct devlink *dl, u32 id,
+				 struct devlink_param_gset_ctx *ctx)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+
+	if (!bp->ptp_cfg)
+		return -EOPNOTSUPP;
+
+	ctx->val.vu32 = bp->ptp_cfg->txts_tmo;
+	return 0;
+}
+
+static int bnxt_dl_ptp_param_set(struct devlink *dl, u32 id,
+				 struct devlink_param_gset_ctx *ctx)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+
+	if (!bp->ptp_cfg)
+		return -EOPNOTSUPP;
+
+	bp->ptp_cfg->txts_tmo = ctx->val.vu32;
+	return 0;
+}
+
+static int bnxt_dl_ptp_param_validate(struct devlink *dl, u32 id,
+				      union devlink_param_value val,
+				      struct netlink_ext_ack *extack)
+{
+	if (val.vu32 > BNXT_PTP_MAX_TX_TMO) {
+		NL_SET_ERR_MSG_FMT_MOD(extack, "TX timeout value exceeds the maximum (%d ms)",
+				       BNXT_PTP_MAX_TX_TMO);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int bnxt_dl_nvm_param_get(struct devlink *dl, u32 id,
 				 struct devlink_param_gset_ctx *ctx)
 {
@@ -1180,6 +1217,11 @@ static const struct devlink_param bnxt_dl_params[] = {
 			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
 			     bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
 			     NULL),
+	DEVLINK_PARAM_DRIVER(BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO,
+			     "ptp_tx_timeout", DEVLINK_PARAM_TYPE_U32,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     bnxt_dl_ptp_param_get, bnxt_dl_ptp_param_set,
+			     bnxt_dl_ptp_param_validate),
 	/* keep REMOTE_DEV_RESET last, it is excluded based on caps */
 	DEVLINK_PARAM_GENERIC(ENABLE_REMOTE_DEV_RESET,
 			      BIT(DEVLINK_PARAM_CMODE_RUNTIME),
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index cc07660330f5..4b50b07b9771 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -965,6 +965,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
 		spin_unlock_bh(&ptp->ptp_lock);
 		ptp_schedule_worker(ptp->ptp_clock, 0);
 	}
+	ptp->txts_tmo = BNXT_PTP_DFLT_TX_TMO;
 	return 0;
 
 out:
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index fce8dc39a7d0..ee977620d33e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -22,6 +22,8 @@
 #define BNXT_LO_TIMER_MASK	0x0000ffffffffUL
 #define BNXT_HI_TIMER_MASK	0xffff00000000UL
 
+#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_TX_ENABLES	(PORT_TS_QUERY_REQ_ENABLES_PTP_SEQ_ID |	\
 				 PORT_TS_QUERY_REQ_ENABLES_TS_REQ_TIMEOUT | \
@@ -120,6 +122,7 @@ struct bnxt_ptp_cfg {
 
 	u32			refclk_regs[2];
 	u32			refclk_mapped_regs[2];
+	u32			txts_tmo;
 };
 
 #if BITS_PER_LONG == 32
-- 
2.30.1


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

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Vadim Fedorenko]

On 29/02/2024 07:02, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> Sometimes, the current 1ms value that driver waits for firmware
> to obtain a tx timestamp for a PTP packet may not be sufficient.
> User may want the driver to wait for a longer custom period before
> timing out.
> 
> Introduce a new runtime driver param for devlink "ptp_tx_timeout".
> Using this parameter the driver can wait for up to the specified
> time, when it is querying for a TX timestamp from firmware.  By
> default the value is set to 1s.
> 
> 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>
> ---
>   .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 42 +++++++++++++++++++
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c |  1 +
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  3 ++
>   3 files changed, 46 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index ae4529c043f0..0df0baa9d18c 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -652,6 +652,7 @@ static const struct devlink_ops bnxt_vf_dl_ops;
>   enum bnxt_dl_param_id {
>   	BNXT_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>   	BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK,
> +	BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO,
>   };
>   
>   static const struct bnxt_dl_nvm_param nvm_params[] = {
> @@ -1077,6 +1078,42 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
>   	return rc;
>   }
>   
> +static int bnxt_dl_ptp_param_get(struct devlink *dl, u32 id,
> +				 struct devlink_param_gset_ctx *ctx)
> +{
> +	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
> +
> +	if (!bp->ptp_cfg)
> +		return -EOPNOTSUPP;
> +
> +	ctx->val.vu32 = bp->ptp_cfg->txts_tmo;
> +	return 0;
> +}
> +
> +static int bnxt_dl_ptp_param_set(struct devlink *dl, u32 id,
> +				 struct devlink_param_gset_ctx *ctx)
> +{
> +	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
> +
> +	if (!bp->ptp_cfg)
> +		return -EOPNOTSUPP;
> +
> +	bp->ptp_cfg->txts_tmo = ctx->val.vu32;
> +	return 0;
> +}
> +
> +static int bnxt_dl_ptp_param_validate(struct devlink *dl, u32 id,
> +				      union devlink_param_value val,
> +				      struct netlink_ext_ack *extack)
> +{
> +	if (val.vu32 > BNXT_PTP_MAX_TX_TMO) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "TX timeout value exceeds the maximum (%d ms)",
> +				       BNXT_PTP_MAX_TX_TMO);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>   static int bnxt_dl_nvm_param_get(struct devlink *dl, u32 id,
>   				 struct devlink_param_gset_ctx *ctx)
>   {
> @@ -1180,6 +1217,11 @@ static const struct devlink_param bnxt_dl_params[] = {
>   			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
>   			     bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
>   			     NULL),
> +	DEVLINK_PARAM_DRIVER(BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO,
> +			     "ptp_tx_timeout", DEVLINK_PARAM_TYPE_U32,
> +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> +			     bnxt_dl_ptp_param_get, bnxt_dl_ptp_param_set,
> +			     bnxt_dl_ptp_param_validate),
>   	/* keep REMOTE_DEV_RESET last, it is excluded based on caps */
>   	DEVLINK_PARAM_GENERIC(ENABLE_REMOTE_DEV_RESET,
>   			      BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> index cc07660330f5..4b50b07b9771 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> @@ -965,6 +965,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
>   		spin_unlock_bh(&ptp->ptp_lock);
>   		ptp_schedule_worker(ptp->ptp_clock, 0);
>   	}
> +	ptp->txts_tmo = BNXT_PTP_DFLT_TX_TMO;
>   	return 0;
>   
>   out:
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> index fce8dc39a7d0..ee977620d33e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> @@ -22,6 +22,8 @@
>   #define BNXT_LO_TIMER_MASK	0x0000ffffffffUL
>   #define BNXT_HI_TIMER_MASK	0xffff00000000UL
>   
> +#define BNXT_PTP_DFLT_TX_TMO	1000 /* ms */

I'm not happy with such huge timeout, but looks like other vendors do
expect the same timeouts, I'm ok.

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

> +#define BNXT_PTP_MAX_TX_TMO	5000 /* ms */
>   #define BNXT_PTP_QTS_TIMEOUT	1000
>   #define BNXT_PTP_QTS_TX_ENABLES	(PORT_TS_QUERY_REQ_ENABLES_PTP_SEQ_ID |	\
>   				 PORT_TS_QUERY_REQ_ENABLES_TS_REQ_TIMEOUT | \
> @@ -120,6 +122,7 @@ struct bnxt_ptp_cfg {
>   
>   	u32			refclk_regs[2];
>   	u32			refclk_mapped_regs[2];
> +	u32			txts_tmo;
>   };
>   
>   #if BITS_PER_LONG == 32

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Jiri Pirko]

Thu, Feb 29, 2024 at 08:02:01AM CET, michael.chan@broadcom.com wrote:
>From: Pavan Chebbi <pavan.chebbi@broadcom.com>
>
>Sometimes, the current 1ms value that driver waits for firmware
>to obtain a tx timestamp for a PTP packet may not be sufficient.
>User may want the driver to wait for a longer custom period before
>timing out.
>
>Introduce a new runtime driver param for devlink "ptp_tx_timeout".
>Using this parameter the driver can wait for up to the specified
>time, when it is querying for a TX timestamp from firmware.  By
>default the value is set to 1s.
>
>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>
>---
> .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 42 +++++++++++++++++++
> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c |  1 +
> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  3 ++
> 3 files changed, 46 insertions(+)
>
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>index ae4529c043f0..0df0baa9d18c 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>@@ -652,6 +652,7 @@ static const struct devlink_ops bnxt_vf_dl_ops;
> enum bnxt_dl_param_id {
> 	BNXT_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
> 	BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK,
>+	BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO,
> };
> 
> static const struct bnxt_dl_nvm_param nvm_params[] = {
>@@ -1077,6 +1078,42 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
> 	return rc;
> }
> 
>+static int bnxt_dl_ptp_param_get(struct devlink *dl, u32 id,
>+				 struct devlink_param_gset_ctx *ctx)
>+{
>+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
>+
>+	if (!bp->ptp_cfg)
>+		return -EOPNOTSUPP;
>+
>+	ctx->val.vu32 = bp->ptp_cfg->txts_tmo;
>+	return 0;
>+}
>+
>+static int bnxt_dl_ptp_param_set(struct devlink *dl, u32 id,
>+				 struct devlink_param_gset_ctx *ctx)
>+{
>+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
>+
>+	if (!bp->ptp_cfg)
>+		return -EOPNOTSUPP;
>+
>+	bp->ptp_cfg->txts_tmo = ctx->val.vu32;
>+	return 0;
>+}
>+
>+static int bnxt_dl_ptp_param_validate(struct devlink *dl, u32 id,
>+				      union devlink_param_value val,
>+				      struct netlink_ext_ack *extack)
>+{
>+	if (val.vu32 > BNXT_PTP_MAX_TX_TMO) {
>+		NL_SET_ERR_MSG_FMT_MOD(extack, "TX timeout value exceeds the maximum (%d ms)",
>+				       BNXT_PTP_MAX_TX_TMO);
>+		return -EINVAL;
>+	}
>+	return 0;
>+}
>+
> static int bnxt_dl_nvm_param_get(struct devlink *dl, u32 id,
> 				 struct devlink_param_gset_ctx *ctx)
> {
>@@ -1180,6 +1217,11 @@ static const struct devlink_param bnxt_dl_params[] = {
> 			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
> 			     bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
> 			     NULL),
>+	DEVLINK_PARAM_DRIVER(BNXT_DEVLINK_PARAM_ID_PTP_TXTS_TMO,
>+			     "ptp_tx_timeout", DEVLINK_PARAM_TYPE_U32,
>+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
>+			     bnxt_dl_ptp_param_get, bnxt_dl_ptp_param_set,
>+			     bnxt_dl_ptp_param_validate),

Idk. This does not look sane to me at all. Will we have custom knobs to
change timeout for arbitrary FW commands as this as a common thing?
Driver is the one to take care of timeouts of FW gracefully, he should
know the FW, not the user. Therefore exposing user knobs like this
sounds pure wrong to me.

nack for adding this to devlink.

If this is some maybe-to-be-common ptp thing, can that be done as part
of ptp api perhaps?

pw-bot: cr


> 	/* keep REMOTE_DEV_RESET last, it is excluded based on caps */
> 	DEVLINK_PARAM_GENERIC(ENABLE_REMOTE_DEV_RESET,
> 			      BIT(DEVLINK_PARAM_CMODE_RUNTIME),
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>index cc07660330f5..4b50b07b9771 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>@@ -965,6 +965,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
> 		spin_unlock_bh(&ptp->ptp_lock);
> 		ptp_schedule_worker(ptp->ptp_clock, 0);
> 	}
>+	ptp->txts_tmo = BNXT_PTP_DFLT_TX_TMO;
> 	return 0;
> 
> out:
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>index fce8dc39a7d0..ee977620d33e 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>@@ -22,6 +22,8 @@
> #define BNXT_LO_TIMER_MASK	0x0000ffffffffUL
> #define BNXT_HI_TIMER_MASK	0xffff00000000UL
> 
>+#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_TX_ENABLES	(PORT_TS_QUERY_REQ_ENABLES_PTP_SEQ_ID |	\
> 				 PORT_TS_QUERY_REQ_ENABLES_TS_REQ_TIMEOUT | \
>@@ -120,6 +122,7 @@ struct bnxt_ptp_cfg {
> 
> 	u32			refclk_regs[2];
> 	u32			refclk_mapped_regs[2];
>+	u32			txts_tmo;
> };
> 
> #if BITS_PER_LONG == 32
>-- 
>2.30.1
>

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Jakub Kicinski]

On Thu, 29 Feb 2024 18:11:49 +0100 Jiri Pirko wrote:
> Idk. This does not look sane to me at all. Will we have custom knobs to
> change timeout for arbitrary FW commands as this as a common thing?
> Driver is the one to take care of timeouts of FW gracefully, he should
> know the FW, not the user. Therefore exposing user knobs like this
> sounds pure wrong to me.
> 
> nack for adding this to devlink.

+1

BTW why is the documentation in a different patch that the param :(

> If this is some maybe-to-be-common ptp thing, can that be done as part
> of ptp api perhaps?

Perhaps, but also I think it's fairly impractical. Specialized users may
be able to tune this, but in DC environment PTP is handled at the host
level, and the applications come and go. So all the poor admin can do
is set this to the max value. While in the driver you can actually try
to be a bit more intelligent. Expecting the user to tune this strikes me
as trying to take the easy way out..

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Vadim Fedorenko]

On 29/02/2024 17:30, Jakub Kicinski wrote:
> On Thu, 29 Feb 2024 18:11:49 +0100 Jiri Pirko wrote:
>> Idk. This does not look sane to me at all. Will we have custom knobs to
>> change timeout for arbitrary FW commands as this as a common thing?
>> Driver is the one to take care of timeouts of FW gracefully, he should
>> know the FW, not the user. Therefore exposing user knobs like this
>> sounds pure wrong to me.
>>
>> nack for adding this to devlink.
> 
> +1
> 
> BTW why is the documentation in a different patch that the param :(
> 
>> If this is some maybe-to-be-common ptp thing, can that be done as part
>> of ptp api perhaps?

Jiri, do you mean extend current ioctl used to enable/disable HW
timestamps?

> 
> Perhaps, but also I think it's fairly impractical. Specialized users may
> be able to tune this, but in DC environment PTP is handled at the host

That's correct, only 1 app is actually doing syncronization

> level, and the applications come and go. So all the poor admin can do

Container/VM level applications don't care about PTP packets timestamps.
They only care about the time being synchronized.

> is set this to the max value. While in the driver you can actually try

Pure admin will tune it according to the host level app configuration
which may differ because of environment.

> to be a bit more intelligent. Expecting the user to tune this strikes me
> as trying to take the easy way out..

There is no actual way for application to signal down to driver that it
gave up waiting for TX timestamp, what other kind of smartness can we
expect here?

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Jakub Kicinski]

On Thu, 29 Feb 2024 21:22:19 +0000 Vadim Fedorenko wrote:
> > Perhaps, but also I think it's fairly impractical. Specialized users may
> > be able to tune this, but in DC environment PTP is handled at the host  
> 
> That's correct, only 1 app is actually doing syncronization
> 
> > level, and the applications come and go. So all the poor admin can do  
> 
> Container/VM level applications don't care about PTP packets timestamps.
> They only care about the time being synchronized.

What I was saying is that in the PTP daemon you don't know whether
the app running is likely to cause delays or not, and how long.

> > is set this to the max value. While in the driver you can actually try  
> 
> Pure admin will tune it according to the host level app configuration
> which may differ because of environment.

Concrete example?

> > to be a bit more intelligent. Expecting the user to tune this strikes me
> > as trying to take the easy way out..  
> 
> There is no actual way for application to signal down to driver that it
> gave up waiting for TX timestamp, what other kind of smartness can we
> expect here?

Let's figure out why the timeouts happen, before we create uAPIs.
If it's because there's buffer bloat or a pause storm, the next TS
request that gets queued will get stuck in the same exact way.

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Pavan Chebbi]

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

On Fri, Mar 1, 2024 at 7:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 29 Feb 2024 21:22:19 +0000 Vadim Fedorenko wrote:
> > > Perhaps, but also I think it's fairly impractical. Specialized users may
> > > be able to tune this, but in DC environment PTP is handled at the host
> >
> > That's correct, only 1 app is actually doing syncronization
> >
> > > level, and the applications come and go. So all the poor admin can do
> >
> > Container/VM level applications don't care about PTP packets timestamps.
> > They only care about the time being synchronized.
>
> What I was saying is that in the PTP daemon you don't know whether
> the app running is likely to cause delays or not, and how long.
>

As such timeouts are rare but still normal. But if you've an
environment where you want to have some kind of sync between the
application and the NIC, as to when should both conclude that the
timestamp is absolutely lost, we need some knob like this. Like you
pointed out it's for an informed user who has knowledge of the
workloads/flow control and how (badly) could they affect the TX. Of
course the user cannot make an accurate estimation of the exact time
out, but he can always experiment with the knob.
We are not sure if others need this as well, hence the private devlink
parameter. For most common users, the default 1s timeout should serve
well.

> > > is set this to the max value. While in the driver you can actually try
> >
> > Pure admin will tune it according to the host level app configuration
> > which may differ because of environment.
>
> Concrete example?
>
> > > to be a bit more intelligent. Expecting the user to tune this strikes me
> > > as trying to take the easy way out..
> >
> > There is no actual way for application to signal down to driver that it
> > gave up waiting for TX timestamp, what other kind of smartness can we
> > expect here?
>
> Let's figure out why the timeouts happen, before we create uAPIs.
> If it's because there's buffer bloat or a pause storm, the next TS
> request that gets queued will get stuck in the same exact way.

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

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Jakub Kicinski]

On Fri, 1 Mar 2024 13:09:30 +0530 Pavan Chebbi wrote:
> > What I was saying is that in the PTP daemon you don't know whether
> > the app running is likely to cause delays or not, and how long.
> 
> As such timeouts are rare but still normal.

Normal, because...? Why do they happen?

> But if you've an environment where you want to have some kind of sync
> between the application and the NIC, as to when should both conclude
> that the timestamp is absolutely lost, we need some knob like this.
> Like you pointed out it's for an informed user who has knowledge of
> the

Let's start from informing the user why timeout happens.

> workloads/flow control and how (badly) could they affect the TX. Of
> course the user cannot make an accurate estimation of the exact time
> out, but he can always experiment with the knob.
> We are not sure if others need this as well, hence the private devlink
> parameter. For most common users, the default 1s timeout should serve
> well.

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Pavan Chebbi]

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

On Fri, Mar 1, 2024 at 10:48 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 1 Mar 2024 13:09:30 +0530 Pavan Chebbi wrote:
> > > What I was saying is that in the PTP daemon you don't know whether
> > > the app running is likely to cause delays or not, and how long.
> >
> > As such timeouts are rare but still normal.
>
> Normal, because...? Why do they happen?

Excuse me for the late reply.
In my experience so far, it's primarily because of flow control and
how stressed the underlying HW queue is. (I am sure it's not unique to
our hardware alone)
Hence we wanted to accommodate cases where the expected wait time is
higher than what is default in the driver, for the packets to go out.
But it's disappointing to know that even private devlink params are
discouraged for such purposes.
I'd think that non-generic driver params in devlink serve exactly such
requirements and having such a knob would be useful for an advanced
user.
Not to mention, in my view, such additions to devlink would make it
more popular and would help in its wider adoption.
For now, we will drop this series and try to get back with a different solution.

>
> > But if you've an environment where you want to have some kind of sync
> > between the application and the NIC, as to when should both conclude
> > that the timestamp is absolutely lost, we need some knob like this.
> > Like you pointed out it's for an informed user who has knowledge of
> > the
>
> Let's start from informing the user why timeout happens.
>
> > workloads/flow control and how (badly) could they affect the TX. Of
> > course the user cannot make an accurate estimation of the exact time
> > out, but he can always experiment with the knob.
> > We are not sure if others need this as well, hence the private devlink
> > parameter. For most common users, the default 1s timeout should serve
> > well.

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

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Jakub Kicinski]

On Thu, 7 Mar 2024 09:20:44 +0530 Pavan Chebbi wrote:
> > > As such timeouts are rare but still normal.  
> >
> > Normal, because...? Why do they happen?  
> 
> Excuse me for the late reply.
> In my experience so far, it's primarily because of flow control and
> how stressed the underlying HW queue is. (I am sure it's not unique to
> our hardware alone)
> Hence we wanted to accommodate cases where the expected wait time is
> higher than what is default in the driver, for the packets to go out.
> But it's disappointing to know that even private devlink params are
> discouraged for such purposes.
> I'd think that non-generic driver params in devlink serve exactly such
> requirements and having such a knob would be useful for an advanced
> user.
> Not to mention, in my view, such additions to devlink would make it
> more popular and would help in its wider adoption.

The problem can be solved more intelligently.

Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver param to set ptp tx timeout

[Jiri Pirko]

Thu, Feb 29, 2024 at 10:22:19PM CET, vadim.fedorenko@linux.dev wrote:
>On 29/02/2024 17:30, Jakub Kicinski wrote:
>> On Thu, 29 Feb 2024 18:11:49 +0100 Jiri Pirko wrote:
>> > Idk. This does not look sane to me at all. Will we have custom knobs to
>> > change timeout for arbitrary FW commands as this as a common thing?
>> > Driver is the one to take care of timeouts of FW gracefully, he should
>> > know the FW, not the user. Therefore exposing user knobs like this
>> > sounds pure wrong to me.
>> > 
>> > nack for adding this to devlink.
>> 
>> +1
>> 
>> BTW why is the documentation in a different patch that the param :(
>> 
>> > If this is some maybe-to-be-common ptp thing, can that be done as part
>> > of ptp api perhaps?
>
>Jiri, do you mean extend current ioctl used to enable/disable HW
>timestamps?

Maybe.

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

[Michael Chan]

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

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);
+		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
-- 
2.30.1


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

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

[Vadim Fedorenko]

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>

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

[Michael Chan]

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

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.

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