[PATCH net-next 0/5] Eliminate CONFIG_NR_CPUS dependency in dpaa-eth and enable COMPILE_TEST in fsl_qbman

[PATCH net-next 0/5] Eliminate CONFIG_NR_CPUS dependency in dpaa-eth and enable COMPILE_TEST in fsl_qbman

[Vladimir Oltean]

Breno's previous attempt at enabling COMPILE_TEST for the fsl_qbman
driver (now included here as patch 5/5) triggered compilation warnings
for large CONFIG_NR_CPUS values:
https://lore.kernel.org/all/202406261920.l5pzM1rj-lkp@intel.com/

Patch 1/5 switches two NR_CPUS arrays in the dpaa-eth driver to dynamic
allocation to avoid that warning. There is more NR_CPUS usage in the
fsl-qbman driver, but that looks relatively harmless and I couldn't find
a good reason to change it.

I noticed, while testing, that the driver doesn't actually work properly
with high CONFIG_NR_CPUS values, and patch 2/5 addresses that.

During code analysis, I have identified two places which treat
conditions that can never happen. Patches 4/5 and 5/5 simplify the
probing code - dpaa_fq_setup() - just a little bit.

Finally we have at 5/5 the patch that triggered all of this. There is
an okay from Herbert to take it via netdev, despite it being on soc/qbman:
https://lore.kernel.org/all/Zns%2FeVVBc7pdv0yM@gondor.apana.org.au/

Breno Leitao (1):
  soc: fsl: qbman: FSL_DPAA depends on COMPILE_TEST

Vladimir Oltean (4):
  net: dpaa: avoid on-stack arrays of NR_CPUS elements
  net: dpaa: eliminate NR_CPUS dependency in egress_fqs[] and conf_fqs[]
  net: dpaa: stop ignoring TX queues past the number of CPUs
  net: dpaa: no need to make sure all CPUs receive a corresponding Tx
    queue

 .../net/ethernet/freescale/dpaa/dpaa_eth.c    | 72 +++++++++++--------
 .../net/ethernet/freescale/dpaa/dpaa_eth.h    | 20 ++++--
 .../ethernet/freescale/dpaa/dpaa_ethtool.c    | 10 ++-
 drivers/soc/fsl/qbman/Kconfig                 |  2 +-
 4 files changed, 65 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH net-next 1/5] net: dpaa: avoid on-stack arrays of NR_CPUS elements

[Vladimir Oltean]

The dpaa-eth driver is written for PowerPC and Arm SoCs which have 1-24
CPUs. It depends on CONFIG_NR_CPUS having a reasonably small value in
Kconfig. Otherwise, there are 2 functions which allocate on-stack arrays
of NR_CPUS elements, and these can quickly explode in size, leading to
warnings such as:

  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c:3280:12: warning:
  stack frame size (16664) exceeds limit (2048) in 'dpaa_eth_probe' [-Wframe-larger-than]

The problem is twofold:
- Reducing the array size to the boot-time num_possible_cpus() (rather
  than the compile-time NR_CPUS) creates a variable-length array,
  which should be avoided in the Linux kernel.
- Using NR_CPUS as an array size makes the driver blow up in stack
  consumption with generic, as opposed to hand-crafted, .config files.

A simple solution is to use dynamic allocation for num_possible_cpus()
elements (aka a small number determined at runtime).

Link: https://lore.kernel.org/all/202406261920.l5pzM1rj-lkp@intel.com/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../net/ethernet/freescale/dpaa/dpaa_eth.c    | 20 ++++++++++++++-----
 .../ethernet/freescale/dpaa/dpaa_ethtool.c    | 10 +++++++++-
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index ddeb0a5f2317..c856b556929d 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -931,14 +931,18 @@ static inline void dpaa_setup_egress(const struct dpaa_priv *priv,
 	}
 }
 
-static void dpaa_fq_setup(struct dpaa_priv *priv,
-			  const struct dpaa_fq_cbs *fq_cbs,
-			  struct fman_port *tx_port)
+static int dpaa_fq_setup(struct dpaa_priv *priv,
+			 const struct dpaa_fq_cbs *fq_cbs,
+			 struct fman_port *tx_port)
 {
 	int egress_cnt = 0, conf_cnt = 0, num_portals = 0, portal_cnt = 0, cpu;
 	const cpumask_t *affine_cpus = qman_affine_cpus();
-	u16 channels[NR_CPUS];
 	struct dpaa_fq *fq;
+	u16 *channels;
+
+	channels = kcalloc(num_possible_cpus(), sizeof(u16), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
 
 	for_each_cpu_and(cpu, affine_cpus, cpu_online_mask)
 		channels[num_portals++] = qman_affine_channel(cpu);
@@ -997,6 +1001,10 @@ static void dpaa_fq_setup(struct dpaa_priv *priv,
 				break;
 		}
 	}
+
+	kfree(channels);
+
+	return 0;
 }
 
 static inline int dpaa_tx_fq_to_id(const struct dpaa_priv *priv,
@@ -3416,7 +3424,9 @@ static int dpaa_eth_probe(struct platform_device *pdev)
 	 */
 	dpaa_eth_add_channel(priv->channel, &pdev->dev);
 
-	dpaa_fq_setup(priv, &dpaa_fq_cbs, priv->mac_dev->port[TX]);
+	err = dpaa_fq_setup(priv, &dpaa_fq_cbs, priv->mac_dev->port[TX]);
+	if (err)
+		goto free_dpaa_bps;
 
 	/* Create a congestion group for this netdev, with
 	 * dynamically-allocated CGR ID.
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
index 5bd0b36d1feb..3f8cd4a7d845 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
@@ -457,12 +457,16 @@ static int dpaa_set_coalesce(struct net_device *dev,
 			     struct netlink_ext_ack *extack)
 {
 	const cpumask_t *cpus = qman_affine_cpus();
-	bool needs_revert[NR_CPUS] = {false};
 	struct qman_portal *portal;
 	u32 period, prev_period;
 	u8 thresh, prev_thresh;
+	bool *needs_revert;
 	int cpu, res;
 
+	needs_revert = kcalloc(num_possible_cpus(), sizeof(bool), GFP_KERNEL);
+	if (!needs_revert)
+		return -ENOMEM;
+
 	period = c->rx_coalesce_usecs;
 	thresh = c->rx_max_coalesced_frames;
 
@@ -485,6 +489,8 @@ static int dpaa_set_coalesce(struct net_device *dev,
 		needs_revert[cpu] = true;
 	}
 
+	kfree(needs_revert);
+
 	return 0;
 
 revert_values:
@@ -498,6 +504,8 @@ static int dpaa_set_coalesce(struct net_device *dev,
 		qman_dqrr_set_ithresh(portal, prev_thresh);
 	}
 
+	kfree(needs_revert);
+
 	return res;
 }
 
-- 
2.34.1

[PATCH net-next 2/5] net: dpaa: eliminate NR_CPUS dependency in egress_fqs[] and conf_fqs[]

[Vladimir Oltean]

The driver uses the DPAA_TC_TXQ_NUM and DPAA_ETH_TXQ_NUM macros for TX
queue handling, and they depend on CONFIG_NR_CPUS.

In generic .config files, these can go to very large (8096 CPUs) values
for the systems that DPAA1 is integrated in (1-24 CPUs). We allocate a
lot of resources that will never be used. Those are:
- system memory
- QMan FQIDs as managed by qman_alloc_fqid_range(). This is especially
  painful since currently, when booting with CONFIG_NR_CPUS=8096, a
  LS1046A-RDB system will only manage to probe 3 of its 6 interfaces.
  The rest will run out of FQD ("/reserved-memory/qman-fqd" in the
  device tree) and fail at the qman_create_fq() stage of the probing
  process.
- netdev queues as alloc_etherdev_mq() argument. The high queue indices
  are simply hidden from the network stack after the call to
  netif_set_real_num_tx_queues().

With just a tiny bit more effort, we can replace the NR_CPUS
compile-time constant with the num_possible_cpus() run-time constant,
and dynamically allocate the egress_fqs[] and conf_fqs[] arrays.
Even on a system with a high CONFIG_NR_CPUS, num_possible_cpus() will
remain equal to the number of available cores on the SoC.

The replacement is as follows:
- DPAA_TC_TXQ_NUM -> dpaa_num_txqs_per_tc()
- DPAA_ETH_TXQ_NUM -> dpaa_max_num_txqs()

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../net/ethernet/freescale/dpaa/dpaa_eth.c    | 43 +++++++++++++------
 .../net/ethernet/freescale/dpaa/dpaa_eth.h    | 20 ++++++---
 2 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index c856b556929d..7b0317020c89 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -371,6 +371,7 @@ static int dpaa_setup_tc(struct net_device *net_dev, enum tc_setup_type type,
 			 void *type_data)
 {
 	struct dpaa_priv *priv = netdev_priv(net_dev);
+	int num_txqs_per_tc = dpaa_num_txqs_per_tc();
 	struct tc_mqprio_qopt *mqprio = type_data;
 	u8 num_tc;
 	int i;
@@ -398,12 +399,12 @@ static int dpaa_setup_tc(struct net_device *net_dev, enum tc_setup_type type,
 	netdev_set_num_tc(net_dev, num_tc);
 
 	for (i = 0; i < num_tc; i++)
-		netdev_set_tc_queue(net_dev, i, DPAA_TC_TXQ_NUM,
-				    i * DPAA_TC_TXQ_NUM);
+		netdev_set_tc_queue(net_dev, i, num_txqs_per_tc,
+				    i * num_txqs_per_tc);
 
 out:
 	priv->num_tc = num_tc ? : 1;
-	netif_set_real_num_tx_queues(net_dev, priv->num_tc * DPAA_TC_TXQ_NUM);
+	netif_set_real_num_tx_queues(net_dev, priv->num_tc * num_txqs_per_tc);
 	return 0;
 }
 
@@ -649,7 +650,7 @@ static inline void dpaa_assign_wq(struct dpaa_fq *fq, int idx)
 		fq->wq = 6;
 		break;
 	case FQ_TYPE_TX:
-		switch (idx / DPAA_TC_TXQ_NUM) {
+		switch (idx / dpaa_num_txqs_per_tc()) {
 		case 0:
 			/* Low priority (best effort) */
 			fq->wq = 6;
@@ -667,8 +668,8 @@ static inline void dpaa_assign_wq(struct dpaa_fq *fq, int idx)
 			fq->wq = 0;
 			break;
 		default:
-			WARN(1, "Too many TX FQs: more than %d!\n",
-			     DPAA_ETH_TXQ_NUM);
+			WARN(1, "Too many TX FQs: more than %zu!\n",
+			     dpaa_max_num_txqs());
 		}
 		break;
 	default:
@@ -740,7 +741,8 @@ static int dpaa_alloc_all_fqs(struct device *dev, struct list_head *list,
 
 	port_fqs->rx_pcdq = &dpaa_fq[0];
 
-	if (!dpaa_fq_alloc(dev, 0, DPAA_ETH_TXQ_NUM, list, FQ_TYPE_TX_CONF_MQ))
+	if (!dpaa_fq_alloc(dev, 0, dpaa_max_num_txqs(), list,
+			   FQ_TYPE_TX_CONF_MQ))
 		goto fq_alloc_failed;
 
 	dpaa_fq = dpaa_fq_alloc(dev, 0, 1, list, FQ_TYPE_TX_ERROR);
@@ -755,7 +757,7 @@ static int dpaa_alloc_all_fqs(struct device *dev, struct list_head *list,
 
 	port_fqs->tx_defq = &dpaa_fq[0];
 
-	if (!dpaa_fq_alloc(dev, 0, DPAA_ETH_TXQ_NUM, list, FQ_TYPE_TX))
+	if (!dpaa_fq_alloc(dev, 0, dpaa_max_num_txqs(), list, FQ_TYPE_TX))
 		goto fq_alloc_failed;
 
 	return 0;
@@ -972,7 +974,7 @@ static int dpaa_fq_setup(struct dpaa_priv *priv,
 			/* If we have more Tx queues than the number of cores,
 			 * just ignore the extra ones.
 			 */
-			if (egress_cnt < DPAA_ETH_TXQ_NUM)
+			if (egress_cnt < dpaa_max_num_txqs())
 				priv->egress_fqs[egress_cnt++] = &fq->fq_base;
 			break;
 		case FQ_TYPE_TX_CONF_MQ:
@@ -992,12 +994,12 @@ static int dpaa_fq_setup(struct dpaa_priv *priv,
 	}
 
 	 /* Make sure all CPUs receive a corresponding Tx queue. */
-	while (egress_cnt < DPAA_ETH_TXQ_NUM) {
+	while (egress_cnt < dpaa_max_num_txqs()) {
 		list_for_each_entry(fq, &priv->dpaa_fq_list, list) {
 			if (fq->fq_type != FQ_TYPE_TX)
 				continue;
 			priv->egress_fqs[egress_cnt++] = &fq->fq_base;
-			if (egress_cnt == DPAA_ETH_TXQ_NUM)
+			if (egress_cnt == dpaa_max_num_txqs())
 				break;
 		}
 	}
@@ -1012,7 +1014,7 @@ static inline int dpaa_tx_fq_to_id(const struct dpaa_priv *priv,
 {
 	int i;
 
-	for (i = 0; i < DPAA_ETH_TXQ_NUM; i++)
+	for (i = 0; i < dpaa_max_num_txqs(); i++)
 		if (priv->egress_fqs[i] == tx_fq)
 			return i;
 
@@ -3332,7 +3334,7 @@ static int dpaa_eth_probe(struct platform_device *pdev)
 	/* Allocate this early, so we can store relevant information in
 	 * the private area
 	 */
-	net_dev = alloc_etherdev_mq(sizeof(*priv), DPAA_ETH_TXQ_NUM);
+	net_dev = alloc_etherdev_mq(sizeof(*priv), dpaa_max_num_txqs());
 	if (!net_dev) {
 		dev_err(dev, "alloc_etherdev_mq() failed\n");
 		return -ENOMEM;
@@ -3347,6 +3349,18 @@ static int dpaa_eth_probe(struct platform_device *pdev)
 
 	priv->msg_enable = netif_msg_init(debug, DPAA_MSG_DEFAULT);
 
+	priv->egress_fqs = devm_kcalloc(dev, dpaa_max_num_txqs(),
+					sizeof(*priv->egress_fqs),
+					GFP_KERNEL);
+	if (!priv->egress_fqs)
+		goto free_netdev;
+
+	priv->conf_fqs = devm_kcalloc(dev, dpaa_max_num_txqs(),
+				      sizeof(*priv->conf_fqs),
+				      GFP_KERNEL);
+	if (!priv->conf_fqs)
+		goto free_netdev;
+
 	mac_dev = dpaa_mac_dev_get(pdev);
 	if (IS_ERR(mac_dev)) {
 		netdev_err(net_dev, "dpaa_mac_dev_get() failed\n");
@@ -3472,7 +3486,8 @@ static int dpaa_eth_probe(struct platform_device *pdev)
 	}
 
 	priv->num_tc = 1;
-	netif_set_real_num_tx_queues(net_dev, priv->num_tc * DPAA_TC_TXQ_NUM);
+	netif_set_real_num_tx_queues(net_dev,
+				     priv->num_tc * dpaa_num_txqs_per_tc());
 
 	/* Initialize NAPI */
 	err = dpaa_napi_add(net_dev);
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
index ac3c8ed57bbe..7ed659eb08de 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
@@ -18,10 +18,6 @@
 
 /* Number of prioritised traffic classes */
 #define DPAA_TC_NUM		4
-/* Number of Tx queues per traffic class */
-#define DPAA_TC_TXQ_NUM		NR_CPUS
-/* Total number of Tx queues */
-#define DPAA_ETH_TXQ_NUM	(DPAA_TC_NUM * DPAA_TC_TXQ_NUM)
 
 /* More detailed FQ types - used for fine-grained WQ assignments */
 enum dpaa_fq_type {
@@ -142,8 +138,8 @@ struct dpaa_priv {
 	struct mac_device *mac_dev;
 	struct device *rx_dma_dev;
 	struct device *tx_dma_dev;
-	struct qman_fq *egress_fqs[DPAA_ETH_TXQ_NUM];
-	struct qman_fq *conf_fqs[DPAA_ETH_TXQ_NUM];
+	struct qman_fq **egress_fqs;
+	struct qman_fq **conf_fqs;
 
 	u16 channel;
 	struct list_head dpaa_fq_list;
@@ -185,4 +181,16 @@ extern const struct ethtool_ops dpaa_ethtool_ops;
 /* from dpaa_eth_sysfs.c */
 void dpaa_eth_sysfs_remove(struct device *dev);
 void dpaa_eth_sysfs_init(struct device *dev);
+
+static inline size_t dpaa_num_txqs_per_tc(void)
+{
+	return num_possible_cpus();
+}
+
+/* Total number of Tx queues */
+static inline size_t dpaa_max_num_txqs(void)
+{
+	return DPAA_TC_NUM * dpaa_num_txqs_per_tc();
+}
+
 #endif	/* __DPAA_H */
-- 
2.34.1

[PATCH net-next 3/5] net: dpaa: stop ignoring TX queues past the number of CPUs

[Vladimir Oltean]

dpaa_fq_setup() iterates through the queues allocated by dpaa_alloc_all_fqs()
and saved in &priv->dpaa_fq_list.

The allocation for FQ_TYPE_TX looks as follows:

	if (!dpaa_fq_alloc(dev, 0, dpaa_max_num_txqs(), list, FQ_TYPE_TX))
		goto fq_alloc_failed;

Thus, iterating again through FQ_TYPE_TX queues in dpaa_fq_setup() and
counting them will never yield an egress_cnt larger than the allocated
size, dpaa_max_num_txqs().

The comparison serves no purpose since it is always true; remove it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 7b0317020c89..e52f4cd95f97 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -971,11 +971,7 @@ static int dpaa_fq_setup(struct dpaa_priv *priv,
 		case FQ_TYPE_TX:
 			dpaa_setup_egress(priv, fq, tx_port,
 					  &fq_cbs->egress_ern);
-			/* If we have more Tx queues than the number of cores,
-			 * just ignore the extra ones.
-			 */
-			if (egress_cnt < dpaa_max_num_txqs())
-				priv->egress_fqs[egress_cnt++] = &fq->fq_base;
+			priv->egress_fqs[egress_cnt++] = &fq->fq_base;
 			break;
 		case FQ_TYPE_TX_CONF_MQ:
 			priv->conf_fqs[conf_cnt++] = &fq->fq_base;
-- 
2.34.1

[PATCH net-next 4/5] net: dpaa: no need to make sure all CPUs receive a corresponding Tx queue

[Vladimir Oltean]

dpaa_fq_setup() iterates through the &priv->dpaa_fq_list elements
allocated by dpaa_alloc_all_fqs(). This includes a call to:

	if (!dpaa_fq_alloc(dev, 0, dpaa_max_num_txqs(), list, FQ_TYPE_TX))
		goto fq_alloc_failed;

which gives us dpaa_max_num_txqs() elements of FQ_TYPE_TX type.

The code block which we are deleting runs after an earlier iteration
through &priv->dpaa_fq_list. So at the end of this iteration (for which
there is no early break), egress_cnt will be unconditionally equal to
dpaa_max_num_txqs().

In other words, dpaa_alloc_all_fqs() has already allocated TX queues for
all possible CPUs and the maximal number of traffic classes, and we've
already iterated once through them all.

The while() condition is dead code, remove it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index e52f4cd95f97..9c0bac58a0da 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -989,17 +989,6 @@ static int dpaa_fq_setup(struct dpaa_priv *priv,
 		}
 	}
 
-	 /* Make sure all CPUs receive a corresponding Tx queue. */
-	while (egress_cnt < dpaa_max_num_txqs()) {
-		list_for_each_entry(fq, &priv->dpaa_fq_list, list) {
-			if (fq->fq_type != FQ_TYPE_TX)
-				continue;
-			priv->egress_fqs[egress_cnt++] = &fq->fq_base;
-			if (egress_cnt == dpaa_max_num_txqs())
-				break;
-		}
-	}
-
 	kfree(channels);
 
 	return 0;
-- 
2.34.1

[PATCH net-next 5/5] soc: fsl: qbman: FSL_DPAA depends on COMPILE_TEST

[Vladimir Oltean]

From: Breno Leitao <leitao@debian.org>

As most of the drivers that depend on ARCH_LAYERSCAPE, make FSL_DPAA
depend on COMPILE_TEST for compilation and testing.

	# grep -r depends.\*ARCH_LAYERSCAPE.\*COMPILE_TEST | wc -l
	29

Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/soc/fsl/qbman/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/fsl/qbman/Kconfig b/drivers/soc/fsl/qbman/Kconfig
index bdecb86bb656..27774ec6ff90 100644
--- a/drivers/soc/fsl/qbman/Kconfig
+++ b/drivers/soc/fsl/qbman/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 menuconfig FSL_DPAA
 	bool "QorIQ DPAA1 framework support"
-	depends on ((FSL_SOC_BOOKE || ARCH_LAYERSCAPE) && ARCH_DMA_ADDR_T_64BIT)
+	depends on ((FSL_SOC_BOOKE || ARCH_LAYERSCAPE || COMPILE_TEST) && ARCH_DMA_ADDR_T_64BIT)
 	select GENERIC_ALLOCATOR
 	help
 	  The Freescale Data Path Acceleration Architecture (DPAA) is a set of
-- 
2.34.1

Re: [PATCH net-next 1/5] net: dpaa: avoid on-stack arrays of NR_CPUS elements

[Breno Leitao]

Hello Vladimir,

On Thu, Jul 11, 2024 at 02:00:21AM +0300, Vladimir Oltean wrote:
> The dpaa-eth driver is written for PowerPC and Arm SoCs which have 1-24
> CPUs. It depends on CONFIG_NR_CPUS having a reasonably small value in
> Kconfig. Otherwise, there are 2 functions which allocate on-stack arrays
> of NR_CPUS elements, and these can quickly explode in size, leading to
> warnings such as:
> 
>   drivers/net/ethernet/freescale/dpaa/dpaa_eth.c:3280:12: warning:
>   stack frame size (16664) exceeds limit (2048) in 'dpaa_eth_probe' [-Wframe-larger-than]
> 
> The problem is twofold:
> - Reducing the array size to the boot-time num_possible_cpus() (rather
>   than the compile-time NR_CPUS) creates a variable-length array,
>   which should be avoided in the Linux kernel.
> - Using NR_CPUS as an array size makes the driver blow up in stack
>   consumption with generic, as opposed to hand-crafted, .config files.
> 
> A simple solution is to use dynamic allocation for num_possible_cpus()
> elements (aka a small number determined at runtime).
> 
> Link: https://lore.kernel.org/all/202406261920.l5pzM1rj-lkp@intel.com/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Breno Leitao <leitao@debian.org>

Thanks for working on it.

--breno

RE: [PATCH net-next 0/5] Eliminate CONFIG_NR_CPUS dependency in dpaa-eth and enable COMPILE_TEST in fsl_qbman

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Sent: Thursday, July 11, 2024 2:00 AM
> To: netdev@vger.kernel.org
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Breno Leitao <leitao@debian.org>; Herbert Xu
> <herbert@gondor.apana.org.au>; Madalin Bucur <madalin.bucur@nxp.com>;
> linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-arm-
> kernel@lists.infradead.org
> Subject: [PATCH net-next 0/5] Eliminate CONFIG_NR_CPUS dependency in dpaa-
> eth and enable COMPILE_TEST in fsl_qbman
> 
> Breno's previous attempt at enabling COMPILE_TEST for the fsl_qbman
> driver (now included here as patch 5/5) triggered compilation warnings
> for large CONFIG_NR_CPUS values:
> https://lore.kernel.org/all/202406261920.l5pzM1rj-lkp@intel.com/
> 
> Patch 1/5 switches two NR_CPUS arrays in the dpaa-eth driver to dynamic
> allocation to avoid that warning. There is more NR_CPUS usage in the
> fsl-qbman driver, but that looks relatively harmless and I couldn't find
> a good reason to change it.
> 
> I noticed, while testing, that the driver doesn't actually work properly
> with high CONFIG_NR_CPUS values, and patch 2/5 addresses that.
> 
> During code analysis, I have identified two places which treat
> conditions that can never happen. Patches 4/5 and 5/5 simplify the
> probing code - dpaa_fq_setup() - just a little bit.
> 
> Finally we have at 5/5 the patch that triggered all of this. There is
> an okay from Herbert to take it via netdev, despite it being on soc/qbman:
> https://lore.kernel.org/all/Zns%2FeVVBc7pdv0yM@gondor.apana.org.au/
> 
> Breno Leitao (1):
>   soc: fsl: qbman: FSL_DPAA depends on COMPILE_TEST
> 
> Vladimir Oltean (4):
>   net: dpaa: avoid on-stack arrays of NR_CPUS elements
>   net: dpaa: eliminate NR_CPUS dependency in egress_fqs[] and conf_fqs[]
>   net: dpaa: stop ignoring TX queues past the number of CPUs
>   net: dpaa: no need to make sure all CPUs receive a corresponding Tx
>     queue
> 
>  .../net/ethernet/freescale/dpaa/dpaa_eth.c    | 72 +++++++++++--------
>  .../net/ethernet/freescale/dpaa/dpaa_eth.h    | 20 ++++--
>  .../ethernet/freescale/dpaa/dpaa_ethtool.c    | 10 ++-
>  drivers/soc/fsl/qbman/Kconfig                 |  2 +-
>  4 files changed, 65 insertions(+), 39 deletions(-)
> 
> --
> 2.34.1


For the series,

Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>

Thanks,
Madalin

Re: [PATCH net-next 5/5] soc: fsl: qbman: FSL_DPAA depends on COMPILE_TEST

[Vladimir Oltean]

On Thu, Jul 11, 2024 at 02:00:25AM +0300, Vladimir Oltean wrote:
> From: Breno Leitao <leitao@debian.org>
> 
> As most of the drivers that depend on ARCH_LAYERSCAPE, make FSL_DPAA
> depend on COMPILE_TEST for compilation and testing.
> 
> 	# grep -r depends.\*ARCH_LAYERSCAPE.\*COMPILE_TEST | wc -l
> 	29
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

I don't know why nipa says there are 800+ new warnings/errors introduced
by this patch, but it looks like a false positive (or I can't seem to
find something relevant).

Re: [PATCH net-next 5/5] soc: fsl: qbman: FSL_DPAA depends on COMPILE_TEST

[Breno Leitao]

On Fri, Jul 12, 2024 at 03:14:00PM +0300, Vladimir Oltean wrote:
> On Thu, Jul 11, 2024 at 02:00:25AM +0300, Vladimir Oltean wrote:
> > From: Breno Leitao <leitao@debian.org>
> > 
> > As most of the drivers that depend on ARCH_LAYERSCAPE, make FSL_DPAA
> > depend on COMPILE_TEST for compilation and testing.
> > 
> > 	# grep -r depends.\*ARCH_LAYERSCAPE.\*COMPILE_TEST | wc -l
> > 	29
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> 
> I don't know why nipa says there are 800+ new warnings/errors introduced
> by this patch, but it looks like a false positive (or I can't seem to
> find something relevant).

Right, All of the warnings are basically a set of MODULES_DESCRIPTION()
that are missing in other parts of the kernel, and not related to this
patch series. None of them seems to be in the network stack. (for
context, I've fixed all the missing MODULES_DESCRIPTION in the past).

nipa also complained about an unused variable, and we have a patch for
it already:

https://lore.kernel.org/all/20240712134817.913756-1-leitao@debian.org/

Re: [PATCH net-next 2/5] net: dpaa: eliminate NR_CPUS dependency in egress_fqs[] and conf_fqs[]

[Jakub Kicinski]

On Thu, 11 Jul 2024 02:00:22 +0300 Vladimir Oltean wrote:
> +	priv->egress_fqs = devm_kcalloc(dev, dpaa_max_num_txqs(),
> +					sizeof(*priv->egress_fqs),
> +					GFP_KERNEL);
> +	if (!priv->egress_fqs)
> +		goto free_netdev;
> +
> +	priv->conf_fqs = devm_kcalloc(dev, dpaa_max_num_txqs(),
> +				      sizeof(*priv->conf_fqs),
> +				      GFP_KERNEL);
> +	if (!priv->conf_fqs)
> +		goto free_netdev;

Gotta set err before jumping
-- 
pw-bot: cr

Re: [PATCH net-next 2/5] net: dpaa: eliminate NR_CPUS dependency in egress_fqs[] and conf_fqs[]

[Vladimir Oltean]

On Sat, Jul 13, 2024 at 03:35:32PM -0700, Jakub Kicinski wrote:
> On Thu, 11 Jul 2024 02:00:22 +0300 Vladimir Oltean wrote:
> > +	priv->egress_fqs = devm_kcalloc(dev, dpaa_max_num_txqs(),
> > +					sizeof(*priv->egress_fqs),
> > +					GFP_KERNEL);
> > +	if (!priv->egress_fqs)
> > +		goto free_netdev;
> > +
> > +	priv->conf_fqs = devm_kcalloc(dev, dpaa_max_num_txqs(),
> > +				      sizeof(*priv->conf_fqs),
> > +				      GFP_KERNEL);
> > +	if (!priv->conf_fqs)
> > +		goto free_netdev;
> 
> Gotta set err before jumping
> -- 
> pw-bot: cr

Good point. Thanks for the review.