| 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: <ZeC61UannrX8sWDk@nanopsycho>
Date: Thu, 29 Feb 2024 18:11:49 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Michael Chan <michael.chan@...adcom.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, pavan.chebbi@...adcom.com,
andrew.gospodarek@...adcom.com, richardcochran@...il.com
Subject: Re: [PATCH net-next 1/2] bnxt_en: Introduce devlink runtime driver
param to set ptp tx timeout
Thu, Feb 29, 2024 at 08:02:01AM CET, michael.chan@...adcom.com wrote:
>From: Pavan Chebbi <pavan.chebbi@...adcom.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@...adcom.com>
>Signed-off-by: Pavan Chebbi <pavan.chebbi@...adcom.com>
>Signed-off-by: Michael Chan <michael.chan@...adcom.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
>
Powered by blists - more mailing lists