Re: [PATCH net-next v11 1/9] net: dsa: add tag driver for LAN9645X

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

This is my reply to:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a%40microchip.com?part=1

> --- Patch [1]: [PATCH net-next v11 1/9] net: dsa: add tag driver for LAN9645X ---
> commit 16799f4195f9118fdca9d3ba803a3d4d2e6a9414
> Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
>
> net: dsa: add tag driver for LAN9645X
>
> This adds a DSA tag driver for the LAN9645x family used with a front port
> as CPU port (NPI mode), with a long prefix on extraction and no prefix on
> injection.  A new header exports the 28 byte internal frame header (IFH)
> field table, and the tagger encodes/decodes it in lan9645x_xmit() and
> lan9645x_rcv().
>
> A few questions below.
>
> > diff --git a/include/linux/dsa/lan9645x.h b/include/linux/dsa/lan9645x.h
> > new file mode 100644
> > index 0000000000000..9896fab649c4d
> > --- /dev/null
> > +++ b/include/linux/dsa/lan9645x.h
> > @@ -0,0 +1,144 @@
>
> [ ... ]
>
> > +#define LAN9645X_IFH_TAG_TYPE_C	0
> > +#define LAN9645X_IFH_TAG_TYPE_S	1
> > +#define LAN9645X_IFH_LEN_U32		7
> > +#define LAN9645X_IFH_LEN		(LAN9645X_IFH_LEN_U32 * sizeof(u32))
> > +#define LAN9645X_IFH_BITS		(LAN9645X_IFH_LEN * BITS_PER_BYTE)
> > +#define LAN9645X_SHORT_PREFIX_LEN	4
> > +#define LAN9645X_LONG_PREFIX_LEN	16
> > +#define LAN9645X_TOTAL_TAG_LEN (LAN9645X_LONG_PREFIX_LEN + LAN9645X_IFH_LEN)
> > +
> > +#define IFH_INJ_TIMESTAMP		192
> > +#define IFH_BYPASS			191
> > +#define IFH_MASQ			190
> > +#define IFH_TIMESTAMP			186
> > +#define IFH_TIMESTAMP_NS		194
> > +#define IFH_TIMESTAMP_SUBNS		186
> > +#define IFH_MASQ_PORT			186
> > +#define IFH_RCT_INJ			185
> > +#define IFH_LEN				171
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Can this IFH_LEN definition clash with the existing ones?  There are already
> two in-tree macros with that name and a different meaning:
>
> drivers/net/ethernet/microchip/lan966x/lan966x_ifh.h:
>     #define IFH_LEN                      7
>     #define IFH_LEN_BYTES                (IFH_LEN * sizeof(u32))
>
> drivers/net/ethernet/microchip/sparx5/sparx5_main.h:
>     #define IFH_LEN                9 /* 36 bytes */
>
> Both are u32 word counts for the IFH of sibling chips in the same family,
> while here IFH_LEN is the bit position 171 of the length field.  A
> translation unit that includes this header plus either of those gets a
> redefinition warning, or silently expands 7 or 9 where bit 171 was meant.
>
> The same header also exports LAN9645X_IFH_LEN (28 bytes), so two macros
> differing only by prefix carry unrelated quantities.  The length/format and
> tag type macros already got the LAN9645X_ prefix - would it make sense to
> prefix the whole field table exported from include/linux/dsa/ as well?
>

I will rename LAN9645X_IFH_LEN to LAN9645X_IFH_LEN_BYTES and move the field
table to tag_lan9645x.c.

> > +#define IFH_WRDMODE			169
> > +#define IFH_RTAGD			167
> > +#define IFH_CUTTHRU			166
>
> [ ... ]
>
> > +#define IFH_RTE_INB_UPDATE		148
> > +#define IFH_POP_CNT			146
> > +#define IFH_ETYPE_OFS			144
> > +#define IFH_SRCPORT			140
>
> [ ... ]
>
> > +#define IFH_INJ_TIMESTAMP_SZ		32
> > +#define IFH_BYPASS_SZ			1
> > +#define IFH_MASQ_SZ			1
> > +#define IFH_TIMESTAMP_SZ		38
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Is IFH_TIMESTAMP usable with the accessors added by this patch?  Both
> lan9645x_ifh_set() and lan9645x_ifh_get() carry:
>
>     BUILD_BUG_ON_MSG(length > 32, "IFH field size wider than 32.");
>
> and pass the value through u32, so a consumer calling
>
>     lan9645x_ifh_get(ifh, IFH_TIMESTAMP, IFH_TIMESTAMP_SZ)
>
> fails to build.  The usable split already exists as IFH_TIMESTAMP_NS (30
> bits) plus IFH_TIMESTAMP_SUBNS (8 bits), so should the 38 bit alias be
> dropped or annotated?
>
> Related, the table mixes the injection and extraction layouts without
> marking which is which: IFH_TIMESTAMP, IFH_TIMESTAMP_SUBNS and
> IFH_MASQ_PORT all start at bit 186, and IFH_INJ_TIMESTAMP (32 bits at 192)
> overlaps IFH_BYPASS and IFH_MASQ.  Only the .c file mentions that "There
> are two field layouts, one for extraction and one for injection".  Could
> the header note the direction per field?

I will remove IFH_TIMESTAMP and just keep the NS/SUBNS subfield split, and
annotate each field with whether it is used in the injection, extraction or both
headers.

>
> > +#define IFH_TIMESTAMP_NS_SZ		30
> > +#define IFH_TIMESTAMP_SUBNS_SZ		8
>
> [ ... ]
>
> > diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> > new file mode 100644
> > index 0000000000000..6a33216c7792f
> > --- /dev/null
> > +++ b/net/dsa/tag_lan9645x.c
> > @@ -0,0 +1,317 @@
>
> [ ... ]
>
> > +static struct sk_buff *lan9645x_xmit(struct sk_buff *skb,
> > +				     struct net_device *ndev)
> > +{
>
> [ ... ]
>
> > +	lan9645x_ifh_set(ifh, 1, IFH_BYPASS, IFH_BYPASS_SZ);
> > +	lan9645x_ifh_set(ifh, ds->num_ports, IFH_SRCPORT, IFH_SRCPORT_SZ);
> > +	lan9645x_ifh_set(ifh, tag_type, IFH_TAG_TYPE, IFH_TAG_TYPE_SZ);
> > +	lan9645x_ifh_set(ifh, vlan_tci, IFH_TCI, IFH_TCI_SZ);
> > +	lan9645x_ifh_set(ifh, qos_class, IFH_QOS_CLASS, IFH_QOS_CLASS_SZ);
> > +	lan9645x_ifh_set(ifh, BIT(dp->index), IFH_DSTS, IFH_DSTS_SZ);
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> This isn't a bug today, but should the destination set use
> dsa_xmit_port_mask() instead of open-coding BIT(dp->index)?
>
> net/dsa/tag.h:dsa_xmit_port_mask() is BIT(dp->index) plus the HSR
> duplication fan-out:
>
>     unsigned long mask = BIT(dp->index);
>
>     if (IS_ENABLED(CONFIG_HSR) &&
>         unlikely(dev->features & NETIF_F_HW_HSR_DUP)) {
>             struct net_device *hsr_dev = dp->hsr_dev;
>             struct dsa_port *other_dp;
>
>             dsa_hsr_foreach_port(other_dp, dp->ds, hsr_dev)
>                     mask |= BIT(other_dp->index);
>     }
>
> tag_ocelot.c writes the same IFH DSTS concept through the helper:
>
>     ocelot_ifh_set_dest(injection, dsa_xmit_port_mask(skb, netdev));
>
> and net/dsa/dsa.c documents the contract as "Helpers for switches without
> specific HSR offloads, but which can implement NETIF_F_HW_HSR_DUP because
> their tagger uses dsa_xmit_port_mask()".  I checked the companion switch
> driver in this series and it implements no HSR offload, so there is no
> functional effect right now; a later HSR/PRP offload would however
> duplicate to a single port only, since the partner bit never reaches IFH
> DSTS.
>

I think this would be a bug on this chip, for HSR/PRP v1. When multiple
ports are set in the IFH_DSTS mask, each port receives a byte-identical copy. But
HSR/PRP v1 require the 2 duplicates differ in a bit (lan_id) in the tag which
tells from which port the frame egresses. Typically 0 for port A and 1 for
port B.

This chip is also capable of offloading several HSR and PRP features such as
duplication and duplicate discard. When I implement the callbacks I plan on
using those capabilities.

> > +
> > +	return skb;
> > +}
> > +
> > +static struct sk_buff *lan9645x_rcv(struct sk_buff *skb,
> > +				    struct net_device *ndev)
> > +{
>
> [ ... ]
>
> > +	popcnt = lan9645x_ifh_get(ifh, IFH_POP_CNT, IFH_POP_CNT_SZ);
> > +	etype_ofs = lan9645x_ifh_get(ifh, IFH_ETYPE_OFS, IFH_ETYPE_OFS_SZ);
> > +	src_port = lan9645x_ifh_get(ifh, IFH_SRCPORT, IFH_SRCPORT_SZ);
>
> [ ... ]
>
> > +	if (popcnt == 0 && etype_ofs == 0)
> > +		ifh_gap_len = 2 * VLAN_HLEN;
> > +	else if (popcnt == 3)
> > +		ifh_gap_len = VLAN_HLEN;
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Which extraction encodings does this cover?  IFH_POP_CNT and IFH_ETYPE_OFS
> are 2 bits each, so there are 16 combinations, and only two of them get a
> non-zero gap here.  Everything else, including popcnt == 1, popcnt == 2 and
> (popcnt == 0, etype_ofs == 1), falls through to the initialiser
> ifh_gap_len = 0.
>
> The mapping also looks internally inconsistent: on one port an untagged
> frame (popcnt == 0, etype_ofs == 0) gets an 8 byte gap, while a
> single-tagged frame on the same port (popcnt == 0, etype_ofs == 1) gets a 0
> byte gap.  That is an 8 byte difference for one 4 byte tag.  The two
> handled values do differ by exactly one tag, which suggests the real
> discriminator is the number of tags left in the delivered frame
> (ETYPE_OFS) rather than the two POP_CNT comparisons.  Could the full
> datasheet mapping be spelled out here?
>
> The companion switch driver added later in this series programs a pop count
> of 1 on every VLAN-aware port:
>
> drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c:
>         val |= ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) |
>                ANA_VLAN_CFG_VLAN_POP_CNT_SET(1);
>
> so POP_CNT == 1 seems to be an expected extraction encoding that lands in
> the zero-gap default.
>

No the current logic covers all cases properly. The values encoded in the fields
are not intuitive. On a vlan aware port ifh.pop_cnt will not be 1 for instance.

This is how the calculation goes:

DMAC_DT = (ifh.pop_cnt == 0 && ifh.etype_ofs == 0) ? 4 : ifh.pop_cnt
DMAC_OFFSET = TAG_SIZE + 4*(DMAC_DT - 2)

With tag pushing disabled we have either

popcnt=0 and etype_ofs=0     => 2x pop
popcnt=3 and etype_ofs=*     => 1x pop
popcnt=2 and etype_ofs=*     => no pop

The remaining combinations indicate a push and will not occur.

I will include this in a comment, and add WARN_ON_ONCE and a bailout for the
impossible combinations.

> > +
> > +	skb_pull(skb, LAN9645X_IFH_LEN);
> > +
> > +	if (unlikely(!pskb_may_pull(skb, ifh_gap_len + ETH_HLEN))) {
> > +		kfree_skb(skb);
> > +		return NULL;
> > +	}
> > +
> > +	skb_pull(skb, ifh_gap_len);
> > +	skb_reset_mac_header(skb);
> > +	skb_set_network_header(skb, ETH_HLEN);
> > +	skb_reset_mac_len(skb);
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Following on from the gap computation above: if ifh_gap_len is smaller than
> the real gap, do these calls describe a header the hardware did not put
> there, so that eth_type_trans() in dsa_switch_rcv() derives a protocol from
> leftover tag bytes?  And if it is larger, are up to 8 bytes of the real
> DMAC pushed back into the headroom and lost?
>
> > +
> > +	/* Reset skb->data past the actual ethernet header. */
> > +	skb_pull(skb, ETH_HLEN);
>
> [ ... ]
>
> > +	skb_postpull_rcsum(skb,
> > +			   skb->data - LAN9645X_TOTAL_TAG_LEN - ifh_gap_len,
> > +			   LAN9645X_TOTAL_TAG_LEN + ifh_gap_len);
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> The same ifh_gap_len is reused as the checksum fixup length here.  Can a
> wrong gap corrupt skb->csum on a CHECKSUM_COMPLETE conduit, since the
> subtracted byte range then does not match the bytes the conduit actually
> summed?
>
> > +
> > +	skb->dev = dsa_conduit_find_user(ndev, 0, src_port);
> > +	if (WARN_ON_ONCE(!skb->dev)) {
> > +		/* This should never happen since we have disabled reflection
> > +		 * back to the CPU.
> > +		 */
> > +		kfree_skb(skb);
> > +		return NULL;
> > +	}
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Is this WARN_ON_ONCE() reachable from frame data?  src_port comes straight
> out of the received IFH:
>
>     src_port = lan9645x_ifh_get(ifh, IFH_SRCPORT, IFH_SRCPORT_SZ);
>
> That is a 4 bit field, so any value 0..15 can arrive, and
> dsa_conduit_find_user() returns NULL for every index that is not a
> registered user port.  Candidates for such an index are the NPI port index
> itself (registered as DSA_PORT_TYPE_CPU), physical ports left unused by the
> device tree, and masqueraded source ports from a frame injected on the bare
> conduit, which the hardware parses as a bare IFH because no injection
> prefix is configured.  With panic_on_warn=1 that becomes a panic from the
> receive path.
>
> The comment relies on reflection being disabled, but the guard in the
> companion driver only covers src_port equal to the CPU port module:
>
> drivers/net/dsa/microchip/lan9645x/lan9645x_npi.c:
>     /* Make sure frames with src_port=<CPU port module> are not reflected
>      * back via the NPI port. ... The *_CPU_KILL_ENA flags above only have an
>      * effect when a frame is output due to a CPU forwarding decision such as
>      * trapping or cpu copy.
>      */
>     lan_rmw(0, BIT(port), lan9645x,
>             ANA_PGID(PGID_SRC + lan9645x->num_phys_ports));
>
> The tagger for the same hardware family deliberately drops silently here:
>
> net/dsa/tag_ocelot.c:ocelot_rcv()
>     skb->dev = dsa_conduit_find_user(netdev, 0, src_port);
>     if (!skb->dev) {
>             /* The switch will reflect back some frames sent through
>              * sockets opened on the bare DSA conduit. ... So don't print any
>              * error message here (ignore and drop those frames).
>              */
>             kfree_skb(skb);
>             return NULL;
>     }
>
> The kfree_skb() and return NULL here are already right - would dropping the
> WARN_ON_ONCE() be preferable?
>

I think it is right that a carefully constructed frame on a raw socket for the
conduit, could be reflected. I will remove the WARN_ON_ONCE().

> [ ... ]