[PATCH resend net-next 1/1] net: stmmac: actually error requests to change the auxiliary snapshot capture channel

[PATCH resend net-next 1/1] net: stmmac: actually error requests to change the auxiliary snapshot capture channel

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Commit 2ddd05d1d5ed ("net: stmmac: do not silently change auxiliary snapshot capture channel")
added code that attempted to return -EBUSY to a PTP_CLK_REQ_EXTTS
request whan a snapshot was already enabled.
However it tested bits in 'acr_value' after they had been masked off
so the check would never return an error.

Change the code so that the test actually works.
Note that when the commit message says:
    Previously in case of a PTP_CLK_REQ_EXTTS request, previously active
    auxiliary snapshot capture channels were silently dropped and the new
    channel was activated.
this only refers to two commits earlier (a few minutes earlier).
Prior to that only a single fixed snapshot channel could be enabled.

Note that the check will reject requests to re-enable the currently
enabled channel.
Plausibly the best fix is just to delete the check completely.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---

Resend with corrected subject and To: Richard Cochran.

Found by a test for FIELD_GET() returning 'constant zero' from a
non-constant 'reg' value.

 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 3e30172fa129..aa17335a2031 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -222,19 +222,16 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 		write_unlock_irqrestore(&priv->ptp_lock, flags);
 		break;
 	}
-	case PTP_CLK_REQ_EXTTS: {
-		u8 channel;
-
+	case PTP_CLK_REQ_EXTTS:
 		mutex_lock(&priv->aux_ts_lock);
 		acr_value = readl(ptpaddr + PTP_ACR);
-		channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
-		acr_value &= ~PTP_ACR_MASK;
 
 		if (on) {
-			if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
+			u32 enabled_snapshots = FIELD_GET(PTP_ACR_MASK, acr_value);
+			if (enabled_snapshots) {
 				netdev_err(priv->dev,
 					   "Cannot enable auxiliary snapshot %d as auxiliary snapshot %d is already enabled",
-					rq->extts.index, channel);
+					rq->extts.index, ilog2(enabled_snapshots));
 				mutex_unlock(&priv->aux_ts_lock);
 				return -EBUSY;
 			}
@@ -245,6 +242,7 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 			acr_value |= PTP_ACR_ATSEN(rq->extts.index);
 			acr_value |= PTP_ACR_ATSFC;
 		} else {
+			acr_value &= ~PTP_ACR_MASK;
 			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
 		}
 		netdev_dbg(priv->dev, "Auxiliary Snapshot %d %s.\n",
@@ -256,7 +254,6 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 					 !(acr_value & PTP_ACR_ATSFC),
 					 10, 10000);
 		break;
-	}
 
 	default:
 		break;
-- 
2.39.5

Re: [PATCH resend net-next 1/1] net: stmmac: actually error requests to change the auxiliary snapshot capture channel

[Paolo Abeni]

On 3/10/26 3:01 PM, david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> Commit 2ddd05d1d5ed ("net: stmmac: do not silently change auxiliary snapshot capture channel")
> added code that attempted to return -EBUSY to a PTP_CLK_REQ_EXTTS
> request whan a snapshot was already enabled.
> However it tested bits in 'acr_value' after they had been masked off
> so the check would never return an error.
> 
> Change the code so that the test actually works.
> Note that when the commit message says:
>     Previously in case of a PTP_CLK_REQ_EXTTS request, previously active
>     auxiliary snapshot capture channels were silently dropped and the new
>     channel was activated.
> this only refers to two commits earlier (a few minutes earlier).
> Prior to that only a single fixed snapshot channel could be enabled.
> 
> Note that the check will reject requests to re-enable the currently
> enabled channel.
> Plausibly the best fix is just to delete the check completely.

I agree with this last statement. Enforcing the check could potentially
break the user-space.

/P

Re: [PATCH resend net-next 1/1] net: stmmac: actually error requests to change the auxiliary snapshot capture channel

[David Laight]

On Thu, 12 Mar 2026 15:07:49 +0100
Paolo Abeni <pabeni@redhat.com> wrote:

> On 3/10/26 3:01 PM, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > Commit 2ddd05d1d5ed ("net: stmmac: do not silently change auxiliary snapshot capture channel")
> > added code that attempted to return -EBUSY to a PTP_CLK_REQ_EXTTS
> > request whan a snapshot was already enabled.
> > However it tested bits in 'acr_value' after they had been masked off
> > so the check would never return an error.
> > 
> > Change the code so that the test actually works.
> > Note that when the commit message says:
> >     Previously in case of a PTP_CLK_REQ_EXTTS request, previously active
> >     auxiliary snapshot capture channels were silently dropped and the new
> >     channel was activated.
> > this only refers to two commits earlier (a few minutes earlier).
> > Prior to that only a single fixed snapshot channel could be enabled.
> > 
> > Note that the check will reject requests to re-enable the currently
> > enabled channel.
> > Plausibly the best fix is just to delete the check completely.  
> 
> I agree with this last statement. Enforcing the check could potentially
> break the user-space.

I did wonder why the 'half hearted' support for multiple snapshots
was added at all.
Either it is needed and should be done properly or always using a fixed
snapshot bit is fine.
Not that I've any idea what this code is for...

	David

> 
> /P
> 
>