[PATCH net-next v7 00/16] net: Make timestamping selectable

[PATCH net-next v7 00/16] net: Make timestamping selectable

[Kory Maincent]

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.

This series updates the default MAC/PHY default timestamping and aims to
allow the user to select the desired layer administratively.

Changes in v2:
- Move selected_timestamping_layer variable of the concerned patch.
- Use sysfs_streq instead of strmcmp.
- Use the PHY timestamp only if available.

Changes in v3:
- Expose the PTP choice to ethtool instead of sysfs.
  You can test it with the ethtool source on branch feature_ptp of:
  https://github.com/kmaincent/ethtool
- Added a devicetree binding to select the preferred timestamp.

Changes in v4:
- Move on to ethtool netlink instead of ioctl.
- Add a netdev notifier to allow packet trapping by the MAC in case of PHY
  time stamping.
- Add a PHY whitelist to not break the old PHY default time-stamping
  preference API.

Changes in v5:
- Update to ndo_hwstamp_get/set. This bring several new patches.
- Add few patches to make the glue.
- Convert macb to ndo_hwstamp_get/set.
- Add netlink specs description of new ethtool commands.
- Removed netdev notifier.
- Split the patches that expose the timestamping to userspace to separate
  the core and ethtool development.
- Add description of software timestamping.
- Convert PHYs hwtstamp callback to use kernel_hwtstamp_config.

Changes in v6:
- Few fixes from the reviews.
- Replace the allowlist to default_timestamp flag to know which phy is
  using old API behavior.
- Rename the timestamping layer enum values.
- Move to a simple enum instead of the mix between enum and bitfield.
- Update ts_info and ts-set in software timestamping case.

Changes in v7:
- Fix a temporary build error.
- Link to v6: https://lore.kernel.org/r/20231019-feature_ptp_netnext-v6-0-71affc27b0e5@bootlin.com

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
Kory Maincent (15):
      net: Convert PHYs hwtstamp callback to use kernel_hwtstamp_config
      net: phy: Remove the call to phy_mii_ioctl in phy_hwstamp_get/set
      net: macb: Convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()
      net: Make dev_set_hwtstamp_phylib accessible
      net: phy: micrel: fix ts_info value in case of no phc
      net_tstamp: Add TIMESTAMPING SOFTWARE and HARDWARE mask
      net: ethtool: Add a command to expose current time stamping layer
      netlink: specs: Introduce new netlink command to get current timestamp
      net: ethtool: Add a command to list available time stamping layers
      netlink: specs: Introduce new netlink command to list available time stamping layers
      net: Replace hwtstamp_source by timestamping layer
      net: Change the API of PHY default timestamp to MAC
      net: ethtool: ts: Update GET_TS to reply the current selected timestamp
      net: ethtool: ts: Let the active time stamping layer be selectable
      netlink: specs: Introduce time stamping set command

Richard Cochran (1):
      net: ethtool: Refactor identical get_ts_info implementations.

 Documentation/netlink/specs/ethtool.yaml           |  57 +++++
 Documentation/networking/ethtool-netlink.rst       |  63 ++++++
 drivers/net/bonding/bond_main.c                    |  29 +--
 drivers/net/ethernet/cadence/macb.h                |  15 +-
 drivers/net/ethernet/cadence/macb_main.c           |  42 +++-
 drivers/net/ethernet/cadence/macb_ptp.c            |  28 +--
 .../net/ethernet/microchip/lan966x/lan966x_main.c  |   6 +-
 drivers/net/macvlan.c                              |  14 +-
 drivers/net/phy/bcm-phy-ptp.c                      |  18 +-
 drivers/net/phy/dp83640.c                          |  27 +--
 drivers/net/phy/micrel.c                           |  50 ++---
 drivers/net/phy/mscc/mscc_ptp.c                    |  20 +-
 drivers/net/phy/nxp-c45-tja11xx.c                  |  20 +-
 drivers/net/phy/phy.c                              |  28 ++-
 drivers/net/phy/phy_device.c                       |  37 ++++
 drivers/ptp/ptp_ines.c                             |  16 +-
 include/linux/ethtool.h                            |   8 +
 include/linux/mii_timestamper.h                    |   4 +-
 include/linux/net_tstamp.h                         |  11 +-
 include/linux/netdevice.h                          |   8 +
 include/linux/phy.h                                |  10 +-
 include/uapi/linux/ethtool_netlink.h               |  29 +++
 include/uapi/linux/net_tstamp.h                    |  18 ++
 net/8021q/vlan_dev.c                               |  15 +-
 net/core/dev.c                                     |   3 +
 net/core/dev_ioctl.c                               |  43 ++--
 net/core/timestamping.c                            |  10 +
 net/ethtool/Makefile                               |   2 +-
 net/ethtool/common.c                               |  25 ++-
 net/ethtool/common.h                               |   1 +
 net/ethtool/netlink.c                              |  28 +++
 net/ethtool/netlink.h                              |   4 +
 net/ethtool/ts.c                                   | 244 +++++++++++++++++++++
 33 files changed, 732 insertions(+), 201 deletions(-)
---
base-commit: 5b7dd66960ba4e5cd2648692a6082a7ac6993867
change-id: 20231011-feature_ptp_netnext-3f278578e84b

Best regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net-next v7 01/16] net: Convert PHYs hwtstamp callback to use kernel_hwtstamp_config

[Kory Maincent]

The PHYs hwtstamp callback are still getting the timestamp config from
ifreq and using copy_from/to_user.
Get rid of these functions by using timestamp configuration in parameter.
This also allow to move on to kernel_hwtstamp_config and be similar to
net devices using the new ndo_hwstamp_get/set.

This adds the possibility to manipulate the timestamp configuration
from the kernel which was not possible with the copy_from/to_user.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---

Change in v6:
- Fix nxp_c45_hwtstamp config variable missing
---
 drivers/net/phy/bcm-phy-ptp.c     | 15 ++++++---------
 drivers/net/phy/dp83640.c         | 24 +++++++++++-------------
 drivers/net/phy/micrel.c          | 38 +++++++++++++++++---------------------
 drivers/net/phy/mscc/mscc_ptp.c   | 18 ++++++++----------
 drivers/net/phy/nxp-c45-tja11xx.c | 17 +++++++----------
 drivers/net/phy/phy.c             | 21 +++++++++++++++++++--
 drivers/ptp/ptp_ines.c            | 16 +++++++---------
 include/linux/mii_timestamper.h   |  4 +++-
 include/linux/phy.h               |  6 ++++--
 9 files changed, 82 insertions(+), 77 deletions(-)

diff --git a/drivers/net/phy/bcm-phy-ptp.c b/drivers/net/phy/bcm-phy-ptp.c
index cb4b91af5e17..617d384d4551 100644
--- a/drivers/net/phy/bcm-phy-ptp.c
+++ b/drivers/net/phy/bcm-phy-ptp.c
@@ -782,16 +782,13 @@ static void bcm_ptp_txtstamp(struct mii_timestamper *mii_ts,
 }
 
 static int bcm_ptp_hwtstamp(struct mii_timestamper *mii_ts,
-			    struct ifreq *ifr)
+			    struct kernel_hwtstamp_config *cfg,
+			    struct netlink_ext_ack *extack)
 {
 	struct bcm_ptp_private *priv = mii2priv(mii_ts);
-	struct hwtstamp_config cfg;
 	u16 mode, ctrl;
 
-	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
-		return -EFAULT;
-
-	switch (cfg.rx_filter) {
+	switch (cfg->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		priv->hwts_rx = false;
 		break;
@@ -804,14 +801,14 @@ static int bcm_ptp_hwtstamp(struct mii_timestamper *mii_ts,
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
 		priv->hwts_rx = true;
 		break;
 	default:
 		return -ERANGE;
 	}
 
-	priv->tx_type = cfg.tx_type;
+	priv->tx_type = cfg->tx_type;
 
 	ctrl  = priv->hwts_rx ? SLICE_RX_EN : 0;
 	ctrl |= priv->tx_type != HWTSTAMP_TX_OFF ? SLICE_TX_EN : 0;
@@ -840,7 +837,7 @@ static int bcm_ptp_hwtstamp(struct mii_timestamper *mii_ts,
 	/* purge existing data */
 	skb_queue_purge(&priv->tx_queue);
 
-	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+	return 0;
 }
 
 static int bcm_ptp_ts_info(struct mii_timestamper *mii_ts,
diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 2657be7cc049..5c42c47dc564 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1207,22 +1207,20 @@ static irqreturn_t dp83640_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
-static int dp83640_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
+static int dp83640_hwtstamp(struct mii_timestamper *mii_ts,
+			    struct kernel_hwtstamp_config *cfg,
+			    struct netlink_ext_ack *extack)
 {
 	struct dp83640_private *dp83640 =
 		container_of(mii_ts, struct dp83640_private, mii_ts);
-	struct hwtstamp_config cfg;
 	u16 txcfg0, rxcfg0;
 
-	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
-		return -EFAULT;
-
-	if (cfg.tx_type < 0 || cfg.tx_type > HWTSTAMP_TX_ONESTEP_SYNC)
+	if (cfg->tx_type < 0 || cfg->tx_type > HWTSTAMP_TX_ONESTEP_SYNC)
 		return -ERANGE;
 
-	dp83640->hwts_tx_en = cfg.tx_type;
+	dp83640->hwts_tx_en = cfg->tx_type;
 
-	switch (cfg.rx_filter) {
+	switch (cfg->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		dp83640->hwts_rx_en = 0;
 		dp83640->layer = 0;
@@ -1234,7 +1232,7 @@ static int dp83640_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 		dp83640->hwts_rx_en = 1;
 		dp83640->layer = PTP_CLASS_L4;
 		dp83640->version = PTP_CLASS_V1;
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
@@ -1242,7 +1240,7 @@ static int dp83640_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 		dp83640->hwts_rx_en = 1;
 		dp83640->layer = PTP_CLASS_L4;
 		dp83640->version = PTP_CLASS_V2;
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
@@ -1250,7 +1248,7 @@ static int dp83640_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 		dp83640->hwts_rx_en = 1;
 		dp83640->layer = PTP_CLASS_L2;
 		dp83640->version = PTP_CLASS_V2;
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
@@ -1258,7 +1256,7 @@ static int dp83640_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 		dp83640->hwts_rx_en = 1;
 		dp83640->layer = PTP_CLASS_L4 | PTP_CLASS_L2;
 		dp83640->version = PTP_CLASS_V2;
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
 		break;
 	default:
 		return -ERANGE;
@@ -1292,7 +1290,7 @@ static int dp83640_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 
 	mutex_unlock(&dp83640->clock->extreg_lock);
 
-	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+	return 0;
 }
 
 static void rx_timestamp_work(struct work_struct *work)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 08e3915001c3..99af1e500c6c 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -2395,24 +2395,22 @@ static void lan8814_flush_fifo(struct phy_device *phydev, bool egress)
 	lanphy_read_page_reg(phydev, 5, PTP_TSU_INT_STS);
 }
 
-static int lan8814_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
+static int lan8814_hwtstamp(struct mii_timestamper *mii_ts,
+			    struct kernel_hwtstamp_config *config,
+			    struct netlink_ext_ack *extack)
 {
 	struct kszphy_ptp_priv *ptp_priv =
 			  container_of(mii_ts, struct kszphy_ptp_priv, mii_ts);
 	struct phy_device *phydev = ptp_priv->phydev;
 	struct lan8814_shared_priv *shared = phydev->shared->priv;
 	struct lan8814_ptp_rx_ts *rx_ts, *tmp;
-	struct hwtstamp_config config;
 	int txcfg = 0, rxcfg = 0;
 	int pkt_ts_enable;
 
-	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
-		return -EFAULT;
+	ptp_priv->hwts_tx_type = config->tx_type;
+	ptp_priv->rx_filter = config->rx_filter;
 
-	ptp_priv->hwts_tx_type = config.tx_type;
-	ptp_priv->rx_filter = config.rx_filter;
-
-	switch (config.rx_filter) {
+	switch (config->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		ptp_priv->layer = 0;
 		ptp_priv->version = 0;
@@ -2458,13 +2456,13 @@ static int lan8814_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 		lanphy_write_page_reg(ptp_priv->phydev, 5, PTP_TX_MOD,
 				      PTP_TX_MOD_TX_PTP_SYNC_TS_INSERT_);
 
-	if (config.rx_filter != HWTSTAMP_FILTER_NONE)
+	if (config->rx_filter != HWTSTAMP_FILTER_NONE)
 		lan8814_config_ts_intr(ptp_priv->phydev, true);
 	else
 		lan8814_config_ts_intr(ptp_priv->phydev, false);
 
 	mutex_lock(&shared->shared_lock);
-	if (config.rx_filter != HWTSTAMP_FILTER_NONE)
+	if (config->rx_filter != HWTSTAMP_FILTER_NONE)
 		shared->ref++;
 	else
 		shared->ref--;
@@ -2488,7 +2486,7 @@ static int lan8814_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 	lan8814_flush_fifo(ptp_priv->phydev, false);
 	lan8814_flush_fifo(ptp_priv->phydev, true);
 
-	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ? -EFAULT : 0;
+	return 0;
 }
 
 static void lan8814_txtstamp(struct mii_timestamper *mii_ts,
@@ -3703,21 +3701,19 @@ static void lan8841_ptp_enable_processing(struct kszphy_ptp_priv *ptp_priv,
 #define LAN8841_PTP_TX_TIMESTAMP_EN		443
 #define LAN8841_PTP_TX_MOD			445
 
-static int lan8841_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
+static int lan8841_hwtstamp(struct mii_timestamper *mii_ts,
+			    struct kernel_hwtstamp_config *config,
+			    struct netlink_ext_ack *extack)
 {
 	struct kszphy_ptp_priv *ptp_priv = container_of(mii_ts, struct kszphy_ptp_priv, mii_ts);
 	struct phy_device *phydev = ptp_priv->phydev;
-	struct hwtstamp_config config;
 	int txcfg = 0, rxcfg = 0;
 	int pkt_ts_enable;
 
-	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
-		return -EFAULT;
+	ptp_priv->hwts_tx_type = config->tx_type;
+	ptp_priv->rx_filter = config->rx_filter;
 
-	ptp_priv->hwts_tx_type = config.tx_type;
-	ptp_priv->rx_filter = config.rx_filter;
-
-	switch (config.rx_filter) {
+	switch (config->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		ptp_priv->layer = 0;
 		ptp_priv->version = 0;
@@ -3771,13 +3767,13 @@ static int lan8841_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 
 	/* Now enable/disable the timestamping */
 	lan8841_ptp_enable_processing(ptp_priv,
-				      config.rx_filter != HWTSTAMP_FILTER_NONE);
+				      config->rx_filter != HWTSTAMP_FILTER_NONE);
 
 	skb_queue_purge(&ptp_priv->tx_queue);
 
 	lan8841_ptp_flush_fifo(ptp_priv);
 
-	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ? -EFAULT : 0;
+	return 0;
 }
 
 static bool lan8841_rxtstamp(struct mii_timestamper *mii_ts,
diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
index cf728bfd83e2..eb0b032cb613 100644
--- a/drivers/net/phy/mscc/mscc_ptp.c
+++ b/drivers/net/phy/mscc/mscc_ptp.c
@@ -1045,19 +1045,17 @@ static void vsc85xx_ts_reset_fifo(struct phy_device *phydev)
 			     val);
 }
 
-static int vsc85xx_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
+static int vsc85xx_hwtstamp(struct mii_timestamper *mii_ts,
+			    struct kernel_hwtstamp_config *cfg,
+			    struct netlink_ext_ack *extack)
 {
 	struct vsc8531_private *vsc8531 =
 		container_of(mii_ts, struct vsc8531_private, mii_ts);
 	struct phy_device *phydev = vsc8531->ptp->phydev;
-	struct hwtstamp_config cfg;
 	bool one_step = false;
 	u32 val;
 
-	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
-		return -EFAULT;
-
-	switch (cfg.tx_type) {
+	switch (cfg->tx_type) {
 	case HWTSTAMP_TX_ONESTEP_SYNC:
 		one_step = true;
 		break;
@@ -1069,9 +1067,9 @@ static int vsc85xx_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 		return -ERANGE;
 	}
 
-	vsc8531->ptp->tx_type = cfg.tx_type;
+	vsc8531->ptp->tx_type = cfg->tx_type;
 
-	switch (cfg.rx_filter) {
+	switch (cfg->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
@@ -1084,7 +1082,7 @@ static int vsc85xx_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 		return -ERANGE;
 	}
 
-	vsc8531->ptp->rx_filter = cfg.rx_filter;
+	vsc8531->ptp->rx_filter = cfg->rx_filter;
 
 	mutex_lock(&vsc8531->ts_lock);
 
@@ -1132,7 +1130,7 @@ static int vsc85xx_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 	vsc8531->ptp->configured = 1;
 	mutex_unlock(&vsc8531->ts_lock);
 
-	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+	return 0;
 }
 
 static int vsc85xx_ts_info(struct mii_timestamper *mii_ts,
diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
index 7ab080ff02df..780ad353cf55 100644
--- a/drivers/net/phy/nxp-c45-tja11xx.c
+++ b/drivers/net/phy/nxp-c45-tja11xx.c
@@ -1022,24 +1022,21 @@ static bool nxp_c45_rxtstamp(struct mii_timestamper *mii_ts,
 }
 
 static int nxp_c45_hwtstamp(struct mii_timestamper *mii_ts,
-			    struct ifreq *ifreq)
+			    struct kernel_hwtstamp_config *cfg,
+			    struct netlink_ext_ack *extack)
 {
 	struct nxp_c45_phy *priv = container_of(mii_ts, struct nxp_c45_phy,
 						mii_ts);
 	struct phy_device *phydev = priv->phydev;
 	const struct nxp_c45_phy_data *data;
-	struct hwtstamp_config cfg;
 
-	if (copy_from_user(&cfg, ifreq->ifr_data, sizeof(cfg)))
-		return -EFAULT;
-
-	if (cfg.tx_type < 0 || cfg.tx_type > HWTSTAMP_TX_ON)
+	if (cfg->tx_type < 0 || cfg->tx_type > HWTSTAMP_TX_ON)
 		return -ERANGE;
 
 	data = nxp_c45_get_data(phydev);
-	priv->hwts_tx = cfg.tx_type;
+	priv->hwts_tx = cfg->tx_type;
 
-	switch (cfg.rx_filter) {
+	switch (cfg->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		priv->hwts_rx = 0;
 		break;
@@ -1047,7 +1044,7 @@ static int nxp_c45_hwtstamp(struct mii_timestamper *mii_ts,
 	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
 		priv->hwts_rx = 1;
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
 		break;
 	default:
 		return -ERANGE;
@@ -1074,7 +1071,7 @@ static int nxp_c45_hwtstamp(struct mii_timestamper *mii_ts,
 		nxp_c45_clear_reg_field(phydev, &data->regmap->irq_egr_ts_en);
 
 nxp_c45_no_ptp_irq:
-	return copy_to_user(ifreq->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+	return 0;
 }
 
 static int nxp_c45_ts_info(struct mii_timestamper *mii_ts,
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index a5fa077650e8..d058316666ba 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -325,9 +325,13 @@ EXPORT_SYMBOL(phy_ethtool_ksettings_get);
 int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 {
 	struct mii_ioctl_data *mii_data = if_mii(ifr);
+	struct kernel_hwtstamp_config kernel_cfg;
+	struct netlink_ext_ack extack = {};
 	u16 val = mii_data->val_in;
 	bool change_autoneg = false;
+	struct hwtstamp_config cfg;
 	int prtad, devad;
+	int ret;
 
 	switch (cmd) {
 	case SIOCGMIIPHY:
@@ -411,8 +415,21 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		return 0;
 
 	case SIOCSHWTSTAMP:
-		if (phydev->mii_ts && phydev->mii_ts->hwtstamp)
-			return phydev->mii_ts->hwtstamp(phydev->mii_ts, ifr);
+		if (phydev->mii_ts && phydev->mii_ts->hwtstamp) {
+			if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
+				return -EFAULT;
+
+			hwtstamp_config_to_kernel(&kernel_cfg, &cfg);
+			ret = phydev->mii_ts->hwtstamp(phydev->mii_ts, &kernel_cfg, &extack);
+			if (ret)
+				return ret;
+
+			hwtstamp_config_from_kernel(&cfg, &kernel_cfg);
+			if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
+				return -EFAULT;
+
+			return 0;
+		}
 		fallthrough;
 
 	default:
diff --git a/drivers/ptp/ptp_ines.c b/drivers/ptp/ptp_ines.c
index ed215b458183..1d2940a78455 100644
--- a/drivers/ptp/ptp_ines.c
+++ b/drivers/ptp/ptp_ines.c
@@ -328,17 +328,15 @@ static u64 ines_find_txts(struct ines_port *port, struct sk_buff *skb)
 	return ns;
 }
 
-static int ines_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
+static int ines_hwtstamp(struct mii_timestamper *mii_ts,
+			 struct kernel_hwtstamp_config *cfg,
+			 struct netlink_ext_ack *extack)
 {
 	struct ines_port *port = container_of(mii_ts, struct ines_port, mii_ts);
 	u32 cm_one_step = 0, port_conf, ts_stat_rx, ts_stat_tx;
-	struct hwtstamp_config cfg;
 	unsigned long flags;
 
-	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
-		return -EFAULT;
-
-	switch (cfg.tx_type) {
+	switch (cfg->tx_type) {
 	case HWTSTAMP_TX_OFF:
 		ts_stat_tx = 0;
 		break;
@@ -353,7 +351,7 @@ static int ines_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 		return -ERANGE;
 	}
 
-	switch (cfg.rx_filter) {
+	switch (cfg->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		ts_stat_rx = 0;
 		break;
@@ -372,7 +370,7 @@ static int ines_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
 		ts_stat_rx = TS_ENABLE;
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
 		break;
 	default:
 		return -ERANGE;
@@ -393,7 +391,7 @@ static int ines_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
 
 	spin_unlock_irqrestore(&port->lock, flags);
 
-	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+	return 0;
 }
 
 static void ines_link_state(struct mii_timestamper *mii_ts,
diff --git a/include/linux/mii_timestamper.h b/include/linux/mii_timestamper.h
index fa940bbaf8ae..26b04f73f214 100644
--- a/include/linux/mii_timestamper.h
+++ b/include/linux/mii_timestamper.h
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/ethtool.h>
 #include <linux/skbuff.h>
+#include <linux/net_tstamp.h>
 
 struct phy_device;
 
@@ -51,7 +52,8 @@ struct mii_timestamper {
 			 struct sk_buff *skb, int type);
 
 	int  (*hwtstamp)(struct mii_timestamper *mii_ts,
-			 struct ifreq *ifreq);
+			 struct kernel_hwtstamp_config *kernel_config,
+			 struct netlink_ext_ack *extack);
 
 	void (*link_state)(struct mii_timestamper *mii_ts,
 			   struct phy_device *phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3cc52826f18e..e5f1f41e399c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1560,9 +1560,11 @@ static inline bool phy_has_txtstamp(struct phy_device *phydev)
 	return phydev && phydev->mii_ts && phydev->mii_ts->txtstamp;
 }
 
-static inline int phy_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
+static inline int phy_hwtstamp(struct phy_device *phydev,
+			       struct kernel_hwtstamp_config *cfg,
+			       struct netlink_ext_ack *extack)
 {
-	return phydev->mii_ts->hwtstamp(phydev->mii_ts, ifr);
+	return phydev->mii_ts->hwtstamp(phydev->mii_ts, cfg, extack);
 }
 
 static inline bool phy_rxtstamp(struct phy_device *phydev, struct sk_buff *skb,

-- 
2.25.1

[PATCH net-next v7 02/16] net: phy: Remove the call to phy_mii_ioctl in phy_hwstamp_get/set

[Kory Maincent]

__phy_hwtstamp_set function were calling the phy_mii_ioctl function
which will then use the ifreq pointer to call the hwtstamp callback.
Now that ifreq has been removed from the hwstamp callback parameters
it seems more logical to not go through the phy_mii_ioctl function and pass
directly kernel_hwtstamp_config parameter to the hwtstamp callback.

Lets do the same for __phy_hwtstamp_get function and return directly
EOPNOTSUPP as SIOCGHWTSTAMP is not supported for now for the PHYs.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/phy/phy.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d058316666ba..3376e58e2b88 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -486,7 +486,7 @@ int __phy_hwtstamp_get(struct phy_device *phydev,
 	if (!phydev)
 		return -ENODEV;
 
-	return phy_mii_ioctl(phydev, config->ifr, SIOCGHWTSTAMP);
+	return -EOPNOTSUPP;
 }
 
 /**
@@ -503,7 +503,10 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
 	if (!phydev)
 		return -ENODEV;
 
-	return phy_mii_ioctl(phydev, config->ifr, SIOCSHWTSTAMP);
+	if (phydev->mii_ts && phydev->mii_ts->hwtstamp)
+		return phydev->mii_ts->hwtstamp(phydev->mii_ts, config, extack);
+
+	return -EOPNOTSUPP;
 }
 
 /**

-- 
2.25.1

[PATCH net-next v7 03/16] net: ethtool: Refactor identical get_ts_info implementations.

[Köry Maincent]

From: Richard Cochran <richardcochran@gmail.com>

The vlan, macvlan and the bonding drivers call their "real" device driver
in order to report the time stamping capabilities.  Provide a core
ethtool helper function to avoid copy/paste in the stack.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Jay Vosburgh <jay.vosburgh@canonical.com>
---

Change in v5:
- Fixe typo.

Change in v6:
- Removed unused variable spotted by kernel test robot.
---
 drivers/net/bonding/bond_main.c | 29 ++---------------------------
 drivers/net/macvlan.c           | 14 +-------------
 include/linux/ethtool.h         |  8 ++++++++
 net/8021q/vlan_dev.c            | 15 +--------------
 net/ethtool/common.c            |  6 ++++++
 5 files changed, 18 insertions(+), 54 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 51d47eda1c87..10181c2c63fc 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5749,10 +5749,8 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct ethtool_ts_info ts_info;
-	const struct ethtool_ops *ops;
 	struct net_device *real_dev;
 	bool sw_tx_support = false;
-	struct phy_device *phydev;
 	struct list_head *iter;
 	struct slave *slave;
 	int ret = 0;
@@ -5763,29 +5761,12 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 	rcu_read_unlock();
 
 	if (real_dev) {
-		ops = real_dev->ethtool_ops;
-		phydev = real_dev->phydev;
-
-		if (phy_has_tsinfo(phydev)) {
-			ret = phy_ts_info(phydev, info);
-			goto out;
-		} else if (ops->get_ts_info) {
-			ret = ops->get_ts_info(real_dev, info);
-			goto out;
-		}
+		ret = ethtool_get_ts_info_by_layer(real_dev, info);
 	} else {
 		/* Check if all slaves support software tx timestamping */
 		rcu_read_lock();
 		bond_for_each_slave_rcu(bond, slave, iter) {
-			ret = -1;
-			ops = slave->dev->ethtool_ops;
-			phydev = slave->dev->phydev;
-
-			if (phy_has_tsinfo(phydev))
-				ret = phy_ts_info(phydev, &ts_info);
-			else if (ops->get_ts_info)
-				ret = ops->get_ts_info(slave->dev, &ts_info);
-
+			ret = ethtool_get_ts_info_by_layer(slave->dev, &ts_info);
 			if (!ret && (ts_info.so_timestamping & SOF_TIMESTAMPING_TX_SOFTWARE)) {
 				sw_tx_support = true;
 				continue;
@@ -5797,15 +5778,9 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 		rcu_read_unlock();
 	}
 
-	ret = 0;
-	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
-				SOF_TIMESTAMPING_SOFTWARE;
 	if (sw_tx_support)
 		info->so_timestamping |= SOF_TIMESTAMPING_TX_SOFTWARE;
 
-	info->phc_index = -1;
-
-out:
 	dev_put(real_dev);
 	return ret;
 }
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 02bd201bc7e5..759406fbaea8 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1086,20 +1086,8 @@ static int macvlan_ethtool_get_ts_info(struct net_device *dev,
 				       struct ethtool_ts_info *info)
 {
 	struct net_device *real_dev = macvlan_dev_real_dev(dev);
-	const struct ethtool_ops *ops = real_dev->ethtool_ops;
-	struct phy_device *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);
-	} else {
-		info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
-			SOF_TIMESTAMPING_SOFTWARE;
-		info->phc_index = -1;
-	}
-
-	return 0;
+	return ethtool_get_ts_info_by_layer(real_dev, info);
 }
 
 static netdev_features_t macvlan_fix_features(struct net_device *dev,
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 689028257fcc..c2bb74143eda 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1043,6 +1043,14 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
 	return -EINVAL;
 }
 
+/**
+ * ethtool_get_ts_info_by_layer - Obtains time stamping capabilities from the MAC or PHY layer.
+ * @dev: pointer to net_device structure
+ * @info: buffer to hold the result
+ * Returns zero on success, non-zero otherwise.
+ */
+int ethtool_get_ts_info_by_layer(struct net_device *dev, struct ethtool_ts_info *info);
+
 /**
  * ethtool_sprintf - Write formatted string to ethtool string data
  * @data: Pointer to a pointer to the start of string to update
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 2a7f1b15714a..407b2335f091 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -702,20 +702,7 @@ static int vlan_ethtool_get_ts_info(struct net_device *dev,
 				    struct ethtool_ts_info *info)
 {
 	const struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
-	const struct ethtool_ops *ops = vlan->real_dev->ethtool_ops;
-	struct phy_device *phydev = vlan->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(vlan->real_dev, info);
-	} else {
-		info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
-			SOF_TIMESTAMPING_SOFTWARE;
-		info->phc_index = -1;
-	}
-
-	return 0;
+	return ethtool_get_ts_info_by_layer(vlan->real_dev, info);
 }
 
 static void vlan_dev_get_stats64(struct net_device *dev,
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index b4419fb6df6a..11d8797f63f6 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -661,6 +661,12 @@ int ethtool_get_phc_vclocks(struct net_device *dev, int **vclock_index)
 }
 EXPORT_SYMBOL(ethtool_get_phc_vclocks);
 
+int ethtool_get_ts_info_by_layer(struct net_device *dev, struct ethtool_ts_info *info)
+{
+	return __ethtool_get_ts_info(dev, info);
+}
+EXPORT_SYMBOL(ethtool_get_ts_info_by_layer);
+
 const struct ethtool_phy_ops *ethtool_phy_ops;
 
 void ethtool_set_ethtool_phy_ops(const struct ethtool_phy_ops *ops)

-- 
2.25.1

[PATCH net-next v7 04/16] net: macb: Convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

[Kory Maincent]

The hardware timestamping through ndo_eth_ioctl() is going away.
Convert the macb driver to the new API before that can be removed.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/cadence/macb.h      | 15 ++++++++----
 drivers/net/ethernet/cadence/macb_main.c | 42 +++++++++++++++++++++++++-------
 drivers/net/ethernet/cadence/macb_ptp.c  | 28 ++++++++-------------
 3 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 78c972bb1d96..aa5700ac9c00 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1165,9 +1165,10 @@ struct macb_ptp_info {
 	int (*get_ts_info)(struct net_device *dev,
 			   struct ethtool_ts_info *info);
 	int (*get_hwtst)(struct net_device *netdev,
-			 struct ifreq *ifr);
+			 struct kernel_hwtstamp_config *tstamp_config);
 	int (*set_hwtst)(struct net_device *netdev,
-			 struct ifreq *ifr, int cmd);
+			 struct kernel_hwtstamp_config *tstamp_config,
+			 struct netlink_ext_ack *extack);
 };
 
 struct macb_pm_data {
@@ -1314,7 +1315,7 @@ struct macb {
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_clock_info;
 	struct tsu_incr tsu_incr;
-	struct hwtstamp_config tstamp_config;
+	struct kernel_hwtstamp_config tstamp_config;
 
 	/* RX queue filer rule set*/
 	struct ethtool_rx_fs_list rx_fs_list;
@@ -1363,8 +1364,12 @@ static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, stru
 
 	gem_ptp_rxstamp(bp, skb, desc);
 }
-int gem_get_hwtst(struct net_device *dev, struct ifreq *rq);
-int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd);
+
+int gem_get_hwtst(struct net_device *dev,
+		  struct kernel_hwtstamp_config *tstamp_config);
+int gem_set_hwtst(struct net_device *dev,
+		  struct kernel_hwtstamp_config *tstamp_config,
+		  struct netlink_ext_ack *extack);
 #else
 static inline void gem_ptp_init(struct net_device *ndev) { }
 static inline void gem_ptp_remove(struct net_device *ndev) { }
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index cebae0f418f2..898debfd4db3 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3773,18 +3773,38 @@ static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	if (!netif_running(dev))
 		return -EINVAL;
 
-	if (bp->ptp_info) {
-		switch (cmd) {
-		case SIOCSHWTSTAMP:
-			return bp->ptp_info->set_hwtst(dev, rq, cmd);
-		case SIOCGHWTSTAMP:
-			return bp->ptp_info->get_hwtst(dev, rq);
-		}
-	}
-
 	return phylink_mii_ioctl(bp->phylink, rq, cmd);
 }
 
+static int macb_hwtstamp_get(struct net_device *dev,
+			     struct kernel_hwtstamp_config *cfg)
+{
+	struct macb *bp = netdev_priv(dev);
+
+	if (!netif_running(dev))
+		return -EINVAL;
+
+	if (!bp->ptp_info)
+		return -EOPNOTSUPP;
+
+	return bp->ptp_info->get_hwtst(dev, cfg);
+}
+
+static int macb_hwtstamp_set(struct net_device *dev,
+			     struct kernel_hwtstamp_config *cfg,
+			     struct netlink_ext_ack *extack)
+{
+	struct macb *bp = netdev_priv(dev);
+
+	if (!netif_running(dev))
+		return -EINVAL;
+
+	if (!bp->ptp_info)
+		return -EOPNOTSUPP;
+
+	return bp->ptp_info->set_hwtst(dev, cfg, extack);
+}
+
 static inline void macb_set_txcsum_feature(struct macb *bp,
 					   netdev_features_t features)
 {
@@ -3884,6 +3904,8 @@ static const struct net_device_ops macb_netdev_ops = {
 #endif
 	.ndo_set_features	= macb_set_features,
 	.ndo_features_check	= macb_features_check,
+	.ndo_hwtstamp_set	= macb_hwtstamp_set,
+	.ndo_hwtstamp_get	= macb_hwtstamp_get,
 };
 
 /* Configure peripheral capabilities according to device tree
@@ -4539,6 +4561,8 @@ static const struct net_device_ops at91ether_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= at91ether_poll_controller,
 #endif
+	.ndo_hwtstamp_set	= macb_hwtstamp_set,
+	.ndo_hwtstamp_get	= macb_hwtstamp_get,
 };
 
 static int at91ether_clk_init(struct platform_device *pdev, struct clk **pclk,
diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index 51d26fa190d7..a63bf29c4fa8 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -374,19 +374,16 @@ static int gem_ptp_set_ts_mode(struct macb *bp,
 	return 0;
 }
 
-int gem_get_hwtst(struct net_device *dev, struct ifreq *rq)
+int gem_get_hwtst(struct net_device *dev,
+		  struct kernel_hwtstamp_config *tstamp_config)
 {
-	struct hwtstamp_config *tstamp_config;
 	struct macb *bp = netdev_priv(dev);
 
-	tstamp_config = &bp->tstamp_config;
+	*tstamp_config = bp->tstamp_config;
 	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
 		return -EOPNOTSUPP;
 
-	if (copy_to_user(rq->ifr_data, tstamp_config, sizeof(*tstamp_config)))
-		return -EFAULT;
-	else
-		return 0;
+	return 0;
 }
 
 static void gem_ptp_set_one_step_sync(struct macb *bp, u8 enable)
@@ -401,22 +398,18 @@ static void gem_ptp_set_one_step_sync(struct macb *bp, u8 enable)
 		macb_writel(bp, NCR, reg_val & ~MACB_BIT(OSSMODE));
 }
 
-int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd)
+int gem_set_hwtst(struct net_device *dev,
+		  struct kernel_hwtstamp_config *tstamp_config,
+		  struct netlink_ext_ack *extack)
 {
 	enum macb_bd_control tx_bd_control = TSTAMP_DISABLED;
 	enum macb_bd_control rx_bd_control = TSTAMP_DISABLED;
-	struct hwtstamp_config *tstamp_config;
 	struct macb *bp = netdev_priv(dev);
 	u32 regval;
 
-	tstamp_config = &bp->tstamp_config;
 	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
 		return -EOPNOTSUPP;
 
-	if (copy_from_user(tstamp_config, ifr->ifr_data,
-			   sizeof(*tstamp_config)))
-		return -EFAULT;
-
 	switch (tstamp_config->tx_type) {
 	case HWTSTAMP_TX_OFF:
 		break;
@@ -463,12 +456,11 @@ int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd)
 		return -ERANGE;
 	}
 
+	bp->tstamp_config = *tstamp_config;
+
 	if (gem_ptp_set_ts_mode(bp, tx_bd_control, rx_bd_control) != 0)
 		return -ERANGE;
 
-	if (copy_to_user(ifr->ifr_data, tstamp_config, sizeof(*tstamp_config)))
-		return -EFAULT;
-	else
-		return 0;
+	return 0;
 }
 

-- 
2.25.1

[PATCH net-next v7 05/16] net: Make dev_set_hwtstamp_phylib accessible

[Kory Maincent]

Make the dev_set_hwtstamp_phylib function accessible in prevision to use
it from ethtool to reset the tstamp current configuration.

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Change in v6:
- Add missing EXPORT_SYMBOL_GPL call.
---
 include/linux/netdevice.h | 3 +++
 net/core/dev_ioctl.c      | 7 ++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a16c9cc063fe..2d840d7056f2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3942,6 +3942,9 @@ int generic_hwtstamp_get_lower(struct net_device *dev,
 int generic_hwtstamp_set_lower(struct net_device *dev,
 			       struct kernel_hwtstamp_config *kernel_cfg,
 			       struct netlink_ext_ack *extack);
+int dev_set_hwtstamp_phylib(struct net_device *dev,
+			    struct kernel_hwtstamp_config *cfg,
+			    struct netlink_ext_ack *extack);
 int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *userdata);
 unsigned int dev_get_flags(const struct net_device *);
 int __dev_change_flags(struct net_device *dev, unsigned int flags,
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index feeddf95f450..9a66cf5015f2 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -322,9 +322,9 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
  * frames and not forward them), it must set IFF_SEE_ALL_HWTSTAMP_REQUESTS in
  * dev->priv_flags.
  */
-static int dev_set_hwtstamp_phylib(struct net_device *dev,
-				   struct kernel_hwtstamp_config *cfg,
-				   struct netlink_ext_ack *extack)
+int dev_set_hwtstamp_phylib(struct net_device *dev,
+			    struct kernel_hwtstamp_config *cfg,
+			    struct netlink_ext_ack *extack)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 	bool phy_ts = phy_has_hwtstamp(dev->phydev);
@@ -363,6 +363,7 @@ static int dev_set_hwtstamp_phylib(struct net_device *dev,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(dev_set_hwtstamp_phylib);
 
 static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 {

-- 
2.25.1

[PATCH net-next v7 06/16] net: phy: micrel: fix ts_info value in case of no phc

[Kory Maincent]

In case of no phc we should not return SOFTWARE TIMESTAMPING flags as we do
not know whether the netdev supports of timestamping.
Remove it from the lan8841_ts_info and simply return 0.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

This patch is not tested but it seems consistent to me.

Changes in v6:
- Update the commit message.
---
 drivers/net/phy/micrel.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 99af1e500c6c..bd4cd082662f 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -3629,12 +3629,8 @@ static int lan8841_ts_info(struct mii_timestamper *mii_ts,
 
 	info->phc_index = ptp_priv->ptp_clock ?
 				ptp_clock_index(ptp_priv->ptp_clock) : -1;
-	if (info->phc_index == -1) {
-		info->so_timestamping |= SOF_TIMESTAMPING_TX_SOFTWARE |
-					 SOF_TIMESTAMPING_RX_SOFTWARE |
-					 SOF_TIMESTAMPING_SOFTWARE;
+	if (info->phc_index == -1)
 		return 0;
-	}
 
 	info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
 				SOF_TIMESTAMPING_RX_HARDWARE |

-- 
2.25.1

[PATCH net-next v7 07/16] net_tstamp: Add TIMESTAMPING SOFTWARE and HARDWARE mask

[Kory Maincent]

Timestamping software or hardware flags are often used as a group,
therefore adding these masks will easier future use.

I did not use SOF_TIMESTAMPING_SYS_HARDWARE flag as it is deprecated and
not use at all.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 include/uapi/linux/net_tstamp.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index a2c66b3d7f0f..df8091998c8d 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -48,6 +48,14 @@ enum {
 					 SOF_TIMESTAMPING_TX_SCHED | \
 					 SOF_TIMESTAMPING_TX_ACK)
 
+#define SOF_TIMESTAMPING_SOFTWARE_MASK	(SOF_TIMESTAMPING_RX_SOFTWARE | \
+					 SOF_TIMESTAMPING_TX_SOFTWARE | \
+					 SOF_TIMESTAMPING_SOFTWARE)
+
+#define SOF_TIMESTAMPING_HARDWARE_MASK	(SOF_TIMESTAMPING_RX_HARDWARE | \
+					 SOF_TIMESTAMPING_TX_HARDWARE | \
+					 SOF_TIMESTAMPING_RAW_HARDWARE)
+
 /**
  * struct so_timestamping - SO_TIMESTAMPING parameter
  *

-- 
2.25.1

[PATCH net-next v7 08/16] net: ethtool: Add a command to expose current time stamping layer

[Kory Maincent]

Time stamping on network packets may happen either in the MAC or in
the PHY, but not both.  In preparation for making the choice
selectable, expose both the current layers via ethtool.

In accordance with the kernel implementation as it stands, the current
layer will always read as "phy" when a PHY time stamping device is
present. Future patches will allow changing the current layer
administratively.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

---
Changes in v2:
- Move the introduction of selected_timestamping_layer variable in next
  patch.

Changes in v3:
- Move on to ethtool instead of syfs

Changes in v4:
- Move on to netlink ethtool instead of ioctl. I am not familiar with
  netlink so there might be some code that does not follow the good code
  practice.

Changes in v5:
- Rename timestamping layers.
- Set a default value of ts_layer in __ethtool_get_ts_info function.
- Separate TS_GET and TS_LIST_GET ethtool command in two separate patches.
- Update documentation.

Changes in v6:
- Moved back to a timestamping_layer simple enum.
- Moved back to MAC/PHY_TIMESTAMING namings.
- Add SOFTWARE timestamping description.
---
 Documentation/networking/ethtool-netlink.rst | 23 ++++++++
 include/uapi/linux/ethtool_netlink.h         | 14 +++++
 include/uapi/linux/net_tstamp.h              | 10 ++++
 net/ethtool/Makefile                         |  2 +-
 net/ethtool/common.h                         |  1 +
 net/ethtool/netlink.c                        | 10 ++++
 net/ethtool/netlink.h                        |  2 +
 net/ethtool/ts.c                             | 88 ++++++++++++++++++++++++++++
 8 files changed, 149 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 2540c70952ff..644b3b764044 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -225,6 +225,7 @@ Userspace to kernel:
   ``ETHTOOL_MSG_RSS_GET``               get RSS settings
   ``ETHTOOL_MSG_MM_GET``                get MAC merge layer state
   ``ETHTOOL_MSG_MM_SET``                set MAC merge layer parameters
+  ``ETHTOOL_MSG_TS_GET``                get current timestamping
   ===================================== =================================
 
 Kernel to userspace:
@@ -268,6 +269,7 @@ Kernel to userspace:
   ``ETHTOOL_MSG_PSE_GET_REPLY``            PSE parameters
   ``ETHTOOL_MSG_RSS_GET_REPLY``            RSS settings
   ``ETHTOOL_MSG_MM_GET_REPLY``             MAC merge layer status
+  ``ETHTOOL_MSG_TS_GET_REPLY``             current timestamping
   ======================================== =================================
 
 ``GET`` requests are sent by userspace applications to retrieve device
@@ -1994,6 +1996,26 @@ The attributes are propagated to the driver through the following structure:
 .. kernel-doc:: include/linux/ethtool.h
     :identifiers: ethtool_mm_cfg
 
+TS_GET
+======
+
+Gets current timestamping.
+
+Request contents:
+
+  =================================  ======  ====================
+  ``ETHTOOL_A_TS_HEADER``            nested  request header
+  =================================  ======  ====================
+
+Kernel response contents:
+
+  =======================  ======  ==============================
+  ``ETHTOOL_A_TS_HEADER``  nested  reply header
+  ``ETHTOOL_A_TS_LAYER``   u32     current timestamping
+  =======================  ======  ==============================
+
+This command get the current timestamp layer.
+
 Request translation
 ===================
 
@@ -2100,4 +2122,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_PLCA_GET_STATUS``
   n/a                                 ``ETHTOOL_MSG_MM_GET``
   n/a                                 ``ETHTOOL_MSG_MM_SET``
+  n/a                                 ``ETHTOOL_MSG_TS_GET``
   =================================== =====================================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 73e2c10dc2cc..cb51136328cf 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -57,6 +57,7 @@ enum {
 	ETHTOOL_MSG_PLCA_GET_STATUS,
 	ETHTOOL_MSG_MM_GET,
 	ETHTOOL_MSG_MM_SET,
+	ETHTOOL_MSG_TS_GET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -109,6 +110,7 @@ enum {
 	ETHTOOL_MSG_PLCA_NTF,
 	ETHTOOL_MSG_MM_GET_REPLY,
 	ETHTOOL_MSG_MM_NTF,
+	ETHTOOL_MSG_TS_GET_REPLY,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -975,6 +977,18 @@ enum {
 	ETHTOOL_A_MM_MAX = (__ETHTOOL_A_MM_CNT - 1)
 };
 
+/* TS LAYER */
+
+enum {
+	ETHTOOL_A_TS_UNSPEC,
+	ETHTOOL_A_TS_HEADER,			/* nest - _A_HEADER_* */
+	ETHTOOL_A_TS_LAYER,			/* u32 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_TS_CNT,
+	ETHTOOL_A_TS_MAX = (__ETHTOOL_A_TS_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index df8091998c8d..4551fb3d7720 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -13,6 +13,16 @@
 #include <linux/types.h>
 #include <linux/socket.h>   /* for SO_TIMESTAMPING */
 
+/* Layer of the TIMESTAMPING provider */
+enum timestamping_layer {
+	NO_TIMESTAMPING,
+	SOFTWARE_TIMESTAMPING,
+	MAC_TIMESTAMPING,
+	PHY_TIMESTAMPING,
+
+	__TIMESTAMPING_COUNT,
+};
+
 /* SO_TIMESTAMPING flags */
 enum {
 	SOF_TIMESTAMPING_TX_HARDWARE = (1<<0),
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 504f954a1b28..4ea64c080639 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,4 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
 		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o mm.o \
-		   module.o pse-pd.o plca.o mm.o
+		   module.o pse-pd.o plca.o mm.o ts.o
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index 28b8aaaf9bcb..a264b635f7d3 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -35,6 +35,7 @@ extern const char wol_mode_names[][ETH_GSTRING_LEN];
 extern const char sof_timestamping_names[][ETH_GSTRING_LEN];
 extern const char ts_tx_type_names[][ETH_GSTRING_LEN];
 extern const char ts_rx_filter_names[][ETH_GSTRING_LEN];
+extern const char ts_layer_names[][ETH_GSTRING_LEN];
 extern const char udp_tunnel_type_names[][ETH_GSTRING_LEN];
 
 int __ethtool_get_link(struct net_device *dev);
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 3bbd5afb7b31..561c0931d055 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -306,6 +306,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_PLCA_GET_STATUS]	= &ethnl_plca_status_request_ops,
 	[ETHTOOL_MSG_MM_GET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
+	[ETHTOOL_MSG_TS_GET]		= &ethnl_ts_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1128,6 +1129,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_mm_set_policy,
 		.maxattr = ARRAY_SIZE(ethnl_mm_set_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_TS_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_ts_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_ts_get_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 9a333a8d04c1..1e6085198acc 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -395,6 +395,7 @@ extern const struct ethnl_request_ops ethnl_rss_request_ops;
 extern const struct ethnl_request_ops ethnl_plca_cfg_request_ops;
 extern const struct ethnl_request_ops ethnl_plca_status_request_ops;
 extern const struct ethnl_request_ops ethnl_mm_request_ops;
+extern const struct ethnl_request_ops ethnl_ts_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -441,6 +442,7 @@ extern const struct nla_policy ethnl_plca_set_cfg_policy[ETHTOOL_A_PLCA_MAX + 1]
 extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADER + 1];
 extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
+extern const struct nla_policy ethnl_ts_get_policy[ETHTOOL_A_TS_HEADER + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/ts.c b/net/ethtool/ts.c
new file mode 100644
index 000000000000..066cb06f4d0b
--- /dev/null
+++ b/net/ethtool/ts.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/net_tstamp.h>
+#include <linux/phy.h>
+
+#include "netlink.h"
+#include "common.h"
+#include "bitset.h"
+
+struct ts_req_info {
+	struct ethnl_req_info		base;
+};
+
+struct ts_reply_data {
+	struct ethnl_reply_data		base;
+	enum timestamping_layer		ts_layer;
+};
+
+#define TS_REPDATA(__reply_base) \
+	container_of(__reply_base, struct ts_reply_data, base)
+
+/* TS_GET */
+const struct nla_policy ethnl_ts_get_policy[] = {
+	[ETHTOOL_A_TS_HEADER]		=
+		NLA_POLICY_NESTED(ethnl_header_policy),
+};
+
+static int ts_prepare_data(const struct ethnl_req_info *req_base,
+			   struct ethnl_reply_data *reply_base,
+			   const struct genl_info *info)
+{
+	struct ts_reply_data *data = TS_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	int ret;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	if (phy_has_tsinfo(dev->phydev)) {
+		data->ts_layer = PHY_TIMESTAMPING;
+	} else if (ops->get_ts_info) {
+		struct ethtool_ts_info ts_info = {0};
+
+		ops->get_ts_info(dev, &ts_info);
+		if (ts_info.so_timestamping &
+		    SOF_TIMESTAMPING_HARDWARE_MASK)
+			data->ts_layer = MAC_TIMESTAMPING;
+
+		if (ts_info.so_timestamping &
+		    SOF_TIMESTAMPING_SOFTWARE_MASK)
+			data->ts_layer = SOFTWARE_TIMESTAMPING;
+	} else {
+		data->ts_layer = NO_TIMESTAMPING;
+	}
+
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+static int ts_reply_size(const struct ethnl_req_info *req_base,
+			 const struct ethnl_reply_data *reply_base)
+{
+	return nla_total_size(sizeof(u32));
+}
+
+static int ts_fill_reply(struct sk_buff *skb,
+			 const struct ethnl_req_info *req_base,
+			 const struct ethnl_reply_data *reply_base)
+{
+	struct ts_reply_data *data = TS_REPDATA(reply_base);
+
+	return nla_put_u32(skb, ETHTOOL_A_TS_LAYER, data->ts_layer);
+}
+
+const struct ethnl_request_ops ethnl_ts_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_TS_GET,
+	.reply_cmd		= ETHTOOL_MSG_TS_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_TS_HEADER,
+	.req_info_size		= sizeof(struct ts_req_info),
+	.reply_data_size	= sizeof(struct ts_reply_data),
+
+	.prepare_data		= ts_prepare_data,
+	.reply_size		= ts_reply_size,
+	.fill_reply		= ts_fill_reply,
+};

-- 
2.25.1

[PATCH net-next v7 09/16] netlink: specs: Introduce new netlink command to get current timestamp

[Kory Maincent]

Add a new commands allowing to get the current time stamping on a
netdevice's link.

Example usage :
./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema --do ts-get \
	     --json '{"header":{"dev-name":"eth0"}}'
{'header': {'dev-index': 3, 'dev-name': 'eth0'}, 'ts-layer': 1}

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 5c7a65b009b4..61b30701fdad 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -939,6 +939,16 @@ attribute-sets:
       -
         name: burst-tmr
         type: u32
+  -
+    name: ts
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: ts-layer
+        type: u32
 
 operations:
   enum-model: directional
@@ -1689,3 +1699,17 @@ operations:
       name: mm-ntf
       doc: Notification for change in MAC Merge configuration.
       notify: mm-get
+    -
+      name: ts-get
+      doc: Get current timestamp
+
+      attribute-set: ts
+
+      do:
+        request:
+          attributes:
+            - header
+        reply:
+          attributes: &ts
+            - header
+            - ts-layer

-- 
2.25.1

[PATCH net-next v7 10/16] net: ethtool: Add a command to list available time stamping layers

[Kory Maincent]

Introduce a new netlink message that lists all available time stamping
layers on a given interface.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Changes in v6:
- Fix Documentation typos.
- Follow timestamping layer naming update.
---
 Documentation/networking/ethtool-netlink.rst | 23 +++++++++
 include/uapi/linux/ethtool_netlink.h         | 14 ++++++
 net/ethtool/netlink.c                        | 10 ++++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/ts.c                             | 73 ++++++++++++++++++++++++++++
 5 files changed, 121 insertions(+)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 644b3b764044..b8d00676ed82 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -226,6 +226,7 @@ Userspace to kernel:
   ``ETHTOOL_MSG_MM_GET``                get MAC merge layer state
   ``ETHTOOL_MSG_MM_SET``                set MAC merge layer parameters
   ``ETHTOOL_MSG_TS_GET``                get current timestamping
+  ``ETHTOOL_MSG_TS_LIST_GET``           list available timestampings
   ===================================== =================================
 
 Kernel to userspace:
@@ -270,6 +271,7 @@ Kernel to userspace:
   ``ETHTOOL_MSG_RSS_GET_REPLY``            RSS settings
   ``ETHTOOL_MSG_MM_GET_REPLY``             MAC merge layer status
   ``ETHTOOL_MSG_TS_GET_REPLY``             current timestamping
+  ``ETHTOOL_MSG_TS_LIST_GET_REPLY``        available timestampings
   ======================================== =================================
 
 ``GET`` requests are sent by userspace applications to retrieve device
@@ -2016,6 +2018,26 @@ Kernel response contents:
 
 This command get the current timestamp layer.
 
+TS_LIST_GET
+===========
+
+Get the list of available timestampings.
+
+Request contents:
+
+  =================================  ======  ====================
+  ``ETHTOOL_A_TS_HEADER``            nested  request header
+  =================================  ======  ====================
+
+Kernel response contents:
+
+  ===========================  ======  ==============================
+  ``ETHTOOL_A_TS_HEADER``      nested  reply header
+  ``ETHTOOL_A_TS_LIST_LAYER``  binary  available timestampings
+  ===========================  ======  ==============================
+
+This command lists all the possible timestamp layer available.
+
 Request translation
 ===================
 
@@ -2123,4 +2145,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_MM_GET``
   n/a                                 ``ETHTOOL_MSG_MM_SET``
   n/a                                 ``ETHTOOL_MSG_TS_GET``
+  n/a                                 ``ETHTOOL_MSG_TS_LIST_GET``
   =================================== =====================================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index cb51136328cf..62b885d44d06 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -58,6 +58,7 @@ enum {
 	ETHTOOL_MSG_MM_GET,
 	ETHTOOL_MSG_MM_SET,
 	ETHTOOL_MSG_TS_GET,
+	ETHTOOL_MSG_TS_LIST_GET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -111,6 +112,7 @@ enum {
 	ETHTOOL_MSG_MM_GET_REPLY,
 	ETHTOOL_MSG_MM_NTF,
 	ETHTOOL_MSG_TS_GET_REPLY,
+	ETHTOOL_MSG_TS_LIST_GET_REPLY,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -989,6 +991,18 @@ enum {
 	ETHTOOL_A_TS_MAX = (__ETHTOOL_A_TS_CNT - 1)
 };
 
+/* TS LIST LAYER */
+
+enum {
+	ETHTOOL_A_TS_LIST_UNSPEC,
+	ETHTOOL_A_TS_LIST_HEADER,			/* nest - _A_HEADER_* */
+	ETHTOOL_A_TS_LIST_LAYER,		/* array, u32 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_TS_LIST_CNT,
+	ETHTOOL_A_TS_LIST_MAX = (__ETHTOOL_A_TS_LIST_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 561c0931d055..842c9db1531f 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -307,6 +307,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_MM_GET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_TS_GET]		= &ethnl_ts_request_ops,
+	[ETHTOOL_MSG_TS_LIST_GET]	= &ethnl_ts_list_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1138,6 +1139,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_ts_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_ts_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_TS_LIST_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_ts_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_ts_get_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 1e6085198acc..ea8c312db3af 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -396,6 +396,7 @@ extern const struct ethnl_request_ops ethnl_plca_cfg_request_ops;
 extern const struct ethnl_request_ops ethnl_plca_status_request_ops;
 extern const struct ethnl_request_ops ethnl_mm_request_ops;
 extern const struct ethnl_request_ops ethnl_ts_request_ops;
+extern const struct ethnl_request_ops ethnl_ts_list_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
diff --git a/net/ethtool/ts.c b/net/ethtool/ts.c
index 066cb06f4d0b..f2dd65a2e69c 100644
--- a/net/ethtool/ts.c
+++ b/net/ethtool/ts.c
@@ -86,3 +86,76 @@ const struct ethnl_request_ops ethnl_ts_request_ops = {
 	.reply_size		= ts_reply_size,
 	.fill_reply		= ts_fill_reply,
 };
+
+/* TS_LIST_GET */
+struct ts_list_reply_data {
+	struct ethnl_reply_data		base;
+	enum timestamping_layer		ts_layer[__TIMESTAMPING_COUNT];
+	u8				num_ts;
+};
+
+#define TS_LIST_REPDATA(__reply_base) \
+	container_of(__reply_base, struct ts_list_reply_data, base)
+
+static int ts_list_prepare_data(const struct ethnl_req_info *req_base,
+				struct ethnl_reply_data *reply_base,
+				const struct genl_info *info)
+{
+	struct ts_list_reply_data *data = TS_LIST_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	int ret, i = 0;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	if (phy_has_tsinfo(dev->phydev))
+		data->ts_layer[i++] = PHY_TIMESTAMPING;
+	if (ops->get_ts_info) {
+		struct ethtool_ts_info ts_info = {0};
+
+		ops->get_ts_info(dev, &ts_info);
+		if (ts_info.so_timestamping &
+		    SOF_TIMESTAMPING_HARDWARE_MASK)
+			data->ts_layer[i++] = MAC_TIMESTAMPING;
+
+		if (ts_info.so_timestamping &
+		    SOF_TIMESTAMPING_SOFTWARE_MASK)
+			data->ts_layer[i++] = SOFTWARE_TIMESTAMPING;
+	}
+
+	data->num_ts = i;
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+static int ts_list_reply_size(const struct ethnl_req_info *req_base,
+			      const struct ethnl_reply_data *reply_base)
+{
+	struct ts_list_reply_data *data = TS_LIST_REPDATA(reply_base);
+
+	return nla_total_size(sizeof(u32)) * data->num_ts;
+}
+
+static int ts_list_fill_reply(struct sk_buff *skb,
+			      const struct ethnl_req_info *req_base,
+			 const struct ethnl_reply_data *reply_base)
+{
+	struct ts_list_reply_data *data = TS_LIST_REPDATA(reply_base);
+
+	return nla_put(skb, ETHTOOL_A_TS_LIST_LAYER, sizeof(u32) * data->num_ts, data->ts_layer);
+}
+
+const struct ethnl_request_ops ethnl_ts_list_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_TS_LIST_GET,
+	.reply_cmd		= ETHTOOL_MSG_TS_LIST_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_TS_HEADER,
+	.req_info_size		= sizeof(struct ts_req_info),
+	.reply_data_size	= sizeof(struct ts_list_reply_data),
+
+	.prepare_data		= ts_list_prepare_data,
+	.reply_size		= ts_list_reply_size,
+	.fill_reply		= ts_list_fill_reply,
+};

-- 
2.25.1

[PATCH net-next v7 11/16] netlink: specs: Introduce new netlink command to list available time stamping layers

[Kory Maincent]

Add a new commands allowing to list available time stamping layers on a
netdevice's link.

Example usage :
./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema \
	     --do ts-list-get \
	     --json '{"header":{"dev-name":"eth0"}}'
{'header': {'dev-index': 3, 'dev-name': 'eth0'},
 'ts-list-layer': b'\x01\x00\x00\x00\x05\x00\x00\x00'}

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 61b30701fdad..591425f7fa76 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -949,6 +949,16 @@ attribute-sets:
       -
         name: ts-layer
         type: u32
+  -
+    name: ts-list
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: ts-list-layer
+        type: binary
 
 operations:
   enum-model: directional
@@ -1713,3 +1723,17 @@ operations:
           attributes: &ts
             - header
             - ts-layer
+    -
+      name: ts-list-get
+      doc: Get list of timestamp devices available on an interface
+
+      attribute-set: ts-list
+
+      do:
+        request:
+          attributes:
+            - header
+        reply:
+          attributes:
+            - header
+            - ts-list-layer

-- 
2.25.1

[PATCH net-next v7 12/16] net: Replace hwtstamp_source by timestamping layer

[Kory Maincent]

Replace hwtstamp_source which is only used by the kernel_hwtstamp_config
structure by the more widely use timestamp_layer structure. This is done
to prepare the support of selectable timestamping source.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Change in v6:
- Follow timestamping layer naming update.

Change in v7:
- Fix a temporary build error of undeclared ts_layer enum value.
---
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c |  6 +++---
 include/linux/net_tstamp.h                            | 11 +++--------
 net/core/dev_ioctl.c                                  |  2 +-
 3 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 2635ef8958c8..fbe56b1bb386 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -470,15 +470,15 @@ static int lan966x_port_hwtstamp_set(struct net_device *dev,
 	struct lan966x_port *port = netdev_priv(dev);
 	int err;
 
-	if (cfg->source != HWTSTAMP_SOURCE_NETDEV &&
-	    cfg->source != HWTSTAMP_SOURCE_PHYLIB)
+	if (cfg->source != MAC_TIMESTAMPING &&
+	    cfg->source != PHY_TIMESTAMPING)
 		return -EOPNOTSUPP;
 
 	err = lan966x_ptp_setup_traps(port, cfg);
 	if (err)
 		return err;
 
-	if (cfg->source == HWTSTAMP_SOURCE_NETDEV) {
+	if (cfg->source == MAC_TIMESTAMPING) {
 		if (!port->lan966x->ptp)
 			return -EOPNOTSUPP;
 
diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
index eb01c37e71e0..bb289c2ad376 100644
--- a/include/linux/net_tstamp.h
+++ b/include/linux/net_tstamp.h
@@ -5,11 +5,6 @@
 
 #include <uapi/linux/net_tstamp.h>
 
-enum hwtstamp_source {
-	HWTSTAMP_SOURCE_NETDEV,
-	HWTSTAMP_SOURCE_PHYLIB,
-};
-
 /**
  * struct kernel_hwtstamp_config - Kernel copy of struct hwtstamp_config
  *
@@ -20,8 +15,8 @@ enum hwtstamp_source {
  *	a legacy implementation of a lower driver
  * @copied_to_user: request was passed to a legacy implementation which already
  *	copied the ioctl request back to user space
- * @source: indication whether timestamps should come from the netdev or from
- *	an attached phylib PHY
+ * @source: indication whether timestamps should come from software, the netdev
+ *	or from an attached phylib PHY
  *
  * Prefer using this structure for in-kernel processing of hardware
  * timestamping configuration, over the inextensible struct hwtstamp_config
@@ -33,7 +28,7 @@ struct kernel_hwtstamp_config {
 	int rx_filter;
 	struct ifreq *ifr;
 	bool copied_to_user;
-	enum hwtstamp_source source;
+	enum timestamping_layer source;
 };
 
 static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kernel_cfg,
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 9a66cf5015f2..267cd00269d0 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -332,7 +332,7 @@ int dev_set_hwtstamp_phylib(struct net_device *dev,
 	bool changed = false;
 	int err;
 
-	cfg->source = phy_ts ? HWTSTAMP_SOURCE_PHYLIB : HWTSTAMP_SOURCE_NETDEV;
+	cfg->source = phy_ts ? PHY_TIMESTAMPING : MAC_TIMESTAMPING;
 
 	if (phy_ts && (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) {
 		err = ops->ndo_hwtstamp_get(dev, &old_cfg);

-- 
2.25.1

[PATCH net-next v7 13/16] net: Change the API of PHY default timestamp to MAC

[Kory Maincent]

Change the API to select MAC default time stamping instead of the PHY.
Indeed the PHY is closer to the wire therefore theoretically it has 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 devices are not very widespread. For not breaking the compatibility I
introduce a default_timestamp flag in phy_device that is set by the phy
driver to know we are using 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.

Change in v6:
- Replace the allowlist phy with a default_timestamp flag to know which
  phy is using old API behavior.
- Fix dereferenced of a possible null pointer.
- Follow timestamping layer naming update.
- Update timestamp default set between MAC and software.
- Update ts_info returned in case of software timestamping.
---
 drivers/net/phy/bcm-phy-ptp.c     |  3 +++
 drivers/net/phy/dp83640.c         |  3 +++
 drivers/net/phy/micrel.c          |  6 ++++++
 drivers/net/phy/mscc/mscc_ptp.c   |  2 ++
 drivers/net/phy/nxp-c45-tja11xx.c |  3 +++
 drivers/net/phy/phy_device.c      | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/netdevice.h         |  5 +++++
 include/linux/phy.h               |  4 ++++
 net/core/dev.c                    |  3 +++
 net/core/dev_ioctl.c              | 36 ++++++++++++++++++++++--------------
 net/core/timestamping.c           | 10 ++++++++++
 net/ethtool/common.c              | 19 +++++++++++++++++--
 12 files changed, 115 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/bcm-phy-ptp.c b/drivers/net/phy/bcm-phy-ptp.c
index 617d384d4551..d3e825c951ee 100644
--- a/drivers/net/phy/bcm-phy-ptp.c
+++ b/drivers/net/phy/bcm-phy-ptp.c
@@ -931,6 +931,9 @@ struct bcm_ptp_private *bcm_ptp_probe(struct phy_device *phydev)
 		return ERR_CAST(clock);
 	priv->ptp_clock = clock;
 
+	/* Timestamp selected by default to keep legacy API */
+	phydev->default_timestamp = true;
+
 	priv->phydev = phydev;
 	bcm_ptp_init(priv);
 
diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 5c42c47dc564..64fd1a109c0f 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1450,6 +1450,9 @@ static int dp83640_probe(struct phy_device *phydev)
 	phydev->mii_ts = &dp83640->mii_ts;
 	phydev->priv = dp83640;
 
+	/* Timestamp selected by default to keep legacy API */
+	phydev->default_timestamp = true;
+
 	spin_lock_init(&dp83640->rx_lock);
 	skb_queue_head_init(&dp83640->rx_queue);
 	skb_queue_head_init(&dp83640->tx_queue);
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index bd4cd082662f..2b8dd0131926 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -3158,6 +3158,9 @@ static void lan8814_ptp_init(struct phy_device *phydev)
 	ptp_priv->mii_ts.ts_info  = lan8814_ts_info;
 
 	phydev->mii_ts = &ptp_priv->mii_ts;
+
+	/* Timestamp selected by default to keep legacy API */
+	phydev->default_timestamp = true;
 }
 
 static int lan8814_ptp_probe_once(struct phy_device *phydev)
@@ -4586,6 +4589,9 @@ static int lan8841_probe(struct phy_device *phydev)
 
 	phydev->mii_ts = &ptp_priv->mii_ts;
 
+	/* Timestamp selected by default to keep legacy API */
+	phydev->default_timestamp = true;
+
 	return 0;
 }
 
diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
index eb0b032cb613..fd174eb06d4a 100644
--- a/drivers/net/phy/mscc/mscc_ptp.c
+++ b/drivers/net/phy/mscc/mscc_ptp.c
@@ -1570,6 +1570,8 @@ int vsc8584_ptp_probe(struct phy_device *phydev)
 		return PTR_ERR(vsc8531->load_save);
 	}
 
+	/* Timestamp selected by default to keep legacy API */
+	phydev->default_timestamp = true;
 	vsc8531->ptp->phydev = phydev;
 
 	return 0;
diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
index 780ad353cf55..0515c7b979db 100644
--- a/drivers/net/phy/nxp-c45-tja11xx.c
+++ b/drivers/net/phy/nxp-c45-tja11xx.c
@@ -1658,6 +1658,9 @@ static int nxp_c45_probe(struct phy_device *phydev)
 		priv->mii_ts.ts_info = nxp_c45_ts_info;
 		phydev->mii_ts = &priv->mii_ts;
 		ret = nxp_c45_init_ptp_clock(priv);
+
+		/* Timestamp selected by default to keep legacy API */
+		phydev->default_timestamp = true;
 	} else {
 		phydev_dbg(phydev, "PTP support not enabled even if the phy supports it");
 	}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2ce74593d6e4..8c4794631daa 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1411,6 +1411,26 @@ int phy_sfp_probe(struct phy_device *phydev,
 }
 EXPORT_SYMBOL(phy_sfp_probe);
 
+/**
+ * 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 if the phydev does not have default_timestamp flag enabled.
+ */
+static void phy_set_timestamp(struct net_device *dev, struct phy_device *phydev)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (!phy_has_tsinfo(phydev))
+		return;
+
+	if (!ops->get_ts_info || phydev->default_timestamp)
+		dev->ts_layer = PHY_TIMESTAMPING;
+}
+
 /**
  * phy_attach_direct - attach a network device to a given PHY device pointer
  * @dev: network device to attach
@@ -1484,6 +1504,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;
 
@@ -1812,6 +1833,22 @@ void phy_detach(struct phy_device *phydev)
 
 	phy_suspend(phydev);
 	if (dev) {
+		const struct ethtool_ops *ops = dev->ethtool_ops;
+		struct ethtool_ts_info ts_info = {0};
+
+		if (ops->get_ts_info) {
+			ops->get_ts_info(dev, &ts_info);
+			if ((ts_info.so_timestamping &
+			    SOF_TIMESTAMPING_HARDWARE_MASK) ==
+			    SOF_TIMESTAMPING_HARDWARE_MASK)
+				dev->ts_layer = MAC_TIMESTAMPING;
+			else if ((ts_info.so_timestamping &
+				 SOF_TIMESTAMPING_SOFTWARE_MASK) ==
+				 SOF_TIMESTAMPING_SOFTWARE_MASK)
+				dev->ts_layer = SOFTWARE_TIMESTAMPING;
+		} else {
+			dev->ts_layer = NO_TIMESTAMPING;
+		}
 		phydev->attached_dev->phydev = NULL;
 		phydev->attached_dev = NULL;
 	}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2d840d7056f2..f020d2790c12 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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>
@@ -2074,6 +2075,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.
@@ -2435,6 +2438,8 @@ struct net_device {
 #if IS_ENABLED(CONFIG_DPLL)
 	struct dpll_pin		*dpll_pin;
 #endif
+
+	enum timestamping_layer	ts_layer;
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e5f1f41e399c..317def2a7843 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -604,6 +604,8 @@ struct macsec_ops;
  *                 handling shall be postponed until PHY has resumed
  * @irq_rerun: Flag indicating interrupts occurred while PHY was suspended,
  *             requiring a rerun of the interrupt handler after resume
+ * @default_timestamp: Flag indicating whether we are using the phy
+ *		       timestamp as the default one
  * @interface: enum phy_interface_t value
  * @skb: Netlink message for cable diagnostics
  * @nest: Netlink nest used for cable diagnostics
@@ -667,6 +669,8 @@ struct phy_device {
 	unsigned irq_suspended:1;
 	unsigned irq_rerun:1;
 
+	unsigned default_timestamp:1;
+
 	int rate_matching;
 
 	enum phy_state state;
diff --git a/net/core/dev.c b/net/core/dev.c
index 0d548431f3fa..784d65c059d0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10210,6 +10210,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 = MAC_TIMESTAMPING;
+
 out:
 	return ret;
 
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 267cd00269d0..bc8be9749376 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -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))
+	enum timestamping_layer ts_layer = dev->ts_layer;
+
+	if (ts_layer == PHY_TIMESTAMPING)
 		return phy_hwtstamp_get(dev->phydev, cfg);
+	else if (ts_layer == MAC_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);
+	enum timestamping_layer ts_layer = dev->ts_layer;
 	struct kernel_hwtstamp_config old_cfg = {};
 	bool changed = false;
 	int err;
 
-	cfg->source = phy_ts ? PHY_TIMESTAMPING : MAC_TIMESTAMPING;
+	cfg->source = ts_layer;
+
+	if (ts_layer != PHY_TIMESTAMPING &&
+	    ts_layer != MAC_TIMESTAMPING)
+		return -EOPNOTSUPP;
 
-	if (phy_ts && (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) {
+	if (ts_layer == PHY_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 == MAC_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 == PHY_TIMESTAMPING &&
+	    dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)
 		changed = kernel_hwtstamp_config_changed(&old_cfg, cfg);
 
-	if (phy_ts) {
+	if (ts_layer == PHY_TIMESTAMPING) {
 		err = phy_hwtstamp_set(dev->phydev, cfg, extack);
 		if (err) {
 			if (changed)
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 04840697fe79..5cf51a523fb3 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -21,6 +21,7 @@ static unsigned int classify(const struct sk_buff *skb)
 
 void skb_clone_tx_timestamp(struct sk_buff *skb)
 {
+	enum timestamping_layer ts_layer;
 	struct mii_timestamper *mii_ts;
 	struct sk_buff *clone;
 	unsigned int type;
@@ -28,6 +29,10 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
 	if (!skb->sk)
 		return;
 
+	ts_layer = skb->dev->ts_layer;
+	if (ts_layer != PHY_TIMESTAMPING)
+		return;
+
 	type = classify(skb);
 	if (type == PTP_CLASS_NONE)
 		return;
@@ -44,12 +49,17 @@ EXPORT_SYMBOL_GPL(skb_clone_tx_timestamp);
 
 bool skb_defer_rx_timestamp(struct sk_buff *skb)
 {
+	enum timestamping_layer ts_layer;
 	struct mii_timestamper *mii_ts;
 	unsigned int type;
 
 	if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts)
 		return false;
 
+	ts_layer = skb->dev->ts_layer;
+	if (ts_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 11d8797f63f6..9f6e3b2c74e2 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -633,13 +633,28 @@ 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;
+	enum timestamping_layer ts_layer;
+	int ret;
 
 	memset(info, 0, sizeof(*info));
 	info->cmd = ETHTOOL_GET_TS_INFO;
 
-	if (phy_has_tsinfo(phydev))
+	ts_layer = dev->ts_layer;
+	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;
+		info->rx_filters = 0;
+		info->tx_types = 0;
+		return 0;
+	}
+
+	if (ts_layer == PHY_TIMESTAMPING)
 		return phy_ts_info(phydev, info);
-	if (ops->get_ts_info)
+
+	if (ts_layer == MAC_TIMESTAMPING)
 		return ops->get_ts_info(dev, info);
 
 	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |

-- 
2.25.1

[PATCH net-next v7 14/16] net: ethtool: ts: Update GET_TS to reply the current selected timestamp

[Kory Maincent]

As the default selected timestamp API change we have to change also the
timestamp return by ethtool. This patch return now the current selected
timestamp.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 net/ethtool/ts.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/net/ethtool/ts.c b/net/ethtool/ts.c
index f2dd65a2e69c..bd219512b8de 100644
--- a/net/ethtool/ts.c
+++ b/net/ethtool/ts.c
@@ -31,29 +31,13 @@ static int ts_prepare_data(const struct ethnl_req_info *req_base,
 {
 	struct ts_reply_data *data = TS_REPDATA(reply_base);
 	struct net_device *dev = reply_base->dev;
-	const struct ethtool_ops *ops = dev->ethtool_ops;
 	int ret;
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		return ret;
 
-	if (phy_has_tsinfo(dev->phydev)) {
-		data->ts_layer = PHY_TIMESTAMPING;
-	} else if (ops->get_ts_info) {
-		struct ethtool_ts_info ts_info = {0};
-
-		ops->get_ts_info(dev, &ts_info);
-		if (ts_info.so_timestamping &
-		    SOF_TIMESTAMPING_HARDWARE_MASK)
-			data->ts_layer = MAC_TIMESTAMPING;
-
-		if (ts_info.so_timestamping &
-		    SOF_TIMESTAMPING_SOFTWARE_MASK)
-			data->ts_layer = SOFTWARE_TIMESTAMPING;
-	} else {
-		data->ts_layer = NO_TIMESTAMPING;
-	}
+	data->ts_layer = dev->ts_layer;
 
 	ethnl_ops_complete(dev);
 

-- 
2.25.1

[PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Kory Maincent]

Now that the current timestamp is saved in a variable lets add the
ETHTOOL_MSG_TS_SET ethtool netlink socket to make it selectable.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Changes in v2:
- Move selected_timestamping_layer introduction in this patch.
- Replace strmcmp by sysfs_streq.
- Use the PHY timestamp only if available.

Changes in v3:
- Add a devicetree binding to select the preferred timestamp
- Replace the way to select timestamp through ethtool instead of sysfs
You can test it with the ethtool source on branch feature_ptp of:
https://github.com/kmaincent/ethtool

Changes in v4:
- Change the API to select MAC default time stamping instead of the PHY.
- Add a whitelist to no break the old timestamp PHY default preferences
  for current PHY.
- Replace the ethtool ioctl by netlink.
- Add a netdev notifier to allow network device to create trap on PTP
  packets. Not tested as it need to change the lan966x driver that
  implement packet traps. I will do after the hwtstamp management change
  to NDOs.

Changes in v5:
- Remove the netdev notifier added in v4.
- Extract the default timestamp API change in another patch.

Changes in v6:
- Update the error message.
- Put ndo_hwtstamp_set check first as it is most likely what most drivers
  currently do not support.
- Follow timestamping layer naming update.
- Update the timestamp layer check.
- Update timestamp set between MAC and software.

Changes in v7:
- Fix commit title typo.
---
 Documentation/networking/ethtool-netlink.rst | 17 +++++
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/netlink.c                        |  8 +++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/ts.c                             | 99 ++++++++++++++++++++++++++++
 5 files changed, 126 insertions(+)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index b8d00676ed82..530c1775e5f4 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -227,6 +227,7 @@ Userspace to kernel:
   ``ETHTOOL_MSG_MM_SET``                set MAC merge layer parameters
   ``ETHTOOL_MSG_TS_GET``                get current timestamping
   ``ETHTOOL_MSG_TS_LIST_GET``           list available timestampings
+  ``ETHTOOL_MSG_TS_SET``                set current timestamping
   ===================================== =================================
 
 Kernel to userspace:
@@ -2038,6 +2039,21 @@ Kernel response contents:
 
 This command lists all the possible timestamp layer available.
 
+TS_SET
+======
+
+Modify the selected timestamping.
+
+Request contents:
+
+  =======================  ======  ===================
+  ``ETHTOOL_A_TS_HEADER``  nested  reply header
+  ``ETHTOOL_A_TS_LAYER``   u32     timestamping
+  =======================  ======  ===================
+
+This command set the timestamping with one that should be listed by the
+TSLIST_GET command.
+
 Request translation
 ===================
 
@@ -2146,4 +2162,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_MM_SET``
   n/a                                 ``ETHTOOL_MSG_TS_GET``
   n/a                                 ``ETHTOOL_MSG_TS_LIST_GET``
+  n/a                                 ``ETHTOOL_MSG_TS_SET``
   =================================== =====================================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 62b885d44d06..df6c4fcc62c1 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -59,6 +59,7 @@ enum {
 	ETHTOOL_MSG_MM_SET,
 	ETHTOOL_MSG_TS_GET,
 	ETHTOOL_MSG_TS_LIST_GET,
+	ETHTOOL_MSG_TS_SET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 842c9db1531f..8322bf71f80d 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -308,6 +308,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_TS_GET]		= &ethnl_ts_request_ops,
 	[ETHTOOL_MSG_TS_LIST_GET]	= &ethnl_ts_list_request_ops,
+	[ETHTOOL_MSG_TS_SET]		= &ethnl_ts_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1148,6 +1149,13 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_ts_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_ts_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_TS_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_default_set_doit,
+		.policy = ethnl_ts_set_policy,
+		.maxattr = ARRAY_SIZE(ethnl_ts_set_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index ea8c312db3af..8fedf234b824 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -444,6 +444,7 @@ extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADE
 extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
 extern const struct nla_policy ethnl_ts_get_policy[ETHTOOL_A_TS_HEADER + 1];
+extern const struct nla_policy ethnl_ts_set_policy[ETHTOOL_A_TS_MAX + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/ts.c b/net/ethtool/ts.c
index bd219512b8de..357265e74e08 100644
--- a/net/ethtool/ts.c
+++ b/net/ethtool/ts.c
@@ -59,6 +59,102 @@ static int ts_fill_reply(struct sk_buff *skb,
 	return nla_put_u32(skb, ETHTOOL_A_TS_LAYER, data->ts_layer);
 }
 
+/* TS_SET */
+const struct nla_policy ethnl_ts_set_policy[] = {
+	[ETHTOOL_A_TS_HEADER]	= NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_TS_LAYER]	= NLA_POLICY_RANGE(NLA_U32, 0,
+						   __TIMESTAMPING_COUNT - 1)
+};
+
+static int ethnl_set_ts_validate(struct ethnl_req_info *req_info,
+				 struct genl_info *info)
+{
+	struct nlattr **tb = info->attrs;
+	const struct net_device_ops *ops = req_info->dev->netdev_ops;
+
+	if (!ops->ndo_hwtstamp_set)
+		return -EOPNOTSUPP;
+
+	if (!tb[ETHTOOL_A_TS_LAYER])
+		return 0;
+
+	return 1;
+}
+
+static int ethnl_set_ts(struct ethnl_req_info *req_info, struct genl_info *info)
+{
+	struct net_device *dev = req_info->dev;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct kernel_hwtstamp_config config = {0};
+	struct nlattr **tb = info->attrs;
+	enum timestamping_layer ts_layer;
+	bool mod = false;
+	int ret;
+
+	ts_layer = dev->ts_layer;
+	ethnl_update_u32(&ts_layer, tb[ETHTOOL_A_TS_LAYER], &mod);
+
+	if (!mod)
+		return 0;
+
+	if (ts_layer == SOFTWARE_TIMESTAMPING) {
+		struct ethtool_ts_info ts_info = {0};
+
+		if (!ops->get_ts_info) {
+			NL_SET_ERR_MSG_ATTR(info->extack,
+					    tb[ETHTOOL_A_TS_LAYER],
+					    "this net device cannot support timestamping");
+			return -EINVAL;
+		}
+
+		ops->get_ts_info(dev, &ts_info);
+		if ((ts_info.so_timestamping &
+		    SOF_TIMESTAMPING_SOFTWARE_MASK) !=
+		    SOF_TIMESTAMPING_SOFTWARE_MASK) {
+			NL_SET_ERR_MSG_ATTR(info->extack,
+					    tb[ETHTOOL_A_TS_LAYER],
+					    "this net device cannot support software timestamping");
+			return -EINVAL;
+		}
+	} else if (ts_layer == MAC_TIMESTAMPING) {
+		struct ethtool_ts_info ts_info = {0};
+
+		if (!ops->get_ts_info) {
+			NL_SET_ERR_MSG_ATTR(info->extack,
+					    tb[ETHTOOL_A_TS_LAYER],
+					    "this net device cannot support timestamping");
+			return -EINVAL;
+		}
+
+		ops->get_ts_info(dev, &ts_info);
+		if ((ts_info.so_timestamping &
+		    SOF_TIMESTAMPING_HARDWARE_MASK) !=
+		    SOF_TIMESTAMPING_HARDWARE_MASK) {
+			NL_SET_ERR_MSG_ATTR(info->extack,
+					    tb[ETHTOOL_A_TS_LAYER],
+					    "this net device cannot support hardware timestamping");
+			return -EINVAL;
+		}
+	} else if (ts_layer == PHY_TIMESTAMPING && !phy_has_tsinfo(dev->phydev)) {
+		NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_TS_LAYER],
+				    "this phy device cannot support timestamping");
+		return -EINVAL;
+	}
+
+	/* Disable time stamping in the current layer. */
+	if (netif_device_present(dev) &&
+	    (dev->ts_layer == PHY_TIMESTAMPING ||
+	    dev->ts_layer == MAC_TIMESTAMPING)) {
+		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);
+		if (ret < 0)
+			return ret;
+	}
+
+	dev->ts_layer = ts_layer;
+
+	return 1;
+}
+
 const struct ethnl_request_ops ethnl_ts_request_ops = {
 	.request_cmd		= ETHTOOL_MSG_TS_GET,
 	.reply_cmd		= ETHTOOL_MSG_TS_GET_REPLY,
@@ -69,6 +165,9 @@ const struct ethnl_request_ops ethnl_ts_request_ops = {
 	.prepare_data		= ts_prepare_data,
 	.reply_size		= ts_reply_size,
 	.fill_reply		= ts_fill_reply,
+
+	.set_validate		= ethnl_set_ts_validate,
+	.set			= ethnl_set_ts,
 };
 
 /* TS_LIST_GET */

-- 
2.25.1

[PATCH net-next v7 16/16] netlink: specs: Introduce time stamping set command

[Kory Maincent]

Add a new commands allowing to set the time stamping.

Example usage :
./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema \
	     --do ts-list-get \
	     --json '{"header":{"dev-name":"eth0"}}'
{'header': {'dev-index': 3, 'dev-name': 'eth0'},
 'ts-list-layer': b'\x02\x00\x00\x00\x01\x00\x00\x00\x05\x00\x00\x00'}

./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema --do ts-set \
	     --json '{"header":{"dev-name":"eth0"}, "ts-layer":5}'
none

./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema --do ts-get \
	     --json '{"header":{"dev-name":"eth0"}}'
{'header': {'dev-index': 3, 'dev-name': 'eth0'}, 'ts-layer': 5}

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 591425f7fa76..06d9120543d3 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1737,3 +1737,12 @@ operations:
           attributes:
             - header
             - ts-list-layer
+    -
+      name: ts-set
+      doc: Set the timestamp device
+
+      attribute-set: ts
+
+      do:
+        request:
+          attributes: *ts

-- 
2.25.1

Re: [PATCH net-next v7 07/16] net_tstamp: Add TIMESTAMPING SOFTWARE and HARDWARE mask

[Willem de Bruijn]

Kory Maincent wrote:
> Timestamping software or hardware flags are often used as a group,
> therefore adding these masks will easier future use.
> 
> I did not use SOF_TIMESTAMPING_SYS_HARDWARE flag as it is deprecated and
> not use at all.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next v7 00/16] net: Make timestamping selectable

[patchwork-bot+netdevbpf]

Hello:

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

On Tue, 14 Nov 2023 12:28:28 +0100 you wrote:
> 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.
> 
> This series updates the default MAC/PHY default timestamping and aims to
> allow the user to select the desired layer administratively.
> 
> [...]

Here is the summary with links:
  - [net-next,v7,01/16] net: Convert PHYs hwtstamp callback to use kernel_hwtstamp_config
    https://git.kernel.org/netdev/net-next/c/446e2305827b
  - [net-next,v7,02/16] net: phy: Remove the call to phy_mii_ioctl in phy_hwstamp_get/set
    https://git.kernel.org/netdev/net-next/c/430dc3256d57
  - [net-next,v7,03/16] net: ethtool: Refactor identical get_ts_info implementations.
    https://git.kernel.org/netdev/net-next/c/b8768dc40777
  - [net-next,v7,04/16] net: macb: Convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()
    https://git.kernel.org/netdev/net-next/c/202cb220026e
  - [net-next,v7,05/16] net: Make dev_set_hwtstamp_phylib accessible
    https://git.kernel.org/netdev/net-next/c/011dd3b3f83f
  - [net-next,v7,06/16] net: phy: micrel: fix ts_info value in case of no phc
    https://git.kernel.org/netdev/net-next/c/915d25a9d69b
  - [net-next,v7,07/16] net_tstamp: Add TIMESTAMPING SOFTWARE and HARDWARE mask
    https://git.kernel.org/netdev/net-next/c/acec05fb78ab
  - [net-next,v7,08/16] net: ethtool: Add a command to expose current time stamping layer
    https://git.kernel.org/netdev/net-next/c/11d55be06df0
  - [net-next,v7,09/16] netlink: specs: Introduce new netlink command to get current timestamp
    https://git.kernel.org/netdev/net-next/c/bb8645b00ced
  - [net-next,v7,10/16] net: ethtool: Add a command to list available time stamping layers
    https://git.kernel.org/netdev/net-next/c/d905f9c75329
  - [net-next,v7,11/16] netlink: specs: Introduce new netlink command to list available time stamping layers
    https://git.kernel.org/netdev/net-next/c/aed5004ee7a0
  - [net-next,v7,12/16] net: Replace hwtstamp_source by timestamping layer
    https://git.kernel.org/netdev/net-next/c/51bdf3165f01
  - [net-next,v7,13/16] net: Change the API of PHY default timestamp to MAC
    https://git.kernel.org/netdev/net-next/c/0f7f463d4821
  - [net-next,v7,14/16] net: ethtool: ts: Update GET_TS to reply the current selected timestamp
    https://git.kernel.org/netdev/net-next/c/091fab122869
  - [net-next,v7,15/16] net: ethtool: ts: Let the active time stamping layer be selectable
    https://git.kernel.org/netdev/net-next/c/152c75e1d002
  - [net-next,v7,16/16] netlink: specs: Introduce time stamping set command
    https://git.kernel.org/netdev/net-next/c/ee60ea6be0d3

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

Re: [PATCH net-next v7 07/16] net_tstamp: Add TIMESTAMPING SOFTWARE and HARDWARE mask

[Jakub Kicinski]

On Tue, 14 Nov 2023 12:28:35 +0100 Kory Maincent wrote:
> Timestamping software or hardware flags are often used as a group,
> therefore adding these masks will easier future use.
> 
> I did not use SOF_TIMESTAMPING_SYS_HARDWARE flag as it is deprecated and
> not use at all.

Does this really need to be in uAPI?

Re: [PATCH net-next v7 08/16] net: ethtool: Add a command to expose current time stamping layer

[Jakub Kicinski]

On Tue, 14 Nov 2023 12:28:36 +0100 Kory Maincent wrote:
> +		ops->get_ts_info(dev, &ts_info);
> +		if (ts_info.so_timestamping &
> +		    SOF_TIMESTAMPING_HARDWARE_MASK)
> +			data->ts_layer = MAC_TIMESTAMPING;
> +
> +		if (ts_info.so_timestamping &
> +		    SOF_TIMESTAMPING_SOFTWARE_MASK)
> +			data->ts_layer = SOFTWARE_TIMESTAMPING;

How does this work? so_timestamping is capabilities, not what's
enabled now. So if driver supports SW stamping we always return
SOFTWARE?

Re: [PATCH net-next v7 09/16] netlink: specs: Introduce new netlink command to get current timestamp

[Jakub Kicinski]

On Tue, 14 Nov 2023 12:28:37 +0100 Kory Maincent wrote:
> +  -
> +    name: ts
> +    attributes:
> +      -
> +        name: header
> +        type: nest
> +        nested-attributes: header
> +      -
> +        name: ts-layer
> +        type: u32

Please add the definition of the enum to the spec,
and link it here, so that Python can convert the values
to names.

Re: [PATCH net-next v7 10/16] net: ethtool: Add a command to list available time stamping layers

[Jakub Kicinski]

On Tue, 14 Nov 2023 12:28:38 +0100 Kory Maincent wrote:
> +	ETHTOOL_A_TS_LIST_LAYER,		/* array, u32 */

multi-attr u32, please

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Tue, 14 Nov 2023 12:28:43 +0100 Kory Maincent wrote:
> +	if (!tb[ETHTOOL_A_TS_LAYER])
> +		return 0;

GENL_REQ_ATTR_CHECK(), not sure why anyone would issue this command
without any useful attr.

> +	/* Disable time stamping in the current layer. */
> +	if (netif_device_present(dev) &&
> +	    (dev->ts_layer == PHY_TIMESTAMPING ||
> +	    dev->ts_layer == MAC_TIMESTAMPING)) {
> +		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);
> +		if (ret < 0)
> +			return ret;

So you only support PHYLIB?

The semantics need to be better documented :(

Re: [PATCH net-next v7 00/16] net: Make timestamping selectable

[Jakub Kicinski]

On Sat, 18 Nov 2023 15:10:26 +0000 patchwork-bot+netdevbpf@kernel.org
wrote:
> This series was applied to netdev/net-next.git (main)
> by David S. Miller <davem@davemloft.net>:

You need to run ./tools/net/ynl/ynl-regen.sh after touching specs.
The tree will now be dirty for anyone trying to do YNL work.
I'm dropping this series, sorry, please post a v8.

Re: [PATCH net-next v7 07/16] net_tstamp: Add TIMESTAMPING SOFTWARE and HARDWARE mask

[Köry Maincent]

On Sat, 18 Nov 2023 18:22:47 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 14 Nov 2023 12:28:35 +0100 Kory Maincent wrote:
> > Timestamping software or hardware flags are often used as a group,
> > therefore adding these masks will easier future use.
> > 
> > I did not use SOF_TIMESTAMPING_SYS_HARDWARE flag as it is deprecated and
> > not use at all.  
> 
> Does this really need to be in uAPI?

I have put it in the same place as SOF_TIMESTAMPING_* flags but indeed I am not
sure ethtool would need it.
I can move it to include/linux/net_tstamp.h and we will move back to uapi if
we see that it is necessary. What do you think?


-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 08/16] net: ethtool: Add a command to expose current time stamping layer

[Köry Maincent]

On Sat, 18 Nov 2023 18:24:24 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 14 Nov 2023 12:28:36 +0100 Kory Maincent wrote:
> > +		ops->get_ts_info(dev, &ts_info);
> > +		if (ts_info.so_timestamping &
> > +		    SOF_TIMESTAMPING_HARDWARE_MASK)
> > +			data->ts_layer = MAC_TIMESTAMPING;
> > +
> > +		if (ts_info.so_timestamping &
> > +		    SOF_TIMESTAMPING_SOFTWARE_MASK)
> > +			data->ts_layer = SOFTWARE_TIMESTAMPING;  
> 
> How does this work? so_timestamping is capabilities, not what's
> enabled now. So if driver supports SW stamping we always return
> SOFTWARE?

Yes, the software timestamping comes from the MAC capabilities.
I decided to separate software and MAC timestamping. If we select PHY
timestamping we can't use software timestamping and for an user, selecting the
MAC as timestamping seems not logical to use software timestamping.

Indeed there is a mistake here I should have used "else if" condition.
Mmh in fact, maybe not, because it would breaks the access to software
timestamping until patch 13.
I will remove the SOFTWARE/MAC timestamping distinction from this patch.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Sat, 18 Nov 2023 18:34:33 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 14 Nov 2023 12:28:43 +0100 Kory Maincent wrote:
> > +	if (!tb[ETHTOOL_A_TS_LAYER])
> > +		return 0;  
> 
> GENL_REQ_ATTR_CHECK(), not sure why anyone would issue this command
> without any useful attr.
> 
> > +	/* Disable time stamping in the current layer. */
> > +	if (netif_device_present(dev) &&
> > +	    (dev->ts_layer == PHY_TIMESTAMPING ||
> > +	    dev->ts_layer == MAC_TIMESTAMPING)) {
> > +		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);
> > +		if (ret < 0)
> > +			return ret;  
> 
> So you only support PHYLIB?
> 
> The semantics need to be better documented :(

Yes as we don't really know how each MAC deal with the timestamping
before ndo_hwstamp_get/set. Using phylib only allows us to be sure these NDO are
implemented and the management of timestamping is coherent in the MAC. Also It
will push people to move on to these NDOs.

Ok I will add documentation.

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 08/16] net: ethtool: Add a command to expose current time stamping layer

[Köry Maincent]

On Mon, 20 Nov 2023 10:17:57 +0100
Köry Maincent <kory.maincent@bootlin.com> wrote:

> On Sat, 18 Nov 2023 18:24:24 -0800
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Tue, 14 Nov 2023 12:28:36 +0100 Kory Maincent wrote:  
> > > +		ops->get_ts_info(dev, &ts_info);
> > > +		if (ts_info.so_timestamping &
> > > +		    SOF_TIMESTAMPING_HARDWARE_MASK)
> > > +			data->ts_layer = MAC_TIMESTAMPING;
> > > +
> > > +		if (ts_info.so_timestamping &
> > > +		    SOF_TIMESTAMPING_SOFTWARE_MASK)
> > > +			data->ts_layer = SOFTWARE_TIMESTAMPING;    
> > 
> > How does this work? so_timestamping is capabilities, not what's
> > enabled now. So if driver supports SW stamping we always return
> > SOFTWARE?  
> 
> Yes, the software timestamping comes from the MAC capabilities.
> I decided to separate software and MAC timestamping. If we select PHY
> timestamping we can't use software timestamping and for an user, selecting the
> MAC as timestamping seems not logical to use software timestamping.
> 
> Indeed there is a mistake here I should have used "else if" condition.
> Mmh in fact, maybe not, because it would breaks the access to software
> timestamping until patch 13.
> I will remove the SOFTWARE/MAC timestamping distinction from this patch.

After thinking again, the "else if" condition works and won't break anything as
we could still access software timestamping within the MAC_TIMESTAMPING layer.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

Hi Köry,

On Mon, Nov 20, 2023 at 10:44:39AM +0100, Köry Maincent wrote:
> On Sat, 18 Nov 2023 18:34:33 -0800
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Tue, 14 Nov 2023 12:28:43 +0100 Kory Maincent wrote:
> > > +	if (!tb[ETHTOOL_A_TS_LAYER])
> > > +		return 0;  
> > 
> > GENL_REQ_ATTR_CHECK(), not sure why anyone would issue this command
> > without any useful attr.
> > 
> > > +	/* Disable time stamping in the current layer. */
> > > +	if (netif_device_present(dev) &&
> > > +	    (dev->ts_layer == PHY_TIMESTAMPING ||
> > > +	    dev->ts_layer == MAC_TIMESTAMPING)) {
> > > +		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);
> > > +		if (ret < 0)
> > > +			return ret;  
> > 
> > So you only support PHYLIB?
> > 
> > The semantics need to be better documented :(
> 
> Yes as we don't really know how each MAC deal with the timestamping
> before ndo_hwstamp_get/set. Using phylib only allows us to be sure these NDO are
> implemented and the management of timestamping is coherent in the MAC. Also It
> will push people to move on to these NDOs.
> 
> Ok I will add documentation.
> 
> -- 
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com/

When Jakub says "the semantics need to be better documented", I'm also
thinking of a different direction.

From what I understand, Maxime is working on representing multiple
phylib PHYs in the UAPI:
https://patchwork.kernel.org/project/netdevbpf/cover/20231117162323.626979-1-maxime.chevallier@bootlin.com/

Does your UAPI proposal make it possible in any way to select
timestamping in phylib PHY A rather than PHY B? Or do you think it is
extensible to support that, somehow?

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Mon, 20 Nov 2023 12:52:55 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> > > So you only support PHYLIB?
> > > 
> > > The semantics need to be better documented :(  
> > 
> > Yes as we don't really know how each MAC deal with the timestamping
> > before ndo_hwstamp_get/set. Using phylib only allows us to be sure these
> > NDO are implemented and the management of timestamping is coherent in the
> > MAC. Also It will push people to move on to these NDOs.
> > 
> > Ok I will add documentation.
> 
> When Jakub says "the semantics need to be better documented", I'm also
> thinking of a different direction.
> 
> From what I understand, Maxime is working on representing multiple
> phylib PHYs in the UAPI:
> https://patchwork.kernel.org/project/netdevbpf/cover/20231117162323.626979-1-maxime.chevallier@bootlin.com/

Yes I am also following his patch series. 
 
> Does your UAPI proposal make it possible in any way to select
> timestamping in phylib PHY A rather than PHY B? Or do you think it is
> extensible to support that, somehow?

It does not support it for now.
I didn't want to base my work on his series as it could work without it for now
and I didn't want to wait to have his series accepted. It is more a future
possible support as I don't have anything to test it and I don't know if such
hardware exists right now.
I think it will be extensible to support that, my thinking was to create this
struct in net_device struct:

struct {
	enum layer;
	u32 id;
} ts;

With id saving the phy_index of the PHY X used when the layer PHY is selected.
This id could also be used to store the timestamp point in case of several
timestamp in a MAC.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Mon, Nov 20, 2023 at 12:14:40PM +0100, Köry Maincent wrote:
> > Does your UAPI proposal make it possible in any way to select
> > timestamping in phylib PHY A rather than PHY B? Or do you think it is
> > extensible to support that, somehow?
> 
> It does not support it for now.
> I didn't want to base my work on his series as it could work without it for now
> and I didn't want to wait to have his series accepted. It is more a future
> possible support as I don't have anything to test it and I don't know if such
> hardware exists right now.
> I think it will be extensible to support that, my thinking was to create this
> struct in net_device struct:
> 
> struct {
> 	enum layer;
> 	u32 id;
> } ts;
> 
> With id saving the phy_index of the PHY X used when the layer PHY is selected.
> This id could also be used to store the timestamp point in case of several
> timestamp in a MAC.

Ok, and I suppose the "u32 id" would be numerically the same as the
ETHTOOL_A_HEADER_PHY_INDEX nlattr that Maxime is proposing?

The next question would be: if a driver performs PHY management in
firmware, and does not use phylib, how should user space interact with it?
What timestamping layer and upon what should the ID be chosen?

Finally (and unrelated to the question above), why is SOFTWARE_TIMESTAMPING
even a layer exposed in the UAPI? My understanding of this patch set is
that it is meant to select the source of hardware timestamps that are
given to a socket. What gap in the UAPI does the introduction of a
SOFTWARE_TIMESTAMPING hwtstamping layer cover?

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Mon, 20 Nov 2023 14:06:01 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Mon, Nov 20, 2023 at 12:14:40PM +0100, Köry Maincent wrote:
> > > Does your UAPI proposal make it possible in any way to select
> > > timestamping in phylib PHY A rather than PHY B? Or do you think it is
> > > extensible to support that, somehow?  
> > 
> > It does not support it for now.
> > I didn't want to base my work on his series as it could work without it for
> > now and I didn't want to wait to have his series accepted. It is more a
> > future possible support as I don't have anything to test it and I don't
> > know if such hardware exists right now.
> > I think it will be extensible to support that, my thinking was to create
> > this struct in net_device struct:
> > 
> > struct {
> > 	enum layer;
> > 	u32 id;
> > } ts;
> > 
> > With id saving the phy_index of the PHY X used when the layer PHY is
> > selected. This id could also be used to store the timestamp point in case
> > of several timestamp in a MAC.  
> 
> Ok, and I suppose the "u32 id" would be numerically the same as the
> ETHTOOL_A_HEADER_PHY_INDEX nlattr that Maxime is proposing?

Yes.

> The next question would be: if a driver performs PHY management in
> firmware, and does not use phylib, how should user space interact with it?
> What timestamping layer and upon what should the ID be chosen?

In that case it could be the second options I refereed to.
Using the id to select the right timestamp within the NIC driver.
It indeed won't be called PHY timestamping as it is managed by the NIC firmware
but as it is managed by only one firmware and driver using the id to separate
the available timestamp seems a good idea.

Another solution would be to create another value in the layer enumeration.
PHY_NIC_TIMESTAMPING? Better idea? I am not good at naming.
 
> Finally (and unrelated to the question above), why is SOFTWARE_TIMESTAMPING
> even a layer exposed in the UAPI? My understanding of this patch set is
> that it is meant to select the source of hardware timestamps that are
> given to a socket. What gap in the UAPI does the introduction of a
> SOFTWARE_TIMESTAMPING hwtstamping layer cover?

As I explained to Jakub:
The software timestamping comes from the MAC driver capabilities and I decided
to separate software and MAC timestamping. If we select PHY timestamping we
can't use software timestamping and for an user, selecting the MAC as
timestamping seems not logical to use software timestamping (I got confused
myself when I first dig into it long time ago). Be able to select
directly Software timestamping seems appropriate and won't bring any harm. What
do you think?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Mon, Nov 20, 2023 at 02:49:29PM +0100, Köry Maincent wrote:
> > The next question would be: if a driver performs PHY management in
> > firmware, and does not use phylib, how should user space interact with it?
> > What timestamping layer and upon what should the ID be chosen?
> 
> In that case it could be the second options I refereed to.
> Using the id to select the right timestamp within the NIC driver.
> It indeed won't be called PHY timestamping as it is managed by the NIC firmware
> but as it is managed by only one firmware and driver using the id to separate
> the available timestamp seems a good idea.
> 
> Another solution would be to create another value in the layer enumeration.
> PHY_NIC_TIMESTAMPING? Better idea? I am not good at naming.

The point I was trying to make is that your current choice of exposing
PHY_TIMESTAMPING in UAPI, when it really only refers to phylib PHYs,
would lead exactly to this sort of UAPI balkanization where everyone
wants to add more timestamping layers, and to define IDs to be specific
to their own invented layer. Maybe the concept of timestamping layers is
not what user space should see at all.

In previous email discussions, I was proposing to Jakub and you "what if
we didn't let user space select a specific layer like PHY_TIMESTAMPING
or MAC_TIMESTAMPING at all, but just select a specific phc_index as the
provider of hardware timestamps"?

The limitation we're trying to lift is that currently, there can be only
a single provider of hardware timestamps. We make that provider customizable.
There is already a good understanding from user space that, if "ethtool -T"
on an interface says there is no PHC, then there are going to be no
hardware timestamps. So I thought it would be much more intuitive if the
timestamping layer could be selected by the user merely by an unified
phc_index (provided by a phylib phy or firmware based driver or whatever),
and everything else would just be an implementation detail of the kernel.
No one should care that it's a phylib phy, and shouldn't use a different
procedure to identify its ID based on whether it's a phylib or firmware
PHY.

It's a bit hard to align my expectation of what this series should offer
with yours. I think we're talking past each other, which unfortunately
makes me lose track and interest. I wish you could have answered my
earlier question about this alternative proposal.
https://lore.kernel.org/netdev/20231013170903.p3ycicebnfrsmoks@skbuf/

> > Finally (and unrelated to the question above), why is SOFTWARE_TIMESTAMPING
> > even a layer exposed in the UAPI? My understanding of this patch set is
> > that it is meant to select the source of hardware timestamps that are
> > given to a socket. What gap in the UAPI does the introduction of a
> > SOFTWARE_TIMESTAMPING hwtstamping layer cover?
> 
> As I explained to Jakub:
> The software timestamping comes from the MAC driver capabilities and I decided
> to separate software and MAC timestamping.

Why? What was the problem? This confuses me because I don't understand
what is the problem that the solution is trying to address, and whether
the solution is orthogonal to all the other UAPI that exists for
software and hardware timestamping at the socket layer - which AFAIK can
happily coexist.

> If we select PHY timestamping we can't use software timestamping and
> for an user, selecting the MAC as timestamping seems not logical to
> use software timestamping (I got confused myself when I first dig into
> it long time ago). Be able to select directly Software timestamping
> seems appropriate and won't bring any harm. What do you think?

Hmm, can you please explain what is the reason why software timestamping
can't coexist with PHY timestamping? It is a genuine question to which I
don't have an answer - I haven't used PHY timestamping. It must be
something specific to that, since I do know that MAC + software
timestamping work simultaneously just fine.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Mon, 20 Nov 2023 16:23:16 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Mon, Nov 20, 2023 at 02:49:29PM +0100, Köry Maincent wrote:
> > > The next question would be: if a driver performs PHY management in
> > > firmware, and does not use phylib, how should user space interact with it?
> > > What timestamping layer and upon what should the ID be chosen?  
> > 
> > In that case it could be the second options I refereed to.
> > Using the id to select the right timestamp within the NIC driver.
> > It indeed won't be called PHY timestamping as it is managed by the NIC
> > firmware but as it is managed by only one firmware and driver using the id
> > to separate the available timestamp seems a good idea.
> > 
> > Another solution would be to create another value in the layer enumeration.
> > PHY_NIC_TIMESTAMPING? Better idea? I am not good at naming.  
> 
> The point I was trying to make is that your current choice of exposing
> PHY_TIMESTAMPING in UAPI, when it really only refers to phylib PHYs,
> would lead exactly to this sort of UAPI balkanization where everyone
> wants to add more timestamping layers, and to define IDs to be specific
> to their own invented layer. Maybe the concept of timestamping layers is
> not what user space should see at all.
> 
> In previous email discussions, I was proposing to Jakub and you "what if
> we didn't let user space select a specific layer like PHY_TIMESTAMPING
> or MAC_TIMESTAMPING at all, but just select a specific phc_index as the
> provider of hardware timestamps"?
> 
> The limitation we're trying to lift is that currently, there can be only
> a single provider of hardware timestamps. We make that provider customizable.
> There is already a good understanding from user space that, if "ethtool -T"
> on an interface says there is no PHC, then there are going to be no
> hardware timestamps. So I thought it would be much more intuitive if the
> timestamping layer could be selected by the user merely by an unified
> phc_index (provided by a phylib phy or firmware based driver or whatever),
> and everything else would just be an implementation detail of the kernel.
> No one should care that it's a phylib phy, and shouldn't use a different
> procedure to identify its ID based on whether it's a phylib or firmware
> PHY.
> 
> It's a bit hard to align my expectation of what this series should offer
> with yours. I think we're talking past each other, which unfortunately
> makes me lose track and interest. I wish you could have answered my
> earlier question about this alternative proposal.
> https://lore.kernel.org/netdev/20231013170903.p3ycicebnfrsmoks@skbuf/

I did thought about it but I got stuck by the case of hardware timestamping
without PHC. Richard explained the reason of its existence here:
https://lore.kernel.org/netdev/ZS3MKWlnPqTe8gkq@hoboy.vegasvil.org/#t

Maybe I got a bit stuck in my implementation and should investigate more your
proposition and how to deal with this case. Do you have an idea on how to
solve it?

> > > Finally (and unrelated to the question above), why is
> > > SOFTWARE_TIMESTAMPING even a layer exposed in the UAPI? My understanding
> > > of this patch set is that it is meant to select the source of hardware
> > > timestamps that are given to a socket. What gap in the UAPI does the
> > > introduction of a SOFTWARE_TIMESTAMPING hwtstamping layer cover?  
> > 
> > As I explained to Jakub:
> > The software timestamping comes from the MAC driver capabilities and I
> > decided to separate software and MAC timestamping.  
> 
> Why? What was the problem? This confuses me because I don't understand
> what is the problem that the solution is trying to address, and whether
> the solution is orthogonal to all the other UAPI that exists for
> software and hardware timestamping at the socket layer - which AFAIK can
> happily coexist.
> 
> > If we select PHY timestamping we can't use software timestamping and
> > for an user, selecting the MAC as timestamping seems not logical to
> > use software timestamping (I got confused myself when I first dig into
> > it long time ago). Be able to select directly Software timestamping
> > seems appropriate and won't bring any harm. What do you think?  
> 
> Hmm, can you please explain what is the reason why software timestamping
> can't coexist with PHY timestamping? It is a genuine question to which I
> don't have an answer - I haven't used PHY timestamping. It must be
> something specific to that, since I do know that MAC + software
> timestamping work simultaneously just fine.

The software timestamp is managed through the MAC driver calling
skb_tx_timestamp() function. The PHY driver does not call it, that's why there
is no software timestamping in PHY driver capabilities. Also the PHY driver
doesn't know if the MAC driver support it so it currently can not coexist with
PHY timestamping.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Mon, Nov 20, 2023 at 03:53:44PM +0100, Köry Maincent wrote:
> I did thought about it but I got stuck by the case of hardware timestamping
> without PHC. Richard explained the reason of its existence here:
> https://lore.kernel.org/netdev/ZS3MKWlnPqTe8gkq@hoboy.vegasvil.org/#t
> 
> Maybe I got a bit stuck in my implementation and should investigate more your
> proposition and how to deal with this case. Do you have an idea on how to
> solve it?

I would take what Richard said with a grain of salt, and interpret as
"there exists hardware with hwts but w/o PHC, and that may work for
marginal use cases", not that "we should design having that as a
first-class citizen in mind".

If there is any need for me to point out that such kind of driver isn't
a first class citizen, then here's an experiment:

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index e993ed04ab57..5755f54197b9 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -842,13 +842,7 @@ static int enetc_get_ts_info(struct net_device *ndev,
 {
 	int *phc_idx;
 	
-	phc_idx = symbol_get(enetc_phc_index);
-	if (phc_idx) {
-		info->phc_index = *phc_idx;
-		symbol_put(enetc_phc_index);
-	} else {
-		info->phc_index = -1;
-	}
+	info->phc_index = -1;
 
 #ifdef CONFIG_FSL_ENETC_PTP_CLOCK
 	info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |

When elected as master, it kinda works, and does synchronize with a
slave, even if ptp4l gets confused about the phc_index being -1.

But when elected as a slave by the BMCA, ptp4l gets confused and thinks
that phc_index == -1 means that it's supposed to use software timestamping.

$ ptp4l -i eno0 -2 -P -m -s
ptp4l[1185.594]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[1185.598]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[1186.887]: port 1: new foreign master 00049f.fffe.05f628-1
ptp4l[1190.889]: selected best master clock 00049f.fffe.05f628
ptp4l[1190.890]: port 1: LISTENING to UNCALIBRATED on RS_SLAVE
ptp4l[1191.891]: master offset 37000003850 s0 freq +100000000 path delay       823
ptp4l[1192.896]: master offset 3[ 1235.693485] systemd-journald[125]: Time jumped backwards, rotating.
7000003848 s1 freq +99999998 path delay       822
ptp4l[1193.603]: clockcheck: clock jumped forward or running faster than expected!
ptp4l[1193.892]: clockcheck: clock jumped forward or running faster than expected!
ptp4l[1193.893]: master offset 37000003852 s0 freq +99999998 path delay       822
ptp4l[1194.342]: clockcheck: clock jumped forward or running faster than expected!
ptp4l[1194.604]: clockcheck: clock jumped forward or running faster than expected!

So, I guess the only thing we need to do to this kind of setup is not do
too much harm to it.

We break nothing if we make the phc_index the central aspect of hwts
layer selection - except for the fact that such a MAC won't be able to
change its timestamping layer to be a PHY.

If we wanted to add such a capability to that MAC driver, the obvious
way to solve the lack of a PHC is to create a PHC that returns
-EOPNOTSUPP for all of its ptp_clock_info operations (gettime, settime
etc). It may possibly be that, in the worst case, ptp4l needs to probe
for each syscall on the NIC's PHC being operational before deciding what
can be done with it. But that's already an improvement over the current
handling to make it more graceful, it's not to keep things on par.

> > Hmm, can you please explain what is the reason why software timestamping
> > can't coexist with PHY timestamping? It is a genuine question to which I
> > don't have an answer - I haven't used PHY timestamping. It must be
> > something specific to that, since I do know that MAC + software
> > timestamping work simultaneously just fine.
> 
> The software timestamp is managed through the MAC driver calling
> skb_tx_timestamp() function. The PHY driver does not call it, that's why there
> is no software timestamping in PHY driver capabilities. Also the PHY driver
> doesn't know if the MAC driver support it so it currently can not coexist with
> PHY timestamping.

I don't understand. Documentation/networking/timestamping.rst says that
skb_tx_timestamp() is one of the actual _mechanisms_ through which phylib
timestamping works. It's called by the parent MAC driver, and
mii_ts->txtstamp() hooks onto that. So in some situations, the PHY
timestamping core _piggybacks_ on top of software timestamping support
in the MAC.

Where I agree is that the PHY driver has no business in deciding whether
the interface should report the socket option flags for software timestamping
or not. But I still don't see a proof that they can't coexist. What you need
to explain is what makes said software timestamping unusable in the
presence of PHY timestamping - to justify this separate software
timestamping layer in your UAPI proposal.

Re: [PATCH net-next v7 07/16] net_tstamp: Add TIMESTAMPING SOFTWARE and HARDWARE mask

[Jakub Kicinski]

On Mon, 20 Nov 2023 10:05:49 +0100 Köry Maincent wrote:
> > Does this really need to be in uAPI?  
> 
> I have put it in the same place as SOF_TIMESTAMPING_* flags but indeed I am not
> sure ethtool would need it.
> I can move it to include/linux/net_tstamp.h and we will move back to uapi if
> we see that it is necessary. What do you think?

include/linux/net_tstamp.h sounds better to me, Willem may disagree..

Re: [PATCH net-next v7 07/16] net_tstamp: Add TIMESTAMPING SOFTWARE and HARDWARE mask

[Willem de Bruijn]

Jakub Kicinski wrote:
> On Mon, 20 Nov 2023 10:05:49 +0100 Köry Maincent wrote:
> > > Does this really need to be in uAPI?  
> > 
> > I have put it in the same place as SOF_TIMESTAMPING_* flags but indeed I am not
> > sure ethtool would need it.
> > I can move it to include/linux/net_tstamp.h and we will move back to uapi if
> > we see that it is necessary. What do you think?
> 
> include/linux/net_tstamp.h sounds better to me, Willem may disagree..

Sounds like the right home to me for non uapi timestamping, too.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Mon, 20 Nov 2023 18:10:04 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Mon, Nov 20, 2023 at 03:53:44PM +0100, Köry Maincent wrote:
> > I did thought about it but I got stuck by the case of hardware timestamping
> > without PHC. Richard explained the reason of its existence here:
> > https://lore.kernel.org/netdev/ZS3MKWlnPqTe8gkq@hoboy.vegasvil.org/#t
> > 
> > Maybe I got a bit stuck in my implementation and should investigate more
> > your proposition and how to deal with this case. Do you have an idea on how
> > to solve it?  
> 
> I would take what Richard said with a grain of salt, and interpret as
> "there exists hardware with hwts but w/o PHC, and that may work for
> marginal use cases", not that "we should design having that as a
> first-class citizen in mind".

> When elected as master, it kinda works, and does synchronize with a
> slave, even if ptp4l gets confused about the phc_index being -1.
> 
> But when elected as a slave by the BMCA, ptp4l gets confused and thinks
> that phc_index == -1 means that it's supposed to use software timestamping.
> 
> So, I guess the only thing we need to do to this kind of setup is not do
> too much harm to it.
> 
> We break nothing if we make the phc_index the central aspect of hwts
> layer selection - except for the fact that such a MAC won't be able to
> change its timestamping layer to be a PHY.
> 
> If we wanted to add such a capability to that MAC driver, the obvious
> way to solve the lack of a PHC is to create a PHC that returns
> -EOPNOTSUPP for all of its ptp_clock_info operations (gettime, settime
> etc). It may possibly be that, in the worst case, ptp4l needs to probe
> for each syscall on the NIC's PHC being operational before deciding what
> can be done with it. But that's already an improvement over the current
> handling to make it more graceful, it's not to keep things on par.

Ok, you convinced me.
Thanks for your arguments and spending time explaining it.


Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Mon, 20 Nov 2023 16:23:16 +0200 Vladimir Oltean wrote:
> In previous email discussions, I was proposing to Jakub and you "what if
> we didn't let user space select a specific layer like PHY_TIMESTAMPING
> or MAC_TIMESTAMPING at all, but just select a specific phc_index as the
> provider of hardware timestamps"?

What about my use case of having a NIC which can stamp at a low rate
at the PHY (for PTP) and high rate at the DMA block (for congestion
control)? Both stamp points have the same PHC index.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Andrew Lunn]

> What about my use case of having a NIC which can stamp at a low rate
> at the PHY (for PTP) and high rate at the DMA block (for congestion
> control)? Both stamp points have the same PHC index.

How theoretical is that? To me, it seems more likely you have two PHC.

The PHY stamping tends to be slow because of the MDIO bus. If the MAC
has fast access to the PHC, it means its not on the MDIO bus. It
probably means you have the PHY integrated into the MAC/SoC, so the
MAC can access it. But if its integrated like this, i don't see why
PHY stamping should be particularly slow. So you can probably use it
for congestion control. And then you don't need DMA stamping.

Do you know of real hardware with a MAC and a PHY sharing a PHC?

   Andrew

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Mon, 20 Nov 2023 19:39:35 +0100 Andrew Lunn wrote:
> > What about my use case of having a NIC which can stamp at a low rate
> > at the PHY (for PTP) and high rate at the DMA block (for congestion
> > control)? Both stamp points have the same PHC index.  
> 
> How theoretical is that? To me, it seems more likely you have two PHC.

Very practical. mlx5 does this today, based on guessing and private
ethtool flags.

> The PHY stamping tends to be slow because of the MDIO bus. If the MAC
> has fast access to the PHC, it means its not on the MDIO bus. It
> probably means you have the PHY integrated into the MAC/SoC, so the
> MAC can access it. But if its integrated like this, i don't see why
> PHY stamping should be particularly slow. So you can probably use it
> for congestion control. And then you don't need DMA stamping.

Tx stamps are harder to carry back to the host all the way from the PHY
than from the DMA block when DMA completion is signaled. Rx stamps seem
much easier to carry down the pipeline but apparently some vendors are
incapable of doing that as well.

> Do you know of real hardware with a MAC and a PHY sharing a PHC?

mlx5 for sure, but other designs, too. PHY, NIC pipeline and PCIe PTM
may all need to time stamp from a single time counter.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Mon, Nov 20, 2023 at 09:37:23AM -0800, Jakub Kicinski wrote:
> On Mon, 20 Nov 2023 16:23:16 +0200 Vladimir Oltean wrote:
> > In previous email discussions, I was proposing to Jakub and you "what if
> > we didn't let user space select a specific layer like PHY_TIMESTAMPING
> > or MAC_TIMESTAMPING at all, but just select a specific phc_index as the
> > provider of hardware timestamps"?
> 
> What about my use case of having a NIC which can stamp at a low rate
> at the PHY (for PTP) and high rate at the DMA block (for congestion
> control)? Both stamp points have the same PHC index.

Well, first of all, given my understanding of the "laws of physics",
I think something has to give in your use case description. I can't
see how on RX, the NIC can decide in advance whether to provide low
rate MAC timestamps for packets going to a socket and high rate DMA
timestamps for packets going to another socket. It can either provide
MAC timestamps, or DMA timestamps, or an unreliable, unpresentable to
user space, mix.

But maybe I'm wrong and there are NICs which can do that filtering.
If such NIC exists, then I guess a SOF_TIMESTAMPING_RX_DMA flag should
be added to the socket layer, and the NIC driver provides timestamps
according to the skb->sk->sk_tsflags, and that problem is completely out
of scope for Köry's patch set - and implicitly compatible with it, since
as you say, the device-wide timestamping layer - PHC index - does not
really change.

If I'm not wrong and the MAC-or-DMA timestamp selection is NIC-wide
(which diverges from your problem description), then neither Köry's work
nor my "everything is a phc_index" proposal will bring your use case to
fruition without further work. Here I would avoid speculating, because a
lot will depend upon the details which you haven't really given.

One question will be whether, in the case of "NIC-wide DMA timestamps",
DMA timestamps should be presented as hardware timestamps - struct
scm_timestamping[2] from CMSG_DATA() - or as their own thing, that user
space needs explicit support for - by parsing a new cmsg level/type.
If DMA timestamps won't look to user space like hardware timestamps,
then the use case is again out of scope for Köry's work, as far as I see
it.

Another simple question is - if NICs do this today - probably by giving
the "unrepresentable mix" to user space in an implicit, hardcoded and
very fine tuned way such that nobody bats an eye - then what is there
more to support? Are you looking at extra UAPI as a way to legitimize
hacks, or do you feel there is extra control that applications can gain?

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Russell King (Oracle)]

On Mon, Nov 20, 2023 at 07:39:35PM +0100, Andrew Lunn wrote:
> > What about my use case of having a NIC which can stamp at a low rate
> > at the PHY (for PTP) and high rate at the DMA block (for congestion
> > control)? Both stamp points have the same PHC index.
> 
> How theoretical is that? To me, it seems more likely you have two PHC.
> 
> The PHY stamping tends to be slow because of the MDIO bus. If the MAC
> has fast access to the PHC, it means its not on the MDIO bus. It
> probably means you have the PHY integrated into the MAC/SoC, so the
> MAC can access it. But if its integrated like this, i don't see why
> PHY stamping should be particularly slow. So you can probably use it
> for congestion control. And then you don't need DMA stamping.
> 
> Do you know of real hardware with a MAC and a PHY sharing a PHC?

Not so much a MAC and PHY sharing a PHC, but I notice in USXGMII-M,
there is the ability for the PHY to pass the timestamp back to the MAC
through the USXGMII-M connection - which would eliminate the problems
of accessing the PHY to get that the timestamps.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Mon, 20 Nov 2023 21:00:23 +0200 Vladimir Oltean wrote:
> Well, first of all, given my understanding of the "laws of physics",
> I think something has to give in your use case description. I can't
> see how on RX, the NIC can decide in advance whether to provide low
> rate MAC timestamps for packets going to a socket and high rate DMA
> timestamps for packets going to another socket. It can either provide
> MAC timestamps, or DMA timestamps, or an unreliable, unpresentable to
> user space, mix.

Rx time stamping is configured by filters. Is there a problem with user
specifying that they want "true" timestamps for PTP/NTP packets, and
"dma" timestamps for all the rest?

Maybe we can extend struct scm_timestamping to carry an indication
which stamp ended up in ts[2] but that's less important to me than
the ability to configure the thing. Right now, as I said, mlx5 uses
an ethtool priv flag :(

> But maybe I'm wrong and there are NICs which can do that filtering.
> If such NIC exists, then I guess a SOF_TIMESTAMPING_RX_DMA flag should
> be added to the socket layer, and the NIC driver provides timestamps
> according to the skb->sk->sk_tsflags, and that problem is completely out
> of scope for Köry's patch set - and implicitly compatible with it, since
> as you say, the device-wide timestamping layer - PHC index - does not
> really change.

IDK. Maybe the sniffles I picked up at LPC are clouding my judgment
but to me this patch set is shaped too much by current implementation
and not enough by what it's modeling. It basically exposes to user
space the "mux" for choosing NETDEV vs PHYLIB.

There are multiple time stamping points as the packet moves thru 
the pipeline. Expose them so that SIOC[GS]HWTSTAMP can target each
on individually.

> If I'm not wrong and the MAC-or-DMA timestamp selection is NIC-wide
> (which diverges from your problem description),

Nope.

> then neither Köry's work
> nor my "everything is a phc_index" proposal will bring your use case to
> fruition without further work. Here I would avoid speculating, because a
> lot will depend upon the details which you haven't really given.

What are the details you'd like? PTP gets stamped at the PHY/MAC, 
the rest gets stamped at DMA. mlx5 achieves this by splitting the
PTP traffic to a separate queue pair, and configuring that qp to
capture PHY/MAC stamps, AFAIU.

> One question will be whether, in the case of "NIC-wide DMA timestamps",
> DMA timestamps should be presented as hardware timestamps - struct
> scm_timestamping[2] from CMSG_DATA() - or as their own thing, that user
> space needs explicit support for - by parsing a new cmsg level/type.
> If DMA timestamps won't look to user space like hardware timestamps,
> then the use case is again out of scope for Köry's work, as far as I see
> it.
> 
> Another simple question is - if NICs do this today - probably by giving
> the "unrepresentable mix" to user space in an implicit, hardcoded and
> very fine tuned way such that nobody bats an eye - then what is there
> more to support? Are you looking at extra UAPI as a way to legitimize
> hacks, or do you feel there is extra control that applications can gain?

I don't understand what you're asking me.

DMA timestamping is becoming increasingly important. Ready any
congestion control paper from the last 5 years and chances are
it will be using delay as a signal. If we're extending uAPI
for Hw stamping we should make sure to cater to CC use cases.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Mon, Nov 20, 2023 at 10:51:48AM -0800, Jakub Kicinski wrote:
> On Mon, 20 Nov 2023 19:39:35 +0100 Andrew Lunn wrote:
> > Do you know of real hardware with a MAC and a PHY sharing a PHC?
> 
> mlx5 for sure, but other designs, too. PHY, NIC pipeline and PCIe PTM
> may all need to time stamp from a single time counter.

I'm still waiting for you to fully clarify the "per socket vs global"
aspect, but independently of that, at least I understand why this is a
counter-argument to my proposal. I need to tune it a bit (ASSUMING that
we want DMA timestamps to "look like" hwtimestamps, and not like their
own thing, to user space), because the PHC index would no longer fully
identify a hwtstamp provider, so we need something more.

I imagine both ETHTOOL_MSG_TSINFO_GET and ETHTOOL_MSG_TSINFO_SET to
support a new (nest) nlattr called ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER.

This would contain (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_PHC_INDEX
and (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER. It could be
extensible in the future, but this is the baseline and forms the key.

The latter takes values from an:

enum ethtool_hwstamp_provider_qualifier {
	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_MAC,
	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PHY,
	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_DMA,
};

with Jakub's comments about the various types providing various
qualities of timestamps, given here:
https://lore.kernel.org/netdev/20230511150902.57d9a437@kernel.org/

 - PHY - per spec, at the RS layer
 - MAC - "close to the wire" in the MAC, specifically the pipeline
   delay (PHY stamp vs MAC stamp) should be constant for all packets;
   there must be no variable-time buffering and (for Tx) the time
   stamping must be past the stage of the pipeline affected by pause
   frames
 - DMA - worst quality, variable delay timestamp, usually taken when
   packets DMA descriptors (Rx or completion) are being written


It _sounds_ like we've all been talking about the same thing for ages,
but we weren't.


So, a PHC could offer multiple hwtstamp providers, as many as there are
qualifiers to uniquely describe them. Each hwstamp provider is
represented by a single ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER nested nlattr.
In TSINFO_GET requests, there are as many ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER
nests as there are hwtstamp providers for the NIC.

In the "normal" case of one single hwtstamp provider per PHC, it would
be the responsibility of the driver to set its qualifier to the right
thing: phylib to ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PHY, and "normal" MAC
drivers to ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_MAC. Here, the qualifier
isn't more than an extra (partially redundant) mechanism for user space
to know what it's juggling with.

As opposed to Köry's proposal (where "dev->ts_layer == PHY_TIMESTAMPING"
means actual phylib), ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PHY describes
the PHY-like timestamp quality of any hwtstamp provider, be it provided
by a phylib PHY or a firmware-based PHY. It doesn't describe "the layer"
itself.

Does this tick all boxes?

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Mon, Nov 20, 2023 at 11:58:39AM -0800, Jakub Kicinski wrote:
> Rx time stamping is configured by filters. Is there a problem with user
> specifying that they want "true" timestamps for PTP/NTP packets, and
> "dma" timestamps for all the rest?

There is, because enum hwtstamp_rx_filters is NIC-wide, and there is
only one of those - corresponding to the single hwtstamp (ts[2]) provider.
There were never talks in this patch set, AFAIR, about multiple hwtstamp
providers active simultaneously (for different traffic streams) and
configuring them independently, with separate RX filters.

> Maybe we can extend struct scm_timestamping to carry an indication
> which stamp ended up in ts[2] but that's less important to me than
> the ability to configure the thing. Right now, as I said, mlx5 uses
> an ethtool priv flag :(

No, you misunderstood me. I didn't suggest (at least not here) to add an
indication to struct scm_timestamping of "what's the source of ts[2]
(the hwtstamp)".

I was just _asking_ (collecting data) whether it's ultimately desirable
for DMA timestamps to be visible in ts[2] (indistinguishable from a
better hwtstamp, as they currently are, I guess) rather than their
own thing. Like for example, in a congestion control algorithm, where
does TCP really get them from.

If they'd rather be their own thing in a fully developed API, then the
whole discussion is rather off-topic to Köry, because here, we're
beating the dead horse of "where does ts[2] come from" - _still_ a
single source, just selectable, that is.

> > But maybe I'm wrong and there are NICs which can do that filtering.
> > If such NIC exists, then I guess a SOF_TIMESTAMPING_RX_DMA flag should
> > be added to the socket layer, and the NIC driver provides timestamps
> > according to the skb->sk->sk_tsflags, and that problem is completely out
> > of scope for Köry's patch set - and implicitly compatible with it, since
> > as you say, the device-wide timestamping layer - PHC index - does not
> > really change.
> 
> IDK. Maybe the sniffles I picked up at LPC are clouding my judgment
> but to me this patch set is shaped too much by current implementation
> and not enough by what it's modeling. It basically exposes to user
> space the "mux" for choosing NETDEV vs PHYLIB.

The last sentence I agree with.

> There are multiple time stamping points as the packet moves thru 
> the pipeline. Expose them so that SIOC[GS]HWTSTAMP can target each
> on individually.

Ok, that is rather vague and complex.

You will forever have to contend with the fact that struct scm_timestamping
can contain a single hwtstamp per packet: ts[2]. So you need complex
control path logic to ensure that the sum of RX filters for all
timestamping points in the packet data path doesn't actually request
more than one ts[2] for any skb.

I understand how that could scrath your itch, but here, it sounds
off-topic?

This is actually starting to get close, in a way, to my feedback to
Richard to allow multiple hwtstamp sources for an skb, and to just give
an indication in the cmsg of what's their source, leaving user space to
figure out the rest.
https://lore.kernel.org/netdev/20220120164832.xdebp5vykib6h6dp@skbuf/

But his response was "There was a fair amount of discussion, and it
seemed to me that everyone wanted a pony."
https://lore.kernel.org/netdev/Y%2F0Idkhy27TObawi@hoboy.vegasvil.org/

I mean, IDK, maybe it's not off-topic, but it's a round-about way of
achieving what they think they can achieve in a more straightforward way.

Rephrased in my own words, you're saying:

Forget the concept of an active hwtstamp provider, just open up the
knobs of _all_ possible hwtstamp providers for a NIC. Simultaneously!
To make one active and all the others inactive, just use
HWTSTAMP_FILTER_NONE/HWTSTAMP_TX_OFF for all except one, and the desired
enum hwtstamp_rx_filters / enum hwtstamp_tx_types for the active one.
Live with this expanded configuration model for a while, just restricted
for a single active timestamping layer, and then, once user space is
ready for an enhanced struct scm_timestamping which supports potentially
multiple cmsgs with distinct hwtstamps, remove the restriction and let
it all rip! Everybody gets their pony!

Additionally, SIOCSHWTSTAMP is kinda rusty, has a fixed binary format,
and is not extensible to target a specific hwtstamp provider. So a
netlink conversion of that, as a first step, would of course be great.

Is it an accurate summary?

If it is, I'll let Köry comment on the feasibility :)

> > If I'm not wrong and the MAC-or-DMA timestamp selection is NIC-wide
> > (which diverges from your problem description),
> 
> Nope.
> 
> > then neither Köry's work
> > nor my "everything is a phc_index" proposal will bring your use case to
> > fruition without further work. Here I would avoid speculating, because a
> > lot will depend upon the details which you haven't really given.
> 
> What are the details you'd like? PTP gets stamped at the PHY/MAC, 
> the rest gets stamped at DMA. mlx5 achieves this by splitting the
> PTP traffic to a separate queue pair, and configuring that qp to
> capture PHY/MAC stamps, AFAIU.

That's enough, thanks.

> > One question will be whether, in the case of "NIC-wide DMA timestamps",
> > DMA timestamps should be presented as hardware timestamps - struct
> > scm_timestamping[2] from CMSG_DATA() - or as their own thing, that user
> > space needs explicit support for - by parsing a new cmsg level/type.
> > If DMA timestamps won't look to user space like hardware timestamps,
> > then the use case is again out of scope for Köry's work, as far as I see
> > it.
> > 
> > Another simple question is - if NICs do this today - probably by giving
> > the "unrepresentable mix" to user space in an implicit, hardcoded and
> > very fine tuned way such that nobody bats an eye - then what is there
> > more to support? Are you looking at extra UAPI as a way to legitimize
> > hacks, or do you feel there is extra control that applications can gain?
> 
> I don't understand what you're asking me.

You've partially answered above. The mix of timestamps coming from the
PHY/MAC and those coming from the DMA is unrepresentable in today's
UAPI, and is just fine-tuned to work for the existing use case of "PTP
gets PHY/MAC, everything else gets DMA".

Still not 100% clear what would the proper UAPI (separate user-controllable
RX filters for PHY, MAC and DMA) gain, in addition to what exists in mlx5.

> DMA timestamping is becoming increasingly important. Ready any
> congestion control paper from the last 5 years and chances are
> it will be using delay as a signal. If we're extending uAPI
> for Hw stamping we should make sure to cater to CC use cases.

I'll stop commenting here for a while and go read some of those papers and
RFCs, in the hope that I find some info about the way in which TCP expects
hwtstamps from a NIC. It's quite evident to me that I don't have enough
information to help this discussion reach a conclusion.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Mon, 20 Nov 2023 23:17:59 +0200 Vladimir Oltean wrote:
> Forget the concept of an active hwtstamp provider, just open up the
> knobs of _all_ possible hwtstamp providers for a NIC. Simultaneously!
> To make one active and all the others inactive, just use
> HWTSTAMP_FILTER_NONE/HWTSTAMP_TX_OFF for all except one, and the desired
> enum hwtstamp_rx_filters / enum hwtstamp_tx_types for the active one.
> Live with this expanded configuration model for a while, just restricted
> for a single active timestamping layer, and then, once user space is
> ready for an enhanced struct scm_timestamping which supports potentially
> multiple cmsgs with distinct hwtstamps, remove the restriction and let
> it all rip! Everybody gets their pony!
> 
> Additionally, SIOCSHWTSTAMP is kinda rusty, has a fixed binary format,
> and is not extensible to target a specific hwtstamp provider. So a
> netlink conversion of that, as a first step, would of course be great.
> 
> Is it an accurate summary?

Yes.

For now we can impose the requirement that only one can be active 
easily at the kernel level. But the uAPI should allow expressing more.

> You've partially answered above. The mix of timestamps coming from the
> PHY/MAC and those coming from the DMA is unrepresentable in today's
> UAPI, and is just fine-tuned to work for the existing use case of "PTP
> gets PHY/MAC, everything else gets DMA".
> 
> Still not 100% clear what would the proper UAPI (separate user-controllable
> RX filters for PHY, MAC and DMA) gain, in addition to what exists in mlx5.

Too late for mlx5 but I'm anticipating that more vendors will start
needing such configuration in the future. At which point it will be
good to have an API in place.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Mon, 20 Nov 2023 21:58:58 +0200 Vladimir Oltean wrote:
> I'm still waiting for you to fully clarify the "per socket vs global"
> aspect, but independently of that, at least I understand why this is a
> counter-argument to my proposal. I need to tune it a bit (ASSUMING that
> we want DMA timestamps to "look like" hwtimestamps, and not like their
> own thing, to user space), because the PHC index would no longer fully
> identify a hwtstamp provider, so we need something more.
> 
> I imagine both ETHTOOL_MSG_TSINFO_GET and ETHTOOL_MSG_TSINFO_SET to
> support a new (nest) nlattr called ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER.
> 
> This would contain (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_PHC_INDEX
> and (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER. It could be
> extensible in the future, but this is the baseline and forms the key.
> 
> The latter takes values from an:
> 
> enum ethtool_hwstamp_provider_qualifier {
> 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_MAC,
> 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PHY,
> 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_DMA,
> };

Sounds reasonable. Having more attributes than just PHC index works.
Given the lack of distinction between MAC and PHY for integrated NICs
I'd lean towards ditching the "layers" completely and exposing 
an "approximate" vs "precise" boolean. Approximate being the DMA point
for NICs, but more generically a point that is separated from the wire
by buffering or other variable length delay. Precise == IEEE 1588
quality.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Mon, Nov 20, 2023 at 01:37:37PM -0800, Jakub Kicinski wrote:
> > Is it an accurate summary?
> 
> Yes.
> 
> For now we can impose the requirement that only one can be active 
> easily at the kernel level. But the uAPI should allow expressing more.

I see. That's quite something to think about for Köry. In its defense,
I also agree that this idea seems the most orthogonal to everything else
that we have or may want to add in the future, and is not likely to
become obsoleted by some other mechanism that can achieve the same
thing, but in a more flexible way. It's just that it's quite the task.

I sense it may be time to dust off and submit the rest of my
ndo_hwtstamp_get()/ ndo_hwtstamp_set() conversions before a netlink
conversion of SIOCGHWTSTAMP/SIOCSHWTSTAMP could even take place...
https://github.com/vladimiroltean/linux/commits/ndo-hwtstamp-v9

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Tue, 21 Nov 2023 00:05:49 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Mon, Nov 20, 2023 at 01:37:37PM -0800, Jakub Kicinski wrote:
> > > Is it an accurate summary?  
> > 
> > Yes.
> > 
> > For now we can impose the requirement that only one can be active 
> > easily at the kernel level. But the uAPI should allow expressing more.  
> 
> I see. That's quite something to think about for Köry. In its defense,
> I also agree that this idea seems the most orthogonal to everything else
> that we have or may want to add in the future, and is not likely to
> become obsoleted by some other mechanism that can achieve the same
> thing, but in a more flexible way. It's just that it's quite the task.
> 
> I sense it may be time to dust off and submit the rest of my
> ndo_hwtstamp_get()/ ndo_hwtstamp_set() conversions before a netlink
> conversion of SIOCGHWTSTAMP/SIOCSHWTSTAMP could even take place...
> https://github.com/vladimiroltean/linux/commits/ndo-hwtstamp-v9

Ok I kind of got an idea of what is your prerequisites.

If I summarize, a solution could be this:

- Expand struct hwtstamp_config with a phc_index member for the SIOCG/SHWTSTAMP
  commands.
  To keep backward compatibility if phc_index is not set in the hwtstamp_config
  data from userspace use the default hwtstamp (the default being selected as
  done in my patch series).
  Is this possible, would it breaks things?

- In netlink part, send one netlink tsinfo skb for each phc_index.

Could be done in a later patch series:
- Expand netlink TSINFO with ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER.
  Describing this struct:
enum ethtool_hwstamp_provider_qualifier {
 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PRECISE,
 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_APPROX,
}; 

  Set the desired qualifier through TSINFO_SET or through SIOCSHWTSTAMP by
  expanding again the struct hwtstamp_config.

Do you think this is feasible?
I might miss some core stuff.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Tue, 21 Nov 2023 18:31:14 +0100 Köry Maincent wrote:
> - Expand struct hwtstamp_config with a phc_index member for the SIOCG/SHWTSTAMP
>   commands.
>   To keep backward compatibility if phc_index is not set in the hwtstamp_config
>   data from userspace use the default hwtstamp (the default being selected as
>   done in my patch series).
>   Is this possible, would it breaks things?

I'd skip this bit, and focus on the ETHTOOL_TSINFO. Keep the ioctl as
"legacy" and do all the extensions in ethtool. TSINFO_GET can serve
as GET, to avoid adding 3rd command for the same thing. TSINFO_SET
would be new (as you indicate below).

> - In netlink part, send one netlink tsinfo skb for each phc_index.

phc_index and netdev combination. A DO command can only generate one
answer (or rather, it should generate only one answer, there are few
hard rules in netlink). So we need to move that functionality to DUMP.
We can filter the DUMP based on user-provided ifindex and/or phc_index.

> Could be done in a later patch series:
> - Expand netlink TSINFO with ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER.
>   Describing this struct:
> enum ethtool_hwstamp_provider_qualifier {
>  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PRECISE,
>  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_APPROX,
> }; 
> 
>   Set the desired qualifier through TSINFO_SET or through SIOCSHWTSTAMP by
>   expanding again the struct hwtstamp_config.
> 
> Do you think this is feasible?

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Richard Cochran]

On Tue, Nov 21, 2023 at 06:31:14PM +0100, Köry Maincent wrote:
> If I summarize, a solution could be this:
> 
> - Expand struct hwtstamp_config with a phc_index member for the SIOCG/SHWTSTAMP
>   commands.
>   To keep backward compatibility if phc_index is not set in the hwtstamp_config
>   data from userspace use the default hwtstamp (the default being selected as
>   done in my patch series).
>   Is this possible, would it breaks things?

You can't "expand" hwtstamp_config because it is an established ABI.

(You could introduce SIOC[GS]HWTSTAMP_2 with an expanded layout)

Thanks,
Richard

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Tue, 21 Nov 2023 09:43:54 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 21 Nov 2023 18:31:14 +0100 Köry Maincent wrote:
> > - Expand struct hwtstamp_config with a phc_index member for the
> > SIOCG/SHWTSTAMP commands.
> >   To keep backward compatibility if phc_index is not set in the
> > hwtstamp_config data from userspace use the default hwtstamp (the default
> > being selected as done in my patch series).
> >   Is this possible, would it breaks things?  
> 
> I'd skip this bit, and focus on the ETHTOOL_TSINFO. Keep the ioctl as
> "legacy" and do all the extensions in ethtool. TSINFO_GET can serve
> as GET, to avoid adding 3rd command for the same thing. TSINFO_SET
> would be new (as you indicate below).

You say this patch series should simply add TSINFO_SET command to set the
current phc_index?

It won't solve your requirement of having simultaneous hwtimestamp and
enabling/disabling them through rx_filter and tx_types.
You want to do this in another patch series alongside a new SIOCG/SHWTSTAMP_2
ABI?

> > - In netlink part, send one netlink tsinfo skb for each phc_index.  
> 
> phc_index and netdev combination. A DO command can only generate one
> answer (or rather, it should generate only one answer, there are few
> hard rules in netlink). So we need to move that functionality to DUMP.
> We can filter the DUMP based on user-provided ifindex and/or phc_index.

Currently, the dumpit function is assigned to ethnl_default_dumpit. Wouldn't
the behavior change of the dumpit callback break the ABI?


> > Could be done in a later patch series:
> > - Expand netlink TSINFO with ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER.
> >   Describing this struct:
> > enum ethtool_hwstamp_provider_qualifier {
> >  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PRECISE,
> >  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_APPROX,
> > }; 
> > 
> >   Set the desired qualifier through TSINFO_SET or through SIOCSHWTSTAMP by
> >   expanding again the struct hwtstamp_config.

Just wondering to have a insight of future support, in the case of several
provider qualifier and the SIOCG/SHWTSTAMP_2 layout containing the phc_index.
Will we be able to talk to the two providers qualifiers simultaneously or is it
not possible. To know if the SIOCG/SHWTSTAMP_2 layout would contain the
description of the qualifier provider.
If I understand well your mail in the thread it will be the case right?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Wed, Nov 22, 2023 at 02:44:53PM +0100, Köry Maincent wrote:
> On Tue, 21 Nov 2023 09:43:54 -0800
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Tue, 21 Nov 2023 18:31:14 +0100 Köry Maincent wrote:
> > > - Expand struct hwtstamp_config with a phc_index member for the
> > > SIOCG/SHWTSTAMP commands.
> > >   To keep backward compatibility if phc_index is not set in the
> > > hwtstamp_config data from userspace use the default hwtstamp (the default
> > > being selected as done in my patch series).
> > >   Is this possible, would it breaks things?  
> > 
> > I'd skip this bit, and focus on the ETHTOOL_TSINFO. Keep the ioctl as
> > "legacy" and do all the extensions in ethtool. TSINFO_GET can serve
> > as GET, to avoid adding 3rd command for the same thing. TSINFO_SET
> > would be new (as you indicate below).
> 
> You say this patch series should simply add TSINFO_SET command to set the
> current phc_index?
> 
> It won't solve your requirement of having simultaneous hwtimestamp and
> enabling/disabling them through rx_filter and tx_types.
> You want to do this in another patch series alongside a new SIOCG/SHWTSTAMP_2
> ABI?
> 
> > > - In netlink part, send one netlink tsinfo skb for each phc_index.  
> > 
> > phc_index and netdev combination. A DO command can only generate one
> > answer (or rather, it should generate only one answer, there are few
> > hard rules in netlink). So we need to move that functionality to DUMP.
> > We can filter the DUMP based on user-provided ifindex and/or phc_index.
> 
> Currently, the dumpit function is assigned to ethnl_default_dumpit. Wouldn't
> the behavior change of the dumpit callback break the ABI?
> 
> 
> > > Could be done in a later patch series:
> > > - Expand netlink TSINFO with ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER.
> > >   Describing this struct:
> > > enum ethtool_hwstamp_provider_qualifier {
> > >  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PRECISE,
> > >  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_APPROX,
> > > }; 
> > > 
> > >   Set the desired qualifier through TSINFO_SET or through SIOCSHWTSTAMP by
> > >   expanding again the struct hwtstamp_config.
> 
> Just wondering to have a insight of future support, in the case of several
> provider qualifier and the SIOCG/SHWTSTAMP_2 layout containing the phc_index.
> Will we be able to talk to the two providers qualifiers simultaneously or is it
> not possible. To know if the SIOCG/SHWTSTAMP_2 layout would contain the
> description of the qualifier provider.
> If I understand well your mail in the thread it will be the case right?
> 
> Regards,
> -- 
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com/

My understanding of Jakub's email was that he wants to see the functionality
offered by SIOCGHWTSTAMP and SIOCSHWTSTAMP converted to netlink. I don't
think that ethtool is the correct netlink family for that, given that
these aren't ethtool ioctls to begin with. Maybe the new netdev netlink
family. The conversion in its basic form would offer exactly the same
functionality. The extended netlink messages would have extra attributes
to identify the targeted hwtstamp provider. In the lack of those
attributes, the default hwtstamp provider is targeted. The definition of
the default hwtstamp provider should be as per your current patch set
(netdev, with a whitelist for current phylib PHYs).

The _listing_ of hwtstamp providers is what could be done through ethtool
netlink, similar but not identical to the way in which you are proposing
today (you are presenting blanket "layers" which correspond to netdev and
phylib, rather than individual providers).

The concept of an "active phc_index" would not explicitly exist in the
UAPI. Thus I'm not sure what's with this TSINFO_SET being floated around.
The only thing would exist is a configurable rx_filter and tx_type per
hwtstamp provider (aka "{phc_index, qualifier}"). User space will have
to learn to select the hwtstamp provider it wants to configure through
netlink, and use for its class of traffic.

This is why I mentioned by ndo_hwtstamp_set() conversion, because
suddenly it is a prerequisite for any further progress to be done.
You can't convert SIOCSHWTSTAMP to netlink if there are some driver
implementations which still use ndo_eth_ioctl(). They need to be
UAPI-agnostic.

I'm not sure what's with Richard's mention of the "_2" variants of the
ioctls. Probably a low-effort suggestion which was a bit out of context.
His main point, that you cannot extend struct hwtstamp_config as that
has a fixed binary format, is perfectly valid though. This is why
netlink is preferable, because if done correctly (meaning not with
NLA_BINARY attributes), then it is much more extensible because all
attributes are TLVs. Use NLA_BINARY, and you will run into the exact
extensibility issues that the ioctl interface has.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Wed, Nov 22, 2023 at 04:08:50PM +0200, Vladimir Oltean wrote:
> The concept of an "active phc_index" would not explicitly exist in the
> UAPI. Thus I'm not sure what's with this TSINFO_SET being floated around.
> The only thing would exist is a configurable rx_filter and tx_type per
> hwtstamp provider (aka "{phc_index, qualifier}"). User space will have
> to learn to select the hwtstamp provider it wants to configure through
> netlink, and use for its class of traffic.

One clarification: the extended timestamping filters are PER NETDEV
(in addition to being per one of the hwtstamp providers listed for that
netdev). This was understated from the fact that the netlink interface
itself targets a netdev, but I didn't say it explicitly.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Wed, Nov 22, 2023 at 04:08:50PM +0200, Vladimir Oltean wrote:
> The concept of an "active phc_index" would not explicitly exist in the
> UAPI. Thus I'm not sure what's with this TSINFO_SET being floated around.
> The only thing would exist is a configurable rx_filter and tx_type per
> hwtstamp provider (aka "{phc_index, qualifier}"). User space will have
> to learn to select the hwtstamp provider it wants to configure through
> netlink, and use for its class of traffic.

@Jakub, for your long-term "MAC timestamps for PTP, DMA for everything else".
How do you see this? I guess we need some sort of priority function in
the UAPI between hwtstamp providers.

And even with that, I think the enums that we currently have for filters
are not specific enough. The most we could expose is:

                      MAC provider                      DMA provider

hwtstamp_rx_filters   HWTSTAMP_FILTER_PTP_V2_EVENT      HWTSTAMP_FILTER_ALL
tx_type               HWTSTAMP_TX_ON                    HWTSTAMP_TX_ON

but it isn't clear: for PTP, does the DMA provider give you an RX
timestamp too? What about a TX timestamp?

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Wed, 22 Nov 2023 16:08:50 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Wed, Nov 22, 2023 at 02:44:53PM +0100, Köry Maincent wrote:
> > On Tue, 21 Nov 2023 09:43:54 -0800
> > Jakub Kicinski <kuba@kernel.org> wrote:
> >   
> > > On Tue, 21 Nov 2023 18:31:14 +0100 Köry Maincent wrote:  
> > > > - Expand struct hwtstamp_config with a phc_index member for the
> > > > SIOCG/SHWTSTAMP commands.
> > > >   To keep backward compatibility if phc_index is not set in the
> > > > hwtstamp_config data from userspace use the default hwtstamp (the
> > > > default being selected as done in my patch series).
> > > >   Is this possible, would it breaks things?    
> > > 
> > > I'd skip this bit, and focus on the ETHTOOL_TSINFO. Keep the ioctl as
> > > "legacy" and do all the extensions in ethtool. TSINFO_GET can serve
> > > as GET, to avoid adding 3rd command for the same thing. TSINFO_SET
> > > would be new (as you indicate below).  
> > 
> > You say this patch series should simply add TSINFO_SET command to set the
> > current phc_index?
> > 
> > It won't solve your requirement of having simultaneous hwtimestamp and
> > enabling/disabling them through rx_filter and tx_types.
> > You want to do this in another patch series alongside a new
> > SIOCG/SHWTSTAMP_2 ABI?
> >   
> > > > - In netlink part, send one netlink tsinfo skb for each phc_index.    
> > > 
> > > phc_index and netdev combination. A DO command can only generate one
> > > answer (or rather, it should generate only one answer, there are few
> > > hard rules in netlink). So we need to move that functionality to DUMP.
> > > We can filter the DUMP based on user-provided ifindex and/or phc_index.  
> > 
> > Currently, the dumpit function is assigned to ethnl_default_dumpit. Wouldn't
> > the behavior change of the dumpit callback break the ABI?
> > 
> >   
> > > > Could be done in a later patch series:
> > > > - Expand netlink TSINFO with
> > > > ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER. Describing this struct:
> > > > enum ethtool_hwstamp_provider_qualifier {
> > > >  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PRECISE,
> > > >  	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_APPROX,
> > > > }; 
> > > > 
> > > >   Set the desired qualifier through TSINFO_SET or through SIOCSHWTSTAMP
> > > > by expanding again the struct hwtstamp_config.  
> > 
> > Just wondering to have a insight of future support, in the case of several
> > provider qualifier and the SIOCG/SHWTSTAMP_2 layout containing the
> > phc_index. Will we be able to talk to the two providers qualifiers
> > simultaneously or is it not possible. To know if the SIOCG/SHWTSTAMP_2
> > layout would contain the description of the qualifier provider.
> > If I understand well your mail in the thread it will be the case right?

> My understanding of Jakub's email was that he wants to see the functionality
> offered by SIOCGHWTSTAMP and SIOCSHWTSTAMP converted to netlink. I don't
> think that ethtool is the correct netlink family for that, given that
> these aren't ethtool ioctls to begin with. Maybe the new netdev netlink
> family. The conversion in its basic form would offer exactly the same
> functionality. The extended netlink messages would have extra attributes
> to identify the targeted hwtstamp provider. In the lack of those
> attributes, the default hwtstamp provider is targeted. The definition of
> the default hwtstamp provider should be as per your current patch set
> (netdev, with a whitelist for current phylib PHYs).
> 
> The _listing_ of hwtstamp providers is what could be done through ethtool
> netlink, similar but not identical to the way in which you are proposing
> today (you are presenting blanket "layers" which correspond to netdev and
> phylib, rather than individual providers).
> 
> The concept of an "active phc_index" would not explicitly exist in the
> UAPI. Thus I'm not sure what's with this TSINFO_SET being floated around.
> The only thing would exist is a configurable rx_filter and tx_type per
> hwtstamp provider (aka "{phc_index, qualifier}"). User space will have
> to learn to select the hwtstamp provider it wants to configure through
> netlink, and use for its class of traffic.
> 
> This is why I mentioned by ndo_hwtstamp_set() conversion, because
> suddenly it is a prerequisite for any further progress to be done.
> You can't convert SIOCSHWTSTAMP to netlink if there are some driver
> implementations which still use ndo_eth_ioctl(). They need to be
> UAPI-agnostic.
> 
> I'm not sure what's with Richard's mention of the "_2" variants of the
> ioctls. Probably a low-effort suggestion which was a bit out of context.
> His main point, that you cannot extend struct hwtstamp_config as that
> has a fixed binary format, is perfectly valid though. This is why
> netlink is preferable, because if done correctly (meaning not with
> NLA_BINARY attributes), then it is much more extensible because all
> attributes are TLVs. Use NLA_BINARY, and you will run into the exact
> extensibility issues that the ioctl interface has.

I appreciate the clarification, it now makes more sense to me.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Wed, 22 Nov 2023 16:08:50 +0200 Vladimir Oltean wrote:
> My understanding of Jakub's email was that he wants to see the functionality
> offered by SIOCGHWTSTAMP and SIOCSHWTSTAMP converted to netlink. I don't
> think that ethtool is the correct netlink family for that, given that
> these aren't ethtool ioctls to begin with. Maybe the new netdev netlink
> family. The conversion in its basic form would offer exactly the same
> functionality.

Well, ethtool has been the catch all for a lot of random things
for the longest time. The question is whether we want to extend
ETHTOOL_GET_TS_INFO or add a third API somewhere else. And if we
do - do we also duplicate the functionality of ETHTOOL_GET_TS_INFO
(i.e. getting capabilities)?

My vote is that keeping it in ethtool is less bad than 3rd API.

> The _listing_ of hwtstamp providers is what could be done through ethtool
> netlink, similar but not identical to the way in which you are proposing
> today (you are presenting blanket "layers" which correspond to netdev and
> phylib, rather than individual providers).
> 
> The concept of an "active phc_index" would not explicitly exist in the
> UAPI. Thus I'm not sure what's with this TSINFO_SET being floated around.
> The only thing would exist is a configurable rx_filter and tx_type per
> hwtstamp provider (aka "{phc_index, qualifier}"). User space will have
> to learn to select the hwtstamp provider it wants to configure through
> netlink, and use for its class of traffic.

"Active provider" is the one that has TX_ON, rx != FILTER_NONE, right?

> This is why I mentioned by ndo_hwtstamp_set() conversion, because
> suddenly it is a prerequisite for any further progress to be done.
> You can't convert SIOCSHWTSTAMP to netlink if there are some driver
> implementations which still use ndo_eth_ioctl(). They need to be
> UAPI-agnostic.

Right, definitely.

> I'm not sure what's with Richard's mention of the "_2" variants of the
> ioctls. Probably a low-effort suggestion which was a bit out of context.
> His main point, that you cannot extend struct hwtstamp_config as that
> has a fixed binary format, is perfectly valid though. This is why
> netlink is preferable, because if done correctly (meaning not with
> NLA_BINARY attributes), then it is much more extensible because all
> attributes are TLVs. Use NLA_BINARY, and you will run into the exact
> extensibility issues that the ioctl interface has.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Wed, 22 Nov 2023 16:36:18 +0200 Vladimir Oltean wrote:
> @Jakub, for your long-term "MAC timestamps for PTP, DMA for everything else".
> How do you see this? I guess we need some sort of priority function in
> the UAPI between hwtstamp providers.
> 
> And even with that, I think the enums that we currently have for filters
> are not specific enough. The most we could expose is:
> 
>                       MAC provider                      DMA provider
> 
> hwtstamp_rx_filters   HWTSTAMP_FILTER_PTP_V2_EVENT      HWTSTAMP_FILTER_ALL
> tx_type               HWTSTAMP_TX_ON                    HWTSTAMP_TX_ON
> 
> but it isn't clear: for PTP, does the DMA provider give you an RX
> timestamp too?

If we phrase it as "precise / approximate" rather than "MAC / DMA" - it
seems fairly intuitive to give the best timestamp available for a given
packet, no?

> What about a TX timestamp?

I was thinking - socket flag to make packets for a given socket request
precise timestamps.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Wed, Nov 22, 2023 at 08:50:00AM -0800, Jakub Kicinski wrote:
> On Wed, 22 Nov 2023 16:08:50 +0200 Vladimir Oltean wrote:
> > My understanding of Jakub's email was that he wants to see the functionality
> > offered by SIOCGHWTSTAMP and SIOCSHWTSTAMP converted to netlink. I don't
> > think that ethtool is the correct netlink family for that, given that
> > these aren't ethtool ioctls to begin with. Maybe the new netdev netlink
> > family. The conversion in its basic form would offer exactly the same
> > functionality.
> 
> Well, ethtool has been the catch all for a lot of random things
> for the longest time. The question is whether we want to extend
> ETHTOOL_GET_TS_INFO or add a third API somewhere else. And if we
> do - do we also duplicate the functionality of ETHTOOL_GET_TS_INFO
> (i.e. getting capabilities)?
> 
> My vote is that keeping it in ethtool is less bad than 3rd API.

With SIOCSHWTSTAMP also implemented by CAN (and presumably also by
wireless in the future), I do wonder whether ethtool is the right place
for the netlink conversion.

I wouldn't suggest duplicating ETHTOOL_GET_TS_INFO towards the netdev
netlink family.

> > The concept of an "active phc_index" would not explicitly exist in the
> > UAPI. Thus I'm not sure what's with this TSINFO_SET being floated around.
> > The only thing would exist is a configurable rx_filter and tx_type per
> > hwtstamp provider (aka "{phc_index, qualifier}"). User space will have
> > to learn to select the hwtstamp provider it wants to configure through
> > netlink, and use for its class of traffic.
> 
> "Active provider" is the one that has TX_ON, rx != FILTER_NONE, right?

In the "implicit" definition of an "active hwtstamp provider", yes.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Wed, Nov 22, 2023 at 08:54:59AM -0800, Jakub Kicinski wrote:
> On Wed, 22 Nov 2023 16:36:18 +0200 Vladimir Oltean wrote:
> > @Jakub, for your long-term "MAC timestamps for PTP, DMA for everything else".
> > How do you see this? I guess we need some sort of priority function in
> > the UAPI between hwtstamp providers.
> > 
> > And even with that, I think the enums that we currently have for filters
> > are not specific enough. The most we could expose is:
> > 
> >                       MAC provider                      DMA provider
> > 
> > hwtstamp_rx_filters   HWTSTAMP_FILTER_PTP_V2_EVENT      HWTSTAMP_FILTER_ALL
> > tx_type               HWTSTAMP_TX_ON                    HWTSTAMP_TX_ON
> > 
> > but it isn't clear: for PTP, does the DMA provider give you an RX
> > timestamp too?
> 
> If we phrase it as "precise / approximate" rather than "MAC / DMA" - it
> seems fairly intuitive to give the best timestamp available for a given
> packet, no?

I wouldn't be so sure. The alternative interpretation "for PTP, give me
timestamps from both sources" also sounds reasonable for the distant
future where that will be possible (with proper cmsg identification).
But I don't see how to distinguish the two - the filters, expressed in
these terms, would be the same.

> > What about a TX timestamp?
> 
> I was thinking - socket flag to make packets for a given socket request
> precise timestamps.

So the ptp4l source code would have to be modified to still work with
the same precision as before? I'm not seeing this through.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Wed, 22 Nov 2023 18:59:55 +0200 Vladimir Oltean wrote:
> I wouldn't be so sure. The alternative interpretation "for PTP, give me
> timestamps from both sources" also sounds reasonable for the distant
> future where that will be possible (with proper cmsg identification).
> But I don't see how to distinguish the two - the filters, expressed in
> these terms, would be the same.

We can add an attribute that explicitly says that the configuration
is only requesting one stamp. But feels like jumping the gun at this
stage, given we have no other option to express there.

> So the ptp4l source code would have to be modified to still work with
> the same precision as before? I'm not seeing this through.

We can do the opposite and add a socket flag which says "DMA is okay".

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Wed, 22 Nov 2023 18:55:17 +0200 Vladimir Oltean wrote:
> > Well, ethtool has been the catch all for a lot of random things
> > for the longest time. The question is whether we want to extend
> > ETHTOOL_GET_TS_INFO or add a third API somewhere else. And if we
> > do - do we also duplicate the functionality of ETHTOOL_GET_TS_INFO
> > (i.e. getting capabilities)?
> > 
> > My vote is that keeping it in ethtool is less bad than 3rd API.  
> 
> With SIOCSHWTSTAMP also implemented by CAN (and presumably also by
> wireless in the future), I do wonder whether ethtool is the right place
> for the netlink conversion.

ethtool currently provides the only way we have to configure ring
length, ring count, RSS, UDP tunnels etc.

It's a matter of taste, IMO ethtool is a bit of a lost cause already
and keeping things together (ethtool already has TS_INFO) is cleaner
than spreading them around.

> I wouldn't suggest duplicating ETHTOOL_GET_TS_INFO towards the netdev
> netlink family.

FTR so far the netdev family is all about SW configuration. We should
probably keep it that way, so it doesn't become ginormous. It's easy
enough to create a new family, if needed.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Willem de Bruijn]

On Wed, Nov 22, 2023 at 12:55 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 22 Nov 2023 18:59:55 +0200 Vladimir Oltean wrote:
> > I wouldn't be so sure. The alternative interpretation "for PTP, give me
> > timestamps from both sources" also sounds reasonable for the distant
> > future where that will be possible (with proper cmsg identification).
> > But I don't see how to distinguish the two - the filters, expressed in
> > these terms, would be the same.
>
> We can add an attribute that explicitly says that the configuration
> is only requesting one stamp. But feels like jumping the gun at this
> stage, given we have no other option to express there.
>
> > So the ptp4l source code would have to be modified to still work with
> > the same precision as before? I'm not seeing this through.
>
> We can do the opposite and add a socket flag which says "DMA is okay".

There already is a disconnect between configuring hardware timestamp
generation. Through the ioctl, which is a global admin-only interface.
And requesting timestamps with SO_TIMESTAMPING.

Today the user of ptp4l already has to know that the admin has
configured the right RX and TX filters. That is no different if
multiple filters can be installed? (PHY for PTP, DMA for everything
else).

If attribution becomes important, we could add another cmsg alongside
the timestamp. On TX this already happens with
IP_RECVERR/IPV6_RECVERR/PACKET_TX_TIMESTAMP. Maybe the
sock_extended_err struct even still has a field that can be (ab)used
for this purpose.

Being able to pass multiple timestamps up to userspace eventually will
be interesting. A large blocker is where to store these values in the
sk_buff on the path between the driver and the socket (skb_ext?). At
Google we already have this scenario, where the local TCP stack and
userspace both want converted hardware timestamps -- but converted
from raw to different timebases.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Wed, 22 Nov 2023 10:01:42 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Wed, 22 Nov 2023 18:55:17 +0200 Vladimir Oltean wrote:
> > > Well, ethtool has been the catch all for a lot of random things
> > > for the longest time. The question is whether we want to extend
> > > ETHTOOL_GET_TS_INFO or add a third API somewhere else. And if we
> > > do - do we also duplicate the functionality of ETHTOOL_GET_TS_INFO
> > > (i.e. getting capabilities)?
> > > 
> > > My vote is that keeping it in ethtool is less bad than 3rd API.    
> > 
> > With SIOCSHWTSTAMP also implemented by CAN (and presumably also by
> > wireless in the future), I do wonder whether ethtool is the right place
> > for the netlink conversion.  
> 
> ethtool currently provides the only way we have to configure ring
> length, ring count, RSS, UDP tunnels etc.
> 
> It's a matter of taste, IMO ethtool is a bit of a lost cause already
> and keeping things together (ethtool already has TS_INFO) is cleaner
> than spreading them around.
> 
> > I wouldn't suggest duplicating ETHTOOL_GET_TS_INFO towards the netdev
> > netlink family.  
> 
> FTR so far the netdev family is all about SW configuration. We should
> probably keep it that way, so it doesn't become ginormous. It's easy
> enough to create a new family, if needed.

So, do we have a consensus? Vlad, do you agree on putting all under ethtool?

ETHTOOL_GET_TS_INFO will be in charge of replacing the SIOCGHWSTAMP
implementation. Need to add ETHTOOL_A_TSINFO_PHC_INDEX
ETHTOOL_A_TSINFO_QUALIFIER to the request.

ETHTOOL_GET_TS_INFO will list all the hwtstamp provider (aka "{phc_index,
qualifier}") through the dumpit callback. I will add a filter to be able to
list only the hwtstamp provider of one netdev.

ETHTOOL_SET_TS_INFO will be in charge of replacing the SIOCSHWSTAMP
implementation.

Is that ok for you?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Thu, 23 Nov 2023 16:00:56 +0100 Köry Maincent wrote:
> > FTR so far the netdev family is all about SW configuration. We should
> > probably keep it that way, so it doesn't become ginormous. It's easy
> > enough to create a new family, if needed.  
> 
> So, do we have a consensus? Vlad, do you agree on putting all under ethtool?

If not we can do a vote/poll? Maybe others don't find the configuration
of timestamping as confusing as me.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Thu, Nov 23, 2023 at 09:32:05AM -0800, Jakub Kicinski wrote:
> On Thu, Nov 23, 2023 at 04:00:56PM +0100, Köry Maincent wrote:
> > So, do we have a consensus? Vlad, do you agree on putting all under ethtool?
> > 
> > ETHTOOL_GET_TS_INFO will be in charge of replacing the SIOCGHWSTAMP
> > implementation. Need to add ETHTOOL_A_TSINFO_PHC_INDEX
> > ETHTOOL_A_TSINFO_QUALIFIER to the request.
> > 
> > ETHTOOL_GET_TS_INFO will list all the hwtstamp provider (aka "{phc_index,
> > qualifier}") through the dumpit callback. I will add a filter to be able to
> > list only the hwtstamp provider of one netdev.
> > 
> > ETHTOOL_SET_TS_INFO will be in charge of replacing the SIOCSHWSTAMP
> > implementation.
> 
> If not we can do a vote/poll? Maybe others don't find the configuration
> of timestamping as confusing as me.

If you mean the ETHTOOL_MSG_TSINFO_GET netlink message (ETHTOOL_GET_TS_INFO
is an ioctl), you're saying that you want to move the entire contents of
SIOCGHWSTAMP there, by making the kernel call ndo_hwtstamp_get() in
addition to the existing __ethtool_get_ts_info()?

Yeah, I don't know, I don't have a real objection, I guess it's fine.

What will be a bit of an "?!" moment for users is when ethtool gains
support for the SIOCGHWSTAMP/SIOCSHWSTAMP netlink replacements, but not
for the original ioctls. So hwstamp_ctl will be able to change timestamping
configuration, but ethtool wouldn't - all on the same system. Unless
ethtool gains an ioctl fallback for a ioctl that was never down its alley.

But by all means, still hold a poll if you want to. I would vote for
ethtool netlink, not because it's great, just because I don't have a
better alternative to propose.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

Hi Willem,

On Wed, Nov 22, 2023 at 01:11:02PM -0500, Willem de Bruijn wrote:
> There already is a disconnect between configuring hardware timestamp
> generation. Through the ioctl, which is a global admin-only interface.
> And requesting timestamps with SO_TIMESTAMPING.
> 
> Today the user of ptp4l already has to know that the admin has
> configured the right RX and TX filters. That is no different if
> multiple filters can be installed? (PHY for PTP, DMA for everything
> else).

Are you saying that ptp4l doesn't configure the RX and TX filters by
itself, just the admin had to do that? Because it does.
https://github.com/richardcochran/linuxptp/blob/master/sk.c#L59

I'm not seeing the disconnect. SO_TIMESTAMPING is for the socket,
SIOCSHWTSTAMP is for the configuration at the device level.

It _is_ different if multiple filters can be installed, because either
we let things be (and ptp4l issues the same ioctl which affects the
default hwtstamp provider, which may or may not coincide with what we
intend), or we teach ptp4l to deal with the multitude of providers that
a port may have.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Fri, 24 Nov 2023 17:43:43 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Thu, Nov 23, 2023 at 09:32:05AM -0800, Jakub Kicinski wrote:
> > On Thu, Nov 23, 2023 at 04:00:56PM +0100, Köry Maincent wrote:  
> > > So, do we have a consensus? Vlad, do you agree on putting all under
> > > ethtool?
> > > 
> > > ETHTOOL_GET_TS_INFO will be in charge of replacing the SIOCGHWSTAMP
> > > implementation. Need to add ETHTOOL_A_TSINFO_PHC_INDEX
> > > ETHTOOL_A_TSINFO_QUALIFIER to the request.
> > > 
> > > ETHTOOL_GET_TS_INFO will list all the hwtstamp provider (aka "{phc_index,
> > > qualifier}") through the dumpit callback. I will add a filter to be able
> > > to list only the hwtstamp provider of one netdev.
> > > 
> > > ETHTOOL_SET_TS_INFO will be in charge of replacing the SIOCSHWSTAMP
> > > implementation.  
> > 
> > If not we can do a vote/poll? Maybe others don't find the configuration
> > of timestamping as confusing as me.  
> 
> If you mean the ETHTOOL_MSG_TSINFO_GET netlink message (ETHTOOL_GET_TS_INFO
> is an ioctl), you're saying that you want to move the entire contents of
> SIOCGHWSTAMP there, by making the kernel call ndo_hwtstamp_get() in
> addition to the existing __ethtool_get_ts_info()?

Yes.
 
> Yeah, I don't know, I don't have a real objection, I guess it's fine.
> 
> What will be a bit of an "?!" moment for users is when ethtool gains
> support for the SIOCGHWSTAMP/SIOCSHWSTAMP netlink replacements, but not
> for the original ioctls. So hwstamp_ctl will be able to change timestamping
> configuration, but ethtool wouldn't - all on the same system. Unless
> ethtool gains an ioctl fallback for a ioctl that was never down its alley.

Yes indeed. Would it break things if both ioctls and netlink can get and set
the hwtstamps configuration? It is only configuration. Both happen under
rtnl_lock it should be alright.

The question is which hwtstamp provider will the original ioctls be able to
change? Maybe the default one (MAC with phy whitelist) and only this one.

> But by all means, still hold a poll if you want to. I would vote for
> ethtool netlink, not because it's great, just because I don't have a
> better alternative to propose.

If you agree on that choice, let's go. Jakub and your are the most proactive
reviewers in this patch series. Willem you are the timestamping maintainer do
you also agree on this? 
If anyone have another proposition let them speak now, or forever remain
silent! ;)

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Willem de Bruijn]

On Fri, Nov 24, 2023 at 12:28 PM Vladimir Oltean
<vladimir.oltean@nxp.com> wrote:
>
> Hi Willem,
>
> On Wed, Nov 22, 2023 at 01:11:02PM -0500, Willem de Bruijn wrote:
> > There already is a disconnect between configuring hardware timestamp
> > generation. Through the ioctl, which is a global admin-only interface.
> > And requesting timestamps with SO_TIMESTAMPING.
> >
> > Today the user of ptp4l already has to know that the admin has
> > configured the right RX and TX filters. That is no different if
> > multiple filters can be installed? (PHY for PTP, DMA for everything
> > else).
>
> Are you saying that ptp4l doesn't configure the RX and TX filters by
> itself, just the admin had to do that? Because it does.
> https://github.com/richardcochran/linuxptp/blob/master/sk.c#L59
>
> I'm not seeing the disconnect. SO_TIMESTAMPING is for the socket,
> SIOCSHWTSTAMP is for the configuration at the device level.
>
> It _is_ different if multiple filters can be installed, because either
> we let things be (and ptp4l issues the same ioctl which affects the
> default hwtstamp provider, which may or may not coincide with what we
> intend), or we teach ptp4l to deal with the multitude of providers that
> a port may have.

I see. By disconnect, I meant that the socket option is unprivileged and
can be set by many processes, while the ioctl is a global privileged
setting, so must be under control of a single admin.

But I did not know that ptp4l can take on both those roles for PTP.

Perhaps multiple SIOCSHWTSTAMP rules can coexist, up to
one per level:

HWTSTAMP_FILTER_PTP_V2_EVENT, level=PHY
HWTSTAMP_FILTER_ALL, level, level=DMA

Then ptp4l can manage all levels except the DMA level. And DMA
timestamps can be configured independently by another admin.

If only one timestamp can be communicated to the host, the earliest
match must takes precedence. Jakub pointed out how one device
handles this by having a separate queue for PHY  timestamped
packets.

This does not address the issue that packets with different
precision skb_shinfo(skb)->hwtstamps->hwtstamp may now exist
in the system. All packets reaching ptp4l sockets must have a high
resolution source, but there is no explicit annotation to ensure or
check this. This is fully based on trusting the HWSTAMP_FILTER.
Expanding the skb infra and cmsg might be follow-on work.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Fri, Nov 24, 2023 at 06:34:31PM +0100, Köry Maincent wrote:
> Would it break things if both ioctls and netlink can get and set the
> hwtstamps configuration?

Uhm, obviously? It would break things if ioctl and netlink were _not_
freely interchangeable, and you couldn't see in a ioctl GET what got set
through a netlink SET.

> It is only configuration. Both happen under rtnl_lock it should be
> alright.

Yeah, but you always need to keep the API interchangeability in mind
during the implementation.

> The question is which hwtstamp provider will the original ioctls be able to
> change? Maybe the default one (MAC with phy whitelist) and only this one.

TL;DR: yeah.

Remember one single rule and go from there: new development should not
change established setups. So SIOCSHWSTAMPs should continue to behave
"as before".

This is also the exact reason why I asked for the phy whitelist. The
introduction of CONFIG_NETWORK_PHY_TIMESTAMPING introduced exactly that:
a breaking change in the mode in which deployed setups operate.

> > But by all means, still hold a poll if you want to. I would vote for
> > ethtool netlink, not because it's great, just because I don't have a
> > better alternative to propose.
> 
> If you agree on that choice, let's go. Jakub and your are the most proactive
> reviewers in this patch series. Willem you are the timestamping maintainer do
> you also agree on this? 
> If anyone have another proposition let them speak now, or forever remain
> silent! ;)

Hmm, proactive means doing stuff in anticipation of being requested to
do it. I'd use the work "active" at most...

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Willem de Bruijn]

On Fri, Nov 24, 2023 at 12:34 PM Köry Maincent
<kory.maincent@bootlin.com> wrote:
>
> On Fri, 24 Nov 2023 17:43:43 +0200
> Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> > On Thu, Nov 23, 2023 at 09:32:05AM -0800, Jakub Kicinski wrote:
> > > On Thu, Nov 23, 2023 at 04:00:56PM +0100, Köry Maincent wrote:
> > > > So, do we have a consensus? Vlad, do you agree on putting all under
> > > > ethtool?
> > > >
> > > > ETHTOOL_GET_TS_INFO will be in charge of replacing the SIOCGHWSTAMP
> > > > implementation. Need to add ETHTOOL_A_TSINFO_PHC_INDEX
> > > > ETHTOOL_A_TSINFO_QUALIFIER to the request.
> > > >
> > > > ETHTOOL_GET_TS_INFO will list all the hwtstamp provider (aka "{phc_index,
> > > > qualifier}") through the dumpit callback. I will add a filter to be able
> > > > to list only the hwtstamp provider of one netdev.
> > > >
> > > > ETHTOOL_SET_TS_INFO will be in charge of replacing the SIOCSHWSTAMP
> > > > implementation.
> > >
> > > If not we can do a vote/poll? Maybe others don't find the configuration
> > > of timestamping as confusing as me.
> >
> > If you mean the ETHTOOL_MSG_TSINFO_GET netlink message (ETHTOOL_GET_TS_INFO
> > is an ioctl), you're saying that you want to move the entire contents of
> > SIOCGHWSTAMP there, by making the kernel call ndo_hwtstamp_get() in
> > addition to the existing __ethtool_get_ts_info()?
>
> Yes.
>
> > Yeah, I don't know, I don't have a real objection, I guess it's fine.
> >
> > What will be a bit of an "?!" moment for users is when ethtool gains
> > support for the SIOCGHWSTAMP/SIOCSHWSTAMP netlink replacements, but not
> > for the original ioctls. So hwstamp_ctl will be able to change timestamping
> > configuration, but ethtool wouldn't - all on the same system. Unless
> > ethtool gains an ioctl fallback for a ioctl that was never down its alley.
>
> Yes indeed. Would it break things if both ioctls and netlink can get and set
> the hwtstamps configuration? It is only configuration. Both happen under
> rtnl_lock it should be alright.
>
> The question is which hwtstamp provider will the original ioctls be able to
> change? Maybe the default one (MAC with phy whitelist) and only this one.
>
> > But by all means, still hold a poll if you want to. I would vote for
> > ethtool netlink, not because it's great, just because I don't have a
> > better alternative to propose.
>
> If you agree on that choice, let's go. Jakub and your are the most proactive
> reviewers in this patch series. Willem you are the timestamping maintainer do
> you also agree on this?

I don't have a strong opinion. Ethtool netlink SGTM.

For new network configuration we are moving away from ioctl towards
netlink in general.

Ethtool itself made this move, where the old ioctl way of things
continues to work, but will no longer be extended.

Since one of the APIs we use already uses ethtool, converting the
other two there makes sense to me.

I'm not familiar enough with configuring CAN or wireless to know
whether it would pose a problem for these mentioned cases.

> If anyone have another proposition let them speak now, or forever remain
> silent! ;)

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Richard Cochran]

On Fri, Nov 24, 2023 at 02:45:46PM -0500, Willem de Bruijn wrote:

> Expanding the skb infra and cmsg might be follow-on work.

Yes, and would I suggest limiting the scope of the present series just
to allow changing the global, device-level time stamping layer
administratively.

Trying to support multiple time stamps from different layers is a much
larger development project, and it will require new kernel/user
interfaces.

(Of course it would be grand if the series is forward looking to the
day when time stamp reporting expands beyond the current hw/sw cmsg.)

Thanks,
Richard

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Mon, 20 Nov 2023 13:45:51 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Mon, 20 Nov 2023 21:58:58 +0200 Vladimir Oltean wrote:
> > I'm still waiting for you to fully clarify the "per socket vs global"
> > aspect, but independently of that, at least I understand why this is a
> > counter-argument to my proposal. I need to tune it a bit (ASSUMING that
> > we want DMA timestamps to "look like" hwtimestamps, and not like their
> > own thing, to user space), because the PHC index would no longer fully
> > identify a hwtstamp provider, so we need something more.
> > 
> > I imagine both ETHTOOL_MSG_TSINFO_GET and ETHTOOL_MSG_TSINFO_SET to
> > support a new (nest) nlattr called ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER.
> > 
> > This would contain (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_PHC_INDEX
> > and (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER. It could be
> > extensible in the future, but this is the baseline and forms the key.
> > 
> > The latter takes values from an:
> > 
> > enum ethtool_hwstamp_provider_qualifier {
> > 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_MAC,
> > 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PHY,
> > 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_DMA,
> > };  
> 
> Sounds reasonable. Having more attributes than just PHC index works.
> Given the lack of distinction between MAC and PHY for integrated NICs
> I'd lean towards ditching the "layers" completely and exposing 
> an "approximate" vs "precise" boolean. Approximate being the DMA point
> for NICs, but more generically a point that is separated from the wire
> by buffering or other variable length delay. Precise == IEEE 1588
> quality.

Hello Jakub, just wondering.
I can add this hwtstamp provider qualifier in the next series version but it
won't be used as it is set and used at the driver level and no driver is using
it for now. It would not be accepted if I use something that is not used, right?
Do you still think I should add this in v8?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Vladimir Oltean]

On Wed, Nov 29, 2023 at 09:09:59PM +0100, Köry Maincent wrote:
> On Mon, 20 Nov 2023 13:45:51 -0800
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Mon, 20 Nov 2023 21:58:58 +0200 Vladimir Oltean wrote:
> > > I'm still waiting for you to fully clarify the "per socket vs global"
> > > aspect, but independently of that, at least I understand why this is a
> > > counter-argument to my proposal. I need to tune it a bit (ASSUMING that
> > > we want DMA timestamps to "look like" hwtimestamps, and not like their
> > > own thing, to user space), because the PHC index would no longer fully
> > > identify a hwtstamp provider, so we need something more.
> > > 
> > > I imagine both ETHTOOL_MSG_TSINFO_GET and ETHTOOL_MSG_TSINFO_SET to
> > > support a new (nest) nlattr called ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER.
> > > 
> > > This would contain (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_PHC_INDEX
> > > and (u32) ETHTOOL_A_TSINFO_HWSTAMP_PROVIDER_QUALIFIER. It could be
> > > extensible in the future, but this is the baseline and forms the key.
> > > 
> > > The latter takes values from an:
> > > 
> > > enum ethtool_hwstamp_provider_qualifier {
> > > 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_MAC,
> > > 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_PHY,
> > > 	ETHTOOL_HWSTAMP_PROVIDER_QUALIFIER_DMA,
> > > };  
> > 
> > Sounds reasonable. Having more attributes than just PHC index works.
> > Given the lack of distinction between MAC and PHY for integrated NICs
> > I'd lean towards ditching the "layers" completely and exposing 
> > an "approximate" vs "precise" boolean. Approximate being the DMA point
> > for NICs, but more generically a point that is separated from the wire
> > by buffering or other variable length delay. Precise == IEEE 1588
> > quality.
> 
> Hello Jakub, just wondering.
> I can add this hwtstamp provider qualifier in the next series version but it
> won't be used as it is set and used at the driver level and no driver is using
> it for now. It would not be accepted if I use something that is not used, right?
> Do you still think I should add this in v8?

Not sure why you say "not used", though. Are you not planning to expose
the qualifier as an attribute to the listing of hwtstamp providers
offered to user space by ETHTOOL_MSG_TSINFO_GET?

Personally, I worry that if the qualifier gets added later (not now) to
the UAPI, we will end up having user space software (written now) that
iterates through the provider listing thinking that there may only ever
be one provider offered by one PHC, and will stop at the first such
provider found, whichever that may be.

With the added qualifier, there's a higher chance that user space
searches will be for a {phc, qualifier} pair (even if there will only be
1 possible qualifier type), and the future addition of a new hwtstamp
provider will keep existing software working in the same way as before,
i.e. user space won't select the DMA provider by mistake, by ignoring
the qualifier attribute altogether.

Generally I'm against adding things upfront that can only be in a certain
way, but in this case I believe that it is necessary in order for the
future extensions that were discussed to be possible. The qualifier is
part of the user space search key and thus pretty important.

My 2 cents, Jakub can absolutely disagree.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Wed, 29 Nov 2023 22:37:00 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Wed, Nov 29, 2023 at 09:09:59PM +0100, Köry Maincent wrote:
> > On Mon, 20 Nov 2023 13:45:51 -0800
> > Jakub Kicinski <kuba@kernel.org> wrote:

> > > Sounds reasonable. Having more attributes than just PHC index works.
> > > Given the lack of distinction between MAC and PHY for integrated NICs
> > > I'd lean towards ditching the "layers" completely and exposing 
> > > an "approximate" vs "precise" boolean. Approximate being the DMA point
> > > for NICs, but more generically a point that is separated from the wire
> > > by buffering or other variable length delay. Precise == IEEE 1588
> > > quality.  
> > 
> > Hello Jakub, just wondering.
> > I can add this hwtstamp provider qualifier in the next series version but it
> > won't be used as it is set and used at the driver level and no driver is
> > using it for now. It would not be accepted if I use something that is not
> > used, right? Do you still think I should add this in v8?  
> 
> Not sure why you say "not used", though. Are you not planning to expose
> the qualifier as an attribute to the listing of hwtstamp providers
> offered to user space by ETHTOOL_MSG_TSINFO_GET?

Yes I will, I was just saying that all the PHC would be set as precise for now.
Approximate timestamp quality won't be used because IIUC there are no NIC driver
supporting it yet.

> Personally, I worry that if the qualifier gets added later (not now) to
> the UAPI, we will end up having user space software (written now) that
> iterates through the provider listing thinking that there may only ever
> be one provider offered by one PHC, and will stop at the first such
> provider found, whichever that may be.
> 
> With the added qualifier, there's a higher chance that user space
> searches will be for a {phc, qualifier} pair (even if there will only be
> 1 possible qualifier type), and the future addition of a new hwtstamp
> provider will keep existing software working in the same way as before,
> i.e. user space won't select the DMA provider by mistake, by ignoring
> the qualifier attribute altogether.
> 
> Generally I'm against adding things upfront that can only be in a certain
> way, but in this case I believe that it is necessary in order for the
> future extensions that were discussed to be possible. The qualifier is
> part of the user space search key and thus pretty important.
> 
> My 2 cents, Jakub can absolutely disagree.

Alright, this seems relevant.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Jakub Kicinski]

On Wed, 29 Nov 2023 23:00:34 +0100 Köry Maincent wrote:
> > Not sure why you say "not used", though. Are you not planning to expose
> > the qualifier as an attribute to the listing of hwtstamp providers
> > offered to user space by ETHTOOL_MSG_TSINFO_GET?  
> 
> Yes I will, I was just saying that all the PHC would be set as precise for now.
> Approximate timestamp quality won't be used because IIUC there are no NIC driver
> supporting it yet.

Agreed that we should add the attr from the start.

Maybe we can ask/work with Rahul <rrameshbabu@nvidia.com>
to implement the right thing in mlx5?

Failing that we can mark mlx5 as imprecise, until its sorted out.
So that we have both types in the tree.

Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Rahul Rameshbabu]

On Wed, 29 Nov, 2023 15:56:13 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
> On Wed, 29 Nov 2023 23:00:34 +0100 Köry Maincent wrote:
>> > Not sure why you say "not used", though. Are you not planning to expose
>> > the qualifier as an attribute to the listing of hwtstamp providers
>> > offered to user space by ETHTOOL_MSG_TSINFO_GET?  
>> 
>> Yes I will, I was just saying that all the PHC would be set as precise for now.
>> Approximate timestamp quality won't be used because IIUC there are no NIC driver
>> supporting it yet.
>
> Agreed that we should add the attr from the start.
>
> Maybe we can ask/work with Rahul <rrameshbabu@nvidia.com>
> to implement the right thing in mlx5?

Thanks for looping me in. We were already looking at this patch series
out of interest. I saw your suggestion to rephrase "MAC / DMA" as
"precise / approximate", which we really like for mlx5 devices because
our "approximate" timestamping logic is not exactly a "MAC" timestamp
but its not a port timestamp that has the greater precision we use. I
have a task already for implementing support for this ethtool attribute.
If folks here are open to it, I can add mlx5 support for both modes in
this patch series for the next revision that will entail the discussed
changes.

>
> Failing that we can mark mlx5 as imprecise, until its sorted out.
> So that we have both types in the tree.

--
Thanks,

Rahul Rameshbabu