[PATCH net-next 0/2] Report tc-taprio window drops in NXP ENETC driver

[PATCH net-next 0/2] Report tc-taprio window drops in NXP ENETC driver

[Vladimir Oltean]

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

This series adds an ethtool statistics counter to the enetc driver which
tracks the number of packets dropped due to insufficient transmit time
in the tc-taprio schedule compared to the required time it takes the MAC
to send that packet at the current link speed.

Po Liu (1):
  net: enetc: count the tc-taprio window drops

Vladimir Oltean (1):
  net: enetc: manage ENETC_F_QBV in priv->active_offloads only when
    enabled

 drivers/net/ethernet/freescale/enetc/enetc.c        | 13 +++++++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h        |  2 ++
 .../net/ethernet/freescale/enetc/enetc_ethtool.c    |  2 ++
 drivers/net/ethernet/freescale/enetc/enetc_hw.h     |  1 +
 drivers/net/ethernet/freescale/enetc/enetc_pf.c     |  6 ++----
 drivers/net/ethernet/freescale/enetc/enetc_qos.c    |  6 ++++++
 6 files changed, 24 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH net-next 1/2] net: enetc: manage ENETC_F_QBV in priv->active_offloads only when enabled

[Vladimir Oltean]

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

All the other flags in priv->active_offloads are managed dynamically,
except ENETC_F_QBV which is set statically based on the probed SI capability.

This change makes priv->active_offloads & ENETC_F_QBV track the presence
of a tc-taprio schedule on the port. Calling enetc_sched_speed_set()
from phylink_mac_link_up() is kept unconditional as long as Qbv is
supported at all on the port, because the tc-taprio offload does not
re-trigger another link mode resolve, so the scheduler needs to be
functional from the get go.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc_pf.c  | 6 ++----
 drivers/net/ethernet/freescale/enetc/enetc_qos.c | 6 ++++++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 31274325159a..aced941e57e1 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -769,9 +769,6 @@ static void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
 
 	ndev->priv_flags |= IFF_UNICAST_FLT;
 
-	if (si->hw_features & ENETC_SI_F_QBV)
-		priv->active_offloads |= ENETC_F_QBV;
-
 	if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) {
 		priv->active_offloads |= ENETC_F_QCI;
 		ndev->features |= NETIF_F_HW_TC;
@@ -1022,7 +1019,8 @@ static void enetc_pl_mac_link_up(struct phylink_config *config,
 	int idx;
 
 	priv = netdev_priv(pf->si->ndev);
-	if (priv->active_offloads & ENETC_F_QBV)
+
+	if (pf->si->hw_features & ENETC_SI_F_QBV)
 		enetc_sched_speed_set(priv, speed);
 
 	if (!phylink_autoneg_inband(mode) &&
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 4577226d3c6a..23ac6e1551ca 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -69,6 +69,9 @@ static int enetc_setup_taprio(struct net_device *ndev,
 		enetc_wr(&priv->si->hw,
 			 ENETC_QBV_PTGCR_OFFSET,
 			 tge & (~ENETC_QBV_TGE));
+
+		priv->active_offloads &= ~ENETC_F_QBV;
+
 		return 0;
 	}
 
@@ -135,6 +138,9 @@ static int enetc_setup_taprio(struct net_device *ndev,
 	dma_unmap_single(&priv->si->pdev->dev, dma, data_size, DMA_TO_DEVICE);
 	kfree(gcl_data);
 
+	if (!err)
+		priv->active_offloads |= ENETC_F_QBV;
+
 	return err;
 }
 
-- 
2.25.1

[PATCH net-next 2/2] net: enetc: count the tc-taprio window drops

[Vladimir Oltean]

From: Po Liu <Po.Liu@nxp.com>

The enetc scheduler for IEEE 802.1Qbv has 2 options (depending on
PTGCR[TG_DROP_DISABLE]) when we attempt to send an oversized packet
which will never fit in its allotted time slot for its traffic class:
either block the entire port due to head-of-line blocking, or drop the
packet and set a bit in the writeback format of the transmit buffer
descriptor, allowing other packets to be sent.

We obviously choose the second option in the driver, but we do not
detect the drop condition, so from the perspective of the network stack,
the packet is sent and no error counter is incremented.

This change checks the writeback of the TX BD when tc-taprio is enabled,
and increments a specific ethtool statistics counter and a generic
"tx_dropped" counter in ndo_get_stats64.

Signed-off-by: Po Liu <Po.Liu@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c        | 13 +++++++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h        |  2 ++
 .../net/ethernet/freescale/enetc/enetc_ethtool.c    |  2 ++
 drivers/net/ethernet/freescale/enetc/enetc_hw.h     |  1 +
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 3ca93adb9662..3f1cb921571a 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -170,7 +170,8 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 	}
 
 	tx_swbd->do_twostep_tstamp = do_twostep_tstamp;
-	tx_swbd->check_wb = tx_swbd->do_twostep_tstamp;
+	tx_swbd->qbv_en = !!(priv->active_offloads & ENETC_F_QBV);
+	tx_swbd->check_wb = tx_swbd->do_twostep_tstamp || tx_swbd->qbv_en;
 
 	if (do_vlan || do_onestep_tstamp || do_twostep_tstamp)
 		flags |= ENETC_TXBD_FLAGS_EX;
@@ -525,9 +526,9 @@ static void enetc_recycle_xdp_tx_buff(struct enetc_bdr *tx_ring,
 
 static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 {
+	int tx_frm_cnt = 0, tx_byte_cnt = 0, tx_win_drop = 0;
 	struct net_device *ndev = tx_ring->ndev;
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
-	int tx_frm_cnt = 0, tx_byte_cnt = 0;
 	struct enetc_tx_swbd *tx_swbd;
 	int i, bds_to_clean;
 	bool do_twostep_tstamp;
@@ -557,6 +558,10 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 						    &tstamp);
 				do_twostep_tstamp = true;
 			}
+
+			if (tx_swbd->qbv_en &&
+			    txbd->wb.status & ENETC_TXBD_STATS_WIN)
+				tx_win_drop++;
 		}
 
 		if (tx_swbd->is_xdp_tx)
@@ -610,6 +615,7 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 	tx_ring->next_to_clean = i;
 	tx_ring->stats.packets += tx_frm_cnt;
 	tx_ring->stats.bytes += tx_byte_cnt;
+	tx_ring->stats.win_drop += tx_win_drop;
 
 	if (unlikely(tx_frm_cnt && netif_carrier_ok(ndev) &&
 		     __netif_subqueue_stopped(ndev, tx_ring->index) &&
@@ -2271,6 +2277,7 @@ struct net_device_stats *enetc_get_stats(struct net_device *ndev)
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	struct net_device_stats *stats = &ndev->stats;
 	unsigned long packets = 0, bytes = 0;
+	unsigned long tx_dropped = 0;
 	int i;
 
 	for (i = 0; i < priv->num_rx_rings; i++) {
@@ -2286,10 +2293,12 @@ struct net_device_stats *enetc_get_stats(struct net_device *ndev)
 	for (i = 0; i < priv->num_tx_rings; i++) {
 		packets += priv->tx_ring[i]->stats.packets;
 		bytes	+= priv->tx_ring[i]->stats.bytes;
+		tx_dropped += priv->tx_ring[i]->stats.win_drop;
 	}
 
 	stats->tx_packets = packets;
 	stats->tx_bytes = bytes;
+	stats->tx_dropped = tx_dropped;
 
 	return stats;
 }
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 08b283347d9c..2a5973aebc31 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -34,6 +34,7 @@ struct enetc_tx_swbd {
 	u8 is_eof:1;
 	u8 is_xdp_tx:1;
 	u8 is_xdp_redirect:1;
+	u8 qbv_en:1;
 };
 
 #define ENETC_RX_MAXFRM_SIZE	ENETC_MAC_MAXFRM_SIZE
@@ -70,6 +71,7 @@ struct enetc_ring_stats {
 	unsigned int xdp_redirect_sg;
 	unsigned int recycles;
 	unsigned int recycle_failures;
+	unsigned int win_drop;
 };
 
 struct enetc_xdp_data {
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index ebccaf02411c..4d55e78fa353 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -204,6 +204,7 @@ static const char tx_ring_stats[][ETH_GSTRING_LEN] = {
 	"Tx ring %2d frames",
 	"Tx ring %2d XDP frames",
 	"Tx ring %2d XDP drops",
+	"Tx window drop %2d frames",
 };
 
 static int enetc_get_sset_count(struct net_device *ndev, int sset)
@@ -279,6 +280,7 @@ static void enetc_get_ethtool_stats(struct net_device *ndev,
 		data[o++] = priv->tx_ring[i]->stats.packets;
 		data[o++] = priv->tx_ring[i]->stats.xdp_tx;
 		data[o++] = priv->tx_ring[i]->stats.xdp_tx_drops;
+		data[o++] = priv->tx_ring[i]->stats.win_drop;
 	}
 
 	for (i = 0; i < priv->num_rx_rings; i++) {
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 0f5f081a5baf..e7fa4fb85d0b 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -543,6 +543,7 @@ enum enetc_txbd_flags {
 	ENETC_TXBD_FLAGS_EX = BIT(6),
 	ENETC_TXBD_FLAGS_F = BIT(7)
 };
+#define ENETC_TXBD_STATS_WIN	BIT(7)
 #define ENETC_TXBD_TXSTART_MASK GENMASK(24, 0)
 #define ENETC_TXBD_FLAGS_OFFSET 24
 
-- 
2.25.1

Re: [PATCH net-next 2/2] net: enetc: count the tc-taprio window drops

[Jakub Kicinski]

On Wed,  2 Jun 2021 15:21:14 +0300 Vladimir Oltean wrote:
> From: Po Liu <Po.Liu@nxp.com>
> 
> The enetc scheduler for IEEE 802.1Qbv has 2 options (depending on
> PTGCR[TG_DROP_DISABLE]) when we attempt to send an oversized packet
> which will never fit in its allotted time slot for its traffic class:
> either block the entire port due to head-of-line blocking, or drop the

the entire port or the entire queue?

> packet and set a bit in the writeback format of the transmit buffer
> descriptor, allowing other packets to be sent.
> 
> We obviously choose the second option in the driver, but we do not
> detect the drop condition, so from the perspective of the network stack,
> the packet is sent and no error counter is incremented.
> 
> This change checks the writeback of the TX BD when tc-taprio is enabled,
> and increments a specific ethtool statistics counter and a generic
> "tx_dropped" counter in ndo_get_stats64.

Any chance we should also report that back to the qdisc to have 
a standard way of querying from user space? Qdisc offload supports
stats in general, it shouldn't be an issue, and the stat seems generic
enough, no?

Re: [PATCH net-next 2/2] net: enetc: count the tc-taprio window drops

[Vladimir Oltean]

On Wed, Jun 02, 2021 at 10:19:20AM -0700, Jakub Kicinski wrote:
> On Wed,  2 Jun 2021 15:21:14 +0300 Vladimir Oltean wrote:
> > From: Po Liu <Po.Liu@nxp.com>
> > 
> > The enetc scheduler for IEEE 802.1Qbv has 2 options (depending on
> > PTGCR[TG_DROP_DISABLE]) when we attempt to send an oversized packet
> > which will never fit in its allotted time slot for its traffic class:
> > either block the entire port due to head-of-line blocking, or drop the
> 
> the entire port or the entire queue?

I don't remember, I need to re-test.

> > packet and set a bit in the writeback format of the transmit buffer
> > descriptor, allowing other packets to be sent.
> > 
> > We obviously choose the second option in the driver, but we do not
> > detect the drop condition, so from the perspective of the network stack,
> > the packet is sent and no error counter is incremented.
> > 
> > This change checks the writeback of the TX BD when tc-taprio is enabled,
> > and increments a specific ethtool statistics counter and a generic
> > "tx_dropped" counter in ndo_get_stats64.
> 
> Any chance we should also report that back to the qdisc to have 
> a standard way of querying from user space? Qdisc offload supports
> stats in general, it shouldn't be an issue, and the stat seems generic
> enough, no?

You're thinking of something along the lines of tc_codel_xstats?
How do you propose I pass this on to the taprio qdisc? Just call a
function in enetc that is exported by net/sched/sch_taprio.c?
If the skb is bound to a socket, I'm thinking it might be more useful to
report a struct sock_extended_err similar to the SO_EE_TXTIME_MISSED
stuff for tc-etf, what do you think?

Re: [PATCH net-next 2/2] net: enetc: count the tc-taprio window drops

[Jakub Kicinski]

On Wed, 2 Jun 2021 20:59:37 +0300 Vladimir Oltean wrote:
> On Wed, Jun 02, 2021 at 10:19:20AM -0700, Jakub Kicinski wrote:
> > On Wed,  2 Jun 2021 15:21:14 +0300 Vladimir Oltean wrote:  
> > > From: Po Liu <Po.Liu@nxp.com>
> > > 
> > > The enetc scheduler for IEEE 802.1Qbv has 2 options (depending on
> > > PTGCR[TG_DROP_DISABLE]) when we attempt to send an oversized packet
> > > which will never fit in its allotted time slot for its traffic class:
> > > either block the entire port due to head-of-line blocking, or drop the  
> > 
> > the entire port or the entire queue?  
> 
> I don't remember, I need to re-test.
> 
> > > packet and set a bit in the writeback format of the transmit buffer
> > > descriptor, allowing other packets to be sent.
> > > 
> > > We obviously choose the second option in the driver, but we do not
> > > detect the drop condition, so from the perspective of the network stack,
> > > the packet is sent and no error counter is incremented.
> > > 
> > > This change checks the writeback of the TX BD when tc-taprio is enabled,
> > > and increments a specific ethtool statistics counter and a generic
> > > "tx_dropped" counter in ndo_get_stats64.  
> > 
> > Any chance we should also report that back to the qdisc to have 
> > a standard way of querying from user space? Qdisc offload supports
> > stats in general, it shouldn't be an issue, and the stat seems generic
> > enough, no?  
> 
> You're thinking of something along the lines of tc_codel_xstats?
> How do you propose I pass this on to the taprio qdisc? Just call a
> function in enetc that is exported by net/sched/sch_taprio.c?

Check out red_dump_offload_stats()/TC_RED_STATS, this is the usual way
of handling stats offload in TC.

> If the skb is bound to a socket, I'm thinking it might be more useful to
> report a struct sock_extended_err similar to the SO_EE_TXTIME_MISSED
> stuff for tc-etf, what do you think?

That's an interesting idea as well, dunno enough about the practical
uses to be able to tell if applications care about per-socket state,
per-interface state, or both.

RE: [PATCH net-next 1/2] net: enetc: manage ENETC_F_QBV in priv->active_offloads only when enabled

[Claudiu Manoil]

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Sent: Tuesday, May 10, 2022 7:36 PM
[...]
> Subject: [PATCH net-next 1/2] net: enetc: manage ENETC_F_QBV in priv-
> >active_offloads only when enabled
> 
> Future work in this driver would like to look at priv->active_offloads &
> ENETC_F_QBV to determine whether a tc-taprio qdisc offload was
> installed, but this does not produce the intended effect.
> 
> All the other flags in priv->active_offloads are managed dynamically,
> except ENETC_F_QBV which is set statically based on the probed SI capability.
> 
> This change makes priv->active_offloads & ENETC_F_QBV really track the
> presence of a tc-taprio schedule on the port.
> 
> Some existing users, like the enetc_sched_speed_set() call from
> phylink_mac_link_up(), are best kept using the old logic: the tc-taprio
> offload does not re-trigger another link mode resolve, so the scheduler
> needs to be functional from the get go, as long as Qbv is supported at
> all on the port. So to preserve functionality there, look at the static
> station interface capability from pf->si->hw_features instead.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>

[PATCH net-next 1/2] net: enetc: manage ENETC_F_QBV in priv->active_offloads only when enabled

[Vladimir Oltean]

Future work in this driver would like to look at priv->active_offloads &
ENETC_F_QBV to determine whether a tc-taprio qdisc offload was
installed, but this does not produce the intended effect.

All the other flags in priv->active_offloads are managed dynamically,
except ENETC_F_QBV which is set statically based on the probed SI capability.

This change makes priv->active_offloads & ENETC_F_QBV really track the
presence of a tc-taprio schedule on the port.

Some existing users, like the enetc_sched_speed_set() call from
phylink_mac_link_up(), are best kept using the old logic: the tc-taprio
offload does not re-trigger another link mode resolve, so the scheduler
needs to be functional from the get go, as long as Qbv is supported at
all on the port. So to preserve functionality there, look at the static
station interface capability from pf->si->hw_features instead.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc_pf.c  | 6 ++----
 drivers/net/ethernet/freescale/enetc/enetc_qos.c | 6 ++++++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index a0c75c717073..7cccdf54359f 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -777,9 +777,6 @@ static void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
 
 	ndev->priv_flags |= IFF_UNICAST_FLT;
 
-	if (si->hw_features & ENETC_SI_F_QBV)
-		priv->active_offloads |= ENETC_F_QBV;
-
 	if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) {
 		priv->active_offloads |= ENETC_F_QCI;
 		ndev->features |= NETIF_F_HW_TC;
@@ -993,7 +990,8 @@ static void enetc_pl_mac_link_up(struct phylink_config *config,
 	int idx;
 
 	priv = netdev_priv(pf->si->ndev);
-	if (priv->active_offloads & ENETC_F_QBV)
+
+	if (pf->si->hw_features & ENETC_SI_F_QBV)
 		enetc_sched_speed_set(priv, speed);
 
 	if (!phylink_autoneg_inband(mode) &&
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 9182631856d5..582a663ed0ba 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -70,6 +70,9 @@ static int enetc_setup_taprio(struct net_device *ndev,
 		enetc_wr(&priv->si->hw,
 			 ENETC_QBV_PTGCR_OFFSET,
 			 tge & (~ENETC_QBV_TGE));
+
+		priv->active_offloads &= ~ENETC_F_QBV;
+
 		return 0;
 	}
 
@@ -125,6 +128,9 @@ static int enetc_setup_taprio(struct net_device *ndev,
 
 	enetc_cbd_free_data_mem(priv->si, data_size, tmp, &dma);
 
+	if (!err)
+		priv->active_offloads |= ENETC_F_QBV;
+
 	return err;
 }
 
-- 
2.25.1