[PATCH v2 net-next 0/9] Improve the taprio qdisc's relationship with its children

[PATCH v2 net-next 0/9] Improve the taprio qdisc's relationship with its children

[Vladimir Oltean]

Changes in v2:
It was requested to add test cases for the taprio software and offload modes.
Those are patches 08 and 09.

That implies adding taprio offload support to netdevsim, which is patch 07.

In turn, that implies adding a PHC driver for netdevsim, which is patch 06.

v1 at:
https://lore.kernel.org/lkml/20230531173928.1942027-1-vladimir.oltean@nxp.com/

Original message:

Prompted by Vinicius' request to consolidate some child Qdisc
dereferences in taprio:
https://lore.kernel.org/netdev/87edmxv7x2.fsf@intel.com/

I remembered that I had left some unfinished work in this Qdisc, namely
commit af7b29b1deaa ("Revert "net/sched: taprio: make qdisc_leaf() see
the per-netdev-queue pfifo child qdiscs"").

This patch set represents another stab at, essentially, what's in the
title. Not only does taprio not properly detect when it's grafted as a
non-root qdisc, but it also returns incorrect per-class stats.
Eventually, Vinicius' request is addressed too, although in a different
form than the one he requested (which was purely cosmetic).

Review from people more experienced with Qdiscs than me would be
appreciated. I tried my best to explain what I consider to be problems.
I am deliberately targeting net-next because the changes are too
invasive for net - they were reverted from stable once already.

Vladimir Oltean (9):
  net/sched: taprio: don't access q->qdiscs[] in unoffloaded mode during
    attach()
  net/sched: taprio: keep child Qdisc refcount elevated at 2 in offload
    mode
  net/sched: taprio: try again to report q->qdiscs[] to qdisc_leaf()
  net/sched: taprio: delete misleading comment about preallocating child
    qdiscs
  net/sched: taprio: dump class stats for the actual q->qdiscs[]
  net: netdevsim: create a mock-up PTP Hardware Clock driver
  net: netdevsim: mimic tc-taprio offload
  selftests/tc-testing: test that taprio can only be attached as root
  selftests/tc-testing: verify that a qdisc can be grafted onto a taprio
    class

 drivers/net/Kconfig                           |   1 +
 drivers/net/netdevsim/ethtool.c               |  11 ++
 drivers/net/netdevsim/netdev.c                |  38 +++-
 drivers/net/netdevsim/netdevsim.h             |   2 +
 drivers/ptp/Kconfig                           |  11 ++
 drivers/ptp/Makefile                          |   1 +
 drivers/ptp/ptp_mock.c                        | 175 ++++++++++++++++++
 include/linux/ptp_mock.h                      |  38 ++++
 net/sched/sch_taprio.c                        |  68 ++++---
 .../tc-testing/tc-tests/qdiscs/taprio.json    |  98 ++++++++++
 10 files changed, 415 insertions(+), 28 deletions(-)
 create mode 100644 drivers/ptp/ptp_mock.c
 create mode 100644 include/linux/ptp_mock.h

-- 
2.34.1

[PATCH v2 net-next 1/9] net/sched: taprio: don't access q->qdiscs[] in unoffloaded mode during attach()

[Vladimir Oltean]

This is a simple code transformation with no intended behavior change,
just to make it absolutely clear that q->qdiscs[] is only attached to
the child taprio classes in full offload mode.

Right now we use the q->qdiscs[] variable in taprio_attach() for
software mode too, but that is quite confusing and avoidable. We use
it only to reach the netdev TX queue, but we could as well just use
netdev_get_tx_queue() for that.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 net/sched/sch_taprio.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index c6627f5abdfa..3ee8a7cca786 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -2131,14 +2131,20 @@ static void taprio_attach(struct Qdisc *sch)
 
 	/* Attach underlying qdisc */
 	for (ntx = 0; ntx < dev->num_tx_queues; ntx++) {
-		struct Qdisc *qdisc = q->qdiscs[ntx];
+		struct netdev_queue *dev_queue = netdev_get_tx_queue(dev, ntx);
 		struct Qdisc *old;
 
 		if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
+			struct Qdisc *qdisc = q->qdiscs[ntx];
+
 			qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
-			old = dev_graft_qdisc(qdisc->dev_queue, qdisc);
+			old = dev_graft_qdisc(dev_queue, qdisc);
 		} else {
-			old = dev_graft_qdisc(qdisc->dev_queue, sch);
+			/* In software mode, attach the root taprio qdisc
+			 * to all netdev TX queues, so that dev_qdisc_enqueue()
+			 * goes through taprio_enqueue().
+			 */
+			old = dev_graft_qdisc(dev_queue, sch);
 			qdisc_refcount_inc(sch);
 		}
 		if (old)
-- 
2.34.1

[PATCH v2 net-next 2/9] net/sched: taprio: keep child Qdisc refcount elevated at 2 in offload mode

[Vladimir Oltean]

Normally, Qdiscs have one reference on them held by their owner and one
held for each TXQ to which they are attached, however this is not the
case with the children of an offloaded taprio. Instead, the taprio qdisc
currently lives in the following fragile equilibrium.

In the software scheduling case, taprio attaches itself (the root Qdisc)
to all TXQs, thus having a refcount of 1 + the number of TX queues. In
this mode, the q->qdiscs[] children are not visible directly to the
Qdisc API. The lifetime of the Qdiscs from this private array lasts
until qdisc_destroy() -> taprio_destroy().

In the fully offloaded case, the root taprio has a refcount of 1, and
all child q->qdiscs[] also have a refcount of 1. The child q->qdiscs[]
are attached to the netdev TXQs directly and thus are visible to the
Qdisc API, however taprio loses a reference to them very early - during
qdisc_graft(parent==NULL) -> taprio_attach(). At that time, taprio frees
the q->qdiscs[] array to not leak memory, but interestingly, it does not
release a reference on these qdiscs because it doesn't effectively own
them - they are created by taprio but owned by the Qdisc core, and will
be freed by qdisc_graft(parent==NULL, new==NULL) -> qdisc_put(old) when
the Qdisc is deleted or when the child Qdisc is replaced with something
else.

My interest is to change this equilibrium such that taprio also owns a
reference on the q->qdiscs[] child Qdiscs for the lifetime of the root
Qdisc, including in full offload mode. I want this because I would like
taprio_leaf(), taprio_dump_class(), taprio_dump_class_stats() to have
insight into q->qdiscs[] for the software scheduling mode - currently
they look at dev_queue->qdisc_sleeping, which is, as mentioned, the same
as the root taprio.

The following set of changes is necessary:
- don't free q->qdiscs[] early in taprio_attach(), free it late in
  taprio_destroy() for consistency with software mode. But:
- currently that's not possible, because taprio doesn't own a reference
  on q->qdiscs[]. So hold that reference - once during the initial
  attach() and once during subsequent graft() calls when the child is
  changed.
- always keep track of the current child in q->qdiscs[], even for full
  offload mode, so that we free in taprio_destroy() what we should, and
  not something stale.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2:
- fix refcount not dropping to 0 after a graft operation - spotted by
  Paolo
- slightly reword commit message and comments

 net/sched/sch_taprio.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 3ee8a7cca786..b5f533914415 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -2132,30 +2132,31 @@ static void taprio_attach(struct Qdisc *sch)
 	/* Attach underlying qdisc */
 	for (ntx = 0; ntx < dev->num_tx_queues; ntx++) {
 		struct netdev_queue *dev_queue = netdev_get_tx_queue(dev, ntx);
-		struct Qdisc *old;
+		struct Qdisc *old, *dev_queue_qdisc;
 
 		if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
 			struct Qdisc *qdisc = q->qdiscs[ntx];
 
+			/* In offload mode, the root taprio qdisc is bypassed
+			 * and the netdev TX queues see the children directly
+			 */
 			qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
-			old = dev_graft_qdisc(dev_queue, qdisc);
+			dev_queue_qdisc = qdisc;
 		} else {
 			/* In software mode, attach the root taprio qdisc
 			 * to all netdev TX queues, so that dev_qdisc_enqueue()
 			 * goes through taprio_enqueue().
 			 */
-			old = dev_graft_qdisc(dev_queue, sch);
-			qdisc_refcount_inc(sch);
+			dev_queue_qdisc = sch;
 		}
+		old = dev_graft_qdisc(dev_queue, dev_queue_qdisc);
+		/* The qdisc's refcount requires to be elevated once
+		 * for each netdev TX queue it is grafted onto
+		 */
+		qdisc_refcount_inc(dev_queue_qdisc);
 		if (old)
 			qdisc_put(old);
 	}
-
-	/* access to the child qdiscs is not needed in offload mode */
-	if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
-		kfree(q->qdiscs);
-		q->qdiscs = NULL;
-	}
 }
 
 static struct netdev_queue *taprio_queue_get(struct Qdisc *sch,
@@ -2184,13 +2185,23 @@ static int taprio_graft(struct Qdisc *sch, unsigned long cl,
 	if (dev->flags & IFF_UP)
 		dev_deactivate(dev);
 
+	/* In offload mode, the child Qdisc is directly attached to the netdev
+	 * TX queue, and thus, we need to keep its refcount elevated in order
+	 * to counteract qdisc_graft()'s call to qdisc_put() once per TX queue.
+	 * However, save the reference to the new qdisc in the private array in
+	 * both software and offload cases, to have an up-to-date reference to
+	 * our children.
+	 */
+	*old = q->qdiscs[cl - 1];
 	if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
-		*old = dev_graft_qdisc(dev_queue, new);
-	} else {
-		*old = q->qdiscs[cl - 1];
-		q->qdiscs[cl - 1] = new;
+		WARN_ON_ONCE(dev_graft_qdisc(dev_queue, new) != *old);
+		if (new)
+			qdisc_refcount_inc(new);
+		if (*old)
+			qdisc_put(*old);
 	}
 
+	q->qdiscs[cl - 1] = new;
 	if (new)
 		new->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
 
-- 
2.34.1

[PATCH v2 net-next 3/9] net/sched: taprio: try again to report q->qdiscs[] to qdisc_leaf()

[Vladimir Oltean]

This is another stab at commit 1461d212ab27 ("net/sched: taprio: make
qdisc_leaf() see the per-netdev-queue pfifo child qdiscs"), later
reverted in commit af7b29b1deaa ("Revert "net/sched: taprio: make
qdisc_leaf() see the per-netdev-queue pfifo child qdiscs"").

I believe that the problems that caused the revert were fixed, and thus,
this change is identical to the original patch.

Its purpose is to properly reject attaching a software taprio child
qdisc to a software taprio parent. Because unoffloaded taprio currently
reports itself (the root Qdisc) as the return value from qdisc_leaf(),
then the process of attaching another taprio as child to a Qdisc class
of the root will just result in a Qdisc_ops :: change() call for the
root. Whereas that's not we want. We want Qdisc_ops :: init() to be
called for the taprio child, in order to give the taprio child a chance
to check whether its sch->parent is TC_H_ROOT or not (and reject this
configuration).

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: rebase on top of rtnl_dereference() change for txq->qdisc_sleeping

 net/sched/sch_taprio.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index b5f533914415..14d628926d61 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -2439,12 +2439,14 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 
 static struct Qdisc *taprio_leaf(struct Qdisc *sch, unsigned long cl)
 {
-	struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
+	struct taprio_sched *q = qdisc_priv(sch);
+	struct net_device *dev = qdisc_dev(sch);
+	unsigned int ntx = cl - 1;
 
-	if (!dev_queue)
+	if (ntx >= dev->num_tx_queues)
 		return NULL;
 
-	return rtnl_dereference(dev_queue->qdisc_sleeping);
+	return q->qdiscs[ntx];
 }
 
 static unsigned long taprio_find(struct Qdisc *sch, u32 classid)
-- 
2.34.1

[PATCH v2 net-next 4/9] net/sched: taprio: delete misleading comment about preallocating child qdiscs

[Vladimir Oltean]

As mentioned in commit af7b29b1deaa ("Revert "net/sched: taprio: make
qdisc_leaf() see the per-netdev-queue pfifo child qdiscs"") - unlike
mqprio, taprio doesn't use q->qdiscs[] only as a temporary transport
between Qdisc_ops :: init() and Qdisc_ops :: attach().

Delete the comment, which is just stolen from mqprio, but there, the
usage patterns are a lot different, and this is nothing but confusing.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 net/sched/sch_taprio.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 14d628926d61..c35e27d0e49e 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -2085,11 +2085,8 @@ static int taprio_init(struct Qdisc *sch, struct nlattr *opt,
 		return -EOPNOTSUPP;
 	}
 
-	/* pre-allocate qdisc, attachment can't fail */
-	q->qdiscs = kcalloc(dev->num_tx_queues,
-			    sizeof(q->qdiscs[0]),
+	q->qdiscs = kcalloc(dev->num_tx_queues, sizeof(q->qdiscs[0]),
 			    GFP_KERNEL);
-
 	if (!q->qdiscs)
 		return -ENOMEM;
 
-- 
2.34.1

[PATCH v2 net-next 5/9] net/sched: taprio: dump class stats for the actual q->qdiscs[]

[Vladimir Oltean]

This makes a difference for the software scheduling mode, where
dev_queue->qdisc_sleeping is the same as the taprio root Qdisc itself,
but when we're talking about what Qdisc and stats get reported for a
traffic class, the root taprio isn't what comes to mind, but q->qdiscs[]
is.

To understand the difference, I've attempted to send 100 packets in
software mode through class 8001:5, and recorded the stats before and
after the change.

Here is before:

$ tc -s class show dev eth0
class taprio 8001:1 root leaf 8001:
 Sent 9400 bytes 100 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:2 root leaf 8001:
 Sent 9400 bytes 100 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:3 root leaf 8001:
 Sent 9400 bytes 100 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:4 root leaf 8001:
 Sent 9400 bytes 100 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:5 root leaf 8001:
 Sent 9400 bytes 100 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:6 root leaf 8001:
 Sent 9400 bytes 100 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:7 root leaf 8001:
 Sent 9400 bytes 100 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:8 root leaf 8001:
 Sent 9400 bytes 100 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0

and here is after:

class taprio 8001:1 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:2 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:3 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:4 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:5 root
 Sent 9400 bytes 100 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:6 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:7 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0
class taprio 8001:8 root leaf 800d:
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
 window_drops 0

The most glaring (and expected) difference is that before, all class
stats reported the global stats, whereas now, they really report just
the counters for that traffic class.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2:
- reword commit message
- rebase on top of the TAPRIO_CMD_TC_STATS -> TAPRIO_CMD_QUEUE_STATS change

 net/sched/sch_taprio.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index c35e27d0e49e..86450e67be14 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -2458,11 +2458,11 @@ static unsigned long taprio_find(struct Qdisc *sch, u32 classid)
 static int taprio_dump_class(struct Qdisc *sch, unsigned long cl,
 			     struct sk_buff *skb, struct tcmsg *tcm)
 {
-	struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
+	struct Qdisc *child = taprio_leaf(sch, cl);
 
 	tcm->tcm_parent = TC_H_ROOT;
 	tcm->tcm_handle |= TC_H_MIN(cl);
-	tcm->tcm_info = rtnl_dereference(dev_queue->qdisc_sleeping)->handle;
+	tcm->tcm_info = child->handle;
 
 	return 0;
 }
@@ -2472,16 +2472,14 @@ static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
 	__releases(d->lock)
 	__acquires(d->lock)
 {
-	struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
+	struct Qdisc *child = taprio_leaf(sch, cl);
 	struct tc_taprio_qopt_offload offload = {
 		.cmd = TAPRIO_CMD_QUEUE_STATS,
 		.queue_stats = {
 			.queue = cl - 1,
 		},
 	};
-	struct Qdisc *child;
 
-	child = rtnl_dereference(dev_queue->qdisc_sleeping);
 	if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 ||
 	    qdisc_qstats_copy(d, child) < 0)
 		return -1;
-- 
2.34.1

[PATCH v2 net-next 6/9] net: netdevsim: create a mock-up PTP Hardware Clock driver

[Vladimir Oltean]

I'd like to make netdevsim offload tc-taprio, but currently, this Qdisc
emits a ETHTOOL_GET_TS_INFO call to the driver to make sure that it has
a PTP clock, so that it is reasonably capable of offloading the schedule.
Needless to say, netdevsim doesn't have a PTP clock, so that's something
to think about.

It wouldn't be that hard to create an object which emulates PTP clock
operations on top of the unadjustable CLOCK_MONOTONIC_RAW plus a
software-controlled time domain via a timecounter/cyclecounter and then
link that PHC to the netdevsim device, so this is what is done here.

The scope of the mock-up PHC driver seems to be in drivers/ptp/, since
it is in principle reusable by other virtual network devices as well,
such as veth, and those could even take packet timestamps when passing
skbs between peers (the same timestamp is given as TX timestamp to one
peer, and as RX timestamp to the other, resulting in a zero-delay link).
Nonetheless, netdevsim doesn't support packet RX/TX (and taprio doesn't
need packet timestamping), so for now, the mock-up PHC driver doesn't
support packet timestamping either.

The driver is fully functional for its intended purpose, and it
successfully passes the PTP selftests.

$ echo "1 1 8" > /sys/bus/netdevsim/new_device
$ ethtool -T eni1np1
Time stamping parameters for eni1np1:
Capabilities:
PTP Hardware Clock: 2
Hardware Transmit Timestamp Modes: none
Hardware Receive Filter Modes: none
$ cd tools/testing/selftests/ptp/
$ ./phc.sh /dev/ptp2
TEST: settime                                                       [ OK ]
TEST: adjtime                                                       [ OK ]
TEST: adjfreq                                                       [ OK ]

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 drivers/net/Kconfig               |   1 +
 drivers/net/netdevsim/ethtool.c   |  11 ++
 drivers/net/netdevsim/netdev.c    |  11 +-
 drivers/net/netdevsim/netdevsim.h |   2 +
 drivers/ptp/Kconfig               |  11 ++
 drivers/ptp/Makefile              |   1 +
 drivers/ptp/ptp_mock.c            | 175 ++++++++++++++++++++++++++++++
 include/linux/ptp_mock.h          |  38 +++++++
 8 files changed, 249 insertions(+), 1 deletion(-)
 create mode 100644 drivers/ptp/ptp_mock.c
 create mode 100644 include/linux/ptp_mock.h

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 368c6f5b327e..4953c1494723 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -592,6 +592,7 @@ config NETDEVSIM
 	depends on INET
 	depends on IPV6 || IPV6=n
 	depends on PSAMPLE || PSAMPLE=n
+	depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
 	select NET_DEVLINK
 	help
 	  This driver is a developer testing tool and software model that can
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index ffd9f84b6644..bd546d4d26c6 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -140,6 +140,16 @@ nsim_set_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
 	return 0;
 }
 
+static int nsim_get_ts_info(struct net_device *dev,
+			    struct ethtool_ts_info *info)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	info->phc_index = mock_phc_index(ns->phc);
+
+	return 0;
+}
+
 static const struct ethtool_ops nsim_ethtool_ops = {
 	.supported_coalesce_params	= ETHTOOL_COALESCE_ALL_PARAMS,
 	.get_pause_stats	        = nsim_get_pause_stats,
@@ -153,6 +163,7 @@ static const struct ethtool_ops nsim_ethtool_ops = {
 	.set_channels			= nsim_set_channels,
 	.get_fecparam			= nsim_get_fecparam,
 	.set_fecparam			= nsim_set_fecparam,
+	.get_ts_info			= nsim_get_ts_info,
 };
 
 static void nsim_ethtool_ring_init(struct netdevsim *ns)
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 35fa1ca98671..58cd51de5b79 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -291,13 +291,19 @@ static void nsim_setup(struct net_device *dev)
 
 static int nsim_init_netdevsim(struct netdevsim *ns)
 {
+	struct mock_phc *phc;
 	int err;
 
+	phc = mock_phc_create(&ns->nsim_bus_dev->dev);
+	if (IS_ERR(phc))
+		return PTR_ERR(phc);
+
+	ns->phc = phc;
 	ns->netdev->netdev_ops = &nsim_netdev_ops;
 
 	err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev);
 	if (err)
-		return err;
+		goto err_phc_destroy;
 
 	rtnl_lock();
 	err = nsim_bpf_init(ns);
@@ -318,6 +324,8 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
 err_utn_destroy:
 	rtnl_unlock();
 	nsim_udp_tunnels_info_destroy(ns->netdev);
+err_phc_destroy:
+	mock_phc_destroy(ns->phc);
 	return err;
 }
 
@@ -380,6 +388,7 @@ void nsim_destroy(struct netdevsim *ns)
 	rtnl_unlock();
 	if (nsim_dev_port_is_pf(ns->nsim_dev_port))
 		nsim_udp_tunnels_info_destroy(dev);
+	mock_phc_destroy(ns->phc);
 	free_netdev(dev);
 }
 
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 7d8ed8d8df5c..59526420c78e 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
+#include <linux/ptp_mock.h>
 #include <linux/u64_stats_sync.h>
 #include <net/devlink.h>
 #include <net/udp_tunnel.h>
@@ -73,6 +74,7 @@ struct netdevsim {
 	struct net_device *netdev;
 	struct nsim_dev *nsim_dev;
 	struct nsim_dev_port *nsim_dev_port;
+	struct mock_phc *phc;
 
 	u64 tx_packets;
 	u64 tx_bytes;
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 32dff1b4f891..ed9d97a032f1 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -155,6 +155,17 @@ config PTP_1588_CLOCK_IDTCM
 	  To compile this driver as a module, choose M here: the module
 	  will be called ptp_clockmatrix.
 
+config PTP_1588_CLOCK_MOCK
+	tristate "Mock-up PTP clock"
+	depends on PTP_1588_CLOCK
+	help
+	  This driver offers a set of PTP clock manipulation operations over
+	  the system monotonic time. It can be used by virtual network device
+	  drivers to emulate PTP capabilities.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called ptp_mock.
+
 config PTP_1588_CLOCK_VMW
 	tristate "VMware virtual PTP clock"
 	depends on ACPI && HYPERVISOR_GUEST && X86
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 553f18bf3c83..dea0cebd2303 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -16,6 +16,7 @@ ptp-qoriq-y				+= ptp_qoriq.o
 ptp-qoriq-$(CONFIG_DEBUG_FS)		+= ptp_qoriq_debugfs.o
 obj-$(CONFIG_PTP_1588_CLOCK_IDTCM)	+= ptp_clockmatrix.o
 obj-$(CONFIG_PTP_1588_CLOCK_IDT82P33)	+= ptp_idt82p33.o
+obj-$(CONFIG_PTP_1588_CLOCK_MOCK)	+= ptp_mock.o
 obj-$(CONFIG_PTP_1588_CLOCK_VMW)	+= ptp_vmw.o
 obj-$(CONFIG_PTP_1588_CLOCK_OCP)	+= ptp_ocp.o
 obj-$(CONFIG_PTP_DFL_TOD)		+= ptp_dfl_tod.o
diff --git a/drivers/ptp/ptp_mock.c b/drivers/ptp/ptp_mock.c
new file mode 100644
index 000000000000..e09e6009c4f7
--- /dev/null
+++ b/drivers/ptp/ptp_mock.c
@@ -0,0 +1,175 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2023 NXP
+ *
+ * Mock-up PTP Hardware Clock driver for virtual network devices
+ *
+ * Create a PTP clock which offers PTP time manipulation operations
+ * using a timecounter/cyclecounter on top of CLOCK_MONOTONIC_RAW.
+ */
+
+#include <linux/ptp_clock_kernel.h>
+#include <linux/ptp_mock.h>
+#include <linux/timecounter.h>
+
+/* Clamp scaled_ppm between -2,097,152,000 and 2,097,152,000,
+ * and thus "adj" between -68,719,476 and 68,719,476
+ */
+#define MOCK_PHC_MAX_ADJ_PPB		32000000
+/* Timestamps from ktime_get_raw() have 1 ns resolution, so the scale factor
+ * (MULT >> SHIFT) needs to be 1. Pick SHIFT as 31 bits, which translates
+ * MULT(freq 0) into 0x80000000.
+ */
+#define MOCK_PHC_CC_SHIFT		31
+#define MOCK_PHC_CC_MULT		(1 << MOCK_PHC_CC_SHIFT)
+#define MOCK_PHC_FADJ_SHIFT		9
+#define MOCK_PHC_FADJ_DENOMINATOR	15625ULL
+
+/* The largest cycle_delta that timecounter_read_delta() can handle without a
+ * 64-bit overflow during the multiplication with cc->mult, given the max "adj"
+ * we permit, is ~8.3 seconds. Make sure readouts are more frequent than that.
+ */
+#define MOCK_PHC_REFRESH_INTERVAL	(HZ * 5)
+
+#define info_to_phc(d) container_of((d), struct mock_phc, info)
+
+struct mock_phc {
+	struct ptp_clock_info info;
+	struct ptp_clock *clock;
+	struct timecounter tc;
+	struct cyclecounter cc;
+	spinlock_t lock;
+};
+
+static u64 mock_phc_cc_read(const struct cyclecounter *cc)
+{
+	return ktime_to_ns(ktime_get_raw());
+}
+
+static int mock_phc_adjfine(struct ptp_clock_info *info, long scaled_ppm)
+{
+	struct mock_phc *phc = info_to_phc(info);
+	s64 adj;
+
+	adj = (s64)scaled_ppm << MOCK_PHC_FADJ_SHIFT;
+	adj = div_s64(adj, MOCK_PHC_FADJ_DENOMINATOR);
+
+	spin_lock(&phc->lock);
+	timecounter_read(&phc->tc);
+	phc->cc.mult = MOCK_PHC_CC_MULT + adj;
+	spin_unlock(&phc->lock);
+
+	return 0;
+}
+
+static int mock_phc_adjtime(struct ptp_clock_info *info, s64 delta)
+{
+	struct mock_phc *phc = info_to_phc(info);
+
+	spin_lock(&phc->lock);
+	timecounter_adjtime(&phc->tc, delta);
+	spin_unlock(&phc->lock);
+
+	return 0;
+}
+
+static int mock_phc_settime64(struct ptp_clock_info *info,
+			      const struct timespec64 *ts)
+{
+	struct mock_phc *phc = info_to_phc(info);
+	u64 ns = timespec64_to_ns(ts);
+
+	spin_lock(&phc->lock);
+	timecounter_init(&phc->tc, &phc->cc, ns);
+	spin_unlock(&phc->lock);
+
+	return 0;
+}
+
+static int mock_phc_gettime64(struct ptp_clock_info *info, struct timespec64 *ts)
+{
+	struct mock_phc *phc = info_to_phc(info);
+	u64 ns;
+
+	spin_lock(&phc->lock);
+	ns = timecounter_read(&phc->tc);
+	spin_unlock(&phc->lock);
+
+	*ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
+static long mock_phc_refresh(struct ptp_clock_info *info)
+{
+	struct timespec64 ts;
+
+	mock_phc_gettime64(info, &ts);
+
+	return MOCK_PHC_REFRESH_INTERVAL;
+}
+
+int mock_phc_index(struct mock_phc *phc)
+{
+	if (!phc)
+		return -1;
+
+	return ptp_clock_index(phc->clock);
+}
+
+struct mock_phc *mock_phc_create(struct device *dev)
+{
+	struct mock_phc *phc;
+	int err;
+
+	phc = kzalloc(sizeof(*phc), GFP_KERNEL);
+	if (!phc) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	phc->info = (struct ptp_clock_info) {
+		.owner		= THIS_MODULE,
+		.name		= "Mock-up PTP clock",
+		.max_adj	= MOCK_PHC_MAX_ADJ_PPB,
+		.adjfine	= mock_phc_adjfine,
+		.adjtime	= mock_phc_adjtime,
+		.gettime64	= mock_phc_gettime64,
+		.settime64	= mock_phc_settime64,
+		.do_aux_work	= mock_phc_refresh,
+	};
+
+	phc->cc = (struct cyclecounter) {
+		.read	= mock_phc_cc_read,
+		.mask	= CYCLECOUNTER_MASK(64),
+		.mult	= MOCK_PHC_CC_MULT,
+		.shift	= MOCK_PHC_CC_SHIFT,
+	};
+
+	spin_lock_init(&phc->lock);
+	timecounter_init(&phc->tc, &phc->cc, 0);
+
+	phc->clock = ptp_clock_register(&phc->info, dev);
+	if (IS_ERR_OR_NULL(phc->clock)) {
+		err = PTR_ERR_OR_ZERO(phc->clock);
+		goto out_free_phc;
+	}
+
+	ptp_schedule_worker(phc->clock, MOCK_PHC_REFRESH_INTERVAL);
+
+	return phc;
+
+out_free_phc:
+	kfree(phc);
+out:
+	return ERR_PTR(err);
+}
+
+void mock_phc_destroy(struct mock_phc *phc)
+{
+	if (!phc)
+		return;
+
+	ptp_clock_unregister(phc->clock);
+	kfree(phc);
+}
diff --git a/include/linux/ptp_mock.h b/include/linux/ptp_mock.h
new file mode 100644
index 000000000000..72eb401034d9
--- /dev/null
+++ b/include/linux/ptp_mock.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Mock-up PTP Hardware Clock driver for virtual network devices
+ *
+ * Copyright 2023 NXP
+ */
+
+#ifndef _PTP_MOCK_H_
+#define _PTP_MOCK_H_
+
+struct device;
+struct mock_phc;
+
+#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_MOCK)
+
+struct mock_phc *mock_phc_create(struct device *dev);
+void mock_phc_destroy(struct mock_phc *phc);
+int mock_phc_index(struct mock_phc *phc);
+
+#else
+
+static inline struct mock_phc *mock_phc_create(struct device *dev)
+{
+	return NULL;
+}
+
+static inline void mock_phc_destroy(struct mock_phc *phc)
+{
+}
+
+static inline int mock_phc_index(struct mock_phc *phc)
+{
+	return -1;
+}
+
+#endif
+
+#endif /* _PTP_MOCK_H_ */
-- 
2.34.1

[PATCH v2 net-next 7/9] net: netdevsim: mimic tc-taprio offload

[Vladimir Oltean]

To be able to use netdevsim for tc-testing with an offloaded tc-taprio
schedule, it needs to report a PTP clock (which it now does), and to
accept ndo_setup_tc(TC_SETUP_QDISC_TAPRIO) calls.

Since netdevsim has no packet I/O, this doesn't do anything intelligent,
it only allows taprio offload code paths to go through some level of
automated testing.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 drivers/net/netdevsim/netdev.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 58cd51de5b79..e26be4bd0d90 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -209,6 +209,31 @@ static int nsim_set_vf_link_state(struct net_device *dev, int vf, int state)
 	return 0;
 }
 
+static void nsim_taprio_stats(struct tc_taprio_qopt_stats *stats)
+{
+	stats->window_drops = 0;
+	stats->tx_overruns = 0;
+}
+
+static int nsim_setup_tc_taprio(struct net_device *dev,
+				struct tc_taprio_qopt_offload *offload)
+{
+	int err = 0;
+
+	switch (offload->cmd) {
+	case TAPRIO_CMD_REPLACE:
+	case TAPRIO_CMD_DESTROY:
+		break;
+	case TAPRIO_CMD_STATS:
+		nsim_taprio_stats(&offload->stats);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+	}
+
+	return err;
+}
+
 static LIST_HEAD(nsim_block_cb_list);
 
 static int
@@ -217,6 +242,8 @@ nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
 	struct netdevsim *ns = netdev_priv(dev);
 
 	switch (type) {
+	case TC_SETUP_QDISC_TAPRIO:
+		return nsim_setup_tc_taprio(dev, type_data);
 	case TC_SETUP_BLOCK:
 		return flow_block_cb_setup_simple(type_data,
 						  &nsim_block_cb_list,
-- 
2.34.1

[PATCH v2 net-next 8/9] selftests/tc-testing: test that taprio can only be attached as root

[Vladimir Oltean]

Check that the "Can only be attached as root qdisc" error message from
taprio is effective by attempting to attach it to a class of another
taprio qdisc. That operation should fail.

In the bug that was squashed by change "net/sched: taprio: try again to
report q->qdiscs[] to qdisc_leaf()", grafting a child taprio to a root
software taprio would be misinterpreted as a change() to the root
taprio. Catch this by looking at whether the base-time of the root
taprio has changed to follow the base-time of the child taprio,
something which should have absolutely never happened assuming correct
semantics.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 .../tc-testing/tc-tests/qdiscs/taprio.json    | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
index a44455372646..58d4d97f4499 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
@@ -131,5 +131,53 @@
         "teardown": [
             "echo \"1\" > /sys/bus/netdevsim/del_device"
         ]
+    },
+    {
+        "id": "39b4",
+        "name": "Reject grafting taprio as child qdisc of software taprio",
+        "category": [
+            "qdisc",
+            "taprio"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "echo \"1 1 8\" > /sys/bus/netdevsim/new_device",
+            "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 0 sched-entry S ff 20000000 clockid CLOCK_TAI"
+        ],
+        "cmdUnderTest": "$TC qdisc replace dev $ETH parent 8001:7 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 200 sched-entry S ff 20000000 clockid CLOCK_TAI",
+        "expExitCode": "2",
+        "verifyCmd": "$TC -j qdisc show dev $ETH root | jq '.[].options.base_time'",
+        "matchPattern": "0",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $ETH root",
+            "echo \"1\" > /sys/bus/netdevsim/del_device"
+        ]
+    },
+    {
+        "id": "e8a1",
+        "name": "Reject grafting taprio as child qdisc of offloaded taprio",
+        "category": [
+            "qdisc",
+            "taprio"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "echo \"1 1 8\" > /sys/bus/netdevsim/new_device",
+            "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 0 sched-entry S ff 20000000 flags 0x2"
+        ],
+        "cmdUnderTest": "$TC qdisc replace dev $ETH parent 8001:7 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 200 sched-entry S ff 20000000 flags 0x2",
+        "expExitCode": "2",
+        "verifyCmd": "$TC -j qdisc show dev $ETH root | jq '.[].options.base_time'",
+        "matchPattern": "0",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $ETH root",
+            "echo \"1\" > /sys/bus/netdevsim/del_device"
+        ]
     }
 ]
-- 
2.34.1

[PATCH v2 net-next 9/9] selftests/tc-testing: verify that a qdisc can be grafted onto a taprio class

[Vladimir Oltean]

The reason behind commit af7b29b1deaa ("Revert "net/sched: taprio: make
qdisc_leaf() see the per-netdev-queue pfifo child qdiscs"") was that the
patch it reverted caused a crash when attaching a CBS shaper to one of
the taprio classes. Prevent that from happening again by adding a test
case for it, which now passes correctly in both offload and software
modes.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 .../tc-testing/tc-tests/qdiscs/taprio.json    | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
index 58d4d97f4499..47692335bcf1 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
@@ -179,5 +179,55 @@
             "$TC qdisc del dev $ETH root",
             "echo \"1\" > /sys/bus/netdevsim/del_device"
         ]
+    },
+    {
+        "id": "a7bf",
+        "name": "Graft cbs as child of software taprio",
+        "category": [
+            "qdisc",
+            "taprio",
+            "cbs"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "echo \"1 1 8\" > /sys/bus/netdevsim/new_device",
+            "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 0 sched-entry S ff 20000000 clockid CLOCK_TAI"
+        ],
+        "cmdUnderTest": "$TC qdisc replace dev $ETH handle 8002: parent 8001:8 cbs idleslope 20000 sendslope -980000 hicredit 30 locredit -1470",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -d qdisc show dev $ETH",
+        "matchPattern": "qdisc cbs 8002: parent 8001:8 hicredit 30 locredit -1470 sendslope -980000 idleslope 20000 offload 0",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $ETH root",
+            "echo \"1\" > /sys/bus/netdevsim/del_device"
+        ]
+    },
+    {
+        "id": "6a83",
+        "name": "Graft cbs as child of offloaded taprio",
+        "category": [
+            "qdisc",
+            "taprio",
+            "cbs"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "echo \"1 1 8\" > /sys/bus/netdevsim/new_device",
+            "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 0 sched-entry S ff 20000000 flags 0x2"
+        ],
+        "cmdUnderTest": "$TC qdisc replace dev $ETH handle 8002: parent 8001:8 cbs idleslope 20000 sendslope -980000 hicredit 30 locredit -1470",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -d qdisc show dev $ETH",
+        "matchPattern": "qdisc cbs 8002: parent 8001:8 hicredit 30 locredit -1470 sendslope -980000 idleslope 20000 offload 0",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $ETH root",
+            "echo \"1\" > /sys/bus/netdevsim/del_device"
+        ]
     }
 ]
-- 
2.34.1

Re: [PATCH v2 net-next 6/9] net: netdevsim: create a mock-up PTP Hardware Clock driver

[Simon Horman]

On Wed, Jun 14, 2023 at 12:54:37AM +0300, Vladimir Oltean wrote:

Hi Vladimir,

some minor feedback from my side.

...

> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 35fa1ca98671..58cd51de5b79 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -291,13 +291,19 @@ static void nsim_setup(struct net_device *dev)
>  
>  static int nsim_init_netdevsim(struct netdevsim *ns)
>  {
> +	struct mock_phc *phc;
>  	int err;
>  
> +	phc = mock_phc_create(&ns->nsim_bus_dev->dev);
> +	if (IS_ERR(phc))
> +		return PTR_ERR(phc);
> +
> +	ns->phc = phc;
>  	ns->netdev->netdev_ops = &nsim_netdev_ops;
>  
>  	err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev);
>  	if (err)
> -		return err;
> +		goto err_phc_destroy;
>  
>  	rtnl_lock();
>  	err = nsim_bpf_init(ns);

...

> diff --git a/drivers/ptp/ptp_mock.c b/drivers/ptp/ptp_mock.c
> new file mode 100644
> index 000000000000..e09e6009c4f7
> --- /dev/null
> +++ b/drivers/ptp/ptp_mock.c
> @@ -0,0 +1,175 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2023 NXP
> + *
> + * Mock-up PTP Hardware Clock driver for virtual network devices
> + *
> + * Create a PTP clock which offers PTP time manipulation operations
> + * using a timecounter/cyclecounter on top of CLOCK_MONOTONIC_RAW.
> + */
> +
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/ptp_mock.h>
> +#include <linux/timecounter.h>
> +
> +/* Clamp scaled_ppm between -2,097,152,000 and 2,097,152,000,
> + * and thus "adj" between -68,719,476 and 68,719,476
> + */
> +#define MOCK_PHC_MAX_ADJ_PPB		32000000
> +/* Timestamps from ktime_get_raw() have 1 ns resolution, so the scale factor
> + * (MULT >> SHIFT) needs to be 1. Pick SHIFT as 31 bits, which translates
> + * MULT(freq 0) into 0x80000000.
> + */
> +#define MOCK_PHC_CC_SHIFT		31
> +#define MOCK_PHC_CC_MULT		(1 << MOCK_PHC_CC_SHIFT)

Maybe BIT()?

...

> +struct mock_phc *mock_phc_create(struct device *dev)
> +{
> +	struct mock_phc *phc;
> +	int err;
> +
> +	phc = kzalloc(sizeof(*phc), GFP_KERNEL);
> +	if (!phc) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	phc->info = (struct ptp_clock_info) {
> +		.owner		= THIS_MODULE,
> +		.name		= "Mock-up PTP clock",
> +		.max_adj	= MOCK_PHC_MAX_ADJ_PPB,
> +		.adjfine	= mock_phc_adjfine,
> +		.adjtime	= mock_phc_adjtime,
> +		.gettime64	= mock_phc_gettime64,
> +		.settime64	= mock_phc_settime64,
> +		.do_aux_work	= mock_phc_refresh,
> +	};
> +
> +	phc->cc = (struct cyclecounter) {
> +		.read	= mock_phc_cc_read,
> +		.mask	= CYCLECOUNTER_MASK(64),
> +		.mult	= MOCK_PHC_CC_MULT,
> +		.shift	= MOCK_PHC_CC_SHIFT,
> +	};
> +
> +	spin_lock_init(&phc->lock);
> +	timecounter_init(&phc->tc, &phc->cc, 0);
> +
> +	phc->clock = ptp_clock_register(&phc->info, dev);
> +	if (IS_ERR_OR_NULL(phc->clock)) {
> +		err = PTR_ERR_OR_ZERO(phc->clock);
> +		goto out_free_phc;
> +	}
> +
> +	ptp_schedule_worker(phc->clock, MOCK_PHC_REFRESH_INTERVAL);
> +
> +	return phc;
> +
> +out_free_phc:
> +	kfree(phc);
> +out:
> +	return ERR_PTR(err);
> +}

Smatch complains that ERR_PTR may be passed zero.
Looking at the IS_ERR_OR_NULL block above, this does indeed seem to be the
case.

Keeping Smatch happy is one thing - your call - but I do wonder if the
caller of mock_phc_create() handles the NULL case correctly.

...

Re: [PATCH v2 net-next 8/9] selftests/tc-testing: test that taprio can only be attached as root

[Pedro Tammela]

On 13/06/2023 18:54, Vladimir Oltean wrote:
> Check that the "Can only be attached as root qdisc" error message from
> taprio is effective by attempting to attach it to a class of another
> taprio qdisc. That operation should fail.
> 
> In the bug that was squashed by change "net/sched: taprio: try again to
> report q->qdiscs[] to qdisc_leaf()", grafting a child taprio to a root
> software taprio would be misinterpreted as a change() to the root
> taprio. Catch this by looking at whether the base-time of the root
> taprio has changed to follow the base-time of the child taprio,
> something which should have absolutely never happened assuming correct
> semantics.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>

> ---
> v1->v2: patch is new
> 
>   .../tc-testing/tc-tests/qdiscs/taprio.json    | 48 +++++++++++++++++++
>   1 file changed, 48 insertions(+)
> 
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> index a44455372646..58d4d97f4499 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> @@ -131,5 +131,53 @@
>           "teardown": [
>               "echo \"1\" > /sys/bus/netdevsim/del_device"
>           ]
> +    },
> +    {
> +        "id": "39b4",
> +        "name": "Reject grafting taprio as child qdisc of software taprio",
> +        "category": [
> +            "qdisc",
> +            "taprio"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [
> +            "echo \"1 1 8\" > /sys/bus/netdevsim/new_device",
> +            "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 0 sched-entry S ff 20000000 clockid CLOCK_TAI"
> +        ],
> +        "cmdUnderTest": "$TC qdisc replace dev $ETH parent 8001:7 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 200 sched-entry S ff 20000000 clockid CLOCK_TAI",
> +        "expExitCode": "2",
> +        "verifyCmd": "$TC -j qdisc show dev $ETH root | jq '.[].options.base_time'",
> +        "matchPattern": "0",
> +        "matchCount": "1",
> +        "teardown": [
> +            "$TC qdisc del dev $ETH root",
> +            "echo \"1\" > /sys/bus/netdevsim/del_device"
> +        ]
> +    },
> +    {
> +        "id": "e8a1",
> +        "name": "Reject grafting taprio as child qdisc of offloaded taprio",
> +        "category": [
> +            "qdisc",
> +            "taprio"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [
> +            "echo \"1 1 8\" > /sys/bus/netdevsim/new_device",
> +            "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 0 sched-entry S ff 20000000 flags 0x2"
> +        ],
> +        "cmdUnderTest": "$TC qdisc replace dev $ETH parent 8001:7 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 200 sched-entry S ff 20000000 flags 0x2",
> +        "expExitCode": "2",
> +        "verifyCmd": "$TC -j qdisc show dev $ETH root | jq '.[].options.base_time'",
> +        "matchPattern": "0",
> +        "matchCount": "1",
> +        "teardown": [
> +            "$TC qdisc del dev $ETH root",
> +            "echo \"1\" > /sys/bus/netdevsim/del_device"
> +        ]
>       }
>   ]

Re: [PATCH v2 net-next 9/9] selftests/tc-testing: verify that a qdisc can be grafted onto a taprio class

[Pedro Tammela]

On 13/06/2023 18:54, Vladimir Oltean wrote:
> The reason behind commit af7b29b1deaa ("Revert "net/sched: taprio: make
> qdisc_leaf() see the per-netdev-queue pfifo child qdiscs"") was that the
> patch it reverted caused a crash when attaching a CBS shaper to one of
> the taprio classes. Prevent that from happening again by adding a test
> case for it, which now passes correctly in both offload and software
> modes.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Other than the comment below,

Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>

> ---
> v1->v2: patch is new
> 
>   .../tc-testing/tc-tests/qdiscs/taprio.json    | 50 +++++++++++++++++++
>   1 file changed, 50 insertions(+)
> 
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> index 58d4d97f4499..47692335bcf1 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> @@ -179,5 +179,55 @@
>               "$TC qdisc del dev $ETH root",
>               "echo \"1\" > /sys/bus/netdevsim/del_device"
>           ]
> +    },
> +    {
> +        "id": "a7bf",
> +        "name": "Graft cbs as child of software taprio",
> +        "category": [
> +            "qdisc",
> +            "taprio",
> +            "cbs"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [
> +            "echo \"1 1 8\" > /sys/bus/netdevsim/new_device",
> +            "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 0 sched-entry S ff 20000000 clockid CLOCK_TAI"
> +        ],
> +        "cmdUnderTest": "$TC qdisc replace dev $ETH handle 8002: parent 8001:8 cbs idleslope 20000 sendslope -980000 hicredit 30 locredit -1470",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC -d qdisc show dev $ETH",
> +        "matchPattern": "qdisc cbs 8002: parent 8001:8 hicredit 30 locredit -1470 sendslope -980000 idleslope 20000 offload 0",
> +        "matchCount": "1",
> +        "teardown": [
> +            "$TC qdisc del dev $ETH root",
> +            "echo \"1\" > /sys/bus/netdevsim/del_device"
> +        ]
> +    },
> +    {
> +        "id": "6a83",
> +        "name": "Graft cbs as child of offloaded taprio",
> +        "category": [
> +            "qdisc",
> +            "taprio",
> +            "cbs"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [
> +            "echo \"1 1 8\" > /sys/bus/netdevsim/new_device",
> +            "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 0 sched-entry S ff 20000000 flags 0x2"
> +        ],
> +        "cmdUnderTest": "$TC qdisc replace dev $ETH handle 8002: parent 8001:8 cbs idleslope 20000 sendslope -980000 hicredit 30 locredit -1470",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC -d qdisc show dev $ETH",
> +        "matchPattern": "qdisc cbs 8002: parent 8001:8 hicredit 30 locredit -1470 sendslope -980000 idleslope 20000 offload 0",

Seems like this test is missing the 'refcnt 2' in the match pattern

> +        "matchCount": "1",
> +        "teardown": [
> +            "$TC qdisc del dev $ETH root",
> +            "echo \"1\" > /sys/bus/netdevsim/del_device"
> +        ]
>       }
>   ]

Re: [PATCH v2 net-next 0/9] Improve the taprio qdisc's relationship with its children

[Pedro Tammela]

On 13/06/2023 18:54, Vladimir Oltean wrote:
> [...]

Hi Vladimir,
Thanks for adding the tdc tests.
This series seem to have broken test 8471 in taprio but I don't see it 
fixed here.
Do you plan to fix it in another patch?

Re: [PATCH v2 net-next 6/9] net: netdevsim: create a mock-up PTP Hardware Clock driver

[Vladimir Oltean]

Hi Simon,

On Wed, Jun 14, 2023 at 03:11:44PM +0200, Simon Horman wrote:
> > +#define MOCK_PHC_CC_SHIFT		31
> > +#define MOCK_PHC_CC_MULT		(1 << MOCK_PHC_CC_SHIFT)
> 
> Maybe BIT()?

Sorry, not everything that is 1 << something has BIT() semantics.
This in particular is quite clearly just a multiplier factor
expressed as a power of 2.

> > +struct mock_phc *mock_phc_create(struct device *dev)
> > +{
> > +	struct mock_phc *phc;
> > +	int err;
> > +
> > +	phc = kzalloc(sizeof(*phc), GFP_KERNEL);
> > +	if (!phc) {
> > +		err = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	phc->info = (struct ptp_clock_info) {
> > +		.owner		= THIS_MODULE,
> > +		.name		= "Mock-up PTP clock",
> > +		.max_adj	= MOCK_PHC_MAX_ADJ_PPB,
> > +		.adjfine	= mock_phc_adjfine,
> > +		.adjtime	= mock_phc_adjtime,
> > +		.gettime64	= mock_phc_gettime64,
> > +		.settime64	= mock_phc_settime64,
> > +		.do_aux_work	= mock_phc_refresh,
> > +	};
> > +
> > +	phc->cc = (struct cyclecounter) {
> > +		.read	= mock_phc_cc_read,
> > +		.mask	= CYCLECOUNTER_MASK(64),
> > +		.mult	= MOCK_PHC_CC_MULT,
> > +		.shift	= MOCK_PHC_CC_SHIFT,
> > +	};
> > +
> > +	spin_lock_init(&phc->lock);
> > +	timecounter_init(&phc->tc, &phc->cc, 0);
> > +
> > +	phc->clock = ptp_clock_register(&phc->info, dev);
> > +	if (IS_ERR_OR_NULL(phc->clock)) {
> > +		err = PTR_ERR_OR_ZERO(phc->clock);
> > +		goto out_free_phc;
> > +	}
> > +
> > +	ptp_schedule_worker(phc->clock, MOCK_PHC_REFRESH_INTERVAL);
> > +
> > +	return phc;
> > +
> > +out_free_phc:
> > +	kfree(phc);
> > +out:
> > +	return ERR_PTR(err);
> > +}
> 
> Smatch complains that ERR_PTR may be passed zero.
> Looking at the IS_ERR_OR_NULL block above, this does indeed seem to be the
> case.

The intention here had something to do with PTP being optional for the
caller (netdevsim). Not sure whether the implementation is the best -
and in particular whether ERR_PTR(0) is NULL or not. I guess this is
what the smatch warning (which I haven't looked at) is saying.

> Keeping Smatch happy is one thing - your call - but I do wonder if the
> caller of mock_phc_create() handles the NULL case correctly.

mock_phc_create() returns a pointer to an opaque data structure -
struct mock_phc - and the caller just carries that pointer around to the
other API calls exported by the mock_phc module. It doesn't need to care
whether the pointer is NULL or not, just the mock_phc module does (and
it does handle that part well, at least assuming that the pointer is NULL).

Re: [PATCH v2 net-next 7/9] net: netdevsim: mimic tc-taprio offload

[Vinicius Costa Gomes]

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> To be able to use netdevsim for tc-testing with an offloaded tc-taprio
> schedule, it needs to report a PTP clock (which it now does), and to
> accept ndo_setup_tc(TC_SETUP_QDISC_TAPRIO) calls.
>
> Since netdevsim has no packet I/O, this doesn't do anything intelligent,
> it only allows taprio offload code paths to go through some level of
> automated testing.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: patch is new
>
>  drivers/net/netdevsim/netdev.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 58cd51de5b79..e26be4bd0d90 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -209,6 +209,31 @@ static int nsim_set_vf_link_state(struct net_device *dev, int vf, int state)
>  	return 0;
>  }
>  
> +static void nsim_taprio_stats(struct tc_taprio_qopt_stats *stats)
> +{
> +	stats->window_drops = 0;
> +	stats->tx_overruns = 0;
> +}
> +
> +static int nsim_setup_tc_taprio(struct net_device *dev,
> +				struct tc_taprio_qopt_offload *offload)
> +{
> +	int err = 0;
> +
> +	switch (offload->cmd) {
> +	case TAPRIO_CMD_REPLACE:
> +	case TAPRIO_CMD_DESTROY:
> +		break;

I was thinking about how useful would proper validation of the
parameters be? Thinking that we could detect "driver API" breakages
earlier, and we want it documented that the drivers should check for the
things that it supports.

Makes sense?

> +	case TAPRIO_CMD_STATS:
> +		nsim_taprio_stats(&offload->stats);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +	}
> +
> +	return err;
> +}
> +
>  static LIST_HEAD(nsim_block_cb_list);
>  
>  static int
> @@ -217,6 +242,8 @@ nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
>  	struct netdevsim *ns = netdev_priv(dev);
>  
>  	switch (type) {
> +	case TC_SETUP_QDISC_TAPRIO:
> +		return nsim_setup_tc_taprio(dev, type_data);
>  	case TC_SETUP_BLOCK:
>  		return flow_block_cb_setup_simple(type_data,
>  						  &nsim_block_cb_list,
> -- 
> 2.34.1
>

Cheers,
-- 
Vinicius

Re: [PATCH v2 net-next 6/9] net: netdevsim: create a mock-up PTP Hardware Clock driver

[Simon Horman]

+ Dan Carpenter

On Thu, Jun 15, 2023 at 01:17:18AM +0300, Vladimir Oltean wrote:
> Hi Simon,
> 
> On Wed, Jun 14, 2023 at 03:11:44PM +0200, Simon Horman wrote:
> > > +#define MOCK_PHC_CC_SHIFT		31
> > > +#define MOCK_PHC_CC_MULT		(1 << MOCK_PHC_CC_SHIFT)
> > 
> > Maybe BIT()?
> 
> Sorry, not everything that is 1 << something has BIT() semantics.
> This in particular is quite clearly just a multiplier factor
> expressed as a power of 2.

Yes, sorry about that.

> > > +struct mock_phc *mock_phc_create(struct device *dev)
> > > +{
> > > +	struct mock_phc *phc;
> > > +	int err;
> > > +
> > > +	phc = kzalloc(sizeof(*phc), GFP_KERNEL);
> > > +	if (!phc) {
> > > +		err = -ENOMEM;
> > > +		goto out;
> > > +	}
> > > +
> > > +	phc->info = (struct ptp_clock_info) {
> > > +		.owner		= THIS_MODULE,
> > > +		.name		= "Mock-up PTP clock",
> > > +		.max_adj	= MOCK_PHC_MAX_ADJ_PPB,
> > > +		.adjfine	= mock_phc_adjfine,
> > > +		.adjtime	= mock_phc_adjtime,
> > > +		.gettime64	= mock_phc_gettime64,
> > > +		.settime64	= mock_phc_settime64,
> > > +		.do_aux_work	= mock_phc_refresh,
> > > +	};
> > > +
> > > +	phc->cc = (struct cyclecounter) {
> > > +		.read	= mock_phc_cc_read,
> > > +		.mask	= CYCLECOUNTER_MASK(64),
> > > +		.mult	= MOCK_PHC_CC_MULT,
> > > +		.shift	= MOCK_PHC_CC_SHIFT,
> > > +	};
> > > +
> > > +	spin_lock_init(&phc->lock);
> > > +	timecounter_init(&phc->tc, &phc->cc, 0);
> > > +
> > > +	phc->clock = ptp_clock_register(&phc->info, dev);
> > > +	if (IS_ERR_OR_NULL(phc->clock)) {
> > > +		err = PTR_ERR_OR_ZERO(phc->clock);
> > > +		goto out_free_phc;
> > > +	}
> > > +
> > > +	ptp_schedule_worker(phc->clock, MOCK_PHC_REFRESH_INTERVAL);
> > > +
> > > +	return phc;
> > > +
> > > +out_free_phc:
> > > +	kfree(phc);
> > > +out:
> > > +	return ERR_PTR(err);
> > > +}
> > 
> > Smatch complains that ERR_PTR may be passed zero.
> > Looking at the IS_ERR_OR_NULL block above, this does indeed seem to be the
> > case.
> 
> The intention here had something to do with PTP being optional for the
> caller (netdevsim). Not sure whether the implementation is the best -
> and in particular whether ERR_PTR(0) is NULL or not. I guess this is
> what the smatch warning (which I haven't looked at) is saying.

Thanks. It's unclear to me if ERR_PTR(0) is actually valid or not.
By itself it does seem harmless to me. But, OTOH, it is sometimes
indicative of some other problem. Fortunately that seems not to
be the case here.

> 
> > Keeping Smatch happy is one thing - your call - but I do wonder if the
> > caller of mock_phc_create() handles the NULL case correctly.
> 
> mock_phc_create() returns a pointer to an opaque data structure -
> struct mock_phc - and the caller just carries that pointer around to the
> other API calls exported by the mock_phc module. It doesn't need to care
> whether the pointer is NULL or not, just the mock_phc module does (and
> it does handle that part well, at least assuming that the pointer is NULL).

Thanks, got it.

Re: [PATCH v2 net-next 6/9] net: netdevsim: create a mock-up PTP Hardware Clock driver

[Dan Carpenter]

On Wed, Jun 14, 2023 at 12:54:37AM +0300, Vladimir Oltean wrote:
> +
> +	spin_lock_init(&phc->lock);
> +	timecounter_init(&phc->tc, &phc->cc, 0);
> +
> +	phc->clock = ptp_clock_register(&phc->info, dev);
> +	if (IS_ERR_OR_NULL(phc->clock)) {
> +		err = PTR_ERR_OR_ZERO(phc->clock);
> +		goto out_free_phc;
> +	}
> +
> +	ptp_schedule_worker(phc->clock, MOCK_PHC_REFRESH_INTERVAL);
> +
> +	return phc;
> +
> +out_free_phc:
> +	kfree(phc);
> +out:
> +	return ERR_PTR(err);
> +}

Simon added me to the CC list because this code generates a Smatch
warning about passing zero to ERR_PTR() which is NULL.  I have written
a blog about this kind of warning.

https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/

Returning NULL can be perfectly fine, but in this code here it will lead
to a crash.  The caller checks for error pointers but after that it
assumes that "phc" is a non-NULL pointer.

The IS_ERR_OR_NULL() check is not correct.  It should just be if
if (IS_ERR()).

However, the question is can this driver work without a phc->clock?
Depending on the answer you have to do one of two things.
If yes, then the correct thing is to add NULL checks throughout the
driver to prevent a NULL dereference.

If no, then the correct thing is to ensure that CONFIG_PTP_1588_CLOCK is
enabled using Kconfig.  We should never have a driver where we compile
it and then it can't probe.

regards,
dan carpenter

Re: [PATCH v2 net-next 0/9] Improve the taprio qdisc's relationship with its children

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 01:47:25PM -0300, Pedro Tammela wrote:
> On 13/06/2023 18:54, Vladimir Oltean wrote:
> > [...]
> 
> Hi Vladimir,
> Thanks for adding the tdc tests.
> This series seem to have broken test 8471 in taprio but I don't see it fixed
> here.
> Do you plan to fix it in another patch?
>

Thanks for pointing it out. I'll be unbreaking it in the next version,
as part of the patch that changes the "tc class show" output.

Re: [PATCH v2 net-next 7/9] net: netdevsim: mimic tc-taprio offload

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 05:06:24PM -0700, Vinicius Costa Gomes wrote:
> > +static int nsim_setup_tc_taprio(struct net_device *dev,
> > +				struct tc_taprio_qopt_offload *offload)
> > +{
> > +	int err = 0;
> > +
> > +	switch (offload->cmd) {
> > +	case TAPRIO_CMD_REPLACE:
> > +	case TAPRIO_CMD_DESTROY:
> > +		break;
> 
> I was thinking about how useful would proper validation of the
> parameters be? Thinking that we could detect "driver API" breakages
> earlier, and we want it documented that the drivers should check for the
> things that it supports.
> 
> Makes sense?

Sorry, I lack imagination as to what the netdevsim driver may check for.
The taprio offload parameters should always be valid, properly speaking,
otherwise the Qdisc wouldn't be passing them on to the driver. At least
that would be the intention. The rest are hardware specific checks for
hardware specific limitations. Here there is no hardware.

The parameters passed to TAPRIO_CMD_REPLACE are:

struct tc_mqprio_qopt_offload mqprio:
	struct tc_mqprio_qopt qopt: validated by taprio_parse_mqprio_opt() for flags 0x2
	u16 mode: always set to TC_MQPRIO_MODE_DCB
	u16 shaper: always set to TC_MQPRIO_SHAPER_DCB
	u32 flags: always set to 0
	u64 min_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
	u64 max_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
	unsigned long preemptible_tcs: always set to 0, because ethtool_dev_mm_supported() returns false

ktime_t base_time: any value is valid

u64 cycle_time: any value is valid

u64 cycle_time_extension: any value <= cycle_time is valid. According to 802.1Q
			  "Q.5 CycleTimeExtension variables", it's the maximum
			  amount by which the penultimate cycle can be extended
			  to avoid a very short cycle upon a ConfigChange event.
			  But if CycleTimeExtension is larger than one CycleTime,
			  then we're not even talking about the penultimate cycle
			  anymore, but about ones previous to that?! Maybe this
			  should be limited to 0 <= cycle_time_extension <= cycle_time
			  by taprio, certainly not by offloading drivers.

u32 max_sdu[TC_MAX_QUEUE]: limited to a value <= dev->max_mtu by taprio

size_t num_entries: any value is valid

struct tc_taprio_sched_entry entries[]:
	u8 command: will be either one of: TC_TAPRIO_CMD_SET_GATES, TC_TAPRIO_CMD_SET_AND_HOLD
		    or TC_TAPRIO_CMD_SET_AND_RELEASE. However 802.1Q "Table 8-7—Gate operations"
		    says "If frame preemption is not supported or not enabled (preemptionActive is
		    FALSE), this operation behaves the same as SetGateStates.". So I
		    see no reason to enforce any restriction here either?

	u32 gate_mask: technically can have bits set, which correspond
		       to traffic classes larger than dev->num_tc.
		       Taprio can enforce this, so I wouldn't see
		       drivers beginning to feel paranoid about it.
		       Actually I had a patch about this:
		       https://patchwork.kernel.org/project/netdevbpf/patch/20230130173145.475943-15-vladimir.oltean@nxp.com/
		       but I decided to drop it because I didn't have
		       any strong case for it.
	u32 interval: any value is valid. If the sum of entry intervals
		      is less than the cycle_time, again that's taprio's
		      problem to check for, in its netlink attribute
		      validation method rather than offloading drivers.

There are no parameters given to TAPRIO_CMD_DESTROY.

Re: [PATCH v2 net-next 9/9] selftests/tc-testing: verify that a qdisc can be grafted onto a taprio class

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 01:45:42PM -0300, Pedro Tammela wrote:
> > +        "cmdUnderTest": "$TC qdisc replace dev $ETH handle 8002: parent 8001:8 cbs idleslope 20000 sendslope -980000 hicredit 30 locredit -1470",
> > +        "expExitCode": "0",
> > +        "verifyCmd": "$TC -d qdisc show dev $ETH",
> > +        "matchPattern": "qdisc cbs 8002: parent 8001:8 hicredit 30 locredit -1470 sendslope -980000 idleslope 20000 offload 0",
> 
> Seems like this test is missing the 'refcnt 2' in the match pattern

Makes sense. This is consistent with the idea of my patch set, which is
that in offloaded taprio mode, each child Qdisc has a refcount elevated
by the fact that it's attached to a netdev TX queue (hence the 2 here).
I had copied this expected output from the "Graft cbs as child of software
taprio" test a7bf (not sure why I didn't catch the failure), but there,
the expected refcount of child Qdiscs is 1 (and thus, not shown).

Re: [PATCH v2 net-next 7/9] net: netdevsim: mimic tc-taprio offload

[Vinicius Costa Gomes]

Hi Vladimir,

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> On Wed, Jun 14, 2023 at 05:06:24PM -0700, Vinicius Costa Gomes wrote:
>> > +static int nsim_setup_tc_taprio(struct net_device *dev,
>> > +				struct tc_taprio_qopt_offload *offload)
>> > +{
>> > +	int err = 0;
>> > +
>> > +	switch (offload->cmd) {
>> > +	case TAPRIO_CMD_REPLACE:
>> > +	case TAPRIO_CMD_DESTROY:
>> > +		break;
>> 
>> I was thinking about how useful would proper validation of the
>> parameters be? Thinking that we could detect "driver API" breakages
>> earlier, and we want it documented that the drivers should check for the
>> things that it supports.
>> 
>> Makes sense?
>
> Sorry, I lack imagination as to what the netdevsim driver may check for.
> The taprio offload parameters should always be valid, properly speaking,
> otherwise the Qdisc wouldn't be passing them on to the driver. At least
> that would be the intention. The rest are hardware specific checks for
> hardware specific limitations. Here there is no hardware.
>

Trying to remember what was going through my mind when I said that.

What I seem to recall is something that would help us "keep honest":
I was worrying about someone (perhaps myself ;-) sneaking a new feature
in taprio and forgetting to update other drivers.

I thought that adding a check for the existing parameters would help
detect those kind of things. If anything unknown was there in the
offload struct, netdevsim would complain loudly.

Perhaps I was worrying too much. And the way to solve that is to keep
active attention against that during review.

> The parameters passed to TAPRIO_CMD_REPLACE are:
>
> struct tc_mqprio_qopt_offload mqprio:
> 	struct tc_mqprio_qopt qopt: validated by taprio_parse_mqprio_opt() for flags 0x2
> 	u16 mode: always set to TC_MQPRIO_MODE_DCB
> 	u16 shaper: always set to TC_MQPRIO_SHAPER_DCB
> 	u32 flags: always set to 0
> 	u64 min_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
> 	u64 max_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
> 	unsigned long preemptible_tcs: always set to 0, because ethtool_dev_mm_supported() returns false
>
> ktime_t base_time: any value is valid
>
> u64 cycle_time: any value is valid
>
> u64 cycle_time_extension: any value <= cycle_time is valid. According to 802.1Q
> 			  "Q.5 CycleTimeExtension variables", it's the maximum
> 			  amount by which the penultimate cycle can be extended
> 			  to avoid a very short cycle upon a ConfigChange event.
> 			  But if CycleTimeExtension is larger than one CycleTime,
> 			  then we're not even talking about the penultimate cycle
> 			  anymore, but about ones previous to that?! Maybe this
> 			  should be limited to 0 <= cycle_time_extension <= cycle_time
> 			  by taprio, certainly not by offloading drivers.
>

Good point. I have to review 802.1Q, but from what I remember that
sounds right, cycle_time_extension greater than cycle_time doesn't make
much sense. Having a check for it in taprio itself sounds good.

> u32 max_sdu[TC_MAX_QUEUE]: limited to a value <= dev->max_mtu by taprio
>
> size_t num_entries: any value is valid
>
> struct tc_taprio_sched_entry entries[]:
> 	u8 command: will be either one of: TC_TAPRIO_CMD_SET_GATES, TC_TAPRIO_CMD_SET_AND_HOLD
> 		    or TC_TAPRIO_CMD_SET_AND_RELEASE. However 802.1Q "Table 8-7—Gate operations"
> 		    says "If frame preemption is not supported or not enabled (preemptionActive is
> 		    FALSE), this operation behaves the same as SetGateStates.". So I
> 		    see no reason to enforce any restriction here either?
>
> 	u32 gate_mask: technically can have bits set, which correspond
> 		       to traffic classes larger than dev->num_tc.
> 		       Taprio can enforce this, so I wouldn't see
> 		       drivers beginning to feel paranoid about it.
> 		       Actually I had a patch about this:
> 		       https://patchwork.kernel.org/project/netdevbpf/patch/20230130173145.475943-15-vladimir.oltean@nxp.com/
> 		       but I decided to drop it because I didn't have
> 		       any strong case for it.
> 	u32 interval: any value is valid. If the sum of entry intervals
> 		      is less than the cycle_time, again that's taprio's
> 		      problem to check for, in its netlink attribute
> 		      validation method rather than offloading drivers.
>

Thank you for the time it took to give this amount of detail.


Cheers,
-- 
Vinicius

Re: [PATCH v2 net-next 7/9] net: netdevsim: mimic tc-taprio offload

[Vladimir Oltean]

On Tue, Aug 01, 2023 at 10:39:23AM -0700, Vinicius Costa Gomes wrote:
> Hi Vladimir,
> 
> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
> 
> > On Wed, Jun 14, 2023 at 05:06:24PM -0700, Vinicius Costa Gomes wrote:
> >> > +static int nsim_setup_tc_taprio(struct net_device *dev,
> >> > +				struct tc_taprio_qopt_offload *offload)
> >> > +{
> >> > +	int err = 0;
> >> > +
> >> > +	switch (offload->cmd) {
> >> > +	case TAPRIO_CMD_REPLACE:
> >> > +	case TAPRIO_CMD_DESTROY:
> >> > +		break;
> >> 
> >> I was thinking about how useful would proper validation of the
> >> parameters be? Thinking that we could detect "driver API" breakages
> >> earlier, and we want it documented that the drivers should check for the
> >> things that it supports.
> >> 
> >> Makes sense?
> >
> > Sorry, I lack imagination as to what the netdevsim driver may check for.
> > The taprio offload parameters should always be valid, properly speaking,
> > otherwise the Qdisc wouldn't be passing them on to the driver. At least
> > that would be the intention. The rest are hardware specific checks for
> > hardware specific limitations. Here there is no hardware.
> >
> 
> Trying to remember what was going through my mind when I said that.
> 
> What I seem to recall is something that would help us "keep honest":
> I was worrying about someone (perhaps myself ;-) sneaking a new feature
> in taprio and forgetting to update other drivers.
> 
> I thought that adding a check for the existing parameters would help
> detect those kind of things. If anything unknown was there in the
> offload struct, netdevsim would complain loudly.
> 
> Perhaps I was worrying too much. And the way to solve that is to keep
> active attention against that during review.

Ok, so I'm not making any change to the patch set as a result of this
comment, would you agree?

Re: [PATCH v2 net-next 7/9] net: netdevsim: mimic tc-taprio offload

[Vinicius Costa Gomes]

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> On Tue, Aug 01, 2023 at 10:39:23AM -0700, Vinicius Costa Gomes wrote:
>> Hi Vladimir,
>> 
>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
>> 
>> > On Wed, Jun 14, 2023 at 05:06:24PM -0700, Vinicius Costa Gomes wrote:
>> >> > +static int nsim_setup_tc_taprio(struct net_device *dev,
>> >> > +				struct tc_taprio_qopt_offload *offload)
>> >> > +{
>> >> > +	int err = 0;
>> >> > +
>> >> > +	switch (offload->cmd) {
>> >> > +	case TAPRIO_CMD_REPLACE:
>> >> > +	case TAPRIO_CMD_DESTROY:
>> >> > +		break;
>> >> 
>> >> I was thinking about how useful would proper validation of the
>> >> parameters be? Thinking that we could detect "driver API" breakages
>> >> earlier, and we want it documented that the drivers should check for the
>> >> things that it supports.
>> >> 
>> >> Makes sense?
>> >
>> > Sorry, I lack imagination as to what the netdevsim driver may check for.
>> > The taprio offload parameters should always be valid, properly speaking,
>> > otherwise the Qdisc wouldn't be passing them on to the driver. At least
>> > that would be the intention. The rest are hardware specific checks for
>> > hardware specific limitations. Here there is no hardware.
>> >
>> 
>> Trying to remember what was going through my mind when I said that.
>> 
>> What I seem to recall is something that would help us "keep honest":
>> I was worrying about someone (perhaps myself ;-) sneaking a new feature
>> in taprio and forgetting to update other drivers.
>> 
>> I thought that adding a check for the existing parameters would help
>> detect those kind of things. If anything unknown was there in the
>> offload struct, netdevsim would complain loudly.
>> 
>> Perhaps I was worrying too much. And the way to solve that is to keep
>> active attention against that during review.
>
> Ok, so I'm not making any change to the patch set as a result of this
> comment, would you agree?

Agreed.


Cheers,
-- 
Vinicius