[PATCH net-next v4 0/5] net: dsa: mv88e6xxx: various hwstamp fixes

[PATCH net-next v4 0/5] net: dsa: mv88e6xxx: various hwstamp fixes

[Luke Howard]

Three fixes for improving the reliably of hardware timestamp acquisition
on Marvell switches. In our tests this eliminated missed timestamps in
ptp4l, whilst also preserving PTP event and general message ordering on
switches that support embedded timestamps.

Signed-off-by: Luke Howard <lukeh@padl.com>
---
Changes in v4:
- Gate embedded PTP arrival time support on new DSA protocol
- Only timestamp PTPv2 frames (Marvell switches do not support PTPv1)
- Link to v3: https://patch.msgid.link/20260719-mv88e6xxx-ptp-fixes-v3-0-161386ac8a62@padl.com

Changes in v3:
- Unset UDP checksum on all embedded timestamp cases
- Handle embedded PTP arrival times without worker dispatch
- Link to v2: https://patch.msgid.link/20260710-mv88e6xxx-ptp-fixes-v2-0-af97c38df247@padl.com

Changes in v2:
- Lineraize SKBs before retrieving embedded timestamp
- Hoist driver register lock out of per-frame loop
- Link to v1: https://patch.msgid.link/20260703-mv88e6xxx-ptp-fixes-v1-0-0138581889a9@padl.com

To: Andrew Lunn <andrew@lunn.ch>
To: Vladimir Oltean <olteanv@gmail.com>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Richard Cochran <richardcochran@gmail.com>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Florian Fainelli <f.fainelli@gmail.com>
To: Simon Horman <horms@kernel.org>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org

---
Luke Howard (5):
      net: dsa: mv88e6xxx: only time stamp PTPv2 frames
      net: dsa: mv88e6xxx: use ARRIVAL1 counter for all peer delay messages
      net: dsa: mv88e6xxx: decouple the PTP timecounter from the register lock
      dt-bindings: net: dsa: add EDSA arrival-timestamp tag protocol
      net: dsa: mv88e6xxx: embedded PTP timestamp support

 .../devicetree/bindings/net/dsa/dsa-port.yaml      |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c                   |  30 ++-
 drivers/net/dsa/mv88e6xxx/chip.h                   |  22 ++-
 drivers/net/dsa/mv88e6xxx/hwtstamp.c               | 218 ++++++++++++++++++---
 drivers/net/dsa/mv88e6xxx/hwtstamp.h               |  21 +-
 drivers/net/dsa/mv88e6xxx/ptp.c                    |  65 +++++-
 drivers/net/dsa/mv88e6xxx/ptp.h                    |   1 +
 include/net/dsa.h                                  |   2 +
 net/dsa/tag_dsa.c                                  |  18 ++
 9 files changed, 342 insertions(+), 36 deletions(-)
---
base-commit: ce6b4d3216b63f902bb8e9695ee6c10c83415f65
change-id: 20260630-mv88e6xxx-ptp-fixes-1732570b8829

Best regards,
--  
Luke Howard <lukeh@padl.com>

[PATCH net-next v4 1/5] net: dsa: mv88e6xxx: only time stamp PTPv2 frames

[Luke Howard]

Marvell switches only document support for IEEE 1588v2, not for PTPv1
(IEEE 1588-2022). Reject PTPv1 frames in mv88e6xxx_should_tstamp().

Assisted-by: Claude:claude-opus-5
Signed-off-by: Luke Howard <lukeh@padl.com>
---
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index 6e6472a3b75ad..e4326165e1896 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -216,6 +216,10 @@ static struct ptp_header *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip,
 	if (!chip->info->ptp_support)
 		return NULL;
 
+	/* Marvell switches do not support IEEE 1588-2002 (PTPv1). */
+	if ((type & PTP_CLASS_VMASK) != PTP_CLASS_V2)
+		return NULL;
+
 	hdr = ptp_parse_header(skb, type);
 	if (!hdr)
 		return NULL;

-- 
2.43.0

Re: [PATCH net-next v4 1/5] net: dsa: mv88e6xxx: only time stamp PTPv2 frames

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing PTP header linearization leading to potential out-of-bounds memory accesses.
- [High] The MV88E6XXX_HWTSTAMP_ENABLED bit flag is used as an ad-hoc synchronization mechanism for hardware reconfiguration, failing to protect against in-flight readers.
--

--- Patch [1]: [PATCH net-next v4 1/5] net: dsa: mv88e6xxx: only time stamp PTPv2 frames ---
commit 105747c72e976395df1d9d44672056bd86952a7c
Author: Luke Howard <lukeh@padl.com>

net: dsa: mv88e6xxx: only time stamp PTPv2 frames

Marvell switches only document support for IEEE 1588v2, not for PTPv1
(IEEE 1588-2022). Reject PTPv1 frames in mv88e6xxx_should_tstamp().

> diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> index 6e6472a3b75ad..e4326165e1896 100644
> --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> @@ -216,6 +216,10 @@ static struct ptp_header *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip,
>  	if (!chip->info->ptp_support)
>  		return NULL;
>  
> +	/* Marvell switches do not support IEEE 1588-2002 (PTPv1). */
> +	if ((type & PTP_CLASS_VMASK) != PTP_CLASS_V2)
> +		return NULL;
> +
>  	hdr = ptp_parse_header(skb, type);
>  	if (!hdr)
>  		return NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does ptp_parse_header()
guarantee that the returned header is in the linear region of the skb?

ptp_parse_header() bounds checks against skb->data + skb->len, which includes
paged data. If the header spans into paged fragments, callers that directly
dereference the returned header without first calling pskb_may_pull() could
cause an out-of-bounds read.

This happens in the RX path when calling is_pdelay_resp():

drivers/net/dsa/mv88e6xxx/hwtstamp.c:is_pdelay_resp() {
    return (hdr->tsmt & 0xf) == 3;
}

It also happens in seq_match():

drivers/net/dsa/mv88e6xxx/hwtstamp.c:seq_match() {
    ...
    hdr = ptp_parse_header(skb, type);
    return ts_seqid == ntohs(hdr->sequence_id);
}

and in the TX path:

drivers/net/dsa/mv88e6xxx/hwtstamp.c:mv88e6xxx_port_txtstamp() {
    ...
    ps->tx_seq_id = be16_to_cpu(hdr->sequence_id);
}

Could this lead to kernel oopses or garbage reads?


[Severity: High]
This is another pre-existing issue, but does using the
MV88E6XXX_HWTSTAMP_ENABLED bit flag provide adequate synchronization?

The configuration path in mv88e6xxx_set_hwtstamp_config() clears this bit
to act as an ad-hoc lock:

drivers/net/dsa/mv88e6xxx/hwtstamp.c:mv88e6xxx_set_hwtstamp_config() {
    /* Prevent the TX/RX paths from trying to interact with the
     * timestamp hardware while we reconfigure it.
     */
    clear_bit_unlock(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state);
    ...
}

And the TX/RX paths check it here in mv88e6xxx_should_tstamp(), just below
the diff hunk:

drivers/net/dsa/mv88e6xxx/hwtstamp.c:mv88e6xxx_should_tstamp() {
    ...
    if (!test_bit(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state))
        return NULL;
    ...
}

If an in-flight reader in the TX/RX path checks the flag, gets preempted,
and resumes after the configuration thread alters the hardware state,
couldn't it queue timestamp requests to disabled hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-mv88e6xxx-ptp-fixes-v4-0-10b4020155fc@padl.com?part=1

[PATCH net-next v4 2/5] net: dsa: mv88e6xxx: use ARRIVAL1 counter for all peer delay messages

[Luke Howard]

mv88e6xxx switches have two arrival timestamp counters for timestamping
PTP event messages. This permits more than one arriving event message's
timestamp to be captured. This is useful for the case where event
messages from a grandmaster arrive at the same time as PDelayReq/PDelayResp
messages from a peer.

Previously only PDelayResp messages were assigned to the second arrival
counter; this patch does so for PDelayReq messages as well.

Signed-off-by: Luke Howard <lukeh@padl.com>
---
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index e4326165e1896..d468d988b258c 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -323,9 +323,16 @@ static void mv88e6xxx_rxtstamp_work(struct mv88e6xxx_chip *chip,
 				   &ps->rx_queue2);
 }
 
-static int is_pdelay_resp(const struct ptp_header *hdr)
+static bool is_pdelay_msg(const struct ptp_header *hdr)
 {
-	return (hdr->tsmt & 0xf) == 3;
+	switch (ptp_get_msgtype(hdr, PTP_CLASS_V2)) {
+	case PTP_MSGTYPE_PDELAY_REQ:
+		fallthrough;
+	case PTP_MSGTYPE_PDELAY_RESP:
+		return true;
+	default:
+		return false;
+	}
 }
 
 bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
@@ -347,7 +354,7 @@ bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
 
 	SKB_PTP_TYPE(skb) = type;
 
-	if (is_pdelay_resp(hdr))
+	if (is_pdelay_msg(hdr))
 		skb_queue_tail(&ps->rx_queue2, skb);
 	else
 		skb_queue_tail(&ps->rx_queue, skb);
@@ -588,8 +595,9 @@ int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip)
 	if (err)
 		return err;
 
-	/* Use ARRIVAL1 for peer delay response messages. */
+	/* Use ARRIVAL1 for peer delay messages. */
 	err = mv88e6xxx_ptp_write(chip, MV88E6XXX_PTP_TS_ARRIVAL_PTR,
+				  MV88E6XXX_PTP_MSGTYPE_PDLAY_REQ |
 				  MV88E6XXX_PTP_MSGTYPE_PDLAY_RES);
 	if (err)
 		return err;

-- 
2.43.0

Re: [PATCH net-next v4 2/5] net: dsa: mv88e6xxx: use ARRIVAL1 counter for all peer delay messages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing SKB Linearization for PTP Header Access
--

--- Patch [2]: [PATCH net-next v4 2/5] net: dsa: mv88e6xxx: use ARRIVAL1 counter for all peer delay messages ---
commit 4e21fb1f9ef35cdb7536e32b19f9424e5f8c90e2
Author: Luke Howard <lukeh@padl.com>

net: dsa: mv88e6xxx: use ARRIVAL1 counter for all peer delay messages

This commit updates the mv88e6xxx driver to use the second arrival counter
(ARRIVAL1) for PDelayReq messages as well as PDelayResp messages. This allows
capturing timestamps for both when they arrive simultaneously from a peer.

> diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> @@ -323,9 +323,16 @@ static void mv88e6xxx_rxtstamp_work(struct mv88e6xxx_chip *chip,
>  				   &ps->rx_queue2);
>  }
>  
> -static int is_pdelay_resp(const struct ptp_header *hdr)
> +static bool is_pdelay_msg(const struct ptp_header *hdr)
>  {
> -	return (hdr->tsmt & 0xf) == 3;
> +	switch (ptp_get_msgtype(hdr, PTP_CLASS_V2)) {

[Severity: High]
This is a pre-existing issue, but can this dereference of the header cause an
out-of-bounds read if the PTP packet is fragmented?

Looking at ptp_parse_header() which provides the hdr pointer, it ensures the
header is within the total length of the SKB, but it does not seem to check if
it is within the linear data buffer:

net/core/ptp_classifier.c:ptp_parse_header() {
    ...
	/* Ensure that the entire header is present in this packet. */
	if (ptr + sizeof(struct ptp_header) > skb->data + skb->len)
		return NULL;

	return (struct ptp_header *)ptr;
}

If a maliciously crafted or fragmented PTP packet pushes the header into the
non-linear paged fragments, could this direct pointer access read past the
linear boundary when ptp_get_msgtype() accesses hdr->tsmt?

The same concern seems to apply to the read of hdr->sequence_id in
mv88e6xxx_port_txtstamp() on the TX path.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-mv88e6xxx-ptp-fixes-v4-0-10b4020155fc@padl.com?part=2

[PATCH net-next v4 3/5] net: dsa: mv88e6xxx: decouple the PTP timecounter from the register lock

[Luke Howard]

Use a dedicated spinlock, rather than the chip register mutex, to
protect read access to the hardware timestamp cycle counter.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Luke Howard <lukeh@padl.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c     |  1 +
 drivers/net/dsa/mv88e6xxx/chip.h     |  7 ++--
 drivers/net/dsa/mv88e6xxx/hwtstamp.c |  8 ++---
 drivers/net/dsa/mv88e6xxx/ptp.c      | 65 +++++++++++++++++++++++++++++++-----
 drivers/net/dsa/mv88e6xxx/ptp.h      |  1 +
 5 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 80b877c74513d..b24ebbb275b34 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -6635,6 +6635,7 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
 	chip->dev = dev;
 
 	mutex_init(&chip->reg_lock);
+	spin_lock_init(&chip->ptp_clock_lock);
 	INIT_LIST_HEAD(&chip->mdios);
 	idr_init(&chip->policies);
 	INIT_LIST_HEAD(&chip->msts);
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index e966e7c4cc5de..7272b7dc4955c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -419,9 +419,12 @@ struct mv88e6xxx_chip {
 	/* GPIO resources */
 	u8 gpio_data[2];
 
-	/* This cyclecounter abstracts the switch PTP time.
-	 * reg_lock must be held for any operation that read()s.
+	/* This cyclecounter abstracts the switch PTP time. ptp_clock_lock
+	 * protects tstamp_cc and tstamp_tc. tstamp_cycles caches the
+	 * result most recently returned by mv88e6xxx_ptp_read_cycles().
 	 */
+	spinlock_t		ptp_clock_lock;
+	u64			tstamp_cycles;
 	struct cyclecounter	tstamp_cc;
 	struct timecounter	tstamp_tc;
 	struct delayed_work	overflow_work;
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index d468d988b258c..a5b362a999e64 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -293,9 +293,7 @@ static void mv88e6xxx_get_rxts(struct mv88e6xxx_chip *chip,
 		if (mv88e6xxx_ts_valid(status) && seq_match(skb, seq_id)) {
 			ns = timehi << 16 | timelo;
 
-			mv88e6xxx_reg_lock(chip);
-			ns = timecounter_cyc2time(&chip->tstamp_tc, ns);
-			mv88e6xxx_reg_unlock(chip);
+			ns = mv88e6xxx_timecounter_cyc2time(chip, ns);
 			shwt = skb_hwtstamps(skb);
 			memset(shwt, 0, sizeof(*shwt));
 			shwt->hwtstamp = ns_to_ktime(ns);
@@ -420,9 +418,7 @@ static int mv88e6xxx_txtstamp_work(struct mv88e6xxx_chip *chip,
 
 	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
 	time_raw = ((u32)departure_block[2] << 16) | departure_block[1];
-	mv88e6xxx_reg_lock(chip);
-	ns = timecounter_cyc2time(&chip->tstamp_tc, time_raw);
-	mv88e6xxx_reg_unlock(chip);
+	ns = mv88e6xxx_timecounter_cyc2time(chip, time_raw);
 	shhwtstamps.hwtstamp = ns_to_ktime(ns);
 
 	dev_dbg(chip->dev,
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
index f7603573d3a98..2a7f97625e375 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.c
+++ b/drivers/net/dsa/mv88e6xxx/ptp.c
@@ -231,15 +231,37 @@ static void mv88e6352_tai_event_work(struct work_struct *ugly)
 
 	/* We only have one timestamping channel. */
 	ev.index = 0;
-	mv88e6xxx_reg_lock(chip);
-	ev.timestamp = timecounter_cyc2time(&chip->tstamp_tc, raw_ts);
-	mv88e6xxx_reg_unlock(chip);
+	ev.timestamp = mv88e6xxx_timecounter_cyc2time(chip, raw_ts);
 
 	ptp_clock_event(chip->ptp_clock, &ev);
 out:
 	schedule_delayed_work(&chip->tai_event_work, TAI_EVENT_WORK_INTERVAL);
 }
 
+/* Refresh the cached counter value that the read() callback returns. The
+ * read cannot acquire the register lock whilst holding ptp_clock_lock
+ * because MDIO reads can sleep. The caller must hold reg_lock, which
+ * serializes against the other timecounter writers.
+ */
+static void mv88e6xxx_ptp_read_cycles(struct mv88e6xxx_chip *chip)
+{
+	const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops;
+
+	if (ptp_ops->clock_read)
+		chip->tstamp_cycles = ptp_ops->clock_read(&chip->tstamp_cc);
+}
+
+u64 mv88e6xxx_timecounter_cyc2time(struct mv88e6xxx_chip *chip, u64 cycles)
+{
+	u64 ns;
+
+	spin_lock_bh(&chip->ptp_clock_lock);
+	ns = timecounter_cyc2time(&chip->tstamp_tc, cycles);
+	spin_unlock_bh(&chip->ptp_clock_lock);
+
+	return ns;
+}
+
 static int mv88e6xxx_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 {
 	struct mv88e6xxx_chip *chip = ptp_to_chip(ptp);
@@ -258,9 +280,12 @@ static int mv88e6xxx_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	diff = div_u64(adj, chip->cc_coeffs->cc_mult_dem);
 
 	mv88e6xxx_reg_lock(chip);
+	mv88e6xxx_ptp_read_cycles(chip);
 
+	spin_lock_bh(&chip->ptp_clock_lock);
 	timecounter_read(&chip->tstamp_tc);
 	chip->tstamp_cc.mult = neg_adj ? mult - diff : mult + diff;
+	spin_unlock_bh(&chip->ptp_clock_lock);
 
 	mv88e6xxx_reg_unlock(chip);
 
@@ -271,8 +296,16 @@ static int mv88e6xxx_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 {
 	struct mv88e6xxx_chip *chip = ptp_to_chip(ptp);
 
+	/* No register access is needed here, but reg_lock still serialises
+	 * this against the other timecounter writers, which drop it only
+	 * after their hardware read has completed.
+	 */
 	mv88e6xxx_reg_lock(chip);
+
+	spin_lock_bh(&chip->ptp_clock_lock);
 	timecounter_adjtime(&chip->tstamp_tc, delta);
+	spin_unlock_bh(&chip->ptp_clock_lock);
+
 	mv88e6xxx_reg_unlock(chip);
 
 	return 0;
@@ -285,7 +318,12 @@ static int mv88e6xxx_ptp_gettime(struct ptp_clock_info *ptp,
 	u64 ns;
 
 	mv88e6xxx_reg_lock(chip);
+	mv88e6xxx_ptp_read_cycles(chip);
+
+	spin_lock_bh(&chip->ptp_clock_lock);
 	ns = timecounter_read(&chip->tstamp_tc);
+	spin_unlock_bh(&chip->ptp_clock_lock);
+
 	mv88e6xxx_reg_unlock(chip);
 
 	*ts = ns_to_timespec64(ns);
@@ -302,7 +340,12 @@ static int mv88e6xxx_ptp_settime(struct ptp_clock_info *ptp,
 	ns = timespec64_to_ns(ts);
 
 	mv88e6xxx_reg_lock(chip);
+	mv88e6xxx_ptp_read_cycles(chip);
+
+	spin_lock_bh(&chip->ptp_clock_lock);
 	timecounter_init(&chip->tstamp_tc, &chip->tstamp_cc, ns);
+	spin_unlock_bh(&chip->ptp_clock_lock);
+
 	mv88e6xxx_reg_unlock(chip);
 
 	return 0;
@@ -444,14 +487,12 @@ const struct mv88e6xxx_ptp_ops mv88e6390_ptp_ops = {
 		(1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ),
 };
 
+/* Return the value most recently fetched by mv88e6xxx_ptp_read_cycles()
+ * rather than reading the hardware over MDIO.
+ */
 static u64 mv88e6xxx_ptp_clock_read(struct cyclecounter *cc)
 {
-	struct mv88e6xxx_chip *chip = cc_to_chip(cc);
-
-	if (chip->info->ops->ptp_ops->clock_read)
-		return chip->info->ops->ptp_ops->clock_read(cc);
-
-	return 0;
+	return cc_to_chip(cc)->tstamp_cycles;
 }
 
 /* With a 250MHz input clock, the 32-bit timestamp counter overflows in ~17.2
@@ -486,6 +527,12 @@ int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip)
 	chip->tstamp_cc.mult	= chip->cc_coeffs->cc_mult;
 	chip->tstamp_cc.shift	= chip->cc_coeffs->cc_shift;
 
+	/* Prime the cycle counter cache for the timecounter_init() below.
+	 * The caller holds reg_lock, and nothing can reach the PTP clock
+	 * until ptp_clock_register() below, so no locking is needed here.
+	 */
+	mv88e6xxx_ptp_read_cycles(chip);
+
 	timecounter_init(&chip->tstamp_tc, &chip->tstamp_cc,
 			 ktime_to_ns(ktime_get_real()));
 
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
index 95bdddb0bf39f..44718e120de20 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.h
+++ b/drivers/net/dsa/mv88e6xxx/ptp.h
@@ -68,6 +68,7 @@
 
 int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip);
 void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip);
+u64 mv88e6xxx_timecounter_cyc2time(struct mv88e6xxx_chip *chip, u64 cycles);
 
 #define ptp_to_chip(ptp) container_of(ptp, struct mv88e6xxx_chip,	\
 				      ptp_clock_info)

-- 
2.43.0

Re: [PATCH net-next v4 3/5] net: dsa: mv88e6xxx: decouple the PTP timecounter from the register lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling cancel_delayed_work_sync() while holding a mutex that the work callback also acquires leads to an AB-BA deadlock.
--

--- Patch [3]: [PATCH net-next v4 3/5] net: dsa: mv88e6xxx: decouple the PTP timecounter from the register lock ---
commit 23ddfde2f70b36ac19aedfb1142d819436fc0957
Author: Luke Howard <lukeh@padl.com>

net: dsa: mv88e6xxx: decouple the PTP timecounter from the register lock

Use a dedicated spinlock, rather than the chip register mutex, to
protect read access to the hardware timestamp cycle counter.

> diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
> index f7603573d3a98..2a7f97625e375 100644
> --- a/drivers/net/dsa/mv88e6xxx/ptp.c
> +++ b/drivers/net/dsa/mv88e6xxx/ptp.c
> @@ -231,15 +231,37 @@ static void mv88e6352_tai_event_work(struct work_struct *ugly)
>  
>  	/* We only have one timestamping channel. */
>  	ev.index = 0;
> -	mv88e6xxx_reg_lock(chip);
> -	ev.timestamp = timecounter_cyc2time(&chip->tstamp_tc, raw_ts);
> -	mv88e6xxx_reg_unlock(chip);
> +	ev.timestamp = mv88e6xxx_timecounter_cyc2time(chip, raw_ts);
>  
>  	ptp_clock_event(chip->ptp_clock, &ev);
>  out:
>  	schedule_delayed_work(&chip->tai_event_work, TAI_EVENT_WORK_INTERVAL);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this worker I
noticed a potential deadlock.

If a userspace process calls the PTP_EXTTS_REQUEST ioctl to disable external
timestamps (on=0), it reaches mv88e6352_ptp_enable_extts() which takes the
reg_lock mutex:

drivers/net/dsa/mv88e6xxx/ptp.c:mv88e6352_ptp_enable_extts() {
...
	mv88e6xxx_reg_lock(chip);
...
	} else {
...
		cancel_delayed_work_sync(&chip->tai_event_work);
	}
...
}

Concurrently, the delayed worker tai_event_work can fire, and it attempts
to take reg_lock via mv88e6xxx_reg_lock() earlier in the function:

drivers/net/dsa/mv88e6xxx/ptp.c:mv88e6352_tai_event_work() {
...
	mv88e6xxx_reg_lock(chip);
	err = mv88e6xxx_tai_read(...);
...
}

Because cancel_delayed_work_sync() is waiting for the worker to finish while
holding the mutex the worker needs, can this lead to an AB-BA deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-mv88e6xxx-ptp-fixes-v4-0-10b4020155fc@padl.com?part=3

[PATCH net-next v4 4/5] dt-bindings: net: dsa: add EDSA arrival-timestamp tag protocol

[Luke Howard]

Add edsa-ptp-reserved2-ts to the dsa-tag-protocol enum. This
EDSA-derived protocol lets a board select in-frame embedding of the PTP
arrival timestamp, which the switch writes over the reserved2 field of
the PTP common header, on switches that support it. Giving it its own
identifier keeps the wire format discoverable via dsa/tagging.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Luke Howard <lukeh@padl.com>
---
 Documentation/devicetree/bindings/net/dsa/dsa-port.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
index 4801204699534..8d12c8f1bd51e 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
@@ -53,6 +53,7 @@ properties:
     enum:
       - dsa
       - edsa
+      - edsa-ptp-reserved2-ts
       - ocelot
       - ocelot-8021q
       - rtl8_4

-- 
2.43.0

[PATCH net-next v4 5/5] net: dsa: mv88e6xxx: embedded PTP timestamp support

[Luke Howard]

mv88e6xxx switches can embed the PTP arrival timestamp directly in the
frame, at a configurable offset from the PTP common header, typically
the offset of the reserved2 field.

Add support for this on the 88E6341 and 88E6352 switches, being those on
which I was able to verify this. Other switch chips may also work. The
arrival timestamp offsets are relative to the PTP common header and will
work for both L2 and L3 PTP packets; the respective headers are skipped
by the number of bytes set in the ETJump and IPJump registers, which are
initialized to sensible defaults and are VLAN tag-aware.

This feature is off by default and must be enabled by selecting the
"edsa-ptp-reserved2-ts" DSA tag protocol.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Luke Howard <lukeh@padl.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c     |  29 +++++-
 drivers/net/dsa/mv88e6xxx/chip.h     |  15 +++
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 194 ++++++++++++++++++++++++++++++++---
 drivers/net/dsa/mv88e6xxx/hwtstamp.h |  21 +++-
 include/net/dsa.h                    |   2 +
 net/dsa/tag_dsa.c                    |  18 ++++
 6 files changed, 262 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b24ebbb275b34..e92524beb40c4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3271,7 +3271,8 @@ static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port)
 	if (chip->tag_protocol == DSA_TAG_PROTO_DSA)
 		return mv88e6xxx_set_port_mode_dsa(chip, port);
 
-	if (chip->tag_protocol == DSA_TAG_PROTO_EDSA)
+	if (chip->tag_protocol == DSA_TAG_PROTO_EDSA ||
+	    chip->tag_protocol == DSA_TAG_PROTO_EDSA_PTP_RESERVED2_TS)
 		return mv88e6xxx_set_port_mode_edsa(chip, port);
 
 	return -EINVAL;
@@ -6370,6 +6371,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ptp_support = true,
 		.ops = &mv88e6341_ops,
+		.supports_ptp_embedded_ts = true,
 	},
 
 	[MV88E6350] = {
@@ -6447,6 +6449,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ptp_support = true,
 		.ops = &mv88e6352_ops,
+		.supports_ptp_embedded_ts = true,
 	},
 	[MV88E6361] = {
 		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6361,
@@ -6659,9 +6662,17 @@ static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	enum dsa_tag_protocol old_protocol;
 	struct dsa_port *cpu_dp;
-	int err;
+	int err = 0;
 
+	/* The embedded arrival-time-stamp protocol is an EDSA extension: EDSA
+	 * is the modern tag format for these switches, so it is the one we
+	 * extend; the legacy DSA tag is intentionally left un-extended.
+	 */
 	switch (proto) {
+	case DSA_TAG_PROTO_EDSA_PTP_RESERVED2_TS:
+		if (!chip->info->supports_ptp_embedded_ts)
+			return -EPROTONOSUPPORT;
+		fallthrough;
 	case DSA_TAG_PROTO_EDSA:
 		switch (chip->info->edsa_support) {
 		case MV88E6XXX_EDSA_UNSUPPORTED:
@@ -6690,7 +6701,12 @@ static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds,
 			goto unwind;
 		}
 	}
+
+	if (chip->info->ptp_support)
+		err = mv88e6xxx_hwtstamp_setup_arr_ts(chip);
 	mv88e6xxx_reg_unlock(chip);
+	if (err)
+		goto unwind;
 
 	return 0;
 
@@ -6700,6 +6716,15 @@ static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds,
 	mv88e6xxx_reg_lock(chip);
 	dsa_switch_for_each_cpu_port_continue_reverse(cpu_dp, ds)
 		mv88e6xxx_setup_port_mode(chip, cpu_dp->index);
+	if (chip->info->ptp_support) {
+		/* Nothing else can be done here, but leaving a port embedding
+		 * time stamps the driver will not strip corrupts every PTP
+		 * event frame it receives, so make the failure visible.
+		 */
+		if (mv88e6xxx_hwtstamp_setup_arr_ts(chip))
+			dev_err(chip->dev,
+				"failed to restore arrival time stamp mode\n");
+	}
 	mv88e6xxx_reg_unlock(chip);
 
 	return err;
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 7272b7dc4955c..c75a71464b20a 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -180,6 +180,13 @@ struct mv88e6xxx_info {
 	 * port 0, 1 means internal PHYs range starts at port 1, etc
 	 */
 	unsigned int internal_phys_offset;
+
+	/* The switch can embed the PTP arrival time stamp in the reserved2
+	 * field of the PTP common header, which the edsa-ptp-reserved2-ts
+	 * tag protocol selects. Otherwise arrival time stamps are only
+	 * available from the switch registers.
+	 */
+	bool supports_ptp_embedded_ts;
 };
 
 struct mv88e6xxx_atu_entry {
@@ -818,6 +825,14 @@ static inline bool mv88e6xxx_has_lag(struct mv88e6xxx_chip *chip)
 	return !!chip->info->global2_addr;
 }
 
+/* Whether the active tagging protocol has the switch embed arrival time stamps
+ * in the frame instead of leaving them in the PTP arrival registers.
+ */
+static inline bool mv88e6xxx_ptp_embedded_ts(struct mv88e6xxx_chip *chip)
+{
+	return chip->tag_protocol == DSA_TAG_PROTO_EDSA_PTP_RESERVED2_TS;
+}
+
 static inline bool mv88e6xxx_has_tcam(struct mv88e6xxx_chip *chip)
 {
 	return !!chip->info->tcam_addr;
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index a5b362a999e64..74eaadeeeb784 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -15,6 +15,7 @@
 #include "hwtstamp.h"
 #include "ptp.h"
 #include <linux/ptp_classify.h>
+#include <linux/unaligned.h>
 
 #define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
 
@@ -94,6 +95,7 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port,
 	const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops;
 	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
 	bool tstamp_enable = false;
+	int err = 0;
 
 	/* Prevent the TX/RX paths from trying to interact with the
 	 * timestamp hardware while we reconfigure it.
@@ -147,20 +149,30 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port,
 	if (tstamp_enable) {
 		chip->enable_count += 1;
 		if (chip->enable_count == 1 && ptp_ops->global_enable)
-			ptp_ops->global_enable(chip);
-		if (ptp_ops->port_enable)
-			ptp_ops->port_enable(chip, port);
+			err = ptp_ops->global_enable(chip);
+		if (!err && ptp_ops->port_enable)
+			err = ptp_ops->port_enable(chip, port);
 	} else {
 		if (ptp_ops->port_disable)
-			ptp_ops->port_disable(chip, port);
+			err = ptp_ops->port_disable(chip, port);
 		chip->enable_count -= 1;
 		if (chip->enable_count == 0 && ptp_ops->global_disable)
 			ptp_ops->global_disable(chip);
 	}
 	mv88e6xxx_reg_unlock(chip);
 
+	if (err)
+		return err;
+
 	/* Once hardware has been configured, enable timestamp checks
 	 * in the RX/TX paths.
+	 *
+	 * In embedded mode the switch starts overwriting the PTP header's
+	 * reserved2 field as soon as CFG2 is armed above, which is before the
+	 * RX path begins restoring it; on the way down the RX path stops
+	 * restoring before the hardware stops embedding. An event frame
+	 * received inside either window is delivered with the switch's
+	 * arrival counter still in its header.
 	 */
 	if (tstamp_enable)
 		set_bit(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state);
@@ -249,6 +261,52 @@ static int seq_match(struct sk_buff *skb, u16 ts_seqid)
 	return ts_seqid == ntohs(hdr->sequence_id);
 }
 
+/* Recover the arrival time the switch wrote over the reserved2 field of the PTP
+ * common header, restoring the field afterwards.
+ */
+static bool parse_embedded_ts(struct sk_buff *skb, u64 *ns)
+{
+	unsigned int off = MV88E6XXX_PTP_ARR_TS_OFFSET;
+	struct ptp_header *hdr;
+
+	*ns = 0;
+
+	if (skb_linearize(skb))
+		return false;
+
+	hdr = ptp_parse_header(skb, SKB_PTP_TYPE(skb));
+	if (!hdr)
+		return false;
+
+	*ns = get_unaligned_be32((u8 *)hdr + off);
+	memset((u8 *)hdr + off, 0, 4);
+	skb_checksum_complete_unset(skb);
+
+	return true;
+}
+
+/* Apply the arrival time the switch embedded in the frame. No register access
+ * is needed, so this runs inline on the receive path rather than being handed
+ * to the PTP worker. Returns false if the frame could not be restored.
+ */
+static bool mv88e6xxx_ptp_rx_timestamp(struct mv88e6xxx_chip *chip,
+				       struct sk_buff *skb)
+{
+	struct skb_shared_hwtstamps *shwt;
+	u64 ns;
+
+	if (!parse_embedded_ts(skb, &ns))
+		return false;
+
+	ns = mv88e6xxx_timecounter_cyc2time(chip, ns);
+
+	shwt = skb_hwtstamps(skb);
+	memset(shwt, 0, sizeof(*shwt));
+	shwt->hwtstamp = ns_to_ktime(ns);
+
+	return true;
+}
+
 static void mv88e6xxx_get_rxts(struct mv88e6xxx_chip *chip,
 			       struct mv88e6xxx_port_hwtstamp *ps,
 			       struct sk_buff *skb, u16 reg,
@@ -310,7 +368,6 @@ static void mv88e6xxx_rxtstamp_work(struct mv88e6xxx_chip *chip,
 	struct sk_buff *skb;
 
 	skb = skb_dequeue(&ps->rx_queue);
-
 	if (skb)
 		mv88e6xxx_get_rxts(chip, ps, skb, ptp_ops->arr0_sts_reg,
 				   &ps->rx_queue);
@@ -352,6 +409,21 @@ bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
 
 	SKB_PTP_TYPE(skb) = type;
 
+	/* Embedded arrival times can be returned inline. The switch has
+	 * overwritten the reserved2 field; if it cannot be restored the frame
+	 * is no longer what the sender transmitted, so drop it rather than
+	 * pass up a corrupted header that will fail the PTP-over-UDP checksum
+	 * anyway.
+	 */
+	if (mv88e6xxx_ptp_embedded_ts(chip)) {
+		if (!mv88e6xxx_ptp_rx_timestamp(chip, skb)) {
+			kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
+			return true;
+		}
+
+		return false;
+	}
+
 	if (is_pdelay_msg(hdr))
 		skb_queue_tail(&ps->rx_queue2, skb);
 	else
@@ -461,7 +533,9 @@ long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp)
 		if (test_bit(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS, &ps->state))
 			restart |= mv88e6xxx_txtstamp_work(chip, ps);
 
-		mv88e6xxx_rxtstamp_work(chip, ps);
+		/* Embedded arrival times are applied on the receive path. */
+		if (!mv88e6xxx_ptp_embedded_ts(chip))
+			mv88e6xxx_rxtstamp_work(chip, ps);
 	}
 
 	return restart ? 1 : -1;
@@ -530,14 +604,43 @@ int mv88e6165_global_enable(struct mv88e6xxx_chip *chip)
 
 int mv88e6352_hwtstamp_port_disable(struct mv88e6xxx_chip *chip, int port)
 {
-	return mv88e6xxx_port_ptp_write(chip, port, MV88E6XXX_PORT_PTP_CFG0,
-					MV88E6XXX_PORT_PTP_CFG0_DISABLE_PTP);
+	int err;
+
+	err = mv88e6xxx_port_ptp_write(chip, port, MV88E6XXX_PORT_PTP_CFG0,
+				       MV88E6XXX_PORT_PTP_CFG0_DISABLE_PTP);
+	if (err)
+		return err;
+
+	err = mv88e6xxx_port_ptp_write(chip, port, MV88E6XXX_PORT_PTP_CFG2, 0);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+/* Return the CFG2 ArrTSMode field for the active arrival time-stamp mode. */
+static u16 mv88e6xxx_arr_ts_cfg2(struct mv88e6xxx_chip *chip)
+{
+	return mv88e6xxx_ptp_embedded_ts(chip) ?
+		MV88E6XXX_PORT_PTP_CFG2_ARR_TS_RESERVED2 :
+		MV88E6XXX_PORT_PTP_CFG2_ARR_TS_DISABLED;
 }
 
 int mv88e6352_hwtstamp_port_enable(struct mv88e6xxx_chip *chip, int port)
 {
-	return mv88e6xxx_port_ptp_write(chip, port, MV88E6XXX_PORT_PTP_CFG0,
-					MV88E6XXX_PORT_PTP_CFG0_DISABLE_TSPEC_MATCH);
+	int err;
+
+	err = mv88e6xxx_port_ptp_write(chip, port, MV88E6XXX_PORT_PTP_CFG2,
+				       mv88e6xxx_arr_ts_cfg2(chip));
+	if (err)
+		return err;
+
+	err = mv88e6xxx_port_ptp_write(chip, port, MV88E6XXX_PORT_PTP_CFG0,
+				       MV88E6XXX_PORT_PTP_CFG0_DISABLE_TSPEC_MATCH);
+	if (err)
+		return err;
+
+	return 0;
 }
 
 static int mv88e6xxx_hwtstamp_port_setup(struct mv88e6xxx_chip *chip, int port)
@@ -556,6 +659,72 @@ static int mv88e6xxx_hwtstamp_port_setup(struct mv88e6xxx_chip *chip, int port)
 	return 0;
 }
 
+/* Program the hardware for the arrival time-stamp mode the active tagging
+ * protocol selects. Called from setup and again whenever the protocol changes,
+ * as that is what decides where arrival time stamps are delivered.
+ *
+ * Must be called with the register lock held.
+ */
+int mv88e6xxx_hwtstamp_setup_arr_ts(struct mv88e6xxx_chip *chip)
+{
+	const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops;
+	int err, ret = 0;
+	int i;
+
+	/* In embedded mode the switch stamps every event message it forwards,
+	 * peer delay included, so leave the arrival capture pointer alone
+	 * rather than diverting peer delay messages to the ARRIVAL1 registers.
+	 */
+	err = mv88e6xxx_ptp_write(chip, MV88E6XXX_PTP_TS_ARRIVAL_PTR,
+				  mv88e6xxx_ptp_embedded_ts(chip) ? 0 :
+				  MV88E6XXX_PTP_MSGTYPE_PDLAY_REQ |
+				  MV88E6XXX_PTP_MSGTYPE_PDLAY_RES);
+	if (err)
+		return err;
+
+	if (!chip->info->supports_ptp_embedded_ts)
+		return 0;
+
+	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
+		struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[i];
+
+		/* Anything already queued was queued under the previous mode
+		 * and can no longer be matched with an arrival time.
+		 */
+		skb_queue_purge(&ps->rx_queue);
+		skb_queue_purge(&ps->rx_queue2);
+
+		/* The arrival registers keep capturing in embedded mode with
+		 * nothing to drain them, so clear the status here or the
+		 * register path resumes on a stale capture.
+		 */
+		err = mv88e6xxx_port_ptp_write(chip, i, ptp_ops->arr0_sts_reg, 0);
+		if (err && !ret)
+			ret = err;
+
+		err = mv88e6xxx_port_ptp_write(chip, i, ptp_ops->arr1_sts_reg, 0);
+		if (err && !ret)
+			ret = err;
+
+		/* Ports that have not enabled time stamping pick the mode up in
+		 * mv88e6352_hwtstamp_port_enable().
+		 */
+		if (ps->tstamp_config.rx_filter == HWTSTAMP_FILTER_NONE)
+			continue;
+
+		/* Program every port even if one fails. A port left embedding
+		 * time stamps that the driver no longer strips corrupts the
+		 * PTP header of every event frame it receives.
+		 */
+		err = mv88e6xxx_port_ptp_write(chip, i, MV88E6XXX_PORT_PTP_CFG2,
+					       mv88e6xxx_arr_ts_cfg2(chip));
+		if (err && !ret)
+			ret = err;
+	}
+
+	return ret;
+}
+
 int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip)
 {
 	const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops;
@@ -591,10 +760,7 @@ int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip)
 	if (err)
 		return err;
 
-	/* Use ARRIVAL1 for peer delay messages. */
-	err = mv88e6xxx_ptp_write(chip, MV88E6XXX_PTP_TS_ARRIVAL_PTR,
-				  MV88E6XXX_PTP_MSGTYPE_PDLAY_REQ |
-				  MV88E6XXX_PTP_MSGTYPE_PDLAY_RES);
+	err = mv88e6xxx_hwtstamp_setup_arr_ts(chip);
 	if (err)
 		return err;
 
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
index c359821d5a6ea..2fe7c7429bff1 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
@@ -13,6 +13,8 @@
 #ifndef _MV88E6XXX_HWTSTAMP_H
 #define _MV88E6XXX_HWTSTAMP_H
 
+#include <linux/ptp_classify.h>
+
 #include "chip.h"
 
 /* Global 6352 PTP registers */
@@ -64,10 +66,21 @@
 
 /* Offset 0x02: PTP Configuration 2 */
 #define MV88E6XXX_PORT_PTP_CFG2				0x02
-#define MV88E6XXX_PORT_PTP_CFG2_EMBED_ARRIVAL		0x1000
 #define MV88E6XXX_PORT_PTP_CFG2_DEP_IRQ_EN		0x0002
 #define MV88E6XXX_PORT_PTP_CFG2_ARR_IRQ_EN		0x0001
 
+/* Arrival Time Stamp Mode (ArrTSMode), CFG2 bits [15:8]: configures how the
+ * switch embeds the arrival time stamp (PTPArr0Time) into enabled PTP event
+ * frames. A non-zero value is the byte offset, from the start of the PTP common
+ * header, at which the switch overwrites four bytes in place; we always point
+ * it at the header's reserved2 field so the frame length is left alone.
+ */
+#define MV88E6XXX_PTP_ARR_TS_OFFSET			\
+	offsetof(struct ptp_header, reserved2)
+#define MV88E6XXX_PORT_PTP_CFG2_ARR_TS_DISABLED		0x0000
+#define MV88E6XXX_PORT_PTP_CFG2_ARR_TS_RESERVED2	\
+	(MV88E6XXX_PTP_ARR_TS_OFFSET << 8)
+
 /* Offset 0x03: PTP LED Configuration */
 #define MV88E6XXX_PORT_PTP_LED_CFG	0x03
 
@@ -126,6 +139,7 @@ int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
 
 long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp);
 int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip);
+int mv88e6xxx_hwtstamp_setup_arr_ts(struct mv88e6xxx_chip *chip);
 void mv88e6xxx_hwtstamp_free(struct mv88e6xxx_chip *chip);
 int mv88e6352_hwtstamp_port_enable(struct mv88e6xxx_chip *chip, int port);
 int mv88e6352_hwtstamp_port_disable(struct mv88e6xxx_chip *chip, int port);
@@ -172,6 +186,11 @@ static inline int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip)
 	return 0;
 }
 
+static inline int mv88e6xxx_hwtstamp_setup_arr_ts(struct mv88e6xxx_chip *chip)
+{
+	return 0;
+}
+
 static inline void mv88e6xxx_hwtstamp_free(struct mv88e6xxx_chip *chip)
 {
 }
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8c16ef23cc102..2dfcc549de296 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -59,6 +59,7 @@ struct tc_action;
 #define DSA_TAG_PROTO_MXL_GSW1XX_VALUE		31
 #define DSA_TAG_PROTO_MXL862_VALUE		32
 #define DSA_TAG_PROTO_NETC_VALUE		33
+#define DSA_TAG_PROTO_EDSA_PTP_RESERVED2_TS_VALUE	34
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
@@ -95,6 +96,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_MXL_GSW1XX	= DSA_TAG_PROTO_MXL_GSW1XX_VALUE,
 	DSA_TAG_PROTO_MXL862		= DSA_TAG_PROTO_MXL862_VALUE,
 	DSA_TAG_PROTO_NETC		= DSA_TAG_PROTO_NETC_VALUE,
+	DSA_TAG_PROTO_EDSA_PTP_RESERVED2_TS = DSA_TAG_PROTO_EDSA_PTP_RESERVED2_TS_VALUE,
 };
 
 struct dsa_switch;
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index d5ffee35fbb53..7490ecc04aa57 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -402,6 +402,23 @@ static const struct dsa_device_ops edsa_netdev_ops = {
 
 DSA_TAG_DRIVER(edsa_netdev_ops);
 MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_EDSA, EDSA_NAME);
+
+#define EDSA_PTP_RESERVED2_TS_NAME	"edsa-ptp-reserved2-ts"
+
+/* Same wire format as EDSA, except that the switch overwrites the reserved2
+ * field of the PTP common header with the frame's arrival time stamp.
+ */
+static const struct dsa_device_ops edsa_ptp_reserved2_ts_netdev_ops = {
+	.name		 = EDSA_PTP_RESERVED2_TS_NAME,
+	.proto		 = DSA_TAG_PROTO_EDSA_PTP_RESERVED2_TS,
+	.xmit		 = edsa_xmit,
+	.rcv		 = edsa_rcv,
+	.needed_headroom = EDSA_HLEN,
+};
+
+DSA_TAG_DRIVER(edsa_ptp_reserved2_ts_netdev_ops);
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_EDSA_PTP_RESERVED2_TS,
+			    EDSA_PTP_RESERVED2_TS_NAME);
 #endif	/* CONFIG_NET_DSA_TAG_EDSA */
 
 static struct dsa_tag_driver *dsa_tag_drivers[] = {
@@ -410,6 +427,7 @@ static struct dsa_tag_driver *dsa_tag_drivers[] = {
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_TAG_EDSA)
 	&DSA_TAG_DRIVER_NAME(edsa_netdev_ops),
+	&DSA_TAG_DRIVER_NAME(edsa_ptp_reserved2_ts_netdev_ops),
 #endif
 };
 

-- 
2.43.0

Re: [PATCH net-next v4 5/5] net: dsa: mv88e6xxx: embedded PTP timestamp support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Hardware configuration failures in `mv88e6xxx_set_hwtstamp_config()` leak `chip->enable_count`, leaving the software state permanently out of sync.
- [Low] Lockless access to `chip->tag_protocol` without `READ_ONCE()` / `WRITE_ONCE()` annotations causes a data race.

Pre-existing issues:
- [High] Dereferencing PTP header pointers on nonlinear SKBs without first ensuring the header is within the linear data buffer causes out-of-bounds reads.
--

--- Patch [5]: [PATCH net-next v4 5/5] net: dsa: mv88e6xxx: embedded PTP timestamp support ---
commit a448ec89fabbabe65af4afa19ebc67d3c379213b
Author: Luke Howard <lukeh@padl.com>

net: dsa: mv88e6xxx: embedded PTP timestamp support

mv88e6xxx switches can embed the PTP arrival timestamp directly in the
frame, at a configurable offset from the PTP common header, typically
the offset of the reserved2 field.

[ ... ]

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index 7272b7dc4955c..c75a71464b20a 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
[ ... ]
> @@ -818,6 +825,14 @@ static inline bool mv88e6xxx_has_lag(struct mv88e6xxx_chip *chip)
>  	return !!chip->info->global2_addr;
>  }
>  
> +/* Whether the active tagging protocol has the switch embed arrival time stamps
> + * in the frame instead of leaving them in the PTP arrival registers.
> + */
> +static inline bool mv88e6xxx_ptp_embedded_ts(struct mv88e6xxx_chip *chip)
> +{
> +	return chip->tag_protocol == DSA_TAG_PROTO_EDSA_PTP_RESERVED2_TS;

[Severity: Low]
Is it possible for chip->tag_protocol to be modified concurrently by
mv88e6xxx_change_tag_protocol() in process context?

If this runs in the fast path rx softirq without READ_ONCE() and
WRITE_ONCE() annotations, could this cause a data race?

> +}
> +
>  static inline bool mv88e6xxx_has_tcam(struct mv88e6xxx_chip *chip)
>  {
>  	return !!chip->info->tcam_addr;
[ ... ]
> diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> index a5b362a999e64..74eaadeeeb784 100644
> --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
[ ... ]
> @@ -147,20 +149,30 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port,
>  	if (tstamp_enable) {
>  		chip->enable_count += 1;
>  		if (chip->enable_count == 1 && ptp_ops->global_enable)
> -			ptp_ops->global_enable(chip);
> -		if (ptp_ops->port_enable)
> -			ptp_ops->port_enable(chip, port);
> +			err = ptp_ops->global_enable(chip);
> +		if (!err && ptp_ops->port_enable)
> +			err = ptp_ops->port_enable(chip, port);
>  	} else {
>  		if (ptp_ops->port_disable)
> -			ptp_ops->port_disable(chip, port);
> +			err = ptp_ops->port_disable(chip, port);
>  		chip->enable_count -= 1;
>  		if (chip->enable_count == 0 && ptp_ops->global_disable)
>  			ptp_ops->global_disable(chip);
>  	}
>  	mv88e6xxx_reg_unlock(chip);
>  
> +	if (err)
> +		return err;

[Severity: High]
If ptp_ops->global_enable() or ptp_ops->port_enable() fails and returns an
error here, does this code leak the increment to chip->enable_count?

If the count remains incremented on failure, could this leave the hardware
state permanently out of sync and break the ability to enable or disable PTP?

> +
>  	/* Once hardware has been configured, enable timestamp checks
>  	 * in the RX/TX paths.
[ ... ]
> @@ -249,6 +261,52 @@ static int seq_match(struct sk_buff *skb, u16 ts_seqid)
>  	return ts_seqid == ntohs(hdr->sequence_id);

[Severity: High]
This is a pre-existing issue, but can this dereference of hdr->sequence_id
result in an out-of-bounds read?

If an skb containing a PTP packet is received fragmented such that the PTP
header sits in the paged data area, ptp_parse_header() returns a pointer
extending past the linear region.

Should we call pskb_may_pull() or skb_linearize() to ensure the header is in
linear memory before dereferencing it here and in is_pdelay_msg()?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-mv88e6xxx-ptp-fixes-v4-0-10b4020155fc@padl.com?part=5