Messages in this thread | | | From | <> | Subject | Re: [Patch net-next v7 06/13] net: ptp: add helper for one-step P2P clocks | Date | Thu, 5 Jan 2023 15:48:55 +0000 |
| |
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 > >
| |