[PATCH net-next v3 0/9] tsnep: XDP support

[PATCH net-next v3 0/9] tsnep: XDP support

[Gerhard Engleder]

Implement XDP support for tsnep driver. I tried to follow existing
drivers like igb/igc as far as possible. Some prework was already done
in previous patch series, so in this series only actual XDP stuff is
included.

Thanks for the NetDev 0x14 slides "Add XDP support on a NIC driver".

v3:
- use spin_lock_bh for TX (Paolo Abeni)
- add comment for XDP TX descriptor available check (Maciej Fijalkowski)
- return value bool for tsnep_xdp_xmit_frame_ring() (Saeed Mahameed)
- do not print DMA mapping error (Saeed Mahameed)
- use reverse xmas tree variable declaration (Saeed Mahameed)
- move struct xdp_rxq_info to end of struct tsnep_rx (Maciej Fijalkowski)
- check __TSNEP_DOWN flag on close to prevent double free (Saeed Mahameed)
- describe TSNEP_RX_INLINE_METADATA_SIZE in comment (Maciej Fijalkowski)
- substract TSNEP_RX_INLINE_METADATA_SIZE after DMA sync (Maciej Fijalkowski)
- use enum tsnep_tx_type for tsnep_xdp_tx_map (Saeed Mahameed)
- use nxmit as loop iterator in tsnep_netdev_xdp_xmit (Saeed Mahameed)
- stop netdev in tsnep_netdev_close() which is called during BPF prog setup

v2:
- move tsnep_xdp_xmit_back() to commit where it is used (Paolo Abeni)
- remove inline from tsnep_rx_offset() (Paolo Abeni)
- remove inline from tsnep_rx_offset_xdp() (Paolo Abeni)
- simplify tsnep_xdp_run_prog() call by moving xdp_status update to it (Paolo Abeni)

Gerhard Engleder (9):
  tsnep: Use spin_lock_bh for TX
  tsnep: Do not print DMA mapping error
  tsnep: Add adapter down state
  tsnep: Add XDP TX support
  tsnep: Substract TSNEP_RX_INLINE_METADATA_SIZE once
  tsnep: Support XDP BPF program setup
  tsnep: Prepare RX buffer for XDP support
  tsnep: Add RX queue info for XDP support
  tsnep: Add XDP RX support

 drivers/net/ethernet/engleder/Makefile     |   2 +-
 drivers/net/ethernet/engleder/tsnep.h      |  32 +-
 drivers/net/ethernet/engleder/tsnep_main.c | 468 +++++++++++++++++++--
 drivers/net/ethernet/engleder/tsnep_xdp.c  |  27 ++
 4 files changed, 480 insertions(+), 49 deletions(-)
 create mode 100644 drivers/net/ethernet/engleder/tsnep_xdp.c

-- 
2.30.2

[PATCH net-next v3 1/9] tsnep: Use spin_lock_bh for TX

[Gerhard Engleder]

TX processing is done only within process or BH context. Therefore,
_irqsafe variant is not necessary.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_main.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index bf0190e1d2ea..7cc5e2407809 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -434,7 +434,6 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)
 static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
 					 struct tsnep_tx *tx)
 {
-	unsigned long flags;
 	int count = 1;
 	struct tsnep_tx_entry *entry;
 	int length;
@@ -444,7 +443,7 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
 	if (skb_shinfo(skb)->nr_frags > 0)
 		count += skb_shinfo(skb)->nr_frags;
 
-	spin_lock_irqsave(&tx->lock, flags);
+	spin_lock_bh(&tx->lock);
 
 	if (tsnep_tx_desc_available(tx) < count) {
 		/* ring full, shall not happen because queue is stopped if full
@@ -452,7 +451,7 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
 		 */
 		netif_stop_queue(tx->adapter->netdev);
 
-		spin_unlock_irqrestore(&tx->lock, flags);
+		spin_unlock_bh(&tx->lock);
 
 		return NETDEV_TX_BUSY;
 	}
@@ -468,7 +467,7 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
 
 		tx->dropped++;
 
-		spin_unlock_irqrestore(&tx->lock, flags);
+		spin_unlock_bh(&tx->lock);
 
 		netdev_err(tx->adapter->netdev, "TX DMA map failed\n");
 
@@ -496,20 +495,19 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
 		netif_stop_queue(tx->adapter->netdev);
 	}
 
-	spin_unlock_irqrestore(&tx->lock, flags);
+	spin_unlock_bh(&tx->lock);
 
 	return NETDEV_TX_OK;
 }
 
 static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 {
-	unsigned long flags;
 	int budget = 128;
 	struct tsnep_tx_entry *entry;
 	int count;
 	int length;
 
-	spin_lock_irqsave(&tx->lock, flags);
+	spin_lock_bh(&tx->lock);
 
 	do {
 		if (tx->read == tx->write)
@@ -568,18 +566,17 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 		netif_wake_queue(tx->adapter->netdev);
 	}
 
-	spin_unlock_irqrestore(&tx->lock, flags);
+	spin_unlock_bh(&tx->lock);
 
 	return (budget != 0);
 }
 
 static bool tsnep_tx_pending(struct tsnep_tx *tx)
 {
-	unsigned long flags;
 	struct tsnep_tx_entry *entry;
 	bool pending = false;
 
-	spin_lock_irqsave(&tx->lock, flags);
+	spin_lock_bh(&tx->lock);
 
 	if (tx->read != tx->write) {
 		entry = &tx->entry[tx->read];
@@ -589,7 +586,7 @@ static bool tsnep_tx_pending(struct tsnep_tx *tx)
 			pending = true;
 	}
 
-	spin_unlock_irqrestore(&tx->lock, flags);
+	spin_unlock_bh(&tx->lock);
 
 	return pending;
 }
-- 
2.30.2

[PATCH net-next v3 2/9] tsnep: Do not print DMA mapping error

[Gerhard Engleder]

Printing in data path shall be avoided. DMA mapping error is already
counted in stats so printing is not necessary.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 7cc5e2407809..0d40728dcfca 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -469,8 +469,6 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
 
 		spin_unlock_bh(&tx->lock);
 
-		netdev_err(tx->adapter->netdev, "TX DMA map failed\n");
-
 		return NETDEV_TX_OK;
 	}
 	length = retval;
-- 
2.30.2

[PATCH net-next v3 3/9] tsnep: Add adapter down state

[Gerhard Engleder]

Add adapter state with flag for down state. This flag will be used by
the XDP TX path to deny TX if adapter is down.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep.h      |  1 +
 drivers/net/ethernet/engleder/tsnep_main.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
index f93ba48bac3f..f72c0c4da1a9 100644
--- a/drivers/net/ethernet/engleder/tsnep.h
+++ b/drivers/net/ethernet/engleder/tsnep.h
@@ -148,6 +148,7 @@ struct tsnep_adapter {
 	phy_interface_t phy_mode;
 	struct phy_device *phydev;
 	int msg_enable;
+	unsigned long state;
 
 	struct platform_device *pdev;
 	struct device *dmadev;
diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 0d40728dcfca..56c8cae6251e 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -43,6 +43,10 @@
 #define TSNEP_COALESCE_USECS_MAX     ((ECM_INT_DELAY_MASK >> ECM_INT_DELAY_SHIFT) * \
 				      ECM_INT_DELAY_BASE_US + ECM_INT_DELAY_BASE_US - 1)
 
+enum {
+	__TSNEP_DOWN,
+};
+
 static void tsnep_enable_irq(struct tsnep_adapter *adapter, u32 mask)
 {
 	iowrite32(mask, adapter->addr + ECM_INT_ENABLE);
@@ -1138,6 +1142,8 @@ static int tsnep_netdev_open(struct net_device *netdev)
 		tsnep_enable_irq(adapter, adapter->queue[i].irq_mask);
 	}
 
+	clear_bit(__TSNEP_DOWN, &adapter->state);
+
 	return 0;
 
 phy_failed:
@@ -1160,6 +1166,8 @@ static int tsnep_netdev_close(struct net_device *netdev)
 	struct tsnep_adapter *adapter = netdev_priv(netdev);
 	int i;
 
+	set_bit(__TSNEP_DOWN, &adapter->state);
+
 	tsnep_disable_irq(adapter, ECM_INT_LINK);
 	tsnep_phy_close(adapter);
 
@@ -1513,6 +1521,7 @@ static int tsnep_probe(struct platform_device *pdev)
 	adapter->msg_enable = NETIF_MSG_DRV | NETIF_MSG_PROBE |
 			      NETIF_MSG_LINK | NETIF_MSG_IFUP |
 			      NETIF_MSG_IFDOWN | NETIF_MSG_TX_QUEUED;
+	set_bit(__TSNEP_DOWN, &adapter->state);
 
 	netdev->min_mtu = ETH_MIN_MTU;
 	netdev->max_mtu = TSNEP_MAX_FRAME_SIZE;
@@ -1609,6 +1618,8 @@ static int tsnep_remove(struct platform_device *pdev)
 {
 	struct tsnep_adapter *adapter = platform_get_drvdata(pdev);
 
+	set_bit(__TSNEP_DOWN, &adapter->state);
+
 	unregister_netdev(adapter->netdev);
 
 	tsnep_rxnfc_cleanup(adapter);
-- 
2.30.2

[PATCH net-next v3 4/9] tsnep: Add XDP TX support

[Gerhard Engleder]

Implement ndo_xdp_xmit() for XDP TX support. Support for fragmented XDP
frames is included.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep.h      |  12 +-
 drivers/net/ethernet/engleder/tsnep_main.c | 208 ++++++++++++++++++++-
 2 files changed, 209 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
index f72c0c4da1a9..29b04127f529 100644
--- a/drivers/net/ethernet/engleder/tsnep.h
+++ b/drivers/net/ethernet/engleder/tsnep.h
@@ -57,6 +57,12 @@ struct tsnep_rxnfc_rule {
 	int location;
 };
 
+enum tsnep_tx_type {
+	TSNEP_TX_TYPE_SKB,
+	TSNEP_TX_TYPE_XDP_TX,
+	TSNEP_TX_TYPE_XDP_NDO,
+};
+
 struct tsnep_tx_entry {
 	struct tsnep_tx_desc *desc;
 	struct tsnep_tx_desc_wb *desc_wb;
@@ -65,7 +71,11 @@ struct tsnep_tx_entry {
 
 	u32 properties;
 
-	struct sk_buff *skb;
+	enum tsnep_tx_type type;
+	union {
+		struct sk_buff *skb;
+		struct xdp_frame *xdpf;
+	};
 	size_t len;
 	DEFINE_DMA_UNMAP_ADDR(dma);
 };
diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 56c8cae6251e..2c7252ded23a 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -310,10 +310,11 @@ static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,
 	struct tsnep_tx_entry *entry = &tx->entry[index];
 
 	entry->properties = 0;
-	if (entry->skb) {
+	if (entry->skb || entry->xdpf) {
 		entry->properties = length & TSNEP_DESC_LENGTH_MASK;
 		entry->properties |= TSNEP_DESC_INTERRUPT_FLAG;
-		if (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
+		if (entry->type == TSNEP_TX_TYPE_SKB &&
+		    skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
 			entry->properties |= TSNEP_DESC_EXTENDED_WRITEBACK_FLAG;
 
 		/* toggle user flag to prevent false acknowledge
@@ -400,6 +401,8 @@ static int tsnep_tx_map(struct sk_buff *skb, struct tsnep_tx *tx, int count)
 
 		entry->desc->tx = __cpu_to_le64(dma);
 
+		entry->type = TSNEP_TX_TYPE_SKB;
+
 		map_len += len;
 	}
 
@@ -417,12 +420,13 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)
 		entry = &tx->entry[(index + i) % TSNEP_RING_SIZE];
 
 		if (entry->len) {
-			if (i == 0)
+			if (i == 0 && entry->type == TSNEP_TX_TYPE_SKB)
 				dma_unmap_single(dmadev,
 						 dma_unmap_addr(entry, dma),
 						 dma_unmap_len(entry, len),
 						 DMA_TO_DEVICE);
-			else
+			else if (entry->type == TSNEP_TX_TYPE_SKB ||
+				 entry->type == TSNEP_TX_TYPE_XDP_NDO)
 				dma_unmap_page(dmadev,
 					       dma_unmap_addr(entry, dma),
 					       dma_unmap_len(entry, len),
@@ -502,12 +506,134 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
+static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,
+			    struct skb_shared_info *shinfo, int count,
+			    enum tsnep_tx_type type)
+{
+	struct device *dmadev = tx->adapter->dmadev;
+	struct tsnep_tx_entry *entry;
+	struct page *page;
+	skb_frag_t *frag;
+	unsigned int len;
+	int map_len = 0;
+	dma_addr_t dma;
+	void *data;
+	int i;
+
+	frag = NULL;
+	len = xdpf->len;
+	for (i = 0; i < count; i++) {
+		entry = &tx->entry[(tx->write + i) % TSNEP_RING_SIZE];
+		if (type == TSNEP_TX_TYPE_XDP_NDO) {
+			data = unlikely(frag) ? skb_frag_address(frag) :
+						xdpf->data;
+			dma = dma_map_single(dmadev, data, len, DMA_TO_DEVICE);
+			if (dma_mapping_error(dmadev, dma))
+				return -ENOMEM;
+
+			entry->type = TSNEP_TX_TYPE_XDP_NDO;
+		} else {
+			page = unlikely(frag) ? skb_frag_page(frag) :
+						virt_to_page(xdpf->data);
+			dma = page_pool_get_dma_addr(page);
+			if (unlikely(frag))
+				dma += skb_frag_off(frag);
+			else
+				dma += sizeof(*xdpf) + xdpf->headroom;
+			dma_sync_single_for_device(dmadev, dma, len,
+						   DMA_BIDIRECTIONAL);
+
+			entry->type = TSNEP_TX_TYPE_XDP_TX;
+		}
+
+		entry->len = len;
+		dma_unmap_addr_set(entry, dma, dma);
+
+		entry->desc->tx = __cpu_to_le64(dma);
+
+		map_len += len;
+
+		if ((i + 1) < count) {
+			frag = &shinfo->frags[i];
+			len = skb_frag_size(frag);
+		}
+	}
+
+	return map_len;
+}
+
+/* This function requires __netif_tx_lock is held by the caller. */
+static bool tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
+				      struct tsnep_tx *tx,
+				      enum tsnep_tx_type type)
+{
+	struct skb_shared_info *shinfo = xdp_get_shared_info_from_frame(xdpf);
+	struct tsnep_tx_entry *entry;
+	int count = 1;
+	int length;
+	int retval;
+	int i;
+
+	if (unlikely(xdp_frame_has_frags(xdpf)))
+		count += shinfo->nr_frags;
+
+	spin_lock_bh(&tx->lock);
+
+	/* ensure that TX ring is not filled up by XDP, always MAX_SKB_FRAGS
+	 * will be available for normal TX path and queue is stopped there if
+	 * necessary
+	 */
+	if (tsnep_tx_desc_available(tx) < (MAX_SKB_FRAGS + 1 + count)) {
+		spin_unlock_bh(&tx->lock);
+
+		return false;
+	}
+
+	entry = &tx->entry[tx->write];
+	entry->xdpf = xdpf;
+
+	retval = tsnep_xdp_tx_map(xdpf, tx, shinfo, count, type);
+	if (retval < 0) {
+		tsnep_tx_unmap(tx, tx->write, count);
+		entry->xdpf = NULL;
+
+		tx->dropped++;
+
+		spin_unlock_bh(&tx->lock);
+
+		return false;
+	}
+	length = retval;
+
+	for (i = 0; i < count; i++)
+		tsnep_tx_activate(tx, (tx->write + i) % TSNEP_RING_SIZE, length,
+				  i == (count - 1));
+	tx->write = (tx->write + count) % TSNEP_RING_SIZE;
+
+	/* descriptor properties shall be valid before hardware is notified */
+	dma_wmb();
+
+	spin_unlock_bh(&tx->lock);
+
+	return true;
+}
+
+static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)
+{
+	iowrite32(TSNEP_CONTROL_TX_ENABLE, tx->addr + TSNEP_CONTROL);
+}
+
 static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 {
-	int budget = 128;
 	struct tsnep_tx_entry *entry;
-	int count;
+	struct xdp_frame_bulk bq;
+	int budget = 128;
 	int length;
+	int count;
+
+	xdp_frame_bulk_init(&bq);
+
+	rcu_read_lock(); /* need for xdp_return_frame_bulk */
 
 	spin_lock_bh(&tx->lock);
 
@@ -527,12 +653,17 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 		dma_rmb();
 
 		count = 1;
-		if (skb_shinfo(entry->skb)->nr_frags > 0)
+		if (entry->type == TSNEP_TX_TYPE_SKB &&
+		    skb_shinfo(entry->skb)->nr_frags > 0)
 			count += skb_shinfo(entry->skb)->nr_frags;
+		else if (entry->type != TSNEP_TX_TYPE_SKB &&
+			 xdp_frame_has_frags(entry->xdpf))
+			count += xdp_get_shared_info_from_frame(entry->xdpf)->nr_frags;
 
 		length = tsnep_tx_unmap(tx, tx->read, count);
 
-		if ((skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS) &&
+		if (entry->type == TSNEP_TX_TYPE_SKB &&
+		    (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS) &&
 		    (__le32_to_cpu(entry->desc_wb->properties) &
 		     TSNEP_DESC_EXTENDED_WRITEBACK_FLAG)) {
 			struct skb_shared_hwtstamps hwtstamps;
@@ -552,8 +683,20 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 			skb_tstamp_tx(entry->skb, &hwtstamps);
 		}
 
-		napi_consume_skb(entry->skb, budget);
-		entry->skb = NULL;
+		switch (entry->type) {
+		case TSNEP_TX_TYPE_SKB:
+			napi_consume_skb(entry->skb, budget);
+			entry->skb = NULL;
+			break;
+		case TSNEP_TX_TYPE_XDP_TX:
+			xdp_return_frame_rx_napi(entry->xdpf);
+			entry->xdpf = NULL;
+			break;
+		case TSNEP_TX_TYPE_XDP_NDO:
+			xdp_return_frame_bulk(entry->xdpf, &bq);
+			entry->xdpf = NULL;
+			break;
+		}
 
 		tx->read = (tx->read + count) % TSNEP_RING_SIZE;
 
@@ -570,6 +713,10 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 
 	spin_unlock_bh(&tx->lock);
 
+	xdp_flush_frame_bulk(&bq);
+
+	rcu_read_unlock();
+
 	return (budget != 0);
 }
 
@@ -1330,6 +1477,46 @@ static ktime_t tsnep_netdev_get_tstamp(struct net_device *netdev,
 	return ns_to_ktime(timestamp);
 }
 
+static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
+				 struct xdp_frame **xdp, u32 flags)
+{
+	struct tsnep_adapter *adapter = netdev_priv(dev);
+	int cpu = smp_processor_id();
+	struct netdev_queue *nq;
+	int nxmit;
+	int queue;
+	bool xmit;
+
+	if (unlikely(test_bit(__TSNEP_DOWN, &adapter->state)))
+		return -ENETDOWN;
+
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
+		return -EINVAL;
+
+	queue = cpu % adapter->num_tx_queues;
+	nq = netdev_get_tx_queue(adapter->netdev, queue);
+
+	__netif_tx_lock(nq, cpu);
+
+	/* Avoid transmit queue timeout since we share it with the slow path */
+	txq_trans_cond_update(nq);
+
+	for (nxmit = 0; nxmit < n; nxmit++) {
+		xmit = tsnep_xdp_xmit_frame_ring(xdp[nxmit],
+						 &adapter->tx[queue],
+						 TSNEP_TX_TYPE_XDP_NDO);
+		if (!xmit)
+			break;
+	}
+
+	if (flags & XDP_XMIT_FLUSH)
+		tsnep_xdp_xmit_flush(&adapter->tx[queue]);
+
+	__netif_tx_unlock(nq);
+
+	return nxmit;
+}
+
 static const struct net_device_ops tsnep_netdev_ops = {
 	.ndo_open = tsnep_netdev_open,
 	.ndo_stop = tsnep_netdev_close,
@@ -1341,6 +1528,7 @@ static const struct net_device_ops tsnep_netdev_ops = {
 	.ndo_set_features = tsnep_netdev_set_features,
 	.ndo_get_tstamp = tsnep_netdev_get_tstamp,
 	.ndo_setup_tc = tsnep_tc_setup,
+	.ndo_xdp_xmit = tsnep_netdev_xdp_xmit,
 };
 
 static int tsnep_mac_init(struct tsnep_adapter *adapter)
-- 
2.30.2

[PATCH net-next v3 5/9] tsnep: Substract TSNEP_RX_INLINE_METADATA_SIZE once

[Gerhard Engleder]

Subtrace size of metadata in front of received data only once. This
simplifies the RX code.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 2c7252ded23a..9a666dbbe8cd 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -980,7 +980,7 @@ static struct sk_buff *tsnep_build_skb(struct tsnep_rx *rx, struct page *page,
 
 	/* update pointers within the skb to store the data */
 	skb_reserve(skb, TSNEP_SKB_PAD + TSNEP_RX_INLINE_METADATA_SIZE);
-	__skb_put(skb, length - TSNEP_RX_INLINE_METADATA_SIZE - ETH_FCS_LEN);
+	__skb_put(skb, length - ETH_FCS_LEN);
 
 	if (rx->adapter->hwtstamp_config.rx_filter == HWTSTAMP_FILTER_ALL) {
 		struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
@@ -1052,6 +1052,13 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
 		dma_sync_single_range_for_cpu(dmadev, entry->dma, TSNEP_SKB_PAD,
 					      length, dma_dir);
 
+		/* RX metadata with timestamps is in front of actual data,
+		 * subtract metadata size to get length of actual data and
+		 * consider metadata size as offset of actual data during RX
+		 * processing
+		 */
+		length -= TSNEP_RX_INLINE_METADATA_SIZE;
+
 		rx->read = (rx->read + 1) % TSNEP_RING_SIZE;
 		desc_available++;
 
@@ -1060,7 +1067,7 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
 			page_pool_release_page(rx->page_pool, entry->page);
 
 			rx->packets++;
-			rx->bytes += length - TSNEP_RX_INLINE_METADATA_SIZE;
+			rx->bytes += length;
 			if (skb->pkt_type == PACKET_MULTICAST)
 				rx->multicast++;
 
-- 
2.30.2

[PATCH net-next v3 6/9] tsnep: Support XDP BPF program setup

[Gerhard Engleder]

Implement setup of BPF programs for XDP RX path with command
XDP_SETUP_PROG of ndo_bpf(). This is prework for XDP RX path support.

tsnep_netdev_close() is called directly during BPF program setup. Add
netif_carrier_off() and netif_tx_stop_all_queues() calls to signal to
network stack that device is down. Otherwise network stack would
continue transmitting pakets.

Return value of tsnep_netdev_open() is not checked during BPF program
setup like in other drivers. Forwarding the return value would result in
a bpf_prog_put() call in dev_xdp_install(), which would make removal of
BPF program necessary.

If tsnep_netdev_open() fails during BPF program setup, then the network
stack would call tsnep_netdev_close() anyway. Thus, tsnep_netdev_close()
checks now if device is already down.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/Makefile     |  2 +-
 drivers/net/ethernet/engleder/tsnep.h      | 13 +++++++++++
 drivers/net/ethernet/engleder/tsnep_main.c | 25 +++++++++++++++++---
 drivers/net/ethernet/engleder/tsnep_xdp.c  | 27 ++++++++++++++++++++++
 4 files changed, 63 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/ethernet/engleder/tsnep_xdp.c

diff --git a/drivers/net/ethernet/engleder/Makefile b/drivers/net/ethernet/engleder/Makefile
index b6e3b16623de..0901801cfcc9 100644
--- a/drivers/net/ethernet/engleder/Makefile
+++ b/drivers/net/ethernet/engleder/Makefile
@@ -6,5 +6,5 @@
 obj-$(CONFIG_TSNEP) += tsnep.o
 
 tsnep-objs := tsnep_main.o tsnep_ethtool.o tsnep_ptp.o tsnep_tc.o \
-	      tsnep_rxnfc.o $(tsnep-y)
+	      tsnep_rxnfc.o tsnep_xdp.o $(tsnep-y)
 tsnep-$(CONFIG_TSNEP_SELFTESTS) += tsnep_selftests.o
diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
index 29b04127f529..0e7fc36a64e1 100644
--- a/drivers/net/ethernet/engleder/tsnep.h
+++ b/drivers/net/ethernet/engleder/tsnep.h
@@ -183,6 +183,8 @@ struct tsnep_adapter {
 	int rxnfc_count;
 	int rxnfc_max;
 
+	struct bpf_prog *xdp_prog;
+
 	int num_tx_queues;
 	struct tsnep_tx tx[TSNEP_MAX_QUEUES];
 	int num_rx_queues;
@@ -192,6 +194,9 @@ struct tsnep_adapter {
 	struct tsnep_queue queue[TSNEP_MAX_QUEUES];
 };
 
+int tsnep_netdev_open(struct net_device *netdev);
+int tsnep_netdev_close(struct net_device *netdev);
+
 extern const struct ethtool_ops tsnep_ethtool_ops;
 
 int tsnep_ptp_init(struct tsnep_adapter *adapter);
@@ -215,6 +220,14 @@ int tsnep_rxnfc_add_rule(struct tsnep_adapter *adapter,
 int tsnep_rxnfc_del_rule(struct tsnep_adapter *adapter,
 			 struct ethtool_rxnfc *cmd);
 
+int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
+			 struct netlink_ext_ack *extack);
+
+static inline bool tsnep_xdp_is_enabled(struct tsnep_adapter *adapter)
+{
+	return !!adapter->xdp_prog;
+}
+
 #if IS_ENABLED(CONFIG_TSNEP_SELFTESTS)
 int tsnep_ethtool_get_test_count(void);
 void tsnep_ethtool_get_test_strings(u8 *data);
diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 9a666dbbe8cd..a603b79b7411 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -1236,7 +1236,7 @@ static void tsnep_free_irq(struct tsnep_queue *queue, bool first)
 	memset(queue->name, 0, sizeof(queue->name));
 }
 
-static int tsnep_netdev_open(struct net_device *netdev)
+int tsnep_netdev_open(struct net_device *netdev)
 {
 	struct tsnep_adapter *adapter = netdev_priv(netdev);
 	int i;
@@ -1296,6 +1296,8 @@ static int tsnep_netdev_open(struct net_device *netdev)
 		tsnep_enable_irq(adapter, adapter->queue[i].irq_mask);
 	}
 
+	netif_tx_start_all_queues(adapter->netdev);
+
 	clear_bit(__TSNEP_DOWN, &adapter->state);
 
 	return 0;
@@ -1315,12 +1317,16 @@ static int tsnep_netdev_open(struct net_device *netdev)
 	return retval;
 }
 
-static int tsnep_netdev_close(struct net_device *netdev)
+int tsnep_netdev_close(struct net_device *netdev)
 {
 	struct tsnep_adapter *adapter = netdev_priv(netdev);
 	int i;
 
-	set_bit(__TSNEP_DOWN, &adapter->state);
+	if (test_and_set_bit(__TSNEP_DOWN, &adapter->state))
+		return 0;
+
+	netif_carrier_off(netdev);
+	netif_tx_stop_all_queues(netdev);
 
 	tsnep_disable_irq(adapter, ECM_INT_LINK);
 	tsnep_phy_close(adapter);
@@ -1484,6 +1490,18 @@ static ktime_t tsnep_netdev_get_tstamp(struct net_device *netdev,
 	return ns_to_ktime(timestamp);
 }
 
+static int tsnep_netdev_bpf(struct net_device *dev, struct netdev_bpf *bpf)
+{
+	struct tsnep_adapter *adapter = netdev_priv(dev);
+
+	switch (bpf->command) {
+	case XDP_SETUP_PROG:
+		return tsnep_xdp_setup_prog(adapter, bpf->prog, bpf->extack);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
 				 struct xdp_frame **xdp, u32 flags)
 {
@@ -1535,6 +1553,7 @@ static const struct net_device_ops tsnep_netdev_ops = {
 	.ndo_set_features = tsnep_netdev_set_features,
 	.ndo_get_tstamp = tsnep_netdev_get_tstamp,
 	.ndo_setup_tc = tsnep_tc_setup,
+	.ndo_bpf = tsnep_netdev_bpf,
 	.ndo_xdp_xmit = tsnep_netdev_xdp_xmit,
 };
 
diff --git a/drivers/net/ethernet/engleder/tsnep_xdp.c b/drivers/net/ethernet/engleder/tsnep_xdp.c
new file mode 100644
index 000000000000..02d84dfbdde4
--- /dev/null
+++ b/drivers/net/ethernet/engleder/tsnep_xdp.c
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2022 Gerhard Engleder <gerhard@engleder-embedded.com> */
+
+#include <linux/if_vlan.h>
+#include <net/xdp_sock_drv.h>
+
+#include "tsnep.h"
+
+int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
+			 struct netlink_ext_ack *extack)
+{
+	struct net_device *dev = adapter->netdev;
+	bool if_running = netif_running(dev);
+	struct bpf_prog *old_prog;
+
+	if (if_running)
+		tsnep_netdev_close(dev);
+
+	old_prog = xchg(&adapter->xdp_prog, prog);
+	if (old_prog)
+		bpf_prog_put(old_prog);
+
+	if (if_running)
+		tsnep_netdev_open(dev);
+
+	return 0;
+}
-- 
2.30.2

[PATCH net-next v3 7/9] tsnep: Prepare RX buffer for XDP support

[Gerhard Engleder]

Reserve XDP_PACKET_HEADROOM in front of RX buffer if XDP is enabled.
Also set DMA direction properly in this case.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
Reviewed-by: Saeed Mahameed <saeed@kernel.org>
---
 drivers/net/ethernet/engleder/tsnep_main.c | 31 +++++++++++++++-------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index a603b79b7411..b24a00782f27 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -26,9 +26,10 @@
 #include <linux/etherdevice.h>
 #include <linux/phy.h>
 #include <linux/iopoll.h>
+#include <linux/bpf.h>
 
 #define TSNEP_SKB_PAD (NET_SKB_PAD + NET_IP_ALIGN)
-#define TSNEP_HEADROOM ALIGN(TSNEP_SKB_PAD, 4)
+#define TSNEP_HEADROOM ALIGN(max(TSNEP_SKB_PAD, XDP_PACKET_HEADROOM), 4)
 #define TSNEP_MAX_RX_BUF_SIZE (PAGE_SIZE - TSNEP_HEADROOM - \
 			       SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
 
@@ -777,6 +778,16 @@ static void tsnep_tx_close(struct tsnep_tx *tx)
 	tsnep_tx_ring_cleanup(tx);
 }
 
+static unsigned int tsnep_rx_offset(struct tsnep_rx *rx)
+{
+	struct tsnep_adapter *adapter = rx->adapter;
+
+	if (tsnep_xdp_is_enabled(adapter))
+		return XDP_PACKET_HEADROOM;
+
+	return TSNEP_SKB_PAD;
+}
+
 static void tsnep_rx_ring_cleanup(struct tsnep_rx *rx)
 {
 	struct device *dmadev = rx->adapter->dmadev;
@@ -838,9 +849,10 @@ static int tsnep_rx_ring_init(struct tsnep_rx *rx)
 	pp_params.pool_size = TSNEP_RING_SIZE;
 	pp_params.nid = dev_to_node(dmadev);
 	pp_params.dev = dmadev;
-	pp_params.dma_dir = DMA_FROM_DEVICE;
+	pp_params.dma_dir = tsnep_xdp_is_enabled(rx->adapter) ?
+			    DMA_BIDIRECTIONAL : DMA_FROM_DEVICE;
 	pp_params.max_len = TSNEP_MAX_RX_BUF_SIZE;
-	pp_params.offset = TSNEP_SKB_PAD;
+	pp_params.offset = tsnep_rx_offset(rx);
 	rx->page_pool = page_pool_create(&pp_params);
 	if (IS_ERR(rx->page_pool)) {
 		retval = PTR_ERR(rx->page_pool);
@@ -875,7 +887,7 @@ static void tsnep_rx_set_page(struct tsnep_rx *rx, struct tsnep_rx_entry *entry,
 	entry->page = page;
 	entry->len = TSNEP_MAX_RX_BUF_SIZE;
 	entry->dma = page_pool_get_dma_addr(entry->page);
-	entry->desc->rx = __cpu_to_le64(entry->dma + TSNEP_SKB_PAD);
+	entry->desc->rx = __cpu_to_le64(entry->dma + tsnep_rx_offset(rx));
 }
 
 static int tsnep_rx_alloc_buffer(struct tsnep_rx *rx, int index)
@@ -979,14 +991,14 @@ static struct sk_buff *tsnep_build_skb(struct tsnep_rx *rx, struct page *page,
 		return NULL;
 
 	/* update pointers within the skb to store the data */
-	skb_reserve(skb, TSNEP_SKB_PAD + TSNEP_RX_INLINE_METADATA_SIZE);
+	skb_reserve(skb, tsnep_rx_offset(rx) + TSNEP_RX_INLINE_METADATA_SIZE);
 	__skb_put(skb, length - ETH_FCS_LEN);
 
 	if (rx->adapter->hwtstamp_config.rx_filter == HWTSTAMP_FILTER_ALL) {
 		struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
 		struct tsnep_rx_inline *rx_inline =
 			(struct tsnep_rx_inline *)(page_address(page) +
-						   TSNEP_SKB_PAD);
+						   tsnep_rx_offset(rx));
 
 		skb_shinfo(skb)->tx_flags |=
 			SKBTX_HW_TSTAMP_NETDEV;
@@ -1046,11 +1058,12 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
 		 */
 		dma_rmb();
 
-		prefetch(page_address(entry->page) + TSNEP_SKB_PAD);
+		prefetch(page_address(entry->page) + tsnep_rx_offset(rx));
 		length = __le32_to_cpu(entry->desc_wb->properties) &
 			 TSNEP_DESC_LENGTH_MASK;
-		dma_sync_single_range_for_cpu(dmadev, entry->dma, TSNEP_SKB_PAD,
-					      length, dma_dir);
+		dma_sync_single_range_for_cpu(dmadev, entry->dma,
+					      tsnep_rx_offset(rx), length,
+					      dma_dir);
 
 		/* RX metadata with timestamps is in front of actual data,
 		 * subtract metadata size to get length of actual data and
-- 
2.30.2

[PATCH net-next v3 8/9] tsnep: Add RX queue info for XDP support

[Gerhard Engleder]

Register xdp_rxq_info with page_pool memory model. This is needed for
XDP buffer handling.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
Reviewed-by: Saeed Mahameed <saeed@kernel.org>
---
 drivers/net/ethernet/engleder/tsnep.h      |  6 ++--
 drivers/net/ethernet/engleder/tsnep_main.c | 34 +++++++++++++++++-----
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
index 0e7fc36a64e1..0210dab90f71 100644
--- a/drivers/net/ethernet/engleder/tsnep.h
+++ b/drivers/net/ethernet/engleder/tsnep.h
@@ -133,17 +133,19 @@ struct tsnep_rx {
 	u32 dropped;
 	u32 multicast;
 	u32 alloc_failed;
+
+	struct xdp_rxq_info xdp_rxq;
 };
 
 struct tsnep_queue {
 	struct tsnep_adapter *adapter;
 	char name[IFNAMSIZ + 9];
 
+	struct napi_struct napi;
+
 	struct tsnep_tx *tx;
 	struct tsnep_rx *rx;
 
-	struct napi_struct napi;
-
 	int irq;
 	u32 irq_mask;
 	void __iomem *irq_delay_addr;
diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index b24a00782f27..6a30f3bf73a6 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -802,6 +802,9 @@ static void tsnep_rx_ring_cleanup(struct tsnep_rx *rx)
 		entry->page = NULL;
 	}
 
+	if (xdp_rxq_info_is_reg(&rx->xdp_rxq))
+		xdp_rxq_info_unreg(&rx->xdp_rxq);
+
 	if (rx->page_pool)
 		page_pool_destroy(rx->page_pool);
 
@@ -817,7 +820,7 @@ static void tsnep_rx_ring_cleanup(struct tsnep_rx *rx)
 	}
 }
 
-static int tsnep_rx_ring_init(struct tsnep_rx *rx)
+static int tsnep_rx_ring_init(struct tsnep_rx *rx, unsigned int napi_id)
 {
 	struct device *dmadev = rx->adapter->dmadev;
 	struct tsnep_rx_entry *entry;
@@ -860,6 +863,15 @@ static int tsnep_rx_ring_init(struct tsnep_rx *rx)
 		goto failed;
 	}
 
+	retval = xdp_rxq_info_reg(&rx->xdp_rxq, rx->adapter->netdev,
+				  rx->queue_index, napi_id);
+	if (retval)
+		goto failed;
+	retval = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq, MEM_TYPE_PAGE_POOL,
+					    rx->page_pool);
+	if (retval)
+		goto failed;
+
 	for (i = 0; i < TSNEP_RING_SIZE; i++) {
 		entry = &rx->entry[i];
 		next_entry = &rx->entry[(i + 1) % TSNEP_RING_SIZE];
@@ -1115,7 +1127,8 @@ static bool tsnep_rx_pending(struct tsnep_rx *rx)
 }
 
 static int tsnep_rx_open(struct tsnep_adapter *adapter, void __iomem *addr,
-			 int queue_index, struct tsnep_rx *rx)
+			 unsigned int napi_id, int queue_index,
+			 struct tsnep_rx *rx)
 {
 	dma_addr_t dma;
 	int retval;
@@ -1125,7 +1138,7 @@ static int tsnep_rx_open(struct tsnep_adapter *adapter, void __iomem *addr,
 	rx->addr = addr;
 	rx->queue_index = queue_index;
 
-	retval = tsnep_rx_ring_init(rx);
+	retval = tsnep_rx_ring_init(rx, napi_id);
 	if (retval)
 		return retval;
 
@@ -1253,6 +1266,7 @@ int tsnep_netdev_open(struct net_device *netdev)
 {
 	struct tsnep_adapter *adapter = netdev_priv(netdev);
 	int i;
+	unsigned int napi_id;
 	void __iomem *addr;
 	int tx_queue_index = 0;
 	int rx_queue_index = 0;
@@ -1260,6 +1274,11 @@ int tsnep_netdev_open(struct net_device *netdev)
 
 	for (i = 0; i < adapter->num_queues; i++) {
 		adapter->queue[i].adapter = adapter;
+
+		netif_napi_add(adapter->netdev, &adapter->queue[i].napi,
+			       tsnep_poll);
+		napi_id = adapter->queue[i].napi.napi_id;
+
 		if (adapter->queue[i].tx) {
 			addr = adapter->addr + TSNEP_QUEUE(tx_queue_index);
 			retval = tsnep_tx_open(adapter, addr, tx_queue_index,
@@ -1270,7 +1289,7 @@ int tsnep_netdev_open(struct net_device *netdev)
 		}
 		if (adapter->queue[i].rx) {
 			addr = adapter->addr + TSNEP_QUEUE(rx_queue_index);
-			retval = tsnep_rx_open(adapter, addr,
+			retval = tsnep_rx_open(adapter, addr, napi_id,
 					       rx_queue_index,
 					       adapter->queue[i].rx);
 			if (retval)
@@ -1302,8 +1321,6 @@ int tsnep_netdev_open(struct net_device *netdev)
 		goto phy_failed;
 
 	for (i = 0; i < adapter->num_queues; i++) {
-		netif_napi_add(adapter->netdev, &adapter->queue[i].napi,
-			       tsnep_poll);
 		napi_enable(&adapter->queue[i].napi);
 
 		tsnep_enable_irq(adapter, adapter->queue[i].irq_mask);
@@ -1326,6 +1343,8 @@ int tsnep_netdev_open(struct net_device *netdev)
 			tsnep_rx_close(adapter->queue[i].rx);
 		if (adapter->queue[i].tx)
 			tsnep_tx_close(adapter->queue[i].tx);
+
+		netif_napi_del(&adapter->queue[i].napi);
 	}
 	return retval;
 }
@@ -1348,7 +1367,6 @@ int tsnep_netdev_close(struct net_device *netdev)
 		tsnep_disable_irq(adapter, adapter->queue[i].irq_mask);
 
 		napi_disable(&adapter->queue[i].napi);
-		netif_napi_del(&adapter->queue[i].napi);
 
 		tsnep_free_irq(&adapter->queue[i], i == 0);
 
@@ -1356,6 +1374,8 @@ int tsnep_netdev_close(struct net_device *netdev)
 			tsnep_rx_close(adapter->queue[i].rx);
 		if (adapter->queue[i].tx)
 			tsnep_tx_close(adapter->queue[i].tx);
+
+		netif_napi_del(&adapter->queue[i].napi);
 	}
 
 	return 0;
-- 
2.30.2

[PATCH net-next v3 9/9] tsnep: Add XDP RX support

[Gerhard Engleder]

If BPF program is set up, then run BPF program for every received frame
and execute the selected action.

Test results with A53 1.2GHz:

XDP_DROP (samples/bpf/xdp1)
proto 17:     883878 pkt/s

XDP_TX (samples/bpf/xdp2)
proto 17:     255693 pkt/s

XDP_REDIRECT (samples/bpf/xdpsock)
 sock0@eth2:0 rxdrop xdp-drv
                   pps            pkts           1.00
rx                 855,582        5,404,523
tx                 0              0

XDP_REDIRECT (samples/bpf/xdp_redirect)
eth2->eth1         613,267 rx/s   0 err,drop/s   613,272 xmit/s

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_main.c | 129 ++++++++++++++++++++-
 1 file changed, 127 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 6a30f3bf73a6..beafaa60a35d 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -27,6 +27,7 @@
 #include <linux/phy.h>
 #include <linux/iopoll.h>
 #include <linux/bpf.h>
+#include <linux/bpf_trace.h>
 
 #define TSNEP_SKB_PAD (NET_SKB_PAD + NET_IP_ALIGN)
 #define TSNEP_HEADROOM ALIGN(max(TSNEP_SKB_PAD, XDP_PACKET_HEADROOM), 4)
@@ -44,6 +45,9 @@
 #define TSNEP_COALESCE_USECS_MAX     ((ECM_INT_DELAY_MASK >> ECM_INT_DELAY_SHIFT) * \
 				      ECM_INT_DELAY_BASE_US + ECM_INT_DELAY_BASE_US - 1)
 
+#define TSNEP_XDP_TX		BIT(0)
+#define TSNEP_XDP_REDIRECT	BIT(1)
+
 enum {
 	__TSNEP_DOWN,
 };
@@ -624,6 +628,34 @@ static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)
 	iowrite32(TSNEP_CONTROL_TX_ENABLE, tx->addr + TSNEP_CONTROL);
 }
 
+static bool tsnep_xdp_xmit_back(struct tsnep_adapter *adapter,
+				struct xdp_buff *xdp)
+{
+	struct xdp_frame *xdpf = xdp_convert_buff_to_frame(xdp);
+	int cpu = smp_processor_id();
+	struct netdev_queue *nq;
+	int queue;
+	bool xmit;
+
+	if (unlikely(!xdpf))
+		return -EFAULT;
+
+	queue = cpu % adapter->num_tx_queues;
+	nq = netdev_get_tx_queue(adapter->netdev, queue);
+
+	__netif_tx_lock(nq, cpu);
+
+	/* Avoid transmit queue timeout since we share it with the slow path */
+	txq_trans_cond_update(nq);
+
+	xmit = tsnep_xdp_xmit_frame_ring(xdpf, &adapter->tx[queue],
+					 TSNEP_TX_TYPE_XDP_TX);
+
+	__netif_tx_unlock(nq);
+
+	return xmit;
+}
+
 static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 {
 	struct tsnep_tx_entry *entry;
@@ -788,6 +820,11 @@ static unsigned int tsnep_rx_offset(struct tsnep_rx *rx)
 	return TSNEP_SKB_PAD;
 }
 
+static unsigned int tsnep_rx_offset_xdp(void)
+{
+	return XDP_PACKET_HEADROOM;
+}
+
 static void tsnep_rx_ring_cleanup(struct tsnep_rx *rx)
 {
 	struct device *dmadev = rx->adapter->dmadev;
@@ -993,6 +1030,67 @@ static int tsnep_rx_refill(struct tsnep_rx *rx, int count, bool reuse)
 	return i;
 }
 
+static bool tsnep_xdp_run_prog(struct tsnep_rx *rx, struct bpf_prog *prog,
+			       struct xdp_buff *xdp, int *status)
+{
+	unsigned int length;
+	unsigned int sync;
+	u32 act;
+
+	length = xdp->data_end - xdp->data_hard_start - tsnep_rx_offset_xdp();
+
+	act = bpf_prog_run_xdp(prog, xdp);
+
+	/* Due xdp_adjust_tail: DMA sync for_device cover max len CPU touch */
+	sync = xdp->data_end - xdp->data_hard_start - tsnep_rx_offset_xdp();
+	sync = max(sync, length);
+
+	switch (act) {
+	case XDP_PASS:
+		return false;
+	case XDP_TX:
+		if (!tsnep_xdp_xmit_back(rx->adapter, xdp))
+			goto out_failure;
+		*status |= TSNEP_XDP_TX;
+		return true;
+	case XDP_REDIRECT:
+		if (xdp_do_redirect(rx->adapter->netdev, xdp, prog) < 0)
+			goto out_failure;
+		*status |= TSNEP_XDP_REDIRECT;
+		return true;
+	default:
+		bpf_warn_invalid_xdp_action(rx->adapter->netdev, prog, act);
+		fallthrough;
+	case XDP_ABORTED:
+out_failure:
+		trace_xdp_exception(rx->adapter->netdev, prog, act);
+		fallthrough;
+	case XDP_DROP:
+		page_pool_put_page(rx->page_pool, virt_to_head_page(xdp->data),
+				   sync, true);
+		return true;
+	}
+}
+
+static void tsnep_finalize_xdp(struct tsnep_adapter *adapter, int status)
+{
+	int cpu = smp_processor_id();
+	struct netdev_queue *nq;
+	int queue;
+
+	if (status & TSNEP_XDP_TX) {
+		queue = cpu % adapter->num_tx_queues;
+		nq = netdev_get_tx_queue(adapter->netdev, queue);
+
+		__netif_tx_lock(nq, cpu);
+		tsnep_xdp_xmit_flush(&adapter->tx[queue]);
+		__netif_tx_unlock(nq);
+	}
+
+	if (status & TSNEP_XDP_REDIRECT)
+		xdp_do_flush();
+}
+
 static struct sk_buff *tsnep_build_skb(struct tsnep_rx *rx, struct page *page,
 				       int length)
 {
@@ -1028,15 +1126,19 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
 			 int budget)
 {
 	struct device *dmadev = rx->adapter->dmadev;
-	int desc_available;
-	int done = 0;
 	enum dma_data_direction dma_dir;
 	struct tsnep_rx_entry *entry;
+	struct bpf_prog *prog;
+	struct xdp_buff xdp;
 	struct sk_buff *skb;
+	int desc_available;
+	int xdp_status = 0;
+	int done = 0;
 	int length;
 
 	desc_available = tsnep_rx_desc_available(rx);
 	dma_dir = page_pool_get_dma_dir(rx->page_pool);
+	prog = READ_ONCE(rx->adapter->xdp_prog);
 
 	while (likely(done < budget) && (rx->read != rx->write)) {
 		entry = &rx->entry[rx->read];
@@ -1087,6 +1189,26 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
 		rx->read = (rx->read + 1) % TSNEP_RING_SIZE;
 		desc_available++;
 
+		if (prog) {
+			bool consume;
+
+			xdp_init_buff(&xdp, PAGE_SIZE, &rx->xdp_rxq);
+			xdp_prepare_buff(&xdp, page_address(entry->page),
+					 tsnep_rx_offset_xdp() + TSNEP_RX_INLINE_METADATA_SIZE,
+					 length, false);
+
+			consume = tsnep_xdp_run_prog(rx, prog, &xdp,
+						     &xdp_status);
+			if (consume) {
+				rx->packets++;
+				rx->bytes += length;
+
+				entry->page = NULL;
+
+				continue;
+			}
+		}
+
 		skb = tsnep_build_skb(rx, entry->page, length);
 		if (skb) {
 			page_pool_release_page(rx->page_pool, entry->page);
@@ -1105,6 +1227,9 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
 		entry->page = NULL;
 	}
 
+	if (xdp_status)
+		tsnep_finalize_xdp(rx->adapter, xdp_status);
+
 	if (desc_available)
 		tsnep_rx_refill(rx, desc_available, false);
 
-- 
2.30.2

Re: [PATCH net-next v3 1/9] tsnep: Use spin_lock_bh for TX

[Alexander Lobakin]

From: Gerhard Engleder <gerhard@engleder-embedded.com>
Date: Wed Jan 04 2023 20:41:24 GMT+0100

> TX processing is done only within process or BH context. Therefore,
> _irqsafe variant is not necessary.

NAPI and .ndo_{start,xdp}_xmit are BH-only, where can the process
context happen here?

> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/tsnep_main.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> index bf0190e1d2ea..7cc5e2407809 100644
> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -434,7 +434,6 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)
>  static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
>  					 struct tsnep_tx *tx)
>  {
> -	unsigned long flags;
>  	int count = 1;
>  	struct tsnep_tx_entry *entry;
>  	int length;
> @@ -444,7 +443,7 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
>  	if (skb_shinfo(skb)->nr_frags > 0)
>  		count += skb_shinfo(skb)->nr_frags;
>  
> -	spin_lock_irqsave(&tx->lock, flags);
> +	spin_lock_bh(&tx->lock);
>  
>  	if (tsnep_tx_desc_available(tx) < count) {
>  		/* ring full, shall not happen because queue is stopped if full
> @@ -452,7 +451,7 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
>  		 */
>  		netif_stop_queue(tx->adapter->netdev);
>  
> -		spin_unlock_irqrestore(&tx->lock, flags);
> +		spin_unlock_bh(&tx->lock);
>  
>  		return NETDEV_TX_BUSY;
>  	}
> @@ -468,7 +467,7 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
>  
>  		tx->dropped++;
>  
> -		spin_unlock_irqrestore(&tx->lock, flags);
> +		spin_unlock_bh(&tx->lock);
>  
>  		netdev_err(tx->adapter->netdev, "TX DMA map failed\n");
>  
> @@ -496,20 +495,19 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
>  		netif_stop_queue(tx->adapter->netdev);
>  	}
>  
> -	spin_unlock_irqrestore(&tx->lock, flags);
> +	spin_unlock_bh(&tx->lock);
>  
>  	return NETDEV_TX_OK;
>  }
>  
>  static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>  {
> -	unsigned long flags;
>  	int budget = 128;
>  	struct tsnep_tx_entry *entry;
>  	int count;
>  	int length;
>  
> -	spin_lock_irqsave(&tx->lock, flags);
> +	spin_lock_bh(&tx->lock);
>  
>  	do {
>  		if (tx->read == tx->write)
> @@ -568,18 +566,17 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>  		netif_wake_queue(tx->adapter->netdev);
>  	}
>  
> -	spin_unlock_irqrestore(&tx->lock, flags);
> +	spin_unlock_bh(&tx->lock);
>  
>  	return (budget != 0);
>  }
>  
>  static bool tsnep_tx_pending(struct tsnep_tx *tx)
>  {
> -	unsigned long flags;
>  	struct tsnep_tx_entry *entry;
>  	bool pending = false;
>  
> -	spin_lock_irqsave(&tx->lock, flags);
> +	spin_lock_bh(&tx->lock);
>  
>  	if (tx->read != tx->write) {
>  		entry = &tx->entry[tx->read];
> @@ -589,7 +586,7 @@ static bool tsnep_tx_pending(struct tsnep_tx *tx)
>  			pending = true;
>  	}
>  
> -	spin_unlock_irqrestore(&tx->lock, flags);
> +	spin_unlock_bh(&tx->lock);
>  
>  	return pending;
>  }

Thanks,
Olek

Re: [PATCH net-next v3 3/9] tsnep: Add adapter down state

[Alexander Lobakin]

From: Gerhard Engleder <gerhard@engleder-embedded.com>
Date: Wed Jan 04 2023 20:41:26 GMT+0100

> Add adapter state with flag for down state. This flag will be used by
> the XDP TX path to deny TX if adapter is down.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/tsnep.h      |  1 +
>  drivers/net/ethernet/engleder/tsnep_main.c | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
> index f93ba48bac3f..f72c0c4da1a9 100644
> --- a/drivers/net/ethernet/engleder/tsnep.h
> +++ b/drivers/net/ethernet/engleder/tsnep.h
> @@ -148,6 +148,7 @@ struct tsnep_adapter {
>  	phy_interface_t phy_mode;
>  	struct phy_device *phydev;
>  	int msg_enable;
> +	unsigned long state;

Now there will be a 4-byte hole in between ::msg_enable and ::state,
maybe add ::state right after ::phydev?

>  
>  	struct platform_device *pdev;
>  	struct device *dmadev;
> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> index 0d40728dcfca..56c8cae6251e 100644
> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -43,6 +43,10 @@
>  #define TSNEP_COALESCE_USECS_MAX     ((ECM_INT_DELAY_MASK >> ECM_INT_DELAY_SHIFT) * \
>  				      ECM_INT_DELAY_BASE_US + ECM_INT_DELAY_BASE_US - 1)
>  
> +enum {
> +	__TSNEP_DOWN,
> +};
> +
>  static void tsnep_enable_irq(struct tsnep_adapter *adapter, u32 mask)
>  {
>  	iowrite32(mask, adapter->addr + ECM_INT_ENABLE);
> @@ -1138,6 +1142,8 @@ static int tsnep_netdev_open(struct net_device *netdev)
>  		tsnep_enable_irq(adapter, adapter->queue[i].irq_mask);
>  	}
>  
> +	clear_bit(__TSNEP_DOWN, &adapter->state);
> +
>  	return 0;
>  
>  phy_failed:
> @@ -1160,6 +1166,8 @@ static int tsnep_netdev_close(struct net_device *netdev)
>  	struct tsnep_adapter *adapter = netdev_priv(netdev);
>  	int i;
>  
> +	set_bit(__TSNEP_DOWN, &adapter->state);
> +
>  	tsnep_disable_irq(adapter, ECM_INT_LINK);
>  	tsnep_phy_close(adapter);
>  
> @@ -1513,6 +1521,7 @@ static int tsnep_probe(struct platform_device *pdev)
>  	adapter->msg_enable = NETIF_MSG_DRV | NETIF_MSG_PROBE |
>  			      NETIF_MSG_LINK | NETIF_MSG_IFUP |
>  			      NETIF_MSG_IFDOWN | NETIF_MSG_TX_QUEUED;
> +	set_bit(__TSNEP_DOWN, &adapter->state);
>  
>  	netdev->min_mtu = ETH_MIN_MTU;
>  	netdev->max_mtu = TSNEP_MAX_FRAME_SIZE;
> @@ -1609,6 +1618,8 @@ static int tsnep_remove(struct platform_device *pdev)
>  {
>  	struct tsnep_adapter *adapter = platform_get_drvdata(pdev);
>  
> +	set_bit(__TSNEP_DOWN, &adapter->state);
> +
>  	unregister_netdev(adapter->netdev);
>  
>  	tsnep_rxnfc_cleanup(adapter);

Thanks,
Olek

Re: [PATCH net-next v3 4/9] tsnep: Add XDP TX support

[Alexander Lobakin]

From: Gerhard Engleder <gerhard@engleder-embedded.com>
Date: Wed Jan 04 2023 20:41:27 GMT+0100

> Implement ndo_xdp_xmit() for XDP TX support. Support for fragmented XDP
> frames is included.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/tsnep.h      |  12 +-
>  drivers/net/ethernet/engleder/tsnep_main.c | 208 ++++++++++++++++++++-
>  2 files changed, 209 insertions(+), 11 deletions(-)

[...]

> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> index 56c8cae6251e..2c7252ded23a 100644
> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -310,10 +310,11 @@ static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,
>  	struct tsnep_tx_entry *entry = &tx->entry[index];
>  
>  	entry->properties = 0;
> -	if (entry->skb) {
> +	if (entry->skb || entry->xdpf) {
>  		entry->properties = length & TSNEP_DESC_LENGTH_MASK;
>  		entry->properties |= TSNEP_DESC_INTERRUPT_FLAG;
> -		if (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
> +		if (entry->type == TSNEP_TX_TYPE_SKB &&
> +		    skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)

Please enclose bitops (& here) hanging around any logical ops (&& here
in their own set of braces ().

>  			entry->properties |= TSNEP_DESC_EXTENDED_WRITEBACK_FLAG;
>  
>  		/* toggle user flag to prevent false acknowledge

[...]

> @@ -417,12 +420,13 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)
>  		entry = &tx->entry[(index + i) % TSNEP_RING_SIZE];
>  
>  		if (entry->len) {
> -			if (i == 0)
> +			if (i == 0 && entry->type == TSNEP_TX_TYPE_SKB)

`if (!i && ...)`, I think even checkpatch warns or was warning about
preferring !a over `a == 0` at some point.

>  				dma_unmap_single(dmadev,
>  						 dma_unmap_addr(entry, dma),
>  						 dma_unmap_len(entry, len),
>  						 DMA_TO_DEVICE);
> -			else
> +			else if (entry->type == TSNEP_TX_TYPE_SKB ||
> +				 entry->type == TSNEP_TX_TYPE_XDP_NDO)
>  				dma_unmap_page(dmadev,
>  					       dma_unmap_addr(entry, dma),
>  					       dma_unmap_len(entry, len),
> @@ -502,12 +506,134 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> +static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,
> +			    struct skb_shared_info *shinfo, int count,

I believe most of those pointers, if not all of them, can be const, you
only read data from them (probably except @tx).

> +			    enum tsnep_tx_type type)
> +{
> +	struct device *dmadev = tx->adapter->dmadev;
> +	struct tsnep_tx_entry *entry;
> +	struct page *page;
> +	skb_frag_t *frag;

This one as well (also please check for @page, can't say for sure).

> +	unsigned int len;
> +	int map_len = 0;
> +	dma_addr_t dma;
> +	void *data;
> +	int i;

[...]

> +		entry->len = len;
> +		dma_unmap_addr_set(entry, dma, dma);
> +
> +		entry->desc->tx = __cpu_to_le64(dma);
> +
> +		map_len += len;
> +
> +		if ((i + 1) < count) {

Those braces are redundant here.

> +			frag = &shinfo->frags[i];
> +			len = skb_frag_size(frag);
> +		}
> +	}
> +
> +	return map_len;
> +}
> +
> +/* This function requires __netif_tx_lock is held by the caller. */
> +static bool tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
> +				      struct tsnep_tx *tx,
> +				      enum tsnep_tx_type type)
> +{
> +	struct skb_shared_info *shinfo = xdp_get_shared_info_from_frame(xdpf);

Same for this one (const).

> +	struct tsnep_tx_entry *entry;
> +	int count = 1;
> +	int length;
> +	int retval;
> +	int i;

Maybe squash some of them into one line as they are of the same type?

> +
> +	if (unlikely(xdp_frame_has_frags(xdpf)))
> +		count += shinfo->nr_frags;
> +
> +	spin_lock_bh(&tx->lock);

[...]

> +	retval = tsnep_xdp_tx_map(xdpf, tx, shinfo, count, type);
> +	if (retval < 0) {
> +		tsnep_tx_unmap(tx, tx->write, count);
> +		entry->xdpf = NULL;
> +
> +		tx->dropped++;
> +
> +		spin_unlock_bh(&tx->lock);
> +
> +		return false;
> +	}
> +	length = retval;
> +
> +	for (i = 0; i < count; i++)
> +		tsnep_tx_activate(tx, (tx->write + i) % TSNEP_RING_SIZE, length,
> +				  i == (count - 1));

Redundant braces around `count - 1`.

> +	tx->write = (tx->write + count) % TSNEP_RING_SIZE;
> +
> +	/* descriptor properties shall be valid before hardware is notified */
> +	dma_wmb();
> +
> +	spin_unlock_bh(&tx->lock);
> +
> +	return true;
> +}
> +
> +static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)
> +{
> +	iowrite32(TSNEP_CONTROL_TX_ENABLE, tx->addr + TSNEP_CONTROL);
> +}
> +
>  static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>  {
> -	int budget = 128;
>  	struct tsnep_tx_entry *entry;
> -	int count;
> +	struct xdp_frame_bulk bq;
> +	int budget = 128;

BTW, why do you ignore NAPI budget and use your own?

>  	int length;
> +	int count;
> +
> +	xdp_frame_bulk_init(&bq);
> +
> +	rcu_read_lock(); /* need for xdp_return_frame_bulk */
>  
>  	spin_lock_bh(&tx->lock);
>  

[...]

> @@ -552,8 +683,20 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>  			skb_tstamp_tx(entry->skb, &hwtstamps);
>  		}
>  
> -		napi_consume_skb(entry->skb, budget);
> -		entry->skb = NULL;
> +		switch (entry->type) {
> +		case TSNEP_TX_TYPE_SKB:
> +			napi_consume_skb(entry->skb, budget);
> +			entry->skb = NULL;
> +			break;
> +		case TSNEP_TX_TYPE_XDP_TX:
> +			xdp_return_frame_rx_napi(entry->xdpf);
> +			entry->xdpf = NULL;
> +			break;
> +		case TSNEP_TX_TYPE_XDP_NDO:
> +			xdp_return_frame_bulk(entry->xdpf, &bq);
> +			entry->xdpf = NULL;
> +			break;
> +		}

entry ::skb and ::xdpf share the same slot, you could nullify it here
once instead of duplicating the same op across each case.

>  
>  		tx->read = (tx->read + count) % TSNEP_RING_SIZE;
>  
> @@ -570,6 +713,10 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>  
>  	spin_unlock_bh(&tx->lock);
>  
> +	xdp_flush_frame_bulk(&bq);
> +
> +	rcu_read_unlock();
> +
>  	return (budget != 0);

Also redundant braces.

>  }
>  
> @@ -1330,6 +1477,46 @@ static ktime_t tsnep_netdev_get_tstamp(struct net_device *netdev,
>  	return ns_to_ktime(timestamp);
>  }
>  
> +static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
> +				 struct xdp_frame **xdp, u32 flags)
> +{
> +	struct tsnep_adapter *adapter = netdev_priv(dev);
> +	int cpu = smp_processor_id();

Can be u32.

> +	struct netdev_queue *nq;
> +	int nxmit;
> +	int queue;

Squash?

> +	bool xmit;
> +
> +	if (unlikely(test_bit(__TSNEP_DOWN, &adapter->state)))
> +		return -ENETDOWN;
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +		return -EINVAL;

[...]

Thanks,
Olek

Re: [PATCH net-next v3 6/9] tsnep: Support XDP BPF program setup

[Alexander Lobakin]

From: Gerhard Engleder <gerhard@engleder-embedded.com>
Date: Wed Jan 04 2023 20:41:29 GMT+0100

> Implement setup of BPF programs for XDP RX path with command
> XDP_SETUP_PROG of ndo_bpf(). This is prework for XDP RX path support.
> 
> tsnep_netdev_close() is called directly during BPF program setup. Add
> netif_carrier_off() and netif_tx_stop_all_queues() calls to signal to
> network stack that device is down. Otherwise network stack would
> continue transmitting pakets.
> 
> Return value of tsnep_netdev_open() is not checked during BPF program
> setup like in other drivers. Forwarding the return value would result in
> a bpf_prog_put() call in dev_xdp_install(), which would make removal of
> BPF program necessary.
> 
> If tsnep_netdev_open() fails during BPF program setup, then the network
> stack would call tsnep_netdev_close() anyway. Thus, tsnep_netdev_close()
> checks now if device is already down.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/Makefile     |  2 +-
>  drivers/net/ethernet/engleder/tsnep.h      | 13 +++++++++++
>  drivers/net/ethernet/engleder/tsnep_main.c | 25 +++++++++++++++++---
>  drivers/net/ethernet/engleder/tsnep_xdp.c  | 27 ++++++++++++++++++++++
>  4 files changed, 63 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/net/ethernet/engleder/tsnep_xdp.c
> 
> diff --git a/drivers/net/ethernet/engleder/Makefile b/drivers/net/ethernet/engleder/Makefile
> index b6e3b16623de..0901801cfcc9 100644
> --- a/drivers/net/ethernet/engleder/Makefile
> +++ b/drivers/net/ethernet/engleder/Makefile
> @@ -6,5 +6,5 @@
>  obj-$(CONFIG_TSNEP) += tsnep.o
>  
>  tsnep-objs := tsnep_main.o tsnep_ethtool.o tsnep_ptp.o tsnep_tc.o \
> -	      tsnep_rxnfc.o $(tsnep-y)
> +	      tsnep_rxnfc.o tsnep_xdp.o $(tsnep-y)

Not related directly to the subject, but could be fixed in that commit I
hope: you don't need to add $(tsnep-y) to $(tsnep-objs), it gets added
automatically.

>  tsnep-$(CONFIG_TSNEP_SELFTESTS) += tsnep_selftests.o
> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
> index 29b04127f529..0e7fc36a64e1 100644
> --- a/drivers/net/ethernet/engleder/tsnep.h
> +++ b/drivers/net/ethernet/engleder/tsnep.h

[...]

> @@ -215,6 +220,14 @@ int tsnep_rxnfc_add_rule(struct tsnep_adapter *adapter,
>  int tsnep_rxnfc_del_rule(struct tsnep_adapter *adapter,
>  			 struct ethtool_rxnfc *cmd);
>  
> +int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
> +			 struct netlink_ext_ack *extack);
> +
> +static inline bool tsnep_xdp_is_enabled(struct tsnep_adapter *adapter)
> +{
> +	return !!adapter->xdp_prog;

Any concurrent access protection? READ_ONCE(), RCU?

> +}
> +
>  #if IS_ENABLED(CONFIG_TSNEP_SELFTESTS)
>  int tsnep_ethtool_get_test_count(void);
>  void tsnep_ethtool_get_test_strings(u8 *data);

[...]

> +static int tsnep_netdev_bpf(struct net_device *dev, struct netdev_bpf *bpf)
> +{
> +	struct tsnep_adapter *adapter = netdev_priv(dev);
> +
> +	switch (bpf->command) {
> +	case XDP_SETUP_PROG:
> +		return tsnep_xdp_setup_prog(adapter, bpf->prog, bpf->extack);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

So, after this commit, I'm able to install an XDP prog to an interface,
but it won't do anything. I think the patch could be moved to the end of
the series, so that it won't end up with such?

> +
>  static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
>  				 struct xdp_frame **xdp, u32 flags)
>  {

[...]

> +int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
> +			 struct netlink_ext_ack *extack)
> +{
> +	struct net_device *dev = adapter->netdev;
> +	bool if_running = netif_running(dev);
> +	struct bpf_prog *old_prog;
> +
> +	if (if_running)
> +		tsnep_netdev_close(dev);

You don't need to close the interface if `!prog == !old_prog`. I
wouldn't introduce redundant down-ups here and leave no possibility for
prog hotswapping.

> +
> +	old_prog = xchg(&adapter->xdp_prog, prog);
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +
> +	if (if_running)
> +		tsnep_netdev_open(dev);
> +
> +	return 0;
> +}

Thanks,
Olek

Re: [PATCH net-next v3 8/9] tsnep: Add RX queue info for XDP support

[Alexander Lobakin]

From: Gerhard Engleder <gerhard@engleder-embedded.com>
Date: Wed Jan 04 2023 20:41:31 GMT+0100

> Register xdp_rxq_info with page_pool memory model. This is needed for
> XDP buffer handling.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> Reviewed-by: Saeed Mahameed <saeed@kernel.org>
> ---
>  drivers/net/ethernet/engleder/tsnep.h      |  6 ++--
>  drivers/net/ethernet/engleder/tsnep_main.c | 34 +++++++++++++++++-----
>  2 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
> index 0e7fc36a64e1..0210dab90f71 100644
> --- a/drivers/net/ethernet/engleder/tsnep.h
> +++ b/drivers/net/ethernet/engleder/tsnep.h
> @@ -133,17 +133,19 @@ struct tsnep_rx {
>  	u32 dropped;
>  	u32 multicast;
>  	u32 alloc_failed;
> +
> +	struct xdp_rxq_info xdp_rxq;
>  };
>  
>  struct tsnep_queue {
>  	struct tsnep_adapter *adapter;
>  	char name[IFNAMSIZ + 9];
>  
> +	struct napi_struct napi;
> +
>  	struct tsnep_tx *tx;
>  	struct tsnep_rx *rx;
>  
> -	struct napi_struct napi;
> -

I'd leave a word in the commit message that you're moving ::napi to
improve structure cacheline span. Or even do that in a separate commit
with some pahole output to make it clear why you do that.

>  	int irq;
>  	u32 irq_mask;
>  	void __iomem *irq_delay_addr;

[...]

> @@ -1253,6 +1266,7 @@ int tsnep_netdev_open(struct net_device *netdev)
>  {
>  	struct tsnep_adapter *adapter = netdev_priv(netdev);
>  	int i;
> +	unsigned int napi_id;

Reverse Christmas Tree variable style is already messed up here, maybe
you could fix it inplace while at it or at least not make it worse? :D

>  	void __iomem *addr;
>  	int tx_queue_index = 0;
>  	int rx_queue_index = 0;

[...]

Thanks,
Olek

Re: [PATCH net-next v3 9/9] tsnep: Add XDP RX support

[Alexander Lobakin]

From: Gerhard Engleder <gerhard@engleder-embedded.com>
Date: Wed Jan 04 2023 20:41:32 GMT+0100

> If BPF program is set up, then run BPF program for every received frame
> and execute the selected action.
> 
> Test results with A53 1.2GHz:
> 
> XDP_DROP (samples/bpf/xdp1)
> proto 17:     883878 pkt/s
> 
> XDP_TX (samples/bpf/xdp2)
> proto 17:     255693 pkt/s
> 
> XDP_REDIRECT (samples/bpf/xdpsock)
>  sock0@eth2:0 rxdrop xdp-drv
>                    pps            pkts           1.00
> rx                 855,582        5,404,523
> tx                 0              0
> 
> XDP_REDIRECT (samples/bpf/xdp_redirect)
> eth2->eth1         613,267 rx/s   0 err,drop/s   613,272 xmit/s
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/tsnep_main.c | 129 ++++++++++++++++++++-
>  1 file changed, 127 insertions(+), 2 deletions(-)

[...]

> @@ -624,6 +628,34 @@ static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)
>  	iowrite32(TSNEP_CONTROL_TX_ENABLE, tx->addr + TSNEP_CONTROL);
>  }
>  
> +static bool tsnep_xdp_xmit_back(struct tsnep_adapter *adapter,
> +				struct xdp_buff *xdp)
> +{
> +	struct xdp_frame *xdpf = xdp_convert_buff_to_frame(xdp);
> +	int cpu = smp_processor_id();
> +	struct netdev_queue *nq;
> +	int queue;

Squash with @cpu above (or make @cpu u32)?

> +	bool xmit;
> +
> +	if (unlikely(!xdpf))
> +		return -EFAULT;
> +
> +	queue = cpu % adapter->num_tx_queues;
> +	nq = netdev_get_tx_queue(adapter->netdev, queue);
> +
> +	__netif_tx_lock(nq, cpu);

[...]

> @@ -788,6 +820,11 @@ static unsigned int tsnep_rx_offset(struct tsnep_rx *rx)
>  	return TSNEP_SKB_PAD;
>  }
>  
> +static unsigned int tsnep_rx_offset_xdp(void)
> +{
> +	return XDP_PACKET_HEADROOM;
> +}

The reason for creating a function to always return a constant?

> +
>  static void tsnep_rx_ring_cleanup(struct tsnep_rx *rx)
>  {
>  	struct device *dmadev = rx->adapter->dmadev;

[...]

> +static void tsnep_finalize_xdp(struct tsnep_adapter *adapter, int status)
> +{
> +	int cpu = smp_processor_id();
> +	struct netdev_queue *nq;
> +	int queue;

(same re squashing)

> +
> +	if (status & TSNEP_XDP_TX) {
> +		queue = cpu % adapter->num_tx_queues;
> +		nq = netdev_get_tx_queue(adapter->netdev, queue);
> +
> +		__netif_tx_lock(nq, cpu);
> +		tsnep_xdp_xmit_flush(&adapter->tx[queue]);
> +		__netif_tx_unlock(nq);
> +	}

This can be optimized. Given that one NAPI cycle is always being run on
one CPU, you can get both @queue and @nq once at the beginning of a
polling cycle and then pass it to perform %XDP_TX and this flush.
Alternatively, if you don't want to do that not knowing in advance if
you'll need it at all during the cycle, you can obtain them at the first
tsnep_xdp_xmit_back() invocation.

> +
> +	if (status & TSNEP_XDP_REDIRECT)
> +		xdp_do_flush();
> +}
> +
>  static struct sk_buff *tsnep_build_skb(struct tsnep_rx *rx, struct page *page,
>  				       int length)
>  {

[...]

> @@ -1087,6 +1189,26 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
>  		rx->read = (rx->read + 1) % TSNEP_RING_SIZE;
>  		desc_available++;
>  
> +		if (prog) {
> +			bool consume;
> +
> +			xdp_init_buff(&xdp, PAGE_SIZE, &rx->xdp_rxq);

xdp_init_buff() is designed to be called once per NAPI cycle, at the
beginning. You don't need to reinit it given that the values you pass
are always the same.

> +			xdp_prepare_buff(&xdp, page_address(entry->page),
> +					 tsnep_rx_offset_xdp() + TSNEP_RX_INLINE_METADATA_SIZE,
> +					 length, false);
> +
> +			consume = tsnep_xdp_run_prog(rx, prog, &xdp,
> +						     &xdp_status);

[...]

Thanks,
Olek

Re: [PATCH net-next v3 1/9] tsnep: Use spin_lock_bh for TX

[Gerhard Engleder]

On 05.01.23 13:40, Alexander Lobakin wrote:
> From: Gerhard Engleder <gerhard@engleder-embedded.com>
> Date: Wed Jan 04 2023 20:41:24 GMT+0100
> 
>> TX processing is done only within process or BH context. Therefore,
>> _irqsafe variant is not necessary.
> 
> NAPI and .ndo_{start,xdp}_xmit are BH-only, where can the process
> context happen here?

I will remove the process context from the description.

Gerhard

Re: [PATCH net-next v3 3/9] tsnep: Add adapter down state

[Gerhard Engleder]

On 05.01.23 13:44, Alexander Lobakin wrote:
> From: Gerhard Engleder <gerhard@engleder-embedded.com>
> Date: Wed Jan 04 2023 20:41:26 GMT+0100
> 
>> Add adapter state with flag for down state. This flag will be used by
>> the XDP TX path to deny TX if adapter is down.
>>
>> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
>> ---
>>   drivers/net/ethernet/engleder/tsnep.h      |  1 +
>>   drivers/net/ethernet/engleder/tsnep_main.c | 11 +++++++++++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
>> index f93ba48bac3f..f72c0c4da1a9 100644
>> --- a/drivers/net/ethernet/engleder/tsnep.h
>> +++ b/drivers/net/ethernet/engleder/tsnep.h
>> @@ -148,6 +148,7 @@ struct tsnep_adapter {
>>   	phy_interface_t phy_mode;
>>   	struct phy_device *phydev;
>>   	int msg_enable;
>> +	unsigned long state;
> 
> Now there will be a 4-byte hole in between ::msg_enable and ::state,
> maybe add ::state right after ::phydev?

I will do that.

Gerhard

Re: [PATCH net-next v3 4/9] tsnep: Add XDP TX support

[Gerhard Engleder]

On 05.01.23 14:01, Alexander Lobakin wrote:
> From: Gerhard Engleder <gerhard@engleder-embedded.com>
> Date: Wed Jan 04 2023 20:41:27 GMT+0100
> 
>> Implement ndo_xdp_xmit() for XDP TX support. Support for fragmented XDP
>> frames is included.
>>
>> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
>> ---
>>   drivers/net/ethernet/engleder/tsnep.h      |  12 +-
>>   drivers/net/ethernet/engleder/tsnep_main.c | 208 ++++++++++++++++++++-
>>   2 files changed, 209 insertions(+), 11 deletions(-)
> 
> [...]
> 
>> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
>> index 56c8cae6251e..2c7252ded23a 100644
>> --- a/drivers/net/ethernet/engleder/tsnep_main.c
>> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
>> @@ -310,10 +310,11 @@ static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,
>>   	struct tsnep_tx_entry *entry = &tx->entry[index];
>>   
>>   	entry->properties = 0;
>> -	if (entry->skb) {
>> +	if (entry->skb || entry->xdpf) {
>>   		entry->properties = length & TSNEP_DESC_LENGTH_MASK;
>>   		entry->properties |= TSNEP_DESC_INTERRUPT_FLAG;
>> -		if (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
>> +		if (entry->type == TSNEP_TX_TYPE_SKB &&
>> +		    skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
> 
> Please enclose bitops (& here) hanging around any logical ops (&& here
> in their own set of braces ().

Will be done.

>>   			entry->properties |= TSNEP_DESC_EXTENDED_WRITEBACK_FLAG;
>>   
>>   		/* toggle user flag to prevent false acknowledge
> 
> [...]
> 
>> @@ -417,12 +420,13 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)
>>   		entry = &tx->entry[(index + i) % TSNEP_RING_SIZE];
>>   
>>   		if (entry->len) {
>> -			if (i == 0)
>> +			if (i == 0 && entry->type == TSNEP_TX_TYPE_SKB)
> 
> `if (!i && ...)`, I think even checkpatch warns or was warning about
> preferring !a over `a == 0` at some point.

There is no checkpatch warning. I will change it here and at a similar
location in normal TX path.

>>   				dma_unmap_single(dmadev,
>>   						 dma_unmap_addr(entry, dma),
>>   						 dma_unmap_len(entry, len),
>>   						 DMA_TO_DEVICE);
>> -			else
>> +			else if (entry->type == TSNEP_TX_TYPE_SKB ||
>> +				 entry->type == TSNEP_TX_TYPE_XDP_NDO)
>>   				dma_unmap_page(dmadev,
>>   					       dma_unmap_addr(entry, dma),
>>   					       dma_unmap_len(entry, len),
>> @@ -502,12 +506,134 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
>>   	return NETDEV_TX_OK;
>>   }
>>   
>> +static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,
>> +			    struct skb_shared_info *shinfo, int count,
> 
> I believe most of those pointers, if not all of them, can be const, you
> only read data from them (probably except @tx).

Will be done.

>> +			    enum tsnep_tx_type type)
>> +{
>> +	struct device *dmadev = tx->adapter->dmadev;
>> +	struct tsnep_tx_entry *entry;
>> +	struct page *page;
>> +	skb_frag_t *frag;
> 
> This one as well (also please check for @page, can't say for sure).

Will be done for frag. page is not possible.

>> +	unsigned int len;
>> +	int map_len = 0;
>> +	dma_addr_t dma;
>> +	void *data;
>> +	int i;
> 
> [...]
> 
>> +		entry->len = len;
>> +		dma_unmap_addr_set(entry, dma, dma);
>> +
>> +		entry->desc->tx = __cpu_to_le64(dma);
>> +
>> +		map_len += len;
>> +
>> +		if ((i + 1) < count) {
> 
> Those braces are redundant here.

Braces will be removed.

>> +			frag = &shinfo->frags[i];
>> +			len = skb_frag_size(frag);
>> +		}
>> +	}
>> +
>> +	return map_len;
>> +}
>> +
>> +/* This function requires __netif_tx_lock is held by the caller. */
>> +static bool tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
>> +				      struct tsnep_tx *tx,
>> +				      enum tsnep_tx_type type)
>> +{
>> +	struct skb_shared_info *shinfo = xdp_get_shared_info_from_frame(xdpf);
> 
> Same for this one (const).

Will be done.

>> +	struct tsnep_tx_entry *entry;
>> +	int count = 1;
>> +	int length;
>> +	int retval;
>> +	int i;
> 
> Maybe squash some of them into one line as they are of the same type?

Will be done.

>> +
>> +	if (unlikely(xdp_frame_has_frags(xdpf)))
>> +		count += shinfo->nr_frags;
>> +
>> +	spin_lock_bh(&tx->lock);
> 
> [...]
> 
>> +	retval = tsnep_xdp_tx_map(xdpf, tx, shinfo, count, type);
>> +	if (retval < 0) {
>> +		tsnep_tx_unmap(tx, tx->write, count);
>> +		entry->xdpf = NULL;
>> +
>> +		tx->dropped++;
>> +
>> +		spin_unlock_bh(&tx->lock);
>> +
>> +		return false;
>> +	}
>> +	length = retval;
>> +
>> +	for (i = 0; i < count; i++)
>> +		tsnep_tx_activate(tx, (tx->write + i) % TSNEP_RING_SIZE, length,
>> +				  i == (count - 1));
> 
> Redundant braces around `count - 1`.

Braces will be removed here and at a similar location in normal TX path.

>> +	tx->write = (tx->write + count) % TSNEP_RING_SIZE;
>> +
>> +	/* descriptor properties shall be valid before hardware is notified */
>> +	dma_wmb();
>> +
>> +	spin_unlock_bh(&tx->lock);
>> +
>> +	return true;
>> +}
>> +
>> +static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)
>> +{
>> +	iowrite32(TSNEP_CONTROL_TX_ENABLE, tx->addr + TSNEP_CONTROL);
>> +}
>> +
>>   static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>>   {
>> -	int budget = 128;
>>   	struct tsnep_tx_entry *entry;
>> -	int count;
>> +	struct xdp_frame_bulk bq;
>> +	int budget = 128;
> 
> BTW, why do you ignore NAPI budget and use your own?

I followed the style of igb/igc which also use their own budget fo 128
for TX polling. I checked again and in contrast to these drivers tsnep
does not forward the budget to napi_consume_skb(). I assume this should
be changed with a separate patch?

I have to confess, that I don't understand exactly how this NAPI budget
stuff works. I did not find any documentation so I followed igb/igc.

>>   	int length;
>> +	int count;
>> +
>> +	xdp_frame_bulk_init(&bq);
>> +
>> +	rcu_read_lock(); /* need for xdp_return_frame_bulk */
>>   
>>   	spin_lock_bh(&tx->lock);
>>   
> 
> [...]
> 
>> @@ -552,8 +683,20 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>>   			skb_tstamp_tx(entry->skb, &hwtstamps);
>>   		}
>>   
>> -		napi_consume_skb(entry->skb, budget);
>> -		entry->skb = NULL;
>> +		switch (entry->type) {
>> +		case TSNEP_TX_TYPE_SKB:
>> +			napi_consume_skb(entry->skb, budget);
>> +			entry->skb = NULL;
>> +			break;
>> +		case TSNEP_TX_TYPE_XDP_TX:
>> +			xdp_return_frame_rx_napi(entry->xdpf);
>> +			entry->xdpf = NULL;
>> +			break;
>> +		case TSNEP_TX_TYPE_XDP_NDO:
>> +			xdp_return_frame_bulk(entry->xdpf, &bq);
>> +			entry->xdpf = NULL;
>> +			break;
>> +		}
> 
> entry ::skb and ::xdpf share the same slot, you could nullify it here
> once instead of duplicating the same op across each case.

I will nullify only ::sbk and add comment about ::xdpf.

>>   
>>   		tx->read = (tx->read + count) % TSNEP_RING_SIZE;
>>   
>> @@ -570,6 +713,10 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>>   
>>   	spin_unlock_bh(&tx->lock);
>>   
>> +	xdp_flush_frame_bulk(&bq);
>> +
>> +	rcu_read_unlock();
>> +
>>   	return (budget != 0);
> 
> Also redundant braces.

Braces will be removed.

>>   }
>>   
>> @@ -1330,6 +1477,46 @@ static ktime_t tsnep_netdev_get_tstamp(struct net_device *netdev,
>>   	return ns_to_ktime(timestamp);
>>   }
>>   
>> +static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
>> +				 struct xdp_frame **xdp, u32 flags)
>> +{
>> +	struct tsnep_adapter *adapter = netdev_priv(dev);
>> +	int cpu = smp_processor_id();
> 
> Can be u32.

u32 will be used.

>> +	struct netdev_queue *nq;
>> +	int nxmit;
>> +	int queue;
> 
> Squash?

Squashed.

Gerhard

Re: [PATCH net-next v3 6/9] tsnep: Support XDP BPF program setup

[Gerhard Engleder]

On 05.01.23 18:24, Alexander Lobakin wrote:
> From: Gerhard Engleder <gerhard@engleder-embedded.com>
> Date: Wed Jan 04 2023 20:41:29 GMT+0100
> 
>> Implement setup of BPF programs for XDP RX path with command
>> XDP_SETUP_PROG of ndo_bpf(). This is prework for XDP RX path support.
>>
>> tsnep_netdev_close() is called directly during BPF program setup. Add
>> netif_carrier_off() and netif_tx_stop_all_queues() calls to signal to
>> network stack that device is down. Otherwise network stack would
>> continue transmitting pakets.
>>
>> Return value of tsnep_netdev_open() is not checked during BPF program
>> setup like in other drivers. Forwarding the return value would result in
>> a bpf_prog_put() call in dev_xdp_install(), which would make removal of
>> BPF program necessary.
>>
>> If tsnep_netdev_open() fails during BPF program setup, then the network
>> stack would call tsnep_netdev_close() anyway. Thus, tsnep_netdev_close()
>> checks now if device is already down.
>>
>> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
>> ---
>>   drivers/net/ethernet/engleder/Makefile     |  2 +-
>>   drivers/net/ethernet/engleder/tsnep.h      | 13 +++++++++++
>>   drivers/net/ethernet/engleder/tsnep_main.c | 25 +++++++++++++++++---
>>   drivers/net/ethernet/engleder/tsnep_xdp.c  | 27 ++++++++++++++++++++++
>>   4 files changed, 63 insertions(+), 4 deletions(-)
>>   create mode 100644 drivers/net/ethernet/engleder/tsnep_xdp.c
>>
>> diff --git a/drivers/net/ethernet/engleder/Makefile b/drivers/net/ethernet/engleder/Makefile
>> index b6e3b16623de..0901801cfcc9 100644
>> --- a/drivers/net/ethernet/engleder/Makefile
>> +++ b/drivers/net/ethernet/engleder/Makefile
>> @@ -6,5 +6,5 @@
>>   obj-$(CONFIG_TSNEP) += tsnep.o
>>   
>>   tsnep-objs := tsnep_main.o tsnep_ethtool.o tsnep_ptp.o tsnep_tc.o \
>> -	      tsnep_rxnfc.o $(tsnep-y)
>> +	      tsnep_rxnfc.o tsnep_xdp.o $(tsnep-y)
> 
> Not related directly to the subject, but could be fixed in that commit I
> hope: you don't need to add $(tsnep-y) to $(tsnep-objs), it gets added
> automatically.

I will fix that with this commit.

>>   tsnep-$(CONFIG_TSNEP_SELFTESTS) += tsnep_selftests.o
>> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
>> index 29b04127f529..0e7fc36a64e1 100644
>> --- a/drivers/net/ethernet/engleder/tsnep.h
>> +++ b/drivers/net/ethernet/engleder/tsnep.h
> 
> [...]
> 
>> @@ -215,6 +220,14 @@ int tsnep_rxnfc_add_rule(struct tsnep_adapter *adapter,
>>   int tsnep_rxnfc_del_rule(struct tsnep_adapter *adapter,
>>   			 struct ethtool_rxnfc *cmd);
>>   
>> +int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
>> +			 struct netlink_ext_ack *extack);
>> +
>> +static inline bool tsnep_xdp_is_enabled(struct tsnep_adapter *adapter)
>> +{
>> +	return !!adapter->xdp_prog;
> 
> Any concurrent access protection? READ_ONCE(), RCU?

I assume this is about prog hotswapping which you mentioned below. Is
concurrent access protection needed without prog hotswapping?

>> +}
>> +
>>   #if IS_ENABLED(CONFIG_TSNEP_SELFTESTS)
>>   int tsnep_ethtool_get_test_count(void);
>>   void tsnep_ethtool_get_test_strings(u8 *data);
> 
> [...]
> 
>> +static int tsnep_netdev_bpf(struct net_device *dev, struct netdev_bpf *bpf)
>> +{
>> +	struct tsnep_adapter *adapter = netdev_priv(dev);
>> +
>> +	switch (bpf->command) {
>> +	case XDP_SETUP_PROG:
>> +		return tsnep_xdp_setup_prog(adapter, bpf->prog, bpf->extack);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
> 
> So, after this commit, I'm able to install an XDP prog to an interface,
> but it won't do anything. I think the patch could be moved to the end of
> the series, so that it won't end up with such?

My thinking was to first implement the base and the actual functionality
last. I can move it to the end.

>> +
>>   static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
>>   				 struct xdp_frame **xdp, u32 flags)
>>   {
> 
> [...]
> 
>> +int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
>> +			 struct netlink_ext_ack *extack)
>> +{
>> +	struct net_device *dev = adapter->netdev;
>> +	bool if_running = netif_running(dev);
>> +	struct bpf_prog *old_prog;
>> +
>> +	if (if_running)
>> +		tsnep_netdev_close(dev);
> 
> You don't need to close the interface if `!prog == !old_prog`. I
> wouldn't introduce redundant down-ups here and leave no possibility for
> prog hotswapping.

If prog hotswapping is possible, then how shall it be ensured that the
old prog is not running anymore before 'bpf_prog_put(old_prog)' is
called? I don't understand how 'READ_ONCE(adapter->xdp_prog)' during RX
path and 'old_prog = xchg(&adapter->xdp_prog, prog)' during prog setup
can ensure that (e.g. in ixgbe driver).

Gerhard

Re: [PATCH net-next v3 8/9] tsnep: Add RX queue info for XDP support

[Gerhard Engleder]

On 05.01.23 18:35, Alexander Lobakin wrote:
> From: Gerhard Engleder <gerhard@engleder-embedded.com>
> Date: Wed Jan 04 2023 20:41:31 GMT+0100
> 
>> Register xdp_rxq_info with page_pool memory model. This is needed for
>> XDP buffer handling.
>>
>> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
>> Reviewed-by: Saeed Mahameed <saeed@kernel.org>
>> ---
>>   drivers/net/ethernet/engleder/tsnep.h      |  6 ++--
>>   drivers/net/ethernet/engleder/tsnep_main.c | 34 +++++++++++++++++-----
>>   2 files changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
>> index 0e7fc36a64e1..0210dab90f71 100644
>> --- a/drivers/net/ethernet/engleder/tsnep.h
>> +++ b/drivers/net/ethernet/engleder/tsnep.h
>> @@ -133,17 +133,19 @@ struct tsnep_rx {
>>   	u32 dropped;
>>   	u32 multicast;
>>   	u32 alloc_failed;
>> +
>> +	struct xdp_rxq_info xdp_rxq;
>>   };
>>   
>>   struct tsnep_queue {
>>   	struct tsnep_adapter *adapter;
>>   	char name[IFNAMSIZ + 9];
>>   
>> +	struct napi_struct napi;
>> +
>>   	struct tsnep_tx *tx;
>>   	struct tsnep_rx *rx;
>>   
>> -	struct napi_struct napi;
>> -
> 
> I'd leave a word in the commit message that you're moving ::napi to
> improve structure cacheline span. Or even do that in a separate commit
> with some pahole output to make it clear why you do that.

I only reordered based on access order during initialization. But I 
understand that this not a valid reason. I will remove that reordering.

>>   	int irq;
>>   	u32 irq_mask;
>>   	void __iomem *irq_delay_addr;
> 
> [...]
> 
>> @@ -1253,6 +1266,7 @@ int tsnep_netdev_open(struct net_device *netdev)
>>   {
>>   	struct tsnep_adapter *adapter = netdev_priv(netdev);
>>   	int i;
>> +	unsigned int napi_id;
> 
> Reverse Christmas Tree variable style is already messed up here, maybe
> you could fix it inplace while at it or at least not make it worse? :D

I forgot to do that here. Will be fixed.

Gerhard

Re: [PATCH net-next v3 9/9] tsnep: Add XDP RX support

[Gerhard Engleder]

On 05.01.23 18:52, Alexander Lobakin wrote:
> From: Gerhard Engleder <gerhard@engleder-embedded.com>
> Date: Wed Jan 04 2023 20:41:32 GMT+0100
> 
>> If BPF program is set up, then run BPF program for every received frame
>> and execute the selected action.
>>
>> Test results with A53 1.2GHz:
>>
>> XDP_DROP (samples/bpf/xdp1)
>> proto 17:     883878 pkt/s
>>
>> XDP_TX (samples/bpf/xdp2)
>> proto 17:     255693 pkt/s
>>
>> XDP_REDIRECT (samples/bpf/xdpsock)
>>   sock0@eth2:0 rxdrop xdp-drv
>>                     pps            pkts           1.00
>> rx                 855,582        5,404,523
>> tx                 0              0
>>
>> XDP_REDIRECT (samples/bpf/xdp_redirect)
>> eth2->eth1         613,267 rx/s   0 err,drop/s   613,272 xmit/s
>>
>> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
>> ---
>>   drivers/net/ethernet/engleder/tsnep_main.c | 129 ++++++++++++++++++++-
>>   1 file changed, 127 insertions(+), 2 deletions(-)
> 
> [...]
> 
>> @@ -624,6 +628,34 @@ static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)
>>   	iowrite32(TSNEP_CONTROL_TX_ENABLE, tx->addr + TSNEP_CONTROL);
>>   }
>>   
>> +static bool tsnep_xdp_xmit_back(struct tsnep_adapter *adapter,
>> +				struct xdp_buff *xdp)
>> +{
>> +	struct xdp_frame *xdpf = xdp_convert_buff_to_frame(xdp);
>> +	int cpu = smp_processor_id();
>> +	struct netdev_queue *nq;
>> +	int queue;
> 
> Squash with @cpu above (or make @cpu u32)?

Will change to u32.

>> +	bool xmit;
>> +
>> +	if (unlikely(!xdpf))
>> +		return -EFAULT;
>> +
>> +	queue = cpu % adapter->num_tx_queues;
>> +	nq = netdev_get_tx_queue(adapter->netdev, queue);
>> +
>> +	__netif_tx_lock(nq, cpu);
> 
> [...]
> 
>> @@ -788,6 +820,11 @@ static unsigned int tsnep_rx_offset(struct tsnep_rx *rx)
>>   	return TSNEP_SKB_PAD;
>>   }
>>   
>> +static unsigned int tsnep_rx_offset_xdp(void)
>> +{
>> +	return XDP_PACKET_HEADROOM;
>> +}
> 
> The reason for creating a function to always return a constant?

It is a variant of tsnep_rx_offset() for the XDP path to prevent
unneeded calls of tsnep_xdp_is_enabled(). With this function I
keep the RX offset local. But yes, it provides actually no
functionality. I will add a comment.

What about always using XDP_PACKET_HEADROOM as offset in the RX buffer?
NET_IP_ALIGN would not be considered then, but it is zero anyway on
the main platforms x86 and arm64. This would simplify the code.

>> +
>>   static void tsnep_rx_ring_cleanup(struct tsnep_rx *rx)
>>   {
>>   	struct device *dmadev = rx->adapter->dmadev;
> 
> [...]
> 
>> +static void tsnep_finalize_xdp(struct tsnep_adapter *adapter, int status)
>> +{
>> +	int cpu = smp_processor_id();
>> +	struct netdev_queue *nq;
>> +	int queue;
> 
> (same re squashing)

u32.

>> +
>> +	if (status & TSNEP_XDP_TX) {
>> +		queue = cpu % adapter->num_tx_queues;
>> +		nq = netdev_get_tx_queue(adapter->netdev, queue);
>> +
>> +		__netif_tx_lock(nq, cpu);
>> +		tsnep_xdp_xmit_flush(&adapter->tx[queue]);
>> +		__netif_tx_unlock(nq);
>> +	}
> 
> This can be optimized. Given that one NAPI cycle is always being run on
> one CPU, you can get both @queue and @nq once at the beginning of a
> polling cycle and then pass it to perform %XDP_TX and this flush.
> Alternatively, if you don't want to do that not knowing in advance if
> you'll need it at all during the cycle, you can obtain them at the first
> tsnep_xdp_xmit_back() invocation.

I will give it a try.

>> +
>> +	if (status & TSNEP_XDP_REDIRECT)
>> +		xdp_do_flush();
>> +}
>> +
>>   static struct sk_buff *tsnep_build_skb(struct tsnep_rx *rx, struct page *page,
>>   				       int length)
>>   {
> 
> [...]
> 
>> @@ -1087,6 +1189,26 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
>>   		rx->read = (rx->read + 1) % TSNEP_RING_SIZE;
>>   		desc_available++;
>>   
>> +		if (prog) {
>> +			bool consume;
>> +
>> +			xdp_init_buff(&xdp, PAGE_SIZE, &rx->xdp_rxq);
> 
> xdp_init_buff() is designed to be called once per NAPI cycle, at the
> beginning. You don't need to reinit it given that the values you pass
> are always the same.

Will be done.

Thanks for the review!

Gerhard

Re: [PATCH net-next v3 4/9] tsnep: Add XDP TX support

[Jakub Kicinski]

On Thu, 5 Jan 2023 22:13:00 +0100 Gerhard Engleder wrote:
> >> -	if (entry->skb) {
> >> +	if (entry->skb || entry->xdpf) {
> >>   		entry->properties = length & TSNEP_DESC_LENGTH_MASK;
> >>   		entry->properties |= TSNEP_DESC_INTERRUPT_FLAG;
> >> -		if (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
> >> +		if (entry->type == TSNEP_TX_TYPE_SKB &&
> >> +		    skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)  
> > 
> > Please enclose bitops (& here) hanging around any logical ops (&& here
> > in their own set of braces ().  
> 
> Will be done.

Dunno if that's strictly required in the kernel coding style.
Don't we expect a good understanding of operator precedence
from people reading the code?

Re: [PATCH net-next v3 4/9] tsnep: Add XDP TX support

[Gerhard Engleder]

On 06.01.23 23:13, Jakub Kicinski wrote:
> On Thu, 5 Jan 2023 22:13:00 +0100 Gerhard Engleder wrote:
>>>> -	if (entry->skb) {
>>>> +	if (entry->skb || entry->xdpf) {
>>>>    		entry->properties = length & TSNEP_DESC_LENGTH_MASK;
>>>>    		entry->properties |= TSNEP_DESC_INTERRUPT_FLAG;
>>>> -		if (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
>>>> +		if (entry->type == TSNEP_TX_TYPE_SKB &&
>>>> +		    skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
>>>
>>> Please enclose bitops (& here) hanging around any logical ops (&& here
>>> in their own set of braces ().
>>
>> Will be done.
> 
> Dunno if that's strictly required in the kernel coding style.
> Don't we expect a good understanding of operator precedence
> from people reading the code?

checkpatch accepts both and I found no ruling in coding-style.rst.
I also found both styles in Ethernet drivers. checkpatch often
complained about unnecessary braces in my code, so I assumed less
braces are welcome. This fits to that a good understanding of operator
precedence is expected.

Gerhard

Re: [PATCH net-next v3 9/9] tsnep: Add XDP RX support

[Dan Carpenter]

Hi Gerhard,

url:    https://github.com/intel-lab-lkp/linux/commits/Gerhard-Engleder/tsnep-Use-spin_lock_bh-for-TX/20230105-034351
patch link:    https://lore.kernel.org/r/20230104194132.24637-10-gerhard%40engleder-embedded.com
patch subject: [PATCH net-next v3 9/9] tsnep: Add XDP RX support
config: arc-randconfig-m041-20230106
compiler: arc-elf-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>

New smatch warnings:
drivers/net/ethernet/engleder/tsnep_main.c:641 tsnep_xdp_xmit_back() warn: signedness bug returning '(-14)'

vim +641 drivers/net/ethernet/engleder/tsnep_main.c

dd23b834a352b5 Gerhard Engleder 2023-01-04  631  static bool tsnep_xdp_xmit_back(struct tsnep_adapter *adapter,
                                                        ^^^^
dd23b834a352b5 Gerhard Engleder 2023-01-04  632  				struct xdp_buff *xdp)
dd23b834a352b5 Gerhard Engleder 2023-01-04  633  {
dd23b834a352b5 Gerhard Engleder 2023-01-04  634  	struct xdp_frame *xdpf = xdp_convert_buff_to_frame(xdp);
dd23b834a352b5 Gerhard Engleder 2023-01-04  635  	int cpu = smp_processor_id();
dd23b834a352b5 Gerhard Engleder 2023-01-04  636  	struct netdev_queue *nq;
dd23b834a352b5 Gerhard Engleder 2023-01-04  637  	int queue;
dd23b834a352b5 Gerhard Engleder 2023-01-04  638  	bool xmit;
dd23b834a352b5 Gerhard Engleder 2023-01-04  639  
dd23b834a352b5 Gerhard Engleder 2023-01-04  640  	if (unlikely(!xdpf))
dd23b834a352b5 Gerhard Engleder 2023-01-04 @641  		return -EFAULT;

return false?

dd23b834a352b5 Gerhard Engleder 2023-01-04  642  
dd23b834a352b5 Gerhard Engleder 2023-01-04  643  	queue = cpu % adapter->num_tx_queues;
dd23b834a352b5 Gerhard Engleder 2023-01-04  644  	nq = netdev_get_tx_queue(adapter->netdev, queue);
dd23b834a352b5 Gerhard Engleder 2023-01-04  645  
dd23b834a352b5 Gerhard Engleder 2023-01-04  646  	__netif_tx_lock(nq, cpu);
dd23b834a352b5 Gerhard Engleder 2023-01-04  647  
dd23b834a352b5 Gerhard Engleder 2023-01-04  648  	/* Avoid transmit queue timeout since we share it with the slow path */
dd23b834a352b5 Gerhard Engleder 2023-01-04  649  	txq_trans_cond_update(nq);
dd23b834a352b5 Gerhard Engleder 2023-01-04  650  
dd23b834a352b5 Gerhard Engleder 2023-01-04  651  	xmit = tsnep_xdp_xmit_frame_ring(xdpf, &adapter->tx[queue],
dd23b834a352b5 Gerhard Engleder 2023-01-04  652  					 TSNEP_TX_TYPE_XDP_TX);
dd23b834a352b5 Gerhard Engleder 2023-01-04  653  
dd23b834a352b5 Gerhard Engleder 2023-01-04  654  	__netif_tx_unlock(nq);
dd23b834a352b5 Gerhard Engleder 2023-01-04  655  
dd23b834a352b5 Gerhard Engleder 2023-01-04  656  	return xmit;
dd23b834a352b5 Gerhard Engleder 2023-01-04  657  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH net-next v3 9/9] tsnep: Add XDP RX support

[Gerhard Engleder]

On 07.01.23 09:46, Dan Carpenter wrote:
> dd23b834a352b5 Gerhard Engleder 2023-01-04  631  static bool tsnep_xdp_xmit_back(struct tsnep_adapter *adapter,
>                                                          ^^^^
> dd23b834a352b5 Gerhard Engleder 2023-01-04  632  				struct xdp_buff *xdp)
> dd23b834a352b5 Gerhard Engleder 2023-01-04  633  {
> dd23b834a352b5 Gerhard Engleder 2023-01-04  634  	struct xdp_frame *xdpf = xdp_convert_buff_to_frame(xdp);
> dd23b834a352b5 Gerhard Engleder 2023-01-04  635  	int cpu = smp_processor_id();
> dd23b834a352b5 Gerhard Engleder 2023-01-04  636  	struct netdev_queue *nq;
> dd23b834a352b5 Gerhard Engleder 2023-01-04  637  	int queue;
> dd23b834a352b5 Gerhard Engleder 2023-01-04  638  	bool xmit;
> dd23b834a352b5 Gerhard Engleder 2023-01-04  639
> dd23b834a352b5 Gerhard Engleder 2023-01-04  640  	if (unlikely(!xdpf))
> dd23b834a352b5 Gerhard Engleder 2023-01-04 @641  		return -EFAULT;
> 
> return false?

Of course. Will be fixed. Thanks!

Gerhard