Re: [PATCH net-next v3 08/10] net: dsa: microchip: add KSZ8463 tail tag handling

Hi,

On 7/31/26 4:23 AM, Jakub Kicinski wrote:

I don't have any strong opinion on this.

quoted

  
  	tagger_data = ksz_tagger_data(ds);

[ ... include/net/dsa.h hunks snipped ... ]

quoted

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index f8b40437c5fa..b4d70ba930dc 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c

[ ... ]

quoted

@@ -396,6 +397,70 @@ static const struct dsa_device_ops ksz9893_netdev_ops = {
  DSA_TAG_DRIVER(ksz9893_netdev_ops);
  MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893, KSZ9893_NAME);
  
+#define KSZ8463_TAIL_TAG_PRIO		GENMASK(4, 3)
+#define KSZ8463_TAIL_TAG_EG_PORT_M	GENMASK(2, 0)
+
+static struct sk_buff *ksz8463_xmit(struct sk_buff *skb,
+				    struct net_device *dev)
+{
+	u16 queue_mapping = skb_get_queue_mapping(skb);
+	u8 prio = netdev_txq_to_tc(dev, queue_mapping);
+
+	return ksz_common_xmit(skb, dev, false,
+			       FIELD_PREP(KSZ8463_TAIL_TAG_PRIO, prio),
+			       0);
+}

[Severity: Medium]
Is the loss of the tail tag override bit intentional here?

Yes it is, it doesn't exist on KSZ8463.

Until this patch KSZ8463 used ksz9893_xmit(), which passes
KSZ9893_TAIL_TAG_OVERRIDE (BIT(5)) as the override_mask argument, and
ksz8795_xmit() likewise passes KSZ8795_TAIL_TAG_OVERRIDE.  ksz8463_xmit()
passes 0, which makes this branch of ksz_common_xmit() dead for the new
tagger:

	if (is_link_local_ether_addr(hdr->h_dest))
		*tag |= override_mask;

Does that mean CPU-injected link-local frames (STP BPDUs, peer delay
multicast, LLDP) are now subject to normal port state filtering, so a port
in BLOCKING or LISTENING state drops them?

KSZ8463_TAIL_TAG_PRIO is GENMASK(4, 3), i.e. the same layout as the KSZ9893
tail tag, which suggests the override bit does exist at BIT(5) on this part.
The KSZ8463 tail tag layout is not documented anywhere in the driver, so if
BIT(5) is in fact reserved on this chip, could the change (and the now
constant override_mask argument) get an explicit note?

[Severity: Medium]
The do_tstamp = false argument means ksz_xmit_timestamp() never runs for
KSZ8463, and that is the only consumer of
KSZ_SKB_CB(skb)->update_correction:

net/dsa/tag_ksz.c:ksz_xmit_timestamp() {
	...
	/* Set correction field to 0 and update UDP checksum */
	ptp_header_update_correction(skb, ptp_type, ptp_hdr, 0);
	...
}

ksz_port_txtstamp() in drivers/net/dsa/microchip/ksz_ptp.c still sets that
field and returns without cloning, expecting the tagger to do the work:

	case PTP_MSGTYPE_PDELAY_RESP:
		if (prt->tstamp_config.tx_type == HWTSTAMP_TX_ONESTEP_P2P) {
			KSZ_SKB_CB(skb)->ptp_type = type;
			KSZ_SKB_CB(skb)->update_correction = true;
			return;
		}

ksz_get_ts_info() also keeps advertising the mode for this chip:

	ts->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ONESTEP_P2P);

	if (is_lan937x(dev))
		ts->tx_types |= BIT(HWTSTAMP_TX_ON);

So user space can select HWTSTAMP_TX_ONESTEP_P2P on a KSZ8463 port while the
tag driver performs no correction field handling at all, and
ksz_port_rxtstamp() still subtracts the ingress timestamp from the
PDelay_Req correction in that mode.  Is the correction field expected to be
handled entirely in hardware here?  If so, could the commit message say so,
since the xmit side behaviour change is not mentioned?

This series only adds support for two step PTP, that's why 
ksz_xmit_timestamp() isn't called by the KSZ8463 code path.

quoted

+
+static struct sk_buff *ksz8463_rcv(struct sk_buff *skb, struct net_device *dev)
+{
+	unsigned int len = KSZ_EGRESS_TAG_LEN;
+	struct ptp_header *ptp_hdr;
+	unsigned int ptp_class;
+	unsigned int port;
+	ktime_t ts;
+	u8 *tag;
+
+	if (skb_linearize(skb)) {
+		kfree_skb(skb);
+		return NULL;
+	}
+
+	KSZ_SKB_CB(skb)->tstamp = 0;
+
+	/* Tag decoding */
+	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
+	port = tag[0] & KSZ8463_TAIL_TAG_EG_PORT_M;
+
+	__skb_push(skb, ETH_HLEN);
+	ptp_class = ptp_classify_raw(skb);
+	__skb_pull(skb, ETH_HLEN);

[Severity: Low]
This isn't a bug, but the DSA core runs the same classifier a few
instructions later and hands the result to the driver:

net/dsa/tag.c:dsa_skb_defer_rx_timestamp() {
	...
	__skb_push(skb, ETH_HLEN);
	type = ptp_classify_raw(skb);
	__skb_pull(skb, ETH_HLEN);
	if (type == PTP_CLASS_NONE)
		return false;
	return ds->ops->port_rxtstamp(ds, p->dp->index, skb, type);
}

and ksz_port_rxtstamp() already calls ptp_parse_header() itself.  So every
received frame on a KSZ8463 conduit now runs the PTP BPF classifier in the
tagger, even with hardware timestamping disabled, and PTP frames are
classified twice.

Unlike KSZ9477/KSZ9893, where the timestamp sits in the tail tag and has to
be consumed before ksz_common_rcv() trims it, ptp_hdr->reserved2 survives
into ksz_port_rxtstamp() untouched.  Could the reserved2 extraction live in
ksz_port_rxtstamp() instead, where the PTP class is already available?

That would mean adding ksz8463-specific handling in the common 
ksz_port_rxtstamp() or create a ksz8463_port_rxtstamp() with once again 
its load of code duplication.

quoted

+	if (ptp_class == PTP_CLASS_NONE)
+		goto common_rcv;
+
+	ptp_hdr = ptp_parse_header(skb, ptp_class);

[Severity: Medium]
Only PTP_CLASS_NONE is filtered here, but the code below interprets the
frame with the PTPv2 struct ptp_header layout.  ptp_classify_raw() also
returns PTP_CLASS_V1_IPV4/PTP_CLASS_V1_IPV6, as documented in
include/linux/ptp_classify.h:

  * PTP_CLASS_V1_IPV{4,6}, PTP_CLASS_V2_IPV{4,6} or
  * PTP_CLASS_V2_{L2,VLAN}, depending on the packet content.

and the classifier program simply masks the version nibble, so any value
1..15 comes back ORed with the encapsulation bits:

net/core/ptp_classifier.c:
	{ 0x48, 0, 0, 0x0016 },	/* ldh [22] */
	{ 0x54, 0, 0, 0x000f },	/* and #0xf */
	{ 0x44, 0, 0, 0x0010 },	/* or #0x10 */

ptp_parse_header() does no version filtering either, it only skips
VLAN/IP/UDP headers and bounds-checks sizeof(struct ptp_header).  For a
PTPv1 frame, offsets 8..11 are inside the 16-byte subdomain field rather
than a reserved field, so real header content would be read as a timestamp
and then zeroed.  Should there be a "if (!(ptp_class & PTP_CLASS_V2))" gate
before the reserved2 access?

ksz8463_set_hwtstamp_config() in PATCH 10 refuses PTP V1 so we 
explicitly tell userspace that V1 isn't supported.

quoted

+	if (ptp_hdr) {
+		ts = ksz_decode_tstamp(get_unaligned_be32(&ptp_hdr->reserved2));
+		KSZ_SKB_CB(skb)->tstamp = ts;
+		ptp_hdr->reserved2 = 0;
+	}

[Severity: Medium]
Is there anything that keeps this from running while the switch PTP unit is
disabled?  The tagger never looks at KSZ_HWTS_EN here, and does not even
fetch ksz_tagger_private(), while the transmit side in the same file does
gate on it:

net/dsa/tag_ksz.c:ksz_xmit_timestamp() {
	...
	if (!test_bit(KSZ_HWTS_EN, &priv->state))
		return;
	...
}

That bit is set together with PTP_ENABLE in ksz_ptp_enable_mode():

	tagger_data->hwtstamp_set_state(dev->ds, tag_en);

	return ksz_rmw16(dev, regs[PTP_MSG_CONF1], PTP_ENABLE,
			 tag_en ? PTP_ENABLE : 0);

so "PTP disabled" is the default state, in which the hardware never wrote
anything into reserved2.  In that state, does this code destroy 4 bytes of a
genuine remote frame (messageTypeSpecific in IEEE 1588-2019) and build
KSZ_SKB_CB(skb)->tstamp out of remote bytes?  ksz_port_rxtstamp() then
publishes it with no prt->hwts_rx_en test:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_port_rxtstamp() {
	...
	tstamp = KSZ_SKB_CB(skb)->tstamp;
	memset(hwtstamps, 0, sizeof(*hwtstamps));
	hwtstamps->hwtstamp = ksz_tstamp_reconstruct(dev, tstamp);
	...
}

ksz9477_rcv() only trusts the timestamp when the hardware sets
KSZ9477_PTP_TAG_INDICATION, and the commit message says KSZ8463 provides no
such indication.  For comparison, hellcreek gates on the enable bit before
touching reserved2, in hellcreek_should_tstamp():

	if (!test_bit(HELLCREEK_HWTSTAMP_ENABLED, &ps->state))
		return NULL;

Would the same gating be appropriate here?

This is only done when we receive PTP frames because otherwise ptp_hdr 
is NULL. I think it's not a big deal to zero-out the reserved area of 
PTP frames we received even if the PTP is disabled, this area is very 
likely already set to 0 by the peer. Then ksz_port_rxtstamp indeed 
reconstructs the timestamp but it also does it for the other switches, 
not only for the KSZ8463.

[Severity: Medium]
Does the "ptp_hdr->reserved2 = 0" store need checksum bookkeeping?  For PTP
over UDP (advertised by ksz_get_ts_info() through
HWTSTAMP_FILTER_PTP_V2_L4_EVENT) these 4 bytes are covered by the UDP
checksum, but neither the checksum field nor skb->csum / skb->ip_summed are
updated.

On a conduit reporting CHECKSUM_COMPLETE, skb->csum then still describes the
unmodified bytes and is carried forward by ksz_common_rcv():

	if (pskb_trim_rcsum(skb, skb->len - len)) {

In HWTSTAMP_TX_ONESTEP_P2P mode, ksz_port_rxtstamp() additionally calls
ptp_header_update_correction() on the same frame, which does:

include/linux/ptp_classify.h:ptp_header_update_correction() {
	...
	uhdr->check = csum_fold(ptp_check_diff8(correction_old, hdr->correction,
						~csum_unfold(uhdr->check)));
	...
	skb->ip_summed = CHECKSUM_NONE;
}

so the stack recomputes the UDP checksum over data whose reserved2 delta was
never accounted for.  If the switch updates the UDP checksum when it inserts
the timestamp, would PDelay_Req frames then be dropped in udp_rcv()?  The
same file already uses ptp_header_update_correction() from
ksz_xmit_timestamp() for in-place PTP header edits.

This series adds support for two-step PTP on the L2 layer so the IP 
checksums won't be involved in the transactions.


Best regards,
Bastien