Message-ID: <CANn89iLOspJsvjPj+y8jikg7erXDomWe8sqHMdfL_2LQSFrPAg@mail.gmail.com>
Date:   Fri, 4 Aug 2023 09:21:28 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Maxim Georgiev <glipus@...il.com>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Köry Maincent <kory.maincent@...tlin.com>,
        Maxime Chevallier <maxime.chevallier@...tlin.com>,
        Richard Cochran <richardcochran@...il.com>,
        Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
        Gerhard Engleder <gerhard@...leder-embedded.com>,
        Hangbin Liu <liuhangbin@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jacob Keller <jacob.e.keller@...el.com>,
        Jay Vosburgh <j.vosburgh@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        Wei Fang <wei.fang@....com>,
        Shenwei Wang <shenwei.wang@....com>,
        Clark Wang <xiaoning.wang@....com>,
        NXP Linux Team <linux-imx@....com>,
        UNGLinuxDriver@...rochip.com,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        Daniel Machon <daniel.machon@...rochip.com>,
        Simon Horman <simon.horman@...igine.com>,
        Casper Andersson <casper.casan@...il.com>,
        Sergey Organov <sorganov@...il.com>,
        Michal Kubecek <mkubecek@...e.cz>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
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@....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@....com>
> Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> Tested-by: Horatiu Vultur <horatiu.vultur@...rochip.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
>

Powered by blists - more mailing lists