Re: [PATCH 1/4] soc: fsl: qbman: FSL_DPAA depends on COMPILE_TEST

Vladimir Oltean
2024-07-09T13:58:49Z

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

Hi Breno,

On Mon, Jul 08, 2024 at 12:08:05PM -0700, Breno Leitao wrote:
> I thought about a patch like the following (compile tested only). What
> do you think?

To be honest, there are several things I don't really like about this
patch.

- I really struggled with applying it in the current format. Could you
please post the output of git format-patch in the future?
- You addressed dpaa_set_coalesce() but not also dpaa_fq_setup()
- You misrepresented the patch content by saying you only allocate size
for online CPUs in the commit message. But you allocate for all
possible CPUs.
- You only kfree(needs_revert) in the error (revert_values) case, but
not in the normal (return 0) case.
- The netdev coding style is to sort the lines with variable
declarations in reverse order of line length (they call this "reverse
Christmas tree"). Your patch broke that order.
- You should use kcalloc() instead of kmalloc_array() + memset()

I have prepared and tested the attached alternative patch on a board and
I am preparing to submit it myself, if you don't have any objection.

Thanks,
Vladimir

[-- Attachment #2: 0001-net-dpaa-avoid-on-stack-arrays-of-NR_CPUS-elements.patch --]
[-- Type: text/x-diff, Size: 4199 bytes --]

From 00b942829ee283baa602011a05b02d18c6988171 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon, 8 Jul 2024 11:57:33 -0700
Subject: [PATCH] net: dpaa: avoid on-stack arrays of NR_CPUS elements

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,
avoidable 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