Re: [PATCH v9 net-next 12/12] net: remove phy_has_hwtstamp() -> phy_mii_ioctl() decision from converted drivers

[Eric Dumazet]

On Tue, Aug 1, 2023 at 4:29 PM Vladimir Oltean <vladimir.oltean@xxxxxxx> 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@xxxxxxx>
> Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
> Tested-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> ---


> +/**
> + * 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
>