| 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: <c3d549db-bc7d-4b89-bc30-7fd4ec6f20e1@intel.com>
Date: Thu, 28 Aug 2025 18:28:22 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Raju Rangoju <Raju.Rangoju@....com>
CC: <netdev@...r.kernel.org>, <andrew+netdev@...n.ch>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<richardcochran@...il.com>, <linux-kernel@...r.kernel.org>,
<Shyam-sundar.S-k@....com>
Subject: Re: [PATCH net-next v3] amd-xgbe: Add PPS periodic output support
From: Raju Rangoju <Raju.Rangoju@....com>
Date: Thu, 28 Aug 2025 14:59:00 +0530
> Add support for hardware PPS (Pulse Per Second) output to the
> AMD XGBE driver. The implementation enables flexible periodic
> output mode, exposing it via the PTP per_out interface.
>
> The driver supports configuring PPS output using the standard
> PTP subsystem, allowing precise periodic signal generation for
> time synchronization applications.
>
> The feature has been verified using the testptp tool and
> oscilloscope.
>
> Signed-off-by: Raju Rangoju <Raju.Rangoju@....com>
> ---
> Changes since v2:
> - avoid redundant checks in xgbe_enable()
> - simplify the mask calculation
>
> Changes since v1:
> - add sanity check to prevent pps_out_num and aux_snap_num exceeding the limit
>
> drivers/net/ethernet/amd/xgbe/Makefile | 2 +-
> drivers/net/ethernet/amd/xgbe/xgbe-common.h | 46 ++++++++++++-
> drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 15 +++++
> drivers/net/ethernet/amd/xgbe/xgbe-pps.c | 73 +++++++++++++++++++++
> drivers/net/ethernet/amd/xgbe/xgbe-ptp.c | 26 +++++++-
> drivers/net/ethernet/amd/xgbe/xgbe.h | 16 +++++
> 6 files changed, 173 insertions(+), 5 deletions(-)
> create mode 100644 drivers/net/ethernet/amd/xgbe/xgbe-pps.c
[...]
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> index 2e9b95a94f89..f0989aa01855 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> @@ -691,6 +691,21 @@ void xgbe_get_all_hw_features(struct xgbe_prv_data *pdata)
> hw_feat->pps_out_num = XGMAC_GET_BITS(mac_hfr2, MAC_HWF2R, PPSOUTNUM);
> hw_feat->aux_snap_num = XGMAC_GET_BITS(mac_hfr2, MAC_HWF2R, AUXSNAPNUM);
>
> + /* Sanity check and warn if hardware reports more than supported */
> + if (hw_feat->pps_out_num > XGBE_MAX_PPS_OUT) {
> + dev_warn(pdata->dev,
1. How often can this function be called? Don't you need the _ratelimit
version here?
2. netdev_ variant instead of dev_?
> + "Hardware reports %u PPS outputs, limiting to %u\n",
> + hw_feat->pps_out_num, XGBE_MAX_PPS_OUT);
> + hw_feat->pps_out_num = XGBE_MAX_PPS_OUT;
> + }
> +
> + if (hw_feat->aux_snap_num > XGBE_MAX_AUX_SNAP) {
> + dev_warn(pdata->dev,
(same)
> + "Hardware reports %u aux snapshot inputs, limiting to %u\n",
> + hw_feat->aux_snap_num, XGBE_MAX_AUX_SNAP);
BTW, these messages are not very meaningful, maybe you should print both
min and max and say that the actual HW output is out of range?
> + hw_feat->aux_snap_num = XGBE_MAX_AUX_SNAP;
> + }
> +
> /* Translate the Hash Table size into actual number */
> switch (hw_feat->hash_table_size) {
> case 0:
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pps.c b/drivers/net/ethernet/amd/xgbe/xgbe-pps.c
> new file mode 100644
> index 000000000000..b5704fbbc5be
> --- /dev/null
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pps.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-3-Clause)
> +/*
> + * Copyright (c) 2014-2025, Advanced Micro Devices, Inc.
> + * Copyright (c) 2014, Synopsys, Inc.
> + * All rights reserved
> + *
> + * Author: Raju Rangoju <Raju.Rangoju@....com>
> + */
> +
> +#include "xgbe.h"
> +#include "xgbe-common.h"
> +
> +static inline u32 PPSx_MASK(unsigned int x)
> +{
> + return GENMASK(PPS_MAXIDX(x), PPS_MINIDX(x));
> +}
> +
> +static inline u32 PPSCMDx(unsigned int x, u32 val)
> +{
> + return ((val & GENMASK(3, 0)) << PPS_MINIDX(x));
Redundant outer ()s.
> +}
> +
> +static inline u32 TRGTMODSELx(unsigned int x, u32 val)
> +{
> + return ((val & GENMASK(1, 0)) << (PPS_MAXIDX(x) - 2));
Same here.
> +}
I believe you shouldn't name these functions that way, also pls no
inlines in .c files.
Either give them proper names and remove `inline` or make macros from them.
> +
> +int xgbe_pps_config(struct xgbe_prv_data *pdata,
> + struct xgbe_pps_config *cfg, int index, int on)
@on can be bool?
> +{
> + unsigned int value = 0;
> + unsigned int tnsec;
> + u64 period;
> +
> + tnsec = XGMAC_IOREAD(pdata, MAC_PPSx_TTNSR(index));
> + if (XGMAC_GET_BITS(tnsec, MAC_PPSx_TTNSR, TRGTBUSY0))
> + return -EBUSY;
> +
> + value = XGMAC_IOREAD(pdata, MAC_PPSCR);
> +
> + value &= ~PPSx_MASK(index);
I'd remove that NL between these two or even squash them into 1 line if
it fits into 80 chars.
> +
> + if (!on) {
> + value |= PPSCMDx(index, 0x5);
> + value |= PPSEN0;
> + XGMAC_IOWRITE(pdata, MAC_PPSCR, value);
> + return 0;
Newline before the return.
> + }
> +
> + XGMAC_IOWRITE(pdata, MAC_PPSx_TTSR(index), cfg->start.tv_sec);
> + XGMAC_IOWRITE(pdata, MAC_PPSx_TTNSR(index), cfg->start.tv_nsec);
> +
> + period = cfg->period.tv_sec * NSEC_PER_SEC;
> + period += cfg->period.tv_nsec;
> + do_div(period, XGBE_V2_TSTAMP_SSINC);
> +
> + if (period <= 1)
> + return -EINVAL;
> +
> + XGMAC_IOWRITE(pdata, MAC_PPSx_INTERVAL(index), period - 1);
> + period >>= 1;
> + if (period <= 1)
> + return -EINVAL;
> +
> + XGMAC_IOWRITE(pdata, MAC_PPSx_WIDTH(index), period - 1);
> +
> + value |= PPSCMDx(index, 0x2);
> + value |= TRGTMODSELx(index, 0x2);
> + value |= PPSEN0;
> +
> + XGMAC_IOWRITE(pdata, MAC_PPSCR, value);
> + return 0;
Same here.
> +}
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c b/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
> index 3658afc7801d..0e0b8ec3b504 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
> @@ -106,7 +106,29 @@ static int xgbe_settime(struct ptp_clock_info *info,
> static int xgbe_enable(struct ptp_clock_info *info,
> struct ptp_clock_request *request, int on)
> {
> - return -EOPNOTSUPP;
> + struct xgbe_prv_data *pdata = container_of(info, struct xgbe_prv_data,
> + ptp_clock_info);
> + struct xgbe_pps_config *pps_cfg;
> + unsigned long flags;
> + int ret;
> +
> + dev_dbg(pdata->dev, "rq->type %d on %d\n", request->type, on);
> +
> + if (request->type != PTP_CLK_REQ_PEROUT)
> + return -EOPNOTSUPP;
> +
> + pps_cfg = &pdata->pps[request->perout.index];
> +
> + pps_cfg->start.tv_sec = request->perout.start.sec;
> + pps_cfg->start.tv_nsec = request->perout.start.nsec;
> + pps_cfg->period.tv_sec = request->perout.period.sec;
> + pps_cfg->period.tv_nsec = request->perout.period.nsec;
> +
> + spin_lock_irqsave(&pdata->tstamp_lock, flags);
> + ret = xgbe_pps_config(pdata, pps_cfg, request->perout.index, on);
> + spin_unlock_irqrestore(&pdata->tstamp_lock, flags);
Are you sure you need to protect the whole xgbe_pps_config() from
interrupts? It's quite large and I don't think you need that lock
(and IRQ protection) for its entire runtime.
> +
> + return ret;
> }
>
> void xgbe_ptp_register(struct xgbe_prv_data *pdata)
Thanks,
Olek
Powered by blists - more mailing lists