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

[Bastien Curutchet]

Hi Simon,

On 7/23/26 12:12 PM, Bastien Curutchet wrote:
> Hi Simon,
>
> On 7/20/26 4:07 PM, Simon Horman 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.
> > https://sashiko.dev/#/patchset/20260713-ksz-new-ptp- 
> > v2-1-7209c05058ef@xxxxxxxxxxx?part=8
> > ---
> > --- Patch [8]: [PATCH net-next v2 08/10] net: dsa: microchip: add 
> > KSZ8463 tail tag handling ---
> > commit bc95662a9488a7827d30f83890008a818f0a5228
> > Author: Bastien Curutchet (Schneider Electric) 
> > <bastien.curutchet@xxxxxxxxxxx>
> >
> > net: dsa: microchip: add KSZ8463 tail tag handling
> >
> > KSZ8463 uses the KSZ9893 DSA TAG driver, but uses reserved fields in 
> > the PTP
> > header for timestamps instead of the tail tag. This commit adds a
> > KSZ8463-specific DSA_TAG driver to handle KSZ8463 timestamps.
> >
> > > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> > > index f8b40437c5fa4..477f2997c1a34 100644
> > > --- a/net/dsa/tag_ksz.c
> > > +++ b/net/dsa/tag_ksz.c
> >
> > [ ... ]
> >
> > > +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;
> > > +    }
> > > +
> > > +    /* 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);
> > > +    if (ptp_class == PTP_CLASS_NONE)
> > > +        goto common_rcv;
> > > +
> > > +    ptp_hdr = ptp_parse_header(skb, ptp_class);
> > > +    if (ptp_hdr) {
> > > +        ts = ksz_decode_tstamp(get_unaligned_be32(&ptp_hdr- 
> > > >reserved2));
> > > +        KSZ_SKB_CB(skb)->tstamp = ts;
> > > +        ptp_hdr->reserved2 = 0;
> > > +        skb->ip_summed = CHECKSUM_NONE;
> >
> > [Severity: High]
> > Does this unconditionally modify the payload of all PTP packets?
> >
> > If a PTPv1 packet is received, ptp_classify_raw() identifies it as 
> > PTP, but
> > the PTPv1 header structure is different. Blindly casting and zeroing
> > ptp_hdr->reserved2 aliases bytes 16-19, which corrupts the subdomain 
> > string.
> > Should this check that ptp_class includes PTP_CLASS_V2?
>
> I don't find in the code a different PTP header for the PTP_CLASS_V1. So 
> my guess is that the reserved2 field is also reserved for V1, so setting 
> it to zero shouldn't matter IMO.
>
> > Additionally, altering the UDP payload without updating the UDP 
> > checksum and
> > setting skb->ip_summed = CHECKSUM_NONE forces the network stack to 
> > verify the
> > now-invalid checksum, which causes the packet to be dropped.
>
> I think I misunderstood Sashiko comment on the previous series and 
> shouldn't have added the skb->ip_summed = CHECKSUM_NONE line in this one.
>
> The switch uses the reserved2 field to convey the frame timestamp. 
> Datasheet doesn't say whether or not it updates the UDP checksum when 
> doing so. What I'm sure of is that packets aren't dropped as the PTP 
> synchronization is successful, so I think the UDP checksum isn't touched 
> by the switch.
> I'm not sure to understand how the skb->ip_summed field is supposed to 
> be used so any guideline here would be welcome.

I took another look at it. skb->ip_summed is only used by the IP layer, 
this series only add support for the L2 layer so I'll simply remove this 
'skb->ip_summed = CHECKSUM_NONE' in the next iteration.


Best regards,
Bastien