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

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

Hi,

On 7/31/26 4:23 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> --- Patch [8]: [PATCH net-next v3 08/10] net: dsa: microchip: add KSZ8463 tail tag handling ---
> commit 20a43ca1297a3640a458d9df2837f23e6b9191ea
> Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
> 
> net: dsa: microchip: add KSZ8463 tail tag handling
> 
> This adds a KSZ8463-specific DSA tag driver so that RX timestamps can be
> taken from the reserved2 field of the PTP header instead of from the tail
> tag, and switches ksz8463_get_tag_protocol() over to the new
> DSA_TAG_PROTO_KSZ8463.  A few questions below about the new receive and
> transmit paths.
> 
>> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
>> index 5e5bfc5cae2d..ac9e8ef5774a 100644
>> --- a/drivers/net/dsa/microchip/ksz8.c
>> +++ b/drivers/net/dsa/microchip/ksz8.c
>> @@ -2966,7 +2966,7 @@ static enum dsa_tag_protocol ksz8463_get_tag_protocol(struct dsa_switch *ds,
>>   						      int port,
>>   						      enum dsa_tag_protocol mp)
>>   {
>> -	return DSA_TAG_PROTO_KSZ9893;
>> +	return DSA_TAG_PROTO_KSZ8463;
>>   }
>>   
>>   static int ksz8463_connect_tag_protocol(struct dsa_switch *ds,
>> @@ -2974,7 +2974,7 @@ static int ksz8463_connect_tag_protocol(struct dsa_switch *ds,
>>   {
>>   	struct ksz_tagger_data *tagger_data;
>>   
>> -	if (proto != DSA_TAG_PROTO_KSZ9893)
>> +	if (proto != DSA_TAG_PROTO_KSZ8463)
>>   		return -EPROTONOSUPPORT;
> 
> [Severity: Low]
> This changes the user-visible tagging protocol name for KSZ8463 boards,
> which is reachable both through the conduit's sysfs "tagging" attribute and
> through the "dsa-tag-protocol" device tree property.
> 
> A board that currently pins the previous default:
> 
> 	dsa-tag-protocol = "ksz9893";
> 
> now hits the -EPROTONOSUPPORT return in ksz8463_connect_tag_protocol()
> instead of degrading to the old behaviour.  Would it be worth mentioning
> this compatibility aspect in the commit message?
>

I don't have any strong opinion on this.

>>   
>>   	tagger_data = ksz_tagger_data(ds);
> 
> [ ... include/net/dsa.h hunks snipped ... ]
> 
>> 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
> 
> [ ... ]
> 
>> @@ -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.

>> +
>> +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.

>> +	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.

>> +	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