| Message ID | 20240829-stmmac-no-ethtool-begin-v2-1-a11b497a7074@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 55ddb6c5a3aef8d8658fe31b1ddda007693ae797 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: stmmac: drop the ethtool begin() callback | expand |
From: Andrew Halaney <ahalaney@redhat.com> Date: Thu, 29 Aug 2024 15:48:44 -0500 > This callback doesn't seem to serve much purpose, and prevents things > like: > > - systemd.link files from disabling autonegotiation > - carrier detection in NetworkManager > - any ethtool setting > > prior to userspace bringing the link up. > > The only fear I can think of is accessing unclocked resources due to > pm_runtime, but ethtool ioctls handle that as of commit > f32a21376573 ("ethtool: runtime-resume netdev parent before ethtool ioctl ops") > > Reviewed-by: Dmitry Dolenko <d.dolenko@metrotek.ru> > Tested-by: Dmitry Dolenko <d.dolenko@metrotek.ru> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Thanks, Olek
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Thu, 29 Aug 2024 15:48:44 -0500 you wrote: > This callback doesn't seem to serve much purpose, and prevents things > like: > > - systemd.link files from disabling autonegotiation > - carrier detection in NetworkManager > - any ethtool setting > > [...] Here is the summary with links: - [net-next,v2] net: stmmac: drop the ethtool begin() callback https://git.kernel.org/netdev/net-next/c/55ddb6c5a3ae You are awesome, thank you!
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index 7008219fd88d..220c582904f4 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -438,13 +438,6 @@ static void stmmac_ethtool_setmsglevel(struct net_device *dev, u32 level) } -static int stmmac_check_if_running(struct net_device *dev) -{ - if (!netif_running(dev)) - return -EBUSY; - return 0; -} - static int stmmac_ethtool_get_regs_len(struct net_device *dev) { struct stmmac_priv *priv = netdev_priv(dev); @@ -1273,7 +1266,6 @@ static int stmmac_set_tunable(struct net_device *dev, static const struct ethtool_ops stmmac_ethtool_ops = { .supported_coalesce_params = ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_MAX_FRAMES, - .begin = stmmac_check_if_running, .get_drvinfo = stmmac_ethtool_getdrvinfo, .get_msglevel = stmmac_ethtool_getmsglevel, .set_msglevel = stmmac_ethtool_setmsglevel,
This callback doesn't seem to serve much purpose, and prevents things like: - systemd.link files from disabling autonegotiation - carrier detection in NetworkManager - any ethtool setting prior to userspace bringing the link up. The only fear I can think of is accessing unclocked resources due to pm_runtime, but ethtool ioctls handle that as of commit f32a21376573 ("ethtool: runtime-resume netdev parent before ethtool ioctl ops") Reviewed-by: Dmitry Dolenko <d.dolenko@metrotek.ru> Tested-by: Dmitry Dolenko <d.dolenko@metrotek.ru> Signed-off-by: Andrew Halaney <ahalaney@redhat.com> --- Changes in v2: - Rebase on next-20240829 - Drop RFC/RFT tags, add Dmitry's Review/Test tag - Link to v1: https://lore.kernel.org/r/20240429-stmmac-no-ethtool-begin-v1-1-04c629c1c142@redhat.com I'd still like a few more folks to test this to feel more confident that I'm not breaking anyone, but at least I've gotten one on list feedback and one off list that its fine for them. --- drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 8 -------- 1 file changed, 8 deletions(-) --- base-commit: b18bbfc14a38b5234e09c2adcf713e38063a7e6e change-id: 20240424-stmmac-no-ethtool-begin-f306f2f1f2f4 Best regards,