Messages in this thread | | | From | Eric Dumazet <> | Date | Fri, 4 Aug 2023 09:21:28 +0200 | Subject | Re: [PATCH v9 net-next 12/12] net: remove phy_has_hwtstamp() -> phy_mii_ioctl() decision from converted drivers |
| |
On Tue, Aug 1, 2023 at 4:29 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > It is desirable that the new .ndo_hwtstamp_set() API gives more > uniformity, less overhead and future flexibility w.r.t. the PHY > timestamping behavior. > > Currently there are some drivers which allow PHY timestamping through > the procedure mentioned in Documentation/networking/timestamping.rst. > They don't do anything locally if phy_has_hwtstamp() is set, except for > lan966x which installs PTP packet traps. > > Centralize that behavior in a new dev_set_hwtstamp_phylib() code > function, which calls either phy_mii_ioctl() for the phylib PHY, > or .ndo_hwtstamp_set() of the netdev, based on a single policy > (currently simplistic: phy_has_hwtstamp()). > > Any driver converted to .ndo_hwtstamp_set() will automatically opt into > the centralized phylib timestamping policy. Unconverted drivers still > get to choose whether they let the PHY handle timestamping or not. > > Netdev drivers with integrated PHY drivers that don't use phylib > presumably don't set dev->phydev, and those will always see > HWTSTAMP_SOURCE_NETDEV requests even when converted. The timestamping > policy will remain 100% up to them. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Tested-by: Horatiu Vultur <horatiu.vultur@microchip.com> > ---
> +/** > + * dev_set_hwtstamp_phylib() - Change hardware timestamping of NIC > + * or of attached phylib PHY > + * @dev: Network device > + * @cfg: Timestamping configuration structure > + * @extack: Netlink extended ack message structure, for error reporting > + * > + * Helper for enforcing a common policy that phylib timestamping, if available, > + * should take precedence in front of hardware timestamping provided by the > + * netdev. If the netdev driver needs to perform specific actions even for PHY > + * timestamping to work properly (a switch port must trap the timestamped > + * frames and not forward them), it must set IFF_SEE_ALL_HWTSTAMP_REQUESTS in > + * dev->priv_flags. > + */ > +static int dev_set_hwtstamp_phylib(struct net_device *dev, > + struct kernel_hwtstamp_config *cfg, > + struct netlink_ext_ack *extack) > +{ > + const struct net_device_ops *ops = dev->netdev_ops; > + bool phy_ts = phy_has_hwtstamp(dev->phydev); > + struct kernel_hwtstamp_config old_cfg = {}; > + bool changed = false; > + int err; > + > + cfg->source = phy_ts ? HWTSTAMP_SOURCE_PHYLIB : HWTSTAMP_SOURCE_NETDEV; > + > + if (!phy_ts || (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) { > + err = ops->ndo_hwtstamp_get(dev, &old_cfg);
old_cfg.ifr is NULL at this point.
This causes a crash later in generic_hwtstamp_ioctl_lower()
ifrr.ifr_ifru = kernel_cfg->ifr->ifr_ifru;
> + if (err) > + return err; > + > + err = ops->ndo_hwtstamp_set(dev, cfg, extack); > + if (err) { > + if (extack->_msg) > + netdev_err(dev, "%s\n", extack->_msg); > + return err; > + } > + > + changed = kernel_hwtstamp_config_changed(&old_cfg, cfg); > + } > + > + if (phy_ts) { > + err = phy_hwtstamp_set(dev->phydev, cfg, extack); > + if (err) { > + if (changed) > + ops->ndo_hwtstamp_set(dev, &old_cfg, NULL); > + return err; > + } > + } > + > + return 0; > +} > + > static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) > { > const struct net_device_ops *ops = dev->netdev_ops; > @@ -314,12 +392,9 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) > if (!netif_device_present(dev)) > return -ENODEV; > > - err = ops->ndo_hwtstamp_set(dev, &kernel_cfg, &extack); > - if (err) { > - if (extack._msg) > - netdev_err(dev, "%s\n", extack._msg); > + err = dev_set_hwtstamp_phylib(dev, &kernel_cfg, &extack); > + if (err) > return err; > - } > > /* The driver may have modified the configuration, so copy the > * updated version of it back to user space > @@ -362,7 +437,7 @@ int generic_hwtstamp_get_lower(struct net_device *dev, > return -ENODEV; > > if (ops->ndo_hwtstamp_get) > - return ops->ndo_hwtstamp_get(dev, kernel_cfg); > + return dev_get_hwtstamp_phylib(dev, kernel_cfg); > > /* Legacy path: unconverted lower driver */ > return generic_hwtstamp_ioctl_lower(dev, SIOCGHWTSTAMP, kernel_cfg); > @@ -379,7 +454,7 @@ int generic_hwtstamp_set_lower(struct net_device *dev, > return -ENODEV; > > if (ops->ndo_hwtstamp_set) > - return ops->ndo_hwtstamp_set(dev, kernel_cfg, extack); > + return dev_set_hwtstamp_phylib(dev, kernel_cfg, extack); > > /* Legacy path: unconverted lower driver */ > return generic_hwtstamp_ioctl_lower(dev, SIOCSHWTSTAMP, kernel_cfg); > -- > 2.34.1 >
| |