netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver
@ 2023-07-05 10:44 Vladimir Oltean
  2023-07-05 10:44 ` [PATCH net 1/3] net: mscc: ocelot: extend ocelot->fwd_domain_lock to cover ocelot->tas_lock Vladimir Oltean
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Vladimir Oltean @ 2023-07-05 10:44 UTC (permalink / raw)
  To: netdev
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Xiaoliang Yang, linux-kernel

It has been reported that preemptible traffic doesn't completely behave
as expected. Namely, large packets should be able to be squeezed
(through fragmentation) through taprio time slots smaller than the
transmission time of the full frame. That does not happen due to logic
in the driver (for oversize frame dropping with taprio) that was not
updated in order for this use case to work.

I am not sure whether it qualifies as "net" material, because some
structural changes are involved, and it is a "never worked" scenario.
OTOH, this is a complaint coming from users for a v6.4 kernel.
It's up to maintainers to decide whether this series can be considered;
I've submitted it as non-RFC in the optimistic case that it will be :)

Demo script illustrating the issue below.

#!/bin/bash

add_taprio()
{
	local ifname=$1

	echo "Creating root taprio"
	tc qdisc replace dev $ifname handle 8001: parent root stab overhead 24 taprio \
		num_tc 8 \
		map 0 1 2 3 4 5 6 7 \
		queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \
		base-time 0 \
		sched-entry S 01 1216 \
		sched-entry S fe 12368 \
		fp P E E E E E E E \
		flags 0x2
}

remove_taprio()
{
	local ifname=$1

	echo "Removing taprio"
	tc qdisc del dev $ifname root
}

ip netns add ns0
ip link set eno0 netns ns0 && ip -n ns0 link set eno0 up && ip -n ns0 addr add 192.168.100.1/24 dev eno0
ip addr add 192.168.100.2/24 dev swp0 && ip link set swp0 up
ip netns exec ns0 ethtool --set-mm eno0 pmac-enabled on verify-enabled off tx-enabled on
ethtool --set-mm swp0 pmac-enabled on verify-enabled off tx-enabled on
add_taprio swp0

ping 192.168.100.1 -s 1000 -c 5 # sent through TC0
ethtool -I --show-mm swp0 | grep MACMergeFragCountTx # should increase

ip addr flush swp0 && ip link set swp0 down
remove_taprio swp0
ethtool --set-mm swp0 pmac-enabled off verify-enabled off tx-enabled off
ip netns exec ns0 ethtool --set-mm eno0 pmac-enabled off verify-enabled off tx-enabled off
ip netns del ns0

Vladimir Oltean (3):
  net: mscc: ocelot: extend ocelot->fwd_domain_lock to cover
    ocelot->tas_lock
  net: dsa: felix: make vsc9959_tas_guard_bands_update() visible to
    ocelot->ops
  net: mscc: ocelot: fix oversize frame dropping for preemptible TCs

 drivers/net/dsa/ocelot/felix.c         |  9 ++--
 drivers/net/dsa/ocelot/felix.h         |  1 -
 drivers/net/dsa/ocelot/felix_vsc9959.c | 59 +++++++++++++++++---------
 drivers/net/ethernet/mscc/ocelot.c     |  1 -
 drivers/net/ethernet/mscc/ocelot_mm.c  | 14 +++---
 include/soc/mscc/ocelot.h              |  9 ++--
 6 files changed, 56 insertions(+), 37 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net 1/3] net: mscc: ocelot: extend ocelot->fwd_domain_lock to cover ocelot->tas_lock
  2023-07-05 10:44 [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver Vladimir Oltean
@ 2023-07-05 10:44 ` Vladimir Oltean
  2023-07-11 16:56   ` Simon Horman
  2023-07-05 10:44 ` [PATCH net 2/3] net: dsa: felix: make vsc9959_tas_guard_bands_update() visible to ocelot->ops Vladimir Oltean
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Vladimir Oltean @ 2023-07-05 10:44 UTC (permalink / raw)
  To: netdev
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Xiaoliang Yang, linux-kernel

In a future commit we will have to call vsc9959_tas_guard_bands_update()
from ocelot_port_update_active_preemptible_tcs(), and that will be
impossible due to the AB/BA locking dependencies between
ocelot->tas_lock and ocelot->fwd_domain_lock.

Just like we did in commit 3ff468ef987e ("net: mscc: ocelot: remove
struct ocelot_mm_state :: lock"), the only solution is to expand the
scope of ocelot->fwd_domain_lock for it to also serialize changes made
to the Time-Aware Shaper, because those will have to result in a
recalculation of cut-through TCs, which is something that depends on the
forwarding domain.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c         |  4 +--
 drivers/net/dsa/ocelot/felix_vsc9959.c | 36 ++++++++++++++++----------
 drivers/net/ethernet/mscc/ocelot.c     |  1 -
 drivers/net/ethernet/mscc/ocelot_mm.c  |  7 ++---
 include/soc/mscc/ocelot.h              |  8 +++---
 5 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 70c0e2b1936b..0c1207613aa4 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1790,12 +1790,12 @@ static int felix_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 
 	ocelot_port_set_maxlen(ocelot, port, new_mtu);
 
-	mutex_lock(&ocelot->tas_lock);
+	mutex_lock(&ocelot->fwd_domain_lock);
 
 	if (ocelot_port->taprio && felix->info->tas_guard_bands_update)
 		felix->info->tas_guard_bands_update(ocelot, port);
 
-	mutex_unlock(&ocelot->tas_lock);
+	mutex_unlock(&ocelot->fwd_domain_lock);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index bb39fedd46c7..56b8bcac9690 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1217,7 +1217,7 @@ static void vsc9959_tas_guard_bands_update(struct ocelot *ocelot, int port)
 	u8 tas_speed;
 	int tc;
 
-	lockdep_assert_held(&ocelot->tas_lock);
+	lockdep_assert_held(&ocelot->fwd_domain_lock);
 
 	taprio = ocelot_port->taprio;
 
@@ -1259,8 +1259,6 @@ static void vsc9959_tas_guard_bands_update(struct ocelot *ocelot, int port)
 
 	vsc9959_tas_min_gate_lengths(taprio, min_gate_len);
 
-	mutex_lock(&ocelot->fwd_domain_lock);
-
 	for (tc = 0; tc < OCELOT_NUM_TC; tc++) {
 		u32 requested_max_sdu = vsc9959_tas_tc_max_sdu(taprio, tc);
 		u64 remaining_gate_len_ps;
@@ -1323,8 +1321,6 @@ static void vsc9959_tas_guard_bands_update(struct ocelot *ocelot, int port)
 	ocelot_write_rix(ocelot, maxlen, QSYS_PORT_MAX_SDU, port);
 
 	ocelot->ops->cut_through_fwd(ocelot);
-
-	mutex_unlock(&ocelot->fwd_domain_lock);
 }
 
 static void vsc9959_sched_speed_set(struct ocelot *ocelot, int port,
@@ -1351,7 +1347,7 @@ static void vsc9959_sched_speed_set(struct ocelot *ocelot, int port,
 		break;
 	}
 
-	mutex_lock(&ocelot->tas_lock);
+	mutex_lock(&ocelot->fwd_domain_lock);
 
 	ocelot_rmw_rix(ocelot,
 		       QSYS_TAG_CONFIG_LINK_SPEED(tas_speed),
@@ -1361,7 +1357,7 @@ static void vsc9959_sched_speed_set(struct ocelot *ocelot, int port,
 	if (ocelot_port->taprio)
 		vsc9959_tas_guard_bands_update(ocelot, port);
 
-	mutex_unlock(&ocelot->tas_lock);
+	mutex_unlock(&ocelot->fwd_domain_lock);
 }
 
 static void vsc9959_new_base_time(struct ocelot *ocelot, ktime_t base_time,
@@ -1409,7 +1405,7 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
 	int ret, i;
 	u32 val;
 
-	mutex_lock(&ocelot->tas_lock);
+	mutex_lock(&ocelot->fwd_domain_lock);
 
 	if (taprio->cmd == TAPRIO_CMD_DESTROY) {
 		ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
@@ -1421,7 +1417,7 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
 
 		vsc9959_tas_guard_bands_update(ocelot, port);
 
-		mutex_unlock(&ocelot->tas_lock);
+		mutex_unlock(&ocelot->fwd_domain_lock);
 		return 0;
 	} else if (taprio->cmd != TAPRIO_CMD_REPLACE) {
 		ret = -EOPNOTSUPP;
@@ -1504,7 +1500,7 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
 	ocelot_port->taprio = taprio_offload_get(taprio);
 	vsc9959_tas_guard_bands_update(ocelot, port);
 
-	mutex_unlock(&ocelot->tas_lock);
+	mutex_unlock(&ocelot->fwd_domain_lock);
 
 	return 0;
 
@@ -1512,7 +1508,7 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
 	taprio->mqprio.qopt.num_tc = 0;
 	ocelot_port_mqprio(ocelot, port, &taprio->mqprio);
 err_unlock:
-	mutex_unlock(&ocelot->tas_lock);
+	mutex_unlock(&ocelot->fwd_domain_lock);
 
 	return ret;
 }
@@ -1525,7 +1521,7 @@ static void vsc9959_tas_clock_adjust(struct ocelot *ocelot)
 	int port;
 	u32 val;
 
-	mutex_lock(&ocelot->tas_lock);
+	mutex_lock(&ocelot->fwd_domain_lock);
 
 	for (port = 0; port < ocelot->num_phys_ports; port++) {
 		ocelot_port = ocelot->ports[port];
@@ -1563,7 +1559,7 @@ static void vsc9959_tas_clock_adjust(struct ocelot *ocelot)
 			       QSYS_TAG_CONFIG_ENABLE,
 			       QSYS_TAG_CONFIG, port);
 	}
-	mutex_unlock(&ocelot->tas_lock);
+	mutex_unlock(&ocelot->fwd_domain_lock);
 }
 
 static int vsc9959_qos_port_cbs_set(struct dsa_switch *ds, int port,
@@ -1634,6 +1630,18 @@ static int vsc9959_qos_query_caps(struct tc_query_caps_base *base)
 	}
 }
 
+static int vsc9959_qos_port_mqprio(struct ocelot *ocelot, int port,
+				   struct tc_mqprio_qopt_offload *mqprio)
+{
+	int ret;
+
+	mutex_lock(&ocelot->fwd_domain_lock);
+	ret = ocelot_port_mqprio(ocelot, port, mqprio);
+	mutex_unlock(&ocelot->fwd_domain_lock);
+
+	return ret;
+}
+
 static int vsc9959_port_setup_tc(struct dsa_switch *ds, int port,
 				 enum tc_setup_type type,
 				 void *type_data)
@@ -1646,7 +1654,7 @@ static int vsc9959_port_setup_tc(struct dsa_switch *ds, int port,
 	case TC_SETUP_QDISC_TAPRIO:
 		return vsc9959_qos_port_tas_set(ocelot, port, type_data);
 	case TC_SETUP_QDISC_MQPRIO:
-		return ocelot_port_mqprio(ocelot, port, type_data);
+		return vsc9959_qos_port_mqprio(ocelot, port, type_data);
 	case TC_SETUP_QDISC_CBS:
 		return vsc9959_qos_port_cbs_set(ds, port, type_data);
 	default:
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 2fa833d041ba..56ccbd4c37fe 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -2927,7 +2927,6 @@ int ocelot_init(struct ocelot *ocelot)
 
 	mutex_init(&ocelot->mact_lock);
 	mutex_init(&ocelot->fwd_domain_lock);
-	mutex_init(&ocelot->tas_lock);
 	spin_lock_init(&ocelot->ptp_clock_lock);
 	spin_lock_init(&ocelot->ts_id_lock);
 
diff --git a/drivers/net/ethernet/mscc/ocelot_mm.c b/drivers/net/ethernet/mscc/ocelot_mm.c
index fb3145118d68..f3c0e6c32934 100644
--- a/drivers/net/ethernet/mscc/ocelot_mm.c
+++ b/drivers/net/ethernet/mscc/ocelot_mm.c
@@ -89,17 +89,14 @@ void ocelot_port_change_fp(struct ocelot *ocelot, int port,
 {
 	struct ocelot_mm_state *mm = &ocelot->mm[port];
 
-	mutex_lock(&ocelot->fwd_domain_lock);
+	lockdep_assert_held(&ocelot->fwd_domain_lock);
 
 	if (mm->preemptible_tcs == preemptible_tcs)
-		goto out_unlock;
+		return;
 
 	mm->preemptible_tcs = preemptible_tcs;
 
 	ocelot_port_update_active_preemptible_tcs(ocelot, port);
-
-out_unlock:
-	mutex_unlock(&ocelot->fwd_domain_lock);
 }
 
 static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port)
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 22aae505c813..eb5f8914a66c 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -863,12 +863,12 @@ struct ocelot {
 	struct mutex			stat_view_lock;
 	/* Lock for serializing access to the MAC table */
 	struct mutex			mact_lock;
-	/* Lock for serializing forwarding domain changes */
+	/* Lock for serializing forwarding domain changes, including the
+	 * configuration of the Time-Aware Shaper, MAC Merge layer and
+	 * cut-through forwarding, on which it depends
+	 */
 	struct mutex			fwd_domain_lock;
 
-	/* Lock for serializing Time-Aware Shaper changes */
-	struct mutex			tas_lock;
-
 	struct workqueue_struct		*owq;
 
 	u8				ptp:1;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH net 2/3] net: dsa: felix: make vsc9959_tas_guard_bands_update() visible to ocelot->ops
  2023-07-05 10:44 [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver Vladimir Oltean
  2023-07-05 10:44 ` [PATCH net 1/3] net: mscc: ocelot: extend ocelot->fwd_domain_lock to cover ocelot->tas_lock Vladimir Oltean
@ 2023-07-05 10:44 ` Vladimir Oltean
  2023-07-05 10:44 ` [PATCH net 3/3] net: mscc: ocelot: fix oversize frame dropping for preemptible TCs Vladimir Oltean
  2023-07-07  2:20 ` [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver patchwork-bot+netdevbpf
  3 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2023-07-05 10:44 UTC (permalink / raw)
  To: netdev
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Xiaoliang Yang, linux-kernel

In a future change we will need to make
ocelot_port_update_active_preemptible_tcs() call
vsc9959_tas_guard_bands_update(), but that is currently not possible,
since the ocelot switch lib does not have access to functions private to
the DSA wrapper.

Move the pointer to vsc9959_tas_guard_bands_update() from felix->info
(which is private to the DSA driver) to ocelot->ops (which is also
visible to the ocelot switch lib).

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c         | 5 ++---
 drivers/net/dsa/ocelot/felix.h         | 1 -
 drivers/net/dsa/ocelot/felix_vsc9959.c | 2 +-
 include/soc/mscc/ocelot.h              | 1 +
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 0c1207613aa4..dee43caee19e 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1786,14 +1786,13 @@ static int felix_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
-	struct felix *felix = ocelot_to_felix(ocelot);
 
 	ocelot_port_set_maxlen(ocelot, port, new_mtu);
 
 	mutex_lock(&ocelot->fwd_domain_lock);
 
-	if (ocelot_port->taprio && felix->info->tas_guard_bands_update)
-		felix->info->tas_guard_bands_update(ocelot, port);
+	if (ocelot_port->taprio && ocelot->ops->tas_guard_bands_update)
+		ocelot->ops->tas_guard_bands_update(ocelot, port);
 
 	mutex_unlock(&ocelot->fwd_domain_lock);
 
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index 96008c046da5..1d4befe7cfe8 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -57,7 +57,6 @@ struct felix_info {
 	void	(*mdio_bus_free)(struct ocelot *ocelot);
 	int	(*port_setup_tc)(struct dsa_switch *ds, int port,
 				 enum tc_setup_type type, void *type_data);
-	void	(*tas_guard_bands_update)(struct ocelot *ocelot, int port);
 	void	(*port_sched_speed_set)(struct ocelot *ocelot, int port,
 					u32 speed);
 	void	(*phylink_mac_config)(struct ocelot *ocelot, int port,
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 56b8bcac9690..d7caadd13f83 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -2599,6 +2599,7 @@ static const struct ocelot_ops vsc9959_ops = {
 	.cut_through_fwd	= vsc9959_cut_through_fwd,
 	.tas_clock_adjust	= vsc9959_tas_clock_adjust,
 	.update_stats		= vsc9959_update_stats,
+	.tas_guard_bands_update	= vsc9959_tas_guard_bands_update,
 };
 
 static const struct felix_info felix_info_vsc9959 = {
@@ -2624,7 +2625,6 @@ static const struct felix_info felix_info_vsc9959 = {
 	.port_modes		= vsc9959_port_modes,
 	.port_setup_tc		= vsc9959_port_setup_tc,
 	.port_sched_speed_set	= vsc9959_sched_speed_set,
-	.tas_guard_bands_update	= vsc9959_tas_guard_bands_update,
 };
 
 /* The INTB interrupt is shared between for PTP TX timestamp availability
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index eb5f8914a66c..a8c2817335b9 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -663,6 +663,7 @@ struct ocelot_ops {
 			      struct flow_stats *stats);
 	void (*cut_through_fwd)(struct ocelot *ocelot);
 	void (*tas_clock_adjust)(struct ocelot *ocelot);
+	void (*tas_guard_bands_update)(struct ocelot *ocelot, int port);
 	void (*update_stats)(struct ocelot *ocelot);
 };
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH net 3/3] net: mscc: ocelot: fix oversize frame dropping for preemptible TCs
  2023-07-05 10:44 [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver Vladimir Oltean
  2023-07-05 10:44 ` [PATCH net 1/3] net: mscc: ocelot: extend ocelot->fwd_domain_lock to cover ocelot->tas_lock Vladimir Oltean
  2023-07-05 10:44 ` [PATCH net 2/3] net: dsa: felix: make vsc9959_tas_guard_bands_update() visible to ocelot->ops Vladimir Oltean
@ 2023-07-05 10:44 ` Vladimir Oltean
  2023-07-07  2:20 ` [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver patchwork-bot+netdevbpf
  3 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2023-07-05 10:44 UTC (permalink / raw)
  To: netdev
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Alexandre Belloni,
	UNGLinuxDriver, Xiaoliang Yang, linux-kernel

This switch implements Hold/Release in a strange way, with no control
from the user as required by IEEE 802.1Q-2018 through Set-And-Hold-MAC
and Set-And-Release-MAC, but rather, it emits HOLD requests implicitly
based on the schedule.

Namely, when the gate of a preemptible TC is about to close (actually
QSYS::PREEMPTION_CFG.HOLD_ADVANCE octet times in advance of this event),
the QSYS seems to emit a HOLD request pulse towards the MAC which
preempts the currently transmitted packet, and further packets are held
back in the queue system.

This allows large frames to be squeezed through small time slots,
because HOLD requests initiated by the gate events result in the frame
being segmented in multiple fragments, the bit time of which is equal to
the size of the time slot.

It has been reported that the vsc9959_tas_guard_bands_update() logic
breaks this, because it doesn't take preemptible TCs into account, and
enables oversized frame dropping when the time slot doesn't allow a full
MTU to be sent, but it does allow 2*minFragSize to be sent (128B).
Packets larger than 128B are dropped instead of being sent in multiple
fragments.

Confusingly, the manual says:

| For guard band, SDU calculation of a traffic class of a port, if
| preemption is enabled (through 'QSYS::PREEMPTION_CFG.P_QUEUES') then
| QSYS::PREEMPTION_CFG.HOLD_ADVANCE is used, otherwise
| QSYS::QMAXSDU_CFG_*.QMAXSDU_* is used.

but this only refers to the static guard band durations, and the
QMAXSDU_CFG_* registers have dual purpose - the other being oversized
frame dropping, which takes place irrespective of whether frames are
preemptible or express.

So, to fix the problem, we need to call vsc9959_tas_guard_bands_update()
from ocelot_port_update_active_preemptible_tcs(), and modify the guard
band logic to consider a different (lower) oversize limit for
preemptible traffic classes.

Fixes: 403ffc2c34de ("net: mscc: ocelot: add support for preemptible traffic classes")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c | 21 +++++++++++++++++----
 drivers/net/ethernet/mscc/ocelot_mm.c  |  7 +++++--
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index d7caadd13f83..1c113957fcf4 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1209,11 +1209,13 @@ static u32 vsc9959_tas_tc_max_sdu(struct tc_taprio_qopt_offload *taprio, int tc)
 static void vsc9959_tas_guard_bands_update(struct ocelot *ocelot, int port)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	struct ocelot_mm_state *mm = &ocelot->mm[port];
 	struct tc_taprio_qopt_offload *taprio;
 	u64 min_gate_len[OCELOT_NUM_TC];
+	u32 val, maxlen, add_frag_size;
+	u64 needed_min_frag_time_ps;
 	int speed, picos_per_byte;
 	u64 needed_bit_time_ps;
-	u32 val, maxlen;
 	u8 tas_speed;
 	int tc;
 
@@ -1253,9 +1255,18 @@ static void vsc9959_tas_guard_bands_update(struct ocelot *ocelot, int port)
 	 */
 	needed_bit_time_ps = (u64)(maxlen + 24) * picos_per_byte;
 
+	/* Preemptible TCs don't need to pass a full MTU, the port will
+	 * automatically emit a HOLD request when a preemptible TC gate closes
+	 */
+	val = ocelot_read_rix(ocelot, QSYS_PREEMPTION_CFG, port);
+	add_frag_size = QSYS_PREEMPTION_CFG_MM_ADD_FRAG_SIZE_X(val);
+	needed_min_frag_time_ps = picos_per_byte *
+		(u64)(24 + 2 * ethtool_mm_frag_size_add_to_min(add_frag_size));
+
 	dev_dbg(ocelot->dev,
-		"port %d: max frame size %d needs %llu ps at speed %d\n",
-		port, maxlen, needed_bit_time_ps, speed);
+		"port %d: max frame size %d needs %llu ps, %llu ps for mPackets at speed %d\n",
+		port, maxlen, needed_bit_time_ps, needed_min_frag_time_ps,
+		speed);
 
 	vsc9959_tas_min_gate_lengths(taprio, min_gate_len);
 
@@ -1267,7 +1278,9 @@ static void vsc9959_tas_guard_bands_update(struct ocelot *ocelot, int port)
 		remaining_gate_len_ps =
 			vsc9959_tas_remaining_gate_len_ps(min_gate_len[tc]);
 
-		if (remaining_gate_len_ps > needed_bit_time_ps) {
+		if ((mm->active_preemptible_tcs & BIT(tc)) ?
+		    remaining_gate_len_ps > needed_min_frag_time_ps :
+		    remaining_gate_len_ps > needed_bit_time_ps) {
 			/* Setting QMAXSDU_CFG to 0 disables oversized frame
 			 * dropping.
 			 */
diff --git a/drivers/net/ethernet/mscc/ocelot_mm.c b/drivers/net/ethernet/mscc/ocelot_mm.c
index f3c0e6c32934..c815ae64e39d 100644
--- a/drivers/net/ethernet/mscc/ocelot_mm.c
+++ b/drivers/net/ethernet/mscc/ocelot_mm.c
@@ -67,10 +67,13 @@ void ocelot_port_update_active_preemptible_tcs(struct ocelot *ocelot, int port)
 		val = mm->preemptible_tcs;
 
 	/* Cut through switching doesn't work for preemptible priorities,
-	 * so first make sure it is disabled.
+	 * so first make sure it is disabled. Also, changing the preemptible
+	 * TCs affects the oversized frame dropping logic, so that needs to be
+	 * re-triggered. And since tas_guard_bands_update() also implicitly
+	 * calls cut_through_fwd(), we don't need to explicitly call it.
 	 */
 	mm->active_preemptible_tcs = val;
-	ocelot->ops->cut_through_fwd(ocelot);
+	ocelot->ops->tas_guard_bands_update(ocelot, port);
 
 	dev_dbg(ocelot->dev,
 		"port %d %s/%s, MM TX %s, preemptible TCs 0x%x, active 0x%x\n",
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver
  2023-07-05 10:44 [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver Vladimir Oltean
                   ` (2 preceding siblings ...)
  2023-07-05 10:44 ` [PATCH net 3/3] net: mscc: ocelot: fix oversize frame dropping for preemptible TCs Vladimir Oltean
@ 2023-07-07  2:20 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-07-07  2:20 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, andrew, f.fainelli, davem, edumazet, kuba, pabeni,
	claudiu.manoil, alexandre.belloni, UNGLinuxDriver,
	xiaoliang.yang_1, linux-kernel

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  5 Jul 2023 13:44:19 +0300 you wrote:
> It has been reported that preemptible traffic doesn't completely behave
> as expected. Namely, large packets should be able to be squeezed
> (through fragmentation) through taprio time slots smaller than the
> transmission time of the full frame. That does not happen due to logic
> in the driver (for oversize frame dropping with taprio) that was not
> updated in order for this use case to work.
> 
> [...]

Here is the summary with links:
  - [net,1/3] net: mscc: ocelot: extend ocelot->fwd_domain_lock to cover ocelot->tas_lock
    https://git.kernel.org/netdev/net/c/009d30f1a777
  - [net,2/3] net: dsa: felix: make vsc9959_tas_guard_bands_update() visible to ocelot->ops
    https://git.kernel.org/netdev/net/c/c60819149b63
  - [net,3/3] net: mscc: ocelot: fix oversize frame dropping for preemptible TCs
    https://git.kernel.org/netdev/net/c/c6efb4ae387c

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net 1/3] net: mscc: ocelot: extend ocelot->fwd_domain_lock to cover ocelot->tas_lock
  2023-07-05 10:44 ` [PATCH net 1/3] net: mscc: ocelot: extend ocelot->fwd_domain_lock to cover ocelot->tas_lock Vladimir Oltean
@ 2023-07-11 16:56   ` Simon Horman
  0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2023-07-11 16:56 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, Andrew Lunn, Florian Fainelli, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Claudiu Manoil,
	Alexandre Belloni, UNGLinuxDriver, Xiaoliang Yang, linux-kernel

On Wed, Jul 05, 2023 at 01:44:20PM +0300, Vladimir Oltean wrote:
> In a future commit we will have to call vsc9959_tas_guard_bands_update()
> from ocelot_port_update_active_preemptible_tcs(), and that will be
> impossible due to the AB/BA locking dependencies between
> ocelot->tas_lock and ocelot->fwd_domain_lock.
> 
> Just like we did in commit 3ff468ef987e ("net: mscc: ocelot: remove
> struct ocelot_mm_state :: lock"), the only solution is to expand the
> scope of ocelot->fwd_domain_lock for it to also serialize changes made
> to the Time-Aware Shaper, because those will have to result in a
> recalculation of cut-through TCs, which is something that depends on the
> forwarding domain.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-07-11 16:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-05 10:44 [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver Vladimir Oltean
2023-07-05 10:44 ` [PATCH net 1/3] net: mscc: ocelot: extend ocelot->fwd_domain_lock to cover ocelot->tas_lock Vladimir Oltean
2023-07-11 16:56   ` Simon Horman
2023-07-05 10:44 ` [PATCH net 2/3] net: dsa: felix: make vsc9959_tas_guard_bands_update() visible to ocelot->ops Vladimir Oltean
2023-07-05 10:44 ` [PATCH net 3/3] net: mscc: ocelot: fix oversize frame dropping for preemptible TCs Vladimir Oltean
2023-07-07  2:20 ` [PATCH net 0/3] Fix dropping of oversize preemptible frames with felix DSA driver patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).