| Message ID | E1tkKmI-004ObG-DL@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bc9d75b0aaed795dcd6009336500fd6094e0c944 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: further cleanups | expand |
On Tue, Feb 18, 2025 at 10:24:34AM +0000, Russell King (Oracle) wrote: > priv->flow_ctrl is only accessed by stmmac_main.c, and the only place > that it is read is in stmmac_mac_flow_ctrl(). This function is only > called from stmmac_mac_link_up() which always sets priv->flow_ctrl > immediately before calling this function. > > Therefore, initialising this at probe time is ineffectual because it > will always be overwritten before it's read. As such, the "flow_ctrl" > module parameter has been useless for some time. Rather than remove > the module parameter, which would risk module load failure, change the > description to indicate that it is obsolete, and warn if it is set by > userspace. > > Moreover, storing the value in the stmmac_priv has no benefit as it's > set and then immediately read stmmac_mac_flow_ctrl(). Instead, pass it > as a parameter to this function.. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi Russell, > -----Original Message----- > From: Russell King <rmk@armlinux.org.uk> On Behalf Of Russell King (Oracle) > Sent: Tuesday, February 18, 2025 7:25 PM > To: netdev@vger.kernel.org > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>; Andrew Lunn > <andrew+netdev@lunn.ch>; Chen-Yu Tsai <wens@csie.org>; David S. Miller > <davem@davemloft.net>; Drew Fustini <drew@pdp7.com>; Emil Renner > Berthing <kernel@esmil.dk>; Eric Dumazet <edumazet@google.com>; Fabio > Estevam <festevam@gmail.com>; Fu Wei <wefu@redhat.com>; Guo Ren > <guoren@kernel.org>; imx@lists.linux.dev; Jakub Kicinski > <kuba@kernel.org>; Jan Petrous <jan.petrous@oss.nxp.com>; Jernej Skrabec > <jernej.skrabec@gmail.com>; Jerome Brunet <jbrunet@baylibre.com>; Kevin > Hilman <khilman@baylibre.com>; linux-amlogic@lists.infradead.org; > linux-arm-kernel@lists.infradead.org; linux-arm-msm@vger.kernel.org; > linux-riscv@lists.infradead.org; linux-stm32@st-md-mailman.stormreply.com; > linux-sunxi@lists.linux.dev; Martin Blumenstingl > <martin.blumenstingl@googlemail.com>; Maxime Coquelin > <mcoquelin.stm32@gmail.com>; Minda Chen > <minda.chen@starfivetech.com>; Neil Armstrong > <neil.armstrong@linaro.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□Ｄ > ＩＴ○ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; NXP S32 Linux Team > <s32@nxp.com>; Paolo Abeni <pabeni@redhat.com>; Pengutronix Kernel > Team <kernel@pengutronix.de>; Samuel Holland <samuel@sholland.org>; > Sascha Hauer <s.hauer@pengutronix.de>; Shawn Guo > <shawnguo@kernel.org>; Vinod Koul <vkoul@kernel.org> > Subject: [PATCH net-next 2/3] net: stmmac: remove useless priv->flow_ctrl > > priv->flow_ctrl is only accessed by stmmac_main.c, and the only place > that it is read is in stmmac_mac_flow_ctrl(). This function is only called from > stmmac_mac_link_up() which always sets priv->flow_ctrl immediately before > calling this function. > > Therefore, initialising this at probe time is ineffectual because it will always be > overwritten before it's read. As such, the "flow_ctrl" > module parameter has been useless for some time. Rather than remove the > module parameter, which would risk module load failure, change the > description to indicate that it is obsolete, and warn if it is set by userspace. > > Moreover, storing the value in the stmmac_priv has no benefit as it's set and > then immediately read stmmac_mac_flow_ctrl(). Instead, pass it as a > parameter to this function.. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> Best regards, Nobuhiro
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index c602ace572b2..3395188c198a 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -282,7 +282,6 @@ struct stmmac_priv { struct stmmac_channel channel[STMMAC_CH_MAX]; int speed; - unsigned int flow_ctrl; unsigned int pause_time; struct mii_bus *mii; diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index c3a13bd8c9b3..4d542f482ecb 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -88,9 +88,9 @@ MODULE_PARM_DESC(phyaddr, "Physical device address"); #define STMMAC_XDP_TX BIT(1) #define STMMAC_XDP_REDIRECT BIT(2) -static int flow_ctrl = FLOW_AUTO; +static int flow_ctrl = 0xdead; module_param(flow_ctrl, int, 0644); -MODULE_PARM_DESC(flow_ctrl, "Flow control ability [on/off]"); +MODULE_PARM_DESC(flow_ctrl, "Flow control ability [on/off] (obsolete)"); static int pause = PAUSE_TIME; module_param(pause, int, 0644); @@ -188,12 +188,11 @@ static void stmmac_verify_args(void) watchdog = TX_TIMEO; if (unlikely((buf_sz < DEFAULT_BUFSIZE) || (buf_sz > BUF_SIZE_16KiB))) buf_sz = DEFAULT_BUFSIZE; - if (unlikely(flow_ctrl > 1)) - flow_ctrl = FLOW_AUTO; - else if (likely(flow_ctrl < 0)) - flow_ctrl = FLOW_OFF; if (unlikely((pause < 0) || (pause > 0xffff))) pause = PAUSE_TIME; + + if (flow_ctrl != 0xdead) + pr_warn("stmmac: module parameter 'flow_ctrl' is obsolete - please remove from your module configuration\n"); } static void __stmmac_disable_all_queues(struct stmmac_priv *priv) @@ -858,14 +857,16 @@ static void stmmac_release_ptp(struct stmmac_priv *priv) * stmmac_mac_flow_ctrl - Configure flow control in all queues * @priv: driver private structure * @duplex: duplex passed to the next function + * @flow_ctrl: desired flow control modes * Description: It is used for configuring the flow control in all queues */ -static void stmmac_mac_flow_ctrl(struct stmmac_priv *priv, u32 duplex) +static void stmmac_mac_flow_ctrl(struct stmmac_priv *priv, u32 duplex, + unsigned int flow_ctrl) { u32 tx_cnt = priv->plat->tx_queues_to_use; - stmmac_flow_ctrl(priv, priv->hw, duplex, priv->flow_ctrl, - priv->pause_time, tx_cnt); + stmmac_flow_ctrl(priv, priv->hw, duplex, flow_ctrl, priv->pause_time, + tx_cnt); } static unsigned long stmmac_mac_get_caps(struct phylink_config *config, @@ -925,6 +926,7 @@ static void stmmac_mac_link_up(struct phylink_config *config, bool tx_pause, bool rx_pause) { struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); + unsigned int flow_ctrl; u32 old_ctrl, ctrl; if ((priv->plat->flags & STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP) && @@ -1005,15 +1007,15 @@ static void stmmac_mac_link_up(struct phylink_config *config, /* Flow Control operation */ if (rx_pause && tx_pause) - priv->flow_ctrl = FLOW_AUTO; + flow_ctrl = FLOW_AUTO; else if (rx_pause && !tx_pause) - priv->flow_ctrl = FLOW_RX; + flow_ctrl = FLOW_RX; else if (!rx_pause && tx_pause) - priv->flow_ctrl = FLOW_TX; + flow_ctrl = FLOW_TX; else - priv->flow_ctrl = FLOW_OFF; + flow_ctrl = FLOW_OFF; - stmmac_mac_flow_ctrl(priv, duplex); + stmmac_mac_flow_ctrl(priv, duplex, flow_ctrl); if (ctrl != old_ctrl) writel(ctrl, priv->ioaddr + MAC_CTRL_REG); @@ -7600,9 +7602,6 @@ int stmmac_dvr_probe(struct device *device, "%s: warning: maxmtu having invalid value (%d)\n", __func__, priv->plat->maxmtu); - if (flow_ctrl) - priv->flow_ctrl = FLOW_AUTO; /* RX/TX pause on */ - ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE; /* Setup channels NAPI */
priv->flow_ctrl is only accessed by stmmac_main.c, and the only place that it is read is in stmmac_mac_flow_ctrl(). This function is only called from stmmac_mac_link_up() which always sets priv->flow_ctrl immediately before calling this function. Therefore, initialising this at probe time is ineffectual because it will always be overwritten before it's read. As such, the "flow_ctrl" module parameter has been useless for some time. Rather than remove the module parameter, which would risk module load failure, change the description to indicate that it is obsolete, and warn if it is set by userspace. Moreover, storing the value in the stmmac_priv has no benefit as it's set and then immediately read stmmac_mac_flow_ctrl(). Instead, pass it as a parameter to this function.. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 - .../net/ethernet/stmicro/stmmac/stmmac_main.c | 33 +++++++++---------- 2 files changed, 16 insertions(+), 18 deletions(-)