[PATCH net-next v2 0/2] net: macb: implement ethtool set channels count operation

[PATCH net-next v2 0/2] net: macb: implement ethtool set channels count operation

[Théo Lebrun]

Add support for changing the active number of queues. Tested on Mobileye
EyeQ5. The first patch is as expected. However the second one might be
more surprising:

GEM has per-queue Tx SRAM segmentation. If we do not touch SRAM
distribution then we'll only be able to exploit a portion of it when a
smaller queue count is configured. It also is beneficial if bootloader
stages write to the register and we don't reset it but attempt to use
all queues (the default).

The operation is only hidden behind MACB_CAPS_QUEUE_DISABLE, we do not
introduce yet another feature flag.

As this series must s/num_queues/max_num_queues/ quite a lot, it
conflicts with other in-flights series touching MACB in the netdev ML.
I'll make sure to resend once net-next gets updated.

Have a nice day,
Thanks,
Théo

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Changes in v2:
- Move gem_sram_distribute_segments() from macb_main.c to macb_utils.c
  and kunit tests from macb_main.c to macb_kunit.c.
- macb_set_channels():
  - Add comment about MACB_CAPS_QUEUE_DISABLE.
  - Refuse operation if netif_running(), with code comment.
  - Drop useless sanity checks on ch->{combined,rx,tx}_count.
- Add help text to CONFIG_MACB_KUNIT_TEST.
- Fix `checkpatch --max-line-length=80` warnings.
- Rebase onto latest net-next; nothing to report.
- Link to v1: https://lore.kernel.org/r/20260305-macb-set-channels-v1-0-28e3a96a3dc3@bootlin.com

---
Théo Lebrun (2):
      net: macb: implement ethtool_ops.get|set_channels()
      net: macb: distribute evenly Tx SRAM segments

 drivers/net/ethernet/cadence/Kconfig      |  13 ++++
 drivers/net/ethernet/cadence/Makefile     |   5 +-
 drivers/net/ethernet/cadence/macb.h       |   9 +++
 drivers/net/ethernet/cadence/macb_kunit.c |  73 ++++++++++++++++++
 drivers/net/ethernet/cadence/macb_main.c  | 118 ++++++++++++++++++++++++++----
 drivers/net/ethernet/cadence/macb_utils.c |  56 ++++++++++++++
 6 files changed, 257 insertions(+), 17 deletions(-)
---
base-commit: df99d268555f52fb2ac3b9fc9a6198ea0579c48a
change-id: 20260305-macb-set-channels-5bf6e07f3270

Best regards,
--  
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH net-next v2 1/2] net: macb: implement ethtool_ops.get|set_channels()

[Théo Lebrun]

bp->num_queues is the total number of queues and is constant from probe.
Introduce bp->max_num_queues which takes the current role of
bp->num_queues and allow `0 < bp->num_queues <= bp->max_num_queues`.
MACB/GEM does not know about rx/tx specific queues; it only has
combined queues.

.set_channels() is reserved to devices with MACB_CAPS_QUEUE_DISABLE.
The tieoff workaround would not work as packets would still be routed
into queues with a tieoff descriptor.

Implement .set_channels() operation by refusing if netif_running().
The reason to implement .set_channels() is memory savings, we cannot
preallocate and swap at runtime.

Nit: fix an alignment issue inside gem_ethtool_ops which does not
deserve its own patch.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb.h      |   1 +
 drivers/net/ethernet/cadence/macb_main.c | 103 ++++++++++++++++++++++++++-----
 2 files changed, 88 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index c69828b27dae..b08afe340996 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1309,6 +1309,7 @@ struct macb {
 	unsigned int		tx_ring_size;
 
 	unsigned int		num_queues;
+	unsigned int		max_num_queues;
 	struct macb_queue	queues[MACB_MAX_QUEUES];
 
 	spinlock_t		lock;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 3dcae4d5f74c..8b2c77446dbd 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -467,9 +467,26 @@ static void macb_init_buffers(struct macb *bp)
 			    upper_32_bits(bp->queues[0].tx_ring_dma));
 	}
 
-	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
-		queue_writel(queue, RBQP, lower_32_bits(queue->rx_ring_dma));
-		queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
+	for (q = 0, queue = bp->queues; q < bp->max_num_queues; ++q, ++queue) {
+		if (q < bp->num_queues) {
+			queue_writel(queue, RBQP,
+				     lower_32_bits(queue->rx_ring_dma));
+			queue_writel(queue, TBQP,
+				     lower_32_bits(queue->tx_ring_dma));
+		} else {
+			/*
+			 * macb_set_channels(), which is the only way of writing
+			 * to bp->num_queues, is only allowed if
+			 * MACB_CAPS_QUEUE_DISABLE.
+			 */
+			queue_writel(queue, RBQP, MACB_BIT(QUEUE_DISABLE));
+
+			/* Disable all interrupts */
+			queue_writel(queue, IDR, -1);
+			queue_readl(queue, ISR);
+			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+				queue_writel(queue, ISR, -1);
+		}
 	}
 }
 
@@ -4022,8 +4039,8 @@ static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd)
 
 	switch (cmd->cmd) {
 	case ETHTOOL_SRXCLSRLINS:
-		if ((cmd->fs.location >= bp->max_tuples)
-				|| (cmd->fs.ring_cookie >= bp->num_queues)) {
+		if (cmd->fs.location >= bp->max_tuples ||
+		    cmd->fs.ring_cookie >= bp->max_num_queues) {
 			ret = -EINVAL;
 			break;
 		}
@@ -4041,6 +4058,54 @@ static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd)
 	return ret;
 }
 
+static void macb_get_channels(struct net_device *netdev,
+			      struct ethtool_channels *ch)
+{
+	struct macb *bp = netdev_priv(netdev);
+
+	ch->max_combined = bp->max_num_queues;
+	ch->combined_count = bp->num_queues;
+}
+
+static int macb_set_channels(struct net_device *netdev,
+			     struct ethtool_channels *ch)
+{
+	struct macb *bp = netdev_priv(netdev);
+	unsigned int old_count = bp->num_queues;
+	unsigned int count = ch->combined_count;
+	int ret = 0;
+
+	/*
+	 * MACB_CAPS_QUEUE_DISABLE means that the field QUEUE_DISABLE/BIT0 in
+	 * the per-queue RBQP register disables queue Rx. If we don't have that
+	 * capability we can have multiple queues but we must always run with
+	 * all enabled.
+	 */
+	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE))
+		return -EOPNOTSUPP;
+
+	/*
+	 * An ideal .set_channels() implementation uses upfront allocated
+	 * resources and swaps them in, bringing reliability under memory
+	 * pressure. However, here we implement it for memory savings in
+	 * setups with less than max number of queues active.
+	 *
+	 * Signal it by refusing .set_channels() once interface is opened.
+	 */
+	if (netif_running(bp->dev))
+		return -EBUSY;
+
+	if (count == old_count)
+		return 0;
+
+	ret = netif_set_real_num_queues(bp->dev, count, count);
+	if (ret)
+		return ret;
+
+	bp->num_queues = count;
+	return 0;
+}
+
 static const struct ethtool_ops macb_ethtool_ops = {
 	.get_regs_len		= macb_get_regs_len,
 	.get_regs		= macb_get_regs,
@@ -4056,6 +4121,8 @@ static const struct ethtool_ops macb_ethtool_ops = {
 	.set_link_ksettings     = macb_set_link_ksettings,
 	.get_ringparam		= macb_get_ringparam,
 	.set_ringparam		= macb_set_ringparam,
+	.get_channels		= macb_get_channels,
+	.set_channels		= macb_set_channels,
 };
 
 static int macb_get_eee(struct net_device *dev, struct ethtool_keee *eee)
@@ -4090,12 +4157,14 @@ static const struct ethtool_ops gem_ethtool_ops = {
 	.set_link_ksettings     = macb_set_link_ksettings,
 	.get_ringparam		= macb_get_ringparam,
 	.set_ringparam		= macb_set_ringparam,
-	.get_rxnfc			= gem_get_rxnfc,
-	.set_rxnfc			= gem_set_rxnfc,
-	.get_rx_ring_count		= gem_get_rx_ring_count,
-	.nway_reset			= phy_ethtool_nway_reset,
+	.get_rxnfc		= gem_get_rxnfc,
+	.set_rxnfc		= gem_set_rxnfc,
+	.get_rx_ring_count	= gem_get_rx_ring_count,
+	.nway_reset		= phy_ethtool_nway_reset,
 	.get_eee		= macb_get_eee,
 	.set_eee		= macb_set_eee,
+	.get_channels		= macb_get_channels,
+	.set_channels		= macb_set_channels,
 };
 
 static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
@@ -4236,9 +4305,9 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
 	size_t i;
 	int err;
 
-	if (conf->num_entries > bp->num_queues) {
+	if (conf->num_entries > bp->max_num_queues) {
 		netdev_err(ndev, "Too many TAPRIO entries: %zu > %d queues\n",
-			   conf->num_entries, bp->num_queues);
+			   conf->num_entries, bp->max_num_queues);
 		return -EINVAL;
 	}
 
@@ -4286,9 +4355,9 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
 
 		/* gate_mask must not select queues outside the valid queues */
 		queue_id = order_base_2(entry->gate_mask);
-		if (queue_id >= bp->num_queues) {
+		if (queue_id >= bp->max_num_queues) {
 			netdev_err(ndev, "Entry %zu: gate_mask 0x%x exceeds queue range (max_queues=%d)\n",
-				   i, entry->gate_mask, bp->num_queues);
+				   i, entry->gate_mask, bp->max_num_queues);
 			err = -EINVAL;
 			goto cleanup;
 		}
@@ -4348,7 +4417,7 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
 	/* All validations passed - proceed with hardware configuration */
 	scoped_guard(spinlock_irqsave, &bp->lock) {
 		/* Disable ENST queues if running before configuring */
-		queue_mask = BIT_U32(bp->num_queues) - 1;
+		queue_mask = BIT_U32(bp->max_num_queues) - 1;
 		gem_writel(bp, ENST_CONTROL,
 			   queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET);
 
@@ -4383,7 +4452,7 @@ static void macb_taprio_destroy(struct net_device *ndev)
 	unsigned int q;
 
 	netdev_reset_tc(ndev);
-	queue_mask = BIT_U32(bp->num_queues) - 1;
+	queue_mask = BIT_U32(bp->max_num_queues) - 1;
 
 	scoped_guard(spinlock_irqsave, &bp->lock) {
 		/* Single disable command for all queues */
@@ -4391,7 +4460,8 @@ static void macb_taprio_destroy(struct net_device *ndev)
 			   queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET);
 
 		/* Clear all queue ENST registers in batch */
-		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
+		for (q = 0, queue = bp->queues; q < bp->max_num_queues;
+		     ++q, ++queue) {
 			queue_writel(queue, ENST_START_TIME, 0);
 			queue_writel(queue, ENST_ON_TIME, 0);
 			queue_writel(queue, ENST_OFF_TIME, 0);
@@ -5651,6 +5721,7 @@ static int macb_probe(struct platform_device *pdev)
 		bp->macb_reg_writel = hw_writel;
 	}
 	bp->num_queues = num_queues;
+	bp->max_num_queues = num_queues;
 	bp->dma_burst_length = macb_config->dma_burst_length;
 	bp->pclk = pclk;
 	bp->hclk = hclk;

-- 
2.53.0

[PATCH net-next v2 2/2] net: macb: distribute evenly Tx SRAM segments

[Théo Lebrun]

GEM has registers to configure the Tx SRAM segments distribution across
queues. The reset value is apprioriate (even spread) but we need to
care if/when number of active queues is modified (or if we inherited
unevenly initialised hardware from bootloader).

To distribute segments, we take as input the number of queues
(bp->num_queues) and the number of segments (found inside DCFG6).
Its output is a number of segments for each queue, formatted as
powers-of-two (eg 2 for queue 0 means it has 2^2=4 segments).

As the distribution logic is quite complex (at least its initial
versions had bugs), it is kunit-tested in macb_kunit.c and the
implementation lives in macb_utils.c. To test:

⟩ env --unset=CROSS_COMPILE make ARCH=um mrproper
⟩ env --unset=CROSS_COMPILE ./tools/testing/kunit/kunit.py run \
        --kconfig_add CONFIG_NET=y \
        --kconfig_add CONFIG_COMMON_CLK=y \
        --kconfig_add CONFIG_MACB=y macb

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/Kconfig      | 13 ++++++
 drivers/net/ethernet/cadence/Makefile     |  5 ++-
 drivers/net/ethernet/cadence/macb.h       |  8 ++++
 drivers/net/ethernet/cadence/macb_kunit.c | 73 +++++++++++++++++++++++++++++++
 drivers/net/ethernet/cadence/macb_main.c  | 15 +++++++
 drivers/net/ethernet/cadence/macb_utils.c | 56 ++++++++++++++++++++++++
 6 files changed, 169 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index 5b2a461dfd28..65c8d6ef519b 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -51,4 +51,17 @@ config MACB_PCI
 	  To compile this driver as a module, choose M here: the module
 	  will be called macb_pci.
 
+config MACB_KUNIT_TEST
+	bool "KUnit test for MACB" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	depends on MACB
+	default KUNIT_ALL_TESTS
+	help
+	  Build KUnit tests for the MACB driver.
+
+	  For more information on KUnit and unit tests in general,
+	  please refer to the KUnit documentation.
+
+	  If unsure, say N.
+
 endif # NET_VENDOR_CADENCE
diff --git a/drivers/net/ethernet/cadence/Makefile b/drivers/net/ethernet/cadence/Makefile
index 1f33cdca9a3c..f5a0abb9fdaf 100644
--- a/drivers/net/ethernet/cadence/Makefile
+++ b/drivers/net/ethernet/cadence/Makefile
@@ -2,7 +2,7 @@
 #
 # Makefile for the Atmel network device drivers.
 #
-macb-y	:= macb_main.o
+macb-y	:= macb_main.o macb_utils.o
 
 ifeq ($(CONFIG_MACB_USE_HWSTAMP),y)
 macb-y	+= macb_ptp.o
@@ -10,3 +10,6 @@ endif
 
 obj-$(CONFIG_MACB) += macb.o
 obj-$(CONFIG_MACB_PCI) += macb_pci.o
+
+obj-$(CONFIG_MACB_KUNIT_TEST) += macb_kunit.o
+macb-test-y := macb_kunit.o macb_utils.o
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index b08afe340996..0464e774273a 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -197,6 +197,9 @@
 #define GEM_TXBDCTRL	0x04cc /* TX Buffer Descriptor control register */
 #define GEM_RXBDCTRL	0x04d0 /* RX Buffer Descriptor control register */
 
+#define GEM_TXQSEGALLOC_LOWER	0x05A0 /* Tx queue segment allocation (low) */
+#define GEM_TXQSEGALLOC_UPPER	0x05A4 /* Tx queue segment allocation (high) */
+
 /* Screener Type 2 match registers */
 #define GEM_SCRT2		0x540
 
@@ -549,6 +552,8 @@
 #define GEM_PBUF_CUTTHRU_SIZE			1
 #define GEM_DAW64_OFFSET			23
 #define GEM_DAW64_SIZE				1
+#define GEM_SEGMENTS_BIT_SIZE_OFFSET		16
+#define GEM_SEGMENTS_BIT_SIZE_SIZE		3
 
 /* Bitfields in DCFG8. */
 #define GEM_T1SCR_OFFSET			24
@@ -1494,4 +1499,7 @@ struct macb_queue_enst_config {
 	u8 queue_id;
 };
 
+u64 gem_sram_distribute_segments(unsigned int num_queues,
+				 unsigned int num_segments);
+
 #endif /* _MACB_H */
diff --git a/drivers/net/ethernet/cadence/macb_kunit.c b/drivers/net/ethernet/cadence/macb_kunit.c
new file mode 100644
index 000000000000..74b37669c00c
--- /dev/null
+++ b/drivers/net/ethernet/cadence/macb_kunit.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <kunit/test.h>
+#include <linux/netdevice.h>
+#include "macb.h"
+
+struct macb_sram_segments_case {
+	unsigned int num_queues, num_segments;
+};
+
+static void macb_sram_segments_test(struct kunit *test)
+{
+	const struct macb_sram_segments_case *p = test->param_value;
+	u64 val = gem_sram_distribute_segments(p->num_queues, p->num_segments);
+	unsigned int i, sum_segments = 0, max_assigned_segments;
+	unsigned int num_queues = min(p->num_queues, p->num_segments);
+
+	for (i = 0; i < num_queues; i++) {
+		unsigned int q_segments = (val >> (i * 4)) & 0b11;
+
+		q_segments = 1U << q_segments;
+		sum_segments += q_segments;
+		KUNIT_ASSERT_GT_MSG(test, q_segments, 0, "queue %d, val %#llx",
+				    i, val);
+	}
+
+	for (i = num_queues; i < 16; i++) {
+		unsigned int pow = (val >> (i * 4)) & 0b11;
+
+		KUNIT_ASSERT_EQ_MSG(test, pow, 0, "queue %d, val %#llx",
+				    i, val);
+	}
+
+	max_assigned_segments = min(p->num_segments, 8 * p->num_queues);
+	KUNIT_ASSERT_EQ_MSG(test, sum_segments, max_assigned_segments,
+			    "val %#llx", val);
+}
+
+struct macb_sram_segments_case macb_sram_segments_cases[] = {
+	/* num_segments can only be powers of two. */
+	{ .num_queues = 4,  .num_segments = 2 },
+	{ .num_queues = 1,  .num_segments = 16 },
+	{ .num_queues = 4,  .num_segments = 16 },
+	{ .num_queues = 5,  .num_segments = 16 },
+	{ .num_queues = 15, .num_segments = 16 },
+	{ .num_queues = 16, .num_segments = 16 },
+};
+
+static void macb_sram_segments_case_desc(struct macb_sram_segments_case *t,
+					 char *desc)
+{
+	u64 val = gem_sram_distribute_segments(t->num_queues, t->num_segments);
+
+	snprintf(desc, KUNIT_PARAM_DESC_SIZE,
+		 "num_queues=%d num_segments=%d TXQSEGALLOC=%#llx",
+		 t->num_queues, t->num_segments, val);
+}
+
+KUNIT_ARRAY_PARAM(macb_sram_segments,
+		  macb_sram_segments_cases,
+		  macb_sram_segments_case_desc);
+
+static struct kunit_case macb_test_cases[] = {
+	KUNIT_CASE_PARAM(macb_sram_segments_test,
+			 macb_sram_segments_gen_params),
+	{}
+};
+
+static struct kunit_suite macb_test_suite = {
+	.name = "macb",
+	.test_cases = macb_test_cases,
+};
+
+kunit_test_suite(macb_test_suite);
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 8b2c77446dbd..16d71468fca7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2954,6 +2954,21 @@ static void macb_init_hw(struct macb *bp)
 	if (bp->caps & MACB_CAPS_JUMBO)
 		bp->rx_frm_len_mask = MACB_RX_JFRMLEN_MASK;
 
+	/*
+	 * Distribute Tx SRAM segments evenly based on active number of queues.
+	 */
+	if (macb_is_gem(bp)) {
+		unsigned int num_segments;
+		u64 val;
+
+		num_segments = 1U << GEM_BFEXT(SEGMENTS_BIT_SIZE,
+					       gem_readl(bp, DCFG6));
+		val = gem_sram_distribute_segments(bp->num_queues,
+						   num_segments);
+		gem_writel(bp, TXQSEGALLOC_LOWER, val);
+		gem_writel(bp, TXQSEGALLOC_UPPER, val >> 32);
+	}
+
 	macb_configure_dma(bp);
 
 	/* Enable RX partial store and forward and set watermark */
diff --git a/drivers/net/ethernet/cadence/macb_utils.c b/drivers/net/ethernet/cadence/macb_utils.c
new file mode 100644
index 000000000000..77e0b5c1df86
--- /dev/null
+++ b/drivers/net/ethernet/cadence/macb_utils.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/netdevice.h>
+#include "macb.h"
+
+/*
+ * Distribute evenly available segments across queues. The computation is
+ * complex because (1) segments are counted in powers of two and (2) a queue
+ * can only use up to 8 segments. There are four types of cases:
+ *  - Sharing all segments equally is doable. Take num_queues=4 and
+ *    num_segments=16. Each queue will get 2^2=4 segments.
+ *  - Sharing all segments is doable. Take num_queues=5 and num_segments=16.
+ *    Three queues will get 2^2=4 segments and two will get 2^1=2 segments.
+ *  - Sharing all segments is not doable because not enough queues are
+ *    available. Take num_queues=1 and num_segments=16; queue 0 can only have 8
+ *    segments.
+ *  - Sharing all segments is not doable because not enough segments are
+ *    available. Take num_queues=4 and num_segments=2.
+ *
+ * We start by computing the power each queue will have. For num_queues=5 and
+ * num_segments=16, each queue will have at least 2^1 segments. That leaves us
+ * with remaining_segments=6. If we increase the power for a queue, we get a
+ * delta of 2 (2^2-2^1). The first three queues will therefore be advantaged
+ * and each have 2^2 segments. The remaining 2 queues will only have 2^1
+ * segments.
+ */
+u64 gem_sram_distribute_segments(unsigned int num_queues,
+				 unsigned int num_segments)
+{
+	unsigned int pow, remaining_segments, i;
+	unsigned int num_advantaged_queues = 0;
+	u64 val = 0;
+
+	/* pow=0 for all queues. ilog2(0) is dangerous. */
+	if (num_queues >= num_segments)
+		return 0;
+
+	pow = min(ilog2(num_segments / num_queues), 3);
+	remaining_segments = num_segments - num_queues * (1U << pow);
+
+	/*
+	 * We can only distribute remaining segments if (1) there are remaining
+	 * segments and (2) we did not reach the max segments per queue (2^3).
+	 */
+	if (remaining_segments != 0 && pow != 3) {
+		unsigned int delta = (1U << (pow + 1)) - (1U << pow);
+
+		num_advantaged_queues = remaining_segments / delta;
+	}
+
+	for (i = 0; i < num_advantaged_queues; i++)
+		val |= ((pow + 1) & 0b11) << (i * 4);
+	for (i = num_advantaged_queues; i < num_queues; i++)
+		val |= (pow & 0b11) << (i * 4);
+
+	return val;
+}

-- 
2.53.0

Re: [PATCH net-next v2 1/2] net: macb: implement ethtool_ops.get|set_channels()

[Jakub Kicinski]

On Wed, 11 Mar 2026 17:41:53 +0100 Théo Lebrun wrote:
> +	struct macb *bp = netdev_priv(netdev);
> +	unsigned int old_count = bp->num_queues;
> +	unsigned int count = ch->combined_count;
> +	int ret = 0;

unnecessary init

> +	/*
> +	 * MACB_CAPS_QUEUE_DISABLE means that the field QUEUE_DISABLE/BIT0 in
> +	 * the per-queue RBQP register disables queue Rx. If we don't have that
> +	 * capability we can have multiple queues but we must always run with
> +	 * all enabled.
> +	 */
> +	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE))
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * An ideal .set_channels() implementation uses upfront allocated
> +	 * resources and swaps them in, bringing reliability under memory
> +	 * pressure. However, here we implement it for memory savings in
> +	 * setups with less than max number of queues active.
> +	 *
> +	 * Signal it by refusing .set_channels() once interface is opened.
> +	 */
> +	if (netif_running(bp->dev))
> +		return -EBUSY;
> +
> +	if (count == old_count)
> +		return 0;

should we reorder this with the running() check?

Re: [PATCH net-next v2 2/2] net: macb: distribute evenly Tx SRAM segments

[Jakub Kicinski]

On Wed, 11 Mar 2026 17:41:54 +0100 Théo Lebrun wrote:
> GEM has registers to configure the Tx SRAM segments distribution across
> queues. The reset value is apprioriate (even spread) but we need to
> care if/when number of active queues is modified (or if we inherited
> unevenly initialised hardware from bootloader).
> 
> To distribute segments, we take as input the number of queues
> (bp->num_queues) and the number of segments (found inside DCFG6).
> Its output is a number of segments for each queue, formatted as
> powers-of-two (eg 2 for queue 0 means it has 2^2=4 segments)

allmodconfig builds are not happy with whatever is going on here:

ld.lld: error: undefined symbol: gem_sram_distribute_segments
>>> referenced by macb_kunit.c:13 (drivers/net/ethernet/cadence/macb_kunit.c:13)
>>>               vmlinux.o:(macb_sram_segments_test)
>>> referenced by macb_kunit.c:51 (drivers/net/ethernet/cadence/macb_kunit.c:51)
>>>               vmlinux.o:(macb_sram_segments_gen_params)

ld.lld: error: undefined symbol: kunit_binary_assert_format
>>> referenced by macb_kunit.c:29 (drivers/net/ethernet/cadence/macb_kunit.c:29)
>>>               vmlinux.o:(macb_sram_segments_test)
>>> referenced by macb_kunit.c:34 (drivers/net/ethernet/cadence/macb_kunit.c:34)
>>>               vmlinux.o:(macb_sram_segments_test)

ld.lld: error: undefined symbol: __kunit_do_failed_assertion
>>> referenced by macb_kunit.c:29 (drivers/net/ethernet/cadence/macb_kunit.c:29)
>>>               vmlinux.o:(macb_sram_segments_test)
>>> referenced by macb_kunit.c:34 (drivers/net/ethernet/cadence/macb_kunit.c:34)
>>>               vmlinux.o:(macb_sram_segments_test)

ld.lld: error: undefined symbol: __kunit_abort
>>> referenced by macb_kunit.c:29 (drivers/net/ethernet/cadence/macb_kunit.c:29)
>>>               vmlinux.o:(macb_sram_segments_test)
>>> referenced by macb_kunit.c:34 (drivers/net/ethernet/cadence/macb_kunit.c:34)
>>>               vmlinux.o:(macb_sram_segments_test)

Re: [PATCH net-next v2 1/2] net: macb: implement ethtool_ops.get|set_channels()

[Théo Lebrun]

Hello Jakub,

On Fri Mar 13, 2026 at 2:26 AM CET, Jakub Kicinski wrote:
> On Wed, 11 Mar 2026 17:41:53 +0100 Théo Lebrun wrote:
>> +	struct macb *bp = netdev_priv(netdev);
>> +	unsigned int old_count = bp->num_queues;
>> +	unsigned int count = ch->combined_count;
>> +	int ret = 0;
>
> unnecessary init

ack

>> +	/*
>> +	 * MACB_CAPS_QUEUE_DISABLE means that the field QUEUE_DISABLE/BIT0 in
>> +	 * the per-queue RBQP register disables queue Rx. If we don't have that
>> +	 * capability we can have multiple queues but we must always run with
>> +	 * all enabled.
>> +	 */
>> +	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE))
>> +		return -EOPNOTSUPP;
>> +
>> +	/*
>> +	 * An ideal .set_channels() implementation uses upfront allocated
>> +	 * resources and swaps them in, bringing reliability under memory
>> +	 * pressure. However, here we implement it for memory savings in
>> +	 * setups with less than max number of queues active.
>> +	 *
>> +	 * Signal it by refusing .set_channels() once interface is opened.
>> +	 */
>> +	if (netif_running(bp->dev))
>> +		return -EBUSY;
>> +
>> +	if (count == old_count)
>> +		return 0;
>
> should we reorder this with the running() check?

I don't agree. For example when an operation is not supported, we start
by checking that and returning EOPNOTSUPP. Then we validate the input
data. Then we act.

Here it is the same. When netif_running(), we never reply to any
request even if it happens to be a no-op.

I'll go ahead and send V3. Seeing how this was only a question I'll make
the guess you don't care much about it and are fine either way.
Same for me.

Thanks!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 1/2] net: macb: implement ethtool_ops.get|set_channels()

[Jakub Kicinski]

On Fri, 13 Mar 2026 16:14:23 +0100 Théo Lebrun wrote:
> > should we reorder this with the running() check?  
> 
> I don't agree. For example when an operation is not supported, we start
> by checking that and returning EOPNOTSUPP. Then we validate the input
> data. Then we act.
> 
> Here it is the same. When netif_running(), we never reply to any
> request even if it happens to be a no-op.
> 
> I'll go ahead and send V3. Seeing how this was only a question I'll make
> the guess you don't care much about it and are fine either way.
> Same for me.

Sorry for the delay. This code can only be reached from the IOCTL path.
The Netlink path will check that params haven't changed in
ethnl_set_channels() (look at the @mod variable) and return 0 directly.
So you're basically adding a discrepancy between ioctl and Netlink.
Not a huge deal but I don't envy any user having to debug this..

Re: [PATCH net-next v2 1/2] net: macb: implement ethtool_ops.get|set_channels()

[Théo Lebrun]

On Sat Mar 14, 2026 at 3:54 PM CET, Jakub Kicinski wrote:
> On Fri, 13 Mar 2026 16:14:23 +0100 Théo Lebrun wrote:
>> > should we reorder this with the running() check?  
>> 
>> I don't agree. For example when an operation is not supported, we start
>> by checking that and returning EOPNOTSUPP. Then we validate the input
>> data. Then we act.
>> 
>> Here it is the same. When netif_running(), we never reply to any
>> request even if it happens to be a no-op.
>> 
>> I'll go ahead and send V3. Seeing how this was only a question I'll make
>> the guess you don't care much about it and are fine either way.
>> Same for me.
>
> Sorry for the delay. This code can only be reached from the IOCTL path.
> The Netlink path will check that params haven't changed in
> ethnl_set_channels() (look at the @mod variable) and return 0 directly.
> So you're basically adding a discrepancy between ioctl and Netlink.
> Not a huge deal but I don't envy any user having to debug this..

Actually, the IOCTL path also does the check (see below). So the
`count == old_count` check shall be dropped from the driver
.set_channels() callback because it is redundant. Agreed?

Extract below for the ioctl codepath:

static int ethtool_set_channels(struct net_device *dev,
				void __user *useraddr)
{
	struct ethtool_channels channels, curr = { .cmd = ETHTOOL_GCHANNELS };

	// ...

	if (copy_from_user(&channels, useraddr, sizeof(channels)))
		return -EFAULT;

	dev->ethtool_ops->get_channels(dev, &curr);

	if (channels.rx_count == curr.rx_count &&
	    channels.tx_count == curr.tx_count &&
	    channels.combined_count == curr.combined_count &&
	    channels.other_count == curr.other_count)
		return 0;

	// ...
}

https://elixir.bootlin.com/linux/v6.19.8/source/net/ethtool/ioctl.c#L2263-L2267

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 1/2] net: macb: implement ethtool_ops.get|set_channels()

[Jakub Kicinski]

On Mon, 16 Mar 2026 17:24:28 +0100 Théo Lebrun wrote:
> > Sorry for the delay. This code can only be reached from the IOCTL path.
> > The Netlink path will check that params haven't changed in
> > ethnl_set_channels() (look at the @mod variable) and return 0 directly.
> > So you're basically adding a discrepancy between ioctl and Netlink.
> > Not a huge deal but I don't envy any user having to debug this..  
> 
> Actually, the IOCTL path also does the check (see below). So the
> `count == old_count` check shall be dropped from the driver
> .set_channels() callback because it is redundant. Agreed?

Oh, even better! Some of the ioctl paths don't validate the equivalence
of the config but clearly I don't remember which.