[PATCH net-next 0/5] xstats for tc-taprio

[PATCH net-next 0/5] xstats for tc-taprio

[Vladimir Oltean]

As a result of this discussion:
https://lore.kernel.org/intel-wired-lan/20230411055543.24177-1-muhammad.husaini.zulkifli@intel.com/

it became apparent that tc-taprio should make an effort to standardize
statistics counters related to the 802.1Qbv scheduling as implemented
by the NIC. I'm presenting here one counter suggested by the standard,
and one counter defined by the NXP ENETC controller from LS1028A. Both
counters are reported globally and per traffic class - drivers get
different callbacks for reporting both of these, and get to choose what
to report in both cases.

The iproute2 counterpart is available here for testing:
https://github.com/vladimiroltean/iproute2/commits/taprio-xstats

Vladimir Oltean (5):
  net/sched: taprio: don't overwrite "sch" variable in
    taprio_dump_class_stats()
  net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a
    "cmd" enum
  net/sched: taprio: add netlink reporting for offload statistics
    counters
  net: enetc: refactor enetc_setup_tc_taprio() to have a switch/case for
    cmd
  net: enetc: report statistics counters for taprio

 drivers/net/dsa/hirschmann/hellcreek.c        |  14 ++-
 drivers/net/dsa/ocelot/felix_vsc9959.c        |   4 +-
 drivers/net/dsa/sja1105/sja1105_tas.c         |   7 +-
 .../net/ethernet/engleder/tsnep_selftests.c   |  12 +-
 drivers/net/ethernet/engleder/tsnep_tc.c      |   4 +-
 drivers/net/ethernet/freescale/enetc/enetc.c  |   3 +-
 drivers/net/ethernet/freescale/enetc/enetc.h  |   1 +
 .../net/ethernet/freescale/enetc/enetc_qos.c  | 110 ++++++++++++++----
 drivers/net/ethernet/intel/igc/igc_main.c     |  13 ++-
 .../ethernet/microchip/lan966x/lan966x_tc.c   |  10 +-
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   |   7 +-
 drivers/net/ethernet/ti/am65-cpsw-qos.c       |  11 +-
 include/net/pkt_sched.h                       |  56 +++++++--
 include/uapi/linux/pkt_sched.h                |  10 ++
 net/sched/sch_taprio.c                        |  90 ++++++++++++--
 15 files changed, 286 insertions(+), 66 deletions(-)

-- 
2.34.1

[PATCH net-next 1/5] net/sched: taprio: don't overwrite "sch" variable in taprio_dump_class_stats()

[Vladimir Oltean]

In taprio_dump_class_stats() we don't need a reference to the root Qdisc
once we get the reference to the child corresponding to this traffic
class, so it's okay to overwrite "sch". But in a future patch we will
need the root Qdisc too, so create a dedicated "child" pointer variable
to hold the child reference. This also makes the code adhere to a more
conventional coding style.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/sched/sch_taprio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 76db9a10ef50..d29e6785854d 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -2388,10 +2388,10 @@ static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
 	__acquires(d->lock)
 {
 	struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
+	struct Qdisc *child = dev_queue->qdisc_sleeping;
 
-	sch = dev_queue->qdisc_sleeping;
-	if (gnet_stats_copy_basic(d, NULL, &sch->bstats, true) < 0 ||
-	    qdisc_qstats_copy(d, sch) < 0)
+	if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 ||
+	    qdisc_qstats_copy(d, child) < 0)
 		return -1;
 	return 0;
 }
-- 
2.34.1

Re: [PATCH net-next 1/5] net/sched: taprio: don't overwrite "sch" variable in taprio_dump_class_stats()

[Vinicius Costa Gomes]

Hi,

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

> In taprio_dump_class_stats() we don't need a reference to the root Qdisc
> once we get the reference to the child corresponding to this traffic
> class, so it's okay to overwrite "sch". But in a future patch we will
> need the root Qdisc too, so create a dedicated "child" pointer variable
> to hold the child reference. This also makes the code adhere to a more
> conventional coding style.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

The patch looks good:

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

But I have a suggestion, this "taprio_queue_get() ->
dev_queue->qdisc_sleeping()" dance should have the same result as
calling 'taprio_leaf()'.

I am thinking of using taprio_leaf() here and in taprio_dump_class().
Could be a separate commit.


>  net/sched/sch_taprio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 76db9a10ef50..d29e6785854d 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -2388,10 +2388,10 @@ static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
>  	__acquires(d->lock)
>  {
>  	struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
> +	struct Qdisc *child = dev_queue->qdisc_sleeping;
>  
> -	sch = dev_queue->qdisc_sleeping;
> -	if (gnet_stats_copy_basic(d, NULL, &sch->bstats, true) < 0 ||
> -	    qdisc_qstats_copy(d, sch) < 0)
> +	if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 ||
> +	    qdisc_qstats_copy(d, child) < 0)
>  		return -1;
>  	return 0;
>  }
> -- 
> 2.34.1
>

-- 
Vinicius

Re: [PATCH net-next 1/5] net/sched: taprio: don't overwrite "sch" variable in taprio_dump_class_stats()

[Vladimir Oltean]

On Tue, May 30, 2023 at 02:14:17PM -0700, Vinicius Costa Gomes wrote:
> But I have a suggestion, this "taprio_queue_get() ->
> dev_queue->qdisc_sleeping()" dance should have the same result as
> calling 'taprio_leaf()'.
> 
> I am thinking of using taprio_leaf() here and in taprio_dump_class().
> Could be a separate commit.

Got it, you want to consolidate the dev_queue->qdisc_sleeping pattern.
Since taprio_dump_class() could benefit from the consolidation too, they
could really be both converted separately. Or I could also handle that
in this patch set, if I need to resend it.

Re: [PATCH net-next 1/5] net/sched: taprio: don't overwrite "sch" variable in taprio_dump_class_stats()

[Vinicius Costa Gomes]

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

> On Tue, May 30, 2023 at 02:14:17PM -0700, Vinicius Costa Gomes wrote:
>> But I have a suggestion, this "taprio_queue_get() ->
>> dev_queue->qdisc_sleeping()" dance should have the same result as
>> calling 'taprio_leaf()'.
>> 
>> I am thinking of using taprio_leaf() here and in taprio_dump_class().
>> Could be a separate commit.
>
> Got it, you want to consolidate the dev_queue->qdisc_sleeping pattern.
> Since taprio_dump_class() could benefit from the consolidation too, they
> could really be both converted separately. Or I could also handle that
> in this patch set, if I need to resend it.

Exactly. Both options sound great.


Cheers,
-- 
Vinicius

[PATCH net-next 2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum

[Vladimir Oltean]

Inspired from struct flow_cls_offload :: cmd, in order for taprio to be
able to report statistics (which is future work), it seems that we need
to drill one step further with the ndo_setup_tc(TC_SETUP_QDISC_TAPRIO)
multiplexing, and pass the command as part of the common portion of the
muxed structure.

Since we already have an "enable" variable in tc_taprio_qopt_offload,
refactor all drivers to check for "cmd" instead of "enable", and reject
every other command except "replace" and "destroy" - to be future proof.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/hirschmann/hellcreek.c             | 14 +++++++++-----
 drivers/net/dsa/ocelot/felix_vsc9959.c             |  4 +++-
 drivers/net/dsa/sja1105/sja1105_tas.c              |  7 +++++--
 drivers/net/ethernet/engleder/tsnep_selftests.c    | 12 ++++++------
 drivers/net/ethernet/engleder/tsnep_tc.c           |  4 +++-
 drivers/net/ethernet/freescale/enetc/enetc_qos.c   |  6 +++++-
 drivers/net/ethernet/intel/igc/igc_main.c          | 13 +++++++++++--
 .../net/ethernet/microchip/lan966x/lan966x_tc.c    | 10 ++++++++--
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c    |  7 +++++--
 drivers/net/ethernet/ti/am65-cpsw-qos.c            | 11 ++++++++---
 include/net/pkt_sched.h                            |  7 ++++++-
 net/sched/sch_taprio.c                             |  4 ++--
 12 files changed, 71 insertions(+), 28 deletions(-)

diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index 595a548bb0a8..af50001ccdd4 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -1885,13 +1885,17 @@ static int hellcreek_port_setup_tc(struct dsa_switch *ds, int port,
 	case TC_SETUP_QDISC_TAPRIO: {
 		struct tc_taprio_qopt_offload *taprio = type_data;
 
-		if (!hellcreek_validate_schedule(hellcreek, taprio))
-			return -EOPNOTSUPP;
+		switch (taprio->cmd) {
+		case TAPRIO_CMD_REPLACE:
+			if (!hellcreek_validate_schedule(hellcreek, taprio))
+				return -EOPNOTSUPP;
 
-		if (taprio->enable)
 			return hellcreek_port_set_schedule(ds, port, taprio);
-
-		return hellcreek_port_del_schedule(ds, port);
+		case TAPRIO_CMD_DESTROY:
+			return hellcreek_port_del_schedule(ds, port);
+		default:
+			return -EOPNOTSUPP;
+		}
 	}
 	default:
 		return -EOPNOTSUPP;
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 030738fef60e..5de6a27052fc 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1411,7 +1411,7 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
 
 	mutex_lock(&ocelot->tas_lock);
 
-	if (!taprio->enable) {
+	if (taprio->cmd == TAPRIO_CMD_DESTROY) {
 		ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
 		ocelot_rmw_rix(ocelot, 0, QSYS_TAG_CONFIG_ENABLE,
 			       QSYS_TAG_CONFIG, port);
@@ -1423,6 +1423,8 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
 
 		mutex_unlock(&ocelot->tas_lock);
 		return 0;
+	} else if (taprio->cmd != TAPRIO_CMD_REPLACE) {
+		return -EOPNOTSUPP;
 	}
 
 	ret = ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
diff --git a/drivers/net/dsa/sja1105/sja1105_tas.c b/drivers/net/dsa/sja1105/sja1105_tas.c
index e6153848a950..d7818710bc02 100644
--- a/drivers/net/dsa/sja1105/sja1105_tas.c
+++ b/drivers/net/dsa/sja1105/sja1105_tas.c
@@ -516,10 +516,11 @@ int sja1105_setup_tc_taprio(struct dsa_switch *ds, int port,
 	/* Can't change an already configured port (must delete qdisc first).
 	 * Can't delete the qdisc from an unconfigured port.
 	 */
-	if (!!tas_data->offload[port] == admin->enable)
+	if ((!!tas_data->offload[port] && admin->cmd == TAPRIO_CMD_REPLACE) ||
+	    (!tas_data->offload[port] && admin->cmd == TAPRIO_CMD_DESTROY))
 		return -EINVAL;
 
-	if (!admin->enable) {
+	if (admin->cmd == TAPRIO_CMD_DESTROY) {
 		taprio_offload_free(tas_data->offload[port]);
 		tas_data->offload[port] = NULL;
 
@@ -528,6 +529,8 @@ int sja1105_setup_tc_taprio(struct dsa_switch *ds, int port,
 			return rc;
 
 		return sja1105_static_config_reload(priv, SJA1105_SCHEDULING);
+	} else if (admin->cmd != TAPRIO_CMD_REPLACE) {
+		return -EOPNOTSUPP;
 	}
 
 	/* The cycle time extension is the amount of time the last cycle from
diff --git a/drivers/net/ethernet/engleder/tsnep_selftests.c b/drivers/net/ethernet/engleder/tsnep_selftests.c
index 1581d6b22232..8a9145f93147 100644
--- a/drivers/net/ethernet/engleder/tsnep_selftests.c
+++ b/drivers/net/ethernet/engleder/tsnep_selftests.c
@@ -329,7 +329,7 @@ static bool disable_taprio(struct tsnep_adapter *adapter)
 	int retval;
 
 	memset(&qopt, 0, sizeof(qopt));
-	qopt.enable = 0;
+	qopt.cmd = TAPRIO_CMD_DESTROY;
 	retval = tsnep_tc_setup(adapter->netdev, TC_SETUP_QDISC_TAPRIO, &qopt);
 	if (retval)
 		return false;
@@ -360,7 +360,7 @@ static bool tsnep_test_taprio(struct tsnep_adapter *adapter)
 	for (i = 0; i < 255; i++)
 		qopt->entries[i].command = TC_TAPRIO_CMD_SET_GATES;
 
-	qopt->enable = 1;
+	qopt->cmd = TAPRIO_CMD_REPLACE;
 	qopt->base_time = ktime_set(0, 0);
 	qopt->cycle_time = 1500000;
 	qopt->cycle_time_extension = 0;
@@ -382,7 +382,7 @@ static bool tsnep_test_taprio(struct tsnep_adapter *adapter)
 	if (!run_taprio(adapter, qopt, 100))
 		goto failed;
 
-	qopt->enable = 1;
+	qopt->cmd = TAPRIO_CMD_REPLACE;
 	qopt->base_time = ktime_set(0, 0);
 	qopt->cycle_time = 411854;
 	qopt->cycle_time_extension = 0;
@@ -406,7 +406,7 @@ static bool tsnep_test_taprio(struct tsnep_adapter *adapter)
 	if (!run_taprio(adapter, qopt, 100))
 		goto failed;
 
-	qopt->enable = 1;
+	qopt->cmd = TAPRIO_CMD_REPLACE;
 	qopt->base_time = ktime_set(0, 0);
 	delay_base_time(adapter, qopt, 12);
 	qopt->cycle_time = 125000;
@@ -457,7 +457,7 @@ static bool tsnep_test_taprio_change(struct tsnep_adapter *adapter)
 	for (i = 0; i < 255; i++)
 		qopt->entries[i].command = TC_TAPRIO_CMD_SET_GATES;
 
-	qopt->enable = 1;
+	qopt->cmd = TAPRIO_CMD_REPLACE;
 	qopt->base_time = ktime_set(0, 0);
 	qopt->cycle_time = 100000;
 	qopt->cycle_time_extension = 0;
@@ -610,7 +610,7 @@ static bool tsnep_test_taprio_extension(struct tsnep_adapter *adapter)
 	for (i = 0; i < 255; i++)
 		qopt->entries[i].command = TC_TAPRIO_CMD_SET_GATES;
 
-	qopt->enable = 1;
+	qopt->cmd = TAPRIO_CMD_REPLACE;
 	qopt->base_time = ktime_set(0, 0);
 	qopt->cycle_time = 100000;
 	qopt->cycle_time_extension = 50000;
diff --git a/drivers/net/ethernet/engleder/tsnep_tc.c b/drivers/net/ethernet/engleder/tsnep_tc.c
index d083e6684f12..745b191a5540 100644
--- a/drivers/net/ethernet/engleder/tsnep_tc.c
+++ b/drivers/net/ethernet/engleder/tsnep_tc.c
@@ -325,7 +325,7 @@ static int tsnep_taprio(struct tsnep_adapter *adapter,
 	if (!adapter->gate_control)
 		return -EOPNOTSUPP;
 
-	if (!qopt->enable) {
+	if (qopt->cmd == TAPRIO_CMD_DESTROY) {
 		/* disable gate control if active */
 		mutex_lock(&adapter->gate_control_lock);
 
@@ -337,6 +337,8 @@ static int tsnep_taprio(struct tsnep_adapter *adapter,
 		mutex_unlock(&adapter->gate_control_lock);
 
 		return 0;
+	} else if (qopt->cmd != TAPRIO_CMD_REPLACE) {
+		return -EOPNOTSUPP;
 	}
 
 	retval = tsnep_validate_gcl(qopt);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 83c27bbbc6ed..7aad824f4da7 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -65,7 +65,7 @@ static int enetc_setup_taprio(struct net_device *ndev,
 	gcl_len = admin_conf->num_entries;
 
 	tge = enetc_rd(hw, ENETC_PTGCR);
-	if (!admin_conf->enable) {
+	if (admin_conf->cmd == TAPRIO_CMD_DESTROY) {
 		enetc_wr(hw, ENETC_PTGCR, tge & ~ENETC_PTGCR_TGE);
 		enetc_reset_ptcmsdur(hw);
 
@@ -138,6 +138,10 @@ int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	int err, i;
 
+	if (taprio->cmd != TAPRIO_CMD_REPLACE &&
+	    taprio->cmd != TAPRIO_CMD_DESTROY)
+		return -EOPNOTSUPP;
+
 	/* TSD and Qbv are mutually exclusive in hardware */
 	for (i = 0; i < priv->num_tx_rings; i++)
 		if (priv->tx_ring[i]->tsd_enable)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index c5ef1edcf548..88145c30c919 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6113,9 +6113,18 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 	size_t n;
 	int i;
 
-	adapter->qbv_enable = qopt->enable;
+	switch (qopt->cmd) {
+	case TAPRIO_CMD_REPLACE:
+		adapter->qbv_enable = true;
+		break;
+	case TAPRIO_CMD_DESTROY:
+		adapter->qbv_enable = false;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
 
-	if (!qopt->enable)
+	if (!adapter->qbv_enable)
 		return igc_tsn_clear_schedule(adapter);
 
 	if (qopt->base_time < 0)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c b/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
index cf0cc7562d04..ee652f2d2359 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
@@ -21,8 +21,14 @@ static int lan966x_tc_setup_qdisc_mqprio(struct lan966x_port *port,
 static int lan966x_tc_setup_qdisc_taprio(struct lan966x_port *port,
 					 struct tc_taprio_qopt_offload *taprio)
 {
-	return taprio->enable ? lan966x_taprio_add(port, taprio) :
-				lan966x_taprio_del(port);
+	switch (taprio->cmd) {
+	case TAPRIO_CMD_REPLACE:
+		return lan966x_taprio_add(port, taprio);
+	case TAPRIO_CMD_DESTROY:
+		return lan966x_taprio_del(port);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static int lan966x_tc_setup_qdisc_tbf(struct lan966x_port *port,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 9d55226479b4..ac41ef4cbd2f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -966,8 +966,11 @@ static int tc_setup_taprio(struct stmmac_priv *priv,
 		return -EOPNOTSUPP;
 	}
 
-	if (!qopt->enable)
+	if (qopt->cmd == TAPRIO_CMD_DESTROY)
 		goto disable;
+	else if (qopt->cmd != TAPRIO_CMD_REPLACE)
+		return -EOPNOTSUPP;
+
 	if (qopt->num_entries >= dep)
 		return -EINVAL;
 	if (!qopt->cycle_time)
@@ -988,7 +991,7 @@ static int tc_setup_taprio(struct stmmac_priv *priv,
 
 	mutex_lock(&priv->plat->est->lock);
 	priv->plat->est->gcl_size = size;
-	priv->plat->est->enable = qopt->enable;
+	priv->plat->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
 	mutex_unlock(&priv->plat->est->lock);
 
 	for (i = 0; i < size; i++) {
diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.c b/drivers/net/ethernet/ti/am65-cpsw-qos.c
index 3a908db6e5b2..eced87fa261c 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-qos.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-qos.c
@@ -450,7 +450,7 @@ static int am65_cpsw_configure_taprio(struct net_device *ndev,
 
 	am65_cpsw_est_update_state(ndev);
 
-	if (!est_new->taprio.enable) {
+	if (est_new->taprio.cmd == TAPRIO_CMD_DESTROY) {
 		am65_cpsw_stop_est(ndev);
 		return ret;
 	}
@@ -476,7 +476,7 @@ static int am65_cpsw_configure_taprio(struct net_device *ndev,
 	am65_cpsw_est_set_sched_list(ndev, est_new);
 	am65_cpsw_port_est_assign_buf_num(ndev, est_new->buf);
 
-	am65_cpsw_est_set(ndev, est_new->taprio.enable);
+	am65_cpsw_est_set(ndev, est_new->taprio.cmd == TAPRIO_CMD_REPLACE);
 
 	if (tact == TACT_PROG) {
 		ret = am65_cpsw_timer_set(ndev, est_new);
@@ -520,7 +520,7 @@ static int am65_cpsw_set_taprio(struct net_device *ndev, void *type_data)
 	am65_cpsw_cp_taprio(taprio, &est_new->taprio);
 	ret = am65_cpsw_configure_taprio(ndev, est_new);
 	if (!ret) {
-		if (taprio->enable) {
+		if (taprio->cmd == TAPRIO_CMD_REPLACE) {
 			devm_kfree(&ndev->dev, port->qos.est_admin);
 
 			port->qos.est_admin = est_new;
@@ -564,8 +564,13 @@ static void am65_cpsw_est_link_up(struct net_device *ndev, int link_speed)
 static int am65_cpsw_setup_taprio(struct net_device *ndev, void *type_data)
 {
 	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
+	struct tc_taprio_qopt_offload *taprio = type_data;
 	struct am65_cpsw_common *common = port->common;
 
+	if (taprio->cmd != TAPRIO_CMD_REPLACE &&
+	    taprio->cmd != TAPRIO_CMD_DESTROY)
+		return -EOPNOTSUPP;
+
 	if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
 		return -ENODEV;
 
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index f436688b6efc..f5fb11da357b 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -185,6 +185,11 @@ struct tc_taprio_caps {
 	bool broken_mqprio:1;
 };
 
+enum tc_taprio_qopt_cmd {
+	TAPRIO_CMD_REPLACE,
+	TAPRIO_CMD_DESTROY,
+};
+
 struct tc_taprio_sched_entry {
 	u8 command; /* TC_TAPRIO_CMD_* */
 
@@ -196,7 +201,7 @@ struct tc_taprio_sched_entry {
 struct tc_taprio_qopt_offload {
 	struct tc_mqprio_qopt_offload mqprio;
 	struct netlink_ext_ack *extack;
-	u8 enable;
+	enum tc_taprio_qopt_cmd cmd;
 	ktime_t base_time;
 	u64 cycle_time;
 	u64 cycle_time_extension;
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index d29e6785854d..06bf4c6355a5 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1524,7 +1524,7 @@ static int taprio_enable_offload(struct net_device *dev,
 			       "Not enough memory for enabling offload mode");
 		return -ENOMEM;
 	}
-	offload->enable = 1;
+	offload->cmd = TAPRIO_CMD_REPLACE;
 	offload->extack = extack;
 	mqprio_qopt_reconstruct(dev, &offload->mqprio.qopt);
 	offload->mqprio.extack = extack;
@@ -1572,7 +1572,7 @@ static int taprio_disable_offload(struct net_device *dev,
 			       "Not enough memory to disable offload mode");
 		return -ENOMEM;
 	}
-	offload->enable = 0;
+	offload->cmd = TAPRIO_CMD_DESTROY;
 
 	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
 	if (err < 0) {
-- 
2.34.1

Re: [PATCH net-next 2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum

[Horatiu Vultur]

The 05/30/2023 12:19, Vladimir Oltean wrote:
> 
> Inspired from struct flow_cls_offload :: cmd, in order for taprio to be
> able to report statistics (which is future work), it seems that we need
> to drill one step further with the ndo_setup_tc(TC_SETUP_QDISC_TAPRIO)
> multiplexing, and pass the command as part of the common portion of the
> muxed structure.
> 
> Since we already have an "enable" variable in tc_taprio_qopt_offload,
> refactor all drivers to check for "cmd" instead of "enable", and reject
> every other command except "replace" and "destroy" - to be future proof.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com> # for lan966x

> ---
>  drivers/net/dsa/hirschmann/hellcreek.c             | 14 +++++++++-----
>  drivers/net/dsa/ocelot/felix_vsc9959.c             |  4 +++-
>  drivers/net/dsa/sja1105/sja1105_tas.c              |  7 +++++--
>  drivers/net/ethernet/engleder/tsnep_selftests.c    | 12 ++++++------
>  drivers/net/ethernet/engleder/tsnep_tc.c           |  4 +++-
>  drivers/net/ethernet/freescale/enetc/enetc_qos.c   |  6 +++++-
>  drivers/net/ethernet/intel/igc/igc_main.c          | 13 +++++++++++--
>  .../net/ethernet/microchip/lan966x/lan966x_tc.c    | 10 ++++++++--
>  drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c    |  7 +++++--
>  drivers/net/ethernet/ti/am65-cpsw-qos.c            | 11 ++++++++---
>  include/net/pkt_sched.h                            |  7 ++++++-
>  net/sched/sch_taprio.c                             |  4 ++--
>  12 files changed, 71 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
> index 595a548bb0a8..af50001ccdd4 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
> @@ -1885,13 +1885,17 @@ static int hellcreek_port_setup_tc(struct dsa_switch *ds, int port,
>         case TC_SETUP_QDISC_TAPRIO: {
>                 struct tc_taprio_qopt_offload *taprio = type_data;
> 
> -               if (!hellcreek_validate_schedule(hellcreek, taprio))
> -                       return -EOPNOTSUPP;
> +               switch (taprio->cmd) {
> +               case TAPRIO_CMD_REPLACE:
> +                       if (!hellcreek_validate_schedule(hellcreek, taprio))
> +                               return -EOPNOTSUPP;
> 
> -               if (taprio->enable)
>                         return hellcreek_port_set_schedule(ds, port, taprio);
> -
> -               return hellcreek_port_del_schedule(ds, port);
> +               case TAPRIO_CMD_DESTROY:
> +                       return hellcreek_port_del_schedule(ds, port);
> +               default:
> +                       return -EOPNOTSUPP;
> +               }
>         }
>         default:
>                 return -EOPNOTSUPP;
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 030738fef60e..5de6a27052fc 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -1411,7 +1411,7 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
> 
>         mutex_lock(&ocelot->tas_lock);
> 
> -       if (!taprio->enable) {
> +       if (taprio->cmd == TAPRIO_CMD_DESTROY) {
>                 ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
>                 ocelot_rmw_rix(ocelot, 0, QSYS_TAG_CONFIG_ENABLE,
>                                QSYS_TAG_CONFIG, port);
> @@ -1423,6 +1423,8 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
> 
>                 mutex_unlock(&ocelot->tas_lock);
>                 return 0;
> +       } else if (taprio->cmd != TAPRIO_CMD_REPLACE) {
> +               return -EOPNOTSUPP;
>         }
> 
>         ret = ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
> diff --git a/drivers/net/dsa/sja1105/sja1105_tas.c b/drivers/net/dsa/sja1105/sja1105_tas.c
> index e6153848a950..d7818710bc02 100644
> --- a/drivers/net/dsa/sja1105/sja1105_tas.c
> +++ b/drivers/net/dsa/sja1105/sja1105_tas.c
> @@ -516,10 +516,11 @@ int sja1105_setup_tc_taprio(struct dsa_switch *ds, int port,
>         /* Can't change an already configured port (must delete qdisc first).
>          * Can't delete the qdisc from an unconfigured port.
>          */
> -       if (!!tas_data->offload[port] == admin->enable)
> +       if ((!!tas_data->offload[port] && admin->cmd == TAPRIO_CMD_REPLACE) ||
> +           (!tas_data->offload[port] && admin->cmd == TAPRIO_CMD_DESTROY))
>                 return -EINVAL;
> 
> -       if (!admin->enable) {
> +       if (admin->cmd == TAPRIO_CMD_DESTROY) {
>                 taprio_offload_free(tas_data->offload[port]);
>                 tas_data->offload[port] = NULL;
> 
> @@ -528,6 +529,8 @@ int sja1105_setup_tc_taprio(struct dsa_switch *ds, int port,
>                         return rc;
> 
>                 return sja1105_static_config_reload(priv, SJA1105_SCHEDULING);
> +       } else if (admin->cmd != TAPRIO_CMD_REPLACE) {
> +               return -EOPNOTSUPP;
>         }
> 
>         /* The cycle time extension is the amount of time the last cycle from
> diff --git a/drivers/net/ethernet/engleder/tsnep_selftests.c b/drivers/net/ethernet/engleder/tsnep_selftests.c
> index 1581d6b22232..8a9145f93147 100644
> --- a/drivers/net/ethernet/engleder/tsnep_selftests.c
> +++ b/drivers/net/ethernet/engleder/tsnep_selftests.c
> @@ -329,7 +329,7 @@ static bool disable_taprio(struct tsnep_adapter *adapter)
>         int retval;
> 
>         memset(&qopt, 0, sizeof(qopt));
> -       qopt.enable = 0;
> +       qopt.cmd = TAPRIO_CMD_DESTROY;
>         retval = tsnep_tc_setup(adapter->netdev, TC_SETUP_QDISC_TAPRIO, &qopt);
>         if (retval)
>                 return false;
> @@ -360,7 +360,7 @@ static bool tsnep_test_taprio(struct tsnep_adapter *adapter)
>         for (i = 0; i < 255; i++)
>                 qopt->entries[i].command = TC_TAPRIO_CMD_SET_GATES;
> 
> -       qopt->enable = 1;
> +       qopt->cmd = TAPRIO_CMD_REPLACE;
>         qopt->base_time = ktime_set(0, 0);
>         qopt->cycle_time = 1500000;
>         qopt->cycle_time_extension = 0;
> @@ -382,7 +382,7 @@ static bool tsnep_test_taprio(struct tsnep_adapter *adapter)
>         if (!run_taprio(adapter, qopt, 100))
>                 goto failed;
> 
> -       qopt->enable = 1;
> +       qopt->cmd = TAPRIO_CMD_REPLACE;
>         qopt->base_time = ktime_set(0, 0);
>         qopt->cycle_time = 411854;
>         qopt->cycle_time_extension = 0;
> @@ -406,7 +406,7 @@ static bool tsnep_test_taprio(struct tsnep_adapter *adapter)
>         if (!run_taprio(adapter, qopt, 100))
>                 goto failed;
> 
> -       qopt->enable = 1;
> +       qopt->cmd = TAPRIO_CMD_REPLACE;
>         qopt->base_time = ktime_set(0, 0);
>         delay_base_time(adapter, qopt, 12);
>         qopt->cycle_time = 125000;
> @@ -457,7 +457,7 @@ static bool tsnep_test_taprio_change(struct tsnep_adapter *adapter)
>         for (i = 0; i < 255; i++)
>                 qopt->entries[i].command = TC_TAPRIO_CMD_SET_GATES;
> 
> -       qopt->enable = 1;
> +       qopt->cmd = TAPRIO_CMD_REPLACE;
>         qopt->base_time = ktime_set(0, 0);
>         qopt->cycle_time = 100000;
>         qopt->cycle_time_extension = 0;
> @@ -610,7 +610,7 @@ static bool tsnep_test_taprio_extension(struct tsnep_adapter *adapter)
>         for (i = 0; i < 255; i++)
>                 qopt->entries[i].command = TC_TAPRIO_CMD_SET_GATES;
> 
> -       qopt->enable = 1;
> +       qopt->cmd = TAPRIO_CMD_REPLACE;
>         qopt->base_time = ktime_set(0, 0);
>         qopt->cycle_time = 100000;
>         qopt->cycle_time_extension = 50000;
> diff --git a/drivers/net/ethernet/engleder/tsnep_tc.c b/drivers/net/ethernet/engleder/tsnep_tc.c
> index d083e6684f12..745b191a5540 100644
> --- a/drivers/net/ethernet/engleder/tsnep_tc.c
> +++ b/drivers/net/ethernet/engleder/tsnep_tc.c
> @@ -325,7 +325,7 @@ static int tsnep_taprio(struct tsnep_adapter *adapter,
>         if (!adapter->gate_control)
>                 return -EOPNOTSUPP;
> 
> -       if (!qopt->enable) {
> +       if (qopt->cmd == TAPRIO_CMD_DESTROY) {
>                 /* disable gate control if active */
>                 mutex_lock(&adapter->gate_control_lock);
> 
> @@ -337,6 +337,8 @@ static int tsnep_taprio(struct tsnep_adapter *adapter,
>                 mutex_unlock(&adapter->gate_control_lock);
> 
>                 return 0;
> +       } else if (qopt->cmd != TAPRIO_CMD_REPLACE) {
> +               return -EOPNOTSUPP;
>         }
> 
>         retval = tsnep_validate_gcl(qopt);
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> index 83c27bbbc6ed..7aad824f4da7 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> @@ -65,7 +65,7 @@ static int enetc_setup_taprio(struct net_device *ndev,
>         gcl_len = admin_conf->num_entries;
> 
>         tge = enetc_rd(hw, ENETC_PTGCR);
> -       if (!admin_conf->enable) {
> +       if (admin_conf->cmd == TAPRIO_CMD_DESTROY) {
>                 enetc_wr(hw, ENETC_PTGCR, tge & ~ENETC_PTGCR_TGE);
>                 enetc_reset_ptcmsdur(hw);
> 
> @@ -138,6 +138,10 @@ int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
>         struct enetc_ndev_priv *priv = netdev_priv(ndev);
>         int err, i;
> 
> +       if (taprio->cmd != TAPRIO_CMD_REPLACE &&
> +           taprio->cmd != TAPRIO_CMD_DESTROY)
> +               return -EOPNOTSUPP;
> +
>         /* TSD and Qbv are mutually exclusive in hardware */
>         for (i = 0; i < priv->num_tx_rings; i++)
>                 if (priv->tx_ring[i]->tsd_enable)
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index c5ef1edcf548..88145c30c919 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6113,9 +6113,18 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>         size_t n;
>         int i;
> 
> -       adapter->qbv_enable = qopt->enable;
> +       switch (qopt->cmd) {
> +       case TAPRIO_CMD_REPLACE:
> +               adapter->qbv_enable = true;
> +               break;
> +       case TAPRIO_CMD_DESTROY:
> +               adapter->qbv_enable = false;
> +               break;
> +       default:
> +               return -EOPNOTSUPP;
> +       }
> 
> -       if (!qopt->enable)
> +       if (!adapter->qbv_enable)
>                 return igc_tsn_clear_schedule(adapter);
> 
>         if (qopt->base_time < 0)
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c b/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
> index cf0cc7562d04..ee652f2d2359 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
> @@ -21,8 +21,14 @@ static int lan966x_tc_setup_qdisc_mqprio(struct lan966x_port *port,
>  static int lan966x_tc_setup_qdisc_taprio(struct lan966x_port *port,
>                                          struct tc_taprio_qopt_offload *taprio)
>  {
> -       return taprio->enable ? lan966x_taprio_add(port, taprio) :
> -                               lan966x_taprio_del(port);
> +       switch (taprio->cmd) {
> +       case TAPRIO_CMD_REPLACE:
> +               return lan966x_taprio_add(port, taprio);
> +       case TAPRIO_CMD_DESTROY:
> +               return lan966x_taprio_del(port);
> +       default:
> +               return -EOPNOTSUPP;
> +       }
>  }
> 
>  static int lan966x_tc_setup_qdisc_tbf(struct lan966x_port *port,
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 9d55226479b4..ac41ef4cbd2f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -966,8 +966,11 @@ static int tc_setup_taprio(struct stmmac_priv *priv,
>                 return -EOPNOTSUPP;
>         }
> 
> -       if (!qopt->enable)
> +       if (qopt->cmd == TAPRIO_CMD_DESTROY)
>                 goto disable;
> +       else if (qopt->cmd != TAPRIO_CMD_REPLACE)
> +               return -EOPNOTSUPP;
> +
>         if (qopt->num_entries >= dep)
>                 return -EINVAL;
>         if (!qopt->cycle_time)
> @@ -988,7 +991,7 @@ static int tc_setup_taprio(struct stmmac_priv *priv,
> 
>         mutex_lock(&priv->plat->est->lock);
>         priv->plat->est->gcl_size = size;
> -       priv->plat->est->enable = qopt->enable;
> +       priv->plat->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
>         mutex_unlock(&priv->plat->est->lock);
> 
>         for (i = 0; i < size; i++) {
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.c b/drivers/net/ethernet/ti/am65-cpsw-qos.c
> index 3a908db6e5b2..eced87fa261c 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-qos.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-qos.c
> @@ -450,7 +450,7 @@ static int am65_cpsw_configure_taprio(struct net_device *ndev,
> 
>         am65_cpsw_est_update_state(ndev);
> 
> -       if (!est_new->taprio.enable) {
> +       if (est_new->taprio.cmd == TAPRIO_CMD_DESTROY) {
>                 am65_cpsw_stop_est(ndev);
>                 return ret;
>         }
> @@ -476,7 +476,7 @@ static int am65_cpsw_configure_taprio(struct net_device *ndev,
>         am65_cpsw_est_set_sched_list(ndev, est_new);
>         am65_cpsw_port_est_assign_buf_num(ndev, est_new->buf);
> 
> -       am65_cpsw_est_set(ndev, est_new->taprio.enable);
> +       am65_cpsw_est_set(ndev, est_new->taprio.cmd == TAPRIO_CMD_REPLACE);
> 
>         if (tact == TACT_PROG) {
>                 ret = am65_cpsw_timer_set(ndev, est_new);
> @@ -520,7 +520,7 @@ static int am65_cpsw_set_taprio(struct net_device *ndev, void *type_data)
>         am65_cpsw_cp_taprio(taprio, &est_new->taprio);
>         ret = am65_cpsw_configure_taprio(ndev, est_new);
>         if (!ret) {
> -               if (taprio->enable) {
> +               if (taprio->cmd == TAPRIO_CMD_REPLACE) {
>                         devm_kfree(&ndev->dev, port->qos.est_admin);
> 
>                         port->qos.est_admin = est_new;
> @@ -564,8 +564,13 @@ static void am65_cpsw_est_link_up(struct net_device *ndev, int link_speed)
>  static int am65_cpsw_setup_taprio(struct net_device *ndev, void *type_data)
>  {
>         struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> +       struct tc_taprio_qopt_offload *taprio = type_data;
>         struct am65_cpsw_common *common = port->common;
> 
> +       if (taprio->cmd != TAPRIO_CMD_REPLACE &&
> +           taprio->cmd != TAPRIO_CMD_DESTROY)
> +               return -EOPNOTSUPP;
> +
>         if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
>                 return -ENODEV;
> 
> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
> index f436688b6efc..f5fb11da357b 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -185,6 +185,11 @@ struct tc_taprio_caps {
>         bool broken_mqprio:1;
>  };
> 
> +enum tc_taprio_qopt_cmd {
> +       TAPRIO_CMD_REPLACE,
> +       TAPRIO_CMD_DESTROY,
> +};
> +
>  struct tc_taprio_sched_entry {
>         u8 command; /* TC_TAPRIO_CMD_* */
> 
> @@ -196,7 +201,7 @@ struct tc_taprio_sched_entry {
>  struct tc_taprio_qopt_offload {
>         struct tc_mqprio_qopt_offload mqprio;
>         struct netlink_ext_ack *extack;
> -       u8 enable;
> +       enum tc_taprio_qopt_cmd cmd;
>         ktime_t base_time;
>         u64 cycle_time;
>         u64 cycle_time_extension;
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index d29e6785854d..06bf4c6355a5 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1524,7 +1524,7 @@ static int taprio_enable_offload(struct net_device *dev,
>                                "Not enough memory for enabling offload mode");
>                 return -ENOMEM;
>         }
> -       offload->enable = 1;
> +       offload->cmd = TAPRIO_CMD_REPLACE;
>         offload->extack = extack;
>         mqprio_qopt_reconstruct(dev, &offload->mqprio.qopt);
>         offload->mqprio.extack = extack;
> @@ -1572,7 +1572,7 @@ static int taprio_disable_offload(struct net_device *dev,
>                                "Not enough memory to disable offload mode");
>                 return -ENOMEM;
>         }
> -       offload->enable = 0;
> +       offload->cmd = TAPRIO_CMD_DESTROY;
> 
>         err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
>         if (err < 0) {
> --
> 2.34.1
> 
> 

-- 
/Horatiu

Re: [PATCH net-next 2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

On Tue May 30 2023, Vladimir Oltean wrote:
> Inspired from struct flow_cls_offload :: cmd, in order for taprio to be
> able to report statistics (which is future work), it seems that we need
> to drill one step further with the ndo_setup_tc(TC_SETUP_QDISC_TAPRIO)
> multiplexing, and pass the command as part of the common portion of the
> muxed structure.
>
> Since we already have an "enable" variable in tc_taprio_qopt_offload,
> refactor all drivers to check for "cmd" instead of "enable", and reject
> every other command except "replace" and "destroy" - to be future proof.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

[...]

> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
> index 595a548bb0a8..af50001ccdd4 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
> @@ -1885,13 +1885,17 @@ static int hellcreek_port_setup_tc(struct dsa_switch *ds, int port,
>  	case TC_SETUP_QDISC_TAPRIO: {
>  		struct tc_taprio_qopt_offload *taprio = type_data;
>  
> -		if (!hellcreek_validate_schedule(hellcreek, taprio))
> -			return -EOPNOTSUPP;
> +		switch (taprio->cmd) {
> +		case TAPRIO_CMD_REPLACE:
> +			if (!hellcreek_validate_schedule(hellcreek, taprio))
> +				return -EOPNOTSUPP;
>  
> -		if (taprio->enable)
>  			return hellcreek_port_set_schedule(ds, port, taprio);
> -
> -		return hellcreek_port_del_schedule(ds, port);
> +		case TAPRIO_CMD_DESTROY:
> +			return hellcreek_port_del_schedule(ds, port);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
>  	}
>  	default:
>  		return -EOPNOTSUPP;

Uhm, seems like the current code validates the schedule even for
removing a schedule which seems a bit odd. With your changes it looks
correct.

Acked-by: Kurt Kanzenbach <kurt@linutronix.de> # hellcreek

Anyway, the hellcreek device has Tx overrun counters per TC. Even though
they should be zero, simply because the hardware Length Aware Shaper is
enabled by default.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

RE: [PATCH net-next 2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum

[Zulkifli, Muhammad Husaini]

> Inspired from struct flow_cls_offload :: cmd, in order for taprio to be able to
> report statistics (which is future work), it seems that we need to drill one step
> further with the ndo_setup_tc(TC_SETUP_QDISC_TAPRIO)
> multiplexing, and pass the command as part of the common portion of the
> muxed structure.
> 
> Since we already have an "enable" variable in tc_taprio_qopt_offload, refactor
> all drivers to check for "cmd" instead of "enable", and reject every other
> command except "replace" and "destroy" - to be future proof.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

> ---
>  drivers/net/dsa/hirschmann/hellcreek.c             | 14 +++++++++-----
>  drivers/net/dsa/ocelot/felix_vsc9959.c             |  4 +++-
>  drivers/net/dsa/sja1105/sja1105_tas.c              |  7 +++++--
>  drivers/net/ethernet/engleder/tsnep_selftests.c    | 12 ++++++------
>  drivers/net/ethernet/engleder/tsnep_tc.c           |  4 +++-
>  drivers/net/ethernet/freescale/enetc/enetc_qos.c   |  6 +++++-
>  drivers/net/ethernet/intel/igc/igc_main.c          | 13 +++++++++++--
>  .../net/ethernet/microchip/lan966x/lan966x_tc.c    | 10 ++++++++--
>  drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c    |  7 +++++--
>  drivers/net/ethernet/ti/am65-cpsw-qos.c            | 11 ++++++++---
>  include/net/pkt_sched.h                            |  7 ++++++-
>  net/sched/sch_taprio.c                             |  4 ++--
>  12 files changed, 71 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c
> b/drivers/net/dsa/hirschmann/hellcreek.c
> index 595a548bb0a8..af50001ccdd4 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
> @@ -1885,13 +1885,17 @@ static int hellcreek_port_setup_tc(struct
> dsa_switch *ds, int port,
>  	case TC_SETUP_QDISC_TAPRIO: {
>  		struct tc_taprio_qopt_offload *taprio = type_data;
> 
> -		if (!hellcreek_validate_schedule(hellcreek, taprio))
> -			return -EOPNOTSUPP;
> +		switch (taprio->cmd) {
> +		case TAPRIO_CMD_REPLACE:
> +			if (!hellcreek_validate_schedule(hellcreek, taprio))
> +				return -EOPNOTSUPP;
> 
> -		if (taprio->enable)
>  			return hellcreek_port_set_schedule(ds, port, taprio);
> -
> -		return hellcreek_port_del_schedule(ds, port);
> +		case TAPRIO_CMD_DESTROY:
> +			return hellcreek_port_del_schedule(ds, port);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
>  	}
>  	default:
>  		return -EOPNOTSUPP;
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c
> b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 030738fef60e..5de6a27052fc 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -1411,7 +1411,7 @@ static int vsc9959_qos_port_tas_set(struct ocelot
> *ocelot, int port,
> 
>  	mutex_lock(&ocelot->tas_lock);
> 
> -	if (!taprio->enable) {
> +	if (taprio->cmd == TAPRIO_CMD_DESTROY) {
>  		ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
>  		ocelot_rmw_rix(ocelot, 0, QSYS_TAG_CONFIG_ENABLE,
>  			       QSYS_TAG_CONFIG, port);
> @@ -1423,6 +1423,8 @@ static int vsc9959_qos_port_tas_set(struct ocelot
> *ocelot, int port,
> 
>  		mutex_unlock(&ocelot->tas_lock);
>  		return 0;
> +	} else if (taprio->cmd != TAPRIO_CMD_REPLACE) {
> +		return -EOPNOTSUPP;
>  	}
> 
>  	ret = ocelot_port_mqprio(ocelot, port, &taprio->mqprio); diff --git
> a/drivers/net/dsa/sja1105/sja1105_tas.c
> b/drivers/net/dsa/sja1105/sja1105_tas.c
> index e6153848a950..d7818710bc02 100644
> --- a/drivers/net/dsa/sja1105/sja1105_tas.c
> +++ b/drivers/net/dsa/sja1105/sja1105_tas.c
> @@ -516,10 +516,11 @@ int sja1105_setup_tc_taprio(struct dsa_switch *ds,
> int port,
>  	/* Can't change an already configured port (must delete qdisc first).
>  	 * Can't delete the qdisc from an unconfigured port.
>  	 */
> -	if (!!tas_data->offload[port] == admin->enable)
> +	if ((!!tas_data->offload[port] && admin->cmd ==
> TAPRIO_CMD_REPLACE) ||
> +	    (!tas_data->offload[port] && admin->cmd ==
> TAPRIO_CMD_DESTROY))
>  		return -EINVAL;
> 
> -	if (!admin->enable) {
> +	if (admin->cmd == TAPRIO_CMD_DESTROY) {
>  		taprio_offload_free(tas_data->offload[port]);
>  		tas_data->offload[port] = NULL;
> 
> @@ -528,6 +529,8 @@ int sja1105_setup_tc_taprio(struct dsa_switch *ds, int
> port,
>  			return rc;
> 
>  		return sja1105_static_config_reload(priv,
> SJA1105_SCHEDULING);
> +	} else if (admin->cmd != TAPRIO_CMD_REPLACE) {
> +		return -EOPNOTSUPP;
>  	}
> 
>  	/* The cycle time extension is the amount of time the last cycle from diff
> --git a/drivers/net/ethernet/engleder/tsnep_selftests.c
> b/drivers/net/ethernet/engleder/tsnep_selftests.c
> index 1581d6b22232..8a9145f93147 100644
> --- a/drivers/net/ethernet/engleder/tsnep_selftests.c
> +++ b/drivers/net/ethernet/engleder/tsnep_selftests.c
> @@ -329,7 +329,7 @@ static bool disable_taprio(struct tsnep_adapter
> *adapter)
>  	int retval;
> 
>  	memset(&qopt, 0, sizeof(qopt));
> -	qopt.enable = 0;
> +	qopt.cmd = TAPRIO_CMD_DESTROY;
>  	retval = tsnep_tc_setup(adapter->netdev, TC_SETUP_QDISC_TAPRIO,
> &qopt);
>  	if (retval)
>  		return false;
> @@ -360,7 +360,7 @@ static bool tsnep_test_taprio(struct tsnep_adapter
> *adapter)
>  	for (i = 0; i < 255; i++)
>  		qopt->entries[i].command = TC_TAPRIO_CMD_SET_GATES;
> 
> -	qopt->enable = 1;
> +	qopt->cmd = TAPRIO_CMD_REPLACE;
>  	qopt->base_time = ktime_set(0, 0);
>  	qopt->cycle_time = 1500000;
>  	qopt->cycle_time_extension = 0;
> @@ -382,7 +382,7 @@ static bool tsnep_test_taprio(struct tsnep_adapter
> *adapter)
>  	if (!run_taprio(adapter, qopt, 100))
>  		goto failed;
> 
> -	qopt->enable = 1;
> +	qopt->cmd = TAPRIO_CMD_REPLACE;
>  	qopt->base_time = ktime_set(0, 0);
>  	qopt->cycle_time = 411854;
>  	qopt->cycle_time_extension = 0;
> @@ -406,7 +406,7 @@ static bool tsnep_test_taprio(struct tsnep_adapter
> *adapter)
>  	if (!run_taprio(adapter, qopt, 100))
>  		goto failed;
> 
> -	qopt->enable = 1;
> +	qopt->cmd = TAPRIO_CMD_REPLACE;
>  	qopt->base_time = ktime_set(0, 0);
>  	delay_base_time(adapter, qopt, 12);
>  	qopt->cycle_time = 125000;
> @@ -457,7 +457,7 @@ static bool tsnep_test_taprio_change(struct
> tsnep_adapter *adapter)
>  	for (i = 0; i < 255; i++)
>  		qopt->entries[i].command = TC_TAPRIO_CMD_SET_GATES;
> 
> -	qopt->enable = 1;
> +	qopt->cmd = TAPRIO_CMD_REPLACE;
>  	qopt->base_time = ktime_set(0, 0);
>  	qopt->cycle_time = 100000;
>  	qopt->cycle_time_extension = 0;
> @@ -610,7 +610,7 @@ static bool tsnep_test_taprio_extension(struct
> tsnep_adapter *adapter)
>  	for (i = 0; i < 255; i++)
>  		qopt->entries[i].command = TC_TAPRIO_CMD_SET_GATES;
> 
> -	qopt->enable = 1;
> +	qopt->cmd = TAPRIO_CMD_REPLACE;
>  	qopt->base_time = ktime_set(0, 0);
>  	qopt->cycle_time = 100000;
>  	qopt->cycle_time_extension = 50000;
> diff --git a/drivers/net/ethernet/engleder/tsnep_tc.c
> b/drivers/net/ethernet/engleder/tsnep_tc.c
> index d083e6684f12..745b191a5540 100644
> --- a/drivers/net/ethernet/engleder/tsnep_tc.c
> +++ b/drivers/net/ethernet/engleder/tsnep_tc.c
> @@ -325,7 +325,7 @@ static int tsnep_taprio(struct tsnep_adapter *adapter,
>  	if (!adapter->gate_control)
>  		return -EOPNOTSUPP;
> 
> -	if (!qopt->enable) {
> +	if (qopt->cmd == TAPRIO_CMD_DESTROY) {
>  		/* disable gate control if active */
>  		mutex_lock(&adapter->gate_control_lock);
> 
> @@ -337,6 +337,8 @@ static int tsnep_taprio(struct tsnep_adapter *adapter,
>  		mutex_unlock(&adapter->gate_control_lock);
> 
>  		return 0;
> +	} else if (qopt->cmd != TAPRIO_CMD_REPLACE) {
> +		return -EOPNOTSUPP;
>  	}
> 
>  	retval = tsnep_validate_gcl(qopt);
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> index 83c27bbbc6ed..7aad824f4da7 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> @@ -65,7 +65,7 @@ static int enetc_setup_taprio(struct net_device *ndev,
>  	gcl_len = admin_conf->num_entries;
> 
>  	tge = enetc_rd(hw, ENETC_PTGCR);
> -	if (!admin_conf->enable) {
> +	if (admin_conf->cmd == TAPRIO_CMD_DESTROY) {
>  		enetc_wr(hw, ENETC_PTGCR, tge & ~ENETC_PTGCR_TGE);
>  		enetc_reset_ptcmsdur(hw);
> 
> @@ -138,6 +138,10 @@ int enetc_setup_tc_taprio(struct net_device *ndev,
> void *type_data)
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>  	int err, i;
> 
> +	if (taprio->cmd != TAPRIO_CMD_REPLACE &&
> +	    taprio->cmd != TAPRIO_CMD_DESTROY)
> +		return -EOPNOTSUPP;
> +
>  	/* TSD and Qbv are mutually exclusive in hardware */
>  	for (i = 0; i < priv->num_tx_rings; i++)
>  		if (priv->tx_ring[i]->tsd_enable)
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
> b/drivers/net/ethernet/intel/igc/igc_main.c
> index c5ef1edcf548..88145c30c919 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6113,9 +6113,18 @@ static int igc_save_qbv_schedule(struct
> igc_adapter *adapter,
>  	size_t n;
>  	int i;
> 
> -	adapter->qbv_enable = qopt->enable;
> +	switch (qopt->cmd) {
> +	case TAPRIO_CMD_REPLACE:
> +		adapter->qbv_enable = true;
> +		break;
> +	case TAPRIO_CMD_DESTROY:
> +		adapter->qbv_enable = false;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> 
> -	if (!qopt->enable)
> +	if (!adapter->qbv_enable)
>  		return igc_tsn_clear_schedule(adapter);
> 
>  	if (qopt->base_time < 0)
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
> b/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
> index cf0cc7562d04..ee652f2d2359 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc.c
> @@ -21,8 +21,14 @@ static int lan966x_tc_setup_qdisc_mqprio(struct
> lan966x_port *port,  static int lan966x_tc_setup_qdisc_taprio(struct
> lan966x_port *port,
>  					 struct tc_taprio_qopt_offload
> *taprio)  {
> -	return taprio->enable ? lan966x_taprio_add(port, taprio) :
> -				lan966x_taprio_del(port);
> +	switch (taprio->cmd) {
> +	case TAPRIO_CMD_REPLACE:
> +		return lan966x_taprio_add(port, taprio);
> +	case TAPRIO_CMD_DESTROY:
> +		return lan966x_taprio_del(port);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  }
> 
>  static int lan966x_tc_setup_qdisc_tbf(struct lan966x_port *port, diff --git
> a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 9d55226479b4..ac41ef4cbd2f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -966,8 +966,11 @@ static int tc_setup_taprio(struct stmmac_priv *priv,
>  		return -EOPNOTSUPP;
>  	}
> 
> -	if (!qopt->enable)
> +	if (qopt->cmd == TAPRIO_CMD_DESTROY)
>  		goto disable;
> +	else if (qopt->cmd != TAPRIO_CMD_REPLACE)
> +		return -EOPNOTSUPP;
> +
>  	if (qopt->num_entries >= dep)
>  		return -EINVAL;
>  	if (!qopt->cycle_time)
> @@ -988,7 +991,7 @@ static int tc_setup_taprio(struct stmmac_priv *priv,
> 
>  	mutex_lock(&priv->plat->est->lock);
>  	priv->plat->est->gcl_size = size;
> -	priv->plat->est->enable = qopt->enable;
> +	priv->plat->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
>  	mutex_unlock(&priv->plat->est->lock);
> 
>  	for (i = 0; i < size; i++) {
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.c
> b/drivers/net/ethernet/ti/am65-cpsw-qos.c
> index 3a908db6e5b2..eced87fa261c 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-qos.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-qos.c
> @@ -450,7 +450,7 @@ static int am65_cpsw_configure_taprio(struct
> net_device *ndev,
> 
>  	am65_cpsw_est_update_state(ndev);
> 
> -	if (!est_new->taprio.enable) {
> +	if (est_new->taprio.cmd == TAPRIO_CMD_DESTROY) {
>  		am65_cpsw_stop_est(ndev);
>  		return ret;
>  	}
> @@ -476,7 +476,7 @@ static int am65_cpsw_configure_taprio(struct
> net_device *ndev,
>  	am65_cpsw_est_set_sched_list(ndev, est_new);
>  	am65_cpsw_port_est_assign_buf_num(ndev, est_new->buf);
> 
> -	am65_cpsw_est_set(ndev, est_new->taprio.enable);
> +	am65_cpsw_est_set(ndev, est_new->taprio.cmd ==
> TAPRIO_CMD_REPLACE);
> 
>  	if (tact == TACT_PROG) {
>  		ret = am65_cpsw_timer_set(ndev, est_new); @@ -520,7
> +520,7 @@ static int am65_cpsw_set_taprio(struct net_device *ndev, void
> *type_data)
>  	am65_cpsw_cp_taprio(taprio, &est_new->taprio);
>  	ret = am65_cpsw_configure_taprio(ndev, est_new);
>  	if (!ret) {
> -		if (taprio->enable) {
> +		if (taprio->cmd == TAPRIO_CMD_REPLACE) {
>  			devm_kfree(&ndev->dev, port->qos.est_admin);
> 
>  			port->qos.est_admin = est_new;
> @@ -564,8 +564,13 @@ static void am65_cpsw_est_link_up(struct net_device
> *ndev, int link_speed)  static int am65_cpsw_setup_taprio(struct net_device
> *ndev, void *type_data)  {
>  	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> +	struct tc_taprio_qopt_offload *taprio = type_data;
>  	struct am65_cpsw_common *common = port->common;
> 
> +	if (taprio->cmd != TAPRIO_CMD_REPLACE &&
> +	    taprio->cmd != TAPRIO_CMD_DESTROY)
> +		return -EOPNOTSUPP;
> +
>  	if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
>  		return -ENODEV;
> 
> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index
> f436688b6efc..f5fb11da357b 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -185,6 +185,11 @@ struct tc_taprio_caps {
>  	bool broken_mqprio:1;
>  };
> 
> +enum tc_taprio_qopt_cmd {
> +	TAPRIO_CMD_REPLACE,
> +	TAPRIO_CMD_DESTROY,
> +};
> +
>  struct tc_taprio_sched_entry {
>  	u8 command; /* TC_TAPRIO_CMD_* */
> 
> @@ -196,7 +201,7 @@ struct tc_taprio_sched_entry {  struct
> tc_taprio_qopt_offload {
>  	struct tc_mqprio_qopt_offload mqprio;
>  	struct netlink_ext_ack *extack;
> -	u8 enable;
> +	enum tc_taprio_qopt_cmd cmd;
>  	ktime_t base_time;
>  	u64 cycle_time;
>  	u64 cycle_time_extension;
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index
> d29e6785854d..06bf4c6355a5 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1524,7 +1524,7 @@ static int taprio_enable_offload(struct net_device
> *dev,
>  			       "Not enough memory for enabling offload mode");
>  		return -ENOMEM;
>  	}
> -	offload->enable = 1;
> +	offload->cmd = TAPRIO_CMD_REPLACE;
>  	offload->extack = extack;
>  	mqprio_qopt_reconstruct(dev, &offload->mqprio.qopt);
>  	offload->mqprio.extack = extack;
> @@ -1572,7 +1572,7 @@ static int taprio_disable_offload(struct net_device
> *dev,
>  			       "Not enough memory to disable offload mode");
>  		return -ENOMEM;
>  	}
> -	offload->enable = 0;
> +	offload->cmd = TAPRIO_CMD_DESTROY;
> 
>  	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
>  	if (err < 0) {
> --
> 2.34.1

Re: [PATCH net-next 2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum

[Gerhard Engleder]

On 30.05.23 11:19, Vladimir Oltean wrote:
> Inspired from struct flow_cls_offload :: cmd, in order for taprio to be
> able to report statistics (which is future work), it seems that we need
> to drill one step further with the ndo_setup_tc(TC_SETUP_QDISC_TAPRIO)
> multiplexing, and pass the command as part of the common portion of the
> muxed structure.
> 
> Since we already have an "enable" variable in tc_taprio_qopt_offload,
> refactor all drivers to check for "cmd" instead of "enable", and reject
> every other command except "replace" and "destroy" - to be future proof.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>   drivers/net/dsa/hirschmann/hellcreek.c             | 14 +++++++++-----
>   drivers/net/dsa/ocelot/felix_vsc9959.c             |  4 +++-
>   drivers/net/dsa/sja1105/sja1105_tas.c              |  7 +++++--
>   drivers/net/ethernet/engleder/tsnep_selftests.c    | 12 ++++++------
>   drivers/net/ethernet/engleder/tsnep_tc.c           |  4 +++-
>   drivers/net/ethernet/freescale/enetc/enetc_qos.c   |  6 +++++-
>   drivers/net/ethernet/intel/igc/igc_main.c          | 13 +++++++++++--
>   .../net/ethernet/microchip/lan966x/lan966x_tc.c    | 10 ++++++++--
>   drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c    |  7 +++++--
>   drivers/net/ethernet/ti/am65-cpsw-qos.c            | 11 ++++++++---
>   include/net/pkt_sched.h                            |  7 ++++++-
>   net/sched/sch_taprio.c                             |  4 ++--
>   12 files changed, 71 insertions(+), 28 deletions(-)

Reviewed-by: Gerhard Engleder <gerhard@engleder-embedded.com>

Re: [PATCH net-next 2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum

[Simon Horman]

On Tue, May 30, 2023 at 12:19:45PM +0300, Vladimir Oltean wrote:
> Inspired from struct flow_cls_offload :: cmd, in order for taprio to be
> able to report statistics (which is future work), it seems that we need
> to drill one step further with the ndo_setup_tc(TC_SETUP_QDISC_TAPRIO)
> multiplexing, and pass the command as part of the common portion of the
> muxed structure.
> 
> Since we already have an "enable" variable in tc_taprio_qopt_offload,
> refactor all drivers to check for "cmd" instead of "enable", and reject
> every other command except "replace" and "destroy" - to be future proof.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

...

> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c

...

> @@ -1423,6 +1423,8 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
>  
>  		mutex_unlock(&ocelot->tas_lock);
>  		return 0;
> +	} else if (taprio->cmd != TAPRIO_CMD_REPLACE) {

Hi Vladimir,

Do you need to 'mutex_unlock(&ocelot->tas_lock)' here?

> +		return -EOPNOTSUPP;
>  	}
>  
>  	ret = ocelot_port_mqprio(ocelot, port, &taprio->mqprio);

...

Re: [PATCH net-next 2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum

[Vladimir Oltean]

On Wed, May 31, 2023 at 07:08:10PM +0200, Simon Horman wrote:
> > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> 
> ...
> 
> > @@ -1423,6 +1423,8 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
> >  
> >  		mutex_unlock(&ocelot->tas_lock);
> >  		return 0;
> > +	} else if (taprio->cmd != TAPRIO_CMD_REPLACE) {
> 
> Hi Vladimir,
> 
> Do you need to 'mutex_unlock(&ocelot->tas_lock)' here?

Yup. Nice catch, thanks. I reviewed this a couple of times myself but I
missed it. I'll send a fixup patch later today, hopefully.

> > +		return -EOPNOTSUPP;
> >  	}
> >  
> >  	ret = ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
> 
> ...

[PATCH net-next 3/5] net/sched: taprio: add netlink reporting for offload statistics counters

[Vladimir Oltean]

Offloading drivers may report some additional statistics counters, some
of them even suggested by 802.1Q, like TransmissionOverrun.

In my opinion we don't have to limit ourselves to reporting counters
only globally to the Qdisc/interface, especially if the device has more
detailed reporting (per traffic class), since the more detailed info is
valuable for debugging and can help identifying who is exceeding its
time slot.

But on the other hand, some devices may not be able to report both per
TC and global stats.

So we end up reporting both ways, and use the good old ethtool_put_stat()
strategy to determine which statistics are supported by this NIC.
Statistics which aren't set are simply not reported to netlink. For this
reason, we need something dynamic (a nlattr nest) to be reported through
TCA_STATS_APP, and not something daft like the fixed-size and
inextensible struct tc_codel_xstats. A good model for xstats which are a
nlattr nest rather than a fixed struct seems to be cake.

 # Global stats
 $ tc -s qdisc show dev eth0 root
 # Per-tc stats
 $ tc -s class show dev eth0

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/pkt_sched.h        | 47 ++++++++++++++++----
 include/uapi/linux/pkt_sched.h | 10 +++++
 net/sched/sch_taprio.c         | 78 +++++++++++++++++++++++++++++++++-
 3 files changed, 126 insertions(+), 9 deletions(-)

diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index f5fb11da357b..530d33adec88 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -188,6 +188,27 @@ struct tc_taprio_caps {
 enum tc_taprio_qopt_cmd {
 	TAPRIO_CMD_REPLACE,
 	TAPRIO_CMD_DESTROY,
+	TAPRIO_CMD_STATS,
+	TAPRIO_CMD_TC_STATS,
+};
+
+/**
+ * struct tc_taprio_qopt_stats - IEEE 802.1Qbv statistics
+ * @window_drops: Frames that were dropped because they were too large to be
+ *	transmitted in any of the allotted time windows (open gates) for their
+ *	traffic class.
+ * @tx_overruns: Frames still being transmitted by the MAC after the
+ *	transmission gate associated with their traffic class has closed.
+ *	Equivalent to `12.29.1.1.2 TransmissionOverrun` from 802.1Q-2018.
+ */
+struct tc_taprio_qopt_stats {
+	u64 window_drops;
+	u64 tx_overruns;
+};
+
+struct tc_taprio_qopt_tc_stats {
+	int tc;
+	struct tc_taprio_qopt_stats stats;
 };
 
 struct tc_taprio_sched_entry {
@@ -199,16 +220,26 @@ struct tc_taprio_sched_entry {
 };
 
 struct tc_taprio_qopt_offload {
-	struct tc_mqprio_qopt_offload mqprio;
-	struct netlink_ext_ack *extack;
 	enum tc_taprio_qopt_cmd cmd;
-	ktime_t base_time;
-	u64 cycle_time;
-	u64 cycle_time_extension;
-	u32 max_sdu[TC_MAX_QUEUE];
 
-	size_t num_entries;
-	struct tc_taprio_sched_entry entries[];
+	union {
+		/* TAPRIO_CMD_STATS */
+		struct tc_taprio_qopt_stats stats;
+		/* TAPRIO_CMD_TC_STATS */
+		struct tc_taprio_qopt_tc_stats tc_stats;
+		/* TAPRIO_CMD_REPLACE */
+		struct {
+			struct tc_mqprio_qopt_offload mqprio;
+			struct netlink_ext_ack *extack;
+			ktime_t base_time;
+			u64 cycle_time;
+			u64 cycle_time_extension;
+			u32 max_sdu[TC_MAX_QUEUE];
+
+			size_t num_entries;
+			struct tc_taprio_sched_entry entries[];
+		};
+	};
 };
 
 #if IS_ENABLED(CONFIG_NET_SCH_TAPRIO)
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 51a7addc56c6..00f6ff0aff1f 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -1259,6 +1259,16 @@ enum {
 	TCA_TAPRIO_TC_ENTRY_MAX = (__TCA_TAPRIO_TC_ENTRY_CNT - 1)
 };
 
+enum {
+	TCA_TAPRIO_OFFLOAD_STATS_PAD = 1,	/* u64 */
+	TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS,	/* u64 */
+	TCA_TAPRIO_OFFLOAD_STATS_TX_OVERRUNS,	/* u64 */
+
+	/* add new constants above here */
+	__TCA_TAPRIO_OFFLOAD_STATS_CNT,
+	TCA_TAPRIO_OFFLOAD_STATS_MAX = (__TCA_TAPRIO_OFFLOAD_STATS_CNT - 1)
+};
+
 enum {
 	TCA_TAPRIO_ATTR_UNSPEC,
 	TCA_TAPRIO_ATTR_PRIOMAP, /* struct tc_mqprio_qopt */
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 06bf4c6355a5..3c4c2c334878 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -27,6 +27,8 @@
 #include <net/sock.h>
 #include <net/tcp.h>
 
+#define TAPRIO_STAT_NOT_SET	(~0ULL)
+
 #include "sch_mqprio_lib.h"
 
 static LIST_HEAD(taprio_list);
@@ -2289,6 +2291,72 @@ static int taprio_dump_tc_entries(struct sk_buff *skb,
 	return -EMSGSIZE;
 }
 
+static int taprio_put_stat(struct sk_buff *skb, u64 val, u16 attrtype)
+{
+	if (val == TAPRIO_STAT_NOT_SET)
+		return 0;
+	if (nla_put_u64_64bit(skb, attrtype, val, TCA_TAPRIO_OFFLOAD_STATS_PAD))
+		return -EMSGSIZE;
+	return 0;
+}
+
+static int taprio_dump_xstats(struct Qdisc *sch, struct gnet_dump *d,
+			      struct tc_taprio_qopt_offload *offload,
+			      struct tc_taprio_qopt_stats *stats)
+{
+	struct net_device *dev = qdisc_dev(sch);
+	const struct net_device_ops *ops;
+	struct sk_buff *skb = d->skb;
+	struct nlattr *xstats;
+	int err;
+
+	ops = qdisc_dev(sch)->netdev_ops;
+
+	/* FIXME I could use qdisc_offload_dump_helper(), but that messes
+	 * with sch->flags depending on whether the device reports taprio
+	 * stats, and I'm not sure whether that's a good idea, considering
+	 * that stats are optional to the offload itself
+	 */
+	if (!ops->ndo_setup_tc)
+		return 0;
+
+	memset(stats, 0xff, sizeof(*stats));
+
+	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
+	if (err == -EOPNOTSUPP)
+		return 0;
+	if (err)
+		return err;
+
+	xstats = nla_nest_start(skb, TCA_STATS_APP);
+	if (!xstats)
+		goto err;
+
+	if (taprio_put_stat(skb, stats->window_drops,
+			    TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS) ||
+	    taprio_put_stat(skb, stats->tx_overruns,
+			    TCA_TAPRIO_OFFLOAD_STATS_TX_OVERRUNS))
+		goto err_cancel;
+
+	nla_nest_end(skb, xstats);
+
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, xstats);
+err:
+	return -EMSGSIZE;
+}
+
+static int taprio_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
+{
+	struct tc_taprio_qopt_offload offload = {
+		.cmd = TAPRIO_CMD_STATS,
+	};
+
+	return taprio_dump_xstats(sch, d, &offload, &offload.stats);
+}
+
 static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 {
 	struct taprio_sched *q = qdisc_priv(sch);
@@ -2389,11 +2457,18 @@ static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
 {
 	struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
 	struct Qdisc *child = dev_queue->qdisc_sleeping;
+	struct tc_taprio_qopt_offload offload = {
+		.cmd = TAPRIO_CMD_TC_STATS,
+		.tc_stats = {
+			.tc = cl - 1,
+		},
+	};
 
 	if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 ||
 	    qdisc_qstats_copy(d, child) < 0)
 		return -1;
-	return 0;
+
+	return taprio_dump_xstats(sch, d, &offload, &offload.tc_stats.stats);
 }
 
 static void taprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
@@ -2440,6 +2515,7 @@ static struct Qdisc_ops taprio_qdisc_ops __read_mostly = {
 	.dequeue	= taprio_dequeue,
 	.enqueue	= taprio_enqueue,
 	.dump		= taprio_dump,
+	.dump_stats	= taprio_dump_stats,
 	.owner		= THIS_MODULE,
 };
 
-- 
2.34.1

Re: [PATCH net-next 3/5] net/sched: taprio: add netlink reporting for offload statistics counters

[Vinicius Costa Gomes]

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

> Offloading drivers may report some additional statistics counters, some
> of them even suggested by 802.1Q, like TransmissionOverrun.
>
> In my opinion we don't have to limit ourselves to reporting counters
> only globally to the Qdisc/interface, especially if the device has more
> detailed reporting (per traffic class), since the more detailed info is
> valuable for debugging and can help identifying who is exceeding its
> time slot.
>
> But on the other hand, some devices may not be able to report both per
> TC and global stats.
>
> So we end up reporting both ways, and use the good old ethtool_put_stat()
> strategy to determine which statistics are supported by this NIC.
> Statistics which aren't set are simply not reported to netlink. For this
> reason, we need something dynamic (a nlattr nest) to be reported through
> TCA_STATS_APP, and not something daft like the fixed-size and
> inextensible struct tc_codel_xstats. A good model for xstats which are a
> nlattr nest rather than a fixed struct seems to be cake.
>
>  # Global stats
>  $ tc -s qdisc show dev eth0 root
>  # Per-tc stats
>  $ tc -s class show dev eth0
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  include/net/pkt_sched.h        | 47 ++++++++++++++++----
>  include/uapi/linux/pkt_sched.h | 10 +++++
>  net/sched/sch_taprio.c         | 78 +++++++++++++++++++++++++++++++++-
>  3 files changed, 126 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
> index f5fb11da357b..530d33adec88 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -188,6 +188,27 @@ struct tc_taprio_caps {
>  enum tc_taprio_qopt_cmd {
>  	TAPRIO_CMD_REPLACE,
>  	TAPRIO_CMD_DESTROY,
> +	TAPRIO_CMD_STATS,
> +	TAPRIO_CMD_TC_STATS,
> +};
> +
> +/**
> + * struct tc_taprio_qopt_stats - IEEE 802.1Qbv statistics
> + * @window_drops: Frames that were dropped because they were too large to be
> + *	transmitted in any of the allotted time windows (open gates) for their
> + *	traffic class.
> + * @tx_overruns: Frames still being transmitted by the MAC after the
> + *	transmission gate associated with their traffic class has closed.
> + *	Equivalent to `12.29.1.1.2 TransmissionOverrun` from 802.1Q-2018.
> + */
> +struct tc_taprio_qopt_stats {
> +	u64 window_drops;
> +	u64 tx_overruns;
> +};
> +
> +struct tc_taprio_qopt_tc_stats {
> +	int tc;
> +	struct tc_taprio_qopt_stats stats;
>  };
>  
>  struct tc_taprio_sched_entry {
> @@ -199,16 +220,26 @@ struct tc_taprio_sched_entry {
>  };
>  
>  struct tc_taprio_qopt_offload {
> -	struct tc_mqprio_qopt_offload mqprio;
> -	struct netlink_ext_ack *extack;
>  	enum tc_taprio_qopt_cmd cmd;
> -	ktime_t base_time;
> -	u64 cycle_time;
> -	u64 cycle_time_extension;
> -	u32 max_sdu[TC_MAX_QUEUE];
>  
> -	size_t num_entries;
> -	struct tc_taprio_sched_entry entries[];
> +	union {
> +		/* TAPRIO_CMD_STATS */
> +		struct tc_taprio_qopt_stats stats;
> +		/* TAPRIO_CMD_TC_STATS */
> +		struct tc_taprio_qopt_tc_stats tc_stats;
> +		/* TAPRIO_CMD_REPLACE */
> +		struct {
> +			struct tc_mqprio_qopt_offload mqprio;
> +			struct netlink_ext_ack *extack;
> +			ktime_t base_time;
> +			u64 cycle_time;
> +			u64 cycle_time_extension;
> +			u32 max_sdu[TC_MAX_QUEUE];
> +
> +			size_t num_entries;
> +			struct tc_taprio_sched_entry entries[];
> +		};
> +	};
>  };
>  
>  #if IS_ENABLED(CONFIG_NET_SCH_TAPRIO)
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 51a7addc56c6..00f6ff0aff1f 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -1259,6 +1259,16 @@ enum {
>  	TCA_TAPRIO_TC_ENTRY_MAX = (__TCA_TAPRIO_TC_ENTRY_CNT - 1)
>  };
>  
> +enum {
> +	TCA_TAPRIO_OFFLOAD_STATS_PAD = 1,	/* u64 */
> +	TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS,	/* u64 */
> +	TCA_TAPRIO_OFFLOAD_STATS_TX_OVERRUNS,	/* u64 */
> +
> +	/* add new constants above here */
> +	__TCA_TAPRIO_OFFLOAD_STATS_CNT,
> +	TCA_TAPRIO_OFFLOAD_STATS_MAX = (__TCA_TAPRIO_OFFLOAD_STATS_CNT - 1)
> +};
> +
>  enum {
>  	TCA_TAPRIO_ATTR_UNSPEC,
>  	TCA_TAPRIO_ATTR_PRIOMAP, /* struct tc_mqprio_qopt */
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 06bf4c6355a5..3c4c2c334878 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -27,6 +27,8 @@
>  #include <net/sock.h>
>  #include <net/tcp.h>
>  
> +#define TAPRIO_STAT_NOT_SET	(~0ULL)
> +
>  #include "sch_mqprio_lib.h"
>  
>  static LIST_HEAD(taprio_list);
> @@ -2289,6 +2291,72 @@ static int taprio_dump_tc_entries(struct sk_buff *skb,
>  	return -EMSGSIZE;
>  }
>  
> +static int taprio_put_stat(struct sk_buff *skb, u64 val, u16 attrtype)
> +{
> +	if (val == TAPRIO_STAT_NOT_SET)
> +		return 0;
> +	if (nla_put_u64_64bit(skb, attrtype, val, TCA_TAPRIO_OFFLOAD_STATS_PAD))
> +		return -EMSGSIZE;
> +	return 0;
> +}
> +
> +static int taprio_dump_xstats(struct Qdisc *sch, struct gnet_dump *d,
> +			      struct tc_taprio_qopt_offload *offload,
> +			      struct tc_taprio_qopt_stats *stats)
> +{
> +	struct net_device *dev = qdisc_dev(sch);
> +	const struct net_device_ops *ops;
> +	struct sk_buff *skb = d->skb;
> +	struct nlattr *xstats;
> +	int err;
> +
> +	ops = qdisc_dev(sch)->netdev_ops;
> +
> +	/* FIXME I could use qdisc_offload_dump_helper(), but that messes
> +	 * with sch->flags depending on whether the device reports taprio
> +	 * stats, and I'm not sure whether that's a good idea, considering
> +	 * that stats are optional to the offload itself
> +	 */
> +	if (!ops->ndo_setup_tc)
> +		return 0;
> +
> +	memset(stats, 0xff, sizeof(*stats));

The only part that I didn't like, at first, was this, that the
initialization of the offload struct is divided into two parts: one to
set the command/tc, and one to set the "invalid/not set" value to all
stats fields.

I was thinking of adding a macro to do initialization of the stats
fields, but it has a problem that it won't complain when a new field is
added. Your solution should always work. I don't have better
suggestions.

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

> +
> +	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
> +	if (err == -EOPNOTSUPP)
> +		return 0;
> +	if (err)
> +		return err;
> +
> +	xstats = nla_nest_start(skb, TCA_STATS_APP);
> +	if (!xstats)
> +		goto err;
> +
> +	if (taprio_put_stat(skb, stats->window_drops,
> +			    TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS) ||
> +	    taprio_put_stat(skb, stats->tx_overruns,
> +			    TCA_TAPRIO_OFFLOAD_STATS_TX_OVERRUNS))
> +		goto err_cancel;
> +
> +	nla_nest_end(skb, xstats);
> +
> +	return 0;
> +
> +err_cancel:
> +	nla_nest_cancel(skb, xstats);
> +err:
> +	return -EMSGSIZE;
> +}
> +
> +static int taprio_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
> +{
> +	struct tc_taprio_qopt_offload offload = {
> +		.cmd = TAPRIO_CMD_STATS,
> +	};
> +
> +	return taprio_dump_xstats(sch, d, &offload, &offload.stats);
> +}
> +
>  static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
>  {
>  	struct taprio_sched *q = qdisc_priv(sch);
> @@ -2389,11 +2457,18 @@ static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
>  {
>  	struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
>  	struct Qdisc *child = dev_queue->qdisc_sleeping;
> +	struct tc_taprio_qopt_offload offload = {
> +		.cmd = TAPRIO_CMD_TC_STATS,
> +		.tc_stats = {
> +			.tc = cl - 1,
> +		},
> +	};
>  
>  	if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 ||
>  	    qdisc_qstats_copy(d, child) < 0)
>  		return -1;
> -	return 0;
> +
> +	return taprio_dump_xstats(sch, d, &offload, &offload.tc_stats.stats);
>  }
>  
>  static void taprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
> @@ -2440,6 +2515,7 @@ static struct Qdisc_ops taprio_qdisc_ops __read_mostly = {
>  	.dequeue	= taprio_dequeue,
>  	.enqueue	= taprio_enqueue,
>  	.dump		= taprio_dump,
> +	.dump_stats	= taprio_dump_stats,
>  	.owner		= THIS_MODULE,
>  };
>  
> -- 
> 2.34.1
>

-- 
Vinicius

Re: [PATCH net-next 3/5] net/sched: taprio: add netlink reporting for offload statistics counters

[Vladimir Oltean]

On Tue, May 30, 2023 at 03:52:17PM -0700, Vinicius Costa Gomes wrote:
> > +	memset(stats, 0xff, sizeof(*stats));
> 
> The only part that I didn't like, at first, was this, that the
> initialization of the offload struct is divided into two parts: one to
> set the command/tc, and one to set the "invalid/not set" value to all
> stats fields.
> 
> I was thinking of adding a macro to do initialization of the stats
> fields, but it has a problem that it won't complain when a new field is
> added. Your solution should always work. I don't have better
> suggestions.

Right, it's no coincidence that it's where it is and the way it is.
Again, I drew inspiration from stats_prepare_data() in ethtool.

> Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

Thanks for the review.

RE: [PATCH net-next 3/5] net/sched: taprio: add netlink reporting for offload statistics counters

[Zulkifli, Muhammad Husaini]

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Sent: Tuesday, 30 May, 2023 5:20 PM
> To: netdev@vger.kernel.org
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Hadi Salim, Jamal <jhs@mojatatu.com>; Cong Wang
> <xiyou.wangcong@gmail.com>; Jiri Pirko <jiri@resnulli.us>; Gomes, Vinicius
> <vinicius.gomes@intel.com>; Kurt Kanzenbach <kurt@linutronix.de>;
> Gerhard Engleder <gerhard@engleder-embedded.com>; Nambiar, Amritha
> <amritha.nambiar@intel.com>; Ferenc Fejes <ferenc.fejes@ericsson.com>;
> Xiaoliang Yang <xiaoliang.yang_1@nxp.com>; Roger Quadros
> <rogerq@kernel.org>; Pranavi Somisetty <pranavi.somisetty@amd.com>;
> Harini Katakam <harini.katakam@amd.com>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Sit, Michael Wei Hong
> <michael.wei.hong.sit@intel.com>; Ismail, Mohammad Athari
> <mohammad.athari.ismail@intel.com>; Oleksij Rempel <linux@rempel-
> privat.de>; Keller, Jacob E <jacob.e.keller@intel.com>; linux-
> kernel@vger.kernel.org; Andrew Lunn <andrew@lunn.ch>; Florian Fainelli
> <f.fainelli@gmail.com>; Claudiu Manoil <claudiu.manoil@nxp.com>;
> Alexandre Belloni <alexandre.belloni@bootlin.com>;
> UNGLinuxDriver@microchip.com; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Horatiu Vultur
> <horatiu.vultur@microchip.com>; Jose Abreu <joabreu@synopsys.com>;
> Maxime Coquelin <mcoquelin.stm32@gmail.com>; intel-wired-
> lan@lists.osuosl.org; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@intel.com>
> Subject: [PATCH net-next 3/5] net/sched: taprio: add netlink reporting for
> offload statistics counters
> 
> Offloading drivers may report some additional statistics counters, some of
> them even suggested by 802.1Q, like TransmissionOverrun.
> 
> In my opinion we don't have to limit ourselves to reporting counters only
> globally to the Qdisc/interface, especially if the device has more detailed
> reporting (per traffic class), since the more detailed info is valuable for
> debugging and can help identifying who is exceeding its time slot.
> 
> But on the other hand, some devices may not be able to report both per TC
> and global stats.
> 
> So we end up reporting both ways, and use the good old ethtool_put_stat()
> strategy to determine which statistics are supported by this NIC.
> Statistics which aren't set are simply not reported to netlink. For this reason,
> we need something dynamic (a nlattr nest) to be reported through
> TCA_STATS_APP, and not something daft like the fixed-size and inextensible
> struct tc_codel_xstats. A good model for xstats which are a nlattr nest rather
> than a fixed struct seems to be cake.
> 
>  # Global stats
>  $ tc -s qdisc show dev eth0 root
>  # Per-tc stats
>  $ tc -s class show dev eth0
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Tested-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

Thanks for the patch :) 
I applied my changes on top of your patch series, and it worked for both 
TAPRIO_CMD_STATS and TAPRIO_CMD_TC_STATS.  Awesome!

> ---
>  include/net/pkt_sched.h        | 47 ++++++++++++++++----
>  include/uapi/linux/pkt_sched.h | 10 +++++
>  net/sched/sch_taprio.c         | 78 +++++++++++++++++++++++++++++++++-
>  3 files changed, 126 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index
> f5fb11da357b..530d33adec88 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -188,6 +188,27 @@ struct tc_taprio_caps {  enum tc_taprio_qopt_cmd {
>  	TAPRIO_CMD_REPLACE,
>  	TAPRIO_CMD_DESTROY,
> +	TAPRIO_CMD_STATS,
> +	TAPRIO_CMD_TC_STATS,
> +};
> +
> +/**
> + * struct tc_taprio_qopt_stats - IEEE 802.1Qbv statistics
> + * @window_drops: Frames that were dropped because they were too large
> to be
> + *	transmitted in any of the allotted time windows (open gates) for their
> + *	traffic class.
> + * @tx_overruns: Frames still being transmitted by the MAC after the
> + *	transmission gate associated with their traffic class has closed.
> + *	Equivalent to `12.29.1.1.2 TransmissionOverrun` from 802.1Q-2018.
> + */
> +struct tc_taprio_qopt_stats {
> +	u64 window_drops;
> +	u64 tx_overruns;
> +};
> +
> +struct tc_taprio_qopt_tc_stats {
> +	int tc;
> +	struct tc_taprio_qopt_stats stats;
>  };
> 
>  struct tc_taprio_sched_entry {
> @@ -199,16 +220,26 @@ struct tc_taprio_sched_entry {  };
> 
>  struct tc_taprio_qopt_offload {
> -	struct tc_mqprio_qopt_offload mqprio;
> -	struct netlink_ext_ack *extack;
>  	enum tc_taprio_qopt_cmd cmd;
> -	ktime_t base_time;
> -	u64 cycle_time;
> -	u64 cycle_time_extension;
> -	u32 max_sdu[TC_MAX_QUEUE];
> 
> -	size_t num_entries;
> -	struct tc_taprio_sched_entry entries[];
> +	union {
> +		/* TAPRIO_CMD_STATS */
> +		struct tc_taprio_qopt_stats stats;
> +		/* TAPRIO_CMD_TC_STATS */
> +		struct tc_taprio_qopt_tc_stats tc_stats;
> +		/* TAPRIO_CMD_REPLACE */
> +		struct {
> +			struct tc_mqprio_qopt_offload mqprio;
> +			struct netlink_ext_ack *extack;
> +			ktime_t base_time;
> +			u64 cycle_time;
> +			u64 cycle_time_extension;
> +			u32 max_sdu[TC_MAX_QUEUE];
> +
> +			size_t num_entries;
> +			struct tc_taprio_sched_entry entries[];
> +		};
> +	};
>  };
> 
>  #if IS_ENABLED(CONFIG_NET_SCH_TAPRIO)
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 51a7addc56c6..00f6ff0aff1f 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -1259,6 +1259,16 @@ enum {
>  	TCA_TAPRIO_TC_ENTRY_MAX = (__TCA_TAPRIO_TC_ENTRY_CNT - 1)
> };
> 
> +enum {
> +	TCA_TAPRIO_OFFLOAD_STATS_PAD = 1,	/* u64 */
> +	TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS,	/* u64 */
> +	TCA_TAPRIO_OFFLOAD_STATS_TX_OVERRUNS,	/* u64 */
> +
> +	/* add new constants above here */
> +	__TCA_TAPRIO_OFFLOAD_STATS_CNT,
> +	TCA_TAPRIO_OFFLOAD_STATS_MAX =
> (__TCA_TAPRIO_OFFLOAD_STATS_CNT - 1) };
> +
>  enum {
>  	TCA_TAPRIO_ATTR_UNSPEC,
>  	TCA_TAPRIO_ATTR_PRIOMAP, /* struct tc_mqprio_qopt */ diff --git
> a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index
> 06bf4c6355a5..3c4c2c334878 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -27,6 +27,8 @@
>  #include <net/sock.h>
>  #include <net/tcp.h>
> 
> +#define TAPRIO_STAT_NOT_SET	(~0ULL)
> +
>  #include "sch_mqprio_lib.h"
> 
>  static LIST_HEAD(taprio_list);
> @@ -2289,6 +2291,72 @@ static int taprio_dump_tc_entries(struct sk_buff
> *skb,
>  	return -EMSGSIZE;
>  }
> 
> +static int taprio_put_stat(struct sk_buff *skb, u64 val, u16 attrtype)
> +{
> +	if (val == TAPRIO_STAT_NOT_SET)
> +		return 0;
> +	if (nla_put_u64_64bit(skb, attrtype, val,
> TCA_TAPRIO_OFFLOAD_STATS_PAD))
> +		return -EMSGSIZE;
> +	return 0;
> +}
> +
> +static int taprio_dump_xstats(struct Qdisc *sch, struct gnet_dump *d,
> +			      struct tc_taprio_qopt_offload *offload,
> +			      struct tc_taprio_qopt_stats *stats) {
> +	struct net_device *dev = qdisc_dev(sch);
> +	const struct net_device_ops *ops;
> +	struct sk_buff *skb = d->skb;
> +	struct nlattr *xstats;
> +	int err;
> +
> +	ops = qdisc_dev(sch)->netdev_ops;
> +
> +	/* FIXME I could use qdisc_offload_dump_helper(), but that messes
> +	 * with sch->flags depending on whether the device reports taprio
> +	 * stats, and I'm not sure whether that's a good idea, considering
> +	 * that stats are optional to the offload itself
> +	 */
> +	if (!ops->ndo_setup_tc)
> +		return 0;
> +
> +	memset(stats, 0xff, sizeof(*stats));
> +
> +	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
> +	if (err == -EOPNOTSUPP)
> +		return 0;
> +	if (err)
> +		return err;
> +
> +	xstats = nla_nest_start(skb, TCA_STATS_APP);
> +	if (!xstats)
> +		goto err;
> +
> +	if (taprio_put_stat(skb, stats->window_drops,
> +			    TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS)
> ||
> +	    taprio_put_stat(skb, stats->tx_overruns,
> +			    TCA_TAPRIO_OFFLOAD_STATS_TX_OVERRUNS))
> +		goto err_cancel;
> +
> +	nla_nest_end(skb, xstats);
> +
> +	return 0;
> +
> +err_cancel:
> +	nla_nest_cancel(skb, xstats);
> +err:
> +	return -EMSGSIZE;
> +}
> +
> +static int taprio_dump_stats(struct Qdisc *sch, struct gnet_dump *d) {
> +	struct tc_taprio_qopt_offload offload = {
> +		.cmd = TAPRIO_CMD_STATS,
> +	};
> +
> +	return taprio_dump_xstats(sch, d, &offload, &offload.stats); }
> +
>  static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)  {
>  	struct taprio_sched *q = qdisc_priv(sch); @@ -2389,11 +2457,18 @@
> static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,  {
>  	struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
>  	struct Qdisc *child = dev_queue->qdisc_sleeping;
> +	struct tc_taprio_qopt_offload offload = {
> +		.cmd = TAPRIO_CMD_TC_STATS,
> +		.tc_stats = {
> +			.tc = cl - 1,
> +		},
> +	};
> 
>  	if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 ||
>  	    qdisc_qstats_copy(d, child) < 0)
>  		return -1;
> -	return 0;
> +
> +	return taprio_dump_xstats(sch, d, &offload, &offload.tc_stats.stats);
>  }
> 
>  static void taprio_walk(struct Qdisc *sch, struct qdisc_walker *arg) @@ -
> 2440,6 +2515,7 @@ static struct Qdisc_ops taprio_qdisc_ops __read_mostly
> = {
>  	.dequeue	= taprio_dequeue,
>  	.enqueue	= taprio_enqueue,
>  	.dump		= taprio_dump,
> +	.dump_stats	= taprio_dump_stats,
>  	.owner		= THIS_MODULE,
>  };
> 
> --
> 2.34.1

[PATCH net-next 4/5] net: enetc: refactor enetc_setup_tc_taprio() to have a switch/case for cmd

[Vladimir Oltean]

Make enetc_setup_tc_taprio() more amenable to future extensions, like
reporting statistics.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c  |  3 +-
 drivers/net/ethernet/freescale/enetc/enetc.h  |  1 +
 .../net/ethernet/freescale/enetc/enetc_qos.c  | 79 ++++++++++++-------
 3 files changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 63854294ac33..3aa31a760657 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -2624,7 +2624,7 @@ static void enetc_debug_tx_ring_prios(struct enetc_ndev_priv *priv)
 			   priv->tx_ring[i]->prio);
 }
 
-static void enetc_reset_tc_mqprio(struct net_device *ndev)
+void enetc_reset_tc_mqprio(struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	struct enetc_hw *hw = &priv->si->hw;
@@ -2649,6 +2649,7 @@ static void enetc_reset_tc_mqprio(struct net_device *ndev)
 
 	enetc_change_preemptible_tcs(priv, 0);
 }
+EXPORT_SYMBOL_GPL(enetc_reset_tc_mqprio);
 
 int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
 {
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index c97a8e3d7a7f..8577cf7699a0 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -429,6 +429,7 @@ struct net_device_stats *enetc_get_stats(struct net_device *ndev);
 void enetc_set_features(struct net_device *ndev, netdev_features_t features);
 int enetc_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd);
 int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data);
+void enetc_reset_tc_mqprio(struct net_device *ndev);
 int enetc_setup_bpf(struct net_device *ndev, struct netdev_bpf *bpf);
 int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 		   struct xdp_frame **frames, u32 flags);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 7aad824f4da7..2b8fdfffd02d 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -43,10 +43,9 @@ void enetc_sched_speed_set(struct enetc_ndev_priv *priv, int speed)
 	enetc_port_wr(hw, ENETC_PMR, (tmp & ~ENETC_PMR_PSPEED_MASK) | pspeed);
 }
 
-static int enetc_setup_taprio(struct net_device *ndev,
+static int enetc_setup_taprio(struct enetc_ndev_priv *priv,
 			      struct tc_taprio_qopt_offload *admin_conf)
 {
-	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	struct enetc_hw *hw = &priv->si->hw;
 	struct enetc_cbd cbd = {.cmd = 0};
 	struct tgs_gcl_conf *gcl_config;
@@ -60,19 +59,13 @@ static int enetc_setup_taprio(struct net_device *ndev,
 	int err;
 	int i;
 
+	/* TSD and Qbv are mutually exclusive in hardware */
+	for (i = 0; i < priv->num_tx_rings; i++)
+		if (priv->tx_ring[i]->tsd_enable)
+			return -EBUSY;
+
 	if (admin_conf->num_entries > enetc_get_max_gcl_len(hw))
 		return -EINVAL;
-	gcl_len = admin_conf->num_entries;
-
-	tge = enetc_rd(hw, ENETC_PTGCR);
-	if (admin_conf->cmd == TAPRIO_CMD_DESTROY) {
-		enetc_wr(hw, ENETC_PTGCR, tge & ~ENETC_PTGCR_TGE);
-		enetc_reset_ptcmsdur(hw);
-
-		priv->active_offloads &= ~ENETC_F_QBV;
-
-		return 0;
-	}
 
 	if (admin_conf->cycle_time > U32_MAX ||
 	    admin_conf->cycle_time_extension > U32_MAX)
@@ -82,6 +75,7 @@ static int enetc_setup_taprio(struct net_device *ndev,
 	 * control BD descriptor.
 	 */
 	gcl_config = &cbd.gcl_conf;
+	gcl_len = admin_conf->num_entries;
 
 	data_size = struct_size(gcl_data, entry, gcl_len);
 	tmp = enetc_cbd_alloc_data_mem(priv->si, &cbd, data_size,
@@ -115,6 +109,7 @@ static int enetc_setup_taprio(struct net_device *ndev,
 	cbd.cls = BDCR_CMD_PORT_GCL;
 	cbd.status_flags = 0;
 
+	tge = enetc_rd(hw, ENETC_PTGCR);
 	enetc_wr(hw, ENETC_PTGCR, tge | ENETC_PTGCR_TGE);
 
 	err = enetc_send_cmd(priv->si, &cbd);
@@ -132,29 +127,57 @@ static int enetc_setup_taprio(struct net_device *ndev,
 	return 0;
 }
 
-int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
+static void enetc_reset_taprio(struct enetc_ndev_priv *priv)
+{
+	struct enetc_hw *hw = &priv->si->hw;
+	u32 val;
+
+	val = enetc_rd(hw, ENETC_PTGCR);
+	enetc_wr(hw, ENETC_PTGCR, val & ~ENETC_PTGCR_TGE);
+	enetc_reset_ptcmsdur(hw);
+
+	priv->active_offloads &= ~ENETC_F_QBV;
+}
+
+static void enetc_taprio_destroy(struct net_device *ndev)
 {
-	struct tc_taprio_qopt_offload *taprio = type_data;
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
-	int err, i;
 
-	if (taprio->cmd != TAPRIO_CMD_REPLACE &&
-	    taprio->cmd != TAPRIO_CMD_DESTROY)
-		return -EOPNOTSUPP;
+	enetc_reset_taprio(priv);
+	enetc_reset_tc_mqprio(ndev);
+}
 
-	/* TSD and Qbv are mutually exclusive in hardware */
-	for (i = 0; i < priv->num_tx_rings; i++)
-		if (priv->tx_ring[i]->tsd_enable)
-			return -EBUSY;
+static int enetc_taprio_replace(struct net_device *ndev,
+				struct tc_taprio_qopt_offload *offload)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	int err;
 
-	err = enetc_setup_tc_mqprio(ndev, &taprio->mqprio);
+	err = enetc_setup_tc_mqprio(ndev, &offload->mqprio);
 	if (err)
 		return err;
 
-	err = enetc_setup_taprio(ndev, taprio);
-	if (err) {
-		taprio->mqprio.qopt.num_tc = 0;
-		enetc_setup_tc_mqprio(ndev, &taprio->mqprio);
+	err = enetc_setup_taprio(priv, offload);
+	if (err)
+		enetc_reset_tc_mqprio(ndev);
+
+	return err;
+}
+
+int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
+{
+	struct tc_taprio_qopt_offload *offload = type_data;
+	int err = 0;
+
+	switch (offload->cmd) {
+	case TAPRIO_CMD_REPLACE:
+		err = enetc_taprio_replace(ndev, offload);
+		break;
+	case TAPRIO_CMD_DESTROY:
+		enetc_taprio_destroy(ndev);
+		break;
+	default:
+		err = -EOPNOTSUPP;
 	}
 
 	return err;
-- 
2.34.1

[PATCH net-next 5/5] net: enetc: report statistics counters for taprio

[Vladimir Oltean]

Report the "win_drop" counter from the unstructured ethtool -S as
TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS to the Qdisc layer. It is
available both as a global counter as well as a per-TC one.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../net/ethernet/freescale/enetc/enetc_qos.c  | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 2b8fdfffd02d..71157eba1fbe 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -147,6 +147,35 @@ static void enetc_taprio_destroy(struct net_device *ndev)
 	enetc_reset_tc_mqprio(ndev);
 }
 
+static void enetc_taprio_stats(struct net_device *ndev,
+			       struct tc_taprio_qopt_stats *stats)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	u64 window_drops = 0;
+	int i;
+
+	for (i = 0; i < priv->num_tx_rings; i++)
+		window_drops += priv->tx_ring[i]->stats.win_drop;
+
+	stats->window_drops = window_drops;
+}
+
+static void enetc_taprio_tc_stats(struct net_device *ndev,
+				  struct tc_taprio_qopt_tc_stats *tc_stats)
+{
+	struct tc_taprio_qopt_stats *stats = &tc_stats->stats;
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	int tc = tc_stats->tc;
+	u64 window_drops = 0;
+	int i;
+
+	for (i = 0; i < priv->num_tx_rings; i++)
+		if (priv->tx_ring[i]->prio == tc)
+			window_drops += priv->tx_ring[i]->stats.win_drop;
+
+	stats->window_drops = window_drops;
+}
+
 static int enetc_taprio_replace(struct net_device *ndev,
 				struct tc_taprio_qopt_offload *offload)
 {
@@ -176,6 +205,12 @@ int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
 	case TAPRIO_CMD_DESTROY:
 		enetc_taprio_destroy(ndev);
 		break;
+	case TAPRIO_CMD_STATS:
+		enetc_taprio_stats(ndev, &offload->stats);
+		break;
+	case TAPRIO_CMD_TC_STATS:
+		enetc_taprio_tc_stats(ndev, &offload->tc_stats);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 	}
-- 
2.34.1

Re: [PATCH net-next 0/5] xstats for tc-taprio

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue, 30 May 2023 12:19:43 +0300 you wrote:
> As a result of this discussion:
> https://lore.kernel.org/intel-wired-lan/20230411055543.24177-1-muhammad.husaini.zulkifli@intel.com/
> 
> it became apparent that tc-taprio should make an effort to standardize
> statistics counters related to the 802.1Qbv scheduling as implemented
> by the NIC. I'm presenting here one counter suggested by the standard,
> and one counter defined by the NXP ENETC controller from LS1028A. Both
> counters are reported globally and per traffic class - drivers get
> different callbacks for reporting both of these, and get to choose what
> to report in both cases.
> 
> [...]

Here is the summary with links:
  - [net-next,1/5] net/sched: taprio: don't overwrite "sch" variable in taprio_dump_class_stats()
    https://git.kernel.org/netdev/net-next/c/dced11ef84fb
  - [net-next,2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum
    https://git.kernel.org/netdev/net-next/c/2d800bc500fb
  - [net-next,3/5] net/sched: taprio: add netlink reporting for offload statistics counters
    https://git.kernel.org/netdev/net-next/c/6c1adb650c8d
  - [net-next,4/5] net: enetc: refactor enetc_setup_tc_taprio() to have a switch/case for cmd
    https://git.kernel.org/netdev/net-next/c/5353599aa745
  - [net-next,5/5] net: enetc: report statistics counters for taprio
    https://git.kernel.org/netdev/net-next/c/4802fca8d1af

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html