lkml.org 
[lkml]   [2023]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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
>

\
 
 \ /
  Last update: 2023-08-04 09:22    [W:0.092 / U:0.584 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site