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

[Jakub Kicinski <kuba@kernel.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.
---
--- 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?

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

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?

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

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

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

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

[ ... remaining tag_ksz.c hunks snipped ... ]