| Message ID | 20230804134939.3109763-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c35e927cbe09d38b2d72183bb215901183927c68 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: omit ndo_hwtstamp_get() call when possible in dev_set_hwtstamp_phylib() | expand |
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Fri, 4 Aug 2023 16:49:39 +0300 you wrote: > Setting dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS is only legal > for drivers which were converted to ndo_hwtstamp_get() and > ndo_hwtstamp_set(), and it is only there that we call ndo_hwtstamp_set() > for a request that otherwise goes to phylib (for stuff like packet traps, > which need to be undone if phylib failed, hence the old_cfg logic). > > The problem is that we end up calling ndo_hwtstamp_get() when we don't > need to (even if the SIOCSHWTSTAMP wasn't intended for phylib, or if it > was, but the driver didn't set IFF_SEE_ALL_HWTSTAMP_REQUESTS). For those > unnecessary conditions, we share a code path with virtual drivers (vlan, > macvlan, bonding) where ndo_hwtstamp_get() is implemented as > generic_hwtstamp_get_lower(), and may be resolved through > generic_hwtstamp_ioctl_lower() if the lower device is unconverted. > > [...] Here is the summary with links: - [net-next] net: omit ndo_hwtstamp_get() call when possible in dev_set_hwtstamp_phylib() https://git.kernel.org/netdev/net-next/c/c35e927cbe09 You are awesome, thank you!
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 72e077022348..b46aedc36939 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -334,20 +334,23 @@ static int dev_set_hwtstamp_phylib(struct net_device *dev, cfg->source = phy_ts ? HWTSTAMP_SOURCE_PHYLIB : HWTSTAMP_SOURCE_NETDEV; - if (!phy_ts || (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) { + if (phy_ts && (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) { err = ops->ndo_hwtstamp_get(dev, &old_cfg); if (err) return err; + } + if (!phy_ts || (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) { err = ops->ndo_hwtstamp_set(dev, cfg, extack); if (err) { if (extack->_msg) netdev_err(dev, "%s\n", extack->_msg); return err; } + } + if (phy_ts && (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) changed = kernel_hwtstamp_config_changed(&old_cfg, cfg); - } if (phy_ts) { err = phy_hwtstamp_set(dev->phydev, cfg, extack);
Setting dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS is only legal for drivers which were converted to ndo_hwtstamp_get() and ndo_hwtstamp_set(), and it is only there that we call ndo_hwtstamp_set() for a request that otherwise goes to phylib (for stuff like packet traps, which need to be undone if phylib failed, hence the old_cfg logic). The problem is that we end up calling ndo_hwtstamp_get() when we don't need to (even if the SIOCSHWTSTAMP wasn't intended for phylib, or if it was, but the driver didn't set IFF_SEE_ALL_HWTSTAMP_REQUESTS). For those unnecessary conditions, we share a code path with virtual drivers (vlan, macvlan, bonding) where ndo_hwtstamp_get() is implemented as generic_hwtstamp_get_lower(), and may be resolved through generic_hwtstamp_ioctl_lower() if the lower device is unconverted. I.e. this situation: $ ip link add link eno0 name eno0.100 type vlan id 100 $ hwstamp_ctl -i eno0.100 -t 1 We are unprepared to deal with this, because if ndo_hwtstamp_get() is resolved through a legacy ndo_eth_ioctl(SIOCGHWTSTAMP) lower_dev implementation, that needs a non-NULL old_cfg.ifr pointer, and we don't have it. But we don't even need to deal with it either. In the general case, drivers may not even implement SIOCGHWTSTAMP handling, only SIOCSHWTSTAMP, so it makes sense to completely avoid a SIOCGHWTSTAMP call if we can. The solution is to split the single "if" condition into 3 smaller ones, thus separating the decision to call ndo_hwtstamp_get() from the decision to call ndo_hwtstamp_set(). The third "if" condition is identical to the first one, and both are subsets of the second one. Thus, the "cfg" argument of kernel_hwtstamp_config_changed() is always valid. Reported-by: Eric Dumazet <edumazet@google.com> Closes: https://lore.kernel.org/netdev/CANn89iLOspJsvjPj+y8jikg7erXDomWe8sqHMdfL_2LQSFrPAg@mail.gmail.com/ Fixes: fd770e856e22 ("net: remove phy_has_hwtstamp() -> phy_mii_ioctl() decision from converted drivers") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/core/dev_ioctl.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)