by Arun Ramadoss

[permalink] [raw]

Subject: Re: [Patch net-next v7 06/13] net: ptp: add helper for one-step P2P clocks

Hi Paolo,
Thanks for the review comment.
On Thu, 2023-01-05 at 11:49 +0100, Paolo Abeni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
>
> > > +/**
> > > + * ptp_header_update_correction - Update PTP header's correction
> > > field
> > > + * @skb: packet buffer
> > > + * @type: type of the packet (see ptp_classify_raw())
> > > + * @hdr: ptp header
> > > + * @correction: new correction value
> > > + *
> > > + * This updates the correction field of a PTP header and updates
> > > the UDP
> > > + * checksum (if UDP is used as transport). It is needed for
> > > hardware capable of
> > > + * one-step P2P that does not already modify the correction
> > > field of Pdelay_Req
> > > + * event messages on ingress.
> > > + */
> > > +static inline
> > > +void ptp_header_update_correction(struct sk_buff *skb, unsigned
> > > int type,
> > > + struct ptp_header *hdr, s64
> > > correction)
> > > +{
> > > + __be64 correction_old;
> > > + struct udphdr *uhdr;
> > > +
> > > + /* previous correction value is required for checksum update.
> > > */
> > > + memcpy(&correction_old, &hdr->correction,
> > > sizeof(correction_old));
> > > +
> > > + /* write new correction value */
> > > + put_unaligned_be64((u64)correction, &hdr->correction);
> > > +
> > > + switch (type & PTP_CLASS_PMASK) {
> > > + case PTP_CLASS_IPV4:
> > > + case PTP_CLASS_IPV6:
> > > + /* locate udp header */
> > > + uhdr = (struct udphdr *)((char *)hdr - sizeof(struct
> > > udphdr));
> > > + break;
> > > + default:
> > > + return;
> > > + }
> > > +
> > > + /* update checksum */
> > > + uhdr->check = csum_fold(ptp_check_diff8(correction_old,
> > > + hdr->correction,
> > > + ~csum_unfold(uhdr-
> > > >check)));
> > > + if (!uhdr->check)
> > > + uhdr->check = CSUM_MANGLED_0;
> >
> > AFAICS the above works under the assumption that skb->ip_summed !=
> > CHECKSUM_COMPLETE, and such assumption is true for the existing DSA
> > devices.
> >
> > Still the new helper is a generic one, so perhaps it should take
> > care
> > of CHECKSUM_COMPLETE, too? Or at least add a big fat warning in the
> > helper documentation and/or a warn_on_once(CHECKSUM_COMPLETE).
>
> I see this helper is used later even in the tx path, so even packet
> with ip_summed == CHECKSUM_PARTIAL could reach here and should be
> accomodated accordingly.

Do I need to update the checksum only if ip_sum is not equal to
CHECKSUM_COMPLETE or CHECKSUM_PARTIAL.

if ( skb->ip_summed == CHECKSUM_COMPLETE ||
skb->ip_summed == CHECKSUM_PARTIAL) {
warn_on_once(1);
return;
}

Kindly suggest.

>
> Thanks,
>
> Paolo
>
>

2023-01-09 05:02:23

by Arun Ramadoss

[permalink] [raw]

Subject: Re: [Patch net-next v7 06/13] net: ptp: add helper for one-step P2P clocks

Hi Eric,
On Thu, 2023-01-05 at 12:27 +0100, Eric Dumazet wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Thu, Jan 5, 2023 at 11:09 AM Paolo Abeni <[email protected]>
> wrote:
> >
> >
> > > +static inline
> > > +void ptp_header_update_correction(struct sk_buff *skb, unsigned
> > > int type,
> > > + struct ptp_header *hdr, s64
> > > correction)
> > > +{
> > > + __be64 correction_old;
> > > + struct udphdr *uhdr;
> > > +
> > > + /* previous correction value is required for checksum
> > > update. */
> > > + memcpy(&correction_old, &hdr->correction,
> > > sizeof(correction_old));
> > > +
> > > + /* write new correction value */
> > > + put_unaligned_be64((u64)correction, &hdr->correction);
> > > +
> > > + switch (type & PTP_CLASS_PMASK) {
> > > + case PTP_CLASS_IPV4:
> > > + case PTP_CLASS_IPV6:
> > > + /* locate udp header */
> > > + uhdr = (struct udphdr *)((char *)hdr -
> > > sizeof(struct udphdr));
> > > + break;
> > > + default:
> > > + return;
> > > + }
> > > +
> > > + /* update checksum */
> > > + uhdr->check = csum_fold(ptp_check_diff8(correction_old,
> > > + hdr->correction,
> > > + ~csum_unfold(uhdr-
> > > >check)));
> > > + if (!uhdr->check)
> > > + uhdr->check = CSUM_MANGLED_0;
> >
> > AFAICS the above works under the assumption that skb->ip_summed !=
> > CHECKSUM_COMPLETE, and such assumption is true for the existing DSA
> > devices.
>
> Presumably skb->ip_summed could be forced to CHECKSUM_NONE
>
> Note: if IPV4 UDP checksum is zero, we are not supposed to change it.
>
> (Not sure if this point is already checked in caller)

This function is called only for the Pdelay_Req/Resp packet processing
from the hardware where correction field is updated & checksum is
recomputed.
As per the recommendation, Can I set the skb->ip_summed = CHECKSUM_NONE
in the function after recomputing the checksum and resubmit the patch.
Kindly suggest.

>
> >
> > Still the new helper is a generic one, so perhaps it should take
> > care
> > of CHECKSUM_COMPLETE, too? Or at least add a big fat warning in the
> > helper documentation and/or a warn_on_once(CHECKSUM_COMPLETE).
> >
> > Thanks!
> >
> > Paolo
> >