2023-05-30 09:23:18

by Vladimir Oltean

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

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 <[email protected]>
---
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



2023-05-30 12:07:28

by Horatiu Vultur

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

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 <[email protected]>

Reviewed-by: Horatiu Vultur <[email protected]> # 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

2023-05-30 12:28:34

by Kurt Kanzenbach

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

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 <[email protected]>
> ---

[...]

> 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 <[email protected]> # 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


Attachments:
signature.asc (877.00 B)
Subject: RE: [PATCH net-next 2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum


> 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 <[email protected]>

Reviewed-by: Muhammad Husaini Zulkifli <[email protected]>

> ---
> 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


2023-05-30 22:42:05

by Gerhard Engleder

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

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 <[email protected]>
> ---
> 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 <[email protected]>

2023-05-31 17:18:46

by Vladimir Oltean

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

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);
>
> ...

2023-05-31 17:42:21

by Simon Horman

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

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 <[email protected]>

...

> --- 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);

...