[PATCH net-next v6 0/9] bnxt_en: implement tcp-data-split and thresh option

[PATCH net-next v6 0/9] bnxt_en: implement tcp-data-split and thresh option

[Taehee Yoo]

This series implements hds-thresh ethtool command.
This series also implements backend of tcp-data-split and
hds-thresh ethtool command for bnxt_en driver.
These ethtool commands are mandatory options for device memory TCP.

NICs that use the bnxt_en driver support tcp-data-split feature named
HDS(header-data-split).
But there is no implementation for the HDS to enable by ethtool.
Only getting the current HDS status is implemented and the HDS is just
automatically enabled only when either LRO, HW-GRO, or JUMBO is enabled.
The hds_threshold follows the rx-copybreak value but it wasn't
changeable.

Currently, bnxt_en driver enables tcp-data-split by default but not
always work.
There is hds_threshold value, which indicates that a packet size is
larger than this value, a packet will be split into header and data.
hds_threshold value has been 256, which is a default value of
rx-copybreak value too.
The rx-copybreak value hasn't been allowed to change so the
hds_threshold too.

This patchset decouples hds_threshold and rx-copybreak first.
and make tcp-data-split, rx-copybreak, and
hds-thresh configurable independently.

But the default configuration is the same.
The default value of rx-copybreak is 256 and default
hds-thresh is also 256.

The behavior of rx-copybreak will probably be changed in almost all
drivers. If HDS is not enabled, rx-copybreak copies both header and
payload from a page.
But if HDS is enabled, rx-copybreak copies only header from the first
page.
Due to this change, it may need to disable(set to 0) rx-copybreak when
the HDS is required.

There are several related options.
TPA(HW-GRO, LRO), JUMBO, jumbo_thresh(firmware command), and Aggregation
Ring.

The aggregation ring is fundamental to these all features.
When gro/lro/jumbo packets are received, NIC receives the first packet
from the normal ring.
follow packets come from the aggregation ring.

These features are working regardless of HDS.
If HDS is enabled, the first packet contains the header only, and the
following packets contain only payload.
So, HW-GRO/LRO is working regardless of HDS.

There is another threshold value, which is jumbo_thresh.
This is very similar to hds_thresh, but jumbo thresh doesn't split
header and data.
It just split the first and following data based on length.
When NIC receives 1500 sized packet, and jumbo_thresh is 256(default, but
follows rx-copybreak),
the first data is 256 and the following packet size is 1500-256.

Before this patch, at least if one of GRO, LRO, and JUMBO flags is
enabled, the Aggregation ring will be enabled.
If the Aggregation ring is enabled, both hds_threshold and
jumbo_thresh are set to the default value of rx-copybreak.

So, GRO, LRO, JUMBO frames, they larger than 256 bytes, they will
be split into header and data if the protocol is TCP or UDP.
for the other protocol, jumbo_thresh works instead of hds_thresh.

This means that tcp-data-split relies on the GRO, LRO, and JUMBO flags.
But by this patch, tcp-data-split no longer relies on these flags.
If the tcp-data-split is enabled, the Aggregation ring will be
enabled.
Also, hds_threshold no longer follows rx-copybreak value, it will
be set to the hds-thresh value by user-space, but the
default value is still 256.

If the protocol is TCP or UDP and the HDS is disabled and Aggregation
ring is enabled, a packet will be split into several pieces due to
jumbo_thresh.

When single buffer XDP is attached, tcp-data-split is automatically
disabled.

LRO, GRO, and JUMBO are tested with BCM57414, BCM57504 and the firmware
version is 230.0.157.0.
I couldn't find any specification about minimum and maximum value
of hds_threshold, but from my test result, it was about 0 ~ 1023.
It means, over 1023 sized packets will be split into header and data if
tcp-data-split is enabled regardless of hds_treshold value.
When hds_threshold is 1500 and received packet size is 1400, HDS should
not be activated, but it is activated.
The maximum value of hds-thresh value is 256 because it
has been working. It was decided very conservatively.

I checked out the tcp-data-split(HDS) works independently of GRO, LRO,
JUMBO.
Also, I checked out tcp-data-split should be disabled automatically
when XDP is attached and disallowed to enable it again while XDP is
attached. I tested ranged values from min to max for
hds-thresh and rx-copybreak, and it works.
hds-thresh from 0 to 256, and rx-copybreak 0 to 256.
When testing this patchset, I checked skb->data, skb->data_len, and
nr_frags values.

By this patchset, bnxt_en driver supports a force enable tcp-data-split,
but it doesn't support for disable tcp-data-split.
When tcp-data-split is explicitly enabled, HDS works always.
When tcp-data-split is unknown, it depends on the current
configuration of LRO/GRO/JUMBO.

1/9 patch implements .{set, get}_tunable() in the bnxt_en.
The bnxt_en driver has been supporting the rx-copybreak feature but is
not configurable, Only the default rx-copybreak value has been working.
So, it changes the bnxt_en driver to be able to configure
the rx-copybreak value.

2/9 patch adds a new hds_config member in the ethtool_netdev_state.
It indicates that what tcp-data-split value is really updated from
userspace.
So the driver can distinguish a passed tcp-data-split value is
came from user or driver itself.

3/9 patch adds an implementation of tcp-data-split ethtool
command.
The HDS relies on the Aggregation ring, which is automatically enabled
when either LRO, GRO, or large mtu is configured.
So, if the Aggregation ring is enabled, HDS is automatically enabled by
it.

4/9 patch adds hds-thresh command in the ethtool.
This threshold value indicates if a received packet size is larger
than this threshold, the packet's header and payload will be split.
Example:
   # ethtool -G <interface name> hds-thresh <value>
This option can not be used when tcp-data-split is disabled or not
supported.
   # ethtool -G enp14s0f0np0 tcp-data-split on hds-thresh 256
   # ethtool -g enp14s0f0np0
   Ring parameters for enp14s0f0np0:
   Pre-set maximums:
   ...
   Current hardware settings:
   ...
   TCP data split:         on
   HDS thresh:  256

5/9 patch adds the implementation of hds-thresh logic
in the bnxt_en driver.
The default value is 256, which used to be the default rx-copybreak
value.

6/9, 7/9 add condition checks for devmem and ethtool.
If tcp-data-split is disabled or threshold value is not zero, setup of
devmem will be failed.
Also, tcp-data-split and hds-thresh will not be changed
while devmem is running.

8/9 add condition checks for netdev core.
It disallows setup single buffer XDP program when tcp-data-split is
enabled.

9/9 add HDS feature implementation for netdevsim.
HDS feature is not common so far. Only a few NICs support this feature.
There is no way to test HDS core-API unless we have proper hw NIC.
In order to test HDS core-API without  hw NIC, netdevsim can be used.
So, implements HDS contronplane for netdevsim.

This series is tested with BCM57504 and netdevsim.

v6:
 - use hds_config instead of tcp_data_split_mod.
 - Disallow to attach XDP when HDS is in use.
 - Update ethtool_netlink_generated.h
 - Use "HDS" instead of "HEADER_DATA_SPLIT"
 - HDS_MAX is changed to 1023.
 - Implement netdevsim HDS feature.
 - Add Test tags from Andy.

v5:
 - Remove netdev_devmem_enabled() and use dev_get_min_mp_channel_count()
   instead.
 - change extack messages
 - Drop implementation of device memory TCP for bnxt_en.
 - Add Review tags from Mina.

v4:
 - Remove min rx-copybreak value.
 - Do not support a disable of tcp-data-split by bnxt_en driver.
 - Rename from tcp-data-split-thresh to hds-thresh.
 - Add ETHTOOL_RING_USE_HDS_THRS flag.
 - Add dev_xdp_sb_prog_count() helper.
 - Reduce hole in struct bnxt.
 - Use ETHTOOL_RING_USE_HDS_THRS in bnxt_en driver.
 - Improve condition check.
 - Add netdev_devmem_enabled() helper.
 - Add netmem_is_pfmemalloc() helper.
 - Do not select NET_DEVMEM in Kconfig for bnxt_en driver.
 - Pass PP_FLAG_ALLOW_UNREADABLE_NETMEM flag unconditionally.
 - Use gfp flag in __bnxt_alloc_rx_netmem() in the last patch.
 - Do not add *offset in the __bnxt_alloc_rx_netmem() in the last patch.
 - Do not pass queue_idx to bnxt_alloc_rx_page_pool() in the last patch.
 - Add Test tag from Stanislav.
 - Add Review tag from Brett.
 - Add page_pool_recycle_direct_netmem() helper

v3:
 - Change headline
 - Add condition checks for ethtool and devmem
 - Fix documentation
 - Move validation of tcp-data-split and thresh from dirver to core API
 - Add implementation of device memory TCP for bnxt_en driver

v2:
 - Add tcp-data-split-thresh ethtool command
 - Implement tcp-data-split-threh in the bnxt_en driver
 - Define min/max rx-copybreak value
 - Update commit message

Taehee Yoo (9):
  bnxt_en: add support for rx-copybreak ethtool command
  net: ethtool: add hds_config member in ethtool_netdev_state
  bnxt_en: add support for tcp-data-split ethtool command
  net: ethtool: add support for configuring hds-thresh
  bnxt_en: add support for hds-thresh ethtool command
  net: devmem: add ring parameter filtering
  net: ethtool: add ring parameter filtering
  net: disallow setup single buffer XDP when tcp-data-split is enabled.
  netdevsim: add HDS feature

 Documentation/netlink/specs/ethtool.yaml      |  8 ++
 Documentation/networking/ethtool-netlink.rst  | 10 +++
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 31 ++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     | 12 ++-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 75 ++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  4 +
 drivers/net/netdevsim/ethtool.c               | 15 +++-
 drivers/net/netdevsim/netdevsim.h             |  4 +
 include/linux/ethtool.h                       |  8 ++
 include/linux/netdevice.h                     |  1 +
 .../uapi/linux/ethtool_netlink_generated.h    |  2 +
 net/core/dev.c                                | 29 +++++++
 net/core/devmem.c                             | 19 +++++
 net/ethtool/netlink.h                         |  2 +-
 net/ethtool/rings.c                           | 55 +++++++++++++-
 15 files changed, 254 insertions(+), 21 deletions(-)

-- 
2.34.1

[PATCH net-next v6 1/9] bnxt_en: add support for rx-copybreak ethtool command

[Taehee Yoo]

The bnxt_en driver supports rx-copybreak, but it couldn't be set by
userspace. Only the default value(256) has worked.
This patch makes the bnxt_en driver support following command.
`ethtool --set-tunable <devname> rx-copybreak <value> ` and
`ethtool --get-tunable <devname> rx-copybreak`.

By this patch, hds_threshol is set to the rx-copybreak value.
But it will be set by `ethtool -G eth0 hds-thresh N`
in the next patch.

Reviewed-by: Brett Creeley <brett.creeley@amd.com>
Tested-by: Stanislav Fomichev <sdf@fomichev.me>
Tested-by: Andy Gospodarek <gospo@broadcom.com>
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v6:
 - No changes.

v5:
 - Do not set HDS if XDP is attached.
 - rx_size and pkt_size are always bigger than 256.

v4:
 - Remove min rx-copybreak value.
 - Add Review tag from Brett.
 - Add Test tag from Stanislav.

v3:
 - Update copybreak value after closing nic and before opening nic when
   the device is running.

v2:
 - Define max/vim rx_copybreak value.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 28 ++++++-----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  5 +-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 49 ++++++++++++++++++-
 3 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index b86f980fa7ea..c31894b9187e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -81,7 +81,6 @@ MODULE_DESCRIPTION("Broadcom NetXtreme network driver");
 
 #define BNXT_RX_OFFSET (NET_SKB_PAD + NET_IP_ALIGN)
 #define BNXT_RX_DMA_OFFSET NET_SKB_PAD
-#define BNXT_RX_COPY_THRESH 256
 
 #define BNXT_TX_PUSH_THRESH 164
 
@@ -1343,13 +1342,13 @@ static struct sk_buff *bnxt_copy_data(struct bnxt_napi *bnapi, u8 *data,
 	if (!skb)
 		return NULL;
 
-	dma_sync_single_for_cpu(&pdev->dev, mapping, bp->rx_copy_thresh,
+	dma_sync_single_for_cpu(&pdev->dev, mapping, bp->rx_copybreak,
 				bp->rx_dir);
 
 	memcpy(skb->data - NET_IP_ALIGN, data - NET_IP_ALIGN,
 	       len + NET_IP_ALIGN);
 
-	dma_sync_single_for_device(&pdev->dev, mapping, bp->rx_copy_thresh,
+	dma_sync_single_for_device(&pdev->dev, mapping, bp->rx_copybreak,
 				   bp->rx_dir);
 
 	skb_put(skb, len);
@@ -1842,7 +1841,7 @@ static inline struct sk_buff *bnxt_tpa_end(struct bnxt *bp,
 		return NULL;
 	}
 
-	if (len <= bp->rx_copy_thresh) {
+	if (len <= bp->rx_copybreak) {
 		skb = bnxt_copy_skb(bnapi, data_ptr, len, mapping);
 		if (!skb) {
 			bnxt_abort_tpa(cpr, idx, agg_bufs);
@@ -2176,7 +2175,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 		}
 	}
 
-	if (len <= bp->rx_copy_thresh) {
+	if (len <= bp->rx_copybreak) {
 		if (!xdp_active)
 			skb = bnxt_copy_skb(bnapi, data_ptr, len, dma_addr);
 		else
@@ -4601,6 +4600,11 @@ void bnxt_set_tpa_flags(struct bnxt *bp)
 		bp->flags |= BNXT_FLAG_GRO;
 }
 
+static void bnxt_init_ring_params(struct bnxt *bp)
+{
+	bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
+}
+
 /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
  * be set on entry.
  */
@@ -4615,7 +4619,6 @@ void bnxt_set_ring_params(struct bnxt *bp)
 	rx_space = rx_size + ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) +
 		SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
-	bp->rx_copy_thresh = BNXT_RX_COPY_THRESH;
 	ring_size = bp->rx_ring_size;
 	bp->rx_agg_ring_size = 0;
 	bp->rx_agg_nr_pages = 0;
@@ -4660,7 +4663,9 @@ void bnxt_set_ring_params(struct bnxt *bp)
 				  ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) -
 				  SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 		} else {
-			rx_size = SKB_DATA_ALIGN(BNXT_RX_COPY_THRESH + NET_IP_ALIGN);
+			rx_size = SKB_DATA_ALIGN(max(BNXT_DEFAULT_RX_COPYBREAK,
+						     bp->rx_copybreak) +
+						 NET_IP_ALIGN);
 			rx_space = rx_size + NET_SKB_PAD +
 				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 		}
@@ -6566,16 +6571,14 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
 
 	req->flags = cpu_to_le32(VNIC_PLCMODES_CFG_REQ_FLAGS_JUMBO_PLACEMENT);
 	req->enables = cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_JUMBO_THRESH_VALID);
+	req->jumbo_thresh = cpu_to_le16(bp->rx_buf_use_size);
 
-	if (BNXT_RX_PAGE_MODE(bp)) {
-		req->jumbo_thresh = cpu_to_le16(bp->rx_buf_use_size);
-	} else {
+	if (!BNXT_RX_PAGE_MODE(bp) && (bp->flags & BNXT_FLAG_AGG_RINGS)) {
 		req->flags |= cpu_to_le32(VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV4 |
 					  VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
 		req->enables |=
 			cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
-		req->jumbo_thresh = cpu_to_le16(bp->rx_copy_thresh);
-		req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh);
+		req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
 	}
 	req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
 	return hwrm_req_send(bp, req);
@@ -16188,6 +16191,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	bnxt_init_l2_fltr_tbl(bp);
 	bnxt_set_rx_skb_mode(bp, false);
 	bnxt_set_tpa_flags(bp);
+	bnxt_init_ring_params(bp);
 	bnxt_set_ring_params(bp);
 	bnxt_rdma_aux_device_init(bp);
 	rc = bnxt_set_dflt_rings(bp, true);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 7df7a2233307..b73de5683063 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -34,6 +34,9 @@
 #include <linux/firmware/broadcom/tee_bnxt_fw.h>
 #endif
 
+#define BNXT_DEFAULT_RX_COPYBREAK 256
+#define BNXT_MAX_RX_COPYBREAK 1024
+
 extern struct list_head bnxt_block_cb_list;
 
 struct page_pool;
@@ -2342,7 +2345,7 @@ struct bnxt {
 	enum dma_data_direction	rx_dir;
 	u32			rx_ring_size;
 	u32			rx_agg_ring_size;
-	u32			rx_copy_thresh;
+	u32			rx_copybreak;
 	u32			rx_ring_mask;
 	u32			rx_agg_ring_mask;
 	int			rx_nr_pages;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index d87681d71106..4cdfff5d531c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -4327,6 +4327,50 @@ static int bnxt_get_eee(struct net_device *dev, struct ethtool_keee *edata)
 	return 0;
 }
 
+static int bnxt_set_tunable(struct net_device *dev,
+			    const struct ethtool_tunable *tuna,
+			    const void *data)
+{
+	struct bnxt *bp = netdev_priv(dev);
+	u32 rx_copybreak;
+
+	switch (tuna->id) {
+	case ETHTOOL_RX_COPYBREAK:
+		rx_copybreak = *(u32 *)data;
+		if (rx_copybreak > BNXT_MAX_RX_COPYBREAK)
+			return -ERANGE;
+		if (rx_copybreak != bp->rx_copybreak) {
+			if (netif_running(dev)) {
+				bnxt_close_nic(bp, false, false);
+				bp->rx_copybreak = rx_copybreak;
+				bnxt_set_ring_params(bp);
+				bnxt_open_nic(bp, false, false);
+			} else {
+				bp->rx_copybreak = rx_copybreak;
+			}
+		}
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int bnxt_get_tunable(struct net_device *dev,
+			    const struct ethtool_tunable *tuna, void *data)
+{
+	struct bnxt *bp = netdev_priv(dev);
+
+	switch (tuna->id) {
+	case ETHTOOL_RX_COPYBREAK:
+		*(u32 *)data = bp->rx_copybreak;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int bnxt_read_sfp_module_eeprom_info(struct bnxt *bp, u16 i2c_addr,
 					    u16 page_number, u8 bank,
 					    u16 start_addr, u16 data_length,
@@ -4777,7 +4821,8 @@ static int bnxt_run_loopback(struct bnxt *bp)
 	cpr = &rxr->bnapi->cp_ring;
 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)
 		cpr = rxr->rx_cpr;
-	pkt_size = min(bp->dev->mtu + ETH_HLEN, bp->rx_copy_thresh);
+	pkt_size = min(bp->dev->mtu + ETH_HLEN, max(BNXT_DEFAULT_RX_COPYBREAK,
+						    bp->rx_copybreak));
 	skb = netdev_alloc_skb(bp->dev, pkt_size);
 	if (!skb)
 		return -ENOMEM;
@@ -5350,6 +5395,8 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 	.get_link_ext_stats	= bnxt_get_link_ext_stats,
 	.get_eee		= bnxt_get_eee,
 	.set_eee		= bnxt_set_eee,
+	.get_tunable		= bnxt_get_tunable,
+	.set_tunable		= bnxt_set_tunable,
 	.get_module_info	= bnxt_get_module_info,
 	.get_module_eeprom	= bnxt_get_module_eeprom,
 	.get_module_eeprom_by_page = bnxt_get_module_eeprom_by_page,
-- 
2.34.1

[PATCH net-next v6 2/9] net: ethtool: add hds_config member in ethtool_netdev_state

[Taehee Yoo]

When tcp-data-split is UNKNOWN mode, drivers arbitrarily handle it.
For example, bnxt_en driver automatically enables if at least one of
LRO/GRO/JUMBO is enabled.
If tcp-data-split is UNKNOWN and LRO is enabled, a driver returns
ENABLES of tcp-data-split, not UNKNOWN.
So, `ethtool -g eth0` shows tcp-data-split is enabled.

The problem is in the setting situation.
In the ethnl_set_rings(), it first calls get_ringparam() to get the
current driver's config.
At that moment, if driver's tcp-data-split config is UNKNOWN, it returns
ENABLE if LRO/GRO/JUMBO is enabled.
Then, it sets values from the user and driver's current config to
kernel_ethtool_ringparam.
Last it calls .set_ringparam().
The driver, especially bnxt_en driver receives
ETHTOOL_TCP_DATA_SPLIT_ENABLED.
But it can't distinguish whether it is set by the user or just the
current config.

When user updates ring parameter, the new hds_config value is updated
and current hds_config value is stored to old_hdsconfig.
Driver's .set_ringparam() callback can distinguish a passed
tcp-data-split value is came from user explicitly.
If .set_ringparam() is failed, hds_config is rollbacked immediately.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v6:
 - use hds_config instead of using tcp_data_split_mod.

v5:
 - Patch added.

 include/linux/ethtool.h | 2 ++
 net/ethtool/rings.c     | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f711bfd75c4d..4e451084d58a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1134,12 +1134,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
  * @rss_ctx:		XArray of custom RSS contexts
  * @rss_lock:		Protects entries in @rss_ctx.  May be taken from
  *			within RTNL.
+ * @hds_config:		HDS value from userspace.
  * @wol_enabled:	Wake-on-LAN is enabled
  * @module_fw_flash_in_progress: Module firmware flashing is in progress.
  */
 struct ethtool_netdev_state {
 	struct xarray		rss_ctx;
 	struct mutex		rss_lock;
+	u8			hds_config;
 	unsigned		wol_enabled:1;
 	unsigned		module_fw_flash_in_progress:1;
 };
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index b7865a14fdf8..2e8239a76234 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -203,6 +203,7 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 
 	dev->ethtool_ops->get_ringparam(dev, &ringparam,
 					&kernel_ringparam, info->extack);
+	kernel_ringparam.tcp_data_split = dev->ethtool->hds_config;
 
 	ethnl_update_u32(&ringparam.rx_pending, tb[ETHTOOL_A_RINGS_RX], &mod);
 	ethnl_update_u32(&ringparam.rx_mini_pending,
@@ -252,6 +253,9 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 
 	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
 					      &kernel_ringparam, info->extack);
+	if (!ret)
+		dev->ethtool->hds_config = kernel_ringparam.tcp_data_split;
+
 	return ret < 0 ? ret : 1;
 }
 
-- 
2.34.1

[PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Taehee Yoo]

NICs that uses bnxt_en driver supports tcp-data-split feature by the
name of HDS(header-data-split).
But there is no implementation for the HDS to enable by ethtool.
Only getting the current HDS status is implemented and The HDS is just
automatically enabled only when either LRO, HW-GRO, or JUMBO is enabled.
The hds_threshold follows rx-copybreak value. and it was unchangeable.

This implements `ethtool -G <interface name> tcp-data-split <value>`
command option.
The value can be <on> and <auto>.
The value is <auto> and one of LRO/GRO/JUMBO is enabled, HDS is
automatically enabled and all LRO/GRO/JUMBO are disabled, HDS is
automatically disabled.

HDS feature relies on the aggregation ring.
So, if HDS is enabled, the bnxt_en driver initializes the aggregation ring.
This is the reason why BNXT_FLAG_AGG_RINGS contains HDS condition.

Tested-by: Stanislav Fomichev <sdf@fomichev.me>
Tested-by: Andy Gospodarek <gospo@broadcom.com>
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v6:
 - Disallow to attach XDP when HDS is in use.
 - Add Test tag from Andy.

v5:
 - Do not set HDS if XDP is attached.
 - Enable tcp-data-split only when tcp_data_split_mod is true.

v4:
 - Do not support disable tcp-data-split.
 - Add Test tag from Stanislav.

v3:
 - No changes.

v2:
 - Do not set hds_threshold to 0.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  5 +++--
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 21 +++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  4 ++++
 4 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c31894b9187e..42ffaa88ae4e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4623,7 +4623,7 @@ void bnxt_set_ring_params(struct bnxt *bp)
 	bp->rx_agg_ring_size = 0;
 	bp->rx_agg_nr_pages = 0;
 
-	if (bp->flags & BNXT_FLAG_TPA)
+	if (bp->flags & BNXT_FLAG_TPA || bp->flags & BNXT_FLAG_HDS)
 		agg_factor = min_t(u32, 4, 65536 / BNXT_RX_PAGE_SIZE);
 
 	bp->flags &= ~BNXT_FLAG_JUMBO;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index b73de5683063..847dedf61a9e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2244,8 +2244,6 @@ struct bnxt {
 	#define BNXT_FLAG_TPA		(BNXT_FLAG_LRO | BNXT_FLAG_GRO)
 	#define BNXT_FLAG_JUMBO		0x10
 	#define BNXT_FLAG_STRIP_VLAN	0x20
-	#define BNXT_FLAG_AGG_RINGS	(BNXT_FLAG_JUMBO | BNXT_FLAG_GRO | \
-					 BNXT_FLAG_LRO)
 	#define BNXT_FLAG_RFS		0x100
 	#define BNXT_FLAG_SHARED_RINGS	0x200
 	#define BNXT_FLAG_PORT_STATS	0x400
@@ -2266,6 +2264,9 @@ struct bnxt {
 	#define BNXT_FLAG_ROCE_MIRROR_CAP	0x4000000
 	#define BNXT_FLAG_TX_COAL_CMPL	0x8000000
 	#define BNXT_FLAG_PORT_STATS_EXT	0x10000000
+	#define BNXT_FLAG_HDS		0x20000000
+	#define BNXT_FLAG_AGG_RINGS	(BNXT_FLAG_JUMBO | BNXT_FLAG_GRO | \
+					 BNXT_FLAG_LRO | BNXT_FLAG_HDS)
 
 	#define BNXT_FLAG_ALL_CONFIG_FEATS (BNXT_FLAG_TPA |		\
 					    BNXT_FLAG_RFS |		\
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 4cdfff5d531c..25eb5931aea9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -840,16 +840,36 @@ static int bnxt_set_ringparam(struct net_device *dev,
 			      struct kernel_ethtool_ringparam *kernel_ering,
 			      struct netlink_ext_ack *extack)
 {
+	u8 tcp_data_split = kernel_ering->tcp_data_split;
 	struct bnxt *bp = netdev_priv(dev);
+	u8 hds_config_mod;
+
+	hds_config_mod = tcp_data_split != dev->ethtool->hds_config;
 
 	if ((ering->rx_pending > BNXT_MAX_RX_DESC_CNT) ||
 	    (ering->tx_pending > BNXT_MAX_TX_DESC_CNT) ||
 	    (ering->tx_pending < BNXT_MIN_TX_DESC_CNT))
 		return -EINVAL;
 
+	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED && hds_config_mod)
+		return -EOPNOTSUPP;
+
+	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	    hds_config_mod && BNXT_RX_PAGE_MODE(bp)) {
+		NL_SET_ERR_MSG_MOD(extack, "tcp-data-split is disallowed when XDP is attached");
+		return -EOPNOTSUPP;
+	}
+
 	if (netif_running(dev))
 		bnxt_close_nic(bp, false, false);
 
+	if (hds_config_mod) {
+		if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED)
+			bp->flags |= BNXT_FLAG_HDS;
+		else if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
+			bp->flags &= ~BNXT_FLAG_HDS;
+	}
+
 	bp->rx_ring_size = ering->rx_pending;
 	bp->tx_ring_size = ering->tx_pending;
 	bnxt_set_ring_params(bp);
@@ -5354,6 +5374,7 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 				     ETHTOOL_COALESCE_STATS_BLOCK_USECS |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
 				     ETHTOOL_COALESCE_USE_CQE,
+	.supported_ring_params	= ETHTOOL_RING_USE_TCP_DATA_SPLIT,
 	.get_link_ksettings	= bnxt_get_link_ksettings,
 	.set_link_ksettings	= bnxt_set_link_ksettings,
 	.get_fec_stats		= bnxt_get_fec_stats,
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index f88b641533fc..1bfff7f29310 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
 			    bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
 		return -EOPNOTSUPP;
 	}
+	if (prog && bp->flags & BNXT_FLAG_HDS) {
+		netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
+		return -EOPNOTSUPP;
+	}
 	if (!(bp->flags & BNXT_FLAG_SHARED_RINGS)) {
 		netdev_warn(dev, "ethtool rx/tx channels must be combined to support XDP.\n");
 		return -EOPNOTSUPP;
-- 
2.34.1

[PATCH net-next v6 4/9] net: ethtool: add support for configuring hds-thresh

[Taehee Yoo]

The hds-thresh option configures the threshold value of
the header-data-split.
If a received packet size is larger than this threshold value, a packet
will be split into header and payload.
The header indicates TCP and UDP header, but it depends on driver spec.
The bnxt_en driver supports HDS(Header-Data-Split) configuration at
FW level, affecting TCP and UDP too.
So, If hds-thresh is set, it affects UDP and TCP packets.

Example:
   # ethtool -G <interface name> hds-thresh <value>

   # ethtool -G enp14s0f0np0 tcp-data-split on hds-thresh 256
   # ethtool -g enp14s0f0np0
   Ring parameters for enp14s0f0np0:
   Pre-set maximums:
   ...
   HDS thresh:  1023
   Current hardware settings:
   ...
   TCP data split:         on
   HDS thresh:  256

The default/min/max values are not defined in the ethtool so the drivers
should define themself.
The 0 value means that all TCP/UDP packets' header and payload
will be split.

Tested-by: Stanislav Fomichev <sdf@fomichev.me>
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v6:
 - Update ethtool_netlink_generated.h
 - Use "HDS" instead of "HEADER_DATA_SPLIT"
 - Add Test tag from Andy.

v5:
 - No changes.

v4:
 - Fix 80 charactor wrap.
 - Rename from tcp-data-split-thresh to header-data-split-thresh
 - Add description about overhead of HDS.
 - Add ETHTOOL_RING_USE_HDS_THRS flag.
 - Add dev_xdp_sb_prog_count() helper.
 - Add Test tag from Stanislav.

v3:
 - Fix documentation and ynl
 - Update error messages
 - Validate configuration of tcp-data-split and tcp-data-split-thresh

v2:
 - Patch added.

 Documentation/netlink/specs/ethtool.yaml      |  8 ++++
 Documentation/networking/ethtool-netlink.rst  | 10 +++++
 include/linux/ethtool.h                       |  6 +++
 include/linux/netdevice.h                     |  1 +
 .../uapi/linux/ethtool_netlink_generated.h    |  2 +
 net/core/dev.c                                | 13 ++++++
 net/ethtool/netlink.h                         |  2 +-
 net/ethtool/rings.c                           | 40 +++++++++++++++++--
 8 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 60f85fbf4156..66be04013048 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -332,6 +332,12 @@ attribute-sets:
       -
         name: tx-push-buf-len-max
         type: u32
+      -
+        name: hds-thresh
+        type: u32
+      -
+        name: hds-thresh-max
+        type: u32
 
   -
     name: mm-stat
@@ -1777,6 +1783,8 @@ operations:
             - rx-push
             - tx-push-buf-len
             - tx-push-buf-len-max
+            - hds-thresh
+            - hds-thresh-max
       dump: *ring-get-op
     -
       name: rings-set
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index a7ba6368a4d5..ef1d1750f960 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -899,6 +899,10 @@ Kernel response contents:
   ``ETHTOOL_A_RINGS_RX_PUSH``               u8      flag of RX Push mode
   ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``       u32     size of TX push buffer
   ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX``   u32     max size of TX push buffer
+  ``ETHTOOL_A_RINGS_HDS_THRESH``            u32     threshold of
+                                                    header / data split
+  ``ETHTOOL_A_RINGS_HDS_THRESH_MAX``        u32     max threshold of
+                                                    header / data split
   =======================================   ======  ===========================
 
 ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device is usable with
@@ -941,10 +945,12 @@ Request contents:
   ``ETHTOOL_A_RINGS_RX_JUMBO``          u32     size of RX jumbo ring
   ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
   ``ETHTOOL_A_RINGS_RX_BUF_LEN``        u32     size of buffers on the ring
+  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT``    u8      TCP header / data split
   ``ETHTOOL_A_RINGS_CQE_SIZE``          u32     Size of TX/RX CQE
   ``ETHTOOL_A_RINGS_TX_PUSH``           u8      flag of TX Push mode
   ``ETHTOOL_A_RINGS_RX_PUSH``           u8      flag of RX Push mode
   ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``   u32     size of TX push buffer
+  ``ETHTOOL_A_RINGS_HDS_THRESH``        u32     threshold of header / data split
   ====================================  ======  ===========================
 
 Kernel checks that requested ring sizes do not exceed limits reported by
@@ -961,6 +967,10 @@ A bigger CQE can have more receive buffer pointers, and in turn the NIC can
 transfer a bigger frame from wire. Based on the NIC hardware, the overall
 completion queue size can be adjusted in the driver if CQE size is modified.
 
+``ETHTOOL_A_RINGS_HDS_THRESH`` specifies the threshold value of
+header / data split feature. If a received packet size is larger than this
+threshold value, header and data will be split.
+
 CHANNELS_GET
 ============
 
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 4e451084d58a..4f407ce9eed1 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -78,6 +78,8 @@ enum {
  * @cqe_size: Size of TX/RX completion queue event
  * @tx_push_buf_len: Size of TX push buffer
  * @tx_push_buf_max_len: Maximum allowed size of TX push buffer
+ * @hds_thresh: Threshold value of header-data-split-thresh
+ * @hds_thresh_max: Maximum allowed threshold of header-data-split-thresh
  */
 struct kernel_ethtool_ringparam {
 	u32	rx_buf_len;
@@ -87,6 +89,8 @@ struct kernel_ethtool_ringparam {
 	u32	cqe_size;
 	u32	tx_push_buf_len;
 	u32	tx_push_buf_max_len;
+	u32	hds_thresh;
+	u32	hds_thresh_max;
 };
 
 /**
@@ -97,6 +101,7 @@ struct kernel_ethtool_ringparam {
  * @ETHTOOL_RING_USE_RX_PUSH: capture for setting rx_push
  * @ETHTOOL_RING_USE_TX_PUSH_BUF_LEN: capture for setting tx_push_buf_len
  * @ETHTOOL_RING_USE_TCP_DATA_SPLIT: capture for setting tcp_data_split
+ * @ETHTOOL_RING_USE_HDS_THRS: capture for setting header-data-split-thresh
  */
 enum ethtool_supported_ring_param {
 	ETHTOOL_RING_USE_RX_BUF_LEN		= BIT(0),
@@ -105,6 +110,7 @@ enum ethtool_supported_ring_param {
 	ETHTOOL_RING_USE_RX_PUSH		= BIT(3),
 	ETHTOOL_RING_USE_TX_PUSH_BUF_LEN	= BIT(4),
 	ETHTOOL_RING_USE_TCP_DATA_SPLIT		= BIT(5),
+	ETHTOOL_RING_USE_HDS_THRS		= BIT(6),
 };
 
 #define __ETH_RSS_HASH_BIT(bit)	((u32)1 << (bit))
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2593019ad5b1..b18f249826c5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4083,6 +4083,7 @@ struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 
 int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
 u8 dev_xdp_prog_count(struct net_device *dev);
+u8 dev_xdp_sb_prog_count(struct net_device *dev);
 int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf);
 u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode mode);
 
diff --git a/include/uapi/linux/ethtool_netlink_generated.h b/include/uapi/linux/ethtool_netlink_generated.h
index 43993a2d68e5..2e17ff348f89 100644
--- a/include/uapi/linux/ethtool_netlink_generated.h
+++ b/include/uapi/linux/ethtool_netlink_generated.h
@@ -155,6 +155,8 @@ enum {
 	ETHTOOL_A_RINGS_RX_PUSH,
 	ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN,
 	ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX,
+	ETHTOOL_A_RINGS_HDS_THRESH,
+	ETHTOOL_A_RINGS_HDS_THRESH_MAX,
 
 	__ETHTOOL_A_RINGS_CNT,
 	ETHTOOL_A_RINGS_MAX = (__ETHTOOL_A_RINGS_CNT - 1)
diff --git a/net/core/dev.c b/net/core/dev.c
index c7f3dea3e0eb..6a68db95de76 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9480,6 +9480,19 @@ u8 dev_xdp_prog_count(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_xdp_prog_count);
 
+u8 dev_xdp_sb_prog_count(struct net_device *dev)
+{
+	u8 count = 0;
+	int i;
+
+	for (i = 0; i < __MAX_XDP_MODE; i++)
+		if (dev->xdp_state[i].prog &&
+		    !dev->xdp_state[i].prog->aux->xdp_has_frags)
+			count++;
+	return count;
+}
+EXPORT_SYMBOL_GPL(dev_xdp_sb_prog_count);
+
 int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
 {
 	if (!dev->netdev_ops->ndo_bpf)
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 0a09298fff92..c523b763efa3 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -456,7 +456,7 @@ extern const struct nla_policy ethnl_features_set_policy[ETHTOOL_A_FEATURES_WANT
 extern const struct nla_policy ethnl_privflags_get_policy[ETHTOOL_A_PRIVFLAGS_HEADER + 1];
 extern const struct nla_policy ethnl_privflags_set_policy[ETHTOOL_A_PRIVFLAGS_FLAGS + 1];
 extern const struct nla_policy ethnl_rings_get_policy[ETHTOOL_A_RINGS_HEADER + 1];
-extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX + 1];
+extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_HDS_THRESH_MAX + 1];
 extern const struct nla_policy ethnl_channels_get_policy[ETHTOOL_A_CHANNELS_HEADER + 1];
 extern const struct nla_policy ethnl_channels_set_policy[ETHTOOL_A_CHANNELS_COMBINED_COUNT + 1];
 extern const struct nla_policy ethnl_coalesce_get_policy[ETHTOOL_A_COALESCE_HEADER + 1];
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 2e8239a76234..c0cb9b2c6616 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -61,7 +61,9 @@ static int rings_reply_size(const struct ethnl_req_info *req_base,
 	       nla_total_size(sizeof(u8))  +	/* _RINGS_TX_PUSH */
 	       nla_total_size(sizeof(u8))) +	/* _RINGS_RX_PUSH */
 	       nla_total_size(sizeof(u32)) +	/* _RINGS_TX_PUSH_BUF_LEN */
-	       nla_total_size(sizeof(u32));	/* _RINGS_TX_PUSH_BUF_LEN_MAX */
+	       nla_total_size(sizeof(u32)) +	/* _RINGS_TX_PUSH_BUF_LEN_MAX */
+	       nla_total_size(sizeof(u32)) +	/* _RINGS_HDS_THRESH */
+	       nla_total_size(sizeof(u32));	/* _RINGS_HDS_THRESH_MAX*/
 }
 
 static int rings_fill_reply(struct sk_buff *skb,
@@ -108,7 +110,12 @@ static int rings_fill_reply(struct sk_buff *skb,
 	     (nla_put_u32(skb, ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX,
 			  kr->tx_push_buf_max_len) ||
 	      nla_put_u32(skb, ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN,
-			  kr->tx_push_buf_len))))
+			  kr->tx_push_buf_len))) ||
+	    ((supported_ring_params & ETHTOOL_RING_USE_HDS_THRS) &&
+	     (nla_put_u32(skb, ETHTOOL_A_RINGS_HDS_THRESH,
+			  kr->hds_thresh) ||
+	      nla_put_u32(skb, ETHTOOL_A_RINGS_HDS_THRESH_MAX,
+			  kr->hds_thresh_max))))
 		return -EMSGSIZE;
 
 	return 0;
@@ -130,6 +137,7 @@ const struct nla_policy ethnl_rings_set_policy[] = {
 	[ETHTOOL_A_RINGS_TX_PUSH]		= NLA_POLICY_MAX(NLA_U8, 1),
 	[ETHTOOL_A_RINGS_RX_PUSH]		= NLA_POLICY_MAX(NLA_U8, 1),
 	[ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN]	= { .type = NLA_U32 },
+	[ETHTOOL_A_RINGS_HDS_THRESH]		= { .type = NLA_U32 },
 };
 
 static int
@@ -155,6 +163,14 @@ ethnl_set_rings_validate(struct ethnl_req_info *req_info,
 		return -EOPNOTSUPP;
 	}
 
+	if (tb[ETHTOOL_A_RINGS_HDS_THRESH] &&
+	    !(ops->supported_ring_params & ETHTOOL_RING_USE_HDS_THRS)) {
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    tb[ETHTOOL_A_RINGS_HDS_THRESH],
+				    "setting hds-thresh is not supported");
+		return -EOPNOTSUPP;
+	}
+
 	if (tb[ETHTOOL_A_RINGS_CQE_SIZE] &&
 	    !(ops->supported_ring_params & ETHTOOL_RING_USE_CQE_SIZE)) {
 		NL_SET_ERR_MSG_ATTR(info->extack,
@@ -196,14 +212,16 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 	struct kernel_ethtool_ringparam kernel_ringparam = {};
 	struct ethtool_ringparam ringparam = {};
 	struct net_device *dev = req_info->dev;
+	u8 old_hds_config, hds_config_mod;
 	struct nlattr **tb = info->attrs;
 	const struct nlattr *err_attr;
 	bool mod = false;
 	int ret;
 
+	old_hds_config = dev->ethtool->hds_config;
 	dev->ethtool_ops->get_ringparam(dev, &ringparam,
 					&kernel_ringparam, info->extack);
-	kernel_ringparam.tcp_data_split = dev->ethtool->hds_config;
+	kernel_ringparam.tcp_data_split = old_hds_config;
 
 	ethnl_update_u32(&ringparam.rx_pending, tb[ETHTOOL_A_RINGS_RX], &mod);
 	ethnl_update_u32(&ringparam.rx_mini_pending,
@@ -223,9 +241,25 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 			tb[ETHTOOL_A_RINGS_RX_PUSH], &mod);
 	ethnl_update_u32(&kernel_ringparam.tx_push_buf_len,
 			 tb[ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN], &mod);
+	ethnl_update_u32(&kernel_ringparam.hds_thresh,
+			 tb[ETHTOOL_A_RINGS_HDS_THRESH], &mod);
 	if (!mod)
 		return 0;
 
+	hds_config_mod = old_hds_config != kernel_ringparam.tcp_data_split;
+	if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	    hds_config_mod && dev_xdp_sb_prog_count(dev)) {
+		NL_SET_ERR_MSG(info->extack,
+			       "tcp-data-split can not be enabled with single buffer XDP");
+		return -EINVAL;
+	}
+
+	if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max) {
+		NL_SET_BAD_ATTR(info->extack,
+				tb[ETHTOOL_A_RINGS_HDS_THRESH_MAX]);
+		return -ERANGE;
+	}
+
 	/* ensure new ring parameters are within limits */
 	if (ringparam.rx_pending > ringparam.rx_max_pending)
 		err_attr = tb[ETHTOOL_A_RINGS_RX];
-- 
2.34.1

[PATCH net-next v6 5/9] bnxt_en: add support for hds-thresh ethtool command

[Taehee Yoo]

The bnxt_en driver has configured the hds_threshold value automatically
when TPA is enabled based on the rx-copybreak default value.
Now the hds-thresh ethtool command is added, so it adds an
implementation of hds-thresh option.

Configuration of the hds-thresh is applied only when
the tcp-data-split is enabled. The default value of
hds-thresh is 256, which is the default value of
rx-copybreak, which used to be the hds_thresh value.

The maximum hds-thresh is 1023.

   # Example:
   # ethtool -G enp14s0f0np0 tcp-data-split on hds-thresh 256
   # ethtool -g enp14s0f0np0
   Ring parameters for enp14s0f0np0:
   Pre-set maximums:
   ...
   HDS thresh:  1023
   Current hardware settings:
   ...
   TCP data split:         on
   HDS thresh:  256

Tested-by: Stanislav Fomichev <sdf@fomichev.me>
Tested-by: Andy Gospodarek <gospo@broadcom.com>
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v6:
 - HDS_MAX is changed to 1023.
 - Add Test tag from Andy.

v5:
 - No changes.

v4:
 - Reduce hole in struct bnxt.
 - Add ETHTOOL_RING_USE_HDS_THRS to indicate bnxt_en driver support
   header-data-split-thresh option.
 - Add Test tag from Stanislav.

v3:
 - Drop validation logic tcp-data-split and tcp-data-split-thresh.

v2:
 - Patch added.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 3 ++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h         | 2 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 7 ++++++-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 42ffaa88ae4e..5b16b2ef7739 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4603,6 +4603,7 @@ void bnxt_set_tpa_flags(struct bnxt *bp)
 static void bnxt_init_ring_params(struct bnxt *bp)
 {
 	bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
+	bp->hds_threshold = BNXT_DEFAULT_RX_COPYBREAK;
 }
 
 /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
@@ -6578,7 +6579,7 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
 					  VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
 		req->enables |=
 			cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
-		req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
+		req->hds_threshold = cpu_to_le16(bp->hds_threshold);
 	}
 	req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
 	return hwrm_req_send(bp, req);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 847dedf61a9e..8fc4d630ee21 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2404,6 +2404,8 @@ struct bnxt {
 	u8			q_ids[BNXT_MAX_QUEUE];
 	u8			max_q;
 	u8			num_tc;
+#define BNXT_HDS_THRESHOLD_MAX	1023
+	u16			hds_threshold;
 
 	unsigned int		current_interval;
 #define BNXT_TIMER_INTERVAL	HZ
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 25eb5931aea9..921e7e8333e8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -833,6 +833,9 @@ static void bnxt_get_ringparam(struct net_device *dev,
 	ering->rx_pending = bp->rx_ring_size;
 	ering->rx_jumbo_pending = bp->rx_agg_ring_size;
 	ering->tx_pending = bp->tx_ring_size;
+
+	kernel_ering->hds_thresh = bp->hds_threshold;
+	kernel_ering->hds_thresh_max = BNXT_HDS_THRESHOLD_MAX;
 }
 
 static int bnxt_set_ringparam(struct net_device *dev,
@@ -870,6 +873,7 @@ static int bnxt_set_ringparam(struct net_device *dev,
 			bp->flags &= ~BNXT_FLAG_HDS;
 	}
 
+	bp->hds_threshold = (u16)kernel_ering->hds_thresh;
 	bp->rx_ring_size = ering->rx_pending;
 	bp->tx_ring_size = ering->tx_pending;
 	bnxt_set_ring_params(bp);
@@ -5374,7 +5378,8 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 				     ETHTOOL_COALESCE_STATS_BLOCK_USECS |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
 				     ETHTOOL_COALESCE_USE_CQE,
-	.supported_ring_params	= ETHTOOL_RING_USE_TCP_DATA_SPLIT,
+	.supported_ring_params	= ETHTOOL_RING_USE_TCP_DATA_SPLIT |
+				  ETHTOOL_RING_USE_HDS_THRS,
 	.get_link_ksettings	= bnxt_get_link_ksettings,
 	.set_link_ksettings	= bnxt_set_link_ksettings,
 	.get_fec_stats		= bnxt_get_fec_stats,
-- 
2.34.1

[PATCH net-next v6 6/9] net: devmem: add ring parameter filtering

[Taehee Yoo]

If driver doesn't support ring parameter or tcp-data-split configuration
is not sufficient, the devmem should not be set up.
Before setup the devmem, tcp-data-split should be ON and hds-thresh
value should be 0.

Tested-by: Stanislav Fomichev <sdf@fomichev.me>
Reviewed-by: Mina Almasry <almasrymina@google.com>
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v6:
 - No changes.

v5:
 - Add Review tag from Mina.

v4:
 - Check condition before __netif_get_rx_queue().
 - Separate condition check.
 - Add Test tag from Stanislav.

v3:
 - Patch added.

 net/core/devmem.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/net/core/devmem.c b/net/core/devmem.c
index 0b6ed7525b22..a29333af3ebd 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -8,6 +8,8 @@
  */
 
 #include <linux/dma-buf.h>
+#include <linux/ethtool.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/genalloc.h>
 #include <linux/mm.h>
 #include <linux/netdevice.h>
@@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 				    struct net_devmem_dmabuf_binding *binding,
 				    struct netlink_ext_ack *extack)
 {
+	struct kernel_ethtool_ringparam kernel_ringparam = {};
+	struct ethtool_ringparam ringparam = {};
 	struct netdev_rx_queue *rxq;
 	u32 xa_idx;
 	int err;
@@ -140,6 +144,21 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 		return -ERANGE;
 	}
 
+	if (dev->ethtool->hds_config != ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
+		NL_SET_ERR_MSG(extack, "tcp-data-split is disabled");
+		return -EINVAL;
+	}
+
+	if (!dev->ethtool_ops->get_ringparam)
+		return -EOPNOTSUPP;
+
+	dev->ethtool_ops->get_ringparam(dev, &ringparam, &kernel_ringparam,
+					extack);
+	if (kernel_ringparam.hds_thresh) {
+		NL_SET_ERR_MSG(extack, "hds-thresh is not zero");
+		return -EINVAL;
+	}
+
 	rxq = __netif_get_rx_queue(dev, rxq_idx);
 	if (rxq->mp_params.mp_priv) {
 		NL_SET_ERR_MSG(extack, "designated queue already memory provider bound");
-- 
2.34.1

[PATCH net-next v6 7/9] net: ethtool: add ring parameter filtering

[Taehee Yoo]

While the devmem is running, the tcp-data-split and
hds-thresh configuration should not be changed.
If user tries to change tcp-data-split and threshold value while the
devmem is running, it fails and shows extack message.

Tested-by: Stanislav Fomichev <sdf@fomichev.me>
Reviewed-by: Mina Almasry <almasrymina@google.com>
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v6:
 - No changes.

v5:
 - Change extack message.
 - Add Review tag from Mina.

v4:
 - Check condition before __netif_get_rx_queue().
 - Separate condition check.
 - Add Test tag from Stanislav.

v3:
 - Patch added.

 net/ethtool/rings.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index c0cb9b2c6616..284ca34a3b41 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -260,6 +260,19 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 		return -ERANGE;
 	}
 
+	if (dev_get_min_mp_channel_count(dev)) {
+		if (kernel_ringparam.tcp_data_split !=
+		    ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
+			NL_SET_ERR_MSG(info->extack,
+				       "can't disable tcp-data-split while device has memory provider enabled");
+			return -EINVAL;
+		} else if (kernel_ringparam.hds_thresh) {
+			NL_SET_ERR_MSG(info->extack,
+				       "can't set non-zero hds_thresh while device is memory provider enabled");
+			return -EINVAL;
+		}
+	}
+
 	/* ensure new ring parameters are within limits */
 	if (ringparam.rx_pending > ringparam.rx_max_pending)
 		err_attr = tb[ETHTOOL_A_RINGS_RX];
-- 
2.34.1

[PATCH net-next v6 8/9] net: disallow setup single buffer XDP when tcp-data-split is enabled.

[Taehee Yoo]

When a single buffer XDP is attached, NIC should guarantee only single
page packets will be received.
tcp-data-split feature splits packets into header and payload. single
buffer XDP can't handle it properly.
So attaching single buffer XDP should be disallowed when tcp-data-split
is enabled.

Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v6:
 - Patch added.

 net/core/dev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6a68db95de76..da4a34bfb675 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -92,6 +92,7 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/skbuff.h>
 #include <linux/kthread.h>
 #include <linux/bpf.h>
@@ -9498,6 +9499,15 @@ int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
 	if (!dev->netdev_ops->ndo_bpf)
 		return -EOPNOTSUPP;
 
+	if (dev->ethtool->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	    (bpf->command == XDP_SETUP_PROG ||
+	     bpf->command == XDP_SETUP_PROG_HW) &&
+	    bpf->prog && !bpf->prog->aux->xdp_has_frags) {
+		NL_SET_ERR_MSG(bpf->extack,
+			       "unable to propagate XDP to device using tcp-data-split");
+		return -EBUSY;
+	}
+
 	if (dev_get_min_mp_channel_count(dev)) {
 		NL_SET_ERR_MSG(bpf->extack, "unable to propagate XDP to device using memory provider");
 		return -EBUSY;
@@ -9535,6 +9545,12 @@ static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
 	struct netdev_bpf xdp;
 	int err;
 
+	if (dev->ethtool->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	    prog && !prog->aux->xdp_has_frags) {
+		NL_SET_ERR_MSG(extack, "unable to install XDP to device using tcp-data-split");
+		return -EBUSY;
+	}
+
 	if (dev_get_min_mp_channel_count(dev)) {
 		NL_SET_ERR_MSG(extack, "unable to install XDP to device using memory provider");
 		return -EBUSY;
-- 
2.34.1

[PATCH net-next v6 9/9] netdevsim: add HDS feature

[Taehee Yoo]

HDS options(tcp-data-split, hds-thresh) have dendencies between other
features like XDP. Basic dependencies are checked in the core API.
netdevsim is very useful to check basic dependencies.

The default tcp-data-split mode is UNKNOWN but netdevsim driver
returns DISABLED when ethtool dumps tcp-data-split mode.
The default value of HDS threshold is 0 and the maximum value is 1024.

ethtool shows like this.

ethtool -g eni1np1
Ring parameters for eni1np1:
Pre-set maximums:
...
HDS thresh:             1024
Current hardware settings:
...
TCP data split:         off
HDS thresh:             0

ethtool -G eni1np1 tcp-data-split on hds-thresh 1024
ethtool -g eni1np1
Ring parameters for eni1np1:
Pre-set maximums:
...
HDS thresh:             1024
Current hardware settings:
...
TCP data split:         on
HDS thresh:             1024

Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v6:
 - Patch added.

 drivers/net/netdevsim/ethtool.c   | 15 ++++++++++++++-
 drivers/net/netdevsim/netdevsim.h |  4 ++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 5fe1eaef99b5..aa176f52fc3f 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -2,7 +2,6 @@
 // Copyright (c) 2020 Facebook
 
 #include <linux/debugfs.h>
-#include <linux/ethtool.h>
 #include <linux/random.h>
 
 #include "netdevsim.h"
@@ -72,6 +71,11 @@ static void nsim_get_ringparam(struct net_device *dev,
 	struct netdevsim *ns = netdev_priv(dev);
 
 	memcpy(ring, &ns->ethtool.ring, sizeof(ns->ethtool.ring));
+	memcpy(kernel_ring, &ns->ethtool.kernel_ring,
+	       sizeof(ns->ethtool.kernel_ring));
+
+	if (kernel_ring->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
+		kernel_ring->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;
 }
 
 static int nsim_set_ringparam(struct net_device *dev,
@@ -85,6 +89,9 @@ static int nsim_set_ringparam(struct net_device *dev,
 	ns->ethtool.ring.rx_jumbo_pending = ring->rx_jumbo_pending;
 	ns->ethtool.ring.rx_mini_pending = ring->rx_mini_pending;
 	ns->ethtool.ring.tx_pending = ring->tx_pending;
+	ns->ethtool.kernel_ring.tcp_data_split = kernel_ring->tcp_data_split;
+	ns->ethtool.kernel_ring.hds_thresh = kernel_ring->hds_thresh;
+
 	return 0;
 }
 
@@ -161,6 +168,8 @@ static int nsim_get_ts_info(struct net_device *dev,
 
 static const struct ethtool_ops nsim_ethtool_ops = {
 	.supported_coalesce_params	= ETHTOOL_COALESCE_ALL_PARAMS,
+	.supported_ring_params		= ETHTOOL_RING_USE_TCP_DATA_SPLIT |
+					  ETHTOOL_RING_USE_HDS_THRS,
 	.get_pause_stats	        = nsim_get_pause_stats,
 	.get_pauseparam		        = nsim_get_pauseparam,
 	.set_pauseparam		        = nsim_set_pauseparam,
@@ -182,6 +191,10 @@ static void nsim_ethtool_ring_init(struct netdevsim *ns)
 	ns->ethtool.ring.rx_jumbo_max_pending = 4096;
 	ns->ethtool.ring.rx_mini_max_pending = 4096;
 	ns->ethtool.ring.tx_max_pending = 4096;
+
+	ns->ethtool.kernel_ring.tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
+	ns->ethtool.kernel_ring.hds_thresh = 0;
+	ns->ethtool.kernel_ring.hds_thresh_max = NSIM_HDS_THRESHOLD_MAX;
 }
 
 void nsim_ethtool_init(struct netdevsim *ns)
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index bf02efa10956..6abbc627308d 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -16,6 +16,7 @@
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/ethtool.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
@@ -36,6 +37,8 @@
 #define NSIM_IPSEC_VALID		BIT(31)
 #define NSIM_UDP_TUNNEL_N_PORTS		4
 
+#define NSIM_HDS_THRESHOLD_MAX		1024
+
 struct nsim_sa {
 	struct xfrm_state *xs;
 	__be32 ipaddr[4];
@@ -87,6 +90,7 @@ struct nsim_ethtool {
 	struct nsim_ethtool_pauseparam pauseparam;
 	struct ethtool_coalesce coalesce;
 	struct ethtool_ringparam ring;
+	struct kernel_ethtool_ringparam kernel_ring;
 	struct ethtool_fecparam fec;
 };
 
-- 
2.34.1

Re: [PATCH net-next v6 1/9] bnxt_en: add support for rx-copybreak ethtool command

[Jakub Kicinski]

On Wed, 18 Dec 2024 14:45:22 +0000 Taehee Yoo wrote:
> +			if (netif_running(dev)) {
> +				bnxt_close_nic(bp, false, false);
> +				bp->rx_copybreak = rx_copybreak;
> +				bnxt_set_ring_params(bp);
> +				bnxt_open_nic(bp, false, false);

We really shouldn't allow this any more, we've been rejecting
patches which try to accept reconfiguration requests by taking
the entire NIC down, without any solid recovery if memory allocation
fails.

Let's return -EBUSY if interface is running.

Re: [PATCH net-next v6 2/9] net: ethtool: add hds_config member in ethtool_netdev_state

[Jakub Kicinski]

On Wed, 18 Dec 2024 14:45:23 +0000 Taehee Yoo wrote:
> When tcp-data-split is UNKNOWN mode, drivers arbitrarily handle it.
> For example, bnxt_en driver automatically enables if at least one of
> LRO/GRO/JUMBO is enabled.
> If tcp-data-split is UNKNOWN and LRO is enabled, a driver returns
> ENABLES of tcp-data-split, not UNKNOWN.
> So, `ethtool -g eth0` shows tcp-data-split is enabled.
> 
> The problem is in the setting situation.
> In the ethnl_set_rings(), it first calls get_ringparam() to get the
> current driver's config.
> At that moment, if driver's tcp-data-split config is UNKNOWN, it returns
> ENABLE if LRO/GRO/JUMBO is enabled.
> Then, it sets values from the user and driver's current config to
> kernel_ethtool_ringparam.
> Last it calls .set_ringparam().
> The driver, especially bnxt_en driver receives
> ETHTOOL_TCP_DATA_SPLIT_ENABLED.
> But it can't distinguish whether it is set by the user or just the
> current config.
> 
> When user updates ring parameter, the new hds_config value is updated
> and current hds_config value is stored to old_hdsconfig.
> Driver's .set_ringparam() callback can distinguish a passed
> tcp-data-split value is came from user explicitly.
> If .set_ringparam() is failed, hds_config is rollbacked immediately.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Jakub Kicinski]

On Wed, 18 Dec 2024 14:45:24 +0000 Taehee Yoo wrote:
> +	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED && hds_config_mod)
> +		return -EOPNOTSUPP;

I think ethtool ops generally return -EINVAL when param not supported.
EOPNOTSUPP means entire op is not supported (again, that's just how
ethtool ops generally work, not a kernel-wide rule).

> +	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> +	    hds_config_mod && BNXT_RX_PAGE_MODE(bp)) {

Looks like patch 4 adds this check in the core. I think adding the
check in the core can be a separate patch. If you put it before this
patch in the series this bnxt check can be removed?

I mean this chunk in the core:

+	hds_config_mod = old_hds_config != kernel_ringparam.tcp_data_split;
+	if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	    hds_config_mod && dev_xdp_sb_prog_count(dev)) {
+		NL_SET_ERR_MSG(info->extack,
+			       "tcp-data-split can not be enabled with single buffer XDP");
+		return -EINVAL;
+	}

It's currently in the hds-thresh patch but really it's unrelated 
to the threshold..

> +		NL_SET_ERR_MSG_MOD(extack, "tcp-data-split is disallowed when XDP is attached");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	if (netif_running(dev))
>  		bnxt_close_nic(bp, false, false);
>  
> +	if (hds_config_mod) {
> +		if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED)
> +			bp->flags |= BNXT_FLAG_HDS;
> +		else if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
> +			bp->flags &= ~BNXT_FLAG_HDS;
> +	}
> +
>  	bp->rx_ring_size = ering->rx_pending;
>  	bp->tx_ring_size = ering->tx_pending;
>  	bnxt_set_ring_params(bp);
> @@ -5354,6 +5374,7 @@ const struct ethtool_ops bnxt_ethtool_ops = {
>  				     ETHTOOL_COALESCE_STATS_BLOCK_USECS |
>  				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
>  				     ETHTOOL_COALESCE_USE_CQE,
> +	.supported_ring_params	= ETHTOOL_RING_USE_TCP_DATA_SPLIT,
>  	.get_link_ksettings	= bnxt_get_link_ksettings,
>  	.set_link_ksettings	= bnxt_set_link_ksettings,
>  	.get_fec_stats		= bnxt_get_fec_stats,
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> index f88b641533fc..1bfff7f29310 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> @@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
>  			    bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
>  		return -EOPNOTSUPP;
>  	}
> +	if (prog && bp->flags & BNXT_FLAG_HDS) {
> +		netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
> +		return -EOPNOTSUPP;
> +	}

And this check should also live in the core, now that core has access
to dev->ethtool->hds_config ? I think you can add this check to the
core in the same patch as the chunk referred to above.

Re: [PATCH net-next v6 4/9] net: ethtool: add support for configuring hds-thresh

[Jakub Kicinski]

On Wed, 18 Dec 2024 14:45:25 +0000 Taehee Yoo wrote:
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 4e451084d58a..4f407ce9eed1 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -78,6 +78,8 @@ enum {
>   * @cqe_size: Size of TX/RX completion queue event
>   * @tx_push_buf_len: Size of TX push buffer
>   * @tx_push_buf_max_len: Maximum allowed size of TX push buffer
> + * @hds_thresh: Threshold value of header-data-split-thresh
> + * @hds_thresh_max: Maximum allowed threshold of header-data-split-thresh

nit: s/allowed/supported/

> +u8 dev_xdp_sb_prog_count(struct net_device *dev)
> +{
> +	u8 count = 0;
> +	int i;
> +
> +	for (i = 0; i < __MAX_XDP_MODE; i++)
> +		if (dev->xdp_state[i].prog &&
> +		    !dev->xdp_state[i].prog->aux->xdp_has_frags)
> +			count++;
> +	return count;
> +}
> +EXPORT_SYMBOL_GPL(dev_xdp_sb_prog_count);

No need to export this, AFAICT, none of the callers can be built 
as a module.

> +	hds_config_mod = old_hds_config != kernel_ringparam.tcp_data_split;

Does it really matter if we modified the HDS setting for the XDP check?
Whether it was already set or the current config is asking for it to be
set having XDP SB and HDS is invalid, we can return an error.

> +	if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> +	    hds_config_mod && dev_xdp_sb_prog_count(dev)) {
> +		NL_SET_ERR_MSG(info->extack,

		NL_SET_ERR_MSG_ATTR(info->extack,
				    tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT],
				    ...

> +			       "tcp-data-split can not be enabled with single buffer XDP");
> +		return -EINVAL;
> +	}
> +
> +	if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max) {
> +		NL_SET_BAD_ATTR(info->extack,
> +				tb[ETHTOOL_A_RINGS_HDS_THRESH_MAX]);
> +		return -ERANGE;
> +	}

Can this condition not be handled by the big if "ladder" below?
I mean like this:

@@ -282,6 +276,8 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
                err_attr = tb[ETHTOOL_A_RINGS_RX_JUMBO];
        else if (ringparam.tx_pending > ringparam.tx_max_pending)
                err_attr = tb[ETHTOOL_A_RINGS_TX];
+       else if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max)
+               err_attr = tb[ETHTOOL_A_RINGS_HDS_THRESH_MAX];
        else
                err_attr = NULL;
        if (err_attr) {

>  	/* ensure new ring parameters are within limits */
>  	if (ringparam.rx_pending > ringparam.rx_max_pending)
>  		err_attr = tb[ETHTOOL_A_RINGS_RX];

Re: [PATCH net-next v6 5/9] bnxt_en: add support for hds-thresh ethtool command

[Jakub Kicinski]

On Wed, 18 Dec 2024 14:45:26 +0000 Taehee Yoo wrote:
> +#define BNXT_HDS_THRESHOLD_MAX	1023
> +	u16			hds_threshold;

How about we add hds_threshold to struct ethtool_netdev_state ?
That way in patch 6 we don't have to call the driver to get it.

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Jakub Kicinski]

On Wed, 18 Dec 2024 18:25:47 -0800 Jakub Kicinski wrote:
> > +	if (prog && bp->flags & BNXT_FLAG_HDS) {
> > +		netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
> > +		return -EOPNOTSUPP;
> > +	}  
> 
> And this check should also live in the core, now that core has access
> to dev->ethtool->hds_config ? I think you can add this check to the
> core in the same patch as the chunk referred to above.

Oh, you also already have this logic in patch 7?
So it just needs to be reordered in the series, and then the driver
doesn't need to check?

Re: [PATCH net-next v6 8/9] net: disallow setup single buffer XDP when tcp-data-split is enabled.

[Jakub Kicinski]

On Wed, 18 Dec 2024 14:45:29 +0000 Taehee Yoo wrote:
> +	     bpf->command == XDP_SETUP_PROG_HW) &&

PROG_HW will be fine, you can drop this part of the check.
HDS is a host feature, if the program is offloaded offload driver
does much more precise geometry validation already.

Re: [PATCH net-next v6 9/9] netdevsim: add HDS feature

[Jakub Kicinski]

On Wed, 18 Dec 2024 14:45:30 +0000 Taehee Yoo wrote:
>  drivers/net/netdevsim/ethtool.c   | 15 ++++++++++++++-
>  drivers/net/netdevsim/netdevsim.h |  4 ++++

netdevsim patches must come with a selftest that uses them :)
We support bash, C and Python tests. Look inside
tools/testing/selftests/drivers/net/
These tests could serve as examples: stats.py, hw/ncdevmem.c

TBH it doesn't look like netdevsim _actually_ supports HDS,
as in forwarded packets will not be split with the current
code, or linearized. You'd need to modify nsim_start_xmit()
to either linearize the packet or not based on *peer's* HDS
settings.

Re: [PATCH net-next v6 1/9] bnxt_en: add support for rx-copybreak ethtool command

[Taehee Yoo]

On Thu, Dec 19, 2024 at 11:07 AM Jakub Kicinski <kuba@kernel.org> wrote:
>

Hi Jakub,
Thanks a lot for the review!

> On Wed, 18 Dec 2024 14:45:22 +0000 Taehee Yoo wrote:
> > +                     if (netif_running(dev)) {
> > +                             bnxt_close_nic(bp, false, false);
> > +                             bp->rx_copybreak = rx_copybreak;
> > +                             bnxt_set_ring_params(bp);
> > +                             bnxt_open_nic(bp, false, false);
>
> We really shouldn't allow this any more, we've been rejecting
> patches which try to accept reconfiguration requests by taking
> the entire NIC down, without any solid recovery if memory allocation
> fails.
>
> Let's return -EBUSY if interface is running.

Okay, I will change it.

Thanks a lot,
Taehee Yoo

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Taehee Yoo]

On Thu, Dec 19, 2024 at 11:25 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 18 Dec 2024 14:45:24 +0000 Taehee Yoo wrote:
> > +     if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED && hds_config_mod)
> > +             return -EOPNOTSUPP;
>
> I think ethtool ops generally return -EINVAL when param not supported.
> EOPNOTSUPP means entire op is not supported (again, that's just how
> ethtool ops generally work, not a kernel-wide rule).

Thanks! I will use -EINVAL instead of EOPNOTSUPP.

>
> > +     if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> > +         hds_config_mod && BNXT_RX_PAGE_MODE(bp)) {
>
> Looks like patch 4 adds this check in the core. I think adding the
> check in the core can be a separate patch. If you put it before this
> patch in the series this bnxt check can be removed?
>
> I mean this chunk in the core:
>
> +       hds_config_mod = old_hds_config != kernel_ringparam.tcp_data_split;
> +       if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> +           hds_config_mod && dev_xdp_sb_prog_count(dev)) {
> +               NL_SET_ERR_MSG(info->extack,
> +                              "tcp-data-split can not be enabled with single buffer XDP");
> +               return -EINVAL;
> +       }
>

Right, The core checks single buffer XDP.
But there was a review that bnxt_en driver doesn't support both
single and multi buffer XDP if HDS is in use.
So, this is the reason why logic exists.

> It's currently in the hds-thresh patch but really it's unrelated
> to the threshold..

Thanks, I will move this code to a HDS related patch, not hds-threshold,
or create new patch for this.

>
> > +             NL_SET_ERR_MSG_MOD(extack, "tcp-data-split is disallowed when XDP is attached");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> >       if (netif_running(dev))
> >               bnxt_close_nic(bp, false, false);
> >
> > +     if (hds_config_mod) {
> > +             if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED)
> > +                     bp->flags |= BNXT_FLAG_HDS;
> > +             else if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
> > +                     bp->flags &= ~BNXT_FLAG_HDS;
> > +     }
> > +
> >       bp->rx_ring_size = ering->rx_pending;
> >       bp->tx_ring_size = ering->tx_pending;
> >       bnxt_set_ring_params(bp);
> > @@ -5354,6 +5374,7 @@ const struct ethtool_ops bnxt_ethtool_ops = {
> >                                    ETHTOOL_COALESCE_STATS_BLOCK_USECS |
> >                                    ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
> >                                    ETHTOOL_COALESCE_USE_CQE,
> > +     .supported_ring_params  = ETHTOOL_RING_USE_TCP_DATA_SPLIT,
> >       .get_link_ksettings     = bnxt_get_link_ksettings,
> >       .set_link_ksettings     = bnxt_set_link_ksettings,
> >       .get_fec_stats          = bnxt_get_fec_stats,
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> > index f88b641533fc..1bfff7f29310 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> > @@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
> >                           bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
> >               return -EOPNOTSUPP;
> >       }
> > +     if (prog && bp->flags & BNXT_FLAG_HDS) {
> > +             netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
> > +             return -EOPNOTSUPP;
> > +     }
>
> And this check should also live in the core, now that core has access
> to dev->ethtool->hds_config ? I think you can add this check to the
> core in the same patch as the chunk referred to above.

The bnxt_en disallows setting up both single and multi buffer XDP, but core
checks only single buffer XDP. So, if multi buffer XDP is attaching to
the bnxt_en driver when HDS is enabled, the core can't filter it.

Thanks a lot!
Taehee Yoo

Re: [PATCH net-next v6 4/9] net: ethtool: add support for configuring hds-thresh

[Taehee Yoo]

On Thu, Dec 19, 2024 at 11:35 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 18 Dec 2024 14:45:25 +0000 Taehee Yoo wrote:
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index 4e451084d58a..4f407ce9eed1 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -78,6 +78,8 @@ enum {
> >   * @cqe_size: Size of TX/RX completion queue event
> >   * @tx_push_buf_len: Size of TX push buffer
> >   * @tx_push_buf_max_len: Maximum allowed size of TX push buffer
> > + * @hds_thresh: Threshold value of header-data-split-thresh
> > + * @hds_thresh_max: Maximum allowed threshold of header-data-split-thresh
>
> nit: s/allowed/supported/

Thanks, I will change it.

>
> > +u8 dev_xdp_sb_prog_count(struct net_device *dev)
> > +{
> > +     u8 count = 0;
> > +     int i;
> > +
> > +     for (i = 0; i < __MAX_XDP_MODE; i++)
> > +             if (dev->xdp_state[i].prog &&
> > +                 !dev->xdp_state[i].prog->aux->xdp_has_frags)
> > +                     count++;
> > +     return count;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_xdp_sb_prog_count);
>
> No need to export this, AFAICT, none of the callers can be built
> as a module.

Okay, I will not export this function.

>
> > +     hds_config_mod = old_hds_config != kernel_ringparam.tcp_data_split;
>
> Does it really matter if we modified the HDS setting for the XDP check?
> Whether it was already set or the current config is asking for it to be
> set having XDP SB and HDS is invalid, we can return an error.

Right, it doesn't need to check modification.
I will remove it.

>
> > +     if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> > +         hds_config_mod && dev_xdp_sb_prog_count(dev)) {
> > +             NL_SET_ERR_MSG(info->extack,
>
>                 NL_SET_ERR_MSG_ATTR(info->extack,

Thanks for it too, I will use it.

>                                     tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT],
>                                     ...
>
> > +                            "tcp-data-split can not be enabled with single buffer XDP");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max) {
> > +             NL_SET_BAD_ATTR(info->extack,
> > +                             tb[ETHTOOL_A_RINGS_HDS_THRESH_MAX]);
> > +             return -ERANGE;
> > +     }
>
> Can this condition not be handled by the big if "ladder" below?
> I mean like this:

Thanks for that, I will try to apply it!

>
> @@ -282,6 +276,8 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
>                 err_attr = tb[ETHTOOL_A_RINGS_RX_JUMBO];
>         else if (ringparam.tx_pending > ringparam.tx_max_pending)
>                 err_attr = tb[ETHTOOL_A_RINGS_TX];
> +       else if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max)
> +               err_attr = tb[ETHTOOL_A_RINGS_HDS_THRESH_MAX];
>         else
>                 err_attr = NULL;
>         if (err_attr) {
>
> >       /* ensure new ring parameters are within limits */
> >       if (ringparam.rx_pending > ringparam.rx_max_pending)
> >               err_attr = tb[ETHTOOL_A_RINGS_RX];

Thanks a lot!
Taehee Yoo

Re: [PATCH net-next v6 5/9] bnxt_en: add support for hds-thresh ethtool command

[Taehee Yoo]

On Thu, Dec 19, 2024 at 11:40 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 18 Dec 2024 14:45:26 +0000 Taehee Yoo wrote:
> > +#define BNXT_HDS_THRESHOLD_MAX       1023
> > +     u16                     hds_threshold;
>
> How about we add hds_threshold to struct ethtool_netdev_state ?
> That way in patch 6 we don't have to call the driver to get it.

I agree, it looks much better! I will change it.

Thanks a lot!
Taehee Yoo

Re: [PATCH net-next v6 8/9] net: disallow setup single buffer XDP when tcp-data-split is enabled.

[Taehee Yoo]

On Thu, Dec 19, 2024 at 11:44 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 18 Dec 2024 14:45:29 +0000 Taehee Yoo wrote:
> > +          bpf->command == XDP_SETUP_PROG_HW) &&
>
> PROG_HW will be fine, you can drop this part of the check.
> HDS is a host feature, if the program is offloaded offload driver
> does much more precise geometry validation already.

Thanks, I will remove this condition.

Thanks a lot!
Taehee Yoo

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Jakub Kicinski]

On Thu, 19 Dec 2024 23:05:30 +0900 Taehee Yoo wrote:
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> > > index f88b641533fc..1bfff7f29310 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> > > @@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
> > >                           bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
> > >               return -EOPNOTSUPP;
> > >       }
> > > +     if (prog && bp->flags & BNXT_FLAG_HDS) {
> > > +             netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
> > > +             return -EOPNOTSUPP;
> > > +     }  
> >
> > And this check should also live in the core, now that core has access
> > to dev->ethtool->hds_config ? I think you can add this check to the
> > core in the same patch as the chunk referred to above.  
> 
> The bnxt_en disallows setting up both single and multi buffer XDP, but core
> checks only single buffer XDP. So, if multi buffer XDP is attaching to
> the bnxt_en driver when HDS is enabled, the core can't filter it.

Hm. Did you find this in the code, or did Broadcom folks suggest it?
AFAICT bnxt supports multi-buf XDP. Is there something in the code 
that special-cases aggregation but doesn't work for pure HDS?

Re: [PATCH net-next v6 9/9] netdevsim: add HDS feature

[Taehee Yoo]

On Thu, Dec 19, 2024 at 11:49 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 18 Dec 2024 14:45:30 +0000 Taehee Yoo wrote:
> >  drivers/net/netdevsim/ethtool.c   | 15 ++++++++++++++-
> >  drivers/net/netdevsim/netdevsim.h |  4 ++++
>
> netdevsim patches must come with a selftest that uses them :)
> We support bash, C and Python tests. Look inside
> tools/testing/selftests/drivers/net/
> These tests could serve as examples: stats.py, hw/ncdevmem.c
>


> TBH it doesn't look like netdevsim _actually_ supports HDS,
> as in forwarded packets will not be split with the current
> code, or linearized. You'd need to modify nsim_start_xmit()
> to either linearize the packet or not based on *peer's* HDS
> settings.

Ah, Sorry for the sloppy change.
I will write a selftest. Also, I will modify nsim_start_xmit() to support HDS.
The example would be very helpful to me.

Thank you so much!
Taehee Yoo.

Re: [PATCH net-next v6 9/9] netdevsim: add HDS feature

[Jakub Kicinski]

On Thu, 19 Dec 2024 23:37:45 +0900 Taehee Yoo wrote:
> The example would be very helpful to me.

Just to make sure nothing gets lost in translation, are you saying that:
 - the examples of tests I listed are useful; or
 - you'd appreciate examples of how to code up HDS in netdevsim; or
 - you'd appreciate more suitable examples of the tests?

:)

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Taehee Yoo]

On Thu, Dec 19, 2024 at 11:29 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 19 Dec 2024 23:05:30 +0900 Taehee Yoo wrote:
> > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> > > > index f88b641533fc..1bfff7f29310 100644
> > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> > > > @@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
> > > >                           bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
> > > >               return -EOPNOTSUPP;
> > > >       }
> > > > +     if (prog && bp->flags & BNXT_FLAG_HDS) {
> > > > +             netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
> > > > +             return -EOPNOTSUPP;
> > > > +     }
> > >
> > > And this check should also live in the core, now that core has access
> > > to dev->ethtool->hds_config ? I think you can add this check to the
> > > core in the same patch as the chunk referred to above.
> >
> > The bnxt_en disallows setting up both single and multi buffer XDP, but core
> > checks only single buffer XDP. So, if multi buffer XDP is attaching to
> > the bnxt_en driver when HDS is enabled, the core can't filter it.
>
> Hm. Did you find this in the code, or did Broadcom folks suggest it?
> AFAICT bnxt supports multi-buf XDP. Is there something in the code
> that special-cases aggregation but doesn't work for pure HDS?

There were some comments about HDS with XDP in the following thread.
https://lore.kernel.org/netdev/20241022162359.2713094-1-ap420073@gmail.com/T/
I may misunderstand reviews from Broadcom folks.

Re: [PATCH net-next v6 9/9] netdevsim: add HDS feature

[Taehee Yoo]

On Thu, Dec 19, 2024 at 11:45 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 19 Dec 2024 23:37:45 +0900 Taehee Yoo wrote:
> > The example would be very helpful to me.
>
> Just to make sure nothing gets lost in translation, are you saying that:
>  - the examples of tests I listed are useful; or
>  - you'd appreciate examples of how to code up HDS in netdevsim; or
>  - you'd appreciate more suitable examples of the tests?
>
> :)

Ah, I appreciate example of tests you listed are useful, Thanks!

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Jakub Kicinski]

On Fri, 20 Dec 2024 00:14:01 +0900 Taehee Yoo wrote:
> > > The bnxt_en disallows setting up both single and multi buffer XDP, but core
> > > checks only single buffer XDP. So, if multi buffer XDP is attaching to
> > > the bnxt_en driver when HDS is enabled, the core can't filter it.  
> >
> > Hm. Did you find this in the code, or did Broadcom folks suggest it?
> > AFAICT bnxt supports multi-buf XDP. Is there something in the code
> > that special-cases aggregation but doesn't work for pure HDS?  
> 
> There were some comments about HDS with XDP in the following thread.
> https://lore.kernel.org/netdev/20241022162359.2713094-1-ap420073@gmail.com/T/
> I may misunderstand reviews from Broadcom folks.

I see it now in bnxt_set_rx_skb_mode. I guess with high MTU
the device splits in some "dumb" way, at a fixed offset..
You're right, we have to keep the check in the driver, 
at least for now.

Re: [PATCH net-next v6 9/9] netdevsim: add HDS feature

[Jakub Kicinski]

On Fri, 20 Dec 2024 00:19:38 +0900 Taehee Yoo wrote:
> > On Thu, 19 Dec 2024 23:37:45 +0900 Taehee Yoo wrote:  
> > > The example would be very helpful to me.  
> >
> > Just to make sure nothing gets lost in translation, are you saying that:
> >  - the examples of tests I listed are useful; or
> >  - you'd appreciate examples of how to code up HDS in netdevsim; or
> >  - you'd appreciate more suitable examples of the tests?
> >
> > :)  
> 
> Ah, I appreciate example of tests you listed are useful, Thanks!

Okay :) 

FWIW I don't expect that you'd do anything too complicated to support
HDS in netdevsim. The packets generated by the networking stack are
"split", already. What I was thinking is basically check if HDS is
enabled, compare skb->len to threshold, and if we shouldn't HDS call
skb_linearize().

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Andy Gospodarek]

On Thu, Dec 19, 2024 at 07:25:19AM -0800, Jakub Kicinski wrote:
> On Fri, 20 Dec 2024 00:14:01 +0900 Taehee Yoo wrote:
> > > > The bnxt_en disallows setting up both single and multi buffer XDP, but core
> > > > checks only single buffer XDP. So, if multi buffer XDP is attaching to
> > > > the bnxt_en driver when HDS is enabled, the core can't filter it.  
> > >
> > > Hm. Did you find this in the code, or did Broadcom folks suggest it?
> > > AFAICT bnxt supports multi-buf XDP. Is there something in the code
> > > that special-cases aggregation but doesn't work for pure HDS?  
> > 
> > There were some comments about HDS with XDP in the following thread.
> > https://lore.kernel.org/netdev/20241022162359.2713094-1-ap420073@gmail.com/T/
> > I may misunderstand reviews from Broadcom folks.
> 
> I see it now in bnxt_set_rx_skb_mode. I guess with high MTU
> the device splits in some "dumb" way, at a fixed offset..
> You're right, we have to keep the check in the driver, 
> at least for now.

The mutlti-buffer implementation followed what was done at the time in
other drivers.  Is the 'dumb way' you mention this check?

 4717                 if (dev->mtu > BNXT_MAX_PAGE_MODE_MTU) {
 4718                         bp->flags |= BNXT_FLAG_JUMBO;
 4719                         bp->rx_skb_func = bnxt_rx_multi_page_skb;
 4720                 } else {
 4721                         bp->flags |= BNXT_FLAG_NO_AGG_RINGS;
 4722                         bp->rx_skb_func = bnxt_rx_page_skb;
 4723                 }

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Jakub Kicinski]

On Thu, 19 Dec 2024 14:33:44 -0500 Andy Gospodarek wrote:
> > I see it now in bnxt_set_rx_skb_mode. I guess with high MTU
> > the device splits in some "dumb" way, at a fixed offset..
> > You're right, we have to keep the check in the driver, 
> > at least for now.  
> 
> The mutlti-buffer implementation followed what was done at the time in
> other drivers.  Is the 'dumb way' you mention this check?
> 
>  4717                 if (dev->mtu > BNXT_MAX_PAGE_MODE_MTU) {
>  4718                         bp->flags |= BNXT_FLAG_JUMBO;
>  4719                         bp->rx_skb_func = bnxt_rx_multi_page_skb;
>  4720                 } else {
>  4721                         bp->flags |= BNXT_FLAG_NO_AGG_RINGS;
>  4722                         bp->rx_skb_func = bnxt_rx_page_skb;
>  4723                 }

Yes, that and my interpretation of the previous discussion let me to
believe that the BNXT_FLAG_JUMBO does not enable header-data split.
And speculating further I thought that perhaps the buffer split with
jumbo > 4k is to fill first buffer completely, header+however much
data fits.

I could have misread the previous conversation (perhaps Michael meant
XDP SB / PAGE_MODE when he was referring to XDP limitations?)

Or maybe the HDS does happen with XDP MB but there is another
limitation in the code?

I'm not sure. At this stage we just need to know if the check in the
driver is really needed or XDP MB + HDS are fine, and we can remove
the driver check, as core already prevents XDP SB + HDS. Could you
clarify?

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Michael Chan]

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

On Thu, Dec 19, 2024 at 12:18 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 19 Dec 2024 14:33:44 -0500 Andy Gospodarek wrote:
> > > I see it now in bnxt_set_rx_skb_mode. I guess with high MTU
> > > the device splits in some "dumb" way, at a fixed offset..
> > > You're right, we have to keep the check in the driver,
> > > at least for now.
> >
> > The mutlti-buffer implementation followed what was done at the time in
> > other drivers.  Is the 'dumb way' you mention this check?
> >
> >  4717                 if (dev->mtu > BNXT_MAX_PAGE_MODE_MTU) {
> >  4718                         bp->flags |= BNXT_FLAG_JUMBO;
> >  4719                         bp->rx_skb_func = bnxt_rx_multi_page_skb;
> >  4720                 } else {
> >  4721                         bp->flags |= BNXT_FLAG_NO_AGG_RINGS;
> >  4722                         bp->rx_skb_func = bnxt_rx_page_skb;
> >  4723                 }
>
> Yes, that and my interpretation of the previous discussion let me to
> believe that the BNXT_FLAG_JUMBO does not enable header-data split.
> And speculating further I thought that perhaps the buffer split with
> jumbo > 4k is to fill first buffer completely, header+however much
> data fits.
>
> I could have misread the previous conversation (perhaps Michael meant
> XDP SB / PAGE_MODE when he was referring to XDP limitations?)

To clarify, my review comment applied to XDP SB and MB modes.  Andy's
MB implementation from 2022 disables HWGRO/LRO and HDS in XDP MB mode.
My comment was to preserve this implementation.

>
> Or maybe the HDS does happen with XDP MB but there is another
> limitation in the code?

HW doesn't know whether we're in XDP mode or not and can definitely do
HDS.  But again, HDS is disabled currently in any XDP mode.  Andy will
respond to discuss this further.  Long term, we may be able to enable
HDS in XDP MB mode, but for now I think we should disable it just to
keep it unchanged.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next v6 3/9] bnxt_en: add support for tcp-data-split ethtool command

[Jakub Kicinski]

On Thu, 19 Dec 2024 15:41:17 -0800 Michael Chan wrote:
> > Or maybe the HDS does happen with XDP MB but there is another
> > limitation in the code?  
> 
> HW doesn't know whether we're in XDP mode or not and can definitely do
> HDS.  But again, HDS is disabled currently in any XDP mode.  Andy will
> respond to discuss this further.  Long term, we may be able to enable
> HDS in XDP MB mode, but for now I think we should disable it just to
> keep it unchanged.

SGTM, this series accomplishes enough things already :)