[PATCH v8 net-next 0/5] PHC support in ENA driver

[PATCH v8 net-next 0/5] PHC support in ENA driver

[David Arinzon]

Changes in v8:
- Create a sysfs entry for each PHC stat

Changes in v7 (https://lore.kernel.org/netdev/20250218183948.757-1-darinzon@amazon.com/):
- Move PHC stats to sysfs
- Add information about PHC enablement
- Remove unrelated style changes

Changes in v6 (https://lore.kernel.org/netdev/20250206141538.549-1-darinzon@amazon.com/):
- Remove PHC error bound

Changes in v5 (https://lore.kernel.org/netdev/20250122102040.752-1-darinzon@amazon.com/):
- Add PHC error bound
- Add PHC enablement and error bound retrieval through sysfs

Changes in v4 (https://lore.kernel.org/netdev/20241114095930.200-1-darinzon@amazon.com/):
- Minor documentation change (resolution instead of accuracy)

Changes in v3 (https://lore.kernel.org/netdev/20241103113140.275-1-darinzon@amazon.com/):
- Resolve a compilation error

Changes in v2 (https://lore.kernel.org/netdev/20241031085245.18146-1-darinzon@amazon.com/):
- CCd PTP maintainer
- Fixed style issues
- Fixed documentation warning

v1 (https://lore.kernel.org/netdev/20241021052011.591-1-darinzon@amazon.com/)

This patchset adds the support for PHC (PTP Hardware Clock)
in the ENA driver. The documentation part of the patchset
includes additional information, including statistics,
utilization and invocation examples through the testptp
utility.

David Arinzon (5):
  net: ena: Add PHC support in the ENA driver
  net: ena: PHC silent reset
  net: ena: PHC enable through sysfs
  net: ena: PHC stats through sysfs
  net: ena: Add PHC documentation

 .../device_drivers/ethernet/amazon/ena.rst    |  96 +++++++
 drivers/net/ethernet/amazon/Kconfig           |   1 +
 drivers/net/ethernet/amazon/ena/Makefile      |   2 +-
 .../net/ethernet/amazon/ena/ena_admin_defs.h  |  63 ++++-
 drivers/net/ethernet/amazon/ena/ena_com.c     | 247 ++++++++++++++++++
 drivers/net/ethernet/amazon/ena/ena_com.h     |  83 ++++++
 drivers/net/ethernet/amazon/ena/ena_ethtool.c |  16 +-
 drivers/net/ethernet/amazon/ena/ena_netdev.c  |  44 +++-
 drivers/net/ethernet/amazon/ena/ena_netdev.h  |   6 +
 drivers/net/ethernet/amazon/ena/ena_phc.c     | 230 ++++++++++++++++
 drivers/net/ethernet/amazon/ena/ena_phc.h     |  37 +++
 .../net/ethernet/amazon/ena/ena_regs_defs.h   |   8 +
 drivers/net/ethernet/amazon/ena/ena_sysfs.c   | 163 ++++++++++++
 drivers/net/ethernet/amazon/ena/ena_sysfs.h   |  28 ++
 14 files changed, 1014 insertions(+), 10 deletions(-)
 create mode 100644 drivers/net/ethernet/amazon/ena/ena_phc.c
 create mode 100644 drivers/net/ethernet/amazon/ena/ena_phc.h
 create mode 100644 drivers/net/ethernet/amazon/ena/ena_sysfs.c
 create mode 100644 drivers/net/ethernet/amazon/ena/ena_sysfs.h

-- 
2.47.1

[PATCH v8 net-next 1/5] net: ena: Add PHC support in the ENA driver

[David Arinzon]

The ENA driver will be extended to support the new PHC feature using
ptp_clock interface [1]. this will provide timestamp reference for user
space to allow measuring time offset between the PHC and the system
clock in order to achieve nanosecond accuracy.

[1] - https://www.kernel.org/doc/html/latest/driver-api/ptp.html

Signed-off-by: Amit Bernstein <amitbern@amazon.com>
Signed-off-by: David Arinzon <darinzon@amazon.com>
---
 drivers/net/ethernet/amazon/Kconfig           |   1 +
 drivers/net/ethernet/amazon/ena/Makefile      |   2 +-
 .../net/ethernet/amazon/ena/ena_admin_defs.h  |  63 ++++-
 drivers/net/ethernet/amazon/ena/ena_com.c     | 247 ++++++++++++++++++
 drivers/net/ethernet/amazon/ena/ena_com.h     |  83 ++++++
 drivers/net/ethernet/amazon/ena/ena_ethtool.c |  16 +-
 drivers/net/ethernet/amazon/ena/ena_netdev.c  |  24 +-
 drivers/net/ethernet/amazon/ena/ena_netdev.h  |   4 +
 drivers/net/ethernet/amazon/ena/ena_phc.c     | 222 ++++++++++++++++
 drivers/net/ethernet/amazon/ena/ena_phc.h     |  37 +++
 .../net/ethernet/amazon/ena/ena_regs_defs.h   |   8 +
 11 files changed, 702 insertions(+), 5 deletions(-)
 create mode 100644 drivers/net/ethernet/amazon/ena/ena_phc.c
 create mode 100644 drivers/net/ethernet/amazon/ena/ena_phc.h

diff --git a/drivers/net/ethernet/amazon/Kconfig b/drivers/net/ethernet/amazon/Kconfig
index c37fa393..8d61bc62 100644
--- a/drivers/net/ethernet/amazon/Kconfig
+++ b/drivers/net/ethernet/amazon/Kconfig
@@ -19,6 +19,7 @@ if NET_VENDOR_AMAZON
 config ENA_ETHERNET
 	tristate "Elastic Network Adapter (ENA) support"
 	depends on PCI_MSI && !CPU_BIG_ENDIAN
+	depends on PTP_1588_CLOCK_OPTIONAL
 	select DIMLIB
 	help
 	  This driver supports Elastic Network Adapter (ENA)"
diff --git a/drivers/net/ethernet/amazon/ena/Makefile b/drivers/net/ethernet/amazon/ena/Makefile
index 6ab61536..8c874177 100644
--- a/drivers/net/ethernet/amazon/ena/Makefile
+++ b/drivers/net/ethernet/amazon/ena/Makefile
@@ -5,4 +5,4 @@
 
 obj-$(CONFIG_ENA_ETHERNET) += ena.o
 
-ena-y := ena_netdev.o ena_com.o ena_eth_com.o ena_ethtool.o ena_xdp.o
+ena-y := ena_netdev.o ena_com.o ena_eth_com.o ena_ethtool.o ena_xdp.o ena_phc.o
diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
index 9d9fa655..28770e60 100644
--- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
@@ -60,6 +60,7 @@ enum ena_admin_aq_feature_id {
 	ENA_ADMIN_AENQ_CONFIG                       = 26,
 	ENA_ADMIN_LINK_CONFIG                       = 27,
 	ENA_ADMIN_HOST_ATTR_CONFIG                  = 28,
+	ENA_ADMIN_PHC_CONFIG                        = 29,
 	ENA_ADMIN_FEATURES_OPCODE_NUM               = 32,
 };
 
@@ -127,6 +128,10 @@ enum ena_admin_get_stats_scope {
 	ENA_ADMIN_ETH_TRAFFIC                       = 1,
 };
 
+enum ena_admin_phc_type {
+	ENA_ADMIN_PHC_TYPE_READLESS                 = 0,
+};
+
 /* ENA SRD configuration for ENI */
 enum ena_admin_ena_srd_flags {
 	/* Feature enabled */
@@ -943,7 +948,9 @@ struct ena_admin_host_info {
 	 * 4 : rss_configurable_function_key
 	 * 5 : reserved
 	 * 6 : rx_page_reuse
-	 * 31:7 : reserved
+	 * 7 : reserved
+	 * 8 : phc
+	 * 31:9 : reserved
 	 */
 	u32 driver_supported_features;
 };
@@ -1023,6 +1030,43 @@ struct ena_admin_queue_ext_feature_desc {
 	};
 };
 
+struct ena_admin_feature_phc_desc {
+	/* PHC type as defined in enum ena_admin_get_phc_type,
+	 * used only for GET command.
+	 */
+	u8 type;
+
+	/* Reserved - MBZ */
+	u8 reserved1[3];
+
+	/* PHC doorbell address as an offset to PCIe MMIO REG BAR,
+	 * used only for GET command.
+	 */
+	u32 doorbell_offset;
+
+	/* Max time for valid PHC retrieval, passing this threshold will
+	 * fail the get-time request and block PHC requests for
+	 * block_timeout_usec, used only for GET command.
+	 */
+	u32 expire_timeout_usec;
+
+	/* PHC requests block period, blocking starts if PHC request expired
+	 * in order to prevent floods on busy device,
+	 * used only for GET command.
+	 */
+	u32 block_timeout_usec;
+
+	/* Shared PHC physical address (ena_admin_phc_resp),
+	 * used only for SET command.
+	 */
+	struct ena_common_mem_addr output_address;
+
+	/* Shared PHC Size (ena_admin_phc_resp),
+	 * used only for SET command.
+	 */
+	u32 output_length;
+};
+
 struct ena_admin_get_feat_resp {
 	struct ena_admin_acq_common_desc acq_common_desc;
 
@@ -1052,6 +1096,8 @@ struct ena_admin_get_feat_resp {
 		struct ena_admin_feature_intr_moder_desc intr_moderation;
 
 		struct ena_admin_ena_hw_hints hw_hints;
+
+		struct ena_admin_feature_phc_desc phc;
 	} u;
 };
 
@@ -1085,6 +1131,9 @@ struct ena_admin_set_feat_cmd {
 
 		/* LLQ configuration */
 		struct ena_admin_feature_llq_desc llq;
+
+		/* PHC configuration */
+		struct ena_admin_feature_phc_desc phc;
 	} u;
 };
 
@@ -1162,6 +1211,16 @@ struct ena_admin_ena_mmio_req_read_less_resp {
 	u32 reg_val;
 };
 
+struct ena_admin_phc_resp {
+	u16 req_id;
+
+	u8 reserved1[6];
+
+	u64 timestamp;
+
+	u8 reserved2[48];
+};
+
 /* aq_common_desc */
 #define ENA_ADMIN_AQ_COMMON_DESC_COMMAND_ID_MASK            GENMASK(11, 0)
 #define ENA_ADMIN_AQ_COMMON_DESC_PHASE_MASK                 BIT(0)
@@ -1260,6 +1319,8 @@ struct ena_admin_ena_mmio_req_read_less_resp {
 #define ENA_ADMIN_HOST_INFO_RSS_CONFIGURABLE_FUNCTION_KEY_MASK BIT(4)
 #define ENA_ADMIN_HOST_INFO_RX_PAGE_REUSE_SHIFT             6
 #define ENA_ADMIN_HOST_INFO_RX_PAGE_REUSE_MASK              BIT(6)
+#define ENA_ADMIN_HOST_INFO_PHC_SHIFT                       8
+#define ENA_ADMIN_HOST_INFO_PHC_MASK                        BIT(8)
 
 /* aenq_common_desc */
 #define ENA_ADMIN_AENQ_COMMON_DESC_PHASE_MASK               BIT(0)
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 66445617..c6b9939e 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -41,6 +41,12 @@
 
 #define ENA_MAX_ADMIN_POLL_US 5000
 
+/* PHC definitions */
+#define ENA_PHC_DEFAULT_EXPIRE_TIMEOUT_USEC 10
+#define ENA_PHC_DEFAULT_BLOCK_TIMEOUT_USEC 1000
+#define ENA_PHC_TIMESTAMP_ERROR 0xFFFFFFFFFFFFFFFF
+#define ENA_PHC_REQ_ID_OFFSET 0xDEAD
+
 /*****************************************************************************/
 /*****************************************************************************/
 /*****************************************************************************/
@@ -1641,6 +1647,247 @@ void ena_com_set_admin_polling_mode(struct ena_com_dev *ena_dev, bool polling)
 	ena_dev->admin_queue.polling = polling;
 }
 
+bool ena_com_phc_supported(struct ena_com_dev *ena_dev)
+{
+	return ena_com_check_supported_feature_id(ena_dev, ENA_ADMIN_PHC_CONFIG);
+}
+
+int ena_com_phc_init(struct ena_com_dev *ena_dev)
+{
+	struct ena_com_phc_info *phc = &ena_dev->phc;
+
+	memset(phc, 0x0, sizeof(*phc));
+
+	/* Allocate shared mem used PHC timestamp retrieved from device */
+	phc->virt_addr = dma_alloc_coherent(ena_dev->dmadev,
+					    sizeof(*phc->virt_addr),
+					    &phc->phys_addr,
+					    GFP_KERNEL);
+	if (unlikely(!phc->virt_addr))
+		return -ENOMEM;
+
+	spin_lock_init(&phc->lock);
+
+	phc->virt_addr->req_id = 0;
+	phc->virt_addr->timestamp = 0;
+
+	return 0;
+}
+
+int ena_com_phc_config(struct ena_com_dev *ena_dev)
+{
+	struct ena_com_phc_info *phc = &ena_dev->phc;
+	struct ena_admin_get_feat_resp get_feat_resp;
+	struct ena_admin_set_feat_resp set_feat_resp;
+	struct ena_admin_set_feat_cmd set_feat_cmd;
+	int ret = 0;
+
+	/* Get device PHC default configuration */
+	ret = ena_com_get_feature(ena_dev,
+				  &get_feat_resp,
+				  ENA_ADMIN_PHC_CONFIG,
+				  0);
+	if (unlikely(ret)) {
+		netdev_err(ena_dev->net_device,
+			   "Failed to get PHC feature configuration, error: %d\n",
+			   ret);
+		return ret;
+	}
+
+	/* Supporting only readless PHC retrieval */
+	if (get_feat_resp.u.phc.type != ENA_ADMIN_PHC_TYPE_READLESS) {
+		netdev_err(ena_dev->net_device, "Unsupported PHC type, error: %d\n",
+			   -EOPNOTSUPP);
+		return -EOPNOTSUPP;
+	}
+
+	/* Update PHC doorbell offset according to device value,
+	 * used to write req_id to PHC bar
+	 */
+	phc->doorbell_offset = get_feat_resp.u.phc.doorbell_offset;
+
+	/* Update PHC expire timeout according to device
+	 * or default driver value
+	 */
+	phc->expire_timeout_usec = (get_feat_resp.u.phc.expire_timeout_usec) ?
+				    get_feat_resp.u.phc.expire_timeout_usec :
+				    ENA_PHC_DEFAULT_EXPIRE_TIMEOUT_USEC;
+
+	/* Update PHC block timeout according to device
+	 * or default driver value
+	 */
+	phc->block_timeout_usec = (get_feat_resp.u.phc.block_timeout_usec) ?
+				   get_feat_resp.u.phc.block_timeout_usec :
+				   ENA_PHC_DEFAULT_BLOCK_TIMEOUT_USEC;
+
+	/* Sanity check - expire timeout must not be above skip timeout */
+	if (phc->expire_timeout_usec > phc->block_timeout_usec)
+		phc->expire_timeout_usec = phc->block_timeout_usec;
+
+	/* Prepare PHC config feature command */
+	memset(&set_feat_cmd, 0x0, sizeof(set_feat_cmd));
+	set_feat_cmd.aq_common_descriptor.opcode = ENA_ADMIN_SET_FEATURE;
+	set_feat_cmd.feat_common.feature_id = ENA_ADMIN_PHC_CONFIG;
+	set_feat_cmd.u.phc.output_length = sizeof(*phc->virt_addr);
+	ret = ena_com_mem_addr_set(ena_dev,
+				   &set_feat_cmd.u.phc.output_address,
+				   phc->phys_addr);
+	if (unlikely(ret)) {
+		netdev_err(ena_dev->net_device, "Failed setting PHC output address, error: %d\n",
+			   ret);
+		return ret;
+	}
+
+	/* Send PHC feature command to the device */
+	ret = ena_com_execute_admin_command(&ena_dev->admin_queue,
+					    (struct ena_admin_aq_entry *)&set_feat_cmd,
+					    sizeof(set_feat_cmd),
+					    (struct ena_admin_acq_entry *)&set_feat_resp,
+					    sizeof(set_feat_resp));
+
+	if (unlikely(ret)) {
+		netdev_err(ena_dev->net_device,
+			   "Failed to enable PHC, error: %d\n",
+			   ret);
+		return ret;
+	}
+
+	phc->active = true;
+	netdev_dbg(ena_dev->net_device, "PHC is active in the device\n");
+
+	return ret;
+}
+
+void ena_com_phc_destroy(struct ena_com_dev *ena_dev)
+{
+	struct ena_com_phc_info *phc = &ena_dev->phc;
+	unsigned long flags = 0;
+
+	/* In case PHC is not supported by the device, silently exiting */
+	if (!phc->virt_addr)
+		return;
+
+	spin_lock_irqsave(&phc->lock, flags);
+	phc->active = false;
+	spin_unlock_irqrestore(&phc->lock, flags);
+
+	dma_free_coherent(ena_dev->dmadev,
+			  sizeof(*phc->virt_addr),
+			  phc->virt_addr,
+			  phc->phys_addr);
+	phc->virt_addr = NULL;
+}
+
+int ena_com_phc_get(struct ena_com_dev *ena_dev, u64 *timestamp)
+{
+	volatile struct ena_admin_phc_resp *read_resp = ena_dev->phc.virt_addr;
+	const ktime_t zero_system_time = ktime_set(0, 0);
+	struct ena_com_phc_info *phc = &ena_dev->phc;
+	ktime_t expire_time;
+	ktime_t block_time;
+	unsigned long flags = 0;
+	int ret = 0;
+
+	if (!phc->active) {
+		netdev_err(ena_dev->net_device, "PHC feature is not active in the device\n");
+		return -EOPNOTSUPP;
+	}
+
+	spin_lock_irqsave(&phc->lock, flags);
+
+	/* Check if PHC is in blocked state */
+	if (unlikely(ktime_compare(phc->system_time, zero_system_time))) {
+		/* Check if blocking time expired */
+		block_time = ktime_add_us(phc->system_time, phc->block_timeout_usec);
+		if (!ktime_after(ktime_get(), block_time)) {
+			/* PHC is still in blocked state, skip PHC request */
+			phc->stats.phc_skp++;
+			ret = -EBUSY;
+			goto skip;
+		}
+
+		/* PHC is in active state, update statistics according to
+		 * req_id and timestamp
+		 */
+		if ((READ_ONCE(read_resp->req_id) != phc->req_id) ||
+		    read_resp->timestamp == ENA_PHC_TIMESTAMP_ERROR)
+			/* Device didn't update req_id during blocking time
+			 * or timestamp is invalid, this indicates on a
+			 * device error
+			 */
+			phc->stats.phc_err++;
+		else
+			/* Device updated req_id during blocking time
+			 * with valid timestamp
+			 */
+			phc->stats.phc_exp++;
+	}
+
+	/* Setting relative timeouts */
+	phc->system_time = ktime_get();
+	block_time = ktime_add_us(phc->system_time, phc->block_timeout_usec);
+	expire_time = ktime_add_us(phc->system_time, phc->expire_timeout_usec);
+
+	/* We expect the device to return this req_id once
+	 * the new PHC timestamp is updated
+	 */
+	phc->req_id++;
+
+	/* Initialize PHC shared memory with different req_id value
+	 * to be able to identify once the device changes it to req_id
+	 */
+	read_resp->req_id = phc->req_id + ENA_PHC_REQ_ID_OFFSET;
+
+	/* Writing req_id to PHC bar */
+	writel(phc->req_id, ena_dev->reg_bar + phc->doorbell_offset);
+
+	/* Stalling until the device updates req_id */
+	while (1) {
+		if (unlikely(ktime_after(ktime_get(), expire_time))) {
+			/* Gave up waiting for updated req_id,
+			 * PHC enters into blocked state until passing
+			 * blocking time
+			 */
+			ret = -EBUSY;
+			break;
+		}
+
+		/* Check if req_id was updated by the device */
+		if (READ_ONCE(read_resp->req_id) != phc->req_id) {
+			/* req_id was not updated by the device,
+			 * check again on next loop
+			 */
+			continue;
+		}
+
+		/* req_id was updated which indicates that PHC timestamp
+		 * was updated too
+		 */
+		*timestamp = read_resp->timestamp;
+
+		/* PHC timestamp validty check */
+		if (unlikely(*timestamp == ENA_PHC_TIMESTAMP_ERROR)) {
+			/* Retrieved invalid PHC timestamp, PHC enters into
+			 * blocked state until passing blocking time
+			 */
+			ret = -EBUSY;
+			break;
+		}
+
+		/* Retrieved valid PHC timestamp */
+		phc->stats.phc_cnt++;
+
+		/* This indicates PHC state is active */
+		phc->system_time = zero_system_time;
+		break;
+	}
+
+skip:
+	spin_unlock_irqrestore(&phc->lock, flags);
+
+	return ret;
+}
+
 int ena_com_mmio_reg_read_request_init(struct ena_com_dev *ena_dev)
 {
 	struct ena_com_mmio_read *mmio_read = &ena_dev->mmio_read;
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h b/drivers/net/ethernet/amazon/ena/ena_com.h
index 9414e93d..3905d348 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_com.h
@@ -210,6 +210,13 @@ struct ena_com_stats_admin {
 	u64 no_completion;
 };
 
+struct ena_com_stats_phc {
+	u64 phc_cnt;
+	u64 phc_exp;
+	u64 phc_skp;
+	u64 phc_err;
+};
+
 struct ena_com_admin_queue {
 	void *q_dmadev;
 	struct ena_com_dev *ena_dev;
@@ -258,6 +265,47 @@ struct ena_com_mmio_read {
 	spinlock_t lock;
 };
 
+/* PTP hardware clock (PHC) MMIO read data info */
+struct ena_com_phc_info {
+	/* Internal PHC statistics */
+	struct ena_com_stats_phc stats;
+
+	/* PHC shared memory - virtual address */
+	struct ena_admin_phc_resp *virt_addr;
+
+	/* System time of last PHC request */
+	ktime_t system_time;
+
+	/* Spin lock to ensure a single outstanding PHC read */
+	spinlock_t lock;
+
+	/* PHC doorbell address as an offset to PCIe MMIO REG BAR */
+	u32 doorbell_offset;
+
+	/* Shared memory read expire timeout (usec)
+	 * Max time for valid PHC retrieval, passing this threshold will fail
+	 * the get time request and block new PHC requests for block_timeout_usec
+	 * in order to prevent floods on busy device
+	 */
+	u32 expire_timeout_usec;
+
+	/* Shared memory read abort timeout (usec)
+	 * PHC requests block period, blocking starts once PHC request expired
+	 * in order to prevent floods on busy device,
+	 * any PHC requests during block period will be skipped
+	 */
+	u32 block_timeout_usec;
+
+	/* PHC shared memory - physical address */
+	dma_addr_t phys_addr;
+
+	/* Request id sent to the device */
+	u16 req_id;
+
+	/* True if PHC is active in the device */
+	bool active;
+};
+
 struct ena_rss {
 	/* Indirect table */
 	u16 *host_rss_ind_tbl;
@@ -317,6 +365,7 @@ struct ena_com_dev {
 	u32 ena_min_poll_delay_us;
 
 	struct ena_com_mmio_read mmio_read;
+	struct ena_com_phc_info phc;
 
 	struct ena_rss rss;
 	u32 supported_features;
@@ -382,6 +431,40 @@ struct ena_aenq_handlers {
  */
 int ena_com_mmio_reg_read_request_init(struct ena_com_dev *ena_dev);
 
+/* ena_com_phc_init - Allocate and initialize PHC feature
+ * @ena_dev: ENA communication layer struct
+ * @note: This method assumes PHC is supported by the device
+ * @return - 0 on success, negative value on failure
+ */
+int ena_com_phc_init(struct ena_com_dev *ena_dev);
+
+/* ena_com_phc_supported - Return if PHC feature is supported by the device
+ * @ena_dev: ENA communication layer struct
+ * @note: This method must be called after getting supported features
+ * @return - supported or not
+ */
+bool ena_com_phc_supported(struct ena_com_dev *ena_dev);
+
+/* ena_com_phc_config - Configure PHC feature
+ * @ena_dev: ENA communication layer struct
+ * Configure PHC feature in driver and device
+ * @note: This method assumes PHC is supported by the device
+ * @return - 0 on success, negative value on failure
+ */
+int ena_com_phc_config(struct ena_com_dev *ena_dev);
+
+/* ena_com_phc_destroy - Destroy PHC feature
+ * @ena_dev: ENA communication layer struct
+ */
+void ena_com_phc_destroy(struct ena_com_dev *ena_dev);
+
+/* ena_com_phc_get - Retrieve PHC timestamp
+ * @ena_dev: ENA communication layer struct
+ * @timestamp: Retrieve PHC timestamp
+ * @return - 0 on success, negative value on failure
+ */
+int ena_com_phc_get(struct ena_com_dev *ena_dev, u64 *timestamp);
+
 /* ena_com_set_mmio_read_mode - Enable/disable the indirect mmio reg read mechanism
  * @ena_dev: ENA communication layer struct
  * @readless_supported: readless mode (enable/disable)
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index a3c934c3..b442d247 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -5,9 +5,11 @@
 
 #include <linux/ethtool.h>
 #include <linux/pci.h>
+#include <linux/net_tstamp.h>
 
 #include "ena_netdev.h"
 #include "ena_xdp.h"
+#include "ena_phc.h"
 
 struct ena_stats {
 	char name[ETH_GSTRING_LEN];
@@ -298,6 +300,18 @@ static void ena_get_ethtool_stats(struct net_device *netdev,
 	ena_get_stats(adapter, data, true);
 }
 
+static int ena_get_ts_info(struct net_device *netdev,
+			   struct kernel_ethtool_ts_info *info)
+{
+	struct ena_adapter *adapter = netdev_priv(netdev);
+
+	info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE;
+
+	info->phc_index = ena_phc_get_index(adapter);
+
+	return 0;
+}
+
 static int ena_get_sw_stats_count(struct ena_adapter *adapter)
 {
 	return adapter->num_io_queues * (ENA_STATS_ARRAY_TX + ENA_STATS_ARRAY_RX)
@@ -1107,7 +1121,7 @@ static const struct ethtool_ops ena_ethtool_ops = {
 	.set_channels		= ena_set_channels,
 	.get_tunable		= ena_get_tunable,
 	.set_tunable		= ena_set_tunable,
-	.get_ts_info            = ethtool_op_get_ts_info,
+	.get_ts_info		= ena_get_ts_info,
 };
 
 void ena_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 6aab85a7..9cecb011 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -19,6 +19,8 @@
 #include "ena_pci_id_tbl.h"
 #include "ena_xdp.h"
 
+#include "ena_phc.h"
+
 MODULE_AUTHOR("Amazon.com, Inc. or its affiliates");
 MODULE_DESCRIPTION(DEVICE_NAME);
 MODULE_LICENSE("GPL");
@@ -2739,7 +2741,8 @@ static void ena_config_host_info(struct ena_com_dev *ena_dev, struct pci_dev *pd
 		ENA_ADMIN_HOST_INFO_INTERRUPT_MODERATION_MASK |
 		ENA_ADMIN_HOST_INFO_RX_BUF_MIRRORING_MASK |
 		ENA_ADMIN_HOST_INFO_RSS_CONFIGURABLE_FUNCTION_KEY_MASK |
-		ENA_ADMIN_HOST_INFO_RX_PAGE_REUSE_MASK;
+		ENA_ADMIN_HOST_INFO_RX_PAGE_REUSE_MASK |
+		ENA_ADMIN_HOST_INFO_PHC_MASK;
 
 	rc = ena_com_set_host_attributes(ena_dev);
 	if (rc) {
@@ -3184,6 +3187,10 @@ static int ena_device_init(struct ena_adapter *adapter, struct pci_dev *pdev,
 	if (unlikely(rc))
 		goto err_admin_init;
 
+	rc = ena_phc_init(adapter);
+	if (unlikely(rc && (rc != -EOPNOTSUPP)))
+		netdev_err(netdev, "Failed initializing PHC, error: %d\n", rc);
+
 	return 0;
 
 err_admin_init:
@@ -3267,6 +3274,8 @@ static int ena_destroy_device(struct ena_adapter *adapter, bool graceful)
 
 	ena_com_admin_destroy(ena_dev);
 
+	ena_phc_destroy(adapter);
+
 	ena_com_mmio_reg_read_request_destroy(ena_dev);
 
 	/* return reset reason to default value */
@@ -3340,6 +3349,7 @@ err_device_destroy:
 	ena_com_wait_for_abort_completion(ena_dev);
 	ena_com_admin_destroy(ena_dev);
 	ena_com_dev_reset(ena_dev, ENA_REGS_RESET_DRIVER_INVALID_STATE);
+	ena_phc_destroy(adapter);
 	ena_com_mmio_reg_read_request_destroy(ena_dev);
 err:
 	clear_bit(ENA_FLAG_DEVICE_RUNNING, &adapter->flags);
@@ -3927,10 +3937,16 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	pci_set_drvdata(pdev, adapter);
 
+	rc = ena_phc_alloc(adapter);
+	if (rc) {
+		netdev_err(netdev, "ena_phc_alloc failed\n");
+		goto err_netdev_destroy;
+	}
+
 	rc = ena_com_allocate_customer_metrics_buffer(ena_dev);
 	if (rc) {
 		netdev_err(netdev, "ena_com_allocate_customer_metrics_buffer failed\n");
-		goto err_netdev_destroy;
+		goto err_free_phc;
 	}
 
 	rc = ena_map_llq_mem_bar(pdev, ena_dev, bars);
@@ -4067,6 +4083,8 @@ err_device_destroy:
 	ena_com_admin_destroy(ena_dev);
 err_metrics_destroy:
 	ena_com_delete_customer_metrics_buffer(ena_dev);
+err_free_phc:
+	ena_phc_free(adapter);
 err_netdev_destroy:
 	free_netdev(netdev);
 err_free_region:
@@ -4107,6 +4125,8 @@ static void __ena_shutoff(struct pci_dev *pdev, bool shutdown)
 	adapter->reset_reason = ENA_REGS_RESET_SHUTDOWN;
 	ena_destroy_device(adapter, true);
 
+	ena_phc_free(adapter);
+
 	if (shutdown) {
 		netif_device_detach(netdev);
 		dev_close(netdev);
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 6e12ae3b..7867cd7f 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -110,6 +110,8 @@
 
 #define ENA_MMIO_DISABLE_REG_READ	BIT(0)
 
+struct ena_phc_info;
+
 struct ena_irq {
 	irq_handler_t handler;
 	void *data;
@@ -348,6 +350,8 @@ struct ena_adapter {
 
 	char name[ENA_NAME_MAX_LEN];
 
+	struct ena_phc_info *phc_info;
+
 	unsigned long flags;
 	/* TX */
 	struct ena_ring tx_ring[ENA_MAX_NUM_IO_QUEUES]
diff --git a/drivers/net/ethernet/amazon/ena/ena_phc.c b/drivers/net/ethernet/amazon/ena/ena_phc.c
new file mode 100644
index 00000000..87495de0
--- /dev/null
+++ b/drivers/net/ethernet/amazon/ena/ena_phc.c
@@ -0,0 +1,222 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/*
+ * Copyright 2015-2022 Amazon.com, Inc. or its affiliates. All rights reserved.
+ */
+
+#include <linux/pci.h>
+#include "ena_netdev.h"
+#include "ena_phc.h"
+
+static int ena_phc_adjtime(struct ptp_clock_info *clock_info, s64 delta)
+{
+	return -EOPNOTSUPP;
+}
+
+static int ena_phc_adjfine(struct ptp_clock_info *clock_info, long scaled_ppm)
+{
+	return -EOPNOTSUPP;
+}
+
+static int ena_phc_feature_enable(struct ptp_clock_info *clock_info,
+				  struct ptp_clock_request *rq,
+				  int on)
+{
+	return -EOPNOTSUPP;
+}
+
+static int ena_phc_gettimex64(struct ptp_clock_info *clock_info,
+			      struct timespec64 *ts,
+			      struct ptp_system_timestamp *sts)
+{
+	struct ena_phc_info *phc_info =
+		container_of(clock_info, struct ena_phc_info, clock_info);
+	unsigned long flags;
+	u64 timestamp_nsec;
+	int rc;
+
+	spin_lock_irqsave(&phc_info->lock, flags);
+
+	ptp_read_system_prets(sts);
+
+	rc = ena_com_phc_get(phc_info->adapter->ena_dev, &timestamp_nsec);
+
+	ptp_read_system_postts(sts);
+
+	spin_unlock_irqrestore(&phc_info->lock, flags);
+
+	*ts = ns_to_timespec64(timestamp_nsec);
+
+	return rc;
+}
+
+static int ena_phc_settime64(struct ptp_clock_info *clock_info,
+			     const struct timespec64 *ts)
+{
+	return -EOPNOTSUPP;
+}
+
+static struct ptp_clock_info ena_ptp_clock_info = {
+	.owner		= THIS_MODULE,
+	.n_alarm	= 0,
+	.n_ext_ts	= 0,
+	.n_per_out	= 0,
+	.pps		= 0,
+	.adjtime	= ena_phc_adjtime,
+	.adjfine	= ena_phc_adjfine,
+	.gettimex64	= ena_phc_gettimex64,
+	.settime64	= ena_phc_settime64,
+	.enable		= ena_phc_feature_enable,
+};
+
+/* Enable/Disable PHC by the kernel, affects on the next init flow */
+void ena_phc_enable(struct ena_adapter *adapter, bool enable)
+{
+	struct ena_phc_info *phc_info = adapter->phc_info;
+
+	if (!phc_info) {
+		netdev_err(adapter->netdev, "phc_info is not allocated\n");
+		return;
+	}
+
+	phc_info->enabled = enable;
+}
+
+/* Check if PHC is enabled by the kernel */
+bool ena_phc_is_enabled(struct ena_adapter *adapter)
+{
+	struct ena_phc_info *phc_info = adapter->phc_info;
+
+	return (phc_info && phc_info->enabled);
+}
+
+/* PHC is activated if ptp clock is registered in the kernel */
+bool ena_phc_is_active(struct ena_adapter *adapter)
+{
+	struct ena_phc_info *phc_info = adapter->phc_info;
+
+	return (phc_info && phc_info->clock);
+}
+
+static int ena_phc_register(struct ena_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct ptp_clock_info *clock_info;
+	struct ena_phc_info *phc_info;
+	int rc = 0;
+
+	phc_info = adapter->phc_info;
+	clock_info = &phc_info->clock_info;
+
+	phc_info->adapter = adapter;
+
+	spin_lock_init(&phc_info->lock);
+
+	/* Fill the ptp_clock_info struct and register PTP clock */
+	*clock_info = ena_ptp_clock_info;
+	snprintf(clock_info->name,
+		 sizeof(clock_info->name),
+		 "ena-ptp-%02x",
+		 PCI_SLOT(pdev->devfn));
+
+	phc_info->clock = ptp_clock_register(clock_info, &pdev->dev);
+	if (IS_ERR(phc_info->clock)) {
+		rc = PTR_ERR(phc_info->clock);
+		netdev_err(adapter->netdev, "Failed registering ptp clock, error: %d\n",
+			   rc);
+		phc_info->clock = NULL;
+	}
+
+	return rc;
+}
+
+static void ena_phc_unregister(struct ena_adapter *adapter)
+{
+	struct ena_phc_info *phc_info = adapter->phc_info;
+
+	if (ena_phc_is_active(adapter)) {
+		ptp_clock_unregister(phc_info->clock);
+		phc_info->clock = NULL;
+	}
+}
+
+int ena_phc_alloc(struct ena_adapter *adapter)
+{
+	/* Allocate driver specific PHC info */
+	adapter->phc_info = vzalloc(sizeof(*adapter->phc_info));
+	if (unlikely(!adapter->phc_info)) {
+		netdev_err(adapter->netdev, "Failed to alloc phc_info\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void ena_phc_free(struct ena_adapter *adapter)
+{
+	if (adapter->phc_info) {
+		vfree(adapter->phc_info);
+		adapter->phc_info = NULL;
+	}
+}
+
+int ena_phc_init(struct ena_adapter *adapter)
+{
+	struct ena_com_dev *ena_dev = adapter->ena_dev;
+	struct net_device *netdev = adapter->netdev;
+	int rc = -EOPNOTSUPP;
+
+	/* Validate PHC feature is supported in the device */
+	if (!ena_com_phc_supported(ena_dev)) {
+		netdev_dbg(netdev, "PHC feature is not supported by the device\n");
+		goto err_ena_com_phc_init;
+	}
+
+	/* Validate PHC feature is enabled by the kernel */
+	if (!ena_phc_is_enabled(adapter)) {
+		netdev_dbg(netdev, "PHC feature is not enabled by the kernel\n");
+		goto err_ena_com_phc_init;
+	}
+
+	/* Initialize device specific PHC info */
+	rc = ena_com_phc_init(ena_dev);
+	if (unlikely(rc)) {
+		netdev_err(netdev, "Failed to init phc, error: %d\n", rc);
+		goto err_ena_com_phc_init;
+	}
+
+	/* Configure PHC feature in driver and device */
+	rc = ena_com_phc_config(ena_dev);
+	if (unlikely(rc)) {
+		netdev_err(netdev, "Failed to config phc, error: %d\n", rc);
+		goto err_ena_com_phc_config;
+	}
+
+	/* Register to PTP class driver */
+	rc = ena_phc_register(adapter);
+	if (unlikely(rc)) {
+		netdev_err(netdev, "Failed to register phc, error: %d\n", rc);
+		goto err_ena_com_phc_config;
+	}
+
+	return 0;
+
+err_ena_com_phc_config:
+	ena_com_phc_destroy(ena_dev);
+err_ena_com_phc_init:
+	ena_phc_enable(adapter, false);
+	return rc;
+}
+
+void ena_phc_destroy(struct ena_adapter *adapter)
+{
+	ena_phc_unregister(adapter);
+	ena_com_phc_destroy(adapter->ena_dev);
+}
+
+int ena_phc_get_index(struct ena_adapter *adapter)
+{
+	if (ena_phc_is_active(adapter))
+		return ptp_clock_index(adapter->phc_info->clock);
+
+	return -1;
+}
diff --git a/drivers/net/ethernet/amazon/ena/ena_phc.h b/drivers/net/ethernet/amazon/ena/ena_phc.h
new file mode 100644
index 00000000..7364fe71
--- /dev/null
+++ b/drivers/net/ethernet/amazon/ena/ena_phc.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
+/*
+ * Copyright 2015-2022 Amazon.com, Inc. or its affiliates. All rights reserved.
+ */
+
+#ifndef ENA_PHC_H
+#define ENA_PHC_H
+
+#include <linux/ptp_clock_kernel.h>
+
+struct ena_phc_info {
+	/* PTP hardware capabilities */
+	struct ptp_clock_info clock_info;
+
+	/* Registered PTP clock device */
+	struct ptp_clock *clock;
+
+	/* Adapter specific private data structure */
+	struct ena_adapter *adapter;
+
+	/* PHC lock */
+	spinlock_t lock;
+
+	/* Enabled by kernel */
+	bool enabled;
+};
+
+void ena_phc_enable(struct ena_adapter *adapter, bool enable);
+bool ena_phc_is_enabled(struct ena_adapter *adapter);
+bool ena_phc_is_active(struct ena_adapter *adapter);
+int ena_phc_get_index(struct ena_adapter *adapter);
+int ena_phc_init(struct ena_adapter *adapter);
+void ena_phc_destroy(struct ena_adapter *adapter);
+int ena_phc_alloc(struct ena_adapter *adapter);
+void ena_phc_free(struct ena_adapter *adapter);
+
+#endif /* ENA_PHC_H */
diff --git a/drivers/net/ethernet/amazon/ena/ena_regs_defs.h b/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
index a2efebaf..51068dc1 100644
--- a/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
@@ -53,6 +53,11 @@ enum ena_regs_reset_reason_types {
 #define ENA_REGS_MMIO_RESP_HI_OFF                           0x64
 #define ENA_REGS_RSS_IND_ENTRY_UPDATE_OFF                   0x68
 
+/* phc_registers offsets */
+
+/* 100 base */
+#define ENA_REGS_PHC_DB_OFF                                 0x100
+
 /* version register */
 #define ENA_REGS_VERSION_MINOR_VERSION_MASK                 0xff
 #define ENA_REGS_VERSION_MAJOR_VERSION_SHIFT                8
@@ -129,4 +134,7 @@ enum ena_regs_reset_reason_types {
 #define ENA_REGS_RSS_IND_ENTRY_UPDATE_CQ_IDX_SHIFT          16
 #define ENA_REGS_RSS_IND_ENTRY_UPDATE_CQ_IDX_MASK           0xffff0000
 
+/* phc_db_req_id register */
+#define ENA_REGS_PHC_DB_REQ_ID_MASK                         0xffff
+
 #endif /* _ENA_REGS_H_ */
-- 
2.47.1

[PATCH v8 net-next 2/5] net: ena: PHC silent reset

[David Arinzon]

Each PHC device kernel registration receives a unique kernel index,
which is associated with a new PHC device file located at
"/dev/ptp<index>".
This device file serves as an interface for obtaining PHC timestamps.
Examples of tools that use "/dev/ptp" include testptp [1]
and chrony [2].

A reset flow may occur in the ENA driver while PHC is active.
During a reset, the driver will unregister and then re-register the
PHC device with the kernel.
Under race conditions, particularly during heavy PHC loads,
the driver’s reset flow might complete faster than the kernel’s PHC
unregister/register process.
This can result in the PHC index being different from what it was prior
to the reset, as the PHC index is selected using kernel ID
allocation [3].

While driver rmmod/insmod are done by the user, a reset may occur
at anytime, without the user awareness, consequently, the driver
might receive a new PHC index after the reset, potentially compromising
the user experience.

To prevent this issue, the PHC flow will detect the reset during PHC
destruction and will skip the PHC unregister/register calls to preserve
the kernel PHC index.
During the reset flow, any attempt to get a PHC timestamp will fail as
expected, but the kernel PHC index will remain unchanged.

[1]: https://github.com/torvalds/linux/blob/v6.1/tools/testing/selftests/ptp/testptp.c
[2]: https://github.com/mlichvar/chrony
[3]: https://www.kernel.org/doc/html/latest/core-api/idr.html

Signed-off-by: Amit Bernstein <amitbern@amazon.com>
Signed-off-by: David Arinzon <darinzon@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_phc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_phc.c b/drivers/net/ethernet/amazon/ena/ena_phc.c
index 87495de0..5c1acd88 100644
--- a/drivers/net/ethernet/amazon/ena/ena_phc.c
+++ b/drivers/net/ethernet/amazon/ena/ena_phc.c
@@ -107,6 +107,10 @@ static int ena_phc_register(struct ena_adapter *adapter)
 	phc_info = adapter->phc_info;
 	clock_info = &phc_info->clock_info;
 
+	/* PHC may already be registered in case of a reset */
+	if (ena_phc_is_active(adapter))
+		return 0;
+
 	phc_info->adapter = adapter;
 
 	spin_lock_init(&phc_info->lock);
@@ -133,7 +137,11 @@ static void ena_phc_unregister(struct ena_adapter *adapter)
 {
 	struct ena_phc_info *phc_info = adapter->phc_info;
 
-	if (ena_phc_is_active(adapter)) {
+	/* During reset flow, PHC must stay registered
+	 * to keep kernel's PHC index
+	 */
+	if (ena_phc_is_active(adapter) &&
+	    !test_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags)) {
 		ptp_clock_unregister(phc_info->clock);
 		phc_info->clock = NULL;
 	}
-- 
2.47.1

[PATCH v8 net-next 3/5] net: ena: PHC enable through sysfs

[David Arinzon]

This patch allows controlling PHC feature enablement
through sysfs.

The PHC feature is disabled by default, although its
footprint is small, most customers do not require such
high-precision timestamping. Enabling PHC unnecessarily
in environments that do not need it might introduce a
minor but unnecessary overhead.

Customers who require PHC can enable it via the sysfs entry.

Signed-off-by: David Arinzon <darinzon@amazon.com>
---
 drivers/net/ethernet/amazon/ena/Makefile     |  2 +-
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 20 +++--
 drivers/net/ethernet/amazon/ena/ena_netdev.h |  2 +
 drivers/net/ethernet/amazon/ena/ena_sysfs.c  | 83 ++++++++++++++++++++
 drivers/net/ethernet/amazon/ena/ena_sysfs.h  | 28 +++++++
 5 files changed, 129 insertions(+), 6 deletions(-)
 create mode 100644 drivers/net/ethernet/amazon/ena/ena_sysfs.c
 create mode 100644 drivers/net/ethernet/amazon/ena/ena_sysfs.h

diff --git a/drivers/net/ethernet/amazon/ena/Makefile b/drivers/net/ethernet/amazon/ena/Makefile
index 8c874177..d950ade6 100644
--- a/drivers/net/ethernet/amazon/ena/Makefile
+++ b/drivers/net/ethernet/amazon/ena/Makefile
@@ -5,4 +5,4 @@
 
 obj-$(CONFIG_ENA_ETHERNET) += ena.o
 
-ena-y := ena_netdev.o ena_com.o ena_eth_com.o ena_ethtool.o ena_xdp.o ena_phc.o
+ena-y := ena_netdev.o ena_com.o ena_eth_com.o ena_ethtool.o ena_xdp.o ena_phc.o ena_sysfs.o
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 9cecb011..b565eab1 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -17,6 +17,7 @@
 
 #include "ena_netdev.h"
 #include "ena_pci_id_tbl.h"
+#include "ena_sysfs.h"
 #include "ena_xdp.h"
 
 #include "ena_phc.h"
@@ -41,8 +42,6 @@ MODULE_DEVICE_TABLE(pci, ena_pci_tbl);
 
 static int ena_rss_init_default(struct ena_adapter *adapter);
 static void check_for_admin_com_state(struct ena_adapter *adapter);
-static int ena_destroy_device(struct ena_adapter *adapter, bool graceful);
-static int ena_restore_device(struct ena_adapter *adapter);
 
 static void ena_tx_timeout(struct net_device *dev, unsigned int txqueue)
 {
@@ -3236,7 +3235,7 @@ err_disable_msix:
 	return rc;
 }
 
-static int ena_destroy_device(struct ena_adapter *adapter, bool graceful)
+int ena_destroy_device(struct ena_adapter *adapter, bool graceful)
 {
 	struct net_device *netdev = adapter->netdev;
 	struct ena_com_dev *ena_dev = adapter->ena_dev;
@@ -3287,7 +3286,7 @@ static int ena_destroy_device(struct ena_adapter *adapter, bool graceful)
 	return rc;
 }
 
-static int ena_restore_device(struct ena_adapter *adapter)
+int ena_restore_device(struct ena_adapter *adapter)
 {
 	struct ena_com_dev_get_features_ctx get_feat_ctx;
 	struct ena_com_dev *ena_dev = adapter->ena_dev;
@@ -4022,10 +4021,17 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			"Failed to enable and set the admin interrupts\n");
 		goto err_worker_destroy;
 	}
+
+	rc = ena_sysfs_init(&adapter->pdev->dev);
+	if (rc) {
+		dev_err(&pdev->dev, "Cannot init sysfs\n");
+		goto err_free_msix;
+	}
+
 	rc = ena_rss_init_default(adapter);
 	if (rc && (rc != -EOPNOTSUPP)) {
 		dev_err(&pdev->dev, "Cannot init RSS rc: %d\n", rc);
-		goto err_free_msix;
+		goto err_terminate_sysfs;
 	}
 
 	ena_config_debug_area(adapter);
@@ -4070,6 +4076,8 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 err_rss:
 	ena_com_delete_debug_area(ena_dev);
 	ena_com_rss_destroy(ena_dev);
+err_terminate_sysfs:
+	ena_sysfs_terminate(&pdev->dev);
 err_free_msix:
 	ena_com_dev_reset(ena_dev, ENA_REGS_RESET_INIT_ERR);
 	/* stop submitting admin commands on a device that was reset */
@@ -4115,6 +4123,8 @@ static void __ena_shutoff(struct pci_dev *pdev, bool shutdown)
 	ena_dev = adapter->ena_dev;
 	netdev = adapter->netdev;
 
+	ena_sysfs_terminate(&adapter->pdev->dev);
+
 	/* Make sure timer and reset routine won't be called after
 	 * freeing device resources.
 	 */
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 7867cd7f..e3c7ed9c 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -416,6 +416,8 @@ static inline void ena_reset_device(struct ena_adapter *adapter,
 	set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags);
 }
 
+int ena_destroy_device(struct ena_adapter *adapter, bool graceful);
+int ena_restore_device(struct ena_adapter *adapter);
 int handle_invalid_req_id(struct ena_ring *ring, u16 req_id,
 			  struct ena_tx_buffer *tx_info, bool is_xdp);
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_sysfs.c b/drivers/net/ethernet/amazon/ena/ena_sysfs.c
new file mode 100644
index 00000000..d0ded92d
--- /dev/null
+++ b/drivers/net/ethernet/amazon/ena/ena_sysfs.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/*
+ * Copyright 2015-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/stat.h>
+#include <linux/sysfs.h>
+
+#include "ena_com.h"
+#include "ena_netdev.h"
+#include "ena_phc.h"
+#include "ena_sysfs.h"
+
+static ssize_t ena_phc_enable_set(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t len)
+{
+	struct ena_adapter *adapter = dev_get_drvdata(dev);
+	unsigned long phc_enable_val;
+	int rc;
+
+	if (!ena_com_phc_supported(adapter->ena_dev)) {
+		netif_info(adapter, drv, adapter->netdev,
+			   "Device doesn't support PHC");
+		return -EOPNOTSUPP;
+	}
+
+	rc = kstrtoul(buf, 10, &phc_enable_val);
+	if (rc < 0)
+		return rc;
+
+	if (phc_enable_val != 0 && phc_enable_val != 1)
+		return -EINVAL;
+
+	rtnl_lock();
+
+	/* No change in state */
+	if ((bool)phc_enable_val == ena_phc_is_enabled(adapter))
+		goto out;
+
+	ena_phc_enable(adapter, phc_enable_val);
+
+	ena_destroy_device(adapter, false);
+	rc = ena_restore_device(adapter);
+
+out:
+	rtnl_unlock();
+	return rc ? rc : len;
+}
+
+#define ENA_PHC_ENABLE_STR_MAX_LEN 3
+
+static ssize_t ena_phc_enable_get(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct ena_adapter *adapter = dev_get_drvdata(dev);
+
+	return snprintf(buf, ENA_PHC_ENABLE_STR_MAX_LEN, "%u\n",
+			ena_phc_is_enabled(adapter));
+}
+
+static DEVICE_ATTR(phc_enable, S_IRUGO | S_IWUSR, ena_phc_enable_get,
+		   ena_phc_enable_set);
+
+/******************************************************************************
+ *****************************************************************************/
+int ena_sysfs_init(struct device *dev)
+{
+	if (device_create_file(dev, &dev_attr_phc_enable))
+		dev_err(dev, "Failed to create phc_enable sysfs entry");
+
+	return 0;
+}
+
+/******************************************************************************
+ *****************************************************************************/
+void ena_sysfs_terminate(struct device *dev)
+{
+	device_remove_file(dev, &dev_attr_phc_enable);
+}
diff --git a/drivers/net/ethernet/amazon/ena/ena_sysfs.h b/drivers/net/ethernet/amazon/ena/ena_sysfs.h
new file mode 100644
index 00000000..8c572eee
--- /dev/null
+++ b/drivers/net/ethernet/amazon/ena/ena_sysfs.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
+/*
+ * Copyright 2015-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
+ */
+
+#ifndef __ENA_SYSFS_H__
+#define __ENA_SYSFS_H__
+
+#ifdef CONFIG_SYSFS
+
+int ena_sysfs_init(struct device *dev);
+
+void ena_sysfs_terminate(struct device *dev);
+
+#else /* CONFIG_SYSFS */
+
+static inline int ena_sysfs_init(struct device *dev)
+{
+	return 0;
+}
+
+static inline void ena_sysfs_terminate(struct device *dev)
+{
+}
+
+#endif /* CONFIG_SYSFS */
+
+#endif /* __ENA_SYSFS_H__ */
-- 
2.47.1

[PATCH v8 net-next 4/5] net: ena: PHC stats through sysfs

[David Arinzon]

The patch allows retrieving PHC statistics
through sysfs.
In case the feature is not enabled (through `phc_enable`
sysfs entry), no output will be written.

Signed-off-by: David Arinzon <darinzon@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_sysfs.c | 80 +++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/net/ethernet/amazon/ena/ena_sysfs.c b/drivers/net/ethernet/amazon/ena/ena_sysfs.c
index d0ded92d..441085d2 100644
--- a/drivers/net/ethernet/amazon/ena/ena_sysfs.c
+++ b/drivers/net/ethernet/amazon/ena/ena_sysfs.c
@@ -65,6 +65,82 @@ static ssize_t ena_phc_enable_get(struct device *dev,
 static DEVICE_ATTR(phc_enable, S_IRUGO | S_IWUSR, ena_phc_enable_get,
 		   ena_phc_enable_set);
 
+/* Takes into account max u64 value, null and new line characters */
+#define ENA_PHC_STAT_MAX_LEN 22
+
+static ssize_t ena_phc_cnt_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct ena_adapter *adapter = dev_get_drvdata(dev);
+
+	if (!ena_phc_is_active(adapter))
+		return 0;
+
+	return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n",
+			adapter->ena_dev->phc.stats.phc_cnt);
+}
+
+static DEVICE_ATTR(phc_cnt, S_IRUGO, ena_phc_cnt_show, NULL);
+
+static ssize_t ena_phc_exp_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct ena_adapter *adapter = dev_get_drvdata(dev);
+
+	if (!ena_phc_is_active(adapter))
+		return 0;
+
+	return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n",
+			adapter->ena_dev->phc.stats.phc_exp);
+}
+
+static DEVICE_ATTR(phc_exp, S_IRUGO, ena_phc_exp_show, NULL);
+
+static ssize_t ena_phc_skp_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct ena_adapter *adapter = dev_get_drvdata(dev);
+
+	if (!ena_phc_is_active(adapter))
+		return 0;
+
+	return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n",
+			adapter->ena_dev->phc.stats.phc_skp);
+}
+
+static DEVICE_ATTR(phc_skp, S_IRUGO, ena_phc_skp_show, NULL);
+
+static ssize_t ena_phc_err_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct ena_adapter *adapter = dev_get_drvdata(dev);
+
+	if (!ena_phc_is_active(adapter))
+		return 0;
+
+	return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n",
+			adapter->ena_dev->phc.stats.phc_err);
+}
+
+static DEVICE_ATTR(phc_err, S_IRUGO, ena_phc_err_show, NULL);
+
+static struct attribute *phc_stats_attrs[] = {
+	&dev_attr_phc_cnt.attr,
+	&dev_attr_phc_exp.attr,
+	&dev_attr_phc_skp.attr,
+	&dev_attr_phc_err.attr,
+	NULL
+};
+
+static struct attribute_group phc_stats_group = {
+	.attrs = phc_stats_attrs,
+	.name = "phc_stats",
+};
+
 /******************************************************************************
  *****************************************************************************/
 int ena_sysfs_init(struct device *dev)
@@ -72,6 +148,9 @@ int ena_sysfs_init(struct device *dev)
 	if (device_create_file(dev, &dev_attr_phc_enable))
 		dev_err(dev, "Failed to create phc_enable sysfs entry");
 
+	if (device_add_group(dev, &phc_stats_group))
+		dev_err(dev, "Failed to create phc_stats sysfs group");
+
 	return 0;
 }
 
@@ -80,4 +159,5 @@ int ena_sysfs_init(struct device *dev)
 void ena_sysfs_terminate(struct device *dev)
 {
 	device_remove_file(dev, &dev_attr_phc_enable);
+	device_remove_group(dev, &phc_stats_group);
 }
-- 
2.47.1

[PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[David Arinzon]

Provide the relevant information and guidelines
about the feature support in the ENA driver.

Signed-off-by: Amit Bernstein <amitbern@amazon.com>
Signed-off-by: David Arinzon <darinzon@amazon.com>
---
 .../device_drivers/ethernet/amazon/ena.rst    | 96 +++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/Documentation/networking/device_drivers/ethernet/amazon/ena.rst b/Documentation/networking/device_drivers/ethernet/amazon/ena.rst
index 4561e8ab..158d7100 100644
--- a/Documentation/networking/device_drivers/ethernet/amazon/ena.rst
+++ b/Documentation/networking/device_drivers/ethernet/amazon/ena.rst
@@ -53,9 +53,11 @@ ena_eth_io_defs.h   Definition of ENA data path interface.
 ena_common_defs.h   Common definitions for ena_com layer.
 ena_regs_defs.h     Definition of ENA PCI memory-mapped (MMIO) registers.
 ena_netdev.[ch]     Main Linux kernel driver.
+ena_sysfs.[ch]      Sysfs files.
 ena_ethtool.c       ethtool callbacks.
 ena_xdp.[ch]        XDP files
 ena_pci_id_tbl.h    Supported device IDs.
+ena_phc.[ch]        PTP hardware clock infrastructure (see `PHC`_ for more info)
 =================   ======================================================
 
 Management Interface:
@@ -221,6 +223,100 @@ descriptor it was received on would be recycled. When a packet smaller
 than RX copybreak bytes is received, it is copied into a new memory
 buffer and the RX descriptor is returned to HW.
 
+.. _`PHC`:
+
+PTP Hardware Clock (PHC)
+========================
+.. _`ptp-userspace-api`: https://docs.kernel.org/driver-api/ptp.html#ptp-hardware-clock-user-space-api
+.. _`testptp`: https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/ptp/testptp.c
+
+ENA Linux driver supports PTP hardware clock providing timestamp reference to achieve nanosecond resolution.
+
+**PHC support**
+
+PHC depends on the PTP module, which needs to be either loaded as a module or compiled into the kernel.
+
+Verify if the PTP module is present:
+
+.. code-block:: shell
+
+  grep -w '^CONFIG_PTP_1588_CLOCK=[ym]' /boot/config-`uname -r`
+
+- If no output is provided, the ENA driver cannot be loaded with PHC support.
+
+- ``CONFIG_PTP_1588_CLOCK=y``: the PTP module is already compiled and loaded inside the kernel binary file.
+
+- ``CONFIG_PTP_1588_CLOCK=m``: the PTP module needs to be loaded prior to loading the ENA driver:
+
+Load PTP module:
+
+.. code-block:: shell
+
+  sudo modprobe ptp
+
+**PHC activation**
+
+The feature is turned off by default, in order to turn the feature on,
+please use the following:
+
+- sysfs (during runtime):
+
+.. code-block:: shell
+
+  echo 1 > /sys/bus/pci/devices/<domain:bus:slot.function>/phc_enable
+
+All available PTP clock sources can be tracked here:
+
+.. code-block:: shell
+
+  ls /sys/class/ptp
+
+PHC support and capabilities can be verified using ethtool:
+
+.. code-block:: shell
+
+  ethtool -T <interface>
+
+**PHC timestamp**
+
+To retrieve PHC timestamp, use `ptp-userspace-api`_, usage example using `testptp`_:
+
+.. code-block:: shell
+
+  testptp -d /dev/ptp$(ethtool -T <interface> | awk '/PTP Hardware Clock:/ {print $NF}') -k 1
+
+PHC get time requests should be within reasonable bounds,
+avoid excessive utilization to ensure optimal performance and efficiency.
+The ENA device restricts the frequency of PHC get time requests to a maximum
+of 125 requests per second. If this limit is surpassed, the get time request
+will fail, leading to an increment in the phc_err statistic.
+
+**PHC statistics**
+
+PHC counters can be monitored using the following:
+
+sysfs:
+
+.. code-block:: shell
+
+  cat /sys/bus/pci/devices/<domain:bus:slot.function>/phc_stats/<stat_name>
+
+=================   ======================================================
+**phc_cnt**         Number of successful retrieved timestamps (below expire timeout).
+**phc_exp**         Number of expired retrieved timestamps (above expire timeout).
+**phc_skp**         Number of skipped get time attempts (during block period).
+**phc_err**         Number of failed get time attempts (entering into block state).
+=================   ======================================================
+
+PHC timeouts:
+
+=================   ======================================================
+**expire**          Max time for a valid timestamp retrieval, passing this threshold will fail
+                    the get time request and block new requests until block timeout.
+**block**           Blocking period starts once get time request expires or fails, all get time
+                    requests during block period will be skipped.
+=================   ======================================================
+
 Statistics
 ==========
 
-- 
2.47.1

Re: [PATCH v8 net-next 4/5] net: ena: PHC stats through sysfs

[Andrew Lunn]

> +static ssize_t ena_phc_exp_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct ena_adapter *adapter = dev_get_drvdata(dev);
> +
> +	if (!ena_phc_is_active(adapter))
> +		return 0;
> +
> +	return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n",
> +			adapter->ena_dev->phc.stats.phc_exp);

I've not been following previous versions of this patch, so i could be
repeating questions already asked....

ena_adapter represents a netdev?

/* adapter specific private data structure */
struct ena_adapter {
	struct ena_com_dev *ena_dev;
	/* OS defined structs */
	struct net_device *netdev;

So why are you not using the usual statistics interface for a netdev?

	Andrew

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Andrew Lunn]

> +The feature is turned off by default, in order to turn the feature on,
> +please use the following:
> +
> +- sysfs (during runtime):
> +
> +.. code-block:: shell
> +
> +  echo 1 > /sys/bus/pci/devices/<domain:bus:slot.function>/phc_enable
> +
> +All available PTP clock sources can be tracked here:
> +
> +.. code-block:: shell
> +
> +  ls /sys/class/ptp
> +
> +PHC support and capabilities can be verified using ethtool:
> +
> +.. code-block:: shell
> +
> +  ethtool -T <interface>
> +
> +**PHC timestamp**
> +
> +To retrieve PHC timestamp, use `ptp-userspace-api`_, usage example using `testptp`_:
> +
> +.. code-block:: shell
> +
> +  testptp -d /dev/ptp$(ethtool -T <interface> | awk '/PTP Hardware Clock:/ {print $NF}') -k 1

Why is not opening /dev/ptpX sufficient to enable the PHC?

    Andrew

Re: [PATCH v8 net-next 4/5] net: ena: PHC stats through sysfs

[Jakub Kicinski]

On Tue, 4 Mar 2025 22:07:39 +0100 Andrew Lunn wrote:
> I've not been following previous versions of this patch, so i could be
> repeating questions already asked....
> 
> ena_adapter represents a netdev?
> 
> /* adapter specific private data structure */
> struct ena_adapter {
> 	struct ena_com_dev *ena_dev;
> 	/* OS defined structs */
> 	struct net_device *netdev;
> 
> So why are you not using the usual statistics interface for a netdev?

I asked them to do this.
They are using a PTP device as a pure clock. The netdev doesn't support
any HW timestamping, so none of the stats are related to packets. 
The PTP clock could as well be attached to a storage PCIe device. 
In that way it's more like the OCP PTP driver or other pure clocks 
in my eyes. And such drivers use sysfs, given they have no netdev.

Re: [PATCH v8 net-next 0/5] PHC support in ENA driver

[Jakub Kicinski]

On Tue, 4 Mar 2025 21:04:59 +0200 David Arinzon wrote:
> Changes in v8:
> - Create a sysfs entry for each PHC stat

coccicheck says:

drivers/net/ethernet/amazon/ena/ena_sysfs.c:80:8-16: WARNING: please use sysfs_emit or sysfs_emit_at
drivers/net/ethernet/amazon/ena/ena_sysfs.c:61:8-16: WARNING: please use sysfs_emit or sysfs_emit_at
drivers/net/ethernet/amazon/ena/ena_sysfs.c:125:8-16: WARNING: please use sysfs_emit or sysfs_emit_at
drivers/net/ethernet/amazon/ena/ena_sysfs.c:95:8-16: WARNING: please use sysfs_emit or sysfs_emit_at
drivers/net/ethernet/amazon/ena/ena_sysfs.c:110:8-16: WARNING: please use sysfs_emit or sysfs_emit_at
-- 
pw-bot: cr

RE: [PATCH v8 net-next 0/5] PHC support in ENA driver

[Arinzon, David]

> > Changes in v8:
> > - Create a sysfs entry for each PHC stat
> 
> coccicheck says:
> 
> drivers/net/ethernet/amazon/ena/ena_sysfs.c:80:8-16: WARNING: please
> use sysfs_emit or sysfs_emit_at
> drivers/net/ethernet/amazon/ena/ena_sysfs.c:61:8-16: WARNING: please
> use sysfs_emit or sysfs_emit_at
> drivers/net/ethernet/amazon/ena/ena_sysfs.c:125:8-16: WARNING: please
> use sysfs_emit or sysfs_emit_at
> drivers/net/ethernet/amazon/ena/ena_sysfs.c:95:8-16: WARNING: please
> use sysfs_emit or sysfs_emit_at
> drivers/net/ethernet/amazon/ena/ena_sysfs.c:110:8-16: WARNING: please
> use sysfs_emit or sysfs_emit_at
> --
> pw-bot: cr

Thank you, Jakub.
Will fix in v9

RE: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Arinzon, David]

> > +The feature is turned off by default, in order to turn the feature
> > +on, please use the following:
> > +
> > +- sysfs (during runtime):
> > +
> > +.. code-block:: shell
> > +
> > +  echo 1 > /sys/bus/pci/devices/<domain:bus:slot.function>/phc_enable
> > +
> > +All available PTP clock sources can be tracked here:
> > +
> > +.. code-block:: shell
> > +
> > +  ls /sys/class/ptp
> > +
> > +PHC support and capabilities can be verified using ethtool:
> > +
> > +.. code-block:: shell
> > +
> > +  ethtool -T <interface>
> > +
> > +**PHC timestamp**
> > +
> > +To retrieve PHC timestamp, use `ptp-userspace-api`_, usage example
> using `testptp`_:
> > +
> > +.. code-block:: shell
> > +
> > +  testptp -d /dev/ptp$(ethtool -T <interface> | awk '/PTP Hardware
> > + Clock:/ {print $NF}') -k 1
> 
> Why is not opening /dev/ptpX sufficient to enable the PHC?
> 
>     Andrew

Hi Andrew,

The reasoning for the enablement option of PHC was explained in patch 3 in the series
https://lore.kernel.org/netdev/20250304190504.3743-4-darinzon@amazon.com/

David

Re: [EXTERNAL] [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

On Wed, 2025-03-05 at 12:50 +0000, Arinzon, David wrote:
>  
> > Why is not opening /dev/ptpX sufficient to enable the PHC?
> > 
> >      Andrew
> 
> Hi Andrew,
> 
> The reasoning for the enablement option of PHC was explained in patch 3 in the series
> https://lore.kernel.org/netdev/20250304190504.3743-4-darinzon@amazon.com/

Not really. That says,

"The PHC feature is disabled by default, although its
footprint is small, most customers do not require such
high-precision timestamping. Enabling PHC unnecessarily
in environments that do not need it might introduce a
minor but unnecessary overhead."

Disable by default, sure. But it gives no explanation for why we need a
separate knob in sysfs to enable/disable it, and we can't just enable
it automatically when /dev/ptpX is opened as Andrew suggested.

If you read the actual code in that patch, there's a hint though.
Because the actual process in ena_phc_enable_set() does the following:

+	ena_destroy_device(adapter, false);
+	rc = ena_restore_device(adapter);

Is that actually tearing down the whole netdev and recreating it when
the PHC enable is flipped? Does it really have to? That doesn't seem
ideal even if it *is* a separate knob in sysfs.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH v8 net-next 4/5] net: ena: PHC stats through sysfs

[Andrew Lunn]

On Tue, Mar 04, 2025 at 02:58:57PM -0800, Jakub Kicinski wrote:
> On Tue, 4 Mar 2025 22:07:39 +0100 Andrew Lunn wrote:
> > I've not been following previous versions of this patch, so i could be
> > repeating questions already asked....
> > 
> > ena_adapter represents a netdev?
> > 
> > /* adapter specific private data structure */
> > struct ena_adapter {
> > 	struct ena_com_dev *ena_dev;
> > 	/* OS defined structs */
> > 	struct net_device *netdev;
> > 
> > So why are you not using the usual statistics interface for a netdev?
> 
> I asked them to do this.
> They are using a PTP device as a pure clock. The netdev doesn't support
> any HW timestamping, so none of the stats are related to packets.

So how intertwined is the PHC with the network device? Can it be
separated into a different driver? Moved into drivers/ptp?

We have already been asked if this means network drivers can be
configured via sysfs. Clearly we don't want that, so we want to get
this code out of drivers/net if possible.

	Andrew

Re: [EXTERNAL] [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Andrew Lunn]

> If you read the actual code in that patch, there's a hint though.
> Because the actual process in ena_phc_enable_set() does the following:
> 
> +	ena_destroy_device(adapter, false);
> +	rc = ena_restore_device(adapter);
> 
> Is that actually tearing down the whole netdev and recreating it when
> the PHC enable is flipped?

Well Jakub said it is a pure clock, not related to the netdev. If that
is true, i don't see why this should be needed. But i've not looked at
the code...

	Andrew

   

Re: [EXTERNAL] [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Leon Romanovsky]

On Wed, Mar 05, 2025 at 04:35:48PM +0100, Andrew Lunn wrote:
> > If you read the actual code in that patch, there's a hint though.
> > Because the actual process in ena_phc_enable_set() does the following:
> > 
> > +	ena_destroy_device(adapter, false);
> > +	rc = ena_restore_device(adapter);
> > 
> > Is that actually tearing down the whole netdev and recreating it when
> > the PHC enable is flipped?
> 
> Well Jakub said it is a pure clock, not related to the netdev. 

So why are you ready to merge the code which is not netdev, doesn't have
any association with netdev and doesn't follow netdev semantics (no
custom sysfs files)?

Thanks

Re: [PATCH v8 net-next 4/5] net: ena: PHC stats through sysfs

[Jakub Kicinski]

On Wed, 5 Mar 2025 16:33:10 +0100 Andrew Lunn wrote:
> > I asked them to do this.
> > They are using a PTP device as a pure clock. The netdev doesn't support
> > any HW timestamping, so none of the stats are related to packets.  
> 
> So how intertwined is the PHC with the network device? Can it be
> separated into a different driver? Moved into drivers/ptp?
> 
> We have already been asked if this means network drivers can be
> configured via sysfs. Clearly we don't want that, so we want to get
> this code out of drivers/net if possible.

Is it good enough to move the relevant code to a ptp/ or phc/ dir
under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would require
some weird abstractions, not sure if it's warranted? 

Re: [PATCH v8 net-next 4/5] net: ena: PHC stats through sysfs

[Leon Romanovsky]

On Wed, Mar 05, 2025 at 06:36:37PM -0800, Jakub Kicinski wrote:
> On Wed, 5 Mar 2025 16:33:10 +0100 Andrew Lunn wrote:
> > > I asked them to do this.
> > > They are using a PTP device as a pure clock. The netdev doesn't support
> > > any HW timestamping, so none of the stats are related to packets.  
> > 
> > So how intertwined is the PHC with the network device? Can it be
> > separated into a different driver? Moved into drivers/ptp?
> > 
> > We have already been asked if this means network drivers can be
> > configured via sysfs. Clearly we don't want that, so we want to get
> > this code out of drivers/net if possible.
> 
> Is it good enough to move the relevant code to a ptp/ or phc/ dir
> under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would require
> some weird abstractions, not sure if it's warranted? 

In normal world, where linux kernel driver model is respected, one will write
separate driver for PTP and place it under drivers/ptp. This current series
doesn't belong to netdev at all.

Thanks

> 

Re: [PATCH v8 net-next 4/5] net: ena: PHC stats through sysfs

[Andrew Lunn]

On Wed, Mar 05, 2025 at 06:36:37PM -0800, Jakub Kicinski wrote:
> On Wed, 5 Mar 2025 16:33:10 +0100 Andrew Lunn wrote:
> > > I asked them to do this.
> > > They are using a PTP device as a pure clock. The netdev doesn't support
> > > any HW timestamping, so none of the stats are related to packets.  
> > 
> > So how intertwined is the PHC with the network device? Can it be
> > separated into a different driver? Moved into drivers/ptp?
> > 
> > We have already been asked if this means network drivers can be
> > configured via sysfs. Clearly we don't want that, so we want to get
> > this code out of drivers/net if possible.
> 
> Is it good enough to move the relevant code to a ptp/ or phc/ dir
> under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would require
> some weird abstractions, not sure if it's warranted? 

mtd devices have been doing this for decades. And the auxiliary bus
seems to be a reinvention of the mtd concepts.

As i said, it comes down to how intertwined the PHC is with the
network device.

	Andrew

Re: [PATCH v8 net-next 4/5] net: ena: PHC stats through sysfs

[Leon Romanovsky]

On Thu, Mar 06, 2025 at 04:40:55PM +0100, Andrew Lunn wrote:
> On Wed, Mar 05, 2025 at 06:36:37PM -0800, Jakub Kicinski wrote:
> > On Wed, 5 Mar 2025 16:33:10 +0100 Andrew Lunn wrote:
> > > > I asked them to do this.
> > > > They are using a PTP device as a pure clock. The netdev doesn't support
> > > > any HW timestamping, so none of the stats are related to packets.  
> > > 
> > > So how intertwined is the PHC with the network device? Can it be
> > > separated into a different driver? Moved into drivers/ptp?
> > > 
> > > We have already been asked if this means network drivers can be
> > > configured via sysfs. Clearly we don't want that, so we want to get
> > > this code out of drivers/net if possible.
> > 
> > Is it good enough to move the relevant code to a ptp/ or phc/ dir
> > under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would require
> > some weird abstractions, not sure if it's warranted? 
> 
> mtd devices have been doing this for decades. And the auxiliary bus
> seems to be a reinvention of the mtd concepts.

No, it is not. MTD concepts are no different from standard
register_to_other_subsystem practice, where driver stays in
one subsystem to be used by another.

Auxillary bus is different in that it splits drivers to their logical
parts and places them in right subsystems.

Thanks

RE: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Arinzon, David]

> > > If you read the actual code in that patch, there's a hint though.
> > > Because the actual process in ena_phc_enable_set() does the following:
> > >
> > > +   ena_destroy_device(adapter, false);
> > > +   rc = ena_restore_device(adapter);
> > >
> > > Is that actually tearing down the whole netdev and recreating it
> > > when the PHC enable is flipped?
> >
> > Well Jakub said it is a pure clock, not related to the netdev.

The PHC device is a PTP clock integrated with the networking device under the same PCI device.  
As previously mentioned in this thread, enabling or disabling the ENA PHC requires reconfiguring the ENA network device, 
which involves tearing down and recreating the netdev. 
This necessitates having a separate control knob.

Thanks,
David

> 
> So why are you ready to merge the code which is not netdev, doesn't have
> any association with netdev and doesn't follow netdev semantics (no custom
> sysfs files)?
> 
> Thanks

RE: [PATCH v8 net-next 4/5] net: ena: PHC stats through sysfs

[Arinzon, David]

> > > > > > I've not been following previous versions of this patch, so i could 
> > > > > > be repeating questions already asked....
> > > > > >
> > > > > > ena_adapter represents a netdev?
> > > > > >

Yes, ena_adapter represents a netdev.

> > > > > > /* adapter specific private data structure */ struct ena_adapter {
> > > > > >     struct ena_com_dev *ena_dev;
> > > > > >     /* OS defined structs */
> > > > > >     struct net_device *netdev;
> > > > > >
> > > > > > So why are you not using the usual statistics interface for a netdev?
> > > > >
> > > > > I asked them to do this.
> > > > > They are using a PTP device as a pure clock. The netdev doesn't
> > > > > support any HW timestamping, so none of the stats are related to packets.
> > > >
> > > > So how intertwined is the PHC with the network device? Can it be
> > > > separated into a different driver? Moved into drivers/ptp?
> > > >

The PHC device is not a HW timestamping device but rather a PTP clock
that is integrated with the networking device under the same PCI device.
Enabling or disabling the ENA PHC requires reconfiguring the ENA network device.

> > > > We have already been asked if this means network drivers can be
> > > > configured via sysfs. Clearly we don't want that, so we want to
> > > > get this code out of drivers/net if possible.
> > >
> > > Is it good enough to move the relevant code to a ptp/ or phc/ dir
> > > under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would
> > > require some weird abstractions, not sure if it's warranted?

We agree that relocating the PHC code to drivers/ptp would introduce unnecessary
abstractions and require synchronization mechanisms between drivers.

As with other ENA features, the PHC-related code is contained within
a dedicated file in the ethernet/amazon/ena/ directory.
Would this be an acceptable approach?

Thanks,
David

> > mtd devices have been doing this for decades. And the auxiliary bus
> > seems to be a reinvention of the mtd concepts.
> 
> No, it is not. MTD concepts are no different from standard
> register_to_other_subsystem practice, where driver stays in one subsystem
> to be used by another.
> 
> Auxillary bus is different in that it splits drivers to their logical parts and places
> them in right subsystems.
> 
> Thanks

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Leon Romanovsky]

On Wed, Mar 26, 2025 at 03:32:00PM +0000, Arinzon, David wrote:
> > > > If you read the actual code in that patch, there's a hint though.
> > > > Because the actual process in ena_phc_enable_set() does the following:
> > > >
> > > > +   ena_destroy_device(adapter, false);
> > > > +   rc = ena_restore_device(adapter);
> > > >
> > > > Is that actually tearing down the whole netdev and recreating it
> > > > when the PHC enable is flipped?
> > >
> > > Well Jakub said it is a pure clock, not related to the netdev.
> 
> The PHC device is a PTP clock integrated with the networking device under the same PCI device.  
> As previously mentioned in this thread, enabling or disabling the ENA PHC requires reconfiguring the ENA network device, 
> which involves tearing down and recreating the netdev. 
> This necessitates having a separate control knob.

I appreciate the desire to keep everything in one place and provide
custom, vendor specific sysfs files. However, there is standard way
(auxbus??) to solve it without need to reinvent anything.

Thanks

> 
> Thanks,
> David
> 
> > 
> > So why are you ready to merge the code which is not netdev, doesn't have
> > any association with netdev and doesn't follow netdev semantics (no custom
> > sysfs files)?
> > 
> > Thanks

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

On Thu, 2025-03-27 at 13:15 +0200, Leon Romanovsky wrote:
> On Wed, Mar 26, 2025 at 03:32:00PM +0000, Arinzon, David wrote:
> > > > > If you read the actual code in that patch, there's a hint though.
> > > > > Because the actual process in ena_phc_enable_set() does the following:
> > > > > 
> > > > > +   ena_destroy_device(adapter, false);
> > > > > +   rc = ena_restore_device(adapter);
> > > > > 
> > > > > Is that actually tearing down the whole netdev and recreating it
> > > > > when the PHC enable is flipped?
> > > > 
> > > > Well Jakub said it is a pure clock, not related to the netdev.
> > 
> > The PHC device is a PTP clock integrated with the networking device under the same PCI device.  
> > As previously mentioned in this thread, enabling or disabling the
> > ENA PHC requires reconfiguring the ENA network device, 
> > which involves tearing down and recreating the netdev. 
> > This necessitates having a separate control knob.
> 
> I appreciate the desire to keep everything in one place and provide
> custom, vendor specific sysfs files. However, there is standard way
> (auxbus??) to solve it without need to reinvent anything.

Using auxbus would allow the PHC driver to be loaded as a separate
module. But AIUI enabling the PHC involves reconfiguring the device and
thus bringing the network device down and back up. Which isn't ideal.
It *isn't* as separate as auxbus would make it look.

We don't want to unconditionally enable the PHC support in the device
even when it's not being used, because of the potential blast radius of
such a change if *every* guest starts enabling it even when they don't
need to use it.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3156 bytes --]

On Tue, 2025-03-04 at 22:10 +0100, Andrew Lunn wrote:
> > +The feature is turned off by default, in order to turn the feature
> > on,
> > +please use the following:
> > +
> > +- sysfs (during runtime):
> > +
> > +.. code-block:: shell
> > +
> > +  echo 1 >
> > /sys/bus/pci/devices/<domain:bus:slot.function>/phc_enable
> > +
> > +All available PTP clock sources can be tracked here:
> > +
> > +.. code-block:: shell
> > +
> > +  ls /sys/class/ptp
> > +
> > +PHC support and capabilities can be verified using ethtool:
> > +
> > +.. code-block:: shell
> > +
> > +  ethtool -T <interface>
> > +
> > +**PHC timestamp**
> > +
> > +To retrieve PHC timestamp, use `ptp-userspace-api`_, usage example
> > using `testptp`_:
> > +
> > +.. code-block:: shell
> > +
> > +  testptp -d /dev/ptp$(ethtool -T <interface> | awk '/PTP Hardware
> > Clock:/ {print $NF}') -k 1
> 
> Why is not opening /dev/ptpX sufficient to enable the PHC?

There's currently no hook from ptp_open() and ptp_release() into the
implementation to do anything like that. It could be added, I suppose,
but it isn't a great experience because the act of opening it for the
first time would then take down the netdev.

None of the alternative bikeshed options that David is being bombarded
with here make sense to me, like auxdev or implicit opening, because
it's tied to the netdev.

In a future revision of the device itself, perhaps we can enable the
PHC completely independently of the network device. And then of course
we can just do it on ptp_open(), and we'll add the required hooks in
struct ptp_clock_info. But for now, that doesn't make sense.

I've been a maintainer. I remember looking at submissions and thinking
"there must be a better way to do this", then attempting five different
options and concluding that the original submission is the one I
dislike least after all. This seems like one of those times, with a
bunch of suggestions which ultimately give a worse user experience.
And a contributor caught in the cross-fire with no coherent guidance
about how to proceed.

I think the sysfs control is the best option here.

On the device side, I am internally applying the cluebat because this
whole thing was *predictable*. The PHC feature *should* have been
orthogonal, in the device, to the actual networking. So it can be
enabled or disabled at will. Eventually I hope a later version of the
device should fix that, and we can have a nice clean user interface —
the /dev/ptpX node can always exist, but it does nothing at all if it
isn't being used. In the interim, a sysfs knob to explicitly turn it on
seems like a reasonable approach to me.

I am ambivalent about *also* using auxdev, so that when the netdev is
told to enable the PHC feature, the auxdev 'appears' and a separate PTP
driver binds to it. It's kind of cute, but probably overkill, and not
what any other NIC driver does, is it? CONFIG_PTP_1588_CLOCK_OPTIONAL
exists to handle the case of NICs with optional PTP support.
(Actually it turns out I'm less ambivalent by the end of that paragraph
than I was at the start... :)

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

On Tue, 2025-04-01 at 09:02 +0100, David Woodhouse wrote:
> 
> I think the sysfs control is the best option here.

Actually, it occurs to me that the best option is probably a module
parameter. If you have to take the network down and up to change the
mode, why not just unload and reload the module?

I understand there's a policy about that too, because we don't want
bespoke module parameters changing the behaviour of network devices,
which ought to be uniform. But this isn't changing the behaviour of the
network device per se.

Think of it as three drivers. One for the PCI device itself, which
registers one or two auxdevs (depending on a module option). This
module option is not on the network device driver. Then there's the
netdev and the PTP which bind to those auxdevs.

Now in *practice*, actually using auxdev is overkill and not consistent
with what other network+PTP drivers do, and I think it should remain in
a single driver. I'm using the separation to explain why a module
parameter wouldn't be conceptually on the "network" device, and why we
should consider this one of the cases where "policy" is just a
euphemism for not doing our own thinking.

Just put a module param on the PCI device driver and have done with it.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Jakub Kicinski]

On Tue, 01 Apr 2025 09:46:35 +0100 David Woodhouse wrote:
> On Tue, 2025-04-01 at 09:02 +0100, David Woodhouse wrote:
> > 
> > I think the sysfs control is the best option here.  
> 
> Actually, it occurs to me that the best option is probably a module
> parameter. If you have to take the network down and up to change the
> mode, why not just unload and reload the module?

We have something called devlink params, which support "configuration
modes" (= what level of reset is required to activate the new setting).
Maybe devlink param with cmode of "driver init" would be the best fit?

Module params are annoying because they are scoped to code / module not
instances of the device.

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Leon Romanovsky]

On Wed, Apr 2, 2025, at 19:23, Jakub Kicinski wrote:
> On Tue, 01 Apr 2025 09:46:35 +0100 David Woodhouse wrote:
>> On Tue, 2025-04-01 at 09:02 +0100, David Woodhouse wrote:
>> > 
>> > I think the sysfs control is the best option here.  
>> 
>> Actually, it occurs to me that the best option is probably a module
>> parameter. If you have to take the network down and up to change the
>> mode, why not just unload and reload the module?
>
> We have something called devlink params, which support "configuration
> modes" (= what level of reset is required to activate the new setting).
> Maybe devlink param with cmode of "driver init" would be the best fit?

I had same feeling when I wrote my auxbus response. There is no reason to believe that ptp enable/disable knob won't be usable by other drivers 

It's universally usable, just not related to netdev sysfs layout.

Thanks 

>
> Module params are annoying because they are scoped to code / module not
> instances of the device.

RE: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Arinzon, David]

> >> > I think the sysfs control is the best option here.
> >>
> >> Actually, it occurs to me that the best option is probably a module
> >> parameter. If you have to take the network down and up to change the
> >> mode, why not just unload and reload the module?
> >
> > We have something called devlink params, which support "configuration
> > modes" (= what level of reset is required to activate the new setting).
> > Maybe devlink param with cmode of "driver init" would be the best fit?
> 
> I had same feeling when I wrote my auxbus response. There is no reason to
> believe that ptp enable/disable knob won't be usable by other drivers
> 
> It's universally usable, just not related to netdev sysfs layout.
> 
> Thanks
> 
> >
> > Module params are annoying because they are scoped to code / module
> > not instances of the device.

Hi Jakub,

Thanks for suggesting the devlink params option for enable/disable, we will
explore the option and provide a revised patchset.

Given the pushback on custom sysfs utilization, what can be the alternative for exposing 
the PHC statistics? If `ethtool -S` is not an option, is there another framework that
allows outputting statistics?
We've explored devlink health reporter dump, would that be acceptable?

Thanks,
David

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Andrew Lunn]

On Mon, Apr 07, 2025 at 07:01:46AM +0000, Arinzon, David wrote:
> > >> > I think the sysfs control is the best option here.
> > >>
> > >> Actually, it occurs to me that the best option is probably a module
> > >> parameter. If you have to take the network down and up to change the
> > >> mode, why not just unload and reload the module?
> > >
> > > We have something called devlink params, which support "configuration
> > > modes" (= what level of reset is required to activate the new setting).
> > > Maybe devlink param with cmode of "driver init" would be the best fit?
> > 
> > I had same feeling when I wrote my auxbus response. There is no reason to
> > believe that ptp enable/disable knob won't be usable by other drivers
> > 
> > It's universally usable, just not related to netdev sysfs layout.
> > 
> > Thanks
> > 
> > >
> > > Module params are annoying because they are scoped to code / module
> > > not instances of the device.
> 
> Hi Jakub,
> 
> Thanks for suggesting the devlink params option for enable/disable, we will
> explore the option and provide a revised patchset.
> 
> Given the pushback on custom sysfs utilization, what can be the alternative for exposing 
> the PHC statistics? If `ethtool -S` is not an option, is there another framework that
> allows outputting statistics?
> We've explored devlink health reporter dump, would that be acceptable?

We seem to be going backwards and forwards between this is connected
to a netdev and it is not connected to a netdev. You have to destroy
and recreate the netdev in order to make us if it, which might just be
FUBAR design, but that is what you have. So maybe ethtool -S is an
option?

Or take a step back. Are your statistics specific to your hardware, or
generic about any PTP clock? Could you expand the PTP infrastructure
to return generic statistics? The problem being, PTP is currently
IOCTL based, so not very expandable, unlike netlink used for ethtool.

	Andrew

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Jakub Kicinski]

On Mon, 7 Apr 2025 16:02:33 +0200 Andrew Lunn wrote:
> > Thanks for suggesting the devlink params option for enable/disable, we will
> > explore the option and provide a revised patchset.
> > 
> > Given the pushback on custom sysfs utilization, what can be the alternative for exposing 
> > the PHC statistics? If `ethtool -S` is not an option, is there another framework that
> > allows outputting statistics?
> > We've explored devlink health reporter dump, would that be acceptable?  
> 
> We seem to be going backwards and forwards between this is connected
> to a netdev and it is not connected to a netdev. You have to destroy
> and recreate the netdev in order to make us if it, which might just be
> FUBAR design, but that is what you have. So maybe ethtool -S is an
> option?

The device has to be reinitialized, I don't think full re-initialization
makes the feature connected to other functionality.

> Or take a step back. Are your statistics specific to your hardware, or
> generic about any PTP clock? Could you expand the PTP infrastructure
> to return generic statistics? The problem being, PTP is currently
> IOCTL based, so not very expandable, unlike netlink used for ethtool.

Historic parts are IOCTL-based, but extended functionality for PTP core
and drivers is currently implemented using sysfs.

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Richard Cochran]

On Mon, Apr 07, 2025 at 09:27:49AM -0700, Jakub Kicinski wrote:

> 
> Historic parts are IOCTL-based, but extended functionality for PTP core
> and drivers is currently implemented using sysfs.

The intent was always to offer the same configuration possibilities
over ioctl and sysfs.  That way, the user can choose a light weight
scripting solution or integrate with a binary.

But overall I don't those interfaces are great for reporting
statistics.  netlink seems better.

Thanks,
Richard

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Jakub Kicinski]

On Tue, 8 Apr 2025 07:47:22 -0700 Richard Cochran wrote:
> But overall I don't those interfaces are great for reporting
> statistics.  netlink seems better.

Just to be clear, are you asking them to reimplement PTP config
in netlink just to report 4 counters?

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Richard Cochran]

On Tue, Apr 08, 2025 at 08:24:39AM -0700, Jakub Kicinski wrote:
> On Tue, 8 Apr 2025 07:47:22 -0700 Richard Cochran wrote:
> > But overall I don't those interfaces are great for reporting
> > statistics.  netlink seems better.
> 
> Just to be clear, are you asking them to reimplement PTP config
> in netlink just to report 4 counters?

No, maybe four counters can go into debugfs for this device?

Thanks,
Richard

Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

[Jakub Kicinski]

On Wed, 9 Apr 2025 21:07:59 -0700 Richard Cochran wrote:
> On Tue, Apr 08, 2025 at 08:24:39AM -0700, Jakub Kicinski wrote:
> > On Tue, 8 Apr 2025 07:47:22 -0700 Richard Cochran wrote:  
> > > But overall I don't those interfaces are great for reporting
> > > statistics.  netlink seems better.  
> > 
> > Just to be clear, are you asking them to reimplement PTP config
> > in netlink just to report 4 counters?  
> 
> No, maybe four counters can go into debugfs for this device?

Works for me, FWIW.