| Message ID | 20190918195454.2056139-1-arnd@arndb.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | stmmac: selftest: avoid large stack usage | expand |
From: Arnd Bergmann <arnd@arndb.de> Date: Sep/18/2019, 20:54:34 (UTC+00:00) > + if (!cfg || !cfg->enable) { > value &= ~XGMAC_RSSE; > writel(value, ioaddr + XGMAC_RSS_CTRL); > return 0; > } > > for (i = 0; i < (sizeof(cfg->key) / sizeof(u32)); i++) { > - ret = dwxgmac2_rss_write_reg(ioaddr, true, i, *key++); > + if (cfg) > + ret = dwxgmac2_rss_write_reg(ioaddr, true, i, cfg->key[i]); > + else > + ret = dwxgmac2_rss_write_reg(ioaddr, true, i, 0); > + > if (ret) > return ret; > } > > for (i = 0; i < ARRAY_SIZE(cfg->table); i++) { > - ret = dwxgmac2_rss_write_reg(ioaddr, false, i, cfg->table[i]); > + if (cfg) > + ret = dwxgmac2_rss_write_reg(ioaddr, false, i, cfg->table[i]); > + else > + ret = dwxgmac2_rss_write_reg(ioaddr, false, i, 0); > + I don't get these "if (cfg)" checks. You already check earlier if cfg is NULL and return if so. I don't think you need these extra checks. Also, your subject line should be something like: "net: stmmac: selftests: ..." --- Thanks, Jose Miguel Abreu
On Thu, Sep 19, 2019 at 9:58 AM Jose Abreu <Jose.Abreu@synopsys.com> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > Date: Sep/18/2019, 20:54:34 (UTC+00:00) > > > + if (!cfg || !cfg->enable) { > > value &= ~XGMAC_RSSE; > > writel(value, ioaddr + XGMAC_RSS_CTRL); > > return 0; > > } > > > > for (i = 0; i < (sizeof(cfg->key) / sizeof(u32)); i++) { > > - ret = dwxgmac2_rss_write_reg(ioaddr, true, i, *key++); > > + if (cfg) > > + ret = dwxgmac2_rss_write_reg(ioaddr, true, i, cfg->key[i]); > > + else > > + ret = dwxgmac2_rss_write_reg(ioaddr, true, i, 0); > > + > > if (ret) > > return ret; > > } > > > > for (i = 0; i < ARRAY_SIZE(cfg->table); i++) { > > - ret = dwxgmac2_rss_write_reg(ioaddr, false, i, cfg->table[i]); > > + if (cfg) > > + ret = dwxgmac2_rss_write_reg(ioaddr, false, i, cfg->table[i]); > > + else > > + ret = dwxgmac2_rss_write_reg(ioaddr, false, i, 0); > > + > > I don't get these "if (cfg)" checks. You already check earlier if cfg is > NULL and return if so. I don't think you need these extra checks. Ah, you are right, I missed the 'return 0', that makes it much easier. > Also, your subject line should be something like: "net: stmmac: > selftests: ..." I think both styles is common for network drivers, though I think most just leave out the 'net:'. I changed it in v2 now. Arnd
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c index d5173dd02a71..c2f648062049 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c @@ -523,25 +523,32 @@ static int dwxgmac2_rss_configure(struct mac_device_info *hw, struct stmmac_rss *cfg, u32 num_rxq) { void __iomem *ioaddr = hw->pcsr; - u32 *key = (u32 *)cfg->key; int i, ret; u32 value; value = readl(ioaddr + XGMAC_RSS_CTRL); - if (!cfg->enable) { + if (!cfg || !cfg->enable) { value &= ~XGMAC_RSSE; writel(value, ioaddr + XGMAC_RSS_CTRL); return 0; } for (i = 0; i < (sizeof(cfg->key) / sizeof(u32)); i++) { - ret = dwxgmac2_rss_write_reg(ioaddr, true, i, *key++); + if (cfg) + ret = dwxgmac2_rss_write_reg(ioaddr, true, i, cfg->key[i]); + else + ret = dwxgmac2_rss_write_reg(ioaddr, true, i, 0); + if (ret) return ret; } for (i = 0; i < ARRAY_SIZE(cfg->table); i++) { - ret = dwxgmac2_rss_write_reg(ioaddr, false, i, cfg->table[i]); + if (cfg) + ret = dwxgmac2_rss_write_reg(ioaddr, false, i, cfg->table[i]); + else + ret = dwxgmac2_rss_write_reg(ioaddr, false, i, 0); + if (ret) return ret; } diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c index c56e89e1ae56..9c8d210b2d6a 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c @@ -1233,12 +1233,9 @@ static int __stmmac_test_l3filt(struct stmmac_priv *priv, u32 dst, u32 src, return -EOPNOTSUPP; if (!priv->dma_cap.l3l4fnum) return -EOPNOTSUPP; - if (priv->rss.enable) { - struct stmmac_rss rss = { .enable = false, }; - - stmmac_rss_configure(priv, priv->hw, &rss, + if (priv->rss.enable) + stmmac_rss_configure(priv, priv->hw, NULL, priv->plat->rx_queues_to_use); - } dissector = kzalloc(sizeof(*dissector), GFP_KERNEL); if (!dissector) { @@ -1357,12 +1354,9 @@ static int __stmmac_test_l4filt(struct stmmac_priv *priv, u32 dst, u32 src, return -EOPNOTSUPP; if (!priv->dma_cap.l3l4fnum) return -EOPNOTSUPP; - if (priv->rss.enable) { - struct stmmac_rss rss = { .enable = false, }; - - stmmac_rss_configure(priv, priv->hw, &rss, + if (priv->rss.enable) + stmmac_rss_configure(priv, priv->hw, NULL, priv->plat->rx_queues_to_use); - } dissector = kzalloc(sizeof(*dissector), GFP_KERNEL); if (!dissector) {
Putting a struct stmmac_rss object on the stack is a bad idea, as it exceeds the warning limit for a stack frame on 32-bit architectures: drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:1221:12: error: stack frame size of 1208 bytes in function '__stmmac_test_l3filt' [-Werror,-Wframe-larger-than=] drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:1338:12: error: stack frame size of 1208 bytes in function '__stmmac_test_l4filt' [-Werror,-Wframe-larger-than=] As the object is the trivial empty case, change the called function to accept a NULL pointer to mean the same thing and remove the large variable in the two callers. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 15 +++++++++++---- .../ethernet/stmicro/stmmac/stmmac_selftests.c | 14 ++++---------- 2 files changed, 15 insertions(+), 14 deletions(-) -- 2.20.0