From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 31854C88E72 for ; Tue, 15 Sep 2026 01:50:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: MIME-Version:References:In-Reply-To:Message-ID:Date:Subject:Cc:To:From: Reply-To:Content-Type:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=KEjRpuiPoM3YCn07cLeuZo1dk5pw+ZehBoPJPSHAXmA=; b=pVvlq27gz0zGudfd9oyykaVQN5 gSl9RHiFA35mQWWQ3jYblK4u7q34j1pgeI89fmJ8Y1GpBS6QnHaZZnaYwmGGziYEi4golOCfAyusf 4D589tNv5fb8RAmdlNayi7VCnmCpL4qBqfYhNMrThjCBIlWQWKLEtuqbTHWljQ2i34vDGzIyHrXyW e/XMrJt+Xwiu7VrrZVpz6XMcmh2NYSYw3BYQ8NLDKsQex4yO8FeaQra4JiR+0PaPzIYI0dM3fNOkH u3CHladAreDM6EQfadjwP8unvxVbdAmzBg5lS804IcSDO+vQE2759YmOEERrYF2CsvkODLS2urkc6 HsTmGrwg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1x6IJ3-00000004yrU-1EAD; Tue, 15 Sep 2026 01:49:57 +0000 Received: from sea.source.kernel.org ([2600:3c0a:e001:78e:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1x6IJ1-00000004yrK-3r5v for linux-arm-kernel@lists.infradead.org; Tue, 15 Sep 2026 01:49:55 +0000 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by sea.source.kernel.org (Postfix) with ESMTP id 56EA840475; Tue, 15 Sep 2026 01:49:55 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C5F7E1F000FF; Tue, 15 Sep 2026 01:49:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1789436995; bh=KEjRpuiPoM3YCn07cLeuZo1dk5pw+ZehBoPJPSHAXmA=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=n4Q8UBwkTzN2q6d/hTDJ6UT6qaIyZGzQpeJIVOQDW1/QlQJHp0iXYh81gUtBR6ZCr DeMqliK0+BLaIrz0b1xtrmPxJkBatu3LtUdn4mT5PvUz7VGUrd++fVHlPdv6F30azG 46eMa7eFZvMN/SojwGw0Mn9IGu2I7LDHsIIFt9LUjokFMuQw8EtYW35hEnTEMXAMem zSjaR7km/rn4BSU5zghblVzQh7cT30qYh1R1eiABHH/s6F6LOaeGgzbcJ4WpdrmBU0 f4E/R5hCU7QO2ENZZ/2Q+slBKzsmUshN8JHqbYHAK6YJUjSA7dtSYAGqgnAhkXsQ34 +TfM8QIo2TX1w== From: Jakub Kicinski To: zxyan0222@gmail.com Cc: Jakub Kicinski , andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, pabeni@redhat.com, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, richardcochran@gmail.com, maxime.chevallier@bootlin.com, muhammad.nazim.amirul.nazle.asmade@altera.com, rohan.g.thomas@altera.com, netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next v5 1/2] net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS Date: Mon, 14 Sep 2026 18:49:32 -0700 Message-ID: <20260915014931.873912-2-kuba@kernel.org> X-Mailer: git-send-email 2.55.0 In-Reply-To: <20260910081020.86227-2-zxyan0222@gmail.com> References: <20260910081020.86227-2-zxyan0222@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS This changes the Agilex5 smtg_crosststamp() handler so that it waits for the PTP_ACR_ATSFC FIFO clear to finish, then polls the ATSNS count in XGMAC_TIMESTAMP_STATUS instead of the transient TSIS bit in XGMAC_INT_STATUS. The snapshot count used for the FIFO pop loop is now taken from the value the poll read back rather than from a fresh register read. A few questions below. > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > index 1d7f0a57d2889..ee6f28637878a 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > @@ -340,6 +340,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system, > /* Release the mutex */ > mutex_unlock(&priv->aux_ts_lock); > > + /* Wait for the FIFO clear to complete so a stale ATSNS count from > + * a previous snapshot cannot satisfy the poll below before the new > + * snapshot is latched. > + */ > + ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value, > + !(acr_value & PTP_ACR_ATSFC), 10, 10000); > + if (ret) { > + netdev_err(priv->dev, "%s: Failed to clear snapshot FIFO\n", > + __func__); > + return ret; > + } > + > /* Trigger Internal snapshot signal. Create a rising edge by just toggle > * the GPO0 to low and back to high. > */ > @@ -349,9 +361,17 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system, > gpio_value |= XGMAC_GPIO_GPO0; > writel(gpio_value, ioaddr + XGMAC_GPIO_STATUS); > > - /* Poll for time sync operation done */ > - ret = readl_poll_timeout(priv->ioaddr + XGMAC_INT_STATUS, v, > - (v & XGMAC_INT_TSIS), 100, 10000); > + /* Wait for the auxiliary snapshot to be latched. TSIS is a > + * transient status bit that is set by any MAC timestamp event and > + * cleared by reading XGMAC_TIMESTAMP_STATUS, so it is not a > + * reliable completion condition. Poll the persistent ATSNS count > + * instead: it is cleared only by setting PTP_ACR_ATSFC, so > + * nothing can clear it while we wait, and it reflects exactly the > + * snapshot latched by this trigger. > + */ [Severity: Low] Is the "cleared only by setting PTP_ACR_ATSFC" part of this comment (and the matching "ATSNS is cleared only when the FIFO clear bit (PTP_ACR_ATSFC) is set" in the commit message) accurate? ATSNS is the auxiliary snapshot FIFO fill level, and popping the FIFO decrements it. get_ptptime() in stmmac_hwtstamp.c does the pop: ns = readl(ptpaddr + PTP_ATNR); ns += (u64)readl(ptpaddr + PTP_ATSR) * NSEC_PER_SEC; That is also why both timestamp_interrupt() and the loop just below in smtg_crosststamp() iterate exactly num_snapshot times: /* Repeat until the timestamps are from the FIFO last segment */ for (i = 0; i < num_snapshot; i++) { read_lock_irqsave(&priv->ptp_lock, flags); stmmac_get_ptptime(priv, ptpaddr, &ptp_time); Would it be more precise to say only that ATSNS is not cleared by reading XGMAC_TIMESTAMP_STATUS, rather than that nothing can clear it while the poll runs? > + ret = readl_poll_timeout(ioaddr + XGMAC_TIMESTAMP_STATUS, v, > + FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, v), > + 100, 10000); > if (ret) { > netdev_err(priv->dev, "%s: Wait for time sync operation timeout\n", > __func__); > @@ -364,8 +384,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system, > .use_nsecs = false, > }; > > - num_snapshot = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, > - readl(ioaddr + XGMAC_TIMESTAMP_STATUS)); > + num_snapshot = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, v); > > /* Repeat until the timestamps are from the FIFO last segment */ > for (i = 0; i < num_snapshot; i++) { [Severity: Medium] Can this reuse of the polled value hand a fabricated timestamp back to userspace? Everything from the ATSFC-clear wait through the GPO0 trigger, the ATSNS poll and this pop loop runs after aux_ts_lock has been dropped: /* Release the mutex */ mutex_unlock(&priv->aux_ts_lock); The STMMAC_FLAG_EXT_SNAPSHOT_EN test at the top of smtg_crosststamp(): if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN) return -EBUSY; is done without that mutex, while stmmac_enable() in stmmac_ptp.c sets the flag and writes PTP_ACR under it: priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN; /* Enable External snapshot trigger */ acr_value |= PTP_ACR_ATSEN(rq->extts.index); acr_value |= PTP_ACR_ATSFC; The PTP core does not serialize the callback either: drivers/ptp/ptp_chardev.c:ptp_ioctl() { ... case PTP_SYS_OFFSET_PRECISE: case PTP_SYS_OFFSET_PRECISE2: return ptp_sys_offset_precise(ptp, argptr, ptp->info->getcrosststamp); ... } So after the poll observes ATSNS = N, a PTP_CLK_REQ_EXTTS enable or a second concurrent smtg_crosststamp() can set PTP_ACR_ATSFC and flush the FIFO, or drain it itself. The loop then still runs N iterations against an empty FIFO, sets *device from whatever those reads return, and returns 0. Note the read_lock_irqsave(&priv->ptp_lock, flags) inside the loop is a reader lock, so two concurrent drains can interleave pops of the same FIFO. Before this patch the count was re-read immediately before the loop, so the same race produced num_snapshot == 0 and a skipped loop, i.e. a detectable failure rather than a stale value. Given that, does the commit message claim hold? "derive the count for the FIFO pop loop from the value the poll read back so it is guaranteed non-zero on the success path" Would holding aux_ts_lock (or another writer-exclusive lock) across the ATSFC wait, the trigger, the poll and the drain, and re-checking the count at drain time, be needed to make that guarantee real?