[PATCH v4 net-next 0/7] ethtool: track custom RSS contexts in the core

[PATCH v4 net-next 0/7] ethtool: track custom RSS contexts in the core

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Make the core responsible for tracking the set of custom RSS contexts,
 their IDs, indirection tables, hash keys, and hash functions; this
 lets us get rid of duplicative code in drivers, and will allow us to
 support netlink dumps later.

This series only moves the sfc EF10 & EF100 driver over to the new API; if
 the design is approved of, I plan to post a follow-up series to convert the
 other drivers (mvpp2, octeontx2, mlx5, sfc/siena) and remove the legacy API.
However, I don't have hardware for the drivers besides sfc, so I won't be
 able to test those myself.  Maintainers of those drivers (on CC), your
 comments on the API design would be appreciated, in particular whether the
 rss_ctx_max_id limit (corresponding to your fixed-size arrays of contexts)
 is actually needed.

Changes in v4:
* replaced IDR with XArray
* grouped initialisations together in patch 6
* dropped RFC tags

Edward Cree (7):
  net: move ethtool-related netdev state into its own struct
  net: ethtool: attach an XArray of custom RSS contexts to a netdevice
  net: ethtool: record custom RSS contexts in the XArray
  net: ethtool: let the core choose RSS context IDs
  net: ethtool: add an extack parameter to new rxfh_context APIs
  net: ethtool: add a mutex protecting RSS contexts
  sfc: use new rxfh_context API

 drivers/net/ethernet/realtek/r8169_main.c     |   4 +-
 drivers/net/ethernet/sfc/ef10.c               |   2 +-
 drivers/net/ethernet/sfc/ef100_ethtool.c      |   5 +-
 drivers/net/ethernet/sfc/efx.c                |   2 +-
 drivers/net/ethernet/sfc/efx.h                |   2 +-
 drivers/net/ethernet/sfc/efx_common.c         |  10 +-
 drivers/net/ethernet/sfc/ethtool.c            |   5 +-
 drivers/net/ethernet/sfc/ethtool_common.c     | 147 ++++++++++--------
 drivers/net/ethernet/sfc/ethtool_common.h     |  18 ++-
 drivers/net/ethernet/sfc/mcdi_filters.c       | 135 ++++++++--------
 drivers/net/ethernet/sfc/mcdi_filters.h       |   8 +-
 drivers/net/ethernet/sfc/net_driver.h         |  28 ++--
 drivers/net/ethernet/sfc/rx_common.c          |  64 ++------
 drivers/net/ethernet/sfc/rx_common.h          |   8 +-
 .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c  |   4 +-
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |   2 +-
 drivers/net/phy/phy.c                         |   2 +-
 drivers/net/phy/phy_device.c                  |   5 +-
 drivers/net/phy/phylink.c                     |   2 +-
 include/linux/ethtool.h                       | 109 ++++++++++++-
 include/linux/netdevice.h                     |   7 +-
 net/core/dev.c                                |  42 +++++
 net/ethtool/ioctl.c                           | 125 ++++++++++++++-
 net/ethtool/wol.c                             |   2 +-
 24 files changed, 494 insertions(+), 244 deletions(-)

[PATCH v4 net-next 1/7] net: move ethtool-related netdev state into its own struct

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

net_dev->ethtool is a pointer to new struct ethtool_netdev_state, which
 currently contains only the wol_enabled field.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c        | 4 ++--
 drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c | 4 ++--
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c    | 2 +-
 drivers/net/phy/phy.c                            | 2 +-
 drivers/net/phy/phy_device.c                     | 5 +++--
 drivers/net/phy/phylink.c                        | 2 +-
 include/linux/ethtool.h                          | 8 ++++++++
 include/linux/netdevice.h                        | 7 ++++---
 net/core/dev.c                                   | 4 ++++
 net/ethtool/ioctl.c                              | 2 +-
 net/ethtool/wol.c                                | 2 +-
 11 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 6351a2dc13bc..fe69416f2a93 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -1455,7 +1455,7 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)
 
 	if (tp->dash_type == RTL_DASH_NONE) {
 		rtl_set_d3_pll_down(tp, !wolopts);
-		tp->dev->wol_enabled = wolopts ? 1 : 0;
+		tp->dev->ethtool->wol_enabled = wolopts ? 1 : 0;
 	}
 }
 
@@ -5321,7 +5321,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		rtl_set_d3_pll_down(tp, true);
 	} else {
 		rtl_set_d3_pll_down(tp, false);
-		dev->wol_enabled = 1;
+		dev->ethtool->wol_enabled = 1;
 	}
 
 	jumbo_max = rtl_jumbo_max(tp);
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
index ec0e869e9aac..091ee3d3e74d 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
@@ -34,9 +34,9 @@ static int ngbe_set_wol(struct net_device *netdev,
 	wx->wol = 0;
 	if (wol->wolopts & WAKE_MAGIC)
 		wx->wol = WX_PSR_WKUP_CTL_MAG;
-	netdev->wol_enabled = !!(wx->wol);
+	netdev->ethtool->wol_enabled = !!(wx->wol);
 	wr32(wx, WX_PSR_WKUP_CTL, wx->wol);
-	device_set_wakeup_enable(&pdev->dev, netdev->wol_enabled);
+	device_set_wakeup_enable(&pdev->dev, netdev->ethtool->wol_enabled);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index 2b431db6085a..6752f8d04d9c 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -636,7 +636,7 @@ static int ngbe_probe(struct pci_dev *pdev,
 	if (wx->wol_hw_supported)
 		wx->wol = NGBE_PSR_WKUP_CTL_MAG;
 
-	netdev->wol_enabled = !!(wx->wol);
+	netdev->ethtool->wol_enabled = !!(wx->wol);
 	wr32(wx, NGBE_PSR_WKUP_CTL, wx->wol);
 	device_set_wakeup_enable(&pdev->dev, wx->wol);
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index a5fa077650e8..32bfea9b5c6c 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1282,7 +1282,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 		if (netdev) {
 			struct device *parent = netdev->dev.parent;
 
-			if (netdev->wol_enabled)
+			if (netdev->ethtool->wol_enabled)
 				pm_system_wakeup();
 			else if (device_may_wakeup(&netdev->dev))
 				pm_wakeup_dev_event(&netdev->dev, 0, true);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2ce74593d6e4..62afc0424fbd 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -285,7 +285,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	if (!netdev)
 		goto out;
 
-	if (netdev->wol_enabled)
+	if (netdev->ethtool->wol_enabled)
 		return false;
 
 	/* As long as not all affected network drivers support the
@@ -1859,7 +1859,8 @@ int phy_suspend(struct phy_device *phydev)
 		return 0;
 
 	phy_ethtool_get_wol(phydev, &wol);
-	phydev->wol_enabled = wol.wolopts || (netdev && netdev->wol_enabled);
+	phydev->wol_enabled = wol.wolopts ||
+			      (netdev && netdev->ethtool->wol_enabled);
 	/* If the device has WOL enabled, we cannot suspend the PHY */
 	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
 		return -EBUSY;
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0d7354955d62..b808ba1197c3 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2172,7 +2172,7 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
 {
 	ASSERT_RTNL();
 
-	if (mac_wol && (!pl->netdev || pl->netdev->wol_enabled)) {
+	if (mac_wol && (!pl->netdev || pl->netdev->ethtool->wol_enabled)) {
 		/* Wake-on-Lan enabled, MAC handling */
 		mutex_lock(&pl->state_mutex);
 
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 62b61527bcc4..8aeefc0b4e10 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -935,6 +935,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
 				       const struct ethtool_link_ksettings *cmd,
 				       u32 *dev_speed, u8 *dev_duplex);
 
+/**
+ * struct ethtool_netdev_state - per-netdevice state for ethtool features
+ * @wol_enabled:	Wake-on-LAN is enabled
+ */
+struct ethtool_netdev_state {
+	unsigned		wol_enabled:1;
+};
+
 struct phy_device;
 struct phy_tdr_config;
 struct phy_plca_cfg;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7e520c14eb8c..05ea6cb56800 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -79,6 +79,7 @@ struct xdp_buff;
 struct xdp_frame;
 struct xdp_metadata_ops;
 struct xdp_md;
+struct ethtool_netdev_state;
 /* DPLL specific */
 struct dpll_pin;
 
@@ -2014,8 +2015,6 @@ enum netdev_ml_priv_type {
  *			switch driver and used to set the phys state of the
  *			switch port.
  *
- *	@wol_enabled:	Wake-on-LAN is enabled
- *
  *	@threaded:	napi threaded mode is enabled
  *
  *	@net_notifier_list:	List of per-net netdev notifier block
@@ -2027,6 +2026,7 @@ enum netdev_ml_priv_type {
  *	@udp_tunnel_nic_info:	static structure describing the UDP tunnel
  *				offload capabilities of the device
  *	@udp_tunnel_nic:	UDP tunnel offload state
+ *	@ethtool:	ethtool related state
  *	@xdp_state:		stores info on attached XDP BPF programs
  *
  *	@nested_level:	Used as a parameter of spin_lock_nested() of
@@ -2389,7 +2389,6 @@ struct net_device {
 	struct sfp_bus		*sfp_bus;
 	struct lock_class_key	*qdisc_tx_busylock;
 	bool			proto_down;
-	unsigned		wol_enabled:1;
 	unsigned		threaded:1;
 
 	struct list_head	net_notifier_list;
@@ -2401,6 +2400,8 @@ struct net_device {
 	const struct udp_tunnel_nic_info	*udp_tunnel_nic_info;
 	struct udp_tunnel_nic	*udp_tunnel_nic;
 
+	struct ethtool_netdev_state *ethtool;
+
 	/* protected by rtnl_lock */
 	struct bpf_xdp_entity	xdp_state[__MAX_XDP_MODE];
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 606a366cc209..9e85a71e33ed 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10769,6 +10769,9 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	dev->real_num_rx_queues = rxqs;
 	if (netif_alloc_rx_queues(dev))
 		goto free_all;
+	dev->ethtool = kzalloc(sizeof(*dev->ethtool), GFP_KERNEL_ACCOUNT);
+	if (!dev->ethtool)
+		goto free_all;
 
 	strcpy(dev->name, name);
 	dev->name_assign_type = name_assign_type;
@@ -10819,6 +10822,7 @@ void free_netdev(struct net_device *dev)
 		return;
 	}
 
+	kfree(dev->ethtool);
 	netif_free_tx_queues(dev);
 	netif_free_rx_queues(dev);
 
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 0b0ce4f81c01..de78b24fffc9 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1461,7 +1461,7 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
 	if (ret)
 		return ret;
 
-	dev->wol_enabled = !!wol.wolopts;
+	dev->ethtool->wol_enabled = !!wol.wolopts;
 	ethtool_notify(dev, ETHTOOL_MSG_WOL_NTF, NULL);
 
 	return 0;
diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
index 0ed56c9ac1bc..a39d8000d808 100644
--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -137,7 +137,7 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info)
 	ret = dev->ethtool_ops->set_wol(dev, &wol);
 	if (ret)
 		return ret;
-	dev->wol_enabled = !!wol.wolopts;
+	dev->ethtool->wol_enabled = !!wol.wolopts;
 	return 1;
 }
 

[PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Each context stores the RXFH settings (indir, key, and hfunc) as well
 as optionally some driver private data.
Delete any still-existing contexts at netdev unregister time.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 include/linux/ethtool.h | 43 ++++++++++++++++++++++++++++++++++++++++-
 net/core/dev.c          | 25 ++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 8aeefc0b4e10..bb11cb2f477d 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -157,6 +157,43 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
 	return index % n_rx_rings;
 }
 
+/**
+ * struct ethtool_rxfh_context - a custom RSS context configuration
+ * @indir_size: Number of u32 entries in indirection table
+ * @key_size: Size of hash key, in bytes
+ * @hfunc: RSS hash function identifier.  One of the %ETH_RSS_HASH_*
+ * @priv_size: Size of driver private data, in bytes
+ * @indir_no_change: indir was not specified at create time
+ * @key_no_change: hkey was not specified at create time
+ */
+struct ethtool_rxfh_context {
+	u32 indir_size;
+	u32 key_size;
+	u8 hfunc;
+	u16 priv_size;
+	u8 indir_no_change:1;
+	u8 key_no_change:1;
+	/* private: driver private data, indirection table, and hash key are
+	 * stored sequentially in @data area.  Use below helpers to access.
+	 */
+	u8 data[] __aligned(sizeof(void *));
+};
+
+static inline void *ethtool_rxfh_context_priv(struct ethtool_rxfh_context *ctx)
+{
+	return ctx->data;
+}
+
+static inline u32 *ethtool_rxfh_context_indir(struct ethtool_rxfh_context *ctx)
+{
+	return (u32 *)(ctx->data + ALIGN(ctx->priv_size, sizeof(u32)));
+}
+
+static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
+{
+	return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size);
+}
+
 /* declare a link mode bitmap */
 #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
 	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
@@ -937,10 +974,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
 
 /**
  * struct ethtool_netdev_state - per-netdevice state for ethtool features
+ * @rss_ctx:		XArray of custom RSS contexts
+ * @rss_ctx_max_id:	maximum (exclusive) supported RSS context ID
  * @wol_enabled:	Wake-on-LAN is enabled
  */
 struct ethtool_netdev_state {
-	unsigned		wol_enabled:1;
+	struct xarray		rss_ctx;
+	u32			rss_ctx_max_id;
+	u32			wol_enabled:1;
 };
 
 struct phy_device;
diff --git a/net/core/dev.c b/net/core/dev.c
index 9e85a71e33ed..05e95abdfd17 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10072,6 +10072,9 @@ int register_netdevice(struct net_device *dev)
 	if (ret)
 		return ret;
 
+	/* rss ctx ID 0 is reserved for the default context, start from 1 */
+	xa_init_flags(&dev->ethtool->rss_ctx, XA_FLAGS_ALLOC1);
+
 	spin_lock_init(&dev->addr_list_lock);
 	netdev_set_addr_lockdep_class(dev);
 
@@ -10874,6 +10877,26 @@ void synchronize_net(void)
 }
 EXPORT_SYMBOL(synchronize_net);
 
+static void netdev_rss_contexts_free(struct net_device *dev)
+{
+	struct ethtool_rxfh_context *ctx;
+	unsigned long context;
+
+	if (dev->ethtool_ops->set_rxfh_context)
+		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
+			u32 *indir = ethtool_rxfh_context_indir(ctx);
+			u8 *key = ethtool_rxfh_context_key(ctx);
+			u32 concast = context;
+
+			xa_erase(&dev->ethtool->rss_ctx, context);
+			dev->ethtool_ops->set_rxfh_context(dev, indir, key,
+							   ctx->hfunc, &concast,
+							   true);
+			kfree(ctx);
+		}
+	xa_destroy(&dev->ethtool->rss_ctx);
+}
+
 /**
  *	unregister_netdevice_queue - remove device from the kernel
  *	@dev: device
@@ -10978,6 +11001,8 @@ void unregister_netdevice_many_notify(struct list_head *head,
 		netdev_name_node_alt_flush(dev);
 		netdev_name_node_free(dev->name_node);
 
+		netdev_rss_contexts_free(dev);
+
 		call_netdevice_notifiers(NETDEV_PRE_UNINIT, dev);
 
 		if (dev->netdev_ops->ndo_uninit)

[PATCH v4 net-next 3/7] net: ethtool: record custom RSS contexts in the XArray

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Since drivers are still choosing the context IDs, we have to force the
 XArray to use the ID they've chosen rather than picking one ourselves,
 and handle the case where they give us an ID that's already in use.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 include/linux/ethtool.h | 14 ++++++++
 net/ethtool/ioctl.c     | 73 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index bb11cb2f477d..229a23571008 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -194,6 +194,17 @@ static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
 	return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size);
 }
 
+static inline size_t ethtool_rxfh_context_size(u32 indir_size, u32 key_size,
+					       u16 priv_size)
+{
+	size_t indir_bytes = array_size(indir_size, sizeof(u32));
+	size_t flex_len;
+
+	flex_len = size_add(size_add(indir_bytes, key_size),
+			    ALIGN(priv_size, sizeof(u32)));
+	return struct_size((struct ethtool_rxfh_context *)0, data, flex_len);
+}
+
 /* declare a link mode bitmap */
 #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
 	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
@@ -731,6 +742,8 @@ struct ethtool_mm_stats {
  *	will remain unchanged.
  *	Returns a negative error code or zero. An error code must be returned
  *	if at least one unsupported change was requested.
+ * @rxfh_priv_size: size of the driver private data area the core should
+ *	allocate for an RSS context.
  * @get_rxfh_context: Get the contents of the RX flow hash indirection table,
  *	hash key, and/or hash function assiciated to the given rss context.
  *	Returns a negative error code or zero.
@@ -824,6 +837,7 @@ struct ethtool_ops {
 	u32     cap_link_lanes_supported:1;
 	u32	supported_coalesce_params;
 	u32	supported_ring_params;
+	u16	rxfh_priv_size;
 	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
 	int	(*get_regs_len)(struct net_device *);
 	void	(*get_regs)(struct net_device *, struct ethtool_regs *, void *);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index de78b24fffc9..1d13bc8fbb75 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1249,6 +1249,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 {
 	int ret;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_rxfh_context *ctx = NULL;
 	struct ethtool_rxnfc rx_rings;
 	struct ethtool_rxfh rxfh;
 	u32 dev_indir_size = 0, dev_key_size = 0, i;
@@ -1256,7 +1257,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	u8 *hkey = NULL;
 	u8 *rss_config;
 	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
-	bool delete = false;
+	bool create = false, delete = false;
 
 	if (!ops->get_rxnfc || !ops->set_rxfh)
 		return -EOPNOTSUPP;
@@ -1275,6 +1276,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	/* Most drivers don't handle rss_context, check it's 0 as well */
 	if (rxfh.rss_context && !ops->set_rxfh_context)
 		return -EOPNOTSUPP;
+	create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
 
 	/* If either indir, hash key or function is valid, proceed further.
 	 * Must request at least one change: indir size, hash key or function.
@@ -1332,13 +1334,42 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		}
 	}
 
+	if (create) {
+		if (delete) {
+			ret = -EINVAL;
+			goto out;
+		}
+		ctx = kzalloc(ethtool_rxfh_context_size(dev_indir_size,
+							dev_key_size,
+							ops->rxfh_priv_size),
+			      GFP_KERNEL_ACCOUNT);
+		if (!ctx) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		ctx->indir_size = dev_indir_size;
+		ctx->key_size = dev_key_size;
+		ctx->hfunc = rxfh.hfunc;
+		ctx->priv_size = ops->rxfh_priv_size;
+	} else if (rxfh.rss_context) {
+		ctx = xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context);
+		if (!ctx) {
+			ret = -ENOENT;
+			goto out;
+		}
+	}
+
 	if (rxfh.rss_context)
 		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
 					    &rxfh.rss_context, delete);
 	else
 		ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
-	if (ret)
+	if (ret) {
+		if (create)
+			/* failed to create, free our new tracking entry */
+			kfree(ctx);
 		goto out;
+	}
 
 	if (copy_to_user(useraddr + offsetof(struct ethtool_rxfh, rss_context),
 			 &rxfh.rss_context, sizeof(rxfh.rss_context)))
@@ -1351,6 +1382,44 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		else if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
 			dev->priv_flags |= IFF_RXFH_CONFIGURED;
 	}
+	/* Update rss_ctx tracking */
+	if (create) {
+		/* Ideally this should happen before calling the driver,
+		 * so that we can fail more cleanly; but we don't have the
+		 * context ID until the driver picks it, so we have to
+		 * wait until after.
+		 */
+		if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
+			/* context ID reused, our tracking is screwed */
+			kfree(ctx);
+			goto out;
+		}
+		/* Allocate the exact ID the driver gave us */
+		if (xa_is_err(xa_store(&dev->ethtool->rss_ctx, rxfh.rss_context,
+				       ctx, GFP_KERNEL))) {
+			kfree(ctx);
+			goto out;
+		}
+		ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
+		ctx->key_no_change = !rxfh.key_size;
+	}
+	if (delete) {
+		WARN_ON(xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);
+		kfree(ctx);
+	} else if (ctx) {
+		if (indir) {
+			for (i = 0; i < dev_indir_size; i++)
+				ethtool_rxfh_context_indir(ctx)[i] = indir[i];
+			ctx->indir_no_change = 0;
+		}
+		if (hkey) {
+			memcpy(ethtool_rxfh_context_key(ctx), hkey,
+			       dev_key_size);
+			ctx->key_no_change = 0;
+		}
+		if (rxfh.hfunc != ETH_RSS_HASH_NO_CHANGE)
+			ctx->hfunc = rxfh.hfunc;
+	}
 
 out:
 	kfree(rss_config);

[PATCH v4 net-next 4/7] net: ethtool: let the core choose RSS context IDs

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Add a new API to create/modify/remove RSS contexts, that passes in the
 newly-chosen context ID (not as a pointer) rather than leaving the
 driver to choose it on create.  Also pass in the ctx, allowing drivers
 to easily use its private data area to store their hardware-specific
 state.
Keep the existing .set_rxfh_context API for now as a fallback, but
 deprecate it.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 include/linux/ethtool.h | 40 ++++++++++++++++++++++++---
 net/core/dev.c          | 15 ++++++++---
 net/ethtool/ioctl.c     | 60 ++++++++++++++++++++++++++++++-----------
 3 files changed, 93 insertions(+), 22 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 229a23571008..975fda7218f8 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -747,10 +747,33 @@ struct ethtool_mm_stats {
  * @get_rxfh_context: Get the contents of the RX flow hash indirection table,
  *	hash key, and/or hash function assiciated to the given rss context.
  *	Returns a negative error code or zero.
- * @set_rxfh_context: Create, remove and configure RSS contexts. Allows setting
+ * @create_rxfh_context: Create a new RSS context with the specified RX flow
+ *	hash indirection table, hash key, and hash function.
+ *	Arguments which are set to %NULL or zero will be populated to
+ *	appropriate defaults by the driver.
+ *	The &struct ethtool_rxfh_context for this context is passed in @ctx;
+ *	note that the indir table, hkey and hfunc are not yet populated as
+ *	of this call.  The driver does not need to update these; the core
+ *	will do so if this op succeeds.
+ *	If the driver provides this method, it must also provide
+ *	@modify_rxfh_context and @remove_rxfh_context.
+ *	Returns a negative error code or zero.
+ * @modify_rxfh_context: Reconfigure the specified RSS context.  Allows setting
  *	the contents of the RX flow hash indirection table, hash key, and/or
- *	hash function associated to the given context. Arguments which are set
- *	to %NULL or zero will remain unchanged.
+ *	hash function associated with the given context.
+ *	Arguments which are set to %NULL or zero will remain unchanged.
+ *	The &struct ethtool_rxfh_context for this context is passed in @ctx;
+ *	note that it will still contain the *old* settings.  The driver does
+ *	not need to update these; the core will do so if this op succeeds.
+ *	Returns a negative error code or zero. An error code must be returned
+ *	if at least one unsupported change was requested.
+ * @remove_rxfh_context: Remove the specified RSS context.
+ *	The &struct ethtool_rxfh_context for this context is passed in @ctx.
+ *	Returns a negative error code or zero.
+ * @set_rxfh_context: Deprecated API to create, remove and configure RSS
+ *	contexts. Allows setting the contents of the RX flow hash indirection
+ *	table, hash key, and/or hash function associated to the given context.
+ *	Arguments which are set to %NULL or zero will remain unchanged.
  *	Returns a negative error code or zero. An error code must be returned
  *	if at least one unsupported change was requested.
  * @get_channels: Get number of channels.
@@ -901,6 +924,17 @@ struct ethtool_ops {
 			    const u8 *key, const u8 hfunc);
 	int	(*get_rxfh_context)(struct net_device *, u32 *indir, u8 *key,
 				    u8 *hfunc, u32 rss_context);
+	int	(*create_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       const u32 *indir, const u8 *key,
+				       const u8 hfunc, u32 rss_context);
+	int	(*modify_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       const u32 *indir, const u8 *key,
+				       const u8 hfunc, u32 rss_context);
+	int	(*remove_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       u32 rss_context);
 	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
 				    const u8 *key, const u8 hfunc,
 				    u32 *rss_context, bool delete);
diff --git a/net/core/dev.c b/net/core/dev.c
index 05e95abdfd17..637218adca22 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10882,16 +10882,23 @@ static void netdev_rss_contexts_free(struct net_device *dev)
 	struct ethtool_rxfh_context *ctx;
 	unsigned long context;
 
-	if (dev->ethtool_ops->set_rxfh_context)
+	if (dev->ethtool_ops->create_rxfh_context ||
+	    dev->ethtool_ops->set_rxfh_context)
 		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
 			u32 *indir = ethtool_rxfh_context_indir(ctx);
 			u8 *key = ethtool_rxfh_context_key(ctx);
 			u32 concast = context;
 
 			xa_erase(&dev->ethtool->rss_ctx, context);
-			dev->ethtool_ops->set_rxfh_context(dev, indir, key,
-							   ctx->hfunc, &concast,
-							   true);
+			if (dev->ethtool_ops->create_rxfh_context)
+				dev->ethtool_ops->remove_rxfh_context(dev, ctx,
+								      context);
+			else
+				dev->ethtool_ops->set_rxfh_context(dev, indir,
+								   key,
+								   ctx->hfunc,
+								   &concast,
+								   true);
 			kfree(ctx);
 		}
 	xa_destroy(&dev->ethtool->rss_ctx);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 1d13bc8fbb75..c23d2bd3cd2a 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1274,7 +1274,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd8[2] || rxfh.rsvd32)
 		return -EINVAL;
 	/* Most drivers don't handle rss_context, check it's 0 as well */
-	if (rxfh.rss_context && !ops->set_rxfh_context)
+	if (rxfh.rss_context && !(ops->create_rxfh_context ||
+				  ops->set_rxfh_context))
 		return -EOPNOTSUPP;
 	create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
 
@@ -1349,8 +1350,24 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		}
 		ctx->indir_size = dev_indir_size;
 		ctx->key_size = dev_key_size;
-		ctx->hfunc = rxfh.hfunc;
 		ctx->priv_size = ops->rxfh_priv_size;
+		/* Initialise to an empty context */
+		ctx->indir_no_change = ctx->key_no_change = 1;
+		ctx->hfunc = ETH_RSS_HASH_NO_CHANGE;
+		if (ops->create_rxfh_context) {
+			u32 limit = dev->ethtool->rss_ctx_max_id ?: U32_MAX;
+			u32 ctx_id;
+
+			/* driver uses new API, core allocates ID */
+			ret = xa_alloc(&dev->ethtool->rss_ctx, &ctx_id, ctx,
+				       XA_LIMIT(1, limit), GFP_KERNEL_ACCOUNT);
+			if (ret < 0) {
+				kfree(ctx);
+				goto out;
+			}
+			WARN_ON(!ctx_id); /* can't happen */
+			rxfh.rss_context = ctx_id;
+		}
 	} else if (rxfh.rss_context) {
 		ctx = xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context);
 		if (!ctx) {
@@ -1359,15 +1376,34 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		}
 	}
 
-	if (rxfh.rss_context)
-		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
-					    &rxfh.rss_context, delete);
-	else
+	if (rxfh.rss_context) {
+		if (ops->create_rxfh_context) {
+			if (create)
+				ret = ops->create_rxfh_context(dev, ctx, indir,
+							       hkey, rxfh.hfunc,
+							       rxfh.rss_context);
+			else if (delete)
+				ret = ops->remove_rxfh_context(dev, ctx,
+							       rxfh.rss_context);
+			else
+				ret = ops->modify_rxfh_context(dev, ctx, indir,
+							       hkey, rxfh.hfunc,
+							       rxfh.rss_context);
+		} else {
+			ret = ops->set_rxfh_context(dev, indir, hkey,
+						    rxfh.hfunc,
+						    &rxfh.rss_context, delete);
+		}
+	} else {
 		ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
+	}
 	if (ret) {
-		if (create)
+		if (create) {
 			/* failed to create, free our new tracking entry */
+			if (ops->create_rxfh_context)
+				xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context);
 			kfree(ctx);
+		}
 		goto out;
 	}
 
@@ -1383,12 +1419,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 			dev->priv_flags |= IFF_RXFH_CONFIGURED;
 	}
 	/* Update rss_ctx tracking */
-	if (create) {
-		/* Ideally this should happen before calling the driver,
-		 * so that we can fail more cleanly; but we don't have the
-		 * context ID until the driver picks it, so we have to
-		 * wait until after.
-		 */
+	if (create && !ops->create_rxfh_context) {
+		/* driver uses old API, it chose context ID */
 		if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
 			/* context ID reused, our tracking is screwed */
 			kfree(ctx);
@@ -1400,8 +1432,6 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 			kfree(ctx);
 			goto out;
 		}
-		ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
-		ctx->key_no_change = !rxfh.key_size;
 	}
 	if (delete) {
 		WARN_ON(xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);

[PATCH v4 net-next 5/7] net: ethtool: add an extack parameter to new rxfh_context APIs

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Currently passed as NULL, but will allow drivers to report back errors
 when ethnl support for these ops is added.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 include/linux/ethtool.h | 9 ++++++---
 net/core/dev.c          | 3 ++-
 net/ethtool/ioctl.c     | 9 ++++++---
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 975fda7218f8..c8963bde9289 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -927,14 +927,17 @@ struct ethtool_ops {
 	int	(*create_rxfh_context)(struct net_device *,
 				       struct ethtool_rxfh_context *ctx,
 				       const u32 *indir, const u8 *key,
-				       const u8 hfunc, u32 rss_context);
+				       const u8 hfunc, u32 rss_context,
+				       struct netlink_ext_ack *extack);
 	int	(*modify_rxfh_context)(struct net_device *,
 				       struct ethtool_rxfh_context *ctx,
 				       const u32 *indir, const u8 *key,
-				       const u8 hfunc, u32 rss_context);
+				       const u8 hfunc, u32 rss_context,
+				       struct netlink_ext_ack *extack);
 	int	(*remove_rxfh_context)(struct net_device *,
 				       struct ethtool_rxfh_context *ctx,
-				       u32 rss_context);
+				       u32 rss_context,
+				       struct netlink_ext_ack *extack);
 	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
 				    const u8 *key, const u8 hfunc,
 				    u32 *rss_context, bool delete);
diff --git a/net/core/dev.c b/net/core/dev.c
index 637218adca22..69579d9cd7ba 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10892,7 +10892,8 @@ static void netdev_rss_contexts_free(struct net_device *dev)
 			xa_erase(&dev->ethtool->rss_ctx, context);
 			if (dev->ethtool_ops->create_rxfh_context)
 				dev->ethtool_ops->remove_rxfh_context(dev, ctx,
-								      context);
+								      context,
+								      NULL);
 			else
 				dev->ethtool_ops->set_rxfh_context(dev, indir,
 								   key,
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index c23d2bd3cd2a..3920ddee3ee2 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1381,14 +1381,17 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 			if (create)
 				ret = ops->create_rxfh_context(dev, ctx, indir,
 							       hkey, rxfh.hfunc,
-							       rxfh.rss_context);
+							       rxfh.rss_context,
+							       NULL);
 			else if (delete)
 				ret = ops->remove_rxfh_context(dev, ctx,
-							       rxfh.rss_context);
+							       rxfh.rss_context,
+							       NULL);
 			else
 				ret = ops->modify_rxfh_context(dev, ctx, indir,
 							       hkey, rxfh.hfunc,
-							       rxfh.rss_context);
+							       rxfh.rss_context,
+							       NULL);
 		} else {
 			ret = ops->set_rxfh_context(dev, indir, hkey,
 						    rxfh.hfunc,

[PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

While this is not needed to serialise the ethtool entry points (which
 are all under RTNL), drivers may have cause to asynchronously access
 dev->ethtool->rss_ctx; taking dev->ethtool->rss_lock allows them to
 do this safely without needing to take the RTNL.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 include/linux/ethtool.h | 3 +++
 net/core/dev.c          | 5 +++++
 net/ethtool/ioctl.c     | 7 +++++++
 3 files changed, 15 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index c8963bde9289..d15a21bd6f12 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1026,11 +1026,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
 /**
  * struct ethtool_netdev_state - per-netdevice state for ethtool features
  * @rss_ctx:		XArray of custom RSS contexts
+ * @rss_lock:		Protects entries in @rss_ctx.  May be taken from
+ *			within RTNL.
  * @rss_ctx_max_id:	maximum (exclusive) supported RSS context ID
  * @wol_enabled:	Wake-on-LAN is enabled
  */
 struct ethtool_netdev_state {
 	struct xarray		rss_ctx;
+	struct mutex		rss_lock;
 	u32			rss_ctx_max_id;
 	u32			wol_enabled:1;
 };
diff --git a/net/core/dev.c b/net/core/dev.c
index 69579d9cd7ba..c57456ed4be8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10074,6 +10074,7 @@ int register_netdevice(struct net_device *dev)
 
 	/* rss ctx ID 0 is reserved for the default context, start from 1 */
 	xa_init_flags(&dev->ethtool->rss_ctx, XA_FLAGS_ALLOC1);
+	mutex_init(&dev->ethtool->rss_lock);
 
 	spin_lock_init(&dev->addr_list_lock);
 	netdev_set_addr_lockdep_class(dev);
@@ -10882,6 +10883,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
 	struct ethtool_rxfh_context *ctx;
 	unsigned long context;
 
+	mutex_lock(&dev->ethtool->rss_lock);
 	if (dev->ethtool_ops->create_rxfh_context ||
 	    dev->ethtool_ops->set_rxfh_context)
 		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
@@ -10903,6 +10905,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
 			kfree(ctx);
 		}
 	xa_destroy(&dev->ethtool->rss_ctx);
+	mutex_unlock(&dev->ethtool->rss_lock);
 }
 
 /**
@@ -11016,6 +11019,8 @@ void unregister_netdevice_many_notify(struct list_head *head,
 		if (dev->netdev_ops->ndo_uninit)
 			dev->netdev_ops->ndo_uninit(dev);
 
+		mutex_destroy(&dev->ethtool->rss_lock);
+
 		if (skb)
 			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, portid, nlh);
 
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 3920ddee3ee2..d21bbc92e6fc 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1258,6 +1258,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	u8 *rss_config;
 	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
 	bool create = false, delete = false;
+	bool locked = false; /* dev->ethtool->rss_lock taken */
 
 	if (!ops->get_rxnfc || !ops->set_rxfh)
 		return -EOPNOTSUPP;
@@ -1335,6 +1336,10 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		}
 	}
 
+	if (rxfh.rss_context) {
+		mutex_lock(&dev->ethtool->rss_lock);
+		locked = true;
+	}
 	if (create) {
 		if (delete) {
 			ret = -EINVAL;
@@ -1455,6 +1460,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	}
 
 out:
+	if (locked)
+		mutex_unlock(&dev->ethtool->rss_lock);
 	kfree(rss_config);
 	return ret;
 }

[PATCH v4 net-next 7/7] sfc: use new rxfh_context API

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

The core is now responsible for allocating IDs and a memory region for
 us to store our state (struct efx_rss_context_priv), so we no longer
 need efx_alloc_rss_context_entry() and friends.
Since the contexts are now maintained by the core, use the core's lock
 (net_dev->ethtool->rss_lock), rather than our own mutex (efx->rss_lock),
 to serialise access against changes; and remove the now-unused
 efx->rss_lock from struct efx_nic.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/ef10.c           |   2 +-
 drivers/net/ethernet/sfc/ef100_ethtool.c  |   5 +-
 drivers/net/ethernet/sfc/efx.c            |   2 +-
 drivers/net/ethernet/sfc/efx.h            |   2 +-
 drivers/net/ethernet/sfc/efx_common.c     |  10 +-
 drivers/net/ethernet/sfc/ethtool.c        |   5 +-
 drivers/net/ethernet/sfc/ethtool_common.c | 147 +++++++++++++---------
 drivers/net/ethernet/sfc/ethtool_common.h |  18 ++-
 drivers/net/ethernet/sfc/mcdi_filters.c   | 135 ++++++++++----------
 drivers/net/ethernet/sfc/mcdi_filters.h   |   8 +-
 drivers/net/ethernet/sfc/net_driver.h     |  28 ++---
 drivers/net/ethernet/sfc/rx_common.c      |  64 ++--------
 drivers/net/ethernet/sfc/rx_common.h      |   8 +-
 13 files changed, 214 insertions(+), 220 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 6dfa062feebc..e20305461b57 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -1396,7 +1396,7 @@ static void efx_ef10_table_reset_mc_allocations(struct efx_nic *efx)
 	efx_mcdi_filter_table_reset_mc_allocations(efx);
 	nic_data->must_restore_piobufs = true;
 	efx_ef10_forget_old_piobufs(efx);
-	efx->rss_context.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
+	efx->rss_context.priv.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
 
 	/* Driver-created vswitches and vports must be re-created */
 	nic_data->must_probe_vswitching = true;
diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
index 702abbe59b76..c5f82eb0e5b4 100644
--- a/drivers/net/ethernet/sfc/ef100_ethtool.c
+++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
@@ -58,10 +58,13 @@ const struct ethtool_ops ef100_ethtool_ops = {
 
 	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
 	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
+	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
 	.get_rxfh		= efx_ethtool_get_rxfh,
 	.set_rxfh		= efx_ethtool_set_rxfh,
 	.get_rxfh_context	= efx_ethtool_get_rxfh_context,
-	.set_rxfh_context	= efx_ethtool_set_rxfh_context,
+	.create_rxfh_context	= efx_ethtool_create_rxfh_context,
+	.modify_rxfh_context	= efx_ethtool_modify_rxfh_context,
+	.remove_rxfh_context	= efx_ethtool_remove_rxfh_context,
 
 	.get_module_info	= efx_ethtool_get_module_info,
 	.get_module_eeprom	= efx_ethtool_get_module_eeprom,
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 19f4b4d0b851..6ae84356797f 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -299,7 +299,7 @@ static int efx_probe_nic(struct efx_nic *efx)
 	if (efx->n_channels > 1)
 		netdev_rss_key_fill(efx->rss_context.rx_hash_key,
 				    sizeof(efx->rss_context.rx_hash_key));
-	efx_set_default_rx_indir_table(efx, &efx->rss_context);
+	efx_set_default_rx_indir_table(efx, efx->rss_context.rx_indir_table);
 
 	/* Initialise the interrupt moderation settings */
 	efx->irq_mod_step_us = DIV_ROUND_UP(efx->timer_quantum_ns, 1000);
diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
index 48d3623735ba..7a6cab883d66 100644
--- a/drivers/net/ethernet/sfc/efx.h
+++ b/drivers/net/ethernet/sfc/efx.h
@@ -158,7 +158,7 @@ static inline s32 efx_filter_get_rx_ids(struct efx_nic *efx,
 }
 
 /* RSS contexts */
-static inline bool efx_rss_active(struct efx_rss_context *ctx)
+static inline bool efx_rss_active(struct efx_rss_context_priv *ctx)
 {
 	return ctx->context_id != EFX_MCDI_RSS_CONTEXT_INVALID;
 }
diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
index 175bd9cdfdac..2cd92e1f68db 100644
--- a/drivers/net/ethernet/sfc/efx_common.c
+++ b/drivers/net/ethernet/sfc/efx_common.c
@@ -714,7 +714,7 @@ void efx_reset_down(struct efx_nic *efx, enum reset_type method)
 
 	mutex_lock(&efx->mac_lock);
 	down_write(&efx->filter_sem);
-	mutex_lock(&efx->rss_lock);
+	mutex_lock(&efx->net_dev->ethtool->rss_lock);
 	efx->type->fini(efx);
 }
 
@@ -777,7 +777,7 @@ int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok)
 
 	if (efx->type->rx_restore_rss_contexts)
 		efx->type->rx_restore_rss_contexts(efx);
-	mutex_unlock(&efx->rss_lock);
+	mutex_unlock(&efx->net_dev->ethtool->rss_lock);
 	efx->type->filter_table_restore(efx);
 	up_write(&efx->filter_sem);
 
@@ -793,7 +793,7 @@ int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok)
 fail:
 	efx->port_initialized = false;
 
-	mutex_unlock(&efx->rss_lock);
+	mutex_unlock(&efx->net_dev->ethtool->rss_lock);
 	up_write(&efx->filter_sem);
 	mutex_unlock(&efx->mac_lock);
 
@@ -1000,9 +1000,7 @@ int efx_init_struct(struct efx_nic *efx, struct pci_dev *pci_dev)
 		efx->type->rx_hash_offset - efx->type->rx_prefix_size;
 	efx->rx_packet_ts_offset =
 		efx->type->rx_ts_offset - efx->type->rx_prefix_size;
-	INIT_LIST_HEAD(&efx->rss_context.list);
-	efx->rss_context.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
-	mutex_init(&efx->rss_lock);
+	efx->rss_context.priv.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
 	efx->vport_id = EVB_PORT_ID_ASSIGNED;
 	spin_lock_init(&efx->stats_lock);
 	efx->vi_stride = EFX_DEFAULT_VI_STRIDE;
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 364323599f7b..f5fb7464e025 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -267,10 +267,13 @@ const struct ethtool_ops efx_ethtool_ops = {
 	.set_rxnfc		= efx_ethtool_set_rxnfc,
 	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
 	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
+	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
 	.get_rxfh		= efx_ethtool_get_rxfh,
 	.set_rxfh		= efx_ethtool_set_rxfh,
 	.get_rxfh_context	= efx_ethtool_get_rxfh_context,
-	.set_rxfh_context	= efx_ethtool_set_rxfh_context,
+	.create_rxfh_context	= efx_ethtool_create_rxfh_context,
+	.modify_rxfh_context	= efx_ethtool_modify_rxfh_context,
+	.remove_rxfh_context	= efx_ethtool_remove_rxfh_context,
 	.get_ts_info		= efx_ethtool_get_ts_info,
 	.get_module_info	= efx_ethtool_get_module_info,
 	.get_module_eeprom	= efx_ethtool_get_module_eeprom,
diff --git a/drivers/net/ethernet/sfc/ethtool_common.c b/drivers/net/ethernet/sfc/ethtool_common.c
index a8cbceeb301b..7cd01012152e 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.c
+++ b/drivers/net/ethernet/sfc/ethtool_common.c
@@ -820,10 +820,10 @@ int efx_ethtool_get_rxnfc(struct net_device *net_dev,
 		return 0;
 
 	case ETHTOOL_GRXFH: {
-		struct efx_rss_context *ctx = &efx->rss_context;
+		struct efx_rss_context_priv *ctx = &efx->rss_context.priv;
 		__u64 data;
 
-		mutex_lock(&efx->rss_lock);
+		mutex_lock(&net_dev->ethtool->rss_lock);
 		if (info->flow_type & FLOW_RSS && info->rss_context) {
 			ctx = efx_find_rss_context_entry(efx, info->rss_context);
 			if (!ctx) {
@@ -864,7 +864,7 @@ int efx_ethtool_get_rxnfc(struct net_device *net_dev,
 out_setdata_unlock:
 		info->data = data;
 out_unlock:
-		mutex_unlock(&efx->rss_lock);
+		mutex_unlock(&net_dev->ethtool->rss_lock);
 		return rc;
 	}
 
@@ -1207,96 +1207,121 @@ int efx_ethtool_get_rxfh_context(struct net_device *net_dev, u32 *indir,
 				 u8 *key, u8 *hfunc, u32 rss_context)
 {
 	struct efx_nic *efx = efx_netdev_priv(net_dev);
-	struct efx_rss_context *ctx;
+	struct efx_rss_context_priv *ctx_priv;
+	struct efx_rss_context ctx;
 	int rc = 0;
 
 	if (!efx->type->rx_pull_rss_context_config)
 		return -EOPNOTSUPP;
 
-	mutex_lock(&efx->rss_lock);
-	ctx = efx_find_rss_context_entry(efx, rss_context);
-	if (!ctx) {
+	mutex_lock(&net_dev->ethtool->rss_lock);
+	ctx_priv = efx_find_rss_context_entry(efx, rss_context);
+	if (!ctx_priv) {
 		rc = -ENOENT;
 		goto out_unlock;
 	}
-	rc = efx->type->rx_pull_rss_context_config(efx, ctx);
+	ctx.priv = *ctx_priv;
+	rc = efx->type->rx_pull_rss_context_config(efx, &ctx);
 	if (rc)
 		goto out_unlock;
 
 	if (hfunc)
 		*hfunc = ETH_RSS_HASH_TOP;
 	if (indir)
-		memcpy(indir, ctx->rx_indir_table, sizeof(ctx->rx_indir_table));
+		memcpy(indir, ctx.rx_indir_table, sizeof(ctx.rx_indir_table));
 	if (key)
-		memcpy(key, ctx->rx_hash_key, efx->type->rx_hash_key_size);
+		memcpy(key, ctx.rx_hash_key, efx->type->rx_hash_key_size);
 out_unlock:
-	mutex_unlock(&efx->rss_lock);
+	mutex_unlock(&net_dev->ethtool->rss_lock);
 	return rc;
 }
 
-int efx_ethtool_set_rxfh_context(struct net_device *net_dev,
-				 const u32 *indir, const u8 *key,
-				 const u8 hfunc, u32 *rss_context,
-				 bool delete)
+int efx_ethtool_modify_rxfh_context(struct net_device *net_dev,
+				    struct ethtool_rxfh_context *ctx,
+				    const u32 *indir, const u8 *key,
+				    const u8 hfunc, u32 rss_context,
+				    struct netlink_ext_ack *extack)
 {
 	struct efx_nic *efx = efx_netdev_priv(net_dev);
-	struct efx_rss_context *ctx;
-	bool allocated = false;
-	int rc;
+	struct efx_rss_context_priv *priv;
 
-	if (!efx->type->rx_push_rss_context_config)
+	if (!efx->type->rx_push_rss_context_config) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "NIC type does not support custom contexts");
 		return -EOPNOTSUPP;
+	}
 	/* Hash function is Toeplitz, cannot be changed */
-	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP)
+	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP) {
+		NL_SET_ERR_MSG_MOD(extack, "Only Toeplitz hash is supported");
 		return -EOPNOTSUPP;
+	}
 
-	mutex_lock(&efx->rss_lock);
+	priv = ethtool_rxfh_context_priv(ctx);
 
-	if (*rss_context == ETH_RXFH_CONTEXT_ALLOC) {
-		if (delete) {
-			/* alloc + delete == Nothing to do */
-			rc = -EINVAL;
-			goto out_unlock;
-		}
-		ctx = efx_alloc_rss_context_entry(efx);
-		if (!ctx) {
-			rc = -ENOMEM;
-			goto out_unlock;
-		}
-		ctx->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
-		/* Initialise indir table and key to defaults */
-		efx_set_default_rx_indir_table(efx, ctx);
-		netdev_rss_key_fill(ctx->rx_hash_key, sizeof(ctx->rx_hash_key));
-		allocated = true;
-	} else {
-		ctx = efx_find_rss_context_entry(efx, *rss_context);
-		if (!ctx) {
-			rc = -ENOENT;
-			goto out_unlock;
-		}
-	}
+	if (!key)
+		key = ethtool_rxfh_context_key(ctx);
+	if (!indir)
+		indir = ethtool_rxfh_context_indir(ctx);
 
-	if (delete) {
-		/* delete this context */
-		rc = efx->type->rx_push_rss_context_config(efx, ctx, NULL, NULL);
-		if (!rc)
-			efx_free_rss_context_entry(ctx);
-		goto out_unlock;
+	return efx->type->rx_push_rss_context_config(efx, priv, indir, key,
+						     false);
+}
+
+int efx_ethtool_create_rxfh_context(struct net_device *net_dev,
+				    struct ethtool_rxfh_context *ctx,
+				    const u32 *indir, const u8 *key,
+				    const u8 hfunc, u32 rss_context,
+				    struct netlink_ext_ack *extack)
+{
+	struct efx_nic *efx = efx_netdev_priv(net_dev);
+	struct efx_rss_context_priv *priv;
+
+	if (!efx->type->rx_push_rss_context_config) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "NIC type does not support custom contexts");
+		return -EOPNOTSUPP;
+	}
+	/* Hash function is Toeplitz, cannot be changed */
+	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP) {
+		NL_SET_ERR_MSG_MOD(extack, "Only Toeplitz hash is supported");
+		return -EOPNOTSUPP;
 	}
 
-	if (!key)
-		key = ctx->rx_hash_key;
+	priv = ethtool_rxfh_context_priv(ctx);
+
+	priv->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
+	priv->rx_hash_udp_4tuple = false;
+	/* Generate default indir table and/or key if not specified.
+	 * We use ctx as a place to store these; this is fine because
+	 * we're doing a create, so if we fail then the ctx will just
+	 * be deleted.
+	 */
 	if (!indir)
-		indir = ctx->rx_indir_table;
+		efx_set_default_rx_indir_table(efx, ethtool_rxfh_context_indir(ctx));
+	if (!key)
+		netdev_rss_key_fill(ethtool_rxfh_context_key(ctx),
+				    ctx->key_size);
+	return efx_ethtool_modify_rxfh_context(net_dev, ctx, indir, key, hfunc,
+					       rss_context, extack);
+}
 
-	rc = efx->type->rx_push_rss_context_config(efx, ctx, indir, key);
-	if (rc && allocated)
-		efx_free_rss_context_entry(ctx);
-	else
-		*rss_context = ctx->user_id;
-out_unlock:
-	mutex_unlock(&efx->rss_lock);
-	return rc;
+int efx_ethtool_remove_rxfh_context(struct net_device *net_dev,
+				    struct ethtool_rxfh_context *ctx,
+				    u32 rss_context,
+				    struct netlink_ext_ack *extack)
+{
+	struct efx_nic *efx = efx_netdev_priv(net_dev);
+	struct efx_rss_context_priv *priv;
+
+	if (!efx->type->rx_push_rss_context_config) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "NIC type does not support custom contexts");
+		return -EOPNOTSUPP;
+	}
+
+	priv = ethtool_rxfh_context_priv(ctx);
+	return efx->type->rx_push_rss_context_config(efx, priv, NULL, NULL,
+						     true);
 }
 
 int efx_ethtool_reset(struct net_device *net_dev, u32 *flags)
diff --git a/drivers/net/ethernet/sfc/ethtool_common.h b/drivers/net/ethernet/sfc/ethtool_common.h
index 659491932101..3df852eaab20 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.h
+++ b/drivers/net/ethernet/sfc/ethtool_common.h
@@ -50,10 +50,20 @@ int efx_ethtool_set_rxfh(struct net_device *net_dev,
 			 const u32 *indir, const u8 *key, const u8 hfunc);
 int efx_ethtool_get_rxfh_context(struct net_device *net_dev, u32 *indir,
 				 u8 *key, u8 *hfunc, u32 rss_context);
-int efx_ethtool_set_rxfh_context(struct net_device *net_dev,
-				 const u32 *indir, const u8 *key,
-				 const u8 hfunc, u32 *rss_context,
-				 bool delete);
+int efx_ethtool_create_rxfh_context(struct net_device *net_dev,
+				    struct ethtool_rxfh_context *ctx,
+				    const u32 *indir, const u8 *key,
+				    const u8 hfunc, u32 rss_context,
+				    struct netlink_ext_ack *extack);
+int efx_ethtool_modify_rxfh_context(struct net_device *net_dev,
+				    struct ethtool_rxfh_context *ctx,
+				    const u32 *indir, const u8 *key,
+				    const u8 hfunc, u32 rss_context,
+				    struct netlink_ext_ack *extack);
+int efx_ethtool_remove_rxfh_context(struct net_device *net_dev,
+				    struct ethtool_rxfh_context *ctx,
+				    u32 rss_context,
+				    struct netlink_ext_ack *extack);
 int efx_ethtool_reset(struct net_device *net_dev, u32 *flags);
 int efx_ethtool_get_module_eeprom(struct net_device *net_dev,
 				  struct ethtool_eeprom *ee,
diff --git a/drivers/net/ethernet/sfc/mcdi_filters.c b/drivers/net/ethernet/sfc/mcdi_filters.c
index 4ff6586116ee..6ef96292909a 100644
--- a/drivers/net/ethernet/sfc/mcdi_filters.c
+++ b/drivers/net/ethernet/sfc/mcdi_filters.c
@@ -194,7 +194,7 @@ efx_mcdi_filter_push_prep_set_match_fields(struct efx_nic *efx,
 static void efx_mcdi_filter_push_prep(struct efx_nic *efx,
 				      const struct efx_filter_spec *spec,
 				      efx_dword_t *inbuf, u64 handle,
-				      struct efx_rss_context *ctx,
+				      struct efx_rss_context_priv *ctx,
 				      bool replacing)
 {
 	u32 flags = spec->flags;
@@ -245,7 +245,7 @@ static void efx_mcdi_filter_push_prep(struct efx_nic *efx,
 
 static int efx_mcdi_filter_push(struct efx_nic *efx,
 				const struct efx_filter_spec *spec, u64 *handle,
-				struct efx_rss_context *ctx, bool replacing)
+				struct efx_rss_context_priv *ctx, bool replacing)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_FILTER_OP_EXT_IN_LEN);
 	MCDI_DECLARE_BUF(outbuf, MC_CMD_FILTER_OP_EXT_OUT_LEN);
@@ -345,9 +345,9 @@ static s32 efx_mcdi_filter_insert_locked(struct efx_nic *efx,
 					 bool replace_equal)
 {
 	DECLARE_BITMAP(mc_rem_map, EFX_EF10_FILTER_SEARCH_LIMIT);
+	struct efx_rss_context_priv *ctx = NULL;
 	struct efx_mcdi_filter_table *table;
 	struct efx_filter_spec *saved_spec;
-	struct efx_rss_context *ctx = NULL;
 	unsigned int match_pri, hash;
 	unsigned int priv_flags;
 	bool rss_locked = false;
@@ -380,12 +380,12 @@ static s32 efx_mcdi_filter_insert_locked(struct efx_nic *efx,
 		bitmap_zero(mc_rem_map, EFX_EF10_FILTER_SEARCH_LIMIT);
 
 	if (spec->flags & EFX_FILTER_FLAG_RX_RSS) {
-		mutex_lock(&efx->rss_lock);
+		mutex_lock(&efx->net_dev->ethtool->rss_lock);
 		rss_locked = true;
 		if (spec->rss_context)
 			ctx = efx_find_rss_context_entry(efx, spec->rss_context);
 		else
-			ctx = &efx->rss_context;
+			ctx = &efx->rss_context.priv;
 		if (!ctx) {
 			rc = -ENOENT;
 			goto out_unlock;
@@ -548,7 +548,7 @@ static s32 efx_mcdi_filter_insert_locked(struct efx_nic *efx,
 
 out_unlock:
 	if (rss_locked)
-		mutex_unlock(&efx->rss_lock);
+		mutex_unlock(&efx->net_dev->ethtool->rss_lock);
 	up_write(&table->lock);
 	return rc;
 }
@@ -611,13 +611,13 @@ static int efx_mcdi_filter_remove_internal(struct efx_nic *efx,
 
 		new_spec.priority = EFX_FILTER_PRI_AUTO;
 		new_spec.flags = (EFX_FILTER_FLAG_RX |
-				  (efx_rss_active(&efx->rss_context) ?
+				  (efx_rss_active(&efx->rss_context.priv) ?
 				   EFX_FILTER_FLAG_RX_RSS : 0));
 		new_spec.dmaq_id = 0;
 		new_spec.rss_context = 0;
 		rc = efx_mcdi_filter_push(efx, &new_spec,
 					  &table->entry[filter_idx].handle,
-					  &efx->rss_context,
+					  &efx->rss_context.priv,
 					  true);
 
 		if (rc == 0)
@@ -764,7 +764,7 @@ static int efx_mcdi_filter_insert_addr_list(struct efx_nic *efx,
 		ids = vlan->uc;
 	}
 
-	filter_flags = efx_rss_active(&efx->rss_context) ? EFX_FILTER_FLAG_RX_RSS : 0;
+	filter_flags = efx_rss_active(&efx->rss_context.priv) ? EFX_FILTER_FLAG_RX_RSS : 0;
 
 	/* Insert/renew filters */
 	for (i = 0; i < addr_count; i++) {
@@ -833,7 +833,7 @@ static int efx_mcdi_filter_insert_def(struct efx_nic *efx,
 	int rc;
 	u16 *id;
 
-	filter_flags = efx_rss_active(&efx->rss_context) ? EFX_FILTER_FLAG_RX_RSS : 0;
+	filter_flags = efx_rss_active(&efx->rss_context.priv) ? EFX_FILTER_FLAG_RX_RSS : 0;
 
 	efx_filter_init_rx(&spec, EFX_FILTER_PRI_AUTO, filter_flags, 0);
 
@@ -1375,8 +1375,8 @@ void efx_mcdi_filter_table_restore(struct efx_nic *efx)
 	struct efx_mcdi_filter_table *table = efx->filter_state;
 	unsigned int invalid_filters = 0, failed = 0;
 	struct efx_mcdi_filter_vlan *vlan;
+	struct efx_rss_context_priv *ctx;
 	struct efx_filter_spec *spec;
-	struct efx_rss_context *ctx;
 	unsigned int filter_idx;
 	u32 mcdi_flags;
 	int match_pri;
@@ -1388,7 +1388,7 @@ void efx_mcdi_filter_table_restore(struct efx_nic *efx)
 		return;
 
 	down_write(&table->lock);
-	mutex_lock(&efx->rss_lock);
+	mutex_lock(&efx->net_dev->ethtool->rss_lock);
 
 	for (filter_idx = 0; filter_idx < EFX_MCDI_FILTER_TBL_ROWS; filter_idx++) {
 		spec = efx_mcdi_filter_entry_spec(table, filter_idx);
@@ -1407,7 +1407,7 @@ void efx_mcdi_filter_table_restore(struct efx_nic *efx)
 		if (spec->rss_context)
 			ctx = efx_find_rss_context_entry(efx, spec->rss_context);
 		else
-			ctx = &efx->rss_context;
+			ctx = &efx->rss_context.priv;
 		if (spec->flags & EFX_FILTER_FLAG_RX_RSS) {
 			if (!ctx) {
 				netif_warn(efx, drv, efx->net_dev,
@@ -1444,7 +1444,7 @@ void efx_mcdi_filter_table_restore(struct efx_nic *efx)
 		}
 	}
 
-	mutex_unlock(&efx->rss_lock);
+	mutex_unlock(&efx->net_dev->ethtool->rss_lock);
 	up_write(&table->lock);
 
 	/*
@@ -1861,7 +1861,8 @@ bool efx_mcdi_filter_rfs_expire_one(struct efx_nic *efx, u32 flow_id,
 					 RSS_MODE_HASH_ADDRS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV6_RSS_MODE_LBN |\
 					 RSS_MODE_HASH_ADDRS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_OTHER_IPV6_RSS_MODE_LBN)
 
-int efx_mcdi_get_rss_context_flags(struct efx_nic *efx, u32 context, u32 *flags)
+static int efx_mcdi_get_rss_context_flags(struct efx_nic *efx, u32 context,
+					  u32 *flags)
 {
 	/*
 	 * Firmware had a bug (sfc bug 61952) where it would not actually
@@ -1909,8 +1910,8 @@ int efx_mcdi_get_rss_context_flags(struct efx_nic *efx, u32 context, u32 *flags)
  * Defaults are 4-tuple for TCP and 2-tuple for UDP and other-IP, so we
  * just need to set the UDP ports flags (for both IP versions).
  */
-void efx_mcdi_set_rss_context_flags(struct efx_nic *efx,
-				    struct efx_rss_context *ctx)
+static void efx_mcdi_set_rss_context_flags(struct efx_nic *efx,
+					   struct efx_rss_context_priv *ctx)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_RSS_CONTEXT_SET_FLAGS_IN_LEN);
 	u32 flags;
@@ -1931,7 +1932,7 @@ void efx_mcdi_set_rss_context_flags(struct efx_nic *efx,
 }
 
 static int efx_mcdi_filter_alloc_rss_context(struct efx_nic *efx, bool exclusive,
-					     struct efx_rss_context *ctx,
+					     struct efx_rss_context_priv *ctx,
 					     unsigned *context_size)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_RSS_CONTEXT_ALLOC_IN_LEN);
@@ -2032,25 +2033,26 @@ void efx_mcdi_rx_free_indir_table(struct efx_nic *efx)
 {
 	int rc;
 
-	if (efx->rss_context.context_id != EFX_MCDI_RSS_CONTEXT_INVALID) {
-		rc = efx_mcdi_filter_free_rss_context(efx, efx->rss_context.context_id);
+	if (efx->rss_context.priv.context_id != EFX_MCDI_RSS_CONTEXT_INVALID) {
+		rc = efx_mcdi_filter_free_rss_context(efx, efx->rss_context.priv.context_id);
 		WARN_ON(rc != 0);
 	}
-	efx->rss_context.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
+	efx->rss_context.priv.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
 }
 
 static int efx_mcdi_filter_rx_push_shared_rss_config(struct efx_nic *efx,
 					      unsigned *context_size)
 {
 	struct efx_mcdi_filter_table *table = efx->filter_state;
-	int rc = efx_mcdi_filter_alloc_rss_context(efx, false, &efx->rss_context,
-					    context_size);
+	int rc = efx_mcdi_filter_alloc_rss_context(efx, false,
+						   &efx->rss_context.priv,
+						   context_size);
 
 	if (rc != 0)
 		return rc;
 
 	table->rx_rss_context_exclusive = false;
-	efx_set_default_rx_indir_table(efx, &efx->rss_context);
+	efx_set_default_rx_indir_table(efx, efx->rss_context.rx_indir_table);
 	return 0;
 }
 
@@ -2058,26 +2060,27 @@ static int efx_mcdi_filter_rx_push_exclusive_rss_config(struct efx_nic *efx,
 						 const u32 *rx_indir_table,
 						 const u8 *key)
 {
+	u32 old_rx_rss_context = efx->rss_context.priv.context_id;
 	struct efx_mcdi_filter_table *table = efx->filter_state;
-	u32 old_rx_rss_context = efx->rss_context.context_id;
 	int rc;
 
-	if (efx->rss_context.context_id == EFX_MCDI_RSS_CONTEXT_INVALID ||
+	if (efx->rss_context.priv.context_id == EFX_MCDI_RSS_CONTEXT_INVALID ||
 	    !table->rx_rss_context_exclusive) {
-		rc = efx_mcdi_filter_alloc_rss_context(efx, true, &efx->rss_context,
-						NULL);
+		rc = efx_mcdi_filter_alloc_rss_context(efx, true,
+						       &efx->rss_context.priv,
+						       NULL);
 		if (rc == -EOPNOTSUPP)
 			return rc;
 		else if (rc != 0)
 			goto fail1;
 	}
 
-	rc = efx_mcdi_filter_populate_rss_table(efx, efx->rss_context.context_id,
-					 rx_indir_table, key);
+	rc = efx_mcdi_filter_populate_rss_table(efx, efx->rss_context.priv.context_id,
+						rx_indir_table, key);
 	if (rc != 0)
 		goto fail2;
 
-	if (efx->rss_context.context_id != old_rx_rss_context &&
+	if (efx->rss_context.priv.context_id != old_rx_rss_context &&
 	    old_rx_rss_context != EFX_MCDI_RSS_CONTEXT_INVALID)
 		WARN_ON(efx_mcdi_filter_free_rss_context(efx, old_rx_rss_context) != 0);
 	table->rx_rss_context_exclusive = true;
@@ -2091,9 +2094,9 @@ static int efx_mcdi_filter_rx_push_exclusive_rss_config(struct efx_nic *efx,
 	return 0;
 
 fail2:
-	if (old_rx_rss_context != efx->rss_context.context_id) {
-		WARN_ON(efx_mcdi_filter_free_rss_context(efx, efx->rss_context.context_id) != 0);
-		efx->rss_context.context_id = old_rx_rss_context;
+	if (old_rx_rss_context != efx->rss_context.priv.context_id) {
+		WARN_ON(efx_mcdi_filter_free_rss_context(efx, efx->rss_context.priv.context_id) != 0);
+		efx->rss_context.priv.context_id = old_rx_rss_context;
 	}
 fail1:
 	netif_err(efx, hw, efx->net_dev, "%s: failed rc=%d\n", __func__, rc);
@@ -2101,33 +2104,28 @@ static int efx_mcdi_filter_rx_push_exclusive_rss_config(struct efx_nic *efx,
 }
 
 int efx_mcdi_rx_push_rss_context_config(struct efx_nic *efx,
-					struct efx_rss_context *ctx,
+					struct efx_rss_context_priv *ctx,
 					const u32 *rx_indir_table,
-					const u8 *key)
+					const u8 *key, bool delete)
 {
 	int rc;
 
-	WARN_ON(!mutex_is_locked(&efx->rss_lock));
+	WARN_ON(!mutex_is_locked(&efx->net_dev->ethtool->rss_lock));
 
 	if (ctx->context_id == EFX_MCDI_RSS_CONTEXT_INVALID) {
+		if (delete)
+			/* already wasn't in HW, nothing to do */
+			return 0;
 		rc = efx_mcdi_filter_alloc_rss_context(efx, true, ctx, NULL);
 		if (rc)
 			return rc;
 	}
 
-	if (!rx_indir_table) /* Delete this context */
+	if (delete) /* Delete this context */
 		return efx_mcdi_filter_free_rss_context(efx, ctx->context_id);
 
-	rc = efx_mcdi_filter_populate_rss_table(efx, ctx->context_id,
-					 rx_indir_table, key);
-	if (rc)
-		return rc;
-
-	memcpy(ctx->rx_indir_table, rx_indir_table,
-	       sizeof(efx->rss_context.rx_indir_table));
-	memcpy(ctx->rx_hash_key, key, efx->type->rx_hash_key_size);
-
-	return 0;
+	return efx_mcdi_filter_populate_rss_table(efx, ctx->context_id,
+						  rx_indir_table, key);
 }
 
 int efx_mcdi_rx_pull_rss_context_config(struct efx_nic *efx,
@@ -2139,16 +2137,16 @@ int efx_mcdi_rx_pull_rss_context_config(struct efx_nic *efx,
 	size_t outlen;
 	int rc, i;
 
-	WARN_ON(!mutex_is_locked(&efx->rss_lock));
+	WARN_ON(!mutex_is_locked(&efx->net_dev->ethtool->rss_lock));
 
 	BUILD_BUG_ON(MC_CMD_RSS_CONTEXT_GET_TABLE_IN_LEN !=
 		     MC_CMD_RSS_CONTEXT_GET_KEY_IN_LEN);
 
-	if (ctx->context_id == EFX_MCDI_RSS_CONTEXT_INVALID)
+	if (ctx->priv.context_id == EFX_MCDI_RSS_CONTEXT_INVALID)
 		return -ENOENT;
 
 	MCDI_SET_DWORD(inbuf, RSS_CONTEXT_GET_TABLE_IN_RSS_CONTEXT_ID,
-		       ctx->context_id);
+		       ctx->priv.context_id);
 	BUILD_BUG_ON(ARRAY_SIZE(ctx->rx_indir_table) !=
 		     MC_CMD_RSS_CONTEXT_GET_TABLE_OUT_INDIRECTION_TABLE_LEN);
 	rc = efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_GET_TABLE, inbuf, sizeof(inbuf),
@@ -2164,7 +2162,7 @@ int efx_mcdi_rx_pull_rss_context_config(struct efx_nic *efx,
 				RSS_CONTEXT_GET_TABLE_OUT_INDIRECTION_TABLE)[i];
 
 	MCDI_SET_DWORD(inbuf, RSS_CONTEXT_GET_KEY_IN_RSS_CONTEXT_ID,
-		       ctx->context_id);
+		       ctx->priv.context_id);
 	BUILD_BUG_ON(ARRAY_SIZE(ctx->rx_hash_key) !=
 		     MC_CMD_RSS_CONTEXT_SET_KEY_IN_TOEPLITZ_KEY_LEN);
 	rc = efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_GET_KEY, inbuf, sizeof(inbuf),
@@ -2186,35 +2184,42 @@ int efx_mcdi_rx_pull_rss_config(struct efx_nic *efx)
 {
 	int rc;
 
-	mutex_lock(&efx->rss_lock);
+	mutex_lock(&efx->net_dev->ethtool->rss_lock);
 	rc = efx_mcdi_rx_pull_rss_context_config(efx, &efx->rss_context);
-	mutex_unlock(&efx->rss_lock);
+	mutex_unlock(&efx->net_dev->ethtool->rss_lock);
 	return rc;
 }
 
 void efx_mcdi_rx_restore_rss_contexts(struct efx_nic *efx)
 {
 	struct efx_mcdi_filter_table *table = efx->filter_state;
-	struct efx_rss_context *ctx;
+	struct ethtool_rxfh_context *ctx;
+	unsigned long context;
 	int rc;
 
-	WARN_ON(!mutex_is_locked(&efx->rss_lock));
+	WARN_ON(!mutex_is_locked(&efx->net_dev->ethtool->rss_lock));
 
 	if (!table->must_restore_rss_contexts)
 		return;
 
-	list_for_each_entry(ctx, &efx->rss_context.list, list) {
+	xa_for_each(&efx->net_dev->ethtool->rss_ctx, context, ctx) {
+		struct efx_rss_context_priv *priv;
+		u32 *indir;
+		u8 *key;
+
+		priv = ethtool_rxfh_context_priv(ctx);
 		/* previous NIC RSS context is gone */
-		ctx->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
+		priv->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
 		/* so try to allocate a new one */
-		rc = efx_mcdi_rx_push_rss_context_config(efx, ctx,
-							 ctx->rx_indir_table,
-							 ctx->rx_hash_key);
+		indir = ethtool_rxfh_context_indir(ctx);
+		key = ethtool_rxfh_context_key(ctx);
+		rc = efx_mcdi_rx_push_rss_context_config(efx, priv, indir, key,
+							 false);
 		if (rc)
 			netif_warn(efx, probe, efx->net_dev,
-				   "failed to restore RSS context %u, rc=%d"
+				   "failed to restore RSS context %lu, rc=%d"
 				   "; RSS filters may fail to be applied\n",
-				   ctx->user_id, rc);
+				   context, rc);
 	}
 	table->must_restore_rss_contexts = false;
 }
@@ -2276,7 +2281,7 @@ int efx_mcdi_vf_rx_push_rss_config(struct efx_nic *efx, bool user,
 {
 	if (user)
 		return -EOPNOTSUPP;
-	if (efx->rss_context.context_id != EFX_MCDI_RSS_CONTEXT_INVALID)
+	if (efx->rss_context.priv.context_id != EFX_MCDI_RSS_CONTEXT_INVALID)
 		return 0;
 	return efx_mcdi_filter_rx_push_shared_rss_config(efx, NULL);
 }
@@ -2295,7 +2300,7 @@ int efx_mcdi_push_default_indir_table(struct efx_nic *efx,
 
 	efx_mcdi_rx_free_indir_table(efx);
 	if (rss_spread > 1) {
-		efx_set_default_rx_indir_table(efx, &efx->rss_context);
+		efx_set_default_rx_indir_table(efx, efx->rss_context.rx_indir_table);
 		rc = efx->type->rx_push_rss_config(efx, false,
 				   efx->rss_context.rx_indir_table, NULL);
 	}
diff --git a/drivers/net/ethernet/sfc/mcdi_filters.h b/drivers/net/ethernet/sfc/mcdi_filters.h
index c0d6558b9fd2..11b9f87ed9e1 100644
--- a/drivers/net/ethernet/sfc/mcdi_filters.h
+++ b/drivers/net/ethernet/sfc/mcdi_filters.h
@@ -145,9 +145,9 @@ void efx_mcdi_filter_del_vlan(struct efx_nic *efx, u16 vid);
 
 void efx_mcdi_rx_free_indir_table(struct efx_nic *efx);
 int efx_mcdi_rx_push_rss_context_config(struct efx_nic *efx,
-					struct efx_rss_context *ctx,
+					struct efx_rss_context_priv *ctx,
 					const u32 *rx_indir_table,
-					const u8 *key);
+					const u8 *key, bool delete);
 int efx_mcdi_pf_rx_push_rss_config(struct efx_nic *efx, bool user,
 				   const u32 *rx_indir_table,
 				   const u8 *key);
@@ -161,10 +161,6 @@ int efx_mcdi_push_default_indir_table(struct efx_nic *efx,
 int efx_mcdi_rx_pull_rss_config(struct efx_nic *efx);
 int efx_mcdi_rx_pull_rss_context_config(struct efx_nic *efx,
 					struct efx_rss_context *ctx);
-int efx_mcdi_get_rss_context_flags(struct efx_nic *efx, u32 context,
-				   u32 *flags);
-void efx_mcdi_set_rss_context_flags(struct efx_nic *efx,
-				    struct efx_rss_context *ctx);
 void efx_mcdi_rx_restore_rss_contexts(struct efx_nic *efx);
 
 static inline void efx_mcdi_update_rx_scatter(struct efx_nic *efx)
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 27d86e90a3bb..d2a54a03f84d 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -737,21 +737,24 @@ struct vfdi_status;
 /* The reserved RSS context value */
 #define EFX_MCDI_RSS_CONTEXT_INVALID	0xffffffff
 /**
- * struct efx_rss_context - A user-defined RSS context for filtering
- * @list: node of linked list on which this struct is stored
+ * struct efx_rss_context_priv - driver private data for an RSS context
  * @context_id: the RSS_CONTEXT_ID returned by MC firmware, or
  *	%EFX_MCDI_RSS_CONTEXT_INVALID if this context is not present on the NIC.
- *	For Siena, 0 if RSS is active, else %EFX_MCDI_RSS_CONTEXT_INVALID.
- * @user_id: the rss_context ID exposed to userspace over ethtool.
  * @rx_hash_udp_4tuple: UDP 4-tuple hashing enabled
+ */
+struct efx_rss_context_priv {
+	u32 context_id;
+	bool rx_hash_udp_4tuple;
+};
+
+/**
+ * struct efx_rss_context - an RSS context
+ * @priv: hardware-specific state
  * @rx_hash_key: Toeplitz hash key for this RSS context
  * @indir_table: Indirection table for this RSS context
  */
 struct efx_rss_context {
-	struct list_head list;
-	u32 context_id;
-	u32 user_id;
-	bool rx_hash_udp_4tuple;
+	struct efx_rss_context_priv priv;
 	u8 rx_hash_key[40];
 	u32 rx_indir_table[128];
 };
@@ -883,9 +886,7 @@ struct efx_mae;
  * @rx_packet_ts_offset: Offset of timestamp from start of packet data
  *	(valid only if channel->sync_timestamps_enabled; always negative)
  * @rx_scatter: Scatter mode enabled for receives
- * @rss_context: Main RSS context.  Its @list member is the head of the list of
- *	RSS contexts created by user requests
- * @rss_lock: Protects custom RSS context software state in @rss_context.list
+ * @rss_context: Main RSS context.
  * @vport_id: The function's vport ID, only relevant for PFs
  * @int_error_count: Number of internal errors seen recently
  * @int_error_expire: Time at which error count will be expired
@@ -1052,7 +1053,6 @@ struct efx_nic {
 	int rx_packet_ts_offset;
 	bool rx_scatter;
 	struct efx_rss_context rss_context;
-	struct mutex rss_lock;
 	u32 vport_id;
 
 	unsigned int_error_count;
@@ -1416,9 +1416,9 @@ struct efx_nic_type {
 				  const u32 *rx_indir_table, const u8 *key);
 	int (*rx_pull_rss_config)(struct efx_nic *efx);
 	int (*rx_push_rss_context_config)(struct efx_nic *efx,
-					  struct efx_rss_context *ctx,
+					  struct efx_rss_context_priv *ctx,
 					  const u32 *rx_indir_table,
-					  const u8 *key);
+					  const u8 *key, bool delete);
 	int (*rx_pull_rss_context_config)(struct efx_nic *efx,
 					  struct efx_rss_context *ctx);
 	void (*rx_restore_rss_contexts)(struct efx_nic *efx);
diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
index d2f35ee15eff..c79cec9050c5 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -556,69 +556,25 @@ efx_rx_packet_gro(struct efx_channel *channel, struct efx_rx_buffer *rx_buf,
 	napi_gro_frags(napi);
 }
 
-/* RSS contexts.  We're using linked lists and crappy O(n) algorithms, because
- * (a) this is an infrequent control-plane operation and (b) n is small (max 64)
- */
-struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
+struct efx_rss_context_priv *efx_find_rss_context_entry(struct efx_nic *efx,
+							u32 id)
 {
-	struct list_head *head = &efx->rss_context.list;
-	struct efx_rss_context *ctx, *new;
-	u32 id = 1; /* Don't use zero, that refers to the master RSS context */
-
-	WARN_ON(!mutex_is_locked(&efx->rss_lock));
+	struct ethtool_rxfh_context *ctx;
 
-	/* Search for first gap in the numbering */
-	list_for_each_entry(ctx, head, list) {
-		if (ctx->user_id != id)
-			break;
-		id++;
-		/* Check for wrap.  If this happens, we have nearly 2^32
-		 * allocated RSS contexts, which seems unlikely.
-		 */
-		if (WARN_ON_ONCE(!id))
-			return NULL;
-	}
+	WARN_ON(!mutex_is_locked(&efx->net_dev->ethtool->rss_lock));
 
-	/* Create the new entry */
-	new = kmalloc(sizeof(*new), GFP_KERNEL);
-	if (!new)
+	ctx = xa_load(&efx->net_dev->ethtool->rss_ctx, id);
+	if (!ctx)
 		return NULL;
-	new->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
-	new->rx_hash_udp_4tuple = false;
-
-	/* Insert the new entry into the gap */
-	new->user_id = id;
-	list_add_tail(&new->list, &ctx->list);
-	return new;
-}
-
-struct efx_rss_context *efx_find_rss_context_entry(struct efx_nic *efx, u32 id)
-{
-	struct list_head *head = &efx->rss_context.list;
-	struct efx_rss_context *ctx;
-
-	WARN_ON(!mutex_is_locked(&efx->rss_lock));
-
-	list_for_each_entry(ctx, head, list)
-		if (ctx->user_id == id)
-			return ctx;
-	return NULL;
-}
-
-void efx_free_rss_context_entry(struct efx_rss_context *ctx)
-{
-	list_del(&ctx->list);
-	kfree(ctx);
+	return ethtool_rxfh_context_priv(ctx);
 }
 
-void efx_set_default_rx_indir_table(struct efx_nic *efx,
-				    struct efx_rss_context *ctx)
+void efx_set_default_rx_indir_table(struct efx_nic *efx, u32 *indir)
 {
 	size_t i;
 
-	for (i = 0; i < ARRAY_SIZE(ctx->rx_indir_table); i++)
-		ctx->rx_indir_table[i] =
-			ethtool_rxfh_indir_default(i, efx->rss_spread);
+	for (i = 0; i < ARRAY_SIZE(efx->rss_context.rx_indir_table); i++)
+		indir[i] = ethtool_rxfh_indir_default(i, efx->rss_spread);
 }
 
 /**
diff --git a/drivers/net/ethernet/sfc/rx_common.h b/drivers/net/ethernet/sfc/rx_common.h
index fbd2769307f9..75fa84192362 100644
--- a/drivers/net/ethernet/sfc/rx_common.h
+++ b/drivers/net/ethernet/sfc/rx_common.h
@@ -84,11 +84,9 @@ void
 efx_rx_packet_gro(struct efx_channel *channel, struct efx_rx_buffer *rx_buf,
 		  unsigned int n_frags, u8 *eh, __wsum csum);
 
-struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx);
-struct efx_rss_context *efx_find_rss_context_entry(struct efx_nic *efx, u32 id);
-void efx_free_rss_context_entry(struct efx_rss_context *ctx);
-void efx_set_default_rx_indir_table(struct efx_nic *efx,
-				    struct efx_rss_context *ctx);
+struct efx_rss_context_priv *efx_find_rss_context_entry(struct efx_nic *efx,
+							u32 id);
+void efx_set_default_rx_indir_table(struct efx_nic *efx, u32 *indir);
 
 bool efx_filter_is_mc_recipient(const struct efx_filter_spec *spec);
 bool efx_filter_spec_equal(const struct efx_filter_spec *left,

Re: [PATCH v4 net-next 1/7] net: move ethtool-related netdev state into its own struct

[Jacob Keller]

On 9/27/2023 11:13 AM, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> net_dev->ethtool is a pointer to new struct ethtool_netdev_state, which
>  currently contains only the wol_enabled field.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---

Good suggestion, nice to consolidate these bits I think.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[Jacob Keller]

On 9/27/2023 11:13 AM, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Each context stores the RXFH settings (indir, key, and hfunc) as well
>  as optionally some driver private data.
> Delete any still-existing contexts at netdev unregister time.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  include/linux/ethtool.h | 43 ++++++++++++++++++++++++++++++++++++++++-
>  net/core/dev.c          | 25 ++++++++++++++++++++++++
>  2 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 8aeefc0b4e10..bb11cb2f477d 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -157,6 +157,43 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>  	return index % n_rx_rings;
>  }
>  
> +/**
> + * struct ethtool_rxfh_context - a custom RSS context configuration
> + * @indir_size: Number of u32 entries in indirection table
> + * @key_size: Size of hash key, in bytes
> + * @hfunc: RSS hash function identifier.  One of the %ETH_RSS_HASH_*
> + * @priv_size: Size of driver private data, in bytes
> + * @indir_no_change: indir was not specified at create time
> + * @key_no_change: hkey was not specified at create time
> + */
> +struct ethtool_rxfh_context {
> +	u32 indir_size;
> +	u32 key_size;
> +	u8 hfunc;
> +	u16 priv_size;
> +	u8 indir_no_change:1;
> +	u8 key_no_change:1;
> +	/* private: driver private data, indirection table, and hash key are
> +	 * stored sequentially in @data area.  Use below helpers to access.
> +	 */
> +	u8 data[] __aligned(sizeof(void *));
> +};

Is it not feasible to use container_of to get to private data for the
drivers? I guess this change in particular doesn't actually include any
users yet...

Re: [PATCH v4 net-next 3/7] net: ethtool: record custom RSS contexts in the XArray

[Jacob Keller]

On 9/27/2023 11:13 AM, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Since drivers are still choosing the context IDs, we have to force the
>  XArray to use the ID they've chosen rather than picking one ourselves,
>  and handle the case where they give us an ID that's already in use.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  include/linux/ethtool.h | 14 ++++++++
>  net/ethtool/ioctl.c     | 73 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index bb11cb2f477d..229a23571008 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -194,6 +194,17 @@ static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
>  	return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size);
>  }
>  
> +static inline size_t ethtool_rxfh_context_size(u32 indir_size, u32 key_size,
> +					       u16 priv_size)
> +{
> +	size_t indir_bytes = array_size(indir_size, sizeof(u32));
> +	size_t flex_len;
> +
> +	flex_len = size_add(size_add(indir_bytes, key_size),
> +			    ALIGN(priv_size, sizeof(u32)));
> +	return struct_size((struct ethtool_rxfh_context *)0, data, flex_len);
> +}
> +

Ah, we already have to use a flexible area to handle the indirection
table, and this way the core is the only one responsible for allocating
and freeing the memory.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  /* declare a link mode bitmap */
>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
> @@ -731,6 +742,8 @@ struct ethtool_mm_stats {
>   *	will remain unchanged.
>   *	Returns a negative error code or zero. An error code must be returned
>   *	if at least one unsupported change was requested.
> + * @rxfh_priv_size: size of the driver private data area the core should
> + *	allocate for an RSS context.
>   * @get_rxfh_context: Get the contents of the RX flow hash indirection table,
>   *	hash key, and/or hash function assiciated to the given rss context.
>   *	Returns a negative error code or zero.
> @@ -824,6 +837,7 @@ struct ethtool_ops {
>  	u32     cap_link_lanes_supported:1;
>  	u32	supported_coalesce_params;
>  	u32	supported_ring_params;
> +	u16	rxfh_priv_size;
>  	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
>  	int	(*get_regs_len)(struct net_device *);
>  	void	(*get_regs)(struct net_device *, struct ethtool_regs *, void *);
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index de78b24fffc9..1d13bc8fbb75 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1249,6 +1249,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  {
>  	int ret;
>  	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	struct ethtool_rxfh_context *ctx = NULL;
>  	struct ethtool_rxnfc rx_rings;
>  	struct ethtool_rxfh rxfh;
>  	u32 dev_indir_size = 0, dev_key_size = 0, i;
> @@ -1256,7 +1257,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	u8 *hkey = NULL;
>  	u8 *rss_config;
>  	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
> -	bool delete = false;
> +	bool create = false, delete = false;
>  
>  	if (!ops->get_rxnfc || !ops->set_rxfh)
>  		return -EOPNOTSUPP;
> @@ -1275,6 +1276,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	/* Most drivers don't handle rss_context, check it's 0 as well */
>  	if (rxfh.rss_context && !ops->set_rxfh_context)
>  		return -EOPNOTSUPP;
> +	create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
>  
>  	/* If either indir, hash key or function is valid, proceed further.
>  	 * Must request at least one change: indir size, hash key or function.
> @@ -1332,13 +1334,42 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  	}
>  
> +	if (create) {
> +		if (delete) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		ctx = kzalloc(ethtool_rxfh_context_size(dev_indir_size,
> +							dev_key_size,
> +							ops->rxfh_priv_size),
> +			      GFP_KERNEL_ACCOUNT);
> +		if (!ctx) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		ctx->indir_size = dev_indir_size;
> +		ctx->key_size = dev_key_size;
> +		ctx->hfunc = rxfh.hfunc;
> +		ctx->priv_size = ops->rxfh_priv_size;
> +	} else if (rxfh.rss_context) {
> +		ctx = xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context);
> +		if (!ctx) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +	}
> +
>  	if (rxfh.rss_context)
>  		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
>  					    &rxfh.rss_context, delete);
>  	else
>  		ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
> -	if (ret)
> +	if (ret) {
> +		if (create)
> +			/* failed to create, free our new tracking entry */
> +			kfree(ctx);
>  		goto out;
> +	}
>  
>  	if (copy_to_user(useraddr + offsetof(struct ethtool_rxfh, rss_context),
>  			 &rxfh.rss_context, sizeof(rxfh.rss_context)))
> @@ -1351,6 +1382,44 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		else if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
>  			dev->priv_flags |= IFF_RXFH_CONFIGURED;
>  	}
> +	/* Update rss_ctx tracking */
> +	if (create) {
> +		/* Ideally this should happen before calling the driver,
> +		 * so that we can fail more cleanly; but we don't have the
> +		 * context ID until the driver picks it, so we have to
> +		 * wait until after.
> +		 */
> +		if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
> +			/* context ID reused, our tracking is screwed */
> +			kfree(ctx);
> +			goto out;
> +		}
> +		/* Allocate the exact ID the driver gave us */
> +		if (xa_is_err(xa_store(&dev->ethtool->rss_ctx, rxfh.rss_context,
> +				       ctx, GFP_KERNEL))) {
> +			kfree(ctx);
> +			goto out;
> +		}
> +		ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
> +		ctx->key_no_change = !rxfh.key_size;
> +	}
> +	if (delete) {
> +		WARN_ON(xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);
> +		kfree(ctx);
> +	} else if (ctx) {
> +		if (indir) {
> +			for (i = 0; i < dev_indir_size; i++)
> +				ethtool_rxfh_context_indir(ctx)[i] = indir[i];
> +			ctx->indir_no_change = 0;
> +		}
> +		if (hkey) {
> +			memcpy(ethtool_rxfh_context_key(ctx), hkey,
> +			       dev_key_size);
> +			ctx->key_no_change = 0;
> +		}
> +		if (rxfh.hfunc != ETH_RSS_HASH_NO_CHANGE)
> +			ctx->hfunc = rxfh.hfunc;
> +	}
>  
>  out:
>  	kfree(rss_config);
> 

Re: [PATCH v4 net-next 4/7] net: ethtool: let the core choose RSS context IDs

[Jacob Keller]

On 9/27/2023 11:13 AM, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Add a new API to create/modify/remove RSS contexts, that passes in the
>  newly-chosen context ID (not as a pointer) rather than leaving the
>  driver to choose it on create.  Also pass in the ctx, allowing drivers
>  to easily use its private data area to store their hardware-specific
>  state.
> Keep the existing .set_rxfh_context API for now as a fallback, but
>  deprecate it.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
>  include/linux/ethtool.h | 40 ++++++++++++++++++++++++---
>  net/core/dev.c          | 15 ++++++++---
>  net/ethtool/ioctl.c     | 60 ++++++++++++++++++++++++++++++-----------
>  3 files changed, 93 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 229a23571008..975fda7218f8 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -747,10 +747,33 @@ struct ethtool_mm_stats {
>   * @get_rxfh_context: Get the contents of the RX flow hash indirection table,
>   *	hash key, and/or hash function assiciated to the given rss context.
>   *	Returns a negative error code or zero.
> - * @set_rxfh_context: Create, remove and configure RSS contexts. Allows setting
> + * @create_rxfh_context: Create a new RSS context with the specified RX flow
> + *	hash indirection table, hash key, and hash function.
> + *	Arguments which are set to %NULL or zero will be populated to
> + *	appropriate defaults by the driver.
> + *	The &struct ethtool_rxfh_context for this context is passed in @ctx;
> + *	note that the indir table, hkey and hfunc are not yet populated as
> + *	of this call.  The driver does not need to update these; the core
> + *	will do so if this op succeeds.
> + *	If the driver provides this method, it must also provide
> + *	@modify_rxfh_context and @remove_rxfh_context.
> + *	Returns a negative error code or zero.
> + * @modify_rxfh_context: Reconfigure the specified RSS context.  Allows setting
>   *	the contents of the RX flow hash indirection table, hash key, and/or
> - *	hash function associated to the given context. Arguments which are set
> - *	to %NULL or zero will remain unchanged.
> + *	hash function associated with the given context.
> + *	Arguments which are set to %NULL or zero will remain unchanged.
> + *	The &struct ethtool_rxfh_context for this context is passed in @ctx;
> + *	note that it will still contain the *old* settings.  The driver does
> + *	not need to update these; the core will do so if this op succeeds.
> + *	Returns a negative error code or zero. An error code must be returned
> + *	if at least one unsupported change was requested.
> + * @remove_rxfh_context: Remove the specified RSS context.
> + *	The &struct ethtool_rxfh_context for this context is passed in @ctx.
> + *	Returns a negative error code or zero.
> + * @set_rxfh_context: Deprecated API to create, remove and configure RSS
> + *	contexts. Allows setting the contents of the RX flow hash indirection
> + *	table, hash key, and/or hash function associated to the given context.
> + *	Arguments which are set to %NULL or zero will remain unchanged.
>   *	Returns a negative error code or zero. An error code must be returned
>   *	if at least one unsupported change was requested.
>   * @get_channels: Get number of channels.
> @@ -901,6 +924,17 @@ struct ethtool_ops {
>  			    const u8 *key, const u8 hfunc);
>  	int	(*get_rxfh_context)(struct net_device *, u32 *indir, u8 *key,
>  				    u8 *hfunc, u32 rss_context);
> +	int	(*create_rxfh_context)(struct net_device *,
> +				       struct ethtool_rxfh_context *ctx,
> +				       const u32 *indir, const u8 *key,
> +				       const u8 hfunc, u32 rss_context);
> +	int	(*modify_rxfh_context)(struct net_device *,
> +				       struct ethtool_rxfh_context *ctx,
> +				       const u32 *indir, const u8 *key,
> +				       const u8 hfunc, u32 rss_context);
> +	int	(*remove_rxfh_context)(struct net_device *,
> +				       struct ethtool_rxfh_context *ctx,
> +				       u32 rss_context);
>  	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
>  				    const u8 *key, const u8 hfunc,
>  				    u32 *rss_context, bool delete);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 05e95abdfd17..637218adca22 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10882,16 +10882,23 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  	struct ethtool_rxfh_context *ctx;
>  	unsigned long context;
>  
> -	if (dev->ethtool_ops->set_rxfh_context)
> +	if (dev->ethtool_ops->create_rxfh_context ||
> +	    dev->ethtool_ops->set_rxfh_context)
>  		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
>  			u32 *indir = ethtool_rxfh_context_indir(ctx);
>  			u8 *key = ethtool_rxfh_context_key(ctx);
>  			u32 concast = context;
>  
>  			xa_erase(&dev->ethtool->rss_ctx, context);
> -			dev->ethtool_ops->set_rxfh_context(dev, indir, key,
> -							   ctx->hfunc, &concast,
> -							   true);
> +			if (dev->ethtool_ops->create_rxfh_context)
> +				dev->ethtool_ops->remove_rxfh_context(dev, ctx,
> +								      context);

A bit weird here, but the idea is that you must support both create,
remove, and modify, so we only check the presence of "create". Ok.

> +			else
> +				dev->ethtool_ops->set_rxfh_context(dev, indir,
> +								   key,
> +								   ctx->hfunc,
> +								   &concast,
> +								   true);
>  			kfree(ctx);
>  		}
>  	xa_destroy(&dev->ethtool->rss_ctx);
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 1d13bc8fbb75..c23d2bd3cd2a 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1274,7 +1274,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd8[2] || rxfh.rsvd32)
>  		return -EINVAL;
>  	/* Most drivers don't handle rss_context, check it's 0 as well */
> -	if (rxfh.rss_context && !ops->set_rxfh_context)
> +	if (rxfh.rss_context && !(ops->create_rxfh_context ||
> +				  ops->set_rxfh_context))
>  		return -EOPNOTSUPP;
>  	create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
>  
> @@ -1349,8 +1350,24 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  		ctx->indir_size = dev_indir_size;
>  		ctx->key_size = dev_key_size;
> -		ctx->hfunc = rxfh.hfunc;
>  		ctx->priv_size = ops->rxfh_priv_size;
> +		/* Initialise to an empty context */
> +		ctx->indir_no_change = ctx->key_no_change = 1;
> +		ctx->hfunc = ETH_RSS_HASH_NO_CHANGE;
> +		if (ops->create_rxfh_context) {
> +			u32 limit = dev->ethtool->rss_ctx_max_id ?: U32_MAX;
> +			u32 ctx_id;
> +
> +			/* driver uses new API, core allocates ID */
> +			ret = xa_alloc(&dev->ethtool->rss_ctx, &ctx_id, ctx,
> +				       XA_LIMIT(1, limit), GFP_KERNEL_ACCOUNT);
> +			if (ret < 0) {
> +				kfree(ctx);
> +				goto out;
> +			}
> +			WARN_ON(!ctx_id); /* can't happen */
> +			rxfh.rss_context = ctx_id;
> +		}
>  	} else if (rxfh.rss_context) {
>  		ctx = xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context);
>  		if (!ctx) {
> @@ -1359,15 +1376,34 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  	}
>  
> -	if (rxfh.rss_context)
> -		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
> -					    &rxfh.rss_context, delete);
> -	else
> +	if (rxfh.rss_context) {
> +		if (ops->create_rxfh_context) {
> +			if (create)
> +				ret = ops->create_rxfh_context(dev, ctx, indir,
> +							       hkey, rxfh.hfunc,
> +							       rxfh.rss_context);
> +			else if (delete)
> +				ret = ops->remove_rxfh_context(dev, ctx,
> +							       rxfh.rss_context);
> +			else
> +				ret = ops->modify_rxfh_context(dev, ctx, indir,
> +							       hkey, rxfh.hfunc,
> +							       rxfh.rss_context);
> +		} else {
> +			ret = ops->set_rxfh_context(dev, indir, hkey,
> +						    rxfh.hfunc,
> +						    &rxfh.rss_context, delete);
> +		}
> +	} else {
>  		ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
> +	}
>  	if (ret) {
> -		if (create)
> +		if (create) {
>  			/* failed to create, free our new tracking entry */
> +			if (ops->create_rxfh_context)
> +				xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context);
>  			kfree(ctx);
> +		}
>  		goto out;
>  	}
>  
> @@ -1383,12 +1419,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  			dev->priv_flags |= IFF_RXFH_CONFIGURED;
>  	}
>  	/* Update rss_ctx tracking */
> -	if (create) {
> -		/* Ideally this should happen before calling the driver,
> -		 * so that we can fail more cleanly; but we don't have the
> -		 * context ID until the driver picks it, so we have to
> -		 * wait until after.
> -		 */
> +	if (create && !ops->create_rxfh_context) {
> +		/* driver uses old API, it chose context ID */
>  		if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
>  			/* context ID reused, our tracking is screwed */
>  			kfree(ctx);
> @@ -1400,8 +1432,6 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  			kfree(ctx);
>  			goto out;
>  		}
> -		ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
> -		ctx->key_no_change = !rxfh.key_size;
>  	}
>  	if (delete) {
>  		WARN_ON(xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);
> 

Re: [PATCH v4 net-next 5/7] net: ethtool: add an extack parameter to new rxfh_context APIs

[Jacob Keller]

On 9/27/2023 11:13 AM, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Currently passed as NULL, but will allow drivers to report back errors
>  when ethnl support for these ops is added.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---

Why not just squash this into the version that introduces the ops?

I guess this calls it out a bit better?

Thanks,
Jake

>  include/linux/ethtool.h | 9 ++++++---
>  net/core/dev.c          | 3 ++-
>  net/ethtool/ioctl.c     | 9 ++++++---
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 975fda7218f8..c8963bde9289 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -927,14 +927,17 @@ struct ethtool_ops {
>  	int	(*create_rxfh_context)(struct net_device *,
>  				       struct ethtool_rxfh_context *ctx,
>  				       const u32 *indir, const u8 *key,
> -				       const u8 hfunc, u32 rss_context);
> +				       const u8 hfunc, u32 rss_context,
> +				       struct netlink_ext_ack *extack);
>  	int	(*modify_rxfh_context)(struct net_device *,
>  				       struct ethtool_rxfh_context *ctx,
>  				       const u32 *indir, const u8 *key,
> -				       const u8 hfunc, u32 rss_context);
> +				       const u8 hfunc, u32 rss_context,
> +				       struct netlink_ext_ack *extack);
>  	int	(*remove_rxfh_context)(struct net_device *,
>  				       struct ethtool_rxfh_context *ctx,
> -				       u32 rss_context);
> +				       u32 rss_context,
> +				       struct netlink_ext_ack *extack);
>  	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
>  				    const u8 *key, const u8 hfunc,
>  				    u32 *rss_context, bool delete);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 637218adca22..69579d9cd7ba 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10892,7 +10892,8 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  			xa_erase(&dev->ethtool->rss_ctx, context);
>  			if (dev->ethtool_ops->create_rxfh_context)
>  				dev->ethtool_ops->remove_rxfh_context(dev, ctx,
> -								      context);
> +								      context,
> +								      NULL);
>  			else
>  				dev->ethtool_ops->set_rxfh_context(dev, indir,
>  								   key,
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index c23d2bd3cd2a..3920ddee3ee2 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1381,14 +1381,17 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  			if (create)
>  				ret = ops->create_rxfh_context(dev, ctx, indir,
>  							       hkey, rxfh.hfunc,
> -							       rxfh.rss_context);
> +							       rxfh.rss_context,
> +							       NULL);
>  			else if (delete)
>  				ret = ops->remove_rxfh_context(dev, ctx,
> -							       rxfh.rss_context);
> +							       rxfh.rss_context,
> +							       NULL);
>  			else
>  				ret = ops->modify_rxfh_context(dev, ctx, indir,
>  							       hkey, rxfh.hfunc,
> -							       rxfh.rss_context);
> +							       rxfh.rss_context,
> +							       NULL);
>  		} else {
>  			ret = ops->set_rxfh_context(dev, indir, hkey,
>  						    rxfh.hfunc,
> 

Re: [PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

[Jacob Keller]

On 9/27/2023 11:13 AM, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> While this is not needed to serialise the ethtool entry points (which
>  are all under RTNL), drivers may have cause to asynchronously access
>  dev->ethtool->rss_ctx; taking dev->ethtool->rss_lock allows them to
>  do this safely without needing to take the RTNL.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
>  include/linux/ethtool.h | 3 +++
>  net/core/dev.c          | 5 +++++
>  net/ethtool/ioctl.c     | 7 +++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index c8963bde9289..d15a21bd6f12 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -1026,11 +1026,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>  /**
>   * struct ethtool_netdev_state - per-netdevice state for ethtool features
>   * @rss_ctx:		XArray of custom RSS contexts
> + * @rss_lock:		Protects entries in @rss_ctx.  May be taken from
> + *			within RTNL.
>   * @rss_ctx_max_id:	maximum (exclusive) supported RSS context ID
>   * @wol_enabled:	Wake-on-LAN is enabled
>   */
>  struct ethtool_netdev_state {
>  	struct xarray		rss_ctx;
> +	struct mutex		rss_lock;
>  	u32			rss_ctx_max_id;
>  	u32			wol_enabled:1;
>  };
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 69579d9cd7ba..c57456ed4be8 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10074,6 +10074,7 @@ int register_netdevice(struct net_device *dev)
>  
>  	/* rss ctx ID 0 is reserved for the default context, start from 1 */
>  	xa_init_flags(&dev->ethtool->rss_ctx, XA_FLAGS_ALLOC1);
> +	mutex_init(&dev->ethtool->rss_lock);
>  
>  	spin_lock_init(&dev->addr_list_lock);
>  	netdev_set_addr_lockdep_class(dev);
> @@ -10882,6 +10883,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  	struct ethtool_rxfh_context *ctx;
>  	unsigned long context;
>  
> +	mutex_lock(&dev->ethtool->rss_lock);
>  	if (dev->ethtool_ops->create_rxfh_context ||
>  	    dev->ethtool_ops->set_rxfh_context)
>  		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
> @@ -10903,6 +10905,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  			kfree(ctx);
>  		}
>  	xa_destroy(&dev->ethtool->rss_ctx);
> +	mutex_unlock(&dev->ethtool->rss_lock);
>  }
>  
>  /**
> @@ -11016,6 +11019,8 @@ void unregister_netdevice_many_notify(struct list_head *head,
>  		if (dev->netdev_ops->ndo_uninit)
>  			dev->netdev_ops->ndo_uninit(dev);
>  
> +		mutex_destroy(&dev->ethtool->rss_lock);
> +
>  		if (skb)
>  			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, portid, nlh);
>  
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 3920ddee3ee2..d21bbc92e6fc 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1258,6 +1258,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	u8 *rss_config;
>  	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
>  	bool create = false, delete = false;
> +	bool locked = false; /* dev->ethtool->rss_lock taken */
>  
>  	if (!ops->get_rxnfc || !ops->set_rxfh)
>  		return -EOPNOTSUPP;
> @@ -1335,6 +1336,10 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  	}
>  
> +	if (rxfh.rss_context) {
> +		mutex_lock(&dev->ethtool->rss_lock);
> +		locked = true;
> +	}
>  	if (create) {
>  		if (delete) {
>  			ret = -EINVAL;
> @@ -1455,6 +1460,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	}
>  
>  out:
> +	if (locked)
> +		mutex_unlock(&dev->ethtool->rss_lock);
>  	kfree(rss_config);
>  	return ret;
>  }
> 

Re: [PATCH v4 net-next 7/7] sfc: use new rxfh_context API

[Jacob Keller]

On 9/27/2023 11:13 AM, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> The core is now responsible for allocating IDs and a memory region for
>  us to store our state (struct efx_rss_context_priv), so we no longer
>  need efx_alloc_rss_context_entry() and friends.
> Since the contexts are now maintained by the core, use the core's lock
>  (net_dev->ethtool->rss_lock), rather than our own mutex (efx->rss_lock),
>  to serialise access against changes; and remove the now-unused
>  efx->rss_lock from struct efx_nic.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

RE: [PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[Mogilappagari, Sudheer]

> -----Original Message-----
> From: edward.cree@amd.com <edward.cree@amd.com>
> Sent: Wednesday, September 27, 2023 11:14 AM
> 
> +/**
> + * struct ethtool_rxfh_context - a custom RSS context configuration
> + * @indir_size: Number of u32 entries in indirection table
> + * @key_size: Size of hash key, in bytes
> + * @hfunc: RSS hash function identifier.  One of the %ETH_RSS_HASH_*
> + * @priv_size: Size of driver private data, in bytes
> + * @indir_no_change: indir was not specified at create time
> + * @key_no_change: hkey was not specified at create time  */ struct
> +ethtool_rxfh_context {

nit: */ and struct definition alignment.  

Re: [PATCH v4 net-next 1/7] net: move ethtool-related netdev state into its own struct

[Martin Habets]

On Wed, Sep 27, 2023 at 07:13:32PM +0100, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> net_dev->ethtool is a pointer to new struct ethtool_netdev_state, which
>  currently contains only the wol_enabled field.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

One small nit below.

Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>

> ---
>  drivers/net/ethernet/realtek/r8169_main.c        | 4 ++--
>  drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c | 4 ++--
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c    | 2 +-
>  drivers/net/phy/phy.c                            | 2 +-
>  drivers/net/phy/phy_device.c                     | 5 +++--
>  drivers/net/phy/phylink.c                        | 2 +-
>  include/linux/ethtool.h                          | 8 ++++++++
>  include/linux/netdevice.h                        | 7 ++++---
>  net/core/dev.c                                   | 4 ++++
>  net/ethtool/ioctl.c                              | 2 +-
>  net/ethtool/wol.c                                | 2 +-
>  11 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 6351a2dc13bc..fe69416f2a93 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -1455,7 +1455,7 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)
>  
>  	if (tp->dash_type == RTL_DASH_NONE) {
>  		rtl_set_d3_pll_down(tp, !wolopts);
> -		tp->dev->wol_enabled = wolopts ? 1 : 0;
> +		tp->dev->ethtool->wol_enabled = wolopts ? 1 : 0;
>  	}
>  }
>  
> @@ -5321,7 +5321,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		rtl_set_d3_pll_down(tp, true);
>  	} else {
>  		rtl_set_d3_pll_down(tp, false);
> -		dev->wol_enabled = 1;
> +		dev->ethtool->wol_enabled = 1;
>  	}
>  
>  	jumbo_max = rtl_jumbo_max(tp);
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> index ec0e869e9aac..091ee3d3e74d 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> @@ -34,9 +34,9 @@ static int ngbe_set_wol(struct net_device *netdev,
>  	wx->wol = 0;
>  	if (wol->wolopts & WAKE_MAGIC)
>  		wx->wol = WX_PSR_WKUP_CTL_MAG;
> -	netdev->wol_enabled = !!(wx->wol);
> +	netdev->ethtool->wol_enabled = !!(wx->wol);
>  	wr32(wx, WX_PSR_WKUP_CTL, wx->wol);
> -	device_set_wakeup_enable(&pdev->dev, netdev->wol_enabled);
> +	device_set_wakeup_enable(&pdev->dev, netdev->ethtool->wol_enabled);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 2b431db6085a..6752f8d04d9c 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -636,7 +636,7 @@ static int ngbe_probe(struct pci_dev *pdev,
>  	if (wx->wol_hw_supported)
>  		wx->wol = NGBE_PSR_WKUP_CTL_MAG;
>  
> -	netdev->wol_enabled = !!(wx->wol);
> +	netdev->ethtool->wol_enabled = !!(wx->wol);
>  	wr32(wx, NGBE_PSR_WKUP_CTL, wx->wol);
>  	device_set_wakeup_enable(&pdev->dev, wx->wol);
>  
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index a5fa077650e8..32bfea9b5c6c 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1282,7 +1282,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>  		if (netdev) {
>  			struct device *parent = netdev->dev.parent;
>  
> -			if (netdev->wol_enabled)
> +			if (netdev->ethtool->wol_enabled)
>  				pm_system_wakeup();
>  			else if (device_may_wakeup(&netdev->dev))
>  				pm_wakeup_dev_event(&netdev->dev, 0, true);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 2ce74593d6e4..62afc0424fbd 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -285,7 +285,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
>  	if (!netdev)
>  		goto out;
>  
> -	if (netdev->wol_enabled)
> +	if (netdev->ethtool->wol_enabled)
>  		return false;
>  
>  	/* As long as not all affected network drivers support the
> @@ -1859,7 +1859,8 @@ int phy_suspend(struct phy_device *phydev)
>  		return 0;
>  
>  	phy_ethtool_get_wol(phydev, &wol);
> -	phydev->wol_enabled = wol.wolopts || (netdev && netdev->wol_enabled);
> +	phydev->wol_enabled = wol.wolopts ||
> +			      (netdev && netdev->ethtool->wol_enabled);
>  	/* If the device has WOL enabled, we cannot suspend the PHY */
>  	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
>  		return -EBUSY;
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 0d7354955d62..b808ba1197c3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2172,7 +2172,7 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
>  {
>  	ASSERT_RTNL();
>  
> -	if (mac_wol && (!pl->netdev || pl->netdev->wol_enabled)) {
> +	if (mac_wol && (!pl->netdev || pl->netdev->ethtool->wol_enabled)) {
>  		/* Wake-on-Lan enabled, MAC handling */
>  		mutex_lock(&pl->state_mutex);
>  
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 62b61527bcc4..8aeefc0b4e10 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -935,6 +935,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>  				       const struct ethtool_link_ksettings *cmd,
>  				       u32 *dev_speed, u8 *dev_duplex);
>  
> +/**
> + * struct ethtool_netdev_state - per-netdevice state for ethtool features
> + * @wol_enabled:	Wake-on-LAN is enabled
> + */
> +struct ethtool_netdev_state {
> +	unsigned		wol_enabled:1;

The use of bool seems to be quite well established in the kernel these
days. I suggest you use that.

Martin

> +};
> +
>  struct phy_device;
>  struct phy_tdr_config;
>  struct phy_plca_cfg;
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7e520c14eb8c..05ea6cb56800 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -79,6 +79,7 @@ struct xdp_buff;
>  struct xdp_frame;
>  struct xdp_metadata_ops;
>  struct xdp_md;
> +struct ethtool_netdev_state;
>  /* DPLL specific */
>  struct dpll_pin;
>  
> @@ -2014,8 +2015,6 @@ enum netdev_ml_priv_type {
>   *			switch driver and used to set the phys state of the
>   *			switch port.
>   *
> - *	@wol_enabled:	Wake-on-LAN is enabled
> - *
>   *	@threaded:	napi threaded mode is enabled
>   *
>   *	@net_notifier_list:	List of per-net netdev notifier block
> @@ -2027,6 +2026,7 @@ enum netdev_ml_priv_type {
>   *	@udp_tunnel_nic_info:	static structure describing the UDP tunnel
>   *				offload capabilities of the device
>   *	@udp_tunnel_nic:	UDP tunnel offload state
> + *	@ethtool:	ethtool related state
>   *	@xdp_state:		stores info on attached XDP BPF programs
>   *
>   *	@nested_level:	Used as a parameter of spin_lock_nested() of
> @@ -2389,7 +2389,6 @@ struct net_device {
>  	struct sfp_bus		*sfp_bus;
>  	struct lock_class_key	*qdisc_tx_busylock;
>  	bool			proto_down;
> -	unsigned		wol_enabled:1;
>  	unsigned		threaded:1;
>  
>  	struct list_head	net_notifier_list;
> @@ -2401,6 +2400,8 @@ struct net_device {
>  	const struct udp_tunnel_nic_info	*udp_tunnel_nic_info;
>  	struct udp_tunnel_nic	*udp_tunnel_nic;
>  
> +	struct ethtool_netdev_state *ethtool;
> +
>  	/* protected by rtnl_lock */
>  	struct bpf_xdp_entity	xdp_state[__MAX_XDP_MODE];
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 606a366cc209..9e85a71e33ed 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10769,6 +10769,9 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>  	dev->real_num_rx_queues = rxqs;
>  	if (netif_alloc_rx_queues(dev))
>  		goto free_all;
> +	dev->ethtool = kzalloc(sizeof(*dev->ethtool), GFP_KERNEL_ACCOUNT);
> +	if (!dev->ethtool)
> +		goto free_all;
>  
>  	strcpy(dev->name, name);
>  	dev->name_assign_type = name_assign_type;
> @@ -10819,6 +10822,7 @@ void free_netdev(struct net_device *dev)
>  		return;
>  	}
>  
> +	kfree(dev->ethtool);
>  	netif_free_tx_queues(dev);
>  	netif_free_rx_queues(dev);
>  
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 0b0ce4f81c01..de78b24fffc9 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1461,7 +1461,7 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
>  	if (ret)
>  		return ret;
>  
> -	dev->wol_enabled = !!wol.wolopts;
> +	dev->ethtool->wol_enabled = !!wol.wolopts;
>  	ethtool_notify(dev, ETHTOOL_MSG_WOL_NTF, NULL);
>  
>  	return 0;
> diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
> index 0ed56c9ac1bc..a39d8000d808 100644
> --- a/net/ethtool/wol.c
> +++ b/net/ethtool/wol.c
> @@ -137,7 +137,7 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info)
>  	ret = dev->ethtool_ops->set_wol(dev, &wol);
>  	if (ret)
>  		return ret;
> -	dev->wol_enabled = !!wol.wolopts;
> +	dev->ethtool->wol_enabled = !!wol.wolopts;
>  	return 1;
>  }
>  

Re: [PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[Martin Habets]

On Wed, Sep 27, 2023 at 07:13:33PM +0100, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Each context stores the RXFH settings (indir, key, and hfunc) as well
>  as optionally some driver private data.
> Delete any still-existing contexts at netdev unregister time.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  include/linux/ethtool.h | 43 ++++++++++++++++++++++++++++++++++++++++-
>  net/core/dev.c          | 25 ++++++++++++++++++++++++
>  2 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 8aeefc0b4e10..bb11cb2f477d 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -157,6 +157,43 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>  	return index % n_rx_rings;
>  }
>  
> +/**
> + * struct ethtool_rxfh_context - a custom RSS context configuration
> + * @indir_size: Number of u32 entries in indirection table
> + * @key_size: Size of hash key, in bytes
> + * @hfunc: RSS hash function identifier.  One of the %ETH_RSS_HASH_*
> + * @priv_size: Size of driver private data, in bytes
> + * @indir_no_change: indir was not specified at create time
> + * @key_no_change: hkey was not specified at create time
> + */
> +struct ethtool_rxfh_context {
> +	u32 indir_size;
> +	u32 key_size;
> +	u8 hfunc;
> +	u16 priv_size;
> +	u8 indir_no_change:1;
> +	u8 key_no_change:1;

On 32-bit architectures this has a hole after hfunc and 3 empty bytes here.
Move these 2 1-bit fields before priv_size to avoid that.

Martin

> +	/* private: driver private data, indirection table, and hash key are
> +	 * stored sequentially in @data area.  Use below helpers to access.
> +	 */
> +	u8 data[] __aligned(sizeof(void *));
> +};
> +
> +static inline void *ethtool_rxfh_context_priv(struct ethtool_rxfh_context *ctx)
> +{
> +	return ctx->data;
> +}
> +
> +static inline u32 *ethtool_rxfh_context_indir(struct ethtool_rxfh_context *ctx)
> +{
> +	return (u32 *)(ctx->data + ALIGN(ctx->priv_size, sizeof(u32)));
> +}
> +
> +static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
> +{
> +	return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size);
> +}
> +
>  /* declare a link mode bitmap */
>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
> @@ -937,10 +974,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>  
>  /**
>   * struct ethtool_netdev_state - per-netdevice state for ethtool features
> + * @rss_ctx:		XArray of custom RSS contexts
> + * @rss_ctx_max_id:	maximum (exclusive) supported RSS context ID
>   * @wol_enabled:	Wake-on-LAN is enabled
>   */
>  struct ethtool_netdev_state {
> -	unsigned		wol_enabled:1;
> +	struct xarray		rss_ctx;
> +	u32			rss_ctx_max_id;
> +	u32			wol_enabled:1;
>  };
>  
>  struct phy_device;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 9e85a71e33ed..05e95abdfd17 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10072,6 +10072,9 @@ int register_netdevice(struct net_device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* rss ctx ID 0 is reserved for the default context, start from 1 */
> +	xa_init_flags(&dev->ethtool->rss_ctx, XA_FLAGS_ALLOC1);
> +
>  	spin_lock_init(&dev->addr_list_lock);
>  	netdev_set_addr_lockdep_class(dev);
>  
> @@ -10874,6 +10877,26 @@ void synchronize_net(void)
>  }
>  EXPORT_SYMBOL(synchronize_net);
>  
> +static void netdev_rss_contexts_free(struct net_device *dev)
> +{
> +	struct ethtool_rxfh_context *ctx;
> +	unsigned long context;
> +
> +	if (dev->ethtool_ops->set_rxfh_context)
> +		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
> +			u32 *indir = ethtool_rxfh_context_indir(ctx);
> +			u8 *key = ethtool_rxfh_context_key(ctx);
> +			u32 concast = context;
> +
> +			xa_erase(&dev->ethtool->rss_ctx, context);
> +			dev->ethtool_ops->set_rxfh_context(dev, indir, key,
> +							   ctx->hfunc, &concast,
> +							   true);
> +			kfree(ctx);
> +		}
> +	xa_destroy(&dev->ethtool->rss_ctx);
> +}
> +
>  /**
>   *	unregister_netdevice_queue - remove device from the kernel
>   *	@dev: device
> @@ -10978,6 +11001,8 @@ void unregister_netdevice_many_notify(struct list_head *head,
>  		netdev_name_node_alt_flush(dev);
>  		netdev_name_node_free(dev->name_node);
>  
> +		netdev_rss_contexts_free(dev);
> +
>  		call_netdevice_notifiers(NETDEV_PRE_UNINIT, dev);
>  
>  		if (dev->netdev_ops->ndo_uninit)

Re: [PATCH v4 net-next 3/7] net: ethtool: record custom RSS contexts in the XArray

[Martin Habets]

On Wed, Sep 27, 2023 at 07:13:34PM +0100, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Since drivers are still choosing the context IDs, we have to force the
>  XArray to use the ID they've chosen rather than picking one ourselves,
>  and handle the case where they give us an ID that's already in use.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  include/linux/ethtool.h | 14 ++++++++
>  net/ethtool/ioctl.c     | 73 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index bb11cb2f477d..229a23571008 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -194,6 +194,17 @@ static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
>  	return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size);
>  }
>  
> +static inline size_t ethtool_rxfh_context_size(u32 indir_size, u32 key_size,
> +					       u16 priv_size)
> +{
> +	size_t indir_bytes = array_size(indir_size, sizeof(u32));
> +	size_t flex_len;
> +
> +	flex_len = size_add(size_add(indir_bytes, key_size),
> +			    ALIGN(priv_size, sizeof(u32)));
> +	return struct_size((struct ethtool_rxfh_context *)0, data, flex_len);
> +}
> +
>  /* declare a link mode bitmap */
>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
> @@ -731,6 +742,8 @@ struct ethtool_mm_stats {
>   *	will remain unchanged.
>   *	Returns a negative error code or zero. An error code must be returned
>   *	if at least one unsupported change was requested.
> + * @rxfh_priv_size: size of the driver private data area the core should
> + *	allocate for an RSS context.

An odd place to push the documentation. Please keep the ordering the same
as the struct has below.

Martin

>   * @get_rxfh_context: Get the contents of the RX flow hash indirection table,
>   *	hash key, and/or hash function assiciated to the given rss context.
>   *	Returns a negative error code or zero.
> @@ -824,6 +837,7 @@ struct ethtool_ops {
>  	u32     cap_link_lanes_supported:1;
>  	u32	supported_coalesce_params;
>  	u32	supported_ring_params;
> +	u16	rxfh_priv_size;
>  	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
>  	int	(*get_regs_len)(struct net_device *);
>  	void	(*get_regs)(struct net_device *, struct ethtool_regs *, void *);
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index de78b24fffc9..1d13bc8fbb75 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1249,6 +1249,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  {
>  	int ret;
>  	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	struct ethtool_rxfh_context *ctx = NULL;
>  	struct ethtool_rxnfc rx_rings;
>  	struct ethtool_rxfh rxfh;
>  	u32 dev_indir_size = 0, dev_key_size = 0, i;
> @@ -1256,7 +1257,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	u8 *hkey = NULL;
>  	u8 *rss_config;
>  	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
> -	bool delete = false;
> +	bool create = false, delete = false;
>  
>  	if (!ops->get_rxnfc || !ops->set_rxfh)
>  		return -EOPNOTSUPP;
> @@ -1275,6 +1276,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	/* Most drivers don't handle rss_context, check it's 0 as well */
>  	if (rxfh.rss_context && !ops->set_rxfh_context)
>  		return -EOPNOTSUPP;
> +	create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
>  
>  	/* If either indir, hash key or function is valid, proceed further.
>  	 * Must request at least one change: indir size, hash key or function.
> @@ -1332,13 +1334,42 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  	}
>  
> +	if (create) {
> +		if (delete) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		ctx = kzalloc(ethtool_rxfh_context_size(dev_indir_size,
> +							dev_key_size,
> +							ops->rxfh_priv_size),
> +			      GFP_KERNEL_ACCOUNT);
> +		if (!ctx) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		ctx->indir_size = dev_indir_size;
> +		ctx->key_size = dev_key_size;
> +		ctx->hfunc = rxfh.hfunc;
> +		ctx->priv_size = ops->rxfh_priv_size;
> +	} else if (rxfh.rss_context) {
> +		ctx = xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context);
> +		if (!ctx) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +	}
> +
>  	if (rxfh.rss_context)
>  		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
>  					    &rxfh.rss_context, delete);
>  	else
>  		ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
> -	if (ret)
> +	if (ret) {
> +		if (create)
> +			/* failed to create, free our new tracking entry */
> +			kfree(ctx);
>  		goto out;
> +	}
>  
>  	if (copy_to_user(useraddr + offsetof(struct ethtool_rxfh, rss_context),
>  			 &rxfh.rss_context, sizeof(rxfh.rss_context)))
> @@ -1351,6 +1382,44 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		else if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
>  			dev->priv_flags |= IFF_RXFH_CONFIGURED;
>  	}
> +	/* Update rss_ctx tracking */
> +	if (create) {
> +		/* Ideally this should happen before calling the driver,
> +		 * so that we can fail more cleanly; but we don't have the
> +		 * context ID until the driver picks it, so we have to
> +		 * wait until after.
> +		 */
> +		if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
> +			/* context ID reused, our tracking is screwed */
> +			kfree(ctx);
> +			goto out;
> +		}
> +		/* Allocate the exact ID the driver gave us */
> +		if (xa_is_err(xa_store(&dev->ethtool->rss_ctx, rxfh.rss_context,
> +				       ctx, GFP_KERNEL))) {
> +			kfree(ctx);
> +			goto out;
> +		}
> +		ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
> +		ctx->key_no_change = !rxfh.key_size;
> +	}
> +	if (delete) {
> +		WARN_ON(xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);
> +		kfree(ctx);
> +	} else if (ctx) {
> +		if (indir) {
> +			for (i = 0; i < dev_indir_size; i++)
> +				ethtool_rxfh_context_indir(ctx)[i] = indir[i];
> +			ctx->indir_no_change = 0;
> +		}
> +		if (hkey) {
> +			memcpy(ethtool_rxfh_context_key(ctx), hkey,
> +			       dev_key_size);
> +			ctx->key_no_change = 0;
> +		}
> +		if (rxfh.hfunc != ETH_RSS_HASH_NO_CHANGE)
> +			ctx->hfunc = rxfh.hfunc;
> +	}
>  
>  out:
>  	kfree(rss_config);

Re: [PATCH v4 net-next 4/7] net: ethtool: let the core choose RSS context IDs

[Martin Habets]

On Wed, Sep 27, 2023 at 07:13:35PM +0100, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Add a new API to create/modify/remove RSS contexts, that passes in the
>  newly-chosen context ID (not as a pointer) rather than leaving the
>  driver to choose it on create.  Also pass in the ctx, allowing drivers
>  to easily use its private data area to store their hardware-specific
>  state.
> Keep the existing .set_rxfh_context API for now as a fallback, but
>  deprecate it.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>

> ---
>  include/linux/ethtool.h | 40 ++++++++++++++++++++++++---
>  net/core/dev.c          | 15 ++++++++---
>  net/ethtool/ioctl.c     | 60 ++++++++++++++++++++++++++++++-----------
>  3 files changed, 93 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 229a23571008..975fda7218f8 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -747,10 +747,33 @@ struct ethtool_mm_stats {
>   * @get_rxfh_context: Get the contents of the RX flow hash indirection table,
>   *	hash key, and/or hash function assiciated to the given rss context.
>   *	Returns a negative error code or zero.
> - * @set_rxfh_context: Create, remove and configure RSS contexts. Allows setting
> + * @create_rxfh_context: Create a new RSS context with the specified RX flow
> + *	hash indirection table, hash key, and hash function.
> + *	Arguments which are set to %NULL or zero will be populated to
> + *	appropriate defaults by the driver.
> + *	The &struct ethtool_rxfh_context for this context is passed in @ctx;
> + *	note that the indir table, hkey and hfunc are not yet populated as
> + *	of this call.  The driver does not need to update these; the core
> + *	will do so if this op succeeds.
> + *	If the driver provides this method, it must also provide
> + *	@modify_rxfh_context and @remove_rxfh_context.
> + *	Returns a negative error code or zero.
> + * @modify_rxfh_context: Reconfigure the specified RSS context.  Allows setting
>   *	the contents of the RX flow hash indirection table, hash key, and/or
> - *	hash function associated to the given context. Arguments which are set
> - *	to %NULL or zero will remain unchanged.
> + *	hash function associated with the given context.
> + *	Arguments which are set to %NULL or zero will remain unchanged.
> + *	The &struct ethtool_rxfh_context for this context is passed in @ctx;
> + *	note that it will still contain the *old* settings.  The driver does
> + *	not need to update these; the core will do so if this op succeeds.
> + *	Returns a negative error code or zero. An error code must be returned
> + *	if at least one unsupported change was requested.
> + * @remove_rxfh_context: Remove the specified RSS context.
> + *	The &struct ethtool_rxfh_context for this context is passed in @ctx.
> + *	Returns a negative error code or zero.
> + * @set_rxfh_context: Deprecated API to create, remove and configure RSS
> + *	contexts. Allows setting the contents of the RX flow hash indirection
> + *	table, hash key, and/or hash function associated to the given context.
> + *	Arguments which are set to %NULL or zero will remain unchanged.
>   *	Returns a negative error code or zero. An error code must be returned
>   *	if at least one unsupported change was requested.
>   * @get_channels: Get number of channels.
> @@ -901,6 +924,17 @@ struct ethtool_ops {
>  			    const u8 *key, const u8 hfunc);
>  	int	(*get_rxfh_context)(struct net_device *, u32 *indir, u8 *key,
>  				    u8 *hfunc, u32 rss_context);
> +	int	(*create_rxfh_context)(struct net_device *,
> +				       struct ethtool_rxfh_context *ctx,
> +				       const u32 *indir, const u8 *key,
> +				       const u8 hfunc, u32 rss_context);
> +	int	(*modify_rxfh_context)(struct net_device *,
> +				       struct ethtool_rxfh_context *ctx,
> +				       const u32 *indir, const u8 *key,
> +				       const u8 hfunc, u32 rss_context);
> +	int	(*remove_rxfh_context)(struct net_device *,
> +				       struct ethtool_rxfh_context *ctx,
> +				       u32 rss_context);
>  	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
>  				    const u8 *key, const u8 hfunc,
>  				    u32 *rss_context, bool delete);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 05e95abdfd17..637218adca22 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10882,16 +10882,23 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  	struct ethtool_rxfh_context *ctx;
>  	unsigned long context;
>  
> -	if (dev->ethtool_ops->set_rxfh_context)
> +	if (dev->ethtool_ops->create_rxfh_context ||
> +	    dev->ethtool_ops->set_rxfh_context)
>  		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
>  			u32 *indir = ethtool_rxfh_context_indir(ctx);
>  			u8 *key = ethtool_rxfh_context_key(ctx);
>  			u32 concast = context;
>  
>  			xa_erase(&dev->ethtool->rss_ctx, context);
> -			dev->ethtool_ops->set_rxfh_context(dev, indir, key,
> -							   ctx->hfunc, &concast,
> -							   true);
> +			if (dev->ethtool_ops->create_rxfh_context)
> +				dev->ethtool_ops->remove_rxfh_context(dev, ctx,
> +								      context);
> +			else
> +				dev->ethtool_ops->set_rxfh_context(dev, indir,
> +								   key,
> +								   ctx->hfunc,
> +								   &concast,
> +								   true);
>  			kfree(ctx);
>  		}
>  	xa_destroy(&dev->ethtool->rss_ctx);
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 1d13bc8fbb75..c23d2bd3cd2a 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1274,7 +1274,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd8[2] || rxfh.rsvd32)
>  		return -EINVAL;
>  	/* Most drivers don't handle rss_context, check it's 0 as well */
> -	if (rxfh.rss_context && !ops->set_rxfh_context)
> +	if (rxfh.rss_context && !(ops->create_rxfh_context ||
> +				  ops->set_rxfh_context))
>  		return -EOPNOTSUPP;
>  	create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
>  
> @@ -1349,8 +1350,24 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  		ctx->indir_size = dev_indir_size;
>  		ctx->key_size = dev_key_size;
> -		ctx->hfunc = rxfh.hfunc;
>  		ctx->priv_size = ops->rxfh_priv_size;
> +		/* Initialise to an empty context */
> +		ctx->indir_no_change = ctx->key_no_change = 1;
> +		ctx->hfunc = ETH_RSS_HASH_NO_CHANGE;
> +		if (ops->create_rxfh_context) {
> +			u32 limit = dev->ethtool->rss_ctx_max_id ?: U32_MAX;
> +			u32 ctx_id;
> +
> +			/* driver uses new API, core allocates ID */
> +			ret = xa_alloc(&dev->ethtool->rss_ctx, &ctx_id, ctx,
> +				       XA_LIMIT(1, limit), GFP_KERNEL_ACCOUNT);
> +			if (ret < 0) {
> +				kfree(ctx);
> +				goto out;
> +			}
> +			WARN_ON(!ctx_id); /* can't happen */
> +			rxfh.rss_context = ctx_id;
> +		}
>  	} else if (rxfh.rss_context) {
>  		ctx = xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context);
>  		if (!ctx) {
> @@ -1359,15 +1376,34 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  	}
>  
> -	if (rxfh.rss_context)
> -		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
> -					    &rxfh.rss_context, delete);
> -	else
> +	if (rxfh.rss_context) {
> +		if (ops->create_rxfh_context) {
> +			if (create)
> +				ret = ops->create_rxfh_context(dev, ctx, indir,
> +							       hkey, rxfh.hfunc,
> +							       rxfh.rss_context);
> +			else if (delete)
> +				ret = ops->remove_rxfh_context(dev, ctx,
> +							       rxfh.rss_context);
> +			else
> +				ret = ops->modify_rxfh_context(dev, ctx, indir,
> +							       hkey, rxfh.hfunc,
> +							       rxfh.rss_context);
> +		} else {
> +			ret = ops->set_rxfh_context(dev, indir, hkey,
> +						    rxfh.hfunc,
> +						    &rxfh.rss_context, delete);
> +		}
> +	} else {
>  		ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
> +	}
>  	if (ret) {
> -		if (create)
> +		if (create) {
>  			/* failed to create, free our new tracking entry */
> +			if (ops->create_rxfh_context)
> +				xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context);
>  			kfree(ctx);
> +		}
>  		goto out;
>  	}
>  
> @@ -1383,12 +1419,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  			dev->priv_flags |= IFF_RXFH_CONFIGURED;
>  	}
>  	/* Update rss_ctx tracking */
> -	if (create) {
> -		/* Ideally this should happen before calling the driver,
> -		 * so that we can fail more cleanly; but we don't have the
> -		 * context ID until the driver picks it, so we have to
> -		 * wait until after.
> -		 */
> +	if (create && !ops->create_rxfh_context) {
> +		/* driver uses old API, it chose context ID */
>  		if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
>  			/* context ID reused, our tracking is screwed */
>  			kfree(ctx);
> @@ -1400,8 +1432,6 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  			kfree(ctx);
>  			goto out;
>  		}
> -		ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
> -		ctx->key_no_change = !rxfh.key_size;
>  	}
>  	if (delete) {
>  		WARN_ON(xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);

Re: [PATCH v4 net-next 5/7] net: ethtool: add an extack parameter to new rxfh_context APIs

[Martin Habets]

On Wed, Sep 27, 2023 at 07:13:36PM +0100, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Currently passed as NULL, but will allow drivers to report back errors
>  when ethnl support for these ops is added.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>

> ---
>  include/linux/ethtool.h | 9 ++++++---
>  net/core/dev.c          | 3 ++-
>  net/ethtool/ioctl.c     | 9 ++++++---
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 975fda7218f8..c8963bde9289 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -927,14 +927,17 @@ struct ethtool_ops {
>  	int	(*create_rxfh_context)(struct net_device *,
>  				       struct ethtool_rxfh_context *ctx,
>  				       const u32 *indir, const u8 *key,
> -				       const u8 hfunc, u32 rss_context);
> +				       const u8 hfunc, u32 rss_context,
> +				       struct netlink_ext_ack *extack);
>  	int	(*modify_rxfh_context)(struct net_device *,
>  				       struct ethtool_rxfh_context *ctx,
>  				       const u32 *indir, const u8 *key,
> -				       const u8 hfunc, u32 rss_context);
> +				       const u8 hfunc, u32 rss_context,
> +				       struct netlink_ext_ack *extack);
>  	int	(*remove_rxfh_context)(struct net_device *,
>  				       struct ethtool_rxfh_context *ctx,
> -				       u32 rss_context);
> +				       u32 rss_context,
> +				       struct netlink_ext_ack *extack);
>  	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
>  				    const u8 *key, const u8 hfunc,
>  				    u32 *rss_context, bool delete);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 637218adca22..69579d9cd7ba 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10892,7 +10892,8 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  			xa_erase(&dev->ethtool->rss_ctx, context);
>  			if (dev->ethtool_ops->create_rxfh_context)
>  				dev->ethtool_ops->remove_rxfh_context(dev, ctx,
> -								      context);
> +								      context,
> +								      NULL);
>  			else
>  				dev->ethtool_ops->set_rxfh_context(dev, indir,
>  								   key,
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index c23d2bd3cd2a..3920ddee3ee2 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1381,14 +1381,17 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  			if (create)
>  				ret = ops->create_rxfh_context(dev, ctx, indir,
>  							       hkey, rxfh.hfunc,
> -							       rxfh.rss_context);
> +							       rxfh.rss_context,
> +							       NULL);
>  			else if (delete)
>  				ret = ops->remove_rxfh_context(dev, ctx,
> -							       rxfh.rss_context);
> +							       rxfh.rss_context,
> +							       NULL);
>  			else
>  				ret = ops->modify_rxfh_context(dev, ctx, indir,
>  							       hkey, rxfh.hfunc,
> -							       rxfh.rss_context);
> +							       rxfh.rss_context,
> +							       NULL);
>  		} else {
>  			ret = ops->set_rxfh_context(dev, indir, hkey,
>  						    rxfh.hfunc,

Re: [PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

[Martin Habets]

On Wed, Sep 27, 2023 at 07:13:37PM +0100, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> While this is not needed to serialise the ethtool entry points (which
>  are all under RTNL), drivers may have cause to asynchronously access
>  dev->ethtool->rss_ctx; taking dev->ethtool->rss_lock allows them to
>  do this safely without needing to take the RTNL.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>

> ---
>  include/linux/ethtool.h | 3 +++
>  net/core/dev.c          | 5 +++++
>  net/ethtool/ioctl.c     | 7 +++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index c8963bde9289..d15a21bd6f12 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -1026,11 +1026,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>  /**
>   * struct ethtool_netdev_state - per-netdevice state for ethtool features
>   * @rss_ctx:		XArray of custom RSS contexts
> + * @rss_lock:		Protects entries in @rss_ctx.  May be taken from
> + *			within RTNL.
>   * @rss_ctx_max_id:	maximum (exclusive) supported RSS context ID
>   * @wol_enabled:	Wake-on-LAN is enabled
>   */
>  struct ethtool_netdev_state {
>  	struct xarray		rss_ctx;
> +	struct mutex		rss_lock;
>  	u32			rss_ctx_max_id;
>  	u32			wol_enabled:1;
>  };
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 69579d9cd7ba..c57456ed4be8 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10074,6 +10074,7 @@ int register_netdevice(struct net_device *dev)
>  
>  	/* rss ctx ID 0 is reserved for the default context, start from 1 */
>  	xa_init_flags(&dev->ethtool->rss_ctx, XA_FLAGS_ALLOC1);
> +	mutex_init(&dev->ethtool->rss_lock);
>  
>  	spin_lock_init(&dev->addr_list_lock);
>  	netdev_set_addr_lockdep_class(dev);
> @@ -10882,6 +10883,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  	struct ethtool_rxfh_context *ctx;
>  	unsigned long context;
>  
> +	mutex_lock(&dev->ethtool->rss_lock);
>  	if (dev->ethtool_ops->create_rxfh_context ||
>  	    dev->ethtool_ops->set_rxfh_context)
>  		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
> @@ -10903,6 +10905,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  			kfree(ctx);
>  		}
>  	xa_destroy(&dev->ethtool->rss_ctx);
> +	mutex_unlock(&dev->ethtool->rss_lock);
>  }
>  
>  /**
> @@ -11016,6 +11019,8 @@ void unregister_netdevice_many_notify(struct list_head *head,
>  		if (dev->netdev_ops->ndo_uninit)
>  			dev->netdev_ops->ndo_uninit(dev);
>  
> +		mutex_destroy(&dev->ethtool->rss_lock);
> +
>  		if (skb)
>  			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, portid, nlh);
>  
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 3920ddee3ee2..d21bbc92e6fc 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1258,6 +1258,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	u8 *rss_config;
>  	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
>  	bool create = false, delete = false;
> +	bool locked = false; /* dev->ethtool->rss_lock taken */
>  
>  	if (!ops->get_rxnfc || !ops->set_rxfh)
>  		return -EOPNOTSUPP;
> @@ -1335,6 +1336,10 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  	}
>  
> +	if (rxfh.rss_context) {
> +		mutex_lock(&dev->ethtool->rss_lock);
> +		locked = true;
> +	}
>  	if (create) {
>  		if (delete) {
>  			ret = -EINVAL;
> @@ -1455,6 +1460,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	}
>  
>  out:
> +	if (locked)
> +		mutex_unlock(&dev->ethtool->rss_lock);
>  	kfree(rss_config);
>  	return ret;
>  }

Re: [PATCH v4 net-next 7/7] sfc: use new rxfh_context API

[Martin Habets]

On Wed, Sep 27, 2023 at 07:13:38PM +0100, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> The core is now responsible for allocating IDs and a memory region for
>  us to store our state (struct efx_rss_context_priv), so we no longer
>  need efx_alloc_rss_context_entry() and friends.
> Since the contexts are now maintained by the core, use the core's lock
>  (net_dev->ethtool->rss_lock), rather than our own mutex (efx->rss_lock),
>  to serialise access against changes; and remove the now-unused
>  efx->rss_lock from struct efx_nic.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  drivers/net/ethernet/sfc/ef10.c           |   2 +-
>  drivers/net/ethernet/sfc/ef100_ethtool.c  |   5 +-
>  drivers/net/ethernet/sfc/efx.c            |   2 +-
>  drivers/net/ethernet/sfc/efx.h            |   2 +-
>  drivers/net/ethernet/sfc/efx_common.c     |  10 +-
>  drivers/net/ethernet/sfc/ethtool.c        |   5 +-
>  drivers/net/ethernet/sfc/ethtool_common.c | 147 +++++++++++++---------
>  drivers/net/ethernet/sfc/ethtool_common.h |  18 ++-
>  drivers/net/ethernet/sfc/mcdi_filters.c   | 135 ++++++++++----------
>  drivers/net/ethernet/sfc/mcdi_filters.h   |   8 +-
>  drivers/net/ethernet/sfc/net_driver.h     |  28 ++---
>  drivers/net/ethernet/sfc/rx_common.c      |  64 ++--------
>  drivers/net/ethernet/sfc/rx_common.h      |   8 +-
>  13 files changed, 214 insertions(+), 220 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> index 6dfa062feebc..e20305461b57 100644
> --- a/drivers/net/ethernet/sfc/ef10.c
> +++ b/drivers/net/ethernet/sfc/ef10.c
> @@ -1396,7 +1396,7 @@ static void efx_ef10_table_reset_mc_allocations(struct efx_nic *efx)
>  	efx_mcdi_filter_table_reset_mc_allocations(efx);
>  	nic_data->must_restore_piobufs = true;
>  	efx_ef10_forget_old_piobufs(efx);
> -	efx->rss_context.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
> +	efx->rss_context.priv.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
>  
>  	/* Driver-created vswitches and vports must be re-created */
>  	nic_data->must_probe_vswitching = true;
> diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
> index 702abbe59b76..c5f82eb0e5b4 100644
> --- a/drivers/net/ethernet/sfc/ef100_ethtool.c
> +++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
> @@ -58,10 +58,13 @@ const struct ethtool_ops ef100_ethtool_ops = {
>  
>  	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
>  	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
> +	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
>  	.get_rxfh		= efx_ethtool_get_rxfh,
>  	.set_rxfh		= efx_ethtool_set_rxfh,
>  	.get_rxfh_context	= efx_ethtool_get_rxfh_context,
> -	.set_rxfh_context	= efx_ethtool_set_rxfh_context,
> +	.create_rxfh_context	= efx_ethtool_create_rxfh_context,
> +	.modify_rxfh_context	= efx_ethtool_modify_rxfh_context,
> +	.remove_rxfh_context	= efx_ethtool_remove_rxfh_context,
>  
>  	.get_module_info	= efx_ethtool_get_module_info,
>  	.get_module_eeprom	= efx_ethtool_get_module_eeprom,
> diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
> index 19f4b4d0b851..6ae84356797f 100644
> --- a/drivers/net/ethernet/sfc/efx.c
> +++ b/drivers/net/ethernet/sfc/efx.c
> @@ -299,7 +299,7 @@ static int efx_probe_nic(struct efx_nic *efx)
>  	if (efx->n_channels > 1)
>  		netdev_rss_key_fill(efx->rss_context.rx_hash_key,
>  				    sizeof(efx->rss_context.rx_hash_key));
> -	efx_set_default_rx_indir_table(efx, &efx->rss_context);
> +	efx_set_default_rx_indir_table(efx, efx->rss_context.rx_indir_table);
>  
>  	/* Initialise the interrupt moderation settings */
>  	efx->irq_mod_step_us = DIV_ROUND_UP(efx->timer_quantum_ns, 1000);
> diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
> index 48d3623735ba..7a6cab883d66 100644
> --- a/drivers/net/ethernet/sfc/efx.h
> +++ b/drivers/net/ethernet/sfc/efx.h
> @@ -158,7 +158,7 @@ static inline s32 efx_filter_get_rx_ids(struct efx_nic *efx,
>  }
>  
>  /* RSS contexts */
> -static inline bool efx_rss_active(struct efx_rss_context *ctx)
> +static inline bool efx_rss_active(struct efx_rss_context_priv *ctx)
>  {
>  	return ctx->context_id != EFX_MCDI_RSS_CONTEXT_INVALID;
>  }
> diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
> index 175bd9cdfdac..2cd92e1f68db 100644
> --- a/drivers/net/ethernet/sfc/efx_common.c
> +++ b/drivers/net/ethernet/sfc/efx_common.c
> @@ -714,7 +714,7 @@ void efx_reset_down(struct efx_nic *efx, enum reset_type method)
>  
>  	mutex_lock(&efx->mac_lock);
>  	down_write(&efx->filter_sem);
> -	mutex_lock(&efx->rss_lock);
> +	mutex_lock(&efx->net_dev->ethtool->rss_lock);
>  	efx->type->fini(efx);
>  }
>  
> @@ -777,7 +777,7 @@ int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok)
>  
>  	if (efx->type->rx_restore_rss_contexts)
>  		efx->type->rx_restore_rss_contexts(efx);
> -	mutex_unlock(&efx->rss_lock);
> +	mutex_unlock(&efx->net_dev->ethtool->rss_lock);
>  	efx->type->filter_table_restore(efx);
>  	up_write(&efx->filter_sem);
>  
> @@ -793,7 +793,7 @@ int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok)
>  fail:
>  	efx->port_initialized = false;
>  
> -	mutex_unlock(&efx->rss_lock);
> +	mutex_unlock(&efx->net_dev->ethtool->rss_lock);
>  	up_write(&efx->filter_sem);
>  	mutex_unlock(&efx->mac_lock);
>  
> @@ -1000,9 +1000,7 @@ int efx_init_struct(struct efx_nic *efx, struct pci_dev *pci_dev)
>  		efx->type->rx_hash_offset - efx->type->rx_prefix_size;
>  	efx->rx_packet_ts_offset =
>  		efx->type->rx_ts_offset - efx->type->rx_prefix_size;
> -	INIT_LIST_HEAD(&efx->rss_context.list);
> -	efx->rss_context.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
> -	mutex_init(&efx->rss_lock);
> +	efx->rss_context.priv.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
>  	efx->vport_id = EVB_PORT_ID_ASSIGNED;
>  	spin_lock_init(&efx->stats_lock);
>  	efx->vi_stride = EFX_DEFAULT_VI_STRIDE;
> diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
> index 364323599f7b..f5fb7464e025 100644
> --- a/drivers/net/ethernet/sfc/ethtool.c
> +++ b/drivers/net/ethernet/sfc/ethtool.c
> @@ -267,10 +267,13 @@ const struct ethtool_ops efx_ethtool_ops = {
>  	.set_rxnfc		= efx_ethtool_set_rxnfc,
>  	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
>  	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
> +	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
>  	.get_rxfh		= efx_ethtool_get_rxfh,
>  	.set_rxfh		= efx_ethtool_set_rxfh,
>  	.get_rxfh_context	= efx_ethtool_get_rxfh_context,
> -	.set_rxfh_context	= efx_ethtool_set_rxfh_context,
> +	.create_rxfh_context	= efx_ethtool_create_rxfh_context,
> +	.modify_rxfh_context	= efx_ethtool_modify_rxfh_context,
> +	.remove_rxfh_context	= efx_ethtool_remove_rxfh_context,
>  	.get_ts_info		= efx_ethtool_get_ts_info,
>  	.get_module_info	= efx_ethtool_get_module_info,
>  	.get_module_eeprom	= efx_ethtool_get_module_eeprom,
> diff --git a/drivers/net/ethernet/sfc/ethtool_common.c b/drivers/net/ethernet/sfc/ethtool_common.c
> index a8cbceeb301b..7cd01012152e 100644
> --- a/drivers/net/ethernet/sfc/ethtool_common.c
> +++ b/drivers/net/ethernet/sfc/ethtool_common.c
> @@ -820,10 +820,10 @@ int efx_ethtool_get_rxnfc(struct net_device *net_dev,
>  		return 0;
>  
>  	case ETHTOOL_GRXFH: {
> -		struct efx_rss_context *ctx = &efx->rss_context;
> +		struct efx_rss_context_priv *ctx = &efx->rss_context.priv;
>  		__u64 data;
>  
> -		mutex_lock(&efx->rss_lock);
> +		mutex_lock(&net_dev->ethtool->rss_lock);
>  		if (info->flow_type & FLOW_RSS && info->rss_context) {
>  			ctx = efx_find_rss_context_entry(efx, info->rss_context);
>  			if (!ctx) {
> @@ -864,7 +864,7 @@ int efx_ethtool_get_rxnfc(struct net_device *net_dev,
>  out_setdata_unlock:
>  		info->data = data;
>  out_unlock:
> -		mutex_unlock(&efx->rss_lock);
> +		mutex_unlock(&net_dev->ethtool->rss_lock);
>  		return rc;
>  	}
>  
> @@ -1207,96 +1207,121 @@ int efx_ethtool_get_rxfh_context(struct net_device *net_dev, u32 *indir,
>  				 u8 *key, u8 *hfunc, u32 rss_context)
>  {
>  	struct efx_nic *efx = efx_netdev_priv(net_dev);
> -	struct efx_rss_context *ctx;
> +	struct efx_rss_context_priv *ctx_priv;
> +	struct efx_rss_context ctx;
>  	int rc = 0;
>  
>  	if (!efx->type->rx_pull_rss_context_config)
>  		return -EOPNOTSUPP;
>  
> -	mutex_lock(&efx->rss_lock);
> -	ctx = efx_find_rss_context_entry(efx, rss_context);
> -	if (!ctx) {
> +	mutex_lock(&net_dev->ethtool->rss_lock);
> +	ctx_priv = efx_find_rss_context_entry(efx, rss_context);
> +	if (!ctx_priv) {
>  		rc = -ENOENT;
>  		goto out_unlock;
>  	}
> -	rc = efx->type->rx_pull_rss_context_config(efx, ctx);
> +	ctx.priv = *ctx_priv;
> +	rc = efx->type->rx_pull_rss_context_config(efx, &ctx);
>  	if (rc)
>  		goto out_unlock;
>  
>  	if (hfunc)
>  		*hfunc = ETH_RSS_HASH_TOP;
>  	if (indir)
> -		memcpy(indir, ctx->rx_indir_table, sizeof(ctx->rx_indir_table));
> +		memcpy(indir, ctx.rx_indir_table, sizeof(ctx.rx_indir_table));
>  	if (key)
> -		memcpy(key, ctx->rx_hash_key, efx->type->rx_hash_key_size);
> +		memcpy(key, ctx.rx_hash_key, efx->type->rx_hash_key_size);
>  out_unlock:
> -	mutex_unlock(&efx->rss_lock);
> +	mutex_unlock(&net_dev->ethtool->rss_lock);
>  	return rc;
>  }
>  
> -int efx_ethtool_set_rxfh_context(struct net_device *net_dev,
> -				 const u32 *indir, const u8 *key,
> -				 const u8 hfunc, u32 *rss_context,
> -				 bool delete)
> +int efx_ethtool_modify_rxfh_context(struct net_device *net_dev,
> +				    struct ethtool_rxfh_context *ctx,
> +				    const u32 *indir, const u8 *key,
> +				    const u8 hfunc, u32 rss_context,
> +				    struct netlink_ext_ack *extack)
>  {
>  	struct efx_nic *efx = efx_netdev_priv(net_dev);
> -	struct efx_rss_context *ctx;
> -	bool allocated = false;
> -	int rc;
> +	struct efx_rss_context_priv *priv;
>  
> -	if (!efx->type->rx_push_rss_context_config)
> +	if (!efx->type->rx_push_rss_context_config) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "NIC type does not support custom contexts");
>  		return -EOPNOTSUPP;
> +	}
>  	/* Hash function is Toeplitz, cannot be changed */
> -	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP)
> +	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP) {
> +		NL_SET_ERR_MSG_MOD(extack, "Only Toeplitz hash is supported");
>  		return -EOPNOTSUPP;
> +	}
>  
> -	mutex_lock(&efx->rss_lock);
> +	priv = ethtool_rxfh_context_priv(ctx);
>  
> -	if (*rss_context == ETH_RXFH_CONTEXT_ALLOC) {
> -		if (delete) {
> -			/* alloc + delete == Nothing to do */
> -			rc = -EINVAL;
> -			goto out_unlock;
> -		}
> -		ctx = efx_alloc_rss_context_entry(efx);
> -		if (!ctx) {
> -			rc = -ENOMEM;
> -			goto out_unlock;
> -		}
> -		ctx->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
> -		/* Initialise indir table and key to defaults */
> -		efx_set_default_rx_indir_table(efx, ctx);
> -		netdev_rss_key_fill(ctx->rx_hash_key, sizeof(ctx->rx_hash_key));
> -		allocated = true;
> -	} else {
> -		ctx = efx_find_rss_context_entry(efx, *rss_context);
> -		if (!ctx) {
> -			rc = -ENOENT;
> -			goto out_unlock;
> -		}
> -	}
> +	if (!key)
> +		key = ethtool_rxfh_context_key(ctx);
> +	if (!indir)
> +		indir = ethtool_rxfh_context_indir(ctx);
>  
> -	if (delete) {
> -		/* delete this context */
> -		rc = efx->type->rx_push_rss_context_config(efx, ctx, NULL, NULL);
> -		if (!rc)
> -			efx_free_rss_context_entry(ctx);
> -		goto out_unlock;
> +	return efx->type->rx_push_rss_context_config(efx, priv, indir, key,
> +						     false);
> +}
> +
> +int efx_ethtool_create_rxfh_context(struct net_device *net_dev,
> +				    struct ethtool_rxfh_context *ctx,
> +				    const u32 *indir, const u8 *key,
> +				    const u8 hfunc, u32 rss_context,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct efx_nic *efx = efx_netdev_priv(net_dev);
> +	struct efx_rss_context_priv *priv;
> +
> +	if (!efx->type->rx_push_rss_context_config) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "NIC type does not support custom contexts");
> +		return -EOPNOTSUPP;
> +	}
> +	/* Hash function is Toeplitz, cannot be changed */
> +	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP) {
> +		NL_SET_ERR_MSG_MOD(extack, "Only Toeplitz hash is supported");
> +		return -EOPNOTSUPP;
>  	}
>  
> -	if (!key)
> -		key = ctx->rx_hash_key;
> +	priv = ethtool_rxfh_context_priv(ctx);
> +
> +	priv->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
> +	priv->rx_hash_udp_4tuple = false;
> +	/* Generate default indir table and/or key if not specified.
> +	 * We use ctx as a place to store these; this is fine because
> +	 * we're doing a create, so if we fail then the ctx will just
> +	 * be deleted.
> +	 */
>  	if (!indir)
> -		indir = ctx->rx_indir_table;
> +		efx_set_default_rx_indir_table(efx, ethtool_rxfh_context_indir(ctx));
> +	if (!key)
> +		netdev_rss_key_fill(ethtool_rxfh_context_key(ctx),
> +				    ctx->key_size);
> +	return efx_ethtool_modify_rxfh_context(net_dev, ctx, indir, key, hfunc,
> +					       rss_context, extack);
> +}
>  
> -	rc = efx->type->rx_push_rss_context_config(efx, ctx, indir, key);
> -	if (rc && allocated)
> -		efx_free_rss_context_entry(ctx);
> -	else
> -		*rss_context = ctx->user_id;
> -out_unlock:
> -	mutex_unlock(&efx->rss_lock);
> -	return rc;
> +int efx_ethtool_remove_rxfh_context(struct net_device *net_dev,
> +				    struct ethtool_rxfh_context *ctx,
> +				    u32 rss_context,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct efx_nic *efx = efx_netdev_priv(net_dev);
> +	struct efx_rss_context_priv *priv;
> +
> +	if (!efx->type->rx_push_rss_context_config) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "NIC type does not support custom contexts");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	priv = ethtool_rxfh_context_priv(ctx);
> +	return efx->type->rx_push_rss_context_config(efx, priv, NULL, NULL,
> +						     true);
>  }
>  
>  int efx_ethtool_reset(struct net_device *net_dev, u32 *flags)
> diff --git a/drivers/net/ethernet/sfc/ethtool_common.h b/drivers/net/ethernet/sfc/ethtool_common.h
> index 659491932101..3df852eaab20 100644
> --- a/drivers/net/ethernet/sfc/ethtool_common.h
> +++ b/drivers/net/ethernet/sfc/ethtool_common.h
> @@ -50,10 +50,20 @@ int efx_ethtool_set_rxfh(struct net_device *net_dev,
>  			 const u32 *indir, const u8 *key, const u8 hfunc);
>  int efx_ethtool_get_rxfh_context(struct net_device *net_dev, u32 *indir,
>  				 u8 *key, u8 *hfunc, u32 rss_context);
> -int efx_ethtool_set_rxfh_context(struct net_device *net_dev,
> -				 const u32 *indir, const u8 *key,
> -				 const u8 hfunc, u32 *rss_context,
> -				 bool delete);
> +int efx_ethtool_create_rxfh_context(struct net_device *net_dev,
> +				    struct ethtool_rxfh_context *ctx,
> +				    const u32 *indir, const u8 *key,
> +				    const u8 hfunc, u32 rss_context,
> +				    struct netlink_ext_ack *extack);
> +int efx_ethtool_modify_rxfh_context(struct net_device *net_dev,
> +				    struct ethtool_rxfh_context *ctx,
> +				    const u32 *indir, const u8 *key,
> +				    const u8 hfunc, u32 rss_context,
> +				    struct netlink_ext_ack *extack);
> +int efx_ethtool_remove_rxfh_context(struct net_device *net_dev,
> +				    struct ethtool_rxfh_context *ctx,
> +				    u32 rss_context,
> +				    struct netlink_ext_ack *extack);
>  int efx_ethtool_reset(struct net_device *net_dev, u32 *flags);
>  int efx_ethtool_get_module_eeprom(struct net_device *net_dev,
>  				  struct ethtool_eeprom *ee,
> diff --git a/drivers/net/ethernet/sfc/mcdi_filters.c b/drivers/net/ethernet/sfc/mcdi_filters.c
> index 4ff6586116ee..6ef96292909a 100644
> --- a/drivers/net/ethernet/sfc/mcdi_filters.c
> +++ b/drivers/net/ethernet/sfc/mcdi_filters.c
> @@ -194,7 +194,7 @@ efx_mcdi_filter_push_prep_set_match_fields(struct efx_nic *efx,
>  static void efx_mcdi_filter_push_prep(struct efx_nic *efx,
>  				      const struct efx_filter_spec *spec,
>  				      efx_dword_t *inbuf, u64 handle,
> -				      struct efx_rss_context *ctx,
> +				      struct efx_rss_context_priv *ctx,
>  				      bool replacing)
>  {
>  	u32 flags = spec->flags;
> @@ -245,7 +245,7 @@ static void efx_mcdi_filter_push_prep(struct efx_nic *efx,
>  
>  static int efx_mcdi_filter_push(struct efx_nic *efx,
>  				const struct efx_filter_spec *spec, u64 *handle,
> -				struct efx_rss_context *ctx, bool replacing)
> +				struct efx_rss_context_priv *ctx, bool replacing)
>  {
>  	MCDI_DECLARE_BUF(inbuf, MC_CMD_FILTER_OP_EXT_IN_LEN);
>  	MCDI_DECLARE_BUF(outbuf, MC_CMD_FILTER_OP_EXT_OUT_LEN);
> @@ -345,9 +345,9 @@ static s32 efx_mcdi_filter_insert_locked(struct efx_nic *efx,
>  					 bool replace_equal)
>  {
>  	DECLARE_BITMAP(mc_rem_map, EFX_EF10_FILTER_SEARCH_LIMIT);
> +	struct efx_rss_context_priv *ctx = NULL;
>  	struct efx_mcdi_filter_table *table;
>  	struct efx_filter_spec *saved_spec;
> -	struct efx_rss_context *ctx = NULL;
>  	unsigned int match_pri, hash;
>  	unsigned int priv_flags;
>  	bool rss_locked = false;
> @@ -380,12 +380,12 @@ static s32 efx_mcdi_filter_insert_locked(struct efx_nic *efx,
>  		bitmap_zero(mc_rem_map, EFX_EF10_FILTER_SEARCH_LIMIT);
>  
>  	if (spec->flags & EFX_FILTER_FLAG_RX_RSS) {
> -		mutex_lock(&efx->rss_lock);
> +		mutex_lock(&efx->net_dev->ethtool->rss_lock);
>  		rss_locked = true;
>  		if (spec->rss_context)
>  			ctx = efx_find_rss_context_entry(efx, spec->rss_context);
>  		else
> -			ctx = &efx->rss_context;
> +			ctx = &efx->rss_context.priv;
>  		if (!ctx) {
>  			rc = -ENOENT;
>  			goto out_unlock;
> @@ -548,7 +548,7 @@ static s32 efx_mcdi_filter_insert_locked(struct efx_nic *efx,
>  
>  out_unlock:
>  	if (rss_locked)
> -		mutex_unlock(&efx->rss_lock);
> +		mutex_unlock(&efx->net_dev->ethtool->rss_lock);
>  	up_write(&table->lock);
>  	return rc;
>  }
> @@ -611,13 +611,13 @@ static int efx_mcdi_filter_remove_internal(struct efx_nic *efx,
>  
>  		new_spec.priority = EFX_FILTER_PRI_AUTO;
>  		new_spec.flags = (EFX_FILTER_FLAG_RX |
> -				  (efx_rss_active(&efx->rss_context) ?
> +				  (efx_rss_active(&efx->rss_context.priv) ?
>  				   EFX_FILTER_FLAG_RX_RSS : 0));
>  		new_spec.dmaq_id = 0;
>  		new_spec.rss_context = 0;
>  		rc = efx_mcdi_filter_push(efx, &new_spec,
>  					  &table->entry[filter_idx].handle,
> -					  &efx->rss_context,
> +					  &efx->rss_context.priv,
>  					  true);
>  
>  		if (rc == 0)
> @@ -764,7 +764,7 @@ static int efx_mcdi_filter_insert_addr_list(struct efx_nic *efx,
>  		ids = vlan->uc;
>  	}
>  
> -	filter_flags = efx_rss_active(&efx->rss_context) ? EFX_FILTER_FLAG_RX_RSS : 0;
> +	filter_flags = efx_rss_active(&efx->rss_context.priv) ? EFX_FILTER_FLAG_RX_RSS : 0;
>  
>  	/* Insert/renew filters */
>  	for (i = 0; i < addr_count; i++) {
> @@ -833,7 +833,7 @@ static int efx_mcdi_filter_insert_def(struct efx_nic *efx,
>  	int rc;
>  	u16 *id;
>  
> -	filter_flags = efx_rss_active(&efx->rss_context) ? EFX_FILTER_FLAG_RX_RSS : 0;
> +	filter_flags = efx_rss_active(&efx->rss_context.priv) ? EFX_FILTER_FLAG_RX_RSS : 0;
>  
>  	efx_filter_init_rx(&spec, EFX_FILTER_PRI_AUTO, filter_flags, 0);
>  
> @@ -1375,8 +1375,8 @@ void efx_mcdi_filter_table_restore(struct efx_nic *efx)
>  	struct efx_mcdi_filter_table *table = efx->filter_state;
>  	unsigned int invalid_filters = 0, failed = 0;
>  	struct efx_mcdi_filter_vlan *vlan;
> +	struct efx_rss_context_priv *ctx;
>  	struct efx_filter_spec *spec;
> -	struct efx_rss_context *ctx;
>  	unsigned int filter_idx;
>  	u32 mcdi_flags;
>  	int match_pri;
> @@ -1388,7 +1388,7 @@ void efx_mcdi_filter_table_restore(struct efx_nic *efx)
>  		return;
>  
>  	down_write(&table->lock);
> -	mutex_lock(&efx->rss_lock);
> +	mutex_lock(&efx->net_dev->ethtool->rss_lock);
>  
>  	for (filter_idx = 0; filter_idx < EFX_MCDI_FILTER_TBL_ROWS; filter_idx++) {
>  		spec = efx_mcdi_filter_entry_spec(table, filter_idx);
> @@ -1407,7 +1407,7 @@ void efx_mcdi_filter_table_restore(struct efx_nic *efx)
>  		if (spec->rss_context)
>  			ctx = efx_find_rss_context_entry(efx, spec->rss_context);
>  		else
> -			ctx = &efx->rss_context;
> +			ctx = &efx->rss_context.priv;
>  		if (spec->flags & EFX_FILTER_FLAG_RX_RSS) {
>  			if (!ctx) {
>  				netif_warn(efx, drv, efx->net_dev,
> @@ -1444,7 +1444,7 @@ void efx_mcdi_filter_table_restore(struct efx_nic *efx)
>  		}
>  	}
>  
> -	mutex_unlock(&efx->rss_lock);
> +	mutex_unlock(&efx->net_dev->ethtool->rss_lock);
>  	up_write(&table->lock);
>  
>  	/*
> @@ -1861,7 +1861,8 @@ bool efx_mcdi_filter_rfs_expire_one(struct efx_nic *efx, u32 flow_id,
>  					 RSS_MODE_HASH_ADDRS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV6_RSS_MODE_LBN |\
>  					 RSS_MODE_HASH_ADDRS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_OTHER_IPV6_RSS_MODE_LBN)
>  
> -int efx_mcdi_get_rss_context_flags(struct efx_nic *efx, u32 context, u32 *flags)
> +static int efx_mcdi_get_rss_context_flags(struct efx_nic *efx, u32 context,
> +					  u32 *flags)
>  {
>  	/*
>  	 * Firmware had a bug (sfc bug 61952) where it would not actually
> @@ -1909,8 +1910,8 @@ int efx_mcdi_get_rss_context_flags(struct efx_nic *efx, u32 context, u32 *flags)
>   * Defaults are 4-tuple for TCP and 2-tuple for UDP and other-IP, so we
>   * just need to set the UDP ports flags (for both IP versions).
>   */
> -void efx_mcdi_set_rss_context_flags(struct efx_nic *efx,
> -				    struct efx_rss_context *ctx)
> +static void efx_mcdi_set_rss_context_flags(struct efx_nic *efx,
> +					   struct efx_rss_context_priv *ctx)
>  {
>  	MCDI_DECLARE_BUF(inbuf, MC_CMD_RSS_CONTEXT_SET_FLAGS_IN_LEN);
>  	u32 flags;
> @@ -1931,7 +1932,7 @@ void efx_mcdi_set_rss_context_flags(struct efx_nic *efx,
>  }
>  
>  static int efx_mcdi_filter_alloc_rss_context(struct efx_nic *efx, bool exclusive,
> -					     struct efx_rss_context *ctx,
> +					     struct efx_rss_context_priv *ctx,
>  					     unsigned *context_size)
>  {
>  	MCDI_DECLARE_BUF(inbuf, MC_CMD_RSS_CONTEXT_ALLOC_IN_LEN);
> @@ -2032,25 +2033,26 @@ void efx_mcdi_rx_free_indir_table(struct efx_nic *efx)
>  {
>  	int rc;
>  
> -	if (efx->rss_context.context_id != EFX_MCDI_RSS_CONTEXT_INVALID) {
> -		rc = efx_mcdi_filter_free_rss_context(efx, efx->rss_context.context_id);
> +	if (efx->rss_context.priv.context_id != EFX_MCDI_RSS_CONTEXT_INVALID) {
> +		rc = efx_mcdi_filter_free_rss_context(efx, efx->rss_context.priv.context_id);
>  		WARN_ON(rc != 0);
>  	}
> -	efx->rss_context.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
> +	efx->rss_context.priv.context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
>  }
>  
>  static int efx_mcdi_filter_rx_push_shared_rss_config(struct efx_nic *efx,
>  					      unsigned *context_size)
>  {
>  	struct efx_mcdi_filter_table *table = efx->filter_state;
> -	int rc = efx_mcdi_filter_alloc_rss_context(efx, false, &efx->rss_context,
> -					    context_size);
> +	int rc = efx_mcdi_filter_alloc_rss_context(efx, false,
> +						   &efx->rss_context.priv,
> +						   context_size);
>  
>  	if (rc != 0)
>  		return rc;
>  
>  	table->rx_rss_context_exclusive = false;
> -	efx_set_default_rx_indir_table(efx, &efx->rss_context);
> +	efx_set_default_rx_indir_table(efx, efx->rss_context.rx_indir_table);
>  	return 0;
>  }
>  
> @@ -2058,26 +2060,27 @@ static int efx_mcdi_filter_rx_push_exclusive_rss_config(struct efx_nic *efx,
>  						 const u32 *rx_indir_table,
>  						 const u8 *key)
>  {
> +	u32 old_rx_rss_context = efx->rss_context.priv.context_id;
>  	struct efx_mcdi_filter_table *table = efx->filter_state;
> -	u32 old_rx_rss_context = efx->rss_context.context_id;
>  	int rc;
>  
> -	if (efx->rss_context.context_id == EFX_MCDI_RSS_CONTEXT_INVALID ||
> +	if (efx->rss_context.priv.context_id == EFX_MCDI_RSS_CONTEXT_INVALID ||
>  	    !table->rx_rss_context_exclusive) {
> -		rc = efx_mcdi_filter_alloc_rss_context(efx, true, &efx->rss_context,
> -						NULL);
> +		rc = efx_mcdi_filter_alloc_rss_context(efx, true,
> +						       &efx->rss_context.priv,
> +						       NULL);
>  		if (rc == -EOPNOTSUPP)
>  			return rc;
>  		else if (rc != 0)
>  			goto fail1;
>  	}
>  
> -	rc = efx_mcdi_filter_populate_rss_table(efx, efx->rss_context.context_id,
> -					 rx_indir_table, key);
> +	rc = efx_mcdi_filter_populate_rss_table(efx, efx->rss_context.priv.context_id,
> +						rx_indir_table, key);
>  	if (rc != 0)
>  		goto fail2;
>  
> -	if (efx->rss_context.context_id != old_rx_rss_context &&
> +	if (efx->rss_context.priv.context_id != old_rx_rss_context &&
>  	    old_rx_rss_context != EFX_MCDI_RSS_CONTEXT_INVALID)
>  		WARN_ON(efx_mcdi_filter_free_rss_context(efx, old_rx_rss_context) != 0);
>  	table->rx_rss_context_exclusive = true;
> @@ -2091,9 +2094,9 @@ static int efx_mcdi_filter_rx_push_exclusive_rss_config(struct efx_nic *efx,
>  	return 0;
>  
>  fail2:
> -	if (old_rx_rss_context != efx->rss_context.context_id) {
> -		WARN_ON(efx_mcdi_filter_free_rss_context(efx, efx->rss_context.context_id) != 0);
> -		efx->rss_context.context_id = old_rx_rss_context;
> +	if (old_rx_rss_context != efx->rss_context.priv.context_id) {
> +		WARN_ON(efx_mcdi_filter_free_rss_context(efx, efx->rss_context.priv.context_id) != 0);
> +		efx->rss_context.priv.context_id = old_rx_rss_context;
>  	}
>  fail1:
>  	netif_err(efx, hw, efx->net_dev, "%s: failed rc=%d\n", __func__, rc);
> @@ -2101,33 +2104,28 @@ static int efx_mcdi_filter_rx_push_exclusive_rss_config(struct efx_nic *efx,
>  }
>  
>  int efx_mcdi_rx_push_rss_context_config(struct efx_nic *efx,
> -					struct efx_rss_context *ctx,
> +					struct efx_rss_context_priv *ctx,
>  					const u32 *rx_indir_table,
> -					const u8 *key)
> +					const u8 *key, bool delete)
>  {
>  	int rc;
>  
> -	WARN_ON(!mutex_is_locked(&efx->rss_lock));
> +	WARN_ON(!mutex_is_locked(&efx->net_dev->ethtool->rss_lock));
>  
>  	if (ctx->context_id == EFX_MCDI_RSS_CONTEXT_INVALID) {
> +		if (delete)
> +			/* already wasn't in HW, nothing to do */
> +			return 0;
>  		rc = efx_mcdi_filter_alloc_rss_context(efx, true, ctx, NULL);
>  		if (rc)
>  			return rc;
>  	}
>  
> -	if (!rx_indir_table) /* Delete this context */
> +	if (delete) /* Delete this context */
>  		return efx_mcdi_filter_free_rss_context(efx, ctx->context_id);
>  
> -	rc = efx_mcdi_filter_populate_rss_table(efx, ctx->context_id,
> -					 rx_indir_table, key);
> -	if (rc)
> -		return rc;
> -
> -	memcpy(ctx->rx_indir_table, rx_indir_table,
> -	       sizeof(efx->rss_context.rx_indir_table));
> -	memcpy(ctx->rx_hash_key, key, efx->type->rx_hash_key_size);
> -
> -	return 0;
> +	return efx_mcdi_filter_populate_rss_table(efx, ctx->context_id,
> +						  rx_indir_table, key);
>  }
>  
>  int efx_mcdi_rx_pull_rss_context_config(struct efx_nic *efx,
> @@ -2139,16 +2137,16 @@ int efx_mcdi_rx_pull_rss_context_config(struct efx_nic *efx,
>  	size_t outlen;
>  	int rc, i;
>  
> -	WARN_ON(!mutex_is_locked(&efx->rss_lock));
> +	WARN_ON(!mutex_is_locked(&efx->net_dev->ethtool->rss_lock));
>  
>  	BUILD_BUG_ON(MC_CMD_RSS_CONTEXT_GET_TABLE_IN_LEN !=
>  		     MC_CMD_RSS_CONTEXT_GET_KEY_IN_LEN);
>  
> -	if (ctx->context_id == EFX_MCDI_RSS_CONTEXT_INVALID)
> +	if (ctx->priv.context_id == EFX_MCDI_RSS_CONTEXT_INVALID)
>  		return -ENOENT;
>  
>  	MCDI_SET_DWORD(inbuf, RSS_CONTEXT_GET_TABLE_IN_RSS_CONTEXT_ID,
> -		       ctx->context_id);
> +		       ctx->priv.context_id);
>  	BUILD_BUG_ON(ARRAY_SIZE(ctx->rx_indir_table) !=
>  		     MC_CMD_RSS_CONTEXT_GET_TABLE_OUT_INDIRECTION_TABLE_LEN);
>  	rc = efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_GET_TABLE, inbuf, sizeof(inbuf),
> @@ -2164,7 +2162,7 @@ int efx_mcdi_rx_pull_rss_context_config(struct efx_nic *efx,
>  				RSS_CONTEXT_GET_TABLE_OUT_INDIRECTION_TABLE)[i];
>  
>  	MCDI_SET_DWORD(inbuf, RSS_CONTEXT_GET_KEY_IN_RSS_CONTEXT_ID,
> -		       ctx->context_id);
> +		       ctx->priv.context_id);
>  	BUILD_BUG_ON(ARRAY_SIZE(ctx->rx_hash_key) !=
>  		     MC_CMD_RSS_CONTEXT_SET_KEY_IN_TOEPLITZ_KEY_LEN);
>  	rc = efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_GET_KEY, inbuf, sizeof(inbuf),
> @@ -2186,35 +2184,42 @@ int efx_mcdi_rx_pull_rss_config(struct efx_nic *efx)
>  {
>  	int rc;
>  
> -	mutex_lock(&efx->rss_lock);
> +	mutex_lock(&efx->net_dev->ethtool->rss_lock);
>  	rc = efx_mcdi_rx_pull_rss_context_config(efx, &efx->rss_context);
> -	mutex_unlock(&efx->rss_lock);
> +	mutex_unlock(&efx->net_dev->ethtool->rss_lock);
>  	return rc;
>  }
>  
>  void efx_mcdi_rx_restore_rss_contexts(struct efx_nic *efx)
>  {
>  	struct efx_mcdi_filter_table *table = efx->filter_state;
> -	struct efx_rss_context *ctx;
> +	struct ethtool_rxfh_context *ctx;
> +	unsigned long context;
>  	int rc;
>  
> -	WARN_ON(!mutex_is_locked(&efx->rss_lock));
> +	WARN_ON(!mutex_is_locked(&efx->net_dev->ethtool->rss_lock));
>  
>  	if (!table->must_restore_rss_contexts)
>  		return;
>  
> -	list_for_each_entry(ctx, &efx->rss_context.list, list) {
> +	xa_for_each(&efx->net_dev->ethtool->rss_ctx, context, ctx) {
> +		struct efx_rss_context_priv *priv;
> +		u32 *indir;
> +		u8 *key;
> +
> +		priv = ethtool_rxfh_context_priv(ctx);
>  		/* previous NIC RSS context is gone */
> -		ctx->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
> +		priv->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
>  		/* so try to allocate a new one */
> -		rc = efx_mcdi_rx_push_rss_context_config(efx, ctx,
> -							 ctx->rx_indir_table,
> -							 ctx->rx_hash_key);
> +		indir = ethtool_rxfh_context_indir(ctx);
> +		key = ethtool_rxfh_context_key(ctx);
> +		rc = efx_mcdi_rx_push_rss_context_config(efx, priv, indir, key,
> +							 false);
>  		if (rc)
>  			netif_warn(efx, probe, efx->net_dev,
> -				   "failed to restore RSS context %u, rc=%d"
> +				   "failed to restore RSS context %lu, rc=%d"
>  				   "; RSS filters may fail to be applied\n",
> -				   ctx->user_id, rc);
> +				   context, rc);

If this fails the state in the core is out-of-sync with that in the NIC.
Should we remove the RSS context from efx->net_dev->ethtool->rss_ctx, or do
we expect admins to do that manually?

Martin

>  	}
>  	table->must_restore_rss_contexts = false;
>  }
> @@ -2276,7 +2281,7 @@ int efx_mcdi_vf_rx_push_rss_config(struct efx_nic *efx, bool user,
>  {
>  	if (user)
>  		return -EOPNOTSUPP;
> -	if (efx->rss_context.context_id != EFX_MCDI_RSS_CONTEXT_INVALID)
> +	if (efx->rss_context.priv.context_id != EFX_MCDI_RSS_CONTEXT_INVALID)
>  		return 0;
>  	return efx_mcdi_filter_rx_push_shared_rss_config(efx, NULL);
>  }
> @@ -2295,7 +2300,7 @@ int efx_mcdi_push_default_indir_table(struct efx_nic *efx,
>  
>  	efx_mcdi_rx_free_indir_table(efx);
>  	if (rss_spread > 1) {
> -		efx_set_default_rx_indir_table(efx, &efx->rss_context);
> +		efx_set_default_rx_indir_table(efx, efx->rss_context.rx_indir_table);
>  		rc = efx->type->rx_push_rss_config(efx, false,
>  				   efx->rss_context.rx_indir_table, NULL);
>  	}
> diff --git a/drivers/net/ethernet/sfc/mcdi_filters.h b/drivers/net/ethernet/sfc/mcdi_filters.h
> index c0d6558b9fd2..11b9f87ed9e1 100644
> --- a/drivers/net/ethernet/sfc/mcdi_filters.h
> +++ b/drivers/net/ethernet/sfc/mcdi_filters.h
> @@ -145,9 +145,9 @@ void efx_mcdi_filter_del_vlan(struct efx_nic *efx, u16 vid);
>  
>  void efx_mcdi_rx_free_indir_table(struct efx_nic *efx);
>  int efx_mcdi_rx_push_rss_context_config(struct efx_nic *efx,
> -					struct efx_rss_context *ctx,
> +					struct efx_rss_context_priv *ctx,
>  					const u32 *rx_indir_table,
> -					const u8 *key);
> +					const u8 *key, bool delete);
>  int efx_mcdi_pf_rx_push_rss_config(struct efx_nic *efx, bool user,
>  				   const u32 *rx_indir_table,
>  				   const u8 *key);
> @@ -161,10 +161,6 @@ int efx_mcdi_push_default_indir_table(struct efx_nic *efx,
>  int efx_mcdi_rx_pull_rss_config(struct efx_nic *efx);
>  int efx_mcdi_rx_pull_rss_context_config(struct efx_nic *efx,
>  					struct efx_rss_context *ctx);
> -int efx_mcdi_get_rss_context_flags(struct efx_nic *efx, u32 context,
> -				   u32 *flags);
> -void efx_mcdi_set_rss_context_flags(struct efx_nic *efx,
> -				    struct efx_rss_context *ctx);
>  void efx_mcdi_rx_restore_rss_contexts(struct efx_nic *efx);
>  
>  static inline void efx_mcdi_update_rx_scatter(struct efx_nic *efx)
> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
> index 27d86e90a3bb..d2a54a03f84d 100644
> --- a/drivers/net/ethernet/sfc/net_driver.h
> +++ b/drivers/net/ethernet/sfc/net_driver.h
> @@ -737,21 +737,24 @@ struct vfdi_status;
>  /* The reserved RSS context value */
>  #define EFX_MCDI_RSS_CONTEXT_INVALID	0xffffffff
>  /**
> - * struct efx_rss_context - A user-defined RSS context for filtering
> - * @list: node of linked list on which this struct is stored
> + * struct efx_rss_context_priv - driver private data for an RSS context
>   * @context_id: the RSS_CONTEXT_ID returned by MC firmware, or
>   *	%EFX_MCDI_RSS_CONTEXT_INVALID if this context is not present on the NIC.
> - *	For Siena, 0 if RSS is active, else %EFX_MCDI_RSS_CONTEXT_INVALID.
> - * @user_id: the rss_context ID exposed to userspace over ethtool.
>   * @rx_hash_udp_4tuple: UDP 4-tuple hashing enabled
> + */
> +struct efx_rss_context_priv {
> +	u32 context_id;
> +	bool rx_hash_udp_4tuple;
> +};
> +
> +/**
> + * struct efx_rss_context - an RSS context
> + * @priv: hardware-specific state
>   * @rx_hash_key: Toeplitz hash key for this RSS context
>   * @indir_table: Indirection table for this RSS context
>   */
>  struct efx_rss_context {
> -	struct list_head list;
> -	u32 context_id;
> -	u32 user_id;
> -	bool rx_hash_udp_4tuple;
> +	struct efx_rss_context_priv priv;
>  	u8 rx_hash_key[40];
>  	u32 rx_indir_table[128];
>  };
> @@ -883,9 +886,7 @@ struct efx_mae;
>   * @rx_packet_ts_offset: Offset of timestamp from start of packet data
>   *	(valid only if channel->sync_timestamps_enabled; always negative)
>   * @rx_scatter: Scatter mode enabled for receives
> - * @rss_context: Main RSS context.  Its @list member is the head of the list of
> - *	RSS contexts created by user requests
> - * @rss_lock: Protects custom RSS context software state in @rss_context.list
> + * @rss_context: Main RSS context.
>   * @vport_id: The function's vport ID, only relevant for PFs
>   * @int_error_count: Number of internal errors seen recently
>   * @int_error_expire: Time at which error count will be expired
> @@ -1052,7 +1053,6 @@ struct efx_nic {
>  	int rx_packet_ts_offset;
>  	bool rx_scatter;
>  	struct efx_rss_context rss_context;
> -	struct mutex rss_lock;
>  	u32 vport_id;
>  
>  	unsigned int_error_count;
> @@ -1416,9 +1416,9 @@ struct efx_nic_type {
>  				  const u32 *rx_indir_table, const u8 *key);
>  	int (*rx_pull_rss_config)(struct efx_nic *efx);
>  	int (*rx_push_rss_context_config)(struct efx_nic *efx,
> -					  struct efx_rss_context *ctx,
> +					  struct efx_rss_context_priv *ctx,
>  					  const u32 *rx_indir_table,
> -					  const u8 *key);
> +					  const u8 *key, bool delete);
>  	int (*rx_pull_rss_context_config)(struct efx_nic *efx,
>  					  struct efx_rss_context *ctx);
>  	void (*rx_restore_rss_contexts)(struct efx_nic *efx);
> diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
> index d2f35ee15eff..c79cec9050c5 100644
> --- a/drivers/net/ethernet/sfc/rx_common.c
> +++ b/drivers/net/ethernet/sfc/rx_common.c
> @@ -556,69 +556,25 @@ efx_rx_packet_gro(struct efx_channel *channel, struct efx_rx_buffer *rx_buf,
>  	napi_gro_frags(napi);
>  }
>  
> -/* RSS contexts.  We're using linked lists and crappy O(n) algorithms, because
> - * (a) this is an infrequent control-plane operation and (b) n is small (max 64)
> - */
> -struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
> +struct efx_rss_context_priv *efx_find_rss_context_entry(struct efx_nic *efx,
> +							u32 id)
>  {
> -	struct list_head *head = &efx->rss_context.list;
> -	struct efx_rss_context *ctx, *new;
> -	u32 id = 1; /* Don't use zero, that refers to the master RSS context */
> -
> -	WARN_ON(!mutex_is_locked(&efx->rss_lock));
> +	struct ethtool_rxfh_context *ctx;
>  
> -	/* Search for first gap in the numbering */
> -	list_for_each_entry(ctx, head, list) {
> -		if (ctx->user_id != id)
> -			break;
> -		id++;
> -		/* Check for wrap.  If this happens, we have nearly 2^32
> -		 * allocated RSS contexts, which seems unlikely.
> -		 */
> -		if (WARN_ON_ONCE(!id))
> -			return NULL;
> -	}
> +	WARN_ON(!mutex_is_locked(&efx->net_dev->ethtool->rss_lock));
>  
> -	/* Create the new entry */
> -	new = kmalloc(sizeof(*new), GFP_KERNEL);
> -	if (!new)
> +	ctx = xa_load(&efx->net_dev->ethtool->rss_ctx, id);
> +	if (!ctx)
>  		return NULL;
> -	new->context_id = EFX_MCDI_RSS_CONTEXT_INVALID;
> -	new->rx_hash_udp_4tuple = false;
> -
> -	/* Insert the new entry into the gap */
> -	new->user_id = id;
> -	list_add_tail(&new->list, &ctx->list);
> -	return new;
> -}
> -
> -struct efx_rss_context *efx_find_rss_context_entry(struct efx_nic *efx, u32 id)
> -{
> -	struct list_head *head = &efx->rss_context.list;
> -	struct efx_rss_context *ctx;
> -
> -	WARN_ON(!mutex_is_locked(&efx->rss_lock));
> -
> -	list_for_each_entry(ctx, head, list)
> -		if (ctx->user_id == id)
> -			return ctx;
> -	return NULL;
> -}
> -
> -void efx_free_rss_context_entry(struct efx_rss_context *ctx)
> -{
> -	list_del(&ctx->list);
> -	kfree(ctx);
> +	return ethtool_rxfh_context_priv(ctx);
>  }
>  
> -void efx_set_default_rx_indir_table(struct efx_nic *efx,
> -				    struct efx_rss_context *ctx)
> +void efx_set_default_rx_indir_table(struct efx_nic *efx, u32 *indir)
>  {
>  	size_t i;
>  
> -	for (i = 0; i < ARRAY_SIZE(ctx->rx_indir_table); i++)
> -		ctx->rx_indir_table[i] =
> -			ethtool_rxfh_indir_default(i, efx->rss_spread);
> +	for (i = 0; i < ARRAY_SIZE(efx->rss_context.rx_indir_table); i++)
> +		indir[i] = ethtool_rxfh_indir_default(i, efx->rss_spread);
>  }
>  
>  /**
> diff --git a/drivers/net/ethernet/sfc/rx_common.h b/drivers/net/ethernet/sfc/rx_common.h
> index fbd2769307f9..75fa84192362 100644
> --- a/drivers/net/ethernet/sfc/rx_common.h
> +++ b/drivers/net/ethernet/sfc/rx_common.h
> @@ -84,11 +84,9 @@ void
>  efx_rx_packet_gro(struct efx_channel *channel, struct efx_rx_buffer *rx_buf,
>  		  unsigned int n_frags, u8 *eh, __wsum csum);
>  
> -struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx);
> -struct efx_rss_context *efx_find_rss_context_entry(struct efx_nic *efx, u32 id);
> -void efx_free_rss_context_entry(struct efx_rss_context *ctx);
> -void efx_set_default_rx_indir_table(struct efx_nic *efx,
> -				    struct efx_rss_context *ctx);
> +struct efx_rss_context_priv *efx_find_rss_context_entry(struct efx_nic *efx,
> +							u32 id);
> +void efx_set_default_rx_indir_table(struct efx_nic *efx, u32 *indir);
>  
>  bool efx_filter_is_mc_recipient(const struct efx_filter_spec *spec);
>  bool efx_filter_spec_equal(const struct efx_filter_spec *left,

Re: [PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[Jakub Kicinski]

On Fri, 29 Sep 2023 11:17:53 -0700 Jacob Keller wrote:
> > +struct ethtool_rxfh_context {
> > +	u32 indir_size;
> > +	u32 key_size;
> > +	u8 hfunc;
> > +	u16 priv_size;
> > +	u8 indir_no_change:1;
> > +	u8 key_no_change:1;
> > +	/* private: driver private data, indirection table, and hash key are
> > +	 * stored sequentially in @data area.  Use below helpers to access.
> > +	 */
> > +	u8 data[] __aligned(sizeof(void *));
> > +};  
> 
> Is it not feasible to use container_of to get to private data for the
> drivers? I guess this change in particular doesn't actually include any
> users yet...

That could work in general but here specifically there are also 
2 variable-size arrays hiding in data[].

Re: [PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[Jakub Kicinski]

On Wed, 27 Sep 2023 19:13:33 +0100 edward.cree@amd.com wrote:
> +	struct ethtool_rxfh_context *ctx;
> +	unsigned long context;
> +
> +	if (dev->ethtool_ops->set_rxfh_context)

Can there be contexts if there's no callback to create them?
Perhaps you need this for later patches but would be good to
mention "why" in the commit message.

> +		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
> +			u32 *indir = ethtool_rxfh_context_indir(ctx);
> +			u8 *key = ethtool_rxfh_context_key(ctx);
> +			u32 concast = context;
> +
> +			xa_erase(&dev->ethtool->rss_ctx, context);
> +			dev->ethtool_ops->set_rxfh_context(dev, indir, key,
> +							   ctx->hfunc, &concast,
> +							   true);
> +			kfree(ctx);
> +		}

Re: [PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[Jakub Kicinski]

On Wed, 27 Sep 2023 19:13:33 +0100 edward.cree@amd.com wrote:
>  /**
>   * struct ethtool_netdev_state - per-netdevice state for ethtool features
> + * @rss_ctx:		XArray of custom RSS contexts
> + * @rss_ctx_max_id:	maximum (exclusive) supported RSS context ID

Is this one set by the driver? How would it be set?
It'd be good if drivers didn't access ethtool state directly.
Makes core easier to refactor if the API is constrained.

>   * @wol_enabled:	Wake-on-LAN is enabled
>   */
>  struct ethtool_netdev_state {
> -	unsigned		wol_enabled:1;
> +	struct xarray		rss_ctx;
> +	u32			rss_ctx_max_id;
> +	u32			wol_enabled:1;

Re: [PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

[Jakub Kicinski]

On Wed, 27 Sep 2023 19:13:37 +0100 edward.cree@amd.com wrote:
> While this is not needed to serialise the ethtool entry points (which
>  are all under RTNL), drivers may have cause to asynchronously access
>  dev->ethtool->rss_ctx; taking dev->ethtool->rss_lock allows them to
>  do this safely without needing to take the RTNL.

Can we use a replay mechanism, like we do in TC offloads and VxLAN/UDP
ports? The driver which lost config can ask for the rss contexts to be
"replayed" and the core will issue a series of ->create calls for all
existing entries?

Regarding the lock itself - can we hide it under ethtool_rss_lock(dev)
/ ethtool_rss_unlock(dev) helpers?

Re: [PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[Edward Cree]

On 05/10/2023 00:00, Jakub Kicinski wrote:
> On Wed, 27 Sep 2023 19:13:33 +0100 edward.cree@amd.com wrote:
>> +	struct ethtool_rxfh_context *ctx;
>> +	unsigned long context;
>> +
>> +	if (dev->ethtool_ops->set_rxfh_context)
> 
> Can there be contexts if there's no callback to create them?

I don't believe so.  But maybe making that load-bearing isn't great...

> Perhaps you need this for later patches but would be good to
> mention "why" in the commit message.

Well, the loop below tries to call ->set_rxfh_context, which wouldn't
 go too well if there's no callback.  But I guess the code makes more
 sense to read if this just guards the actual call and not the kfree.

-ed

>> +		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
>> +			u32 *indir = ethtool_rxfh_context_indir(ctx);
>> +			u8 *key = ethtool_rxfh_context_key(ctx);
>> +			u32 concast = context;
>> +
>> +			xa_erase(&dev->ethtool->rss_ctx, context);
>> +			dev->ethtool_ops->set_rxfh_context(dev, indir, key,
>> +							   ctx->hfunc, &concast,
>> +							   true);
>> +			kfree(ctx);
>> +		}

Re: [PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[Edward Cree]

On 05/10/2023 00:10, Jakub Kicinski wrote:
> On Wed, 27 Sep 2023 19:13:33 +0100 edward.cree@amd.com wrote:
>>  /**
>>   * struct ethtool_netdev_state - per-netdevice state for ethtool features
>> + * @rss_ctx:		XArray of custom RSS contexts
>> + * @rss_ctx_max_id:	maximum (exclusive) supported RSS context ID
> 
> Is this one set by the driver? How would it be set?

I was thinking drivers would just assign this directly in their
 probe routine.

> It'd be good if drivers didn't access ethtool state directly.
> Makes core easier to refactor if the API is constrained.

Would you prefer it as a getter in the ethtool ops?  The core
 would call it every time a new context is being allocated.

(In any case, arguably I shouldn't add it in this patch as it's
 not used until patch #4; I'll fix that in v5 either way.)

Re: [PATCH v4 net-next 7/7] sfc: use new rxfh_context API

[Edward Cree]

On 02/10/2023 14:01, Martin Habets wrote:
> On Wed, Sep 27, 2023 at 07:13:38PM +0100, edward.cree@amd.com wrote:
>> From: Edward Cree <ecree.xilinx@gmail.com>
>>
>> The core is now responsible for allocating IDs and a memory region for
>>  us to store our state (struct efx_rss_context_priv), so we no longer
>>  need efx_alloc_rss_context_entry() and friends.
>> Since the contexts are now maintained by the core, use the core's lock
>>  (net_dev->ethtool->rss_lock), rather than our own mutex (efx->rss_lock),
>>  to serialise access against changes; and remove the now-unused
>>  efx->rss_lock from struct efx_nic.
>>
>> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
...
>> +		rc = efx_mcdi_rx_push_rss_context_config(efx, priv, indir, key,
>> +							 false);
>>  		if (rc)
>>  			netif_warn(efx, probe, efx->net_dev,
>> -				   "failed to restore RSS context %u, rc=%d"
>> +				   "failed to restore RSS context %lu, rc=%d"
>>  				   "; RSS filters may fail to be applied\n",
>> -				   ctx->user_id, rc);
>> +				   context, rc);
> 
> If this fails the state in the core is out-of-sync with that in the NIC.
> Should we remove the RSS context from efx->net_dev->ethtool->rss_ctx, or do
> we expect admins to do that manually?
> 
> Martin

I definitely think it's a bad idea to have drivers removing things
 from the array behind the kernel's back.  Besides, I think it ought
 to keep the configuration the user asked for, so that if they fix
 something and then trigger another MC reboot the original config
 will get reapplied.
Possibly if we go with Jakub's suggestion of a replay mechanism
 then the core can react to failure returns from the reoffload call,
 either by removing the context from the array or by setting some
 kind of 'broken' flag that can be reported to userspace in dumps.

-ed

Re: [PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

[Edward Cree]

On 05/10/2023 00:16, Jakub Kicinski wrote:
> On Wed, 27 Sep 2023 19:13:37 +0100 edward.cree@amd.com wrote:
>> While this is not needed to serialise the ethtool entry points (which
>>  are all under RTNL), drivers may have cause to asynchronously access
>>  dev->ethtool->rss_ctx; taking dev->ethtool->rss_lock allows them to
>>  do this safely without needing to take the RTNL.
> 
> Can we use a replay mechanism, like we do in TC offloads and VxLAN/UDP
> ports? The driver which lost config can ask for the rss contexts to be
> "replayed" and the core will issue a series of ->create calls for all
> existing entries?

I like that idea, yes.  Will try to implement it for v5.
There is a question as to how the core should react if the ->create call
 then fails; see my reply to Martin on #7.

> Regarding the lock itself - can we hide it under ethtool_rss_lock(dev)
> / ethtool_rss_unlock(dev) helpers?

Sure.  If I can't get replay to work then I'll do that.

-ed

Re: [PATCH v4 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

[Jakub Kicinski]

On Thu, 5 Oct 2023 19:43:36 +0100 Edward Cree wrote:
> > Is this one set by the driver? How would it be set?  
> 
> I was thinking drivers would just assign this directly in their
>  probe routine.
> 
> > It'd be good if drivers didn't access ethtool state directly.
> > Makes core easier to refactor if the API is constrained.  
> 
> Would you prefer it as a getter in the ethtool ops?  The core
>  would call it every time a new context is being allocated.

If ops work that'd be simplest. I was worried that the exact limit 
may need to be established at runtime based on FW/HW gen, and that's
why you place it here. But that's a more general problem with the
current simplistic approach of sticking all the capabilities into ops.
If you only need a static cofing we can go with ops and add runtime
corrections for all caps later on.

Re: [PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

[Jakub Kicinski]

On Thu, 5 Oct 2023 21:56:47 +0100 Edward Cree wrote:
> > Can we use a replay mechanism, like we do in TC offloads and VxLAN/UDP
> > ports? The driver which lost config can ask for the rss contexts to be
> > "replayed" and the core will issue a series of ->create calls for all
> > existing entries?  
> 
> I like that idea, yes.  Will try to implement it for v5.
> There is a question as to how the core should react if the ->create call
>  then fails; see my reply to Martin on #7.

Hm. The application asked for a config which is no longer applied.
The machine needs to raise some form of an alarm or be taken out
of commission. My first thought would be to print an error message
in the core, and expect the driver to fail some devlink health
reporter.

I don't think a "broken" flag local to RSS would be monitored, there'd
be too many of such local flags throughout the APIs. devlink health
may be monitored.

> > Regarding the lock itself - can we hide it under ethtool_rss_lock(dev)
> > / ethtool_rss_unlock(dev) helpers?  
> 
> Sure.  If I can't get replay to work then I'll do that.

Re: [PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

[Edward Cree]

On 05/10/2023 00:16, Jakub Kicinski wrote:
> On Wed, 27 Sep 2023 19:13:37 +0100 edward.cree@amd.com wrote:
>> While this is not needed to serialise the ethtool entry points (which
>>  are all under RTNL), drivers may have cause to asynchronously access
>>  dev->ethtool->rss_ctx; taking dev->ethtool->rss_lock allows them to
>>  do this safely without needing to take the RTNL.
> 
> Can we use a replay mechanism, like we do in TC offloads and VxLAN/UDP
> ports? The driver which lost config can ask for the rss contexts to be
> "replayed" and the core will issue a series of ->create calls for all
> existing entries?
So I tried to prototype this, and unfortunately I ran into a problem.
While we can replay the contexts alright, that still leaves the ntuple
 filters which we also want to restore, and which might depend on the
 contexts, so that can't be done until after context restore is done.
So to do this we'd need to *also* have the core replay the filters,
 which would mean adding a filter array to the core similar to this
 context array.  Now that's a thing that might be useful to have,
 enabling netlink dumps and so on, but it would considerably extend
 the scope of this work, in which case who knows if it'll ever be
 ready to merge :S
Moreover, at least in the case of sfc (as usual, no idea about other
 NICs), the filter table on the device contains more than just ntuple
 filters; stuff like the device's unicast address list, PTP filters
 and representor filters, some of which are required for correct
 operation, live in the same table.
When coming up after a reset, currently we:
1) restore RSS contexts
2) restore all filters (both driver-internal and ethtool ntuple) from
   the software shadow filter table into the hardware
3) bring up the NIC datapath.
Instead we would need to:
1) restore all the 'internal' filters (which do not, and after these
   changes could not ever, use custom RSS contexts), and discard all
   ntuple filters from the software shadow filter table in the driver
2) request RSS+ntuple replay
3) bring up the NIC datapath
4) the replay workitem runs, and reinserts RSS contexts and ntuple
   filters.
This would also mean that the default RSS context, which is used by
 the unicast/multicast address and Ethernet broadcast filters, could
 not ever migrate to be tracked in the XArray (otherwise a desirable
 simplification).

tl;dr: none of this is impossible, but it'd be a lot of work just to
 get rid of one mutex, and would paint us into a bit of a corner.
So I propose to stick with the locking scheme for now; I'll post v5
 with the other review comments addressed; and if at some point in
 the future core tracking of ntuple filters gets added, we can
 revisit then whether moving to a replay scheme is viable.  Okay?

-ed

Re: [PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

[Jakub Kicinski]

On Thu, 7 Dec 2023 14:15:30 +0000 Edward Cree wrote:
> tl;dr: none of this is impossible, but it'd be a lot of work just to
>  get rid of one mutex, and would paint us into a bit of a corner.
> So I propose to stick with the locking scheme for now; I'll post v5
>  with the other review comments addressed; and if at some point in
>  the future core tracking of ntuple filters gets added, we can
>  revisit then whether moving to a replay scheme is viable.  Okay?

Sounds good, thanks for investigating.