[PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Richard Cochran]

Currently the stack supports time stamping in PHY devices.  However,
there are newer, non-PHY devices that can snoop an MII bus and provide
time stamps.  In order to support such devices, this patch introduces
a new interface to be used by both PHY and non-PHY devices.

In addition, the one and only user of the old PHY time stamping API is
converted to the new interface.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/phy/dp83640.c       | 47 +++++++++++++++++++++++++------------
 drivers/net/phy/phy.c           |  4 ++--
 drivers/net/phy/phy_device.c    |  2 ++
 include/linux/mii_timestamper.h | 52 +++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h             | 25 ++------------------
 net/8021q/vlan_dev.c            |  4 ++--
 net/core/ethtool.c              |  4 ++--
 net/core/timestamping.c         | 20 ++++++++--------
 8 files changed, 104 insertions(+), 54 deletions(-)
 create mode 100644 include/linux/mii_timestamper.h

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index edd4d44a386d..2f895c9bbedb 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -111,6 +111,7 @@ struct dp83640_private {
 	struct list_head list;
 	struct dp83640_clock *clock;
 	struct phy_device *phydev;
+	struct mii_timestamper mii_ts;
 	struct delayed_work ts_work;
 	int hwts_tx_en;
 	int hwts_rx_en;
@@ -214,6 +215,14 @@ static void dp83640_gpio_defaults(struct ptp_pin_desc *pd)
 static LIST_HEAD(phyter_clocks);
 static DEFINE_MUTEX(phyter_clocks_lock);
 
+static int dp83640_hwtstamp(struct mii_timestamper *mii_ts,
+			    struct ifreq *ifr);
+static int dp83640_ts_info(struct mii_timestamper *mii_ts,
+			   struct ethtool_ts_info *info);
+static bool dp83640_rxtstamp(struct mii_timestamper *mii_ts,
+			     struct sk_buff *skb, int type);
+static void dp83640_txtstamp(struct mii_timestamper *mii_ts,
+			     struct sk_buff *skb, int type);
 static void rx_timestamp_work(struct work_struct *work);
 
 /* extended register access functions */
@@ -1141,13 +1150,18 @@ static int dp83640_probe(struct phy_device *phydev)
 		goto no_memory;
 
 	dp83640->phydev = phydev;
-	INIT_DELAYED_WORK(&dp83640->ts_work, rx_timestamp_work);
+	dp83640->mii_ts.rxtstamp = dp83640_rxtstamp;
+	dp83640->mii_ts.txtstamp = dp83640_txtstamp;
+	dp83640->mii_ts.hwtstamp = dp83640_hwtstamp;
+	dp83640->mii_ts.ts_info  = dp83640_ts_info;
 
+	INIT_DELAYED_WORK(&dp83640->ts_work, rx_timestamp_work);
 	INIT_LIST_HEAD(&dp83640->rxts);
 	INIT_LIST_HEAD(&dp83640->rxpool);
 	for (i = 0; i < MAX_RXTS; i++)
 		list_add(&dp83640->rx_pool_data[i].list, &dp83640->rxpool);
 
+	phydev->mii_ts = &dp83640->mii_ts;
 	phydev->priv = dp83640;
 
 	spin_lock_init(&dp83640->rx_lock);
@@ -1188,6 +1202,8 @@ static void dp83640_remove(struct phy_device *phydev)
 	if (phydev->mdio.addr == BROADCAST_ADDR)
 		return;
 
+	phydev->mii_ts = NULL;
+
 	enable_status_frames(phydev, false);
 	cancel_delayed_work_sync(&dp83640->ts_work);
 
@@ -1311,9 +1327,10 @@ static int dp83640_config_intr(struct phy_device *phydev)
 	}
 }
 
-static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
+static int dp83640_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 {
-	struct dp83640_private *dp83640 = phydev->priv;
+	struct dp83640_private *dp83640 =
+		container_of(mii_ts, struct dp83640_private, mii_ts);
 	struct hwtstamp_config cfg;
 	u16 txcfg0, rxcfg0;
 
@@ -1389,8 +1406,8 @@ static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
 
 	mutex_lock(&dp83640->clock->extreg_lock);
 
-	ext_write(0, phydev, PAGE5, PTP_TXCFG0, txcfg0);
-	ext_write(0, phydev, PAGE5, PTP_RXCFG0, rxcfg0);
+	ext_write(0, dp83640->phydev, PAGE5, PTP_TXCFG0, txcfg0);
+	ext_write(0, dp83640->phydev, PAGE5, PTP_RXCFG0, rxcfg0);
 
 	mutex_unlock(&dp83640->clock->extreg_lock);
 
@@ -1420,10 +1437,11 @@ static void rx_timestamp_work(struct work_struct *work)
 		schedule_delayed_work(&dp83640->ts_work, SKB_TIMESTAMP_TIMEOUT);
 }
 
-static bool dp83640_rxtstamp(struct phy_device *phydev,
+static bool dp83640_rxtstamp(struct mii_timestamper *mii_ts,
 			     struct sk_buff *skb, int type)
 {
-	struct dp83640_private *dp83640 = phydev->priv;
+	struct dp83640_private *dp83640 =
+		container_of(mii_ts, struct dp83640_private, mii_ts);
 	struct dp83640_skb_info *skb_info = (struct dp83640_skb_info *)skb->cb;
 	struct list_head *this, *next;
 	struct rxts *rxts;
@@ -1469,10 +1487,11 @@ static bool dp83640_rxtstamp(struct phy_device *phydev,
 	return true;
 }
 
-static void dp83640_txtstamp(struct phy_device *phydev,
+static void dp83640_txtstamp(struct mii_timestamper *mii_ts,
 			     struct sk_buff *skb, int type)
 {
-	struct dp83640_private *dp83640 = phydev->priv;
+	struct dp83640_private *dp83640 =
+		container_of(mii_ts, struct dp83640_private, mii_ts);
 
 	switch (dp83640->hwts_tx_en) {
 
@@ -1494,9 +1513,11 @@ static void dp83640_txtstamp(struct phy_device *phydev,
 	}
 }
 
-static int dp83640_ts_info(struct phy_device *dev, struct ethtool_ts_info *info)
+static int dp83640_ts_info(struct mii_timestamper *mii_ts,
+			   struct ethtool_ts_info *info)
 {
-	struct dp83640_private *dp83640 = dev->priv;
+	struct dp83640_private *dp83640 =
+		container_of(mii_ts, struct dp83640_private, mii_ts);
 
 	info->so_timestamping =
 		SOF_TIMESTAMPING_TX_HARDWARE |
@@ -1528,10 +1549,6 @@ static struct phy_driver dp83640_driver = {
 	.config_init	= dp83640_config_init,
 	.ack_interrupt  = dp83640_ack_interrupt,
 	.config_intr    = dp83640_config_intr,
-	.ts_info	= dp83640_ts_info,
-	.hwtstamp	= dp83640_hwtstamp,
-	.rxtstamp	= dp83640_rxtstamp,
-	.txtstamp	= dp83640_txtstamp,
 };
 
 static int __init dp83640_init(void)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 14509a8903c6..1b84747dc1df 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -457,8 +457,8 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		return 0;
 
 	case SIOCSHWTSTAMP:
-		if (phydev->drv && phydev->drv->hwtstamp)
-			return phydev->drv->hwtstamp(phydev, ifr);
+		if (phydev->mii_ts && phydev->mii_ts->hwtstamp)
+			return phydev->mii_ts->hwtstamp(phydev->mii_ts, ifr);
 		/* fall through */
 
 	default:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 43cb08dcce81..a454432d166f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -871,6 +871,8 @@ static void phy_link_change(struct phy_device *phydev, bool up, bool do_carrier)
 			netif_carrier_off(netdev);
 	}
 	phydev->adjust_link(netdev);
+	if (phydev->mii_ts && phydev->mii_ts->link_state)
+		phydev->mii_ts->link_state(phydev->mii_ts, phydev);
 }
 
 /**
diff --git a/include/linux/mii_timestamper.h b/include/linux/mii_timestamper.h
new file mode 100644
index 000000000000..97e20e7033f6
--- /dev/null
+++ b/include/linux/mii_timestamper.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Support for generic time stamping devices on MII buses.
+ * Copyright (C) 2018 Richard Cochran <richardcochran@gmail.com>
+ */
+#ifndef _LINUX_MII_TIMESTAMPER_H
+#define _LINUX_MII_TIMESTAMPER_H
+
+#include <linux/device.h>
+#include <linux/ethtool.h>
+#include <linux/skbuff.h>
+
+/**
+ * struct mii_timestamper - Callback interface to MII time stamping devices.
+ *
+ * @rxtstamp:	Requests a Rx timestamp for 'skb'.  If the skb is accepted,
+ *		the MII time stamping device promises to deliver it using
+ *		netif_rx() as soon as a timestamp becomes available. One of
+ *		the PTP_CLASS_ values is passed in 'type'.  The function
+ *		must return true if the skb is accepted for delivery.
+ *
+ * @txtstamp:	Requests a Tx timestamp for 'skb'.  The MII time stamping
+ *		device promises to deliver it using skb_complete_tx_timestamp()
+ *		as soon as a timestamp becomes available. One of the PTP_CLASS_
+ *		values is passed in 'type'.
+ *
+ * @hwtstamp:	Handles SIOCSHWTSTAMP ioctl for hardware time stamping.
+ * @link_state:	Allows the device to respond to changes in the link state.
+ * @ts_info:	Handles ethtool queries for hardware time stamping.
+ *
+ * Drivers for PHY time stamping devices should embed their
+ * mii_timestamper within a private structure, obtaining a reference
+ * to it using container_of().
+ */
+struct mii_timestamper {
+	bool (*rxtstamp)(struct mii_timestamper *mii_ts,
+			 struct sk_buff *skb, int type);
+
+	void (*txtstamp)(struct mii_timestamper *mii_ts,
+			 struct sk_buff *skb, int type);
+
+	int  (*hwtstamp)(struct mii_timestamper *mii_ts,
+			 struct ifreq *ifreq);
+
+	void (*link_state)(struct mii_timestamper *mii_ts,
+			   struct phy_device *phydev);
+
+	int  (*ts_info)(struct mii_timestamper *mii_ts,
+			struct ethtool_ts_info *ts_info);
+};
+
+#endif
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3ea87f774a76..09739e420a8e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -22,6 +22,7 @@
 #include <linux/linkmode.h>
 #include <linux/mdio.h>
 #include <linux/mii.h>
+#include <linux/mii_timestamper.h>
 #include <linux/module.h>
 #include <linux/timer.h>
 #include <linux/workqueue.h>
@@ -482,6 +483,7 @@ struct phy_device {
 
 	struct phylink *phylink;
 	struct net_device *attached_dev;
+	struct mii_timestamper *mii_ts;
 
 	u8 mdix;
 	u8 mdix_ctrl;
@@ -578,29 +580,6 @@ struct phy_driver {
 	 */
 	int (*match_phy_device)(struct phy_device *phydev);
 
-	/* Handles ethtool queries for hardware time stamping. */
-	int (*ts_info)(struct phy_device *phydev, struct ethtool_ts_info *ti);
-
-	/* Handles SIOCSHWTSTAMP ioctl for hardware time stamping. */
-	int  (*hwtstamp)(struct phy_device *phydev, struct ifreq *ifr);
-
-	/*
-	 * Requests a Rx timestamp for 'skb'. If the skb is accepted,
-	 * the phy driver promises to deliver it using netif_rx() as
-	 * soon as a timestamp becomes available. One of the
-	 * PTP_CLASS_ values is passed in 'type'. The function must
-	 * return true if the skb is accepted for delivery.
-	 */
-	bool (*rxtstamp)(struct phy_device *dev, struct sk_buff *skb, int type);
-
-	/*
-	 * Requests a Tx timestamp for 'skb'. The phy driver promises
-	 * to deliver it using skb_complete_tx_timestamp() as soon as a
-	 * timestamp becomes available. One of the PTP_CLASS_ values
-	 * is passed in 'type'.
-	 */
-	void (*txtstamp)(struct phy_device *dev, struct sk_buff *skb, int type);
-
 	/* Some devices (e.g. qnap TS-119P II) require PHY register changes to
 	 * enable Wake on LAN, so set_wol is provided to be called in the
 	 * ethernet driver's set_wol function. */
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 546af0e73ac3..1d280a72ff54 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -670,8 +670,8 @@ static int vlan_ethtool_get_ts_info(struct net_device *dev,
 	const struct ethtool_ops *ops = vlan->real_dev->ethtool_ops;
 	struct phy_device *phydev = vlan->real_dev->phydev;
 
-	if (phydev && phydev->drv && phydev->drv->ts_info) {
-		 return phydev->drv->ts_info(phydev, info);
+	if (phydev && phydev->mii_ts && phydev->mii_ts->ts_info) {
+		return phydev->mii_ts->ts_info(phydev->mii_ts, info);
 	} else if (ops->get_ts_info) {
 		return ops->get_ts_info(vlan->real_dev, info);
 	} else {
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 3144ef2bf136..50e46249767f 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -2129,8 +2129,8 @@ static int ethtool_get_ts_info(struct net_device *dev, void __user *useraddr)
 	memset(&info, 0, sizeof(info));
 	info.cmd = ETHTOOL_GET_TS_INFO;
 
-	if (phydev && phydev->drv && phydev->drv->ts_info) {
-		err = phydev->drv->ts_info(phydev, &info);
+	if (phydev && phydev->mii_ts && phydev->mii_ts->ts_info) {
+		err = phydev->mii_ts->ts_info(phydev->mii_ts, &info);
 	} else if (ops->get_ts_info) {
 		err = ops->get_ts_info(dev, &info);
 	} else {
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 42689d5c468c..95c45c4dc0f9 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -26,7 +26,7 @@
 static unsigned int classify(const struct sk_buff *skb)
 {
 	if (likely(skb->dev && skb->dev->phydev &&
-		   skb->dev->phydev->drv))
+		   skb->dev->phydev->mii_ts))
 		return ptp_classify_raw(skb);
 	else
 		return PTP_CLASS_NONE;
@@ -34,7 +34,7 @@ static unsigned int classify(const struct sk_buff *skb)
 
 void skb_clone_tx_timestamp(struct sk_buff *skb)
 {
-	struct phy_device *phydev;
+	struct mii_timestamper *mii_ts;
 	struct sk_buff *clone;
 	unsigned int type;
 
@@ -45,22 +45,22 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
 	if (type == PTP_CLASS_NONE)
 		return;
 
-	phydev = skb->dev->phydev;
-	if (likely(phydev->drv->txtstamp)) {
+	mii_ts = skb->dev->phydev->mii_ts;
+	if (likely(mii_ts->txtstamp)) {
 		clone = skb_clone_sk(skb);
 		if (!clone)
 			return;
-		phydev->drv->txtstamp(phydev, clone, type);
+		mii_ts->txtstamp(mii_ts, clone, type);
 	}
 }
 EXPORT_SYMBOL_GPL(skb_clone_tx_timestamp);
 
 bool skb_defer_rx_timestamp(struct sk_buff *skb)
 {
-	struct phy_device *phydev;
+	struct mii_timestamper *mii_ts;
 	unsigned int type;
 
-	if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->drv)
+	if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts)
 		return false;
 
 	if (skb_headroom(skb) < ETH_HLEN)
@@ -75,9 +75,9 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
 	if (type == PTP_CLASS_NONE)
 		return false;
 
-	phydev = skb->dev->phydev;
-	if (likely(phydev->drv->rxtstamp))
-		return phydev->drv->rxtstamp(phydev, skb, type);
+	mii_ts = skb->dev->phydev->mii_ts;
+	if (likely(mii_ts->rxtstamp))
+		return mii_ts->rxtstamp(mii_ts, skb, type);
 
 	return false;
 }
-- 
2.11.0

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Andrew Lunn]

On Sun, Oct 07, 2018 at 10:38:20AM -0700, Richard Cochran wrote:
> Currently the stack supports time stamping in PHY devices.  However,
> there are newer, non-PHY devices that can snoop an MII bus and provide
> time stamps.  In order to support such devices, this patch introduces
> a new interface to be used by both PHY and non-PHY devices.
> 
> In addition, the one and only user of the old PHY time stamping API is
> converted to the new interface.

Hi Richard

I'm a bit undecided about this. If you look at how we do HWMON sensors
in PHYs, the probe function just registers with the HWMON subsystem.
We don't have any support in phy_device, or anywhere else in the PHY
core.

The mii_timestamper is generic, in the same why hwmon is generic. It
does not matter where the time stamper is. So i'm wondering if we
should remove the special case for a PHY timestamper, remove all the
phylib support, etc.

I need to look at the other patches and see how this all fits
together.

	Andrew

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Florian Fainelli]

On 10/7/2018 11:27 AM, Andrew Lunn wrote:
> On Sun, Oct 07, 2018 at 10:38:20AM -0700, Richard Cochran wrote:
>> Currently the stack supports time stamping in PHY devices.  However,
>> there are newer, non-PHY devices that can snoop an MII bus and provide
>> time stamps.  In order to support such devices, this patch introduces
>> a new interface to be used by both PHY and non-PHY devices.
>>
>> In addition, the one and only user of the old PHY time stamping API is
>> converted to the new interface.
> 
> Hi Richard
> 
> I'm a bit undecided about this. If you look at how we do HWMON sensors
> in PHYs, the probe function just registers with the HWMON subsystem.
> We don't have any support in phy_device, or anywhere else in the PHY
> core.
> 
> The mii_timestamper is generic, in the same why hwmon is generic. It
> does not matter where the time stamper is. So i'm wondering if we
> should remove the special case for a PHY timestamper, remove all the
> phylib support, etc.
> 
> I need to look at the other patches and see how this all fits
> together.

Agreed, the fact that some PHYs capable of timestamping and register 
themselves as a timestamper makes sense, whether this needs to be backed 
into the core PHYLIB might have been something convenient at some point, 
but maybe we can revisit that paradigm now that there is more generic 
timestamping provider framework being proposed here.
-- 
Florian

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Richard Cochran]

On Sun, Oct 07, 2018 at 08:27:51PM +0200, Andrew Lunn wrote:
> The mii_timestamper is generic, in the same why hwmon is generic. It
> does not matter where the time stamper is. So i'm wondering if we
> should remove the special case for a PHY timestamper, remove all the
> phylib support, etc.

This implementation is (to the best of my understanding) what you were
asking for in your review of v1:

> So i really think you need to cleanly integrate into phylib and
> phylink.

> Use a phandle, and have
> of_mdiobus_register_phy() follow the phandle to get the device.

> To keep lifecycle issues simple, i would also keep it in phydev, not
> netdev.

This present series is a reasonable, incremental improvement to the
existing PHY time stamping support.  It will handle any use case that
I can think of, and I would like to avoid over-engineering this.

Thanks,
Richard

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Andrew Lunn]

On Sun, Oct 07, 2018 at 12:15:51PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 08:27:51PM +0200, Andrew Lunn wrote:
> > The mii_timestamper is generic, in the same why hwmon is generic. It
> > does not matter where the time stamper is. So i'm wondering if we
> > should remove the special case for a PHY timestamper, remove all the
> > phylib support, etc.
> 
> This implementation is (to the best of my understanding) what you were
> asking for in your review of v1:

Sure, but things have moved on since then.
 
> > So i really think you need to cleanly integrate into phylib and
> > phylink.
> 
> > Use a phandle, and have
> > of_mdiobus_register_phy() follow the phandle to get the device.
> 
> > To keep lifecycle issues simple, i would also keep it in phydev, not
> > netdev.
> 
> This present series is a reasonable, incremental improvement to the
> existing PHY time stamping support.  It will handle any use case that
> I can think of, and I would like to avoid over-engineering this.

I can think of three obvious use cases where this does not work:

1) phylink, not phdev. We have been pushing some MAC drivers towards
phylink, especially those which support >1Gbp.

2) When an SFP is connected to the MAC, not a copper PHY. The class of
device you are adding a driver for will work just as well for an SFP
as for a copper PHY. The SERDES interface remains the same,
independent of if a copper PHY is used, or a SFP. But an SFP does not
have an instance of a phydv.

2a) An SFP which is actually a Copper PHY. There is a phydev for this,
but it is associated to the phylink, not the netdev.

3) Firmware controlled PHYs. phylib/phylink is not used, the MAC turns
all ethtool calls into RPCs to the firmware. I've no numbers about
this, but i have the feeling this is becoming more popular. It does
however tend to be high end devices, and those are more likely to have
timestamping in the MAC. I suppose they could also offload
tomestamping to the firmware, in which case, they might want to make
use of this new API.

    Andrew

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Richard Cochran]

On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> Sure, but things have moved on since then.

If you have a specific suggestion on how to better implement this,
please tell us what it is.
  
> I can think of three obvious use cases where this does not work:
> 
> 1) phylink, not phdev. We have been pushing some MAC drivers towards
> phylink, especially those which support >1Gbp.

If a phylink device appears that wants time stamping, can't we add the
call to register_mii_timestamper()?
 
> 2) When an SFP is connected to the MAC, not a copper PHY. The class of
> device you are adding a driver for will work just as well for an SFP
> as for a copper PHY. The SERDES interface remains the same,
> independent of if a copper PHY is used, or a SFP. But an SFP does not
> have an instance of a phydv.

Well, as I said before in v1, CONFIG_NETWORK_PHY_TIMESTAMPING depends
on phylib, plain and simple, and expanding beyond phylib is not within
the scope of the this series.
 
> 3) Firmware controlled PHYs. phylib/phylink is not used, the MAC turns
> all ethtool calls into RPCs to the firmware. I've no numbers about
> this, but i have the feeling this is becoming more popular. It does
> however tend to be high end devices, and those are more likely to have
> timestamping in the MAC. I suppose they could also offload
> tomestamping to the firmware, in which case, they might want to make
> use of this new API.

Any MAC with private PHY stuff (that doesn't use phylib) can implement
SO_TIMESTAMPING directly, as if it were a MAC.

Thanks,
Richard

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Richard Cochran]

On Sun, Oct 07, 2018 at 01:59:06PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> > 1) phylink, not phdev. We have been pushing some MAC drivers towards
> > phylink, especially those which support >1Gbp.
> 
> If a phylink device appears that wants time stamping, can't we add the
> call to register_mii_timestamper()?

Actually, I see that 'struct phylink' has a 'struct phy_device *phydev',
and so it can implement the 'struct mii_timestamper' interface directly.

Thanks,
Richard

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Andrew Lunn]

On Sun, Oct 07, 2018 at 01:59:06PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> > Sure, but things have moved on since then.
> 
> If you have a specific suggestion on how to better implement this,
> please tell us what it is.
>   
> > I can think of three obvious use cases where this does not work:
> > 
> > 1) phylink, not phdev. We have been pushing some MAC drivers towards
> > phylink, especially those which support >1Gbp.
> 
> If a phylink device appears that wants time stamping, can't we add the
> call to register_mii_timestamper()?

Hi Richard

The problem is you depend on skbuf->dev->phydev. phydev will be NULL.
net_device does not currently have a phylink member. Even if it did,
you end up add more and more tests looking every place a
mii_timestamper could be placed.
  
> > 2) When an SFP is connected to the MAC, not a copper PHY. The class of
> > device you are adding a driver for will work just as well for an SFP
> > as for a copper PHY. The SERDES interface remains the same,
> > independent of if a copper PHY is used, or a SFP. But an SFP does not
> > have an instance of a phydv.
> 
> Well, as I said before in v1, CONFIG_NETWORK_PHY_TIMESTAMPING depends
> on phylib, plain and simple, and expanding beyond phylib is not within
> the scope of the this series.

True. But we also should be forward looking, to make sure we are not
heading into a dead end.

I'm currently thinking register_mii_timestamper() should take a netdev
argument, and the net_device structure should gain a struct
mii_timestamper.

But we have to look at the lifetime problems. A phydev does not know
what netdev it is associated to until phy_connect() is called. It is
at that point you can call register_mii_timestamper().

   Andrew

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Richard Cochran]

On Sun, Oct 07, 2018 at 11:14:05PM +0200, Andrew Lunn wrote:
> The problem is you depend on skbuf->dev->phydev. phydev will be NULL.
> net_device does not currently have a phylink member. Even if it did,
> you end up add more and more tests looking every place a
> mii_timestamper could be placed.

Ok, so the way to do this is to have something like
CONFIG_NETWORK_PHYLINK_TIMESTAMPING.  We can deal with that if and
when any real devices appear.

> I'm currently thinking register_mii_timestamper() should take a netdev
> argument, and the net_device structure should gain a struct
> mii_timestamper.
> 
> But we have to look at the lifetime problems. A phydev does not know
> what netdev it is associated to until phy_connect() is called. It is
> at that point you can call register_mii_timestamper().

Right, IOW passing a netdev won't work.

Thanks,
Richard

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Andrew Lunn]

On Sun, Oct 07, 2018 at 02:07:28PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 01:59:06PM -0700, Richard Cochran wrote:
> > On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> > > 1) phylink, not phdev. We have been pushing some MAC drivers towards
> > > phylink, especially those which support >1Gbp.
> > 
> > If a phylink device appears that wants time stamping, can't we add the
> > call to register_mii_timestamper()?
> 
> Actually, I see that 'struct phylink' has a 'struct phy_device *phydev',
> and so it can implement the 'struct mii_timestamper' interface directly.

Maybe. But you still don't have skb->dev->phydev. And phylink->phydev
is much more dynamic, since it can be hot-{un}plugged. You need to
handle it going away at any time.

However, your timestamper is unlikely to be hot-{un}pluggable. So
skb->dev->mii_timestamper seems a lot safer.

       Andrew

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Richard Cochran]

On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> Sure, but things have moved on since then.

I was curious about this.  Based on your uses cases, I guess that you
mean phylib?  But not much has changed AFAICT. (There is one new
global function and two were removed, but that doesn't change the
picture WRT time stamping.)

Phylink now has two or three new users, one of which is dsa.  Is that
the big move?

The situation with MACs that handle their own PHYs without phylib is
unchanged, AFAICT.

So what exactly do you mean?

Thanks,
Richard

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Richard Cochran]

On Sun, Oct 07, 2018 at 02:20:33PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 11:14:05PM +0200, Andrew Lunn wrote:
> > I'm currently thinking register_mii_timestamper() should take a netdev
> > argument, and the net_device structure should gain a struct
> > mii_timestamper.

We are going round in circles on this point.  V1 had it this way, but
nobody liked it.  You specifically asked to move the new pointer out
of the netdev and into phydev.

> > But we have to look at the lifetime problems. A phydev does not know
> > what netdev it is associated to until phy_connect() is called. It is
> > at that point you can call register_mii_timestamper().

I had used a netdev notifier on NETDEV_UP for this, but Florian seemed
to suggest using phy_{connect,attach,disconnect} instead.

Thanks,
Richard

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Andrew Lunn]

On Sun, Oct 07, 2018 at 07:04:39PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> > Sure, but things have moved on since then.
> 
> I was curious about this.  Based on your uses cases, I guess that you
> mean phylib?  But not much has changed AFAICT. (There is one new
> global function and two were removed, but that doesn't change the
> picture WRT time stamping.)
> 
> Phylink now has two or three new users, one of which is dsa.  Is that
> the big move?
> 
> The situation with MACs that handle their own PHYs without phylib is
> unchanged, AFAICT.
> 
> So what exactly do you mean?

Hi Richard

We are pushing phylink. I really do think anything using > 1Gbps links
should be using phylink, not phydev. And i think we have reached the
tipping point, that most new MACs will be > 1Gbps. 2.5G or maybe 5G
will be the new default. The MAC-PHY link is quiet messy when you get
above 1G. There are a number of options which you can use, and the MAC
and PHY need to negotiate a common set to use. phylink can do this,
phylib cannot easily do it. So i see phylib slowly becoming a legacy
API for MAC drivers.

We are also slowly seeing more SFPs, and Linux controlling them. SFP
are not new, they have been in top end switches for a long time. But
they are slowly becoming more popular in industrial settings, and such
embedded industrial systems tend to let Linux control them, not
firmware. And i think industry makes more use of PTP than other
fields, but i could be wrong. Since optical SFP modules are passive, a
bump-in-the-wire time stamper actually makes sense for these.

Also, fibre on the last mile is slowly becoming more of a thing, so
maybe we will start seeing CPE, consumer routers, with SFP ports?

As i said before, we are seeing more MACs which use firmware for
controlling the PHYs. I'm not sure why yet. Maybe it is coupled with
more MACs supporting > 1G, which is messy. Or the lack of good PHY
drivers for PHYs which support > 1G? Hopefully the drivers will
improve with time.

So as you said, the phylib API has not changed much, which is common
for mature code. But i think long term, it will become less important.
It will share the space with phylink. And any code which wants to be
generically usable, should not depend on phydev. Architecturally, it
seems wrong for you to hang what should be a generic time stamping
framework on phydev. It is not future proof. net_device is future
proof.

	  Andrew

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Richard Cochran]

On Mon, Oct 08, 2018 at 05:07:22PM +0200, Andrew Lunn wrote:
> So as you said, the phylib API has not changed much, which is common
> for mature code.

I meant that phy-LINK hasn't changed much.

> But i think long term, it will become less important.
> It will share the space with phylink. And any code which wants to be
> generically usable, should not depend on phydev.

Thanks for your view of the big picture.

> Architecturally, it
> seems wrong for you to hang what should be a generic time stamping
> framework on phydev. It is not future proof. net_device is future
> proof.

You still haven't said how net_device is going to work.

Today there are exactly zero phylink devices needing time stamping
support, but there are new phylib devices.  We don't have a
net_device->phylink connection, and it isn't needed yet.  Adding that
is way out of scope for this series.

Let's stick to phylib for now.  We can cross the other bridge when we
come to it.  Maybe the net_device->phylink will emerge for purposes
other that time stamping.  Let's not guess about how it should look.

We are only talking about kernel interfaces here, and so nothing is
set in stone.

Thanks,
Richard

Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.

[Richard Cochran]

On Mon, Oct 08, 2018 at 08:28:00AM -0700, Richard Cochran wrote:
> Let's stick to phylib for now.  We can cross the other bridge when we
> come to it.  Maybe the net_device->phylink will emerge for purposes
> other that time stamping.  Let's not guess about how it should look.

In fact, it is kernel policy to reject new framework that lacks users.
Even if we added new net_device->phylink hooks, I bet that davem would
reject them for that reason.  We can't just add to net_device for
vaporware.

The whole point of _this_ series is the first patch.  That new option
is important for complete PTP support, and it could be used by every
PTP hardware.  Unfortunately I only have one device in hand that
implements this, and that is the reason why you see patches 2-5.

Thanks,
Richard