| Message ID | 20230402123755.2592507-4-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4ee58e1e56800b589afe31c34547e2bc0c59f586 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Convert dsa_master_ioctl() to netdev notifier | expand |
On 4/2/2023 5:37 AM, Vladimir Oltean wrote: > DSA does not want to intercept all ioctls handled by dev_eth_ioctl(), > only SIOCSHWTSTAMP. This can be seen from commit f685e609a301 ("net: > dsa: Deny PTP on master if switch supports it"). However, the way in > which the dsa_ndo_eth_ioctl() is called would suggest otherwise. > > Split the handling of SIOCSHWTSTAMP and SIOCGHWTSTAMP ioctls into > separate case statements of dev_ifsioc(), and make each one call its own > sub-function. This also removes the dsa_ndo_eth_ioctl() call from > dev_eth_ioctl(), which from now on exclusively handles PHY ioctls. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> PS: there could be some interesting use cases with SIO(S|G)MII(REG|PHY) to be explored, but not for now.
On Sun, Apr 02, 2023 at 05:52:29AM -0700, Florian Fainelli wrote: > PS: there could be some interesting use cases with SIO(S|G)MII(REG|PHY) to > be explored, but not for now. You mean with DSA intercepting them on the master? Details?
On 4/2/2023 5:53 AM, Vladimir Oltean wrote: > On Sun, Apr 02, 2023 at 05:52:29AM -0700, Florian Fainelli wrote: >> PS: there could be some interesting use cases with SIO(S|G)MII(REG|PHY) to >> be explored, but not for now. > > You mean with DSA intercepting them on the master? Details? Humm, maybe this is an -ENOTENOUGHCOFFEE situation, if we have a fixed-link, we would not do anything of value. If we have a PHY-less configuration same thing. So it would only be a sort of configuration where the switch side has a PHY and the MAC connects to it that would be of any value.
On Sun, Apr 02, 2023 at 05:56:33AM -0700, Florian Fainelli wrote: > > > On 4/2/2023 5:53 AM, Vladimir Oltean wrote: > > On Sun, Apr 02, 2023 at 05:52:29AM -0700, Florian Fainelli wrote: > > > PS: there could be some interesting use cases with SIO(S|G)MII(REG|PHY) to > > > be explored, but not for now. > > > > You mean with DSA intercepting them on the master? Details? > > Humm, maybe this is an -ENOTENOUGHCOFFEE situation, if we have a fixed-link, > we would not do anything of value. If we have a PHY-less configuration same > thing. So it would only be a sort of configuration where the switch side has > a PHY and the MAC connects to it that would be of any value. But the last case could be handled directly through a phy_mii_ioctl() issued by the DSA master's own ndo_eth_ioctl() handler, no need for DSA to intervene, right?
On 4/2/2023 6:01 AM, Vladimir Oltean wrote: > On Sun, Apr 02, 2023 at 05:56:33AM -0700, Florian Fainelli wrote: >> >> >> On 4/2/2023 5:53 AM, Vladimir Oltean wrote: >>> On Sun, Apr 02, 2023 at 05:52:29AM -0700, Florian Fainelli wrote: >>>> PS: there could be some interesting use cases with SIO(S|G)MII(REG|PHY) to >>>> be explored, but not for now. >>> >>> You mean with DSA intercepting them on the master? Details? >> >> Humm, maybe this is an -ENOTENOUGHCOFFEE situation, if we have a fixed-link, >> we would not do anything of value. If we have a PHY-less configuration same >> thing. So it would only be a sort of configuration where the switch side has >> a PHY and the MAC connects to it that would be of any value. > > But the last case could be handled directly through a phy_mii_ioctl() > issued by the DSA master's own ndo_eth_ioctl() handler, no need for DSA > to intervene, right? Yes of course, no need for the indirection dance, thanks!
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index b299fb23fcfa..3b1402f6897c 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -246,20 +246,34 @@ static int dev_eth_ioctl(struct net_device *dev, struct ifreq *ifr, unsigned int cmd) { const struct net_device_ops *ops = dev->netdev_ops; + + if (!ops->ndo_eth_ioctl) + return -EOPNOTSUPP; + + if (!netif_device_present(dev)) + return -ENODEV; + + return ops->ndo_eth_ioctl(dev, ifr, cmd); +} + +static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr) +{ + return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP); +} + +static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) +{ int err; - err = dsa_ndo_eth_ioctl(dev, ifr, cmd); - if (err != -EOPNOTSUPP) + err = net_hwtstamp_validate(ifr); + if (err) return err; - if (ops->ndo_eth_ioctl) { - if (netif_device_present(dev)) - err = ops->ndo_eth_ioctl(dev, ifr, cmd); - else - err = -ENODEV; - } + err = dsa_ndo_eth_ioctl(dev, ifr, SIOCSHWTSTAMP); + if (err != -EOPNOTSUPP) + return err; - return err; + return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP); } static int dev_siocbond(struct net_device *dev, @@ -395,12 +409,11 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data, return dev_siocdevprivate(dev, ifr, data, cmd); case SIOCSHWTSTAMP: - err = net_hwtstamp_validate(ifr); - if (err) - return err; - fallthrough; + return dev_set_hwtstamp(dev, ifr); case SIOCGHWTSTAMP: + return dev_get_hwtstamp(dev, ifr); + case SIOCGMIIPHY: case SIOCGMIIREG: case SIOCSMIIREG:
DSA does not want to intercept all ioctls handled by dev_eth_ioctl(), only SIOCSHWTSTAMP. This can be seen from commit f685e609a301 ("net: dsa: Deny PTP on master if switch supports it"). However, the way in which the dsa_ndo_eth_ioctl() is called would suggest otherwise. Split the handling of SIOCSHWTSTAMP and SIOCGHWTSTAMP ioctls into separate case statements of dev_ifsioc(), and make each one call its own sub-function. This also removes the dsa_ndo_eth_ioctl() call from dev_eth_ioctl(), which from now on exclusively handles PHY ioctls. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/core/dev_ioctl.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-)