Message-ID: <390393c3-b4ee-473a-8055-813130876e71@intel.com>
Date: Tue, 30 Jul 2024 15:40:08 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, Jacob Keller
	<jacob.e.keller@...el.com>, Wojciech Drewek <wojciech.drewek@...el.com>, "Sai
 Krishna" <saikrishnag@...vell.com>, Simon Horman <horms@...nel.org>, "Ahmed
 Zaki" <ahmed.zaki@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v8 06/14] iavf: add initial
 framework for registering PTP clock

From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Date: Tue, 30 Jul 2024 05:15:01 -0400

> From: Jacob Keller <jacob.e.keller@...el.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@...el.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
> Reviewed-by: Sai Krishna <saikrishnag@...vell.com>
> Reviewed-by: Simon Horman <horms@...nel.org>
> Co-developed-by: Ahmed Zaki <ahmed.zaki@...el.com>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@...el.com>
> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> ---
>  drivers/net/ethernet/intel/iavf/Makefile      |   1 +
>  drivers/net/ethernet/intel/iavf/iavf_main.c   |   5 +
>  drivers/net/ethernet/intel/iavf/iavf_ptp.c    | 126 ++++++++++++++++++
>  drivers/net/ethernet/intel/iavf/iavf_ptp.h    |  10 ++
>  .../net/ethernet/intel/iavf/iavf_virtchnl.c   |   2 +
>  5 files changed, 144 insertions(+)
>  create mode 100644 drivers/net/ethernet/intel/iavf/iavf_ptp.c
> 
> diff --git a/drivers/net/ethernet/intel/iavf/Makefile b/drivers/net/ethernet/intel/iavf/Makefile
> index 356ac9faa5bf..cff88cb49540 100644
> --- a/drivers/net/ethernet/intel/iavf/Makefile
> +++ b/drivers/net/ethernet/intel/iavf/Makefile
> @@ -13,3 +13,4 @@ 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

Newline before this one?

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 11599d62d15a..3961c540c1c4 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -2847,6 +2847,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;
>  
> @@ -5474,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..1344298481d4
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2024 Intel Corporation. */
> +
> +#include "iavf.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(struct iavf_adapter *adapter, u32 cap)
> +{
> +	if (!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;

Aren't these parenthesis redundant?

> +}
> +
> +/**
> + * 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) - 1, "%s-%s-clk",

sizeof(ptp_info->name) without `-1`, snprintf() takes care of it.

> +		 dev_driver_string(dev),
> +		 dev_name(dev));

This can be placed in one line.

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

Shouldn't ptp.clock be nullified when this happens?

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

1. dev_dbg()?
2. pci_* for printing messages from PCI drivers.
3. Empty newline before return.

> +	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)
> +{
> +	struct device *dev = &adapter->pdev->dev;
> +	int err;
> +
> +	if (WARN_ON(adapter->ptp.initialized)) {
> +		dev_err(dev, "PTP functionality was already initialized!\n");

Is this needed?

> +		return;
> +	}
> +
> +	if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) {
> +		dev_dbg(dev, "Device does not have PTP clock support\n");

This is pci_warn(), not dbg I guess?

> +		return;
> +	}
> +
> +	err = iavf_ptp_register_clock(adapter);
> +	if (err) {
> +		dev_warn(dev, "Failed to register PTP clock device (%d)\n",
> +			 err);

		pci_warn(pdev, "failed to ... device, %pe\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_info(&adapter->pdev->dev, "removing PTP clock %s\n",
> +			 adapter->ptp.info.name);

_dbg().

> +		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)
> +{
> +	struct device *dev = &adapter->pdev->dev;
> +
> +	dev_dbg(dev, "PTP capabilities changed at runtime\n");

Is this needed at all?

> +
> +	/* 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 &&
> +	    !iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC))
> +		iavf_ptp_release(adapter);
> +	else if (!adapter->ptp.initialized &&
> +		 iavf_ptp_cap_supported(adapter,
> +					VIRTCHNL_1588_PTP_CAP_READ_PHC))

You can check for the cap support once before the whole block.

> +		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 aee4e2da0b9a..4939c219bd18 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
> @@ -4,9 +4,19 @@
>  #ifndef _IAVF_PTP_H_
>  #define _IAVF_PTP_H_
>  
> +#include <linux/ptp_clock_kernel.h>
> +
>  /* fields used for PTP support */
>  struct iavf_ptp {
>  	struct virtchnl_ptp_caps hw_caps;
> +	bool initialized;

Holes.

> +	struct ptp_clock_info info;
> +	struct ptp_clock *clock;
>  };
>  
> +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(struct iavf_adapter *adapter, u32 cap);
> +
>  #endif /* _IAVF_PTP_H_ */

Thanks,
Olek

Powered by blists - more mailing lists