diff mbox series

[iwl-next,v10,06/14] iavf: add initial framework for registering PTP clock

Message ID 20240821121539.374343-7-wojciech.drewek@intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series Add support for Rx timestamping for both ice and iavf drivers | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Wojciech Drewek Aug. 21, 2024, 12:15 p.m. UTC 
From: Jacob Keller <jacob.e.keller@intel.com>

Add the iavf_ptp.c file and fill it in with a skeleton framework to
allow registering the PTP clock device.
Add implementation of helper functions to check if a PTP capability
is supported and handle change in PTP capabilities.
Enabling virtual clock would be possible, though it would probably
perform poorly due to the lack of direct time access.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Reviewed-by: Sai Krishna <saikrishnag@marvell.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Co-developed-by: Ahmed Zaki <ahmed.zaki@intel.com>
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
 drivers/net/ethernet/intel/iavf/Makefile      |   2 +
 drivers/net/ethernet/intel/iavf/iavf_main.c   |   6 +
 drivers/net/ethernet/intel/iavf/iavf_ptp.c    | 122 ++++++++++++++++++
 drivers/net/ethernet/intel/iavf/iavf_ptp.h    |   5 +
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   |   8 ++
 5 files changed, 143 insertions(+)
 create mode 100644 drivers/net/ethernet/intel/iavf/iavf_ptp.c

Comments

Alexander Lobakin Aug. 21, 2024, 2:20 p.m. UTC | #1 
From: Wojciech Drewek <wojciech.drewek@intel.com>
Date: Wed, 21 Aug 2024 14:15:31 +0200

> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> Add the iavf_ptp.c file and fill it in with a skeleton framework to
> allow registering the PTP clock device.
> Add implementation of helper functions to check if a PTP capability
> is supported and handle change in PTP capabilities.
> Enabling virtual clock would be possible, though it would probably
> perform poorly due to the lack of direct time access.

[...]

> +/**
> + * iavf_ptp_register_clock - Register a new PTP for userspace
> + * @adapter: private adapter structure
> + *
> + * Allocate and register a new PTP clock device if necessary.
> + *
> + * Return: 0 if success, error otherwise

Period ('.') at the end is desired at the end of kdoc.

> + */
> +static int iavf_ptp_register_clock(struct iavf_adapter *adapter)
> +{
> +	struct ptp_clock_info *ptp_info = &adapter->ptp.info;
> +	struct device *dev = &adapter->pdev->dev;
> +
> +	memset(ptp_info, 0, sizeof(*ptp_info));

Is this needed? adapter is allocated using kzalloc() I think?

> +
> +	snprintf(ptp_info->name, sizeof(ptp_info->name), "%s-%s-clk",
> +		 dev_driver_string(dev), dev_name(dev));

dev_driver_string() can be just KBUILD_MODNAME when it's called inside
the actual module. It's mostly used when you need to get a module name
from a different module or core kernel code.

> +	ptp_info->owner = THIS_MODULE;
> +
> +	adapter->ptp.clock = ptp_clock_register(ptp_info, dev);
> +	if (IS_ERR(adapter->ptp.clock)) {
> +		adapter->ptp.clock = NULL;
> +
> +		return PTR_ERR(adapter->ptp.clock);

Braino here.
You first set ptp.clock to %NULL and then return PTR_ERR(ptp.clock).
IOW, this error path will always return 0.

I usually use temporary variables to avoid this.

	clock = ptp_clock_register(ptp_info, dev);
	if (IS_ERR(clock))
		return PTR_ERR(clock);

	adapter->ptp.clock = clock;


> +	}
> +
> +	dev_dbg(&adapter->pdev->dev, "PTP clock %s registered\n",
> +		adapter->ptp.info.name);
> +
> +	return 0;
> +}
> +
> +/**
> + * iavf_ptp_init - Initialize PTP support if capability was negotiated
> + * @adapter: private adapter structure
> + *
> + * Initialize PTP functionality, based on the capabilities that the PF has
> + * enabled for this VF.
> + */
> +void iavf_ptp_init(struct iavf_adapter *adapter)
> +{
> +	int err;
> +
> +	if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) {
> +		pci_warn(adapter->pdev,
> +			 "Device does not have PTP clock support\n");

I think it's pci_notice() or even pci_dbg(). A device can miss PTP
clock, but it's not a failure. _warn() is when something went wrong, but
not as wrong as _err() :D

> +		return;
> +	}
> +
> +	err = iavf_ptp_register_clock(adapter);
> +	if (err) {
> +		pci_err(adapter->pdev,
> +			"Failed to register PTP clock device (%p)\n",
> +			ERR_PTR(err));
> +		return;
> +	}

Why does this function return void if there's an error path? To make
sure the driver works even if PTP fails to register? But I think it's
better to bail out if something failed than to work without certain
functionality?

> +
> +	adapter->ptp.initialized = true;
> +}
> +
> +/**
> + * iavf_ptp_release - Disable PTP support
> + * @adapter: private adapter structure
> + *
> + * Release all PTP resources that were previously initialized.
> + */
> +void iavf_ptp_release(struct iavf_adapter *adapter)
> +{
> +	adapter->ptp.initialized = false;
> +
> +	if (!IS_ERR_OR_NULL(adapter->ptp.clock)) {

Since you always assign clock to %NULL when the initialization failed,
this could be just

	if (adapter->ptp.clock)

> +		dev_dbg(&adapter->pdev->dev, "removing PTP clock %s\n",
> +			adapter->ptp.info.name);

pci_dbg()

> +		ptp_clock_unregister(adapter->ptp.clock);
> +		adapter->ptp.clock = NULL;
> +	}

...but I'd invert the condition to avoid +1 indent level.

	if (!adapter->ptp.clock)
		return;

	pci_dbg() ...

> +}
> +
> +/**
> + * iavf_ptp_process_caps - Handle change in PTP capabilities
> + * @adapter: private adapter structure
> + *
> + * Handle any state changes necessary due to change in PTP capabilities, such
> + * as after a device reset or change in configuration from the PF.
> + */
> +void iavf_ptp_process_caps(struct iavf_adapter *adapter)
> +{
> +	bool read_phc = iavf_ptp_cap_supported(adapter,
> +					       VIRTCHNL_1588_PTP_CAP_READ_PHC);

Maybe split the declaration and initialization to avoid line break? My
editor says it would fit in 80 if you make the variable name shorter,
e.g. 'phc'.

> +
> +	/* Check if the device gained or lost necessary access to support the
> +	 * PTP hardware clock. If so, driver must respond appropriately by
> +	 * creating or destroying the PTP clock device.
> +	 */
> +	if (adapter->ptp.initialized && !read_phc)
> +		iavf_ptp_release(adapter);
> +	else if (!adapter->ptp.initialized && read_phc)
> +		iavf_ptp_init(adapter);
> +}

Thanks,
Olek
Wojciech Drewek Aug. 27, 2024, 10:59 a.m. UTC | #2 
On 21.08.2024 16:20, Alexander Lobakin wrote:
> From: Wojciech Drewek <wojciech.drewek@intel.com>
> Date: Wed, 21 Aug 2024 14:15:31 +0200
> 
>> From: Jacob Keller <jacob.e.keller@intel.com>
>>
>> Add the iavf_ptp.c file and fill it in with a skeleton framework to
>> allow registering the PTP clock device.
>> Add implementation of helper functions to check if a PTP capability
>> is supported and handle change in PTP capabilities.
>> Enabling virtual clock would be possible, though it would probably
>> perform poorly due to the lack of direct time access.
> 
> [...]
> 
>> +/**
>> + * iavf_ptp_register_clock - Register a new PTP for userspace
>> + * @adapter: private adapter structure
>> + *
>> + * Allocate and register a new PTP clock device if necessary.
>> + *
>> + * Return: 0 if success, error otherwise
> 
> Period ('.') at the end is desired at the end of kdoc.

Sure

> 
>> + */
>> +static int iavf_ptp_register_clock(struct iavf_adapter *adapter)
>> +{
>> +	struct ptp_clock_info *ptp_info = &adapter->ptp.info;
>> +	struct device *dev = &adapter->pdev->dev;
>> +
>> +	memset(ptp_info, 0, sizeof(*ptp_info));
> 
> Is this needed? adapter is allocated using kzalloc() I think?

I think it's not needed, adapter is allocated using alloc_etherdev_mq
since this is netdev's priv in iavf

> 
>> +
>> +	snprintf(ptp_info->name, sizeof(ptp_info->name), "%s-%s-clk",
>> +		 dev_driver_string(dev), dev_name(dev));
> 
> dev_driver_string() can be just KBUILD_MODNAME when it's called inside
> the actual module. It's mostly used when you need to get a module name
> from a different module or core kernel code.

Makes sense

> 
>> +	ptp_info->owner = THIS_MODULE;
>> +
>> +	adapter->ptp.clock = ptp_clock_register(ptp_info, dev);
>> +	if (IS_ERR(adapter->ptp.clock)) {
>> +		adapter->ptp.clock = NULL;
>> +
>> +		return PTR_ERR(adapter->ptp.clock);
> 
> Braino here.
> You first set ptp.clock to %NULL and then return PTR_ERR(ptp.clock).
> IOW, this error path will always return 0.
> 
> I usually use temporary variables to avoid this.
> 
> 	clock = ptp_clock_register(ptp_info, dev);
> 	if (IS_ERR(clock))
> 		return PTR_ERR(clock);
> 
> 	adapter->ptp.clock = clock;

will fix

> 
> 
>> +	}
>> +
>> +	dev_dbg(&adapter->pdev->dev, "PTP clock %s registered\n",
>> +		adapter->ptp.info.name);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * iavf_ptp_init - Initialize PTP support if capability was negotiated
>> + * @adapter: private adapter structure
>> + *
>> + * Initialize PTP functionality, based on the capabilities that the PF has
>> + * enabled for this VF.
>> + */
>> +void iavf_ptp_init(struct iavf_adapter *adapter)
>> +{
>> +	int err;
>> +
>> +	if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) {
>> +		pci_warn(adapter->pdev,
>> +			 "Device does not have PTP clock support\n");
> 
> I think it's pci_notice() or even pci_dbg(). A device can miss PTP
> clock, but it's not a failure. _warn() is when something went wrong, but
> not as wrong as _err() :D

sure

> 
>> +		return;
>> +	}
>> +
>> +	err = iavf_ptp_register_clock(adapter);
>> +	if (err) {
>> +		pci_err(adapter->pdev,
>> +			"Failed to register PTP clock device (%p)\n",
>> +			ERR_PTR(err));
>> +		return;
>> +	}
> 
> Why does this function return void if there's an error path? To make
> sure the driver works even if PTP fails to register? But I think it's
> better to bail out if something failed than to work without certain
> functionality?

Most of the drivers don't bail out if ptp init failed, I'll stick to that.

> 
>> +
>> +	adapter->ptp.initialized = true;
>> +}
>> +
>> +/**
>> + * iavf_ptp_release - Disable PTP support
>> + * @adapter: private adapter structure
>> + *
>> + * Release all PTP resources that were previously initialized.
>> + */
>> +void iavf_ptp_release(struct iavf_adapter *adapter)
>> +{
>> +	adapter->ptp.initialized = false;
>> +
>> +	if (!IS_ERR_OR_NULL(adapter->ptp.clock)) {
> 
> Since you always assign clock to %NULL when the initialization failed,
> this could be just

Yep

> 
> 	if (adapter->ptp.clock)
> 
>> +		dev_dbg(&adapter->pdev->dev, "removing PTP clock %s\n",
>> +			adapter->ptp.info.name);
> 
> pci_dbg()
> 
>> +		ptp_clock_unregister(adapter->ptp.clock);
>> +		adapter->ptp.clock = NULL;
>> +	}
> 
> ...but I'd invert the condition to avoid +1 indent level.
> 
> 	if (!adapter->ptp.clock)
> 		return;
> 
> 	pci_dbg() ...

Agree

> 
>> +}
>> +
>> +/**
>> + * iavf_ptp_process_caps - Handle change in PTP capabilities
>> + * @adapter: private adapter structure
>> + *
>> + * Handle any state changes necessary due to change in PTP capabilities, such
>> + * as after a device reset or change in configuration from the PF.
>> + */
>> +void iavf_ptp_process_caps(struct iavf_adapter *adapter)
>> +{
>> +	bool read_phc = iavf_ptp_cap_supported(adapter,
>> +					       VIRTCHNL_1588_PTP_CAP_READ_PHC);
> 
> Maybe split the declaration and initialization to avoid line break? My
> editor says it would fit in 80 if you make the variable name shorter,
> e.g. 'phc'.

Sure, why not

> 
>> +
>> +	/* Check if the device gained or lost necessary access to support the
>> +	 * PTP hardware clock. If so, driver must respond appropriately by
>> +	 * creating or destroying the PTP clock device.
>> +	 */
>> +	if (adapter->ptp.initialized && !read_phc)
>> +		iavf_ptp_release(adapter);
>> +	else if (!adapter->ptp.initialized && read_phc)
>> +		iavf_ptp_init(adapter);
>> +}
> 
> Thanks,
> Olek
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/iavf/Makefile b/drivers/net/ethernet/intel/iavf/Makefile
index 356ac9faa5bf..e13720a728ff 100644
--- a/drivers/net/ethernet/intel/iavf/Makefile
+++ b/drivers/net/ethernet/intel/iavf/Makefile
@@ -13,3 +13,5 @@  obj-$(CONFIG_IAVF) += iavf.o
 
 iavf-y := iavf_main.o iavf_ethtool.o iavf_virtchnl.o iavf_fdir.o \
 	  iavf_adv_rss.o iavf_txrx.o iavf_common.o iavf_adminq.o
+
+iavf-$(CONFIG_PTP_1588_CLOCK) += iavf_ptp.o
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index c26f6fc5250b..8a99aab5bf6f 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -4,6 +4,7 @@ 
 #include <linux/net/intel/libie/rx.h>
 
 #include "iavf.h"
+#include "iavf_ptp.h"
 #include "iavf_prototype.h"
 /* All iavf tracepoints are defined by the include below, which must
  * be included exactly once across the whole kernel with
@@ -2834,6 +2835,9 @@  static void iavf_init_config_adapter(struct iavf_adapter *adapter)
 		/* request initial VLAN offload settings */
 		iavf_set_vlan_offload_features(adapter, 0, netdev->features);
 
+	/* Setup initial PTP configuration */
+	iavf_ptp_init(adapter);
+
 	iavf_schedule_finish_config(adapter);
 	return;
 
@@ -5473,6 +5477,8 @@  static void iavf_remove(struct pci_dev *pdev)
 		msleep(50);
 	}
 
+	iavf_ptp_release(adapter);
+
 	iavf_misc_irq_disable(adapter);
 	/* Shut down all the garbage mashers on the detention level */
 	cancel_work_sync(&adapter->reset_task);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.c b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
new file mode 100644
index 000000000000..045aa8690ac2
--- /dev/null
+++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
@@ -0,0 +1,122 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2024 Intel Corporation. */
+
+#include "iavf.h"
+#include "iavf_ptp.h"
+
+/**
+ * iavf_ptp_cap_supported - Check if a PTP capability is supported
+ * @adapter: private adapter structure
+ * @cap: the capability bitmask to check
+ *
+ * Return: true if every capability set in cap is also set in the enabled
+ *         capabilities reported by the PF, false otherwise.
+ */
+bool iavf_ptp_cap_supported(const struct iavf_adapter *adapter, u32 cap)
+{
+	if (!IAVF_PTP_ALLOWED(adapter))
+		return false;
+
+	/* Only return true if every bit in cap is set in hw_caps.caps */
+	return (adapter->ptp.hw_caps.caps & cap) == cap;
+}
+
+/**
+ * iavf_ptp_register_clock - Register a new PTP for userspace
+ * @adapter: private adapter structure
+ *
+ * Allocate and register a new PTP clock device if necessary.
+ *
+ * Return: 0 if success, error otherwise
+ */
+static int iavf_ptp_register_clock(struct iavf_adapter *adapter)
+{
+	struct ptp_clock_info *ptp_info = &adapter->ptp.info;
+	struct device *dev = &adapter->pdev->dev;
+
+	memset(ptp_info, 0, sizeof(*ptp_info));
+
+	snprintf(ptp_info->name, sizeof(ptp_info->name), "%s-%s-clk",
+		 dev_driver_string(dev), dev_name(dev));
+	ptp_info->owner = THIS_MODULE;
+
+	adapter->ptp.clock = ptp_clock_register(ptp_info, dev);
+	if (IS_ERR(adapter->ptp.clock)) {
+		adapter->ptp.clock = NULL;
+
+		return PTR_ERR(adapter->ptp.clock);
+	}
+
+	dev_dbg(&adapter->pdev->dev, "PTP clock %s registered\n",
+		adapter->ptp.info.name);
+
+	return 0;
+}
+
+/**
+ * iavf_ptp_init - Initialize PTP support if capability was negotiated
+ * @adapter: private adapter structure
+ *
+ * Initialize PTP functionality, based on the capabilities that the PF has
+ * enabled for this VF.
+ */
+void iavf_ptp_init(struct iavf_adapter *adapter)
+{
+	int err;
+
+	if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) {
+		pci_warn(adapter->pdev,
+			 "Device does not have PTP clock support\n");
+		return;
+	}
+
+	err = iavf_ptp_register_clock(adapter);
+	if (err) {
+		pci_err(adapter->pdev,
+			"Failed to register PTP clock device (%p)\n",
+			ERR_PTR(err));
+		return;
+	}
+
+	adapter->ptp.initialized = true;
+}
+
+/**
+ * iavf_ptp_release - Disable PTP support
+ * @adapter: private adapter structure
+ *
+ * Release all PTP resources that were previously initialized.
+ */
+void iavf_ptp_release(struct iavf_adapter *adapter)
+{
+	adapter->ptp.initialized = false;
+
+	if (!IS_ERR_OR_NULL(adapter->ptp.clock)) {
+		dev_dbg(&adapter->pdev->dev, "removing PTP clock %s\n",
+			adapter->ptp.info.name);
+		ptp_clock_unregister(adapter->ptp.clock);
+		adapter->ptp.clock = NULL;
+	}
+}
+
+/**
+ * iavf_ptp_process_caps - Handle change in PTP capabilities
+ * @adapter: private adapter structure
+ *
+ * Handle any state changes necessary due to change in PTP capabilities, such
+ * as after a device reset or change in configuration from the PF.
+ */
+void iavf_ptp_process_caps(struct iavf_adapter *adapter)
+{
+	bool read_phc = iavf_ptp_cap_supported(adapter,
+					       VIRTCHNL_1588_PTP_CAP_READ_PHC);
+
+	/* Check if the device gained or lost necessary access to support the
+	 * PTP hardware clock. If so, driver must respond appropriately by
+	 * creating or destroying the PTP clock device.
+	 */
+	if (adapter->ptp.initialized && !read_phc)
+		iavf_ptp_release(adapter);
+	else if (!adapter->ptp.initialized && read_phc)
+		iavf_ptp_init(adapter);
+}
diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
index 65678e76c34f..c2ed24cef926 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h
+++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
@@ -6,4 +6,9 @@ 
 
 #include "iavf_types.h"
 
+void iavf_ptp_init(struct iavf_adapter *adapter);
+void iavf_ptp_release(struct iavf_adapter *adapter);
+void iavf_ptp_process_caps(struct iavf_adapter *adapter);
+bool iavf_ptp_cap_supported(const struct iavf_adapter *adapter, u32 cap);
+
 #endif /* _IAVF_PTP_H_ */
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 63ea30a20576..03e880d756de 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -4,6 +4,7 @@ 
 #include <linux/net/intel/libie/rx.h>
 
 #include "iavf.h"
+#include "iavf_ptp.h"
 #include "iavf_prototype.h"
 
 /**
@@ -387,6 +388,7 @@  void iavf_configure_queues(struct iavf_adapter *adapter)
 	int pairs = adapter->num_active_queues;
 	struct virtchnl_queue_pair_info *vqpi;
 	u32 i, max_frame;
+	u8 rx_flags = 0;
 	size_t len;
 
 	max_frame = LIBIE_MAX_RX_FRM_LEN(adapter->rx_rings->pp->p.offset);
@@ -404,6 +406,9 @@  void iavf_configure_queues(struct iavf_adapter *adapter)
 	if (!vqci)
 		return;
 
+	if (iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_RX_TSTAMP))
+		rx_flags |= VIRTCHNL_PTP_RX_TSTAMP;
+
 	vqci->vsi_id = adapter->vsi_res->vsi_id;
 	vqci->num_queue_pairs = pairs;
 	vqpi = vqci->qpair;
@@ -426,6 +431,7 @@  void iavf_configure_queues(struct iavf_adapter *adapter)
 		if (CRC_OFFLOAD_ALLOWED(adapter))
 			vqpi->rxq.crc_disable = !!(adapter->netdev->features &
 						   NETIF_F_RXFCS);
+		vqpi->rxq.flags = rx_flags;
 		vqpi++;
 	}
 
@@ -2494,6 +2500,8 @@  void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 	case VIRTCHNL_OP_1588_PTP_GET_CAPS:
 		memcpy(&adapter->ptp.hw_caps, msg,
 		       min_t(u16, msglen, sizeof(adapter->ptp.hw_caps)));
+		/* process any state change needed due to new capabilities */
+		iavf_ptp_process_caps(adapter);
 		break;
 	case VIRTCHNL_OP_ENABLE_QUEUES:
 		/* enable transmits */