Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

On Wed, Nov 10, 2021 at 02:47:19PM +0100, Kurt Kanzenbach wrote:

On Wed Nov 10 2021, Vladimir Oltean wrote:

quoted

Hi Kurt,

On Wed, Nov 10, 2021 at 08:14:32AM +0100, Kurt Kanzenbach wrote:

quoted

Hi Vladimir,

On Tue Nov 09 2021, Vladimir Oltean wrote:

quoted

quoted

+void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
+{
+	struct b53_device *dev = ds->priv;
+	struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
+	struct sk_buff *clone;
+	unsigned int type;
+
+	type = ptp_classify_raw(skb);
+
+	if (type != PTP_CLASS_V2_L2)
+		return;
+
+	if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
+		return;
+
+	clone = skb_clone_sk(skb);
+	if (!clone)
+		return;
+
+	if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {

Is it ok if you simply don't timestamp a second skb which may be sent
while the first one is in flight, I wonder? What PTP profiles have you
tested with? At just one PTP packet at a time, the switch isn't giving
you a lot.

PTP only generates a couple of messages per second which need to be
timestamped. Therefore, this behavior shouldn't be a problem.

hellcreek (and mv88e6xxx) do the same thing, simply because the device
can only hold only one Tx timestamp. If we'd allow more than one PTP
packet in flight, there will be correlation problems. I've tested with
default and gPTP profile without any problems. What PTP profiles do have
in mind?

First of all, let's separate "more than one packet in flight" at the
hardware/driver level vs user space level. Even if there is any hardware
requirement to not request TX timestamping for the 2nd frame until the
1st has been acked, that shouldn't necessarily have an implication upon
what user space sees. After all, we don't tell user space anything about
the realities of the hardware it's running on.

Fair enough.

quoted

So it is true that ptp4l is single threaded and always polls
synchronously for the reception of a TX timestamp on the error queue
before proceeding to do anything else. But writing a kernel driver to
the specification of a single user space program is questionable.
Especially with the SOF_TIMESTAMPING_OPT_ID flag of the SO_TIMESTAMPING
socket option, it is quite possible to write a different PTP stack that
handles TX timestamps differently. It sends event messages on their
respective timer expiry (sync, peer delay request, whatever), and
processes TX timestamps as they come, asynchronously instead of blocking.
That other PTP stack would not work reliably with this driver (or with
mv88e6xxx, or with hellcreek).

Yeah, a PTP stack which e.g. runs delay measurements independently from
the other tasks (sync, announce, ...) may run into trouble with such as
an implementation. I'm wondering how would you solve that problem for
hardware such as hellcreek? If a packet for timestamping is "in-flight"
the Tx path for a concurrent frame would have to be delayed. That might
be a surprise to the user as well.

Yes, of course it would have to be delayed. But it would at least see
the timestamp, on the other hand...

Christian Eggers, while working on the mythical ksz9477 PTP support, had
the same kind of hardware to work with, and he unconditionally deferred
PTP packets, then in a kthread owned by the driver (not the tagger), he
initialized a completion structure, sent the packet, slept until the
completion was done (signaled by an irq thread in his case, can be
signaled by the tagger in Martin's case), and proceeded with the next
packet. See ksz9477_defer_xmit() here:
https://patchwork.ozlabs.org/project/netdev/patch/20201203102117.8995-9-ceggers@arri.de/

It might look like I'm trying really hard to sell deferred xmit here,
and that's not the case - I really hate it myself. But when putting two
and two together, it really seems to be what is needed.