[net-next,v5,13/16] net: Change the API of PHY default timestamp to MAC
Commit Message
From: Kory Maincent <kory.maincent@bootlin.com>
Change the API to select MAC default time stamping instead of the PHY.
Indeed the PHY is closer to the wire therefore theoretically it have less
delay than the MAC timestamping but the reality is different. Due to lower
time stamping clock frequency, latency in the MDIO bus and no PHC hardware
synchronization between different PHY, the PHY PTP is often less precise
than the MAC. The exception is for PHY designed specially for PTP case but
these board are not very widespread. For not breaking the compatibility I
introduce an allowlist to reference all current PHYs that support
time stamping and let them keep the old API behavior.
The phy_set_timestamp function is called at each call of phy_attach_direct.
In case of MAC driver using phylink this function is called when the
interface is turned up. Then if the interface goes down and up again the
last choice of timestamp will be overwritten by the default choice.
A solution could be to cache the timestamp status but it can bring other
issues. In case of SFP, if we change the module, it doesn't make sense to
blindly re-set the timestamp back to PHY, if the new module has a PHY with
mediocre timestamping capabilities.
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
Changes in v5:
- Extract the API change in this patch.
- Rename whitelist to allowlist.
- Set NETDEV_TIMESTAMPING in register_netdevice function.
- Add software timestamping case description in ts_info.
---
drivers/net/phy/phy_device.c | 68 ++++++++++++++++++++++++++++++++++++
include/linux/netdevice.h | 5 +++
net/core/dev.c | 3 ++
net/core/dev_ioctl.c | 36 +++++++++++--------
net/core/timestamping.c | 9 +++++
net/ethtool/common.c | 16 +++++++--
6 files changed, 121 insertions(+), 16 deletions(-)
Comments
On 10/9/23 08:51, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
>
> Change the API to select MAC default time stamping instead of the PHY.
> Indeed the PHY is closer to the wire therefore theoretically it have less
> delay than the MAC timestamping but the reality is different.
s/have/has/ or you need to make the subject plural.
> Due to lower
> time stamping clock frequency, latency in the MDIO bus and no PHC hardware
> synchronization between different PHY, the PHY PTP is often less precise
> than the MAC.
-> different PHYs
> The exception is for PHY designed specially for PTP case but
> these board are not very widespread.
s/board/devices/ maybe?
> For not breaking the compatibility I
> introduce an allowlist to reference all current PHYs that support
> time stamping and let them keep the old API behavior.
>
> The phy_set_timestamp function is called at each call of phy_attach_direct.
> In case of MAC driver using phylink this function is called when the
> interface is turned up. Then if the interface goes down and up again the
> last choice of timestamp will be overwritten by the default choice.
> A solution could be to cache the timestamp status but it can bring other
> issues. In case of SFP, if we change the module, it doesn't make sense to
> blindly re-set the timestamp back to PHY, if the new module has a PHY with
> mediocre timestamping capabilities.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>
> Changes in v5:
> - Extract the API change in this patch.
> - Rename whitelist to allowlist.
> - Set NETDEV_TIMESTAMPING in register_netdevice function.
> - Add software timestamping case description in ts_info.
> ---
> drivers/net/phy/phy_device.c | 68 ++++++++++++++++++++++++++++++++++++
> include/linux/netdevice.h | 5 +++
> net/core/dev.c | 3 ++
> net/core/dev_ioctl.c | 36 +++++++++++--------
> net/core/timestamping.c | 9 +++++
> net/ethtool/common.c | 16 +++++++--
> 6 files changed, 121 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 2ce74593d6e4..2d5a6d57acb3 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1411,6 +1411,68 @@ int phy_sfp_probe(struct phy_device *phydev,
> }
> EXPORT_SYMBOL(phy_sfp_probe);
>
> +/* An allowlist for PHYs selected as default timesetamping.
> + * Its use is to keep compatibility with old PTP API which is selecting
> + * these PHYs as default timestamping.
> + * The new API is selecting the MAC as default timestamping.
> + */
> +const char * const phy_timestamping_allowlist[] = {
> + "Broadcom BCM5411",
> + "Broadcom BCM5421",
> + "Broadcom BCM54210E",
> + "Broadcom BCM5461",
> + "Broadcom BCM54612E",
> + "Broadcom BCM5464",
> + "Broadcom BCM5481",
> + "Broadcom BCM54810",
> + "Broadcom BCM54811",
> + "Broadcom BCM5482",
> + "Broadcom BCM50610",
> + "Broadcom BCM50610M",
> + "Broadcom BCM57780",
> + "Broadcom BCM5395",
> + "Broadcom BCM53125",
> + "Broadcom BCM53128",
> + "Broadcom BCM89610",
The list of Broadcom PHYs that you have is too broad, if you look at
drivers/net/phy/bcm-phy-ptp.c only PHY_ID_BCM54210E is actually
supported as of now.
The allowlist is not maintainable using the PHY device name, especially
not without having a shared header file that is used by both
phy_device.c and the individual PHY device driver. You are guaranteed
that a name change on one side can be missed on the other.
Using PHY OUIs can be a tad complicated with C45 PHYs, so rather, I
would suggest that each of those PHY device drivers be responsible for
overidding the timestamping selection, you could even consider inventing
a specific PHYLIB_TIMESTAMPING_LEGACY and then issue a warning within
the PHY library to encourage a change (if this is even relevant).
> #endif
> +
> + u32 ts_layer;
Likewise, would be preferable to use an enum type.
On Mon, Oct 09, 2023 at 05:51:35PM +0200, Köry Maincent wrote:
Hi Köry,
some minor feedback from my side.
...
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 2ce74593d6e4..2d5a6d57acb3 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1411,6 +1411,68 @@ int phy_sfp_probe(struct phy_device *phydev,
> }
> EXPORT_SYMBOL(phy_sfp_probe);
>
> +/* An allowlist for PHYs selected as default timesetamping.
> + * Its use is to keep compatibility with old PTP API which is selecting
> + * these PHYs as default timestamping.
> + * The new API is selecting the MAC as default timestamping.
> + */
> +const char * const phy_timestamping_allowlist[] = {
Should this be static?
As flagged by Sparse.
> + "Broadcom BCM5411",
> + "Broadcom BCM5421",
> + "Broadcom BCM54210E",
> + "Broadcom BCM5461",
> + "Broadcom BCM54612E",
> + "Broadcom BCM5464",
> + "Broadcom BCM5481",
> + "Broadcom BCM54810",
> + "Broadcom BCM54811",
> + "Broadcom BCM5482",
> + "Broadcom BCM50610",
> + "Broadcom BCM50610M",
> + "Broadcom BCM57780",
> + "Broadcom BCM5395",
> + "Broadcom BCM53125",
> + "Broadcom BCM53128",
> + "Broadcom BCM89610",
> + "NatSemi DP83640",
> + "Microchip LAN8841 Gigabit PHY",
> + "Microchip INDY Gigabit Quad PHY",
> + "Microsemi GE VSC856X SyncE",
> + "Microsemi GE VSC8575 SyncE",
> + "Microsemi GE VSC8582 SyncE",
> + "Microsemi GE VSC8584 SyncE",
> + "NXP C45 TJA1103",
> + NULL,
> +};
> +
> +/**
> + * phy_set_timestamp - set the default selected timestamping device
> + * @dev: Pointer to net_device
> + * @phydev: Pointer to phy_device
> + *
> + * This is used to set default timestamping device taking into account
> + * the new API choice, which is selecting the timestamping from MAC by
> + * default.
> + */
...
> @@ -1484,6 +1546,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>
> phydev->phy_link_change = phy_link_change;
> if (dev) {
> + phy_set_timestamp(dev, phydev);
> phydev->attached_dev = dev;
> dev->phydev = phydev;
>
> @@ -1794,6 +1857,7 @@ EXPORT_SYMBOL_GPL(devm_phy_package_join);
> void phy_detach(struct phy_device *phydev)
> {
> struct net_device *dev = phydev->attached_dev;
> + const struct ethtool_ops *ops = dev->ethtool_ops;
Elsewhere in this function it is assumed that dev may be NULL.
But here it is dereferenced unconditionally.
As flagged by Smatch.
> struct module *ndev_owner = NULL;
> struct mii_bus *bus;
>
> @@ -1812,6 +1876,10 @@ void phy_detach(struct phy_device *phydev)
>
> phy_suspend(phydev);
> if (dev) {
> + if (ops->get_ts_info)
> + dev->ts_layer = NETDEV_TIMESTAMPING;
> + else
> + dev->ts_layer = NO_TIMESTAMPING;
> phydev->attached_dev->phydev = NULL;
> phydev->attached_dev = NULL;
> }
...
Hi Köry,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/K-ry-Maincent/net-Convert-PHYs-hwtstamp-callback-to-use-kernel_hwtstamp_config/20231009-235451
base: net-next/main
patch link: https://lore.kernel.org/r/20231009155138.86458-14-kory.maincent%40bootlin.com
patch subject: [PATCH net-next v5 13/16] net: Change the API of PHY default timestamp to MAC
config: i386-randconfig-063-20231012 (https://download.01.org/0day-ci/archive/20231013/202310130914.kppQjKFp-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231013/202310130914.kppQjKFp-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310130914.kppQjKFp-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/phy_device.c:1419:12: sparse: sparse: symbol 'phy_timestamping_allowlist' was not declared. Should it be static?
@@ -1411,6 +1411,68 @@ int phy_sfp_probe(struct phy_device *phydev,
}
EXPORT_SYMBOL(phy_sfp_probe);
+/* An allowlist for PHYs selected as default timesetamping.
+ * Its use is to keep compatibility with old PTP API which is selecting
+ * these PHYs as default timestamping.
+ * The new API is selecting the MAC as default timestamping.
+ */
+const char * const phy_timestamping_allowlist[] = {
+ "Broadcom BCM5411",
+ "Broadcom BCM5421",
+ "Broadcom BCM54210E",
+ "Broadcom BCM5461",
+ "Broadcom BCM54612E",
+ "Broadcom BCM5464",
+ "Broadcom BCM5481",
+ "Broadcom BCM54810",
+ "Broadcom BCM54811",
+ "Broadcom BCM5482",
+ "Broadcom BCM50610",
+ "Broadcom BCM50610M",
+ "Broadcom BCM57780",
+ "Broadcom BCM5395",
+ "Broadcom BCM53125",
+ "Broadcom BCM53128",
+ "Broadcom BCM89610",
+ "NatSemi DP83640",
+ "Microchip LAN8841 Gigabit PHY",
+ "Microchip INDY Gigabit Quad PHY",
+ "Microsemi GE VSC856X SyncE",
+ "Microsemi GE VSC8575 SyncE",
+ "Microsemi GE VSC8582 SyncE",
+ "Microsemi GE VSC8584 SyncE",
+ "NXP C45 TJA1103",
+ NULL,
+};
+
+/**
+ * phy_set_timestamp - set the default selected timestamping device
+ * @dev: Pointer to net_device
+ * @phydev: Pointer to phy_device
+ *
+ * This is used to set default timestamping device taking into account
+ * the new API choice, which is selecting the timestamping from MAC by
+ * default.
+ */
+static void phy_set_timestamp(struct net_device *dev, struct phy_device *phydev)
+{
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+ int i;
+
+ /* Backward compatibility to old timestamping API */
+ for (i = 0; phy_timestamping_allowlist[i]; i++) {
+ if (!strcmp(phy_timestamping_allowlist[i],
+ phydev->drv->name)) {
+ if (phy_has_tsinfo(phydev))
+ dev->ts_layer = PHYLIB_TIMESTAMPING;
+ return;
+ }
+ }
+
+ if (phy_has_tsinfo(phydev) && !ops->get_ts_info)
+ dev->ts_layer = PHYLIB_TIMESTAMPING;
+}
+
/**
* phy_attach_direct - attach a network device to a given PHY device pointer
* @dev: network device to attach
@@ -1484,6 +1546,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
phydev->phy_link_change = phy_link_change;
if (dev) {
+ phy_set_timestamp(dev, phydev);
phydev->attached_dev = dev;
dev->phydev = phydev;
@@ -1794,6 +1857,7 @@ EXPORT_SYMBOL_GPL(devm_phy_package_join);
void phy_detach(struct phy_device *phydev)
{
struct net_device *dev = phydev->attached_dev;
+ const struct ethtool_ops *ops = dev->ethtool_ops;
struct module *ndev_owner = NULL;
struct mii_bus *bus;
@@ -1812,6 +1876,10 @@ void phy_detach(struct phy_device *phydev)
phy_suspend(phydev);
if (dev) {
+ if (ops->get_ts_info)
+ dev->ts_layer = NETDEV_TIMESTAMPING;
+ else
+ dev->ts_layer = NO_TIMESTAMPING;
phydev->attached_dev->phydev = NULL;
phydev->attached_dev = NULL;
}
@@ -47,6 +47,7 @@
#include <uapi/linux/if_bonding.h>
#include <uapi/linux/pkt_cls.h>
#include <uapi/linux/netdev.h>
+#include <uapi/linux/net_tstamp.h>
#include <linux/hashtable.h>
#include <linux/rbtree.h>
#include <net/net_trackers.h>
@@ -2054,6 +2055,8 @@ enum netdev_ml_priv_type {
*
* @dpll_pin: Pointer to the SyncE source pin of a DPLL subsystem,
* where the clock is recovered.
+ * @ts_layer: Tracks which network device
+ * performs packet time stamping.
*
* FIXME: cleanup struct net_device such that network protocol info
* moves out.
@@ -2415,6 +2418,8 @@ struct net_device {
#if IS_ENABLED(CONFIG_DPLL)
struct dpll_pin *dpll_pin;
#endif
+
+ u32 ts_layer;
};
#define to_net_dev(d) container_of(d, struct net_device, dev)
@@ -10204,6 +10204,9 @@ int register_netdevice(struct net_device *dev)
dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
rtmsg_ifinfo(RTM_NEWLINK, dev, ~0U, GFP_KERNEL, 0, NULL);
+ if (dev->ethtool_ops->get_ts_info)
+ dev->ts_layer = NETDEV_TIMESTAMPING;
+
out:
return ret;
@@ -259,9 +259,7 @@ static int dev_eth_ioctl(struct net_device *dev,
* @dev: Network device
* @cfg: Timestamping configuration structure
*
- * Helper for enforcing a common policy that phylib timestamping, if available,
- * should take precedence in front of hardware timestamping provided by the
- * netdev.
+ * Helper for calling the selected hardware provider timestamping.
*
* Note: phy_mii_ioctl() only handles SIOCSHWTSTAMP (not SIOCGHWTSTAMP), and
* there only exists a phydev->mii_ts->hwtstamp() method. So this will return
@@ -271,10 +269,14 @@ static int dev_eth_ioctl(struct net_device *dev,
static int dev_get_hwtstamp_phylib(struct net_device *dev,
struct kernel_hwtstamp_config *cfg)
{
- if (phy_has_hwtstamp(dev->phydev))
+ u32 ts_layer = dev->ts_layer;
+
+ if (ts_layer & PHYLIB_TIMESTAMPING)
return phy_hwtstamp_get(dev->phydev, cfg);
+ else if (ts_layer & NETDEV_TIMESTAMPING)
+ return dev->netdev_ops->ndo_hwtstamp_get(dev, cfg);
- return dev->netdev_ops->ndo_hwtstamp_get(dev, cfg);
+ return -EOPNOTSUPP;
}
static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
@@ -315,9 +317,8 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
* @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
+ * Helper for calling the selected hardware provider timestamping.
+ * 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.
@@ -327,20 +328,26 @@ int dev_set_hwtstamp_phylib(struct net_device *dev,
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 = {};
+ u32 ts_layer = dev->ts_layer;
bool changed = false;
int err;
- cfg->source = phy_ts ? PHYLIB_TIMESTAMPING : NETDEV_TIMESTAMPING;
+ cfg->source = ts_layer;
+
+ if (!(ts_layer & PHYLIB_TIMESTAMPING) &&
+ !(ts_layer & NETDEV_TIMESTAMPING))
+ return -EOPNOTSUPP;
- if (phy_ts && (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) {
+ if ((ts_layer & PHYLIB_TIMESTAMPING) &&
+ (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) {
err = ops->ndo_hwtstamp_get(dev, &old_cfg);
if (err)
return err;
}
- if (!phy_ts || (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) {
+ if ((ts_layer & NETDEV_TIMESTAMPING) ||
+ (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) {
err = ops->ndo_hwtstamp_set(dev, cfg, extack);
if (err) {
if (extack->_msg)
@@ -349,10 +356,11 @@ int dev_set_hwtstamp_phylib(struct net_device *dev,
}
}
- if (phy_ts && (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS))
+ if ((ts_layer & PHYLIB_TIMESTAMPING) &&
+ (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS))
changed = kernel_hwtstamp_config_changed(&old_cfg, cfg);
- if (phy_ts) {
+ if (ts_layer & PHYLIB_TIMESTAMPING) {
err = phy_hwtstamp_set(dev->phydev, cfg, extack);
if (err) {
if (changed)
@@ -21,6 +21,7 @@ static unsigned int classify(const struct sk_buff *skb)
void skb_clone_tx_timestamp(struct sk_buff *skb)
{
+ u32 ts_layer = skb->dev->ts_layer;
struct mii_timestamper *mii_ts;
struct sk_buff *clone;
unsigned int type;
@@ -28,6 +29,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
if (!skb->sk)
return;
+ if (!(ts_layer & PHYLIB_TIMESTAMPING))
+ return;
+
type = classify(skb);
if (type == PTP_CLASS_NONE)
return;
@@ -46,10 +50,15 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
{
struct mii_timestamper *mii_ts;
unsigned int type;
+ u32 ts_layer;
if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts)
return false;
+ ts_layer = skb->dev->ts_layer;
+ if (!(ts_layer & PHYLIB_TIMESTAMPING))
+ return false;
+
if (skb_headroom(skb) < ETH_HLEN)
return false;
@@ -633,13 +633,25 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
{
const struct ethtool_ops *ops = dev->ethtool_ops;
struct phy_device *phydev = dev->phydev;
+ u32 ts_layer = dev->ts_layer;
+ int ret;
memset(info, 0, sizeof(*info));
info->cmd = ETHTOOL_GET_TS_INFO;
- if (phy_has_tsinfo(phydev))
+ if (ts_layer == SOFTWARE_TIMESTAMPING) {
+ ret = ops->get_ts_info(dev, info);
+ if (ret)
+ return ret;
+ info->so_timestamping &= ~SOF_TIMESTAMPING_HARDWARE_MASK;
+ info->phc_index = -1;
+ return ret;
+ }
+
+ if (ts_layer & PHYLIB_TIMESTAMPING)
return phy_ts_info(phydev, info);
- if (ops->get_ts_info)
+
+ if (ts_layer & NETDEV_TIMESTAMPING)
return ops->get_ts_info(dev, info);
info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |