| Message ID | 20230303164248.499286-4-kory.maincent@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Up until now, there was no way to let the user select the layer at which time stamping occurs. The stack assumed that PHY time stamping is always preferred, but some MAC/PHY combinations were buggy. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
Hi Köry,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v6.2]
[also build test WARNING on next-20230303]
[cannot apply to net/master net-next/master horms-ipvs/master linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
patch link: https://lore.kernel.org/r/20230303164248.499286-4-kory.maincent%40bootlin.com
patch subject: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230304/202303040219.nmNWbGrY-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/00a0656f9b222cfeb7c1253a4a2771b1f63b5c9b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
git checkout 00a0656f9b222cfeb7c1253a4a2771b1f63b5c9b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash net/core/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303040219.nmNWbGrY-lkp@intel.com/
All warnings (new ones prefixed by >>):
net/core/net-sysfs.c: In function 'available_timestamping_providers_show':
net/core/net-sysfs.c:627:35: warning: variable 'ops' set but not used [-Wunused-but-set-variable]
627 | const struct ethtool_ops *ops;
| ^~~
net/core/net-sysfs.c: In function 'current_timestamping_provider_show':
>> net/core/net-sysfs.c:659:28: warning: variable 'phydev' set but not used [-Wunused-but-set-variable]
659 | struct phy_device *phydev;
| ^~~~~~
net/core/net-sysfs.c:657:35: warning: variable 'ops' set but not used [-Wunused-but-set-variable]
657 | const struct ethtool_ops *ops;
| ^~~
vim +/phydev +659 net/core/net-sysfs.c
90d54e1c6ed12a Richard Cochran 2023-03-03 652
90d54e1c6ed12a Richard Cochran 2023-03-03 653 static ssize_t current_timestamping_provider_show(struct device *dev,
90d54e1c6ed12a Richard Cochran 2023-03-03 654 struct device_attribute *attr,
90d54e1c6ed12a Richard Cochran 2023-03-03 655 char *buf)
90d54e1c6ed12a Richard Cochran 2023-03-03 656 {
90d54e1c6ed12a Richard Cochran 2023-03-03 657 const struct ethtool_ops *ops;
90d54e1c6ed12a Richard Cochran 2023-03-03 658 struct net_device *netdev;
90d54e1c6ed12a Richard Cochran 2023-03-03 @659 struct phy_device *phydev;
90d54e1c6ed12a Richard Cochran 2023-03-03 660 int ret;
90d54e1c6ed12a Richard Cochran 2023-03-03 661
90d54e1c6ed12a Richard Cochran 2023-03-03 662 netdev = to_net_dev(dev);
90d54e1c6ed12a Richard Cochran 2023-03-03 663 phydev = netdev->phydev;
90d54e1c6ed12a Richard Cochran 2023-03-03 664 ops = netdev->ethtool_ops;
90d54e1c6ed12a Richard Cochran 2023-03-03 665
90d54e1c6ed12a Richard Cochran 2023-03-03 666 if (!rtnl_trylock())
90d54e1c6ed12a Richard Cochran 2023-03-03 667 return restart_syscall();
90d54e1c6ed12a Richard Cochran 2023-03-03 668
00a0656f9b222c Richard Cochran 2023-03-03 669 switch (netdev->selected_timestamping_layer) {
00a0656f9b222c Richard Cochran 2023-03-03 670 case MAC_TIMESTAMPING:
90d54e1c6ed12a Richard Cochran 2023-03-03 671 ret = sprintf(buf, "%s\n", "mac");
00a0656f9b222c Richard Cochran 2023-03-03 672 break;
00a0656f9b222c Richard Cochran 2023-03-03 673 case PHY_TIMESTAMPING:
00a0656f9b222c Richard Cochran 2023-03-03 674 ret = sprintf(buf, "%s\n", "phy");
00a0656f9b222c Richard Cochran 2023-03-03 675 break;
90d54e1c6ed12a Richard Cochran 2023-03-03 676 }
90d54e1c6ed12a Richard Cochran 2023-03-03 677
90d54e1c6ed12a Richard Cochran 2023-03-03 678 rtnl_unlock();
90d54e1c6ed12a Richard Cochran 2023-03-03 679
90d54e1c6ed12a Richard Cochran 2023-03-03 680 return ret;
90d54e1c6ed12a Richard Cochran 2023-03-03 681 }
00a0656f9b222c Richard Cochran 2023-03-03 682
Köry Maincent wrote: > From: Richard Cochran <richardcochran@gmail.com> > > Make the sysfs knob writable, and add checks in the ioctl and time > stamping paths to respect the currently selected time stamping layer. > > Signed-off-by: Richard Cochran <richardcochran@gmail.com> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> > --- > > Notes: > Changes in v2: > - Move selected_timestamping_layer introduction in this patch. > - Replace strmcmp by sysfs_streq. > - Use the PHY timestamp only if available. > > .../ABI/testing/sysfs-class-net-timestamping | 5 +- > drivers/net/phy/phy_device.c | 6 +++ > include/linux/netdevice.h | 10 ++++ > net/core/dev_ioctl.c | 44 ++++++++++++++-- > net/core/net-sysfs.c | 50 +++++++++++++++++-- > net/core/timestamping.c | 6 +++ > net/ethtool/common.c | 18 +++++-- > 7 files changed, 127 insertions(+), 12 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping > index 529c3a6eb607..6dfd59740cad 100644 > --- a/Documentation/ABI/testing/sysfs-class-net-timestamping > +++ b/Documentation/ABI/testing/sysfs-class-net-timestamping > @@ -11,7 +11,10 @@ What: /sys/class/net/<iface>/current_timestamping_provider > Date: January 2022 > Contact: Richard Cochran <richardcochran@gmail.com> > Description: > - Show the current SO_TIMESTAMPING provider. > + Shows or sets the current SO_TIMESTAMPING provider. > + When changing the value, some packets in the kernel > + networking stack may still be delivered with time > + stamps from the previous provider. > The possible values are: > - "mac" The MAC provides time stamping. > - "phy" The PHY or MII device provides time stamping. > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 8cff61dbc4b5..8dff0c6493b5 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1451,6 +1451,11 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, > > phydev->phy_link_change = phy_link_change; > if (dev) { > + if (phy_has_hwtstamp(phydev)) > + dev->selected_timestamping_layer = PHY_TIMESTAMPING; > + else > + dev->selected_timestamping_layer = MAC_TIMESTAMPING; > + > phydev->attached_dev = dev; > dev->phydev = phydev; > > @@ -1762,6 +1767,7 @@ void phy_detach(struct phy_device *phydev) > > phy_suspend(phydev); > if (dev) { > + dev->selected_timestamping_layer = MAC_TIMESTAMPING; > phydev->attached_dev->phydev = NULL; > phydev->attached_dev = NULL; > } > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index ba2bd604359d..d8e9da2526f0 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1742,6 +1742,11 @@ enum netdev_ml_priv_type { > ML_PRIV_CAN, > }; > > +enum timestamping_layer { > + MAC_TIMESTAMPING, > + PHY_TIMESTAMPING, > +}; > + > /** > * struct net_device - The DEVICE structure. > * > @@ -1981,6 +1986,9 @@ enum netdev_ml_priv_type { > * > * @threaded: napi threaded mode is enabled > * > + * @selected_timestamping_layer: Tracks whether the MAC or the PHY > + * performs packet time stamping. > + * > * @net_notifier_list: List of per-net netdev notifier block > * that follow this device when it is moved > * to another network namespace. > @@ -2339,6 +2347,8 @@ struct net_device { > unsigned wol_enabled:1; > unsigned threaded:1; > > + enum timestamping_layer selected_timestamping_layer; > + > struct list_head net_notifier_list; > > #if IS_ENABLED(CONFIG_MACSEC) > diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c > index 7674bb9f3076..cc7cf2a542fb 100644 > --- a/net/core/dev_ioctl.c > +++ b/net/core/dev_ioctl.c > @@ -262,6 +262,43 @@ static int dev_eth_ioctl(struct net_device *dev, > return err; > } > > +static int dev_hwtstamp_ioctl(struct net_device *dev, > + struct ifreq *ifr, unsigned int cmd) > +{ > + const struct net_device_ops *ops = dev->netdev_ops; > + int err; > + > + err = dsa_ndo_eth_ioctl(dev, ifr, cmd); > + if (err == 0 || err != -EOPNOTSUPP) > + return err; > + > + if (!netif_device_present(dev)) > + return -ENODEV; > + > + switch (dev->selected_timestamping_layer) { > + > + case MAC_TIMESTAMPING: > + if (ops->ndo_do_ioctl == phy_do_ioctl) { > + /* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */ > + err = -EOPNOTSUPP; > + } else { > + err = ops->ndo_eth_ioctl(dev, ifr, cmd); > + } > + break; > + > + case PHY_TIMESTAMPING: > + if (phy_has_hwtstamp(dev->phydev)) { > + err = phy_mii_ioctl(dev->phydev, ifr, cmd); > + } else { > + err = -ENODEV; > + WARN_ON(1); > + } > + break; > + } > + > + return err; > +} > + > static int dev_siocbond(struct net_device *dev, > struct ifreq *ifr, unsigned int cmd) > { > @@ -397,6 +434,9 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data, > return err; > fallthrough; > > + case SIOCGHWTSTAMP: > + return dev_hwtstamp_ioctl(dev, ifr, cmd); > + > /* > * Unknown or private ioctl > */ > @@ -407,9 +447,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data, > > if (cmd == SIOCGMIIPHY || > cmd == SIOCGMIIREG || > - cmd == SIOCSMIIREG || > - cmd == SIOCSHWTSTAMP || > - cmd == SIOCGHWTSTAMP) { > + cmd == SIOCSMIIREG) { > err = dev_eth_ioctl(dev, ifr, cmd); > } else if (cmd == SIOCBONDENSLAVE || > cmd == SIOCBONDRELEASE || > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c > index 26095634fb31..66079424b100 100644 > --- a/net/core/net-sysfs.c > +++ b/net/core/net-sysfs.c > @@ -666,17 +666,59 @@ static ssize_t current_timestamping_provider_show(struct device *dev, > if (!rtnl_trylock()) > return restart_syscall(); > > - if (phy_has_tsinfo(phydev)) { > - ret = sprintf(buf, "%s\n", "phy"); > - } else { > + switch (netdev->selected_timestamping_layer) { > + case MAC_TIMESTAMPING: > ret = sprintf(buf, "%s\n", "mac"); > + break; > + case PHY_TIMESTAMPING: > + ret = sprintf(buf, "%s\n", "phy"); > + break; > } > > rtnl_unlock(); > > return ret; > } > -static DEVICE_ATTR_RO(current_timestamping_provider); > + > +static ssize_t current_timestamping_provider_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct net_device *netdev = to_net_dev(dev); > + struct net *net = dev_net(netdev); > + enum timestamping_layer flavor; > + > + if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) > + return -EPERM; > + > + if (sysfs_streq(buf, "mac")) > + flavor = MAC_TIMESTAMPING; > + else if (sysfs_streq(buf, "phy")) > + flavor = PHY_TIMESTAMPING; > + else > + return -EINVAL; Should setting netdev->selected_timestamping_layer be limited to choices that the device supports? At a higher level, this series assumes that any timestamp not through phydev is a MAC timestamp. I don't think that is necessarily true for all devices. Some may timestamp at the phy, but not expose a phydev. This is a somewhat pedantic point. I understand that the purpose of the series is to select from among two sets of APIs.
Hi Köry,
I love your patch! Yet something to improve:
[auto build test ERROR on v6.2]
[also build test ERROR on next-20230303]
[cannot apply to net/master net-next/master horms-ipvs/master linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
patch link: https://lore.kernel.org/r/20230303164248.499286-4-kory.maincent%40bootlin.com
patch subject: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.
config: csky-defconfig (https://download.01.org/0day-ci/archive/20230304/202303041027.GxlLyldN-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/00a0656f9b222cfeb7c1253a4a2771b1f63b5c9b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
git checkout 00a0656f9b222cfeb7c1253a4a2771b1f63b5c9b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=csky olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303041027.GxlLyldN-lkp@intel.com/
All errors (new ones prefixed by >>):
csky-linux-ld: net/core/dev_ioctl.o: in function `dev_ifsioc':
>> dev_ioctl.c:(.text+0x292): undefined reference to `phy_mii_ioctl'
>> csky-linux-ld: dev_ioctl.c:(.text+0x370): undefined reference to `phy_do_ioctl'
>> csky-linux-ld: dev_ioctl.c:(.text+0x374): undefined reference to `phy_mii_ioctl'
> Should setting netdev->selected_timestamping_layer be limited to > choices that the device supports? > > At a higher level, this series assumes that any timestamp not through > phydev is a MAC timestamp. I don't think that is necessarily true for > all devices. Some may timestamp at the phy, but not expose a phydev. > This is a somewhat pedantic point. I understand that the purpose of > the series is to select from among two sets of APIs. Network drivers tend to fall into one of two classes. 1) Linux drives the whole hardware, MAC, PCS, PHY, SPF cage, LEDs etc. 2) Linux drives just the MAC, and the rest is hidden away by firmware. For this to work, the MAC API should be sufficient to configure and get status information for things which are hidden away from Linux. An example of this is the ethtool .get_link_ksettings, which mostly deals with PHY settings. Those which have linux controlling the hardware call phy_ethtool_get_link_ksettings to get phylib to do the work, where as if the hardware is hidden away, calls into the firmware are made to implement the API. When we are talking about time stamping, i assume you are talking about firmware driver the lower level hardware. I can see two ways this could go: 1) The MAC driver registers two timestamping devices with the core, one for the MAC and another for the PHY. In that case, all we need is the registration API to include some sort of indicator what layer this time stamper works at. The core can then expose to user space that there are two, and mux kAPI calls to one or the other. 2) Despite the hardware having two stampers, it only exposes one to the PTP core. Firmware driven hardware already has intimate knowledge of the hardware, since it has to have firmware to drive the hardware, so it should be able to say which is the better stamper. So it just exposes that one. So i think the proposed API does work for firmware driven stampers, but we might need to extend ptp_caps to include a level indication, MAC, bump in the wire, PHY, etc. Andrew
On Fri, Mar 03, 2023 at 05:42:40PM +0100, Köry Maincent wrote: > From: Richard Cochran <richardcochran@gmail.com> > > Make the sysfs knob writable, and add checks in the ioctl and time > stamping paths to respect the currently selected time stamping layer. Although it probably works, i think the ioctl code is ugly. I think it would be better to pull the IOCTL code into the PTP object interface. Add an ioctl member to struct ptp_clock_info. The PTP core can then directly call into the PTP object. You now have a rather odd semantic that calling the .ndo_eth_ioctl means operate on the MAC PTP. If you look at net_device_ops, i don't think any of the other members have this semantic. They all look at the netdev as a whole, and ask the netdev to do something, without caring what level it operates at. So a PTP ioctl should operate on 'the' PTP of the netdev, whichever that might be, MAC or PHY. Clearly, it is a bigger change, you need to touch every MAC driver with PTP support, but at the end, you have a cleaner architecture. Andrew
On Sat, Mar 04, 2023 at 04:43:47PM +0100, Andrew Lunn wrote: > On Fri, Mar 03, 2023 at 05:42:40PM +0100, Köry Maincent wrote: > > From: Richard Cochran <richardcochran@gmail.com> > > > > Make the sysfs knob writable, and add checks in the ioctl and time > > stamping paths to respect the currently selected time stamping layer. > > Although it probably works, i think the ioctl code is ugly. > > I think it would be better to pull the IOCTL code into the PTP object > interface. Add an ioctl member to struct ptp_clock_info. The PTP core > can then directly call into the PTP object. Putting it into ptp_clock_info makes little sense to me, because this is for the PHC, which is exposed via /dev/ptp*, and that's what the various methods in that structure are for The timestamping part is via the netdev, which is a separate entity, and its that entity which is responsible for identifying which PHC it is connected to (normally by filling in the phc_index field of ethtool_ts_info.) Think of is as: netdev ---- timestamping ---- PHC since we can have: netdev1 ---- timestamping \ netdev2 ---- timestamping -*--- PHC netdev3 ---- timestamping / Since the ioctl is to do with requesting what we want the timestamping layer to be doing with packets, putting it in ptp_clock_info makes very little sense. > You now have a rather odd semantic that calling the .ndo_eth_ioctl > means operate on the MAC PTP. If you look at net_device_ops, i don't > think any of the other members have this semantic. They all look at > the netdev as a whole, and ask the netdev to do something, without > caring what level it operates at. So a PTP ioctl should operate on > 'the' PTP of the netdev, whichever that might be, MAC or PHY. Well, what we have today is: int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info) { ... if (phy_has_tsinfo(phydev)) return phy_ts_info(phydev, info); if (ops->get_ts_info) return ops->get_ts_info(dev, info); } So, one can argue that we already have this "odd" semantic, in that calling get_ts_info() means to operate on the MAC PTP implementation. Making the ioctl also do that merely brings it into line with this existing code! If we want in general for the netdev to always be called, then we need to remove the above, but then we need to go through all the networking drivers working out which need to provide a get_ts_info() and forward that to phylib. Maybe that's a good thing in the longer run though?
> The timestamping part is via the netdev, which is a separate entity, > and its that entity which is responsible for identifying which PHC it > is connected to (normally by filling in the phc_index field of > ethtool_ts_info.) > > Think of is as: > > netdev ---- timestamping ---- PHC > > since we can have: > > netdev1 ---- timestamping \ > netdev2 ---- timestamping -*--- PHC > netdev3 ---- timestamping / > > Since the ioctl is to do with requesting what we want the timestamping > layer to be doing with packets, putting it in ptp_clock_info makes > very little sense. So there does not appear to be an object to represent a time stamper? Should one be added? It looks like it needs two ops hwtstamp_set() and hwtstamp_get(). It would then be registered with the ptp core. And then the rest of what i said would apply... Andrew
On Sat, 4 Mar 2023 20:46:05 +0100 Andrew Lunn wrote: > > Since the ioctl is to do with requesting what we want the timestamping > > layer to be doing with packets, putting it in ptp_clock_info makes > > very little sense. > > So there does not appear to be an object to represent a time stamper? > > Should one be added? It looks like it needs two ops hwtstamp_set() and > hwtstamp_get(). It would then be registered with the ptp core. And > then the rest of what i said would apply... IMHO time stamper is very much part of the netdev. I attribute the lack of clarity palatially to the fact that (for reasons unknown) we still lug the request as a raw IOCTL/ifreq. Rather than converting it to an NDO/phydev op in the core.. Also can't think of a reason why modeling it as a separate object would be useful?
diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping index 529c3a6eb607..6dfd59740cad 100644 --- a/Documentation/ABI/testing/sysfs-class-net-timestamping +++ b/Documentation/ABI/testing/sysfs-class-net-timestamping @@ -11,7 +11,10 @@ What: /sys/class/net/<iface>/current_timestamping_provider Date: January 2022 Contact: Richard Cochran <richardcochran@gmail.com> Description: - Show the current SO_TIMESTAMPING provider. + Shows or sets the current SO_TIMESTAMPING provider. + When changing the value, some packets in the kernel + networking stack may still be delivered with time + stamps from the previous provider. The possible values are: - "mac" The MAC provides time stamping. - "phy" The PHY or MII device provides time stamping. diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 8cff61dbc4b5..8dff0c6493b5 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1451,6 +1451,11 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, phydev->phy_link_change = phy_link_change; if (dev) { + if (phy_has_hwtstamp(phydev)) + dev->selected_timestamping_layer = PHY_TIMESTAMPING; + else + dev->selected_timestamping_layer = MAC_TIMESTAMPING; + phydev->attached_dev = dev; dev->phydev = phydev; @@ -1762,6 +1767,7 @@ void phy_detach(struct phy_device *phydev) phy_suspend(phydev); if (dev) { + dev->selected_timestamping_layer = MAC_TIMESTAMPING; phydev->attached_dev->phydev = NULL; phydev->attached_dev = NULL; } diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index ba2bd604359d..d8e9da2526f0 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1742,6 +1742,11 @@ enum netdev_ml_priv_type { ML_PRIV_CAN, }; +enum timestamping_layer { + MAC_TIMESTAMPING, + PHY_TIMESTAMPING, +}; + /** * struct net_device - The DEVICE structure. * @@ -1981,6 +1986,9 @@ enum netdev_ml_priv_type { * * @threaded: napi threaded mode is enabled * + * @selected_timestamping_layer: Tracks whether the MAC or the PHY + * performs packet time stamping. + * * @net_notifier_list: List of per-net netdev notifier block * that follow this device when it is moved * to another network namespace. @@ -2339,6 +2347,8 @@ struct net_device { unsigned wol_enabled:1; unsigned threaded:1; + enum timestamping_layer selected_timestamping_layer; + struct list_head net_notifier_list; #if IS_ENABLED(CONFIG_MACSEC) diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 7674bb9f3076..cc7cf2a542fb 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -262,6 +262,43 @@ static int dev_eth_ioctl(struct net_device *dev, return err; } +static int dev_hwtstamp_ioctl(struct net_device *dev, + struct ifreq *ifr, unsigned int cmd) +{ + const struct net_device_ops *ops = dev->netdev_ops; + int err; + + err = dsa_ndo_eth_ioctl(dev, ifr, cmd); + if (err == 0 || err != -EOPNOTSUPP) + return err; + + if (!netif_device_present(dev)) + return -ENODEV; + + switch (dev->selected_timestamping_layer) { + + case MAC_TIMESTAMPING: + if (ops->ndo_do_ioctl == phy_do_ioctl) { + /* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */ + err = -EOPNOTSUPP; + } else { + err = ops->ndo_eth_ioctl(dev, ifr, cmd); + } + break; + + case PHY_TIMESTAMPING: + if (phy_has_hwtstamp(dev->phydev)) { + err = phy_mii_ioctl(dev->phydev, ifr, cmd); + } else { + err = -ENODEV; + WARN_ON(1); + } + break; + } + + return err; +} + static int dev_siocbond(struct net_device *dev, struct ifreq *ifr, unsigned int cmd) { @@ -397,6 +434,9 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data, return err; fallthrough; + case SIOCGHWTSTAMP: + return dev_hwtstamp_ioctl(dev, ifr, cmd); + /* * Unknown or private ioctl */ @@ -407,9 +447,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data, if (cmd == SIOCGMIIPHY || cmd == SIOCGMIIREG || - cmd == SIOCSMIIREG || - cmd == SIOCSHWTSTAMP || - cmd == SIOCGHWTSTAMP) { + cmd == SIOCSMIIREG) { err = dev_eth_ioctl(dev, ifr, cmd); } else if (cmd == SIOCBONDENSLAVE || cmd == SIOCBONDRELEASE || diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 26095634fb31..66079424b100 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -666,17 +666,59 @@ static ssize_t current_timestamping_provider_show(struct device *dev, if (!rtnl_trylock()) return restart_syscall(); - if (phy_has_tsinfo(phydev)) { - ret = sprintf(buf, "%s\n", "phy"); - } else { + switch (netdev->selected_timestamping_layer) { + case MAC_TIMESTAMPING: ret = sprintf(buf, "%s\n", "mac"); + break; + case PHY_TIMESTAMPING: + ret = sprintf(buf, "%s\n", "phy"); + break; } rtnl_unlock(); return ret; } -static DEVICE_ATTR_RO(current_timestamping_provider); + +static ssize_t current_timestamping_provider_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct net_device *netdev = to_net_dev(dev); + struct net *net = dev_net(netdev); + enum timestamping_layer flavor; + + if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) + return -EPERM; + + if (sysfs_streq(buf, "mac")) + flavor = MAC_TIMESTAMPING; + else if (sysfs_streq(buf, "phy")) + flavor = PHY_TIMESTAMPING; + else + return -EINVAL; + + if (!rtnl_trylock()) + return restart_syscall(); + + if (!dev_isalive(netdev)) + goto out; + + if (netdev->selected_timestamping_layer != flavor) { + const struct net_device_ops *ops = netdev->netdev_ops; + struct ifreq ifr = {0}; + + /* Disable time stamping in the current layer. */ + if (netif_device_present(netdev) && ops->ndo_eth_ioctl) + ops->ndo_eth_ioctl(netdev, &ifr, SIOCSHWTSTAMP); + + netdev->selected_timestamping_layer = flavor; + } +out: + rtnl_unlock(); + return len; +} +static DEVICE_ATTR_RW(current_timestamping_provider); static struct attribute *net_class_attrs[] __ro_after_init = { &dev_attr_netdev_group.attr, diff --git a/net/core/timestamping.c b/net/core/timestamping.c index 04840697fe79..31c3142787b7 100644 --- a/net/core/timestamping.c +++ b/net/core/timestamping.c @@ -28,6 +28,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb) if (!skb->sk) return; + if (skb->dev->selected_timestamping_layer != PHY_TIMESTAMPING) + return; + type = classify(skb); if (type == PTP_CLASS_NONE) return; @@ -50,6 +53,9 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb) if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts) return false; + if (skb->dev->selected_timestamping_layer != PHY_TIMESTAMPING) + return false; + if (skb_headroom(skb) < ETH_HLEN) return false; diff --git a/net/ethtool/common.c b/net/ethtool/common.c index 64a7e05cf2c2..255170c9345a 100644 --- a/net/ethtool/common.c +++ b/net/ethtool/common.c @@ -548,10 +548,20 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info) memset(info, 0, sizeof(*info)); info->cmd = ETHTOOL_GET_TS_INFO; - if (phy_has_tsinfo(phydev)) - return phy_ts_info(phydev, info); - if (ops->get_ts_info) - return ops->get_ts_info(dev, info); + switch (dev->selected_timestamping_layer) { + + case MAC_TIMESTAMPING: + if (ops->get_ts_info) + return ops->get_ts_info(dev, info); + break; + + case PHY_TIMESTAMPING: + if (phy_has_tsinfo(phydev)) { + return phy_ts_info(phydev, info); + } + WARN_ON(1); + return -ENODEV; + } info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | SOF_TIMESTAMPING_SOFTWARE;