Hi Christian,
On Mon, Oct 18, 2021 at 02:14:12PM +0200, Christian Eggers wrote:
> Hi Vladimir,
>
> On Monday, 18 October 2021, 13:39:42 CEST, Vladimir Oltean wrote:
> > There's something that just doesn't compute for me.
> > In those patches, Christian wrote:
> >
> > /* Currently, only P2P delay measurement is supported. Setting ocmode
> > * to slave will work independently of actually being master or slave.
> > * For E2E delay measurement, switching between master and slave would
> > * be required, as the KSZ devices filters out PTP messages depending on
> > * the ocmode setting:
> > * - in slave mode, DelayReq messages are filtered out
> > * - in master mode, Sync messages are filtered out
> > * Currently (and probably also in future) there is no interface in the
> > * kernel which allows switching between master and slave mode. For
> > * this reason, E2E cannot be supported. See patchwork for full
> > * discussion:
> > *
> > https://patchwork.ozlabs.org/project/netdev/patch/20201019172435.4416-8-cegg...@arri.de/
> > */
> > ksz9477_ptp_tcmode_set(dev, KSZ9477_PTP_TCMODE_P2P);
> > ksz9477_ptp_ocmode_set(dev, KSZ9477_PTP_OCMODE_SLAVE);
> >
> > Did you modify the driver's OCMODE? I am super confused as to which
> > packets ptp4l is actually waiting for a TX timestamp for. Because if
> > you're using E2E and not P2P, then the entire ksz9477_port_deferred_xmit()
> > is just dead code, is it not?
>
> I attached the patch series which I originally provided to Brian. This series
> is for linux-5.10.x. The backports folder contains patches which are already
> present in 5.11 and later kernels (some of them are even in the latest
> 5.10-stable). For recent kernels, the ksz9563_ptp folder is sufficient.
>
> Compared with the latest series I sent to netdev, I added
> 0010-net-dsa-microchip-ksz9477-add-E2E-support.patch for E2E support. This was
> rejected by the ptp4l maintainer as it would require ptp4l to dynamically
> switch the KSZ hardware between master and slave mode (there are packet
> filters
> in hardware which cannot entirely be disabled).
>
> Currently there is not much demand for PTP in our current product development.
> But if the KSZ work can be finished / mainlined, I am highly interested. My
> latest status was (IIRC):
>
> 1. There is something wrong with the time stamping offsets. As a result, 1PPS
> works nearly perfect with two KSZ devices, but shows a constant offset when
> using a Meinberg clock as master.
>
> 2. Currently the user is responsible for providing a start time (for PPS) that
> is in the future. In case the time point has already elapsed, the driver will
> report an error.
>
> 3. Occasional timeouts when waiting for TX timestamps. If think that I already
> implemented the driver changes you requested, but probably the problem still
> persist. It is even possible that the (Brian's) KSZ9567 suffers from hardware
> bugs here which are not present on (my) KSZ9563 (I think the guy from
> Microchip
> mentioned this).
>
> 4. Sometimes the 1PPS becomes completely out of sync (but recovers then
> later).
> This is surprising for me, as ptp4l uses filters/regulators and should not be
> affected by single packet / timestamp failures.
>
> Is there anything I can help?
Thanks for the clarification, it makes more sense now since the picture
is more complete.
I've no idea about the hardware specifics since I don't own the
hardware. But I noticed a logical issue with might be relevant to the TX
timestamp timeout events that Brian is seeing.
Brian, can you try out the patch below? Compile-tested only, as
mentioned I can't really do much more.
-----------------------------[ cut here ]-----------------------------
>From 7f4771bc19db9b48577f3f45ba907e5c13aea808 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.olt...@nxp.com>
Date: Mon, 18 Oct 2021 16:35:18 +0300
Subject: [PATCH] net: dsa: ksz9477: use a kthread work item per deferred skb
There might be a race in tag_ksz.c between these two lines:
skb_queue_tail(&ptp_shared->xmit_queue, skb_get(skb));
kthread_queue_work(ptp_shared->xmit_worker, &ptp_shared->xmit_work);
and the skb dequeue logic in ksz9477_port_deferred_xmit(). For example,
the xmit_work might be already queued, however the work item has just
finished walking through the skb queue. Because we don't check the
return code from kthread_queue_work, we don't do anything if the work
item is already queued.
However, nobody will take that skb and send it, at least until the next
timestampable skb is sent.
With the ksz9477 driver, two-step TX timestamping is a rare process, and
in certain configs it may happen even as rarely as once per second.
So if the race condition described above happens, we might experience
huge delays.
To close that race, let's not keep a single work item per port, and a
skb timestamping queue, but rather dynamically allocate a work item per
packet.
It is also unnecessary to have more than one kthread that does the work.
So delete the per-port kthread allocation and replace them with a single
kthread which is global to the switch.
Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
---
drivers/net/dsa/microchip/ksz9477_ptp.c | 88 ++++++++++++++-----------
drivers/net/dsa/microchip/ksz_common.h | 1 -
include/linux/dsa/ksz_common.h | 11 ++--
net/dsa/tag_ksz.c | 31 +++++----
4 files changed, 73 insertions(+), 58 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c
b/drivers/net/dsa/microchip/ksz9477_ptp.c
index c646689cb71e..0f05aafbdd3d 100644
--- a/drivers/net/dsa/microchip/ksz9477_ptp.c
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.c
@@ -749,42 +749,62 @@ static void ksz9477_ptp_txtstamp_skb(struct ksz_device
*dev,
skb_complete_tx_timestamp(skb, &hwtstamps);
}
-#define work_to_port(work) \
- container_of((work), struct ksz_port_ptp_shared, xmit_work)
-#define ptp_shared_to_ksz_port(t) \
- container_of((t), struct ksz_port, ptp_shared)
-#define ptp_shared_to_ksz_device(t) \
- container_of((t), struct ksz_device, ptp_shared)
+#define work_to_xmit_work(w) \
+ container_of((w), struct ksz_deferred_xmit_work, work)
/* Deferred work is necessary for time stamped PDelay_Req messages. This cannot
* be done from atomic context as we have to wait for the hardware interrupt.
*/
static void ksz9477_port_deferred_xmit(struct kthread_work *work)
{
- struct ksz_port_ptp_shared *prt_ptp_shared = work_to_port(work);
- struct ksz_port *prt = ptp_shared_to_ksz_port(prt_ptp_shared);
- struct ksz_device_ptp_shared *ptp_shared = prt_ptp_shared->dev;
- struct ksz_device *dev = ptp_shared_to_ksz_device(ptp_shared);
- int port = prt - dev->ports;
- struct sk_buff *skb;
+ struct ksz_deferred_xmit_work *xmit_work = work_to_xmit_work(work);
+ struct dsa_switch *ds = xmit_work->dp->ds;
+ struct sk_buff *skb = xmit_work->skb;
+ struct dsa_port *dp = xmit_work->dp;
+ struct ksz_device *dev = ds->priv;
+ struct ksz_port *prt = dp->priv;
+
+ reinit_completion(&prt->tstamp_completion);
- while ((skb = skb_dequeue(&prt_ptp_shared->xmit_queue)) != NULL) {
- struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+ /* Transfer skb to the host port. */
+ dsa_enqueue_skb(skb, dp->slave);
- reinit_completion(&prt->tstamp_completion);
+ ksz9477_ptp_txtstamp_skb(dev, prt, DSA_SKB_CB(skb)->clone);
+ kfree(xmit_work);
+}
- /* Transfer skb to the host port. */
- dsa_enqueue_skb(skb, dsa_to_port(dev->ds, port)->slave);
+static int ksz9477_ptp_shared_init(struct ksz_device *dev)
+{
+ struct ksz_device_ptp_shared *ptp_shared = &dev->ptp_shared;
+ int ret;
- ksz9477_ptp_txtstamp_skb(dev, prt, clone);
+ /* PDelay_Req messages require deferred transmit as the time
+ * stamp unit provides no sequenceId or similar. So we must
+ * wait for the time stamp interrupt.
+ */
+ ptp_shared->xmit_work_fn = ksz9477_port_deferred_xmit;
+ ptp_shared->xmit_worker = kthread_create_worker(0, "ksz_xmit");
+ if (IS_ERR(ptp_shared->xmit_worker)) {
+ ret = PTR_ERR(ptp_shared->xmit_worker);
+ dev_err(dev->dev,
+ "failed to create deferred xmit thread: %d\n", ret);
+ return ret;
}
+
+ return 0;
+}
+
+static void ksz9477_ptp_shared_deinit(struct ksz_device *dev)
+{
+ struct ksz_device_ptp_shared *ptp_shared = &dev->ptp_shared;
+
+ kthread_destroy_worker(ptp_shared->xmit_worker);
}
static int ksz9477_ptp_port_init(struct ksz_device *dev, int port)
{
- struct ksz_port *prt = &dev->ports[port];
- struct ksz_port_ptp_shared *ptp_shared = &prt->ptp_shared;
struct dsa_port *dp = dsa_to_port(dev->ds, port);
+ struct ksz_port *prt = &dev->ports[port];
int ret;
if (port == dev->cpu_port)
@@ -809,31 +829,15 @@ static int ksz9477_ptp_port_init(struct ksz_device *dev,
int port)
if (ret)
goto error_disable_port_ptp_interrupts;
- /* ksz_port::ptp_shared is used in tagging driver */
- ptp_shared->dev = &dev->ptp_shared;
- dp->priv = ptp_shared;
-
/* PDelay_Req messages require deferred transmit as the time
* stamp unit provides no sequenceId or similar. So we must
* wait for the time stamp interrupt.
*/
+ dp->priv = &dev->ptp_shared;
init_completion(&prt->tstamp_completion);
- kthread_init_work(&ptp_shared->xmit_work,
- ksz9477_port_deferred_xmit);
- ptp_shared->xmit_worker = kthread_create_worker(0, "%s_xmit",
- dp->slave->name);
- if (IS_ERR(ptp_shared->xmit_worker)) {
- ret = PTR_ERR(ptp_shared->xmit_worker);
- dev_err(dev->dev,
- "failed to create deferred xmit thread: %d\n", ret);
- goto error_disable_port_egress_interrupts;
- }
- skb_queue_head_init(&ptp_shared->xmit_queue);
return 0;
-error_disable_port_egress_interrupts:
- ksz9477_ptp_enable_port_egress_interrupts(dev, port, false);
error_disable_port_ptp_interrupts:
ksz9477_ptp_enable_port_ptp_interrupts(dev, port, false);
return ret;
@@ -841,12 +845,12 @@ static int ksz9477_ptp_port_init(struct ksz_device *dev,
int port)
static void ksz9477_ptp_port_deinit(struct ksz_device *dev, int port)
{
- struct ksz_port_ptp_shared *ptp_shared = &dev->ports[port].ptp_shared;
+ struct dsa_port *dp = dsa_to_port(dev->ds, port);
if (port == dev->cpu_port)
return;
- kthread_destroy_worker(ptp_shared->xmit_worker);
+ dp->priv = NULL;
ksz9477_ptp_enable_port_egress_interrupts(dev, port, false);
ksz9477_ptp_enable_port_ptp_interrupts(dev, port, false);
}
@@ -856,6 +860,10 @@ static int ksz9477_ptp_ports_init(struct ksz_device *dev)
int port;
int ret;
+ ret = ksz9477_ptp_shared_init(dev);
+ if (ret)
+ return ret;
+
for (port = 0; port < dev->port_cnt; port++) {
ret = ksz9477_ptp_port_init(dev, port);
if (ret)
@@ -867,6 +875,7 @@ static int ksz9477_ptp_ports_init(struct ksz_device *dev)
error_deinit:
while (port-- > 0)
ksz9477_ptp_port_deinit(dev, port);
+ ksz9477_ptp_shared_deinit(dev);
return ret;
}
@@ -876,6 +885,7 @@ static void ksz9477_ptp_ports_deinit(struct ksz_device *dev)
for (port = 0; port < dev->port_cnt; port++)
ksz9477_ptp_port_deinit(dev, port);
+ ksz9477_ptp_shared_deinit(dev);
}
/* device attributes */
diff --git a/drivers/net/dsa/microchip/ksz_common.h
b/drivers/net/dsa/microchip/ksz_common.h
index c9495c92a32d..abcbcbb3fcef 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -45,7 +45,6 @@ struct ksz_port {
struct ksz_port_mib mib;
phy_interface_t interface;
#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
- struct ksz_port_ptp_shared ptp_shared;
ktime_t tstamp_xdelay;
struct completion tstamp_completion;
bool hwts_tx_en;
diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
index a9b4720cc842..c75bc27e3e7a 100644
--- a/include/linux/dsa/ksz_common.h
+++ b/include/linux/dsa/ksz_common.h
@@ -35,13 +35,14 @@ struct ksz_device_ptp_shared {
/* approximated current time, read once per second from hardware */
struct timespec64 ptp_clock_time;
unsigned long state;
+ void (*xmit_work_fn)(struct kthread_work *work);
+ struct kthread_worker *xmit_worker;
};
-struct ksz_port_ptp_shared {
- struct ksz_device_ptp_shared *dev;
- struct kthread_worker *xmit_worker;
- struct kthread_work xmit_work;
- struct sk_buff_head xmit_queue;
+struct ksz_deferred_xmit_work {
+ struct dsa_port *dp;
+ struct sk_buff *skb;
+ struct kthread_work work;
};
/* net/dsa/tag_ksz.c */
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 415a26044565..548f66888b0a 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -175,11 +175,12 @@ static void ksz9477_xmit_timestamp(struct sk_buff *skb)
static struct sk_buff *ksz9477_defer_xmit(struct dsa_port *dp,
struct sk_buff *skb)
{
- struct ksz_port_ptp_shared *ptp_shared = dp->priv;
+ struct ksz_device_ptp_shared *ptp_shared = dp->priv;
struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+ struct ksz_deferred_xmit_work *xmit_work;
u8 ptp_msg_type;
- if (!clone)
+ if (!clone || !ptp_shared)
return skb; /* no deferred xmit for this packet */
/* Use cached PTP msg type from ksz9477_ptp_port_txtstamp(). */
@@ -188,11 +189,18 @@ static struct sk_buff *ksz9477_defer_xmit(struct dsa_port
*dp,
ptp_msg_type != PTP_MSGTYPE_PDELAY_REQ)
goto out_free_clone; /* only PDelay_Req is deferred */
+ xmit_work = kzalloc(sizeof(*xmit_work), GFP_ATOMIC);
+ if (!xmit_work)
+ return NULL;
+
+ kthread_init_work(&xmit_work->work, ptp_shared->xmit_work_fn);
/* Increase refcount so the kfree_skb in dsa_slave_xmit
* won't really free the packet.
*/
- skb_queue_tail(&ptp_shared->xmit_queue, skb_get(skb));
- kthread_queue_work(ptp_shared->xmit_worker, &ptp_shared->xmit_work);
+ xmit_work->dp = dp;
+ xmit_work->skb = skb_get(skb);
+
+ kthread_queue_work(ptp_shared->xmit_worker, &xmit_work->work);
return NULL;
@@ -232,7 +240,7 @@ static void ksz9477_rcv_timestamp(struct sk_buff *skb, u8
*tag,
{
struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
struct dsa_switch *ds = dev->dsa_ptr->ds;
- struct ksz_port_ptp_shared *port_ptp_shared;
+ struct ksz_device_ptp_shared *ptp_shared;
u8 *tstamp_raw = tag - KSZ9477_PTP_TAG_LEN;
struct ptp_header *ptp_hdr;
unsigned int ptp_type;
@@ -240,15 +248,14 @@ static void ksz9477_rcv_timestamp(struct sk_buff *skb, u8
*tag,
ktime_t tstamp;
s64 correction;
- port_ptp_shared = dsa_to_port(ds, port)->priv;
- if (!port_ptp_shared)
+ ptp_shared = dsa_to_port(ds, port)->priv;
+ if (!ptp_shared)
return;
/* convert time stamp and write to skb */
tstamp = ksz9477_decode_tstamp(get_unaligned_be32(tstamp_raw));
memset(hwtstamps, 0, sizeof(*hwtstamps));
- hwtstamps->hwtstamp = ksz9477_tstamp_reconstruct(port_ptp_shared->dev,
- tstamp);
+ hwtstamps->hwtstamp = ksz9477_tstamp_reconstruct(ptp_shared, tstamp);
/* For PDelay_Req messages, user space (ptp4l) expects that the hardware
* subtracts the ingress time stamp from the correction field. The
@@ -289,8 +296,7 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
struct net_device *dev)
{
struct dsa_port *dp = dsa_slave_to_port(dev);
- struct ksz_port_ptp_shared *port_ptp_shared = dp->priv;
- struct ksz_device_ptp_shared *ptp_shared = port_ptp_shared->dev;
+ struct ksz_device_ptp_shared *ptp_shared = dp->priv;
__be16 *tag;
u8 *addr;
u16 val;
@@ -347,8 +353,7 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
struct net_device *dev)
{
struct dsa_port *dp = dsa_slave_to_port(dev);
- struct ksz_port_ptp_shared *port_ptp_shared = dp->priv;
- struct ksz_device_ptp_shared *ptp_shared = port_ptp_shared->dev;
+ struct ksz_device_ptp_shared *ptp_shared = dp->priv;
u8 *addr;
u8 *tag;
-----------------------------[ cut here ]-----------------------------
_______________________________________________
Linuxptp-users mailing list
Linuxptp-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-users
