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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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