[PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[Hangbin Liu]

We have VLAN PTP support(via get_ts_info) on kernel, and bond support(by
getting active interface via netlink message) on userspace tool linuxptp.
But there are always some users who want to use PTP with VLAN over bond,
which is not able to do with the current implementation.

This patch passed get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device
with bond mode active-backup/tlb/alb. With this users could get kernel native
bond or VLAN over bond PTP support.

Test with ptp4l and it works with VLAN over bond after this patch:
]# ptp4l -m -i bond0.23
ptp4l[53377.141]: selected /dev/ptp4 as PTP clock
ptp4l[53377.142]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[53377.143]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[53377.143]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[53384.127]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
ptp4l[53384.127]: selected local clock e41d2d.fffe.123db0 as best master
ptp4l[53384.127]: port 1: assuming the grand master role

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 55 +++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index cf73eacdda91..1fc7249abf6d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -71,6 +71,7 @@
 #include <linux/ethtool.h>
 #include <linux/if_vlan.h>
 #include <linux/if_bonding.h>
+#include <linux/phy.h>
 #include <linux/jiffies.h>
 #include <linux/preempt.h>
 #include <net/route.h>
@@ -4091,7 +4092,10 @@ static int bond_eth_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cm
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct mii_ioctl_data *mii = NULL;
-	int res;
+	const struct net_device_ops *ops;
+	struct net_device *real_dev;
+	struct ifreq ifrr;
+	int res = 0;
 
 	netdev_dbg(bond_dev, "bond_eth_ioctl: cmd=%d\n", cmd);
 
@@ -4117,7 +4121,24 @@ static int bond_eth_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cm
 				mii->val_out = BMSR_LSTATUS;
 		}
 
-		return 0;
+		break;
+	case SIOCSHWTSTAMP:
+	case SIOCGHWTSTAMP:
+		rcu_read_lock();
+		real_dev = bond_option_active_slave_get_rcu(bond);
+		rcu_read_unlock();
+		if (real_dev) {
+			strscpy_pad(ifrr.ifr_name, real_dev->name, IFNAMSIZ);
+			ifrr.ifr_ifru = ifr->ifr_ifru;
+
+			ops = real_dev->netdev_ops;
+			if (netif_device_present(real_dev) && ops->ndo_eth_ioctl)
+				res = ops->ndo_eth_ioctl(real_dev, &ifrr, cmd);
+
+			if (!res)
+				ifr->ifr_ifru = ifrr.ifr_ifru;
+		}
+		break;
 	default:
 		res = -EOPNOTSUPP;
 	}
@@ -5319,10 +5340,40 @@ static void bond_ethtool_get_drvinfo(struct net_device *bond_dev,
 		 BOND_ABI_VERSION);
 }
 
+static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
+				    struct ethtool_ts_info *info)
+{
+	struct bonding *bond = netdev_priv(bond_dev);
+	const struct ethtool_ops *ops;
+	struct net_device *real_dev;
+	struct phy_device *phydev;
+
+	rcu_read_lock();
+	real_dev = bond_option_active_slave_get_rcu(bond);
+	rcu_read_unlock();
+	if (real_dev) {
+		ops = real_dev->ethtool_ops;
+		phydev = real_dev->phydev;
+
+		if (phy_has_tsinfo(phydev)) {
+			return phy_ts_info(phydev, info);
+		} else if (ops->get_ts_info) {
+			return ops->get_ts_info(real_dev, info);
+		}
+	}
+
+	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
+				SOF_TIMESTAMPING_SOFTWARE;
+	info->phc_index = -1;
+
+	return 0;
+}
+
 static const struct ethtool_ops bond_ethtool_ops = {
 	.get_drvinfo		= bond_ethtool_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
 	.get_link_ksettings	= bond_ethtool_get_link_ksettings,
+	.get_ts_info		= bond_ethtool_get_ts_info,
 };
 
 static const struct net_device_ops bond_netdev_ops = {
-- 
2.31.1

Re: [PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Tue, 30 Nov 2021 15:09:32 +0800 you wrote:
> We have VLAN PTP support(via get_ts_info) on kernel, and bond support(by
> getting active interface via netlink message) on userspace tool linuxptp.
> But there are always some users who want to use PTP with VLAN over bond,
> which is not able to do with the current implementation.
> 
> This patch passed get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device
> with bond mode active-backup/tlb/alb. With this users could get kernel native
> bond or VLAN over bond PTP support.
> 
> [...]

Here is the summary with links:
  - [net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device
    https://git.kernel.org/netdev/net-next/c/94dd016ae538

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[Jakub Kicinski]

On Tue, 30 Nov 2021 15:09:32 +0800 Hangbin Liu wrote:
> We have VLAN PTP support(via get_ts_info) on kernel, and bond support(by
> getting active interface via netlink message) on userspace tool linuxptp.
> But there are always some users who want to use PTP with VLAN over bond,
> which is not able to do with the current implementation.
> 
> This patch passed get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device
> with bond mode active-backup/tlb/alb. With this users could get kernel native
> bond or VLAN over bond PTP support.
> 
> Test with ptp4l and it works with VLAN over bond after this patch:
> ]# ptp4l -m -i bond0.23
> ptp4l[53377.141]: selected /dev/ptp4 as PTP clock
> ptp4l[53377.142]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
> ptp4l[53377.143]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
> ptp4l[53377.143]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
> ptp4l[53384.127]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
> ptp4l[53384.127]: selected local clock e41d2d.fffe.123db0 as best master
> ptp4l[53384.127]: port 1: assuming the grand master role

Does the Ethernet spec say something about PTP over bond/LACP?

What happens during failover? Presumably the user space daemon will
start getting HW stamps based on a different PHC than it's disciplining?

Re: [PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[Hangbin Liu]

On Tue, Nov 30, 2021 at 07:19:56AM -0800, Jakub Kicinski wrote:
> On Tue, 30 Nov 2021 15:09:32 +0800 Hangbin Liu wrote:
> > We have VLAN PTP support(via get_ts_info) on kernel, and bond support(by
> > getting active interface via netlink message) on userspace tool linuxptp.
> > But there are always some users who want to use PTP with VLAN over bond,
> > which is not able to do with the current implementation.
> > 
> > This patch passed get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device
> > with bond mode active-backup/tlb/alb. With this users could get kernel native
> > bond or VLAN over bond PTP support.
> > 
> > Test with ptp4l and it works with VLAN over bond after this patch:
> > ]# ptp4l -m -i bond0.23
> > ptp4l[53377.141]: selected /dev/ptp4 as PTP clock
> > ptp4l[53377.142]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
> > ptp4l[53377.143]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
> > ptp4l[53377.143]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
> > ptp4l[53384.127]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
> > ptp4l[53384.127]: selected local clock e41d2d.fffe.123db0 as best master
> > ptp4l[53384.127]: port 1: assuming the grand master role
> 
> Does the Ethernet spec say something about PTP over bond/LACP?

bond_option_active_slave_get_rcu() only supports bond mode active-backup/tlb/alb.
With LACP mode _no_ active interface returns and we will use software
timestamping.

But you remind me that we should return -EOPNOTSUPP when there is no real_dev
for bond_eth_ioctl(). I will send a fixup later.

> 
> What happens during failover? Presumably the user space daemon will
> start getting HW stamps based on a different PHC than it's disciplining?

linuxptp will switch to new active interface's PHC device when there is a
bonding failover by filtering netlink message on pure bond.

But for VLAN over bond I need to figure out how to get the bond failover
message on VLAN interface and update the new PHC device.

Thanks
Hangbin

Re: [PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[Jakub Kicinski]

On Wed, 1 Dec 2021 12:57:22 +0800 Hangbin Liu wrote:
> On Tue, Nov 30, 2021 at 07:19:56AM -0800, Jakub Kicinski wrote:
> > On Tue, 30 Nov 2021 15:09:32 +0800 Hangbin Liu wrote:  
> > > We have VLAN PTP support(via get_ts_info) on kernel, and bond support(by
> > > getting active interface via netlink message) on userspace tool linuxptp.
> > > But there are always some users who want to use PTP with VLAN over bond,
> > > which is not able to do with the current implementation.
> > > 
> > > This patch passed get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device
> > > with bond mode active-backup/tlb/alb. With this users could get kernel native
> > > bond or VLAN over bond PTP support.
> > > 
> > > Test with ptp4l and it works with VLAN over bond after this patch:
> > > ]# ptp4l -m -i bond0.23
> > > ptp4l[53377.141]: selected /dev/ptp4 as PTP clock
> > > ptp4l[53377.142]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
> > > ptp4l[53377.143]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
> > > ptp4l[53377.143]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
> > > ptp4l[53384.127]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
> > > ptp4l[53384.127]: selected local clock e41d2d.fffe.123db0 as best master
> > > ptp4l[53384.127]: port 1: assuming the grand master role  
> > 
> > Does the Ethernet spec say something about PTP over bond/LACP?  
> 
> bond_option_active_slave_get_rcu() only supports bond mode active-backup/tlb/alb.
> With LACP mode _no_ active interface returns and we will use software
> timestamping.

I understand that, I was asking about guidance from LACP as there is 
no IEEE standard for the other modes to my knowledge.

> But you remind me that we should return -EOPNOTSUPP when there is no real_dev
> for bond_eth_ioctl(). I will send a fixup later.
> 
> > What happens during failover? Presumably the user space daemon will
> > start getting HW stamps based on a different PHC than it's disciplining?  
> 
> linuxptp will switch to new active interface's PHC device when there is a
> bonding failover by filtering netlink message on pure bond.
> 
> But for VLAN over bond I need to figure out how to get the bond failover
> message on VLAN interface and update the new PHC device.

Yeah, there should be some form of well understood indication in the
uAPI telling the user space daemon that the PHC may get swapped on the
interface, and a reliable notification which indicates PHC change.
There is a number of user space daemons out there, fixing linuxptp does
not mean other user space won't be broken/surprised/angry.

What notification is linuxptp listening on, SETLINK?

Re: [PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[Hangbin Liu]

On Wed, Dec 01, 2021 at 07:11:18AM -0800, Jakub Kicinski wrote:
> > > Does the Ethernet spec say something about PTP over bond/LACP?  
> > 
> > bond_option_active_slave_get_rcu() only supports bond mode active-backup/tlb/alb.
> > With LACP mode _no_ active interface returns and we will use software
> > timestamping.
> 
> I understand that, I was asking about guidance from LACP as there is 
> no IEEE standard for the other modes to my knowledge.

Oh, I checked IEEE 8021AX, this is only one paragraph mentioned PTP

8. Frame distribution and collection algorithms
8.2 Per-service frame distribution
8.2.1 Goals and objectives
Distributing frames to physical links by service ID, as defined in 8.2,
provides the following:

b) Bidirectional congruity: Providing a means for the two ends of an Aggregation Group to use the
same physical link in both directions for a given service ensures that a failed link or a link
experiencing an excessive error rate will affect the fewest possible number of services and in general
provide support for protocols that have strict symmetry requirements on their transmit and receive
paths, e.g., Precision Time Protocol (PTP) in IEEE Std 1588 ™ -2008.

But I didn't find any valuable information from it.

> 
> > But you remind me that we should return -EOPNOTSUPP when there is no real_dev
> > for bond_eth_ioctl(). I will send a fixup later.
> > 
> > > What happens during failover? Presumably the user space daemon will
> > > start getting HW stamps based on a different PHC than it's disciplining?  
> > 
> > linuxptp will switch to new active interface's PHC device when there is a
> > bonding failover by filtering netlink message on pure bond.
> > 
> > But for VLAN over bond I need to figure out how to get the bond failover
> > message on VLAN interface and update the new PHC device.
> 
> Yeah, there should be some form of well understood indication in the
> uAPI telling the user space daemon that the PHC may get swapped on the
> interface, and a reliable notification which indicates PHC change.
> There is a number of user space daemons out there, fixing linuxptp does
> not mean other user space won't be broken/surprised/angry.

This is a RFE, I don't think this patch will affect the current user space as
the new topology is not supported before. i.e. no user space tool will configure
PTP based on bond or vlan over bond. And even the user space use other ways to
get bond's active interface, e.g. via netlink message. It still not affected
and could keep using the old way. So I think this patch should be safe.

Did I miss any thing?
> 
> What notification is linuxptp listening on, SETLINK?

Currently, I send RTM_GETLINK message on bond and listening on
RTM_NEWLINK message to get IFLA_LINKINFO info.

But for the new VLAN over bond topology. I haven't figure out a good solution.
Maybe I can just check the active interface status. When it get down,
do get_ts_info once again to get the new active interface on the VLAN over
bond interface. I need some testing.

Hangbin

Re: [PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[Jakub Kicinski]

On Thu, 2 Dec 2021 11:04:40 +0800 Hangbin Liu wrote:
> > Yeah, there should be some form of well understood indication in the
> > uAPI telling the user space daemon that the PHC may get swapped on the
> > interface, and a reliable notification which indicates PHC change.
> > There is a number of user space daemons out there, fixing linuxptp does
> > not mean other user space won't be broken/surprised/angry.  
> 
> This is a RFE, I don't think this patch will affect the current user space as
> the new topology is not supported before. i.e. no user space tool will configure
> PTP based on bond or vlan over bond. And even the user space use other ways to
> get bond's active interface, e.g. via netlink message. It still not affected
> and could keep using the old way. So I think this patch should be safe.
> 
> Did I miss any thing?

User can point their PTP daemon at any interface. Since bond now
supports the uAPI the user will be blissfully unaware that their
configuration will break if failover happens.

We can't expect every user and every PTP daemon to magically understand
the implicit quirks of the drivers. Quirks which are not even
documented.

What I'm saying is that we should have a new bit in the uAPI that
tells us that the user space can deal with unstable PHC idx and reject
the request forwarding in bond if that bit is not set. We have a flags
field in hwtstamp_config which should fit the bill. Make sense?

> > What notification is linuxptp listening on, SETLINK?  
> 
> Currently, I send RTM_GETLINK message on bond and listening on
> RTM_NEWLINK message to get IFLA_LINKINFO info.
> 
> But for the new VLAN over bond topology. I haven't figure out a good solution.
> Maybe I can just check the active interface status. When it get down,
> do get_ts_info once again to get the new active interface on the VLAN over
> bond interface. I need some testing.

Re: [PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[Hangbin Liu]

On Thu, Dec 02, 2021 at 06:59:23AM -0800, Jakub Kicinski wrote:
> On Thu, 2 Dec 2021 11:04:40 +0800 Hangbin Liu wrote:
> > > Yeah, there should be some form of well understood indication in the
> > > uAPI telling the user space daemon that the PHC may get swapped on the
> > > interface, and a reliable notification which indicates PHC change.
> > > There is a number of user space daemons out there, fixing linuxptp does
> > > not mean other user space won't be broken/surprised/angry.  
> > 
> > This is a RFE, I don't think this patch will affect the current user space as
> > the new topology is not supported before. i.e. no user space tool will configure
> > PTP based on bond or vlan over bond. And even the user space use other ways to
> > get bond's active interface, e.g. via netlink message. It still not affected
> > and could keep using the old way. So I think this patch should be safe.
> > 
> > Did I miss any thing?
> 
> User can point their PTP daemon at any interface. Since bond now
> supports the uAPI the user will be blissfully unaware that their
> configuration will break if failover happens.
> 
> We can't expect every user and every PTP daemon to magically understand
> the implicit quirks of the drivers. Quirks which are not even
> documented.

Thanks for the explanation. I understand what you mean now.
> 
> What I'm saying is that we should have a new bit in the uAPI that
> tells us that the user space can deal with unstable PHC idx and reject
> the request forwarding in bond if that bit is not set. We have a flags
> field in hwtstamp_config which should fit the bill. Make sense?

Yes, this makes sense for me. I check this and try post a patch next week.

Thanks
Hangbin

Re: [PATCH net-next] bond: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device

[Jakub Kicinski]

On Fri, 3 Dec 2021 10:55:04 +0800 Hangbin Liu wrote:
> On Thu, Dec 02, 2021 at 06:59:23AM -0800, Jakub Kicinski wrote:
> > On Thu, 2 Dec 2021 11:04:40 +0800 Hangbin Liu wrote:  
> > User can point their PTP daemon at any interface. Since bond now
> > supports the uAPI the user will be blissfully unaware that their
> > configuration will break if failover happens.
> > 
> > We can't expect every user and every PTP daemon to magically understand
> > the implicit quirks of the drivers. Quirks which are not even
> > documented.  
> 
> Thanks for the explanation. I understand what you mean now.
> > 
> > What I'm saying is that we should have a new bit in the uAPI that
> > tells us that the user space can deal with unstable PHC idx and reject
> > the request forwarding in bond if that bit is not set. We have a flags
> > field in hwtstamp_config which should fit the bill. Make sense?  
> 
> Yes, this makes sense for me. I check this and try post a patch next week.

SGTM, thanks!