Message-ID: <58b00c7f-b74b-4f14-a8c4-080d3fcedcb1@linux.dev>
Date: Tue, 15 Oct 2024 14:56:28 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Mateusz Polchlopek <mateusz.polchlopek@...el.com>,
 intel-wired-lan@...ts.osuosl.org, aleksander.lobakin@...el.com
Cc: netdev@...r.kernel.org, Jacob Keller <jacob.e.keller@...el.com>,
 Wojciech Drewek <wojciech.drewek@...el.com>,
 Rahul Rameshbabu <rrameshbabu@...dia.com>, Simon Horman <horms@...nel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v11 07/14] iavf: add support
 for indirect access to PHC time

On 13/10/2024 16:44, Mateusz Polchlopek wrote:
> From: Jacob Keller <jacob.e.keller@...el.com>
> 
> Implement support for reading the PHC time indirectly via the
> VIRTCHNL_OP_1588_PTP_GET_TIME operation.
> 
> Based on some simple tests with ftrace, the latency of the indirect
> clock access appears to be about ~110 microseconds. This is due to the
> cost of preparing a message to send over the virtchnl queue.
> 
> This is expected, due to the increased jitter caused by sending messages
> over virtchnl. It is not easy to control the precise time that the
> message is sent by the VF, or the time that the message is responded to
> by the PF, or the time that the message sent from the PF is received by
> the VF.
> 
> For sending the request, note that many PTP related operations will
> require sending of VIRTCHNL messages. Instead of adding a separate AQ
> flag and storage for each operation, setup a simple queue mechanism for
> queuing up virtchnl messages.
> 
> Each message will be converted to a iavf_ptp_aq_cmd structure which ends
> with a flexible array member. A single AQ flag is added for processing
> messages from this queue. In principle this could be extended to handle
> arbitrary virtchnl messages. For now it is kept to PTP-specific as the
> need is primarily for handling PTP-related commands.
> 
> Use this to implement .gettimex64 using the indirect method via the
> virtchnl command. The response from the PF is processed and stored into
> the cached_phc_time. A wait queue is used to allow the PTP clock gettime
> request to sleep until the message is sent from the PF.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
> Reviewed-by: Rahul Rameshbabu <rrameshbabu@...dia.com>
> Reviewed-by: Simon Horman <horms@...nel.org>
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> ---
>   drivers/net/ethernet/intel/iavf/iavf_main.c   |   9 +-
>   drivers/net/ethernet/intel/iavf/iavf_ptp.c    | 147 ++++++++++++++++++
>   drivers/net/ethernet/intel/iavf/iavf_ptp.h    |   1 +
>   .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  93 +++++++++++
>   4 files changed, 249 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index be07e9f8e664..b897dd94a32e 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -2269,7 +2269,10 @@ static int iavf_process_aq_command(struct iavf_adapter *adapter)
>   		iavf_enable_vlan_insertion_v2(adapter, ETH_P_8021AD);
>   		return 0;
>   	}
> -
> +	if (adapter->aq_required & IAVF_FLAG_AQ_SEND_PTP_CMD) {
> +		iavf_virtchnl_send_ptp_cmd(adapter);
> +		return IAVF_SUCCESS;
> +	}
>   	if (adapter->aq_required & IAVF_FLAG_AQ_REQUEST_STATS) {
>   		iavf_request_stats(adapter);
>   		return 0;
> @@ -5496,6 +5499,10 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	/* Setup the wait queue for indicating virtchannel events */
>   	init_waitqueue_head(&adapter->vc_waitqueue);
>   
> +	INIT_LIST_HEAD(&adapter->ptp.aq_cmds);
> +	init_waitqueue_head(&adapter->ptp.phc_time_waitqueue);
> +	mutex_init(&adapter->ptp.aq_cmd_lock);
> +
>   	queue_delayed_work(adapter->wq, &adapter->watchdog_task,
>   			   msecs_to_jiffies(5 * (pdev->devfn & 0x07)));
>   	/* Initialization goes on in the work. Do not add more of it below. */
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.c b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
> index 5a1b5f8b87e5..f4f10692020a 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
> @@ -4,6 +4,9 @@
>   #include "iavf.h"
>   #include "iavf_ptp.h"
>   
> +#define iavf_clock_to_adapter(info)				\
> +	container_of_const(info, struct iavf_adapter, ptp.info)
> +
>   /**
>    * iavf_ptp_cap_supported - Check if a PTP capability is supported
>    * @adapter: private adapter structure
> @@ -21,6 +24,138 @@ bool iavf_ptp_cap_supported(const struct iavf_adapter *adapter, u32 cap)
>   	return (adapter->ptp.hw_caps.caps & cap) == cap;
>   }
>   
> +/**
> + * iavf_allocate_ptp_cmd - Allocate a PTP command message structure
> + * @v_opcode: the virtchnl opcode
> + * @msglen: length in bytes of the associated virtchnl structure
> + *
> + * Allocates a PTP command message and pre-fills it with the provided message
> + * length and opcode.
> + *
> + * Return: allocated PTP command.
> + */
> +static struct iavf_ptp_aq_cmd *iavf_allocate_ptp_cmd(enum virtchnl_ops v_opcode,
> +						     u16 msglen)
> +{
> +	struct iavf_ptp_aq_cmd *cmd;
> +
> +	cmd = kzalloc(struct_size(cmd, msg, msglen), GFP_KERNEL);
> +	if (!cmd)
> +		return NULL;
> +
> +	cmd->v_opcode = v_opcode;
> +	cmd->msglen = msglen;
> +
> +	return cmd;
> +}
> +
> +/**
> + * iavf_queue_ptp_cmd - Queue PTP command for sending over virtchnl
> + * @adapter: private adapter structure
> + * @cmd: the command structure to send
> + *
> + * Queue the given command structure into the PTP virtchnl command queue tos
> + * end to the PF.
> + */
> +static void iavf_queue_ptp_cmd(struct iavf_adapter *adapter,
> +			       struct iavf_ptp_aq_cmd *cmd)
> +{
> +	mutex_lock(&adapter->ptp.aq_cmd_lock);
> +	list_add_tail(&cmd->list, &adapter->ptp.aq_cmds);
> +	mutex_unlock(&adapter->ptp.aq_cmd_lock);
> +
> +	adapter->aq_required |= IAVF_FLAG_AQ_SEND_PTP_CMD;
> +	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
> +}
> +
> +/**
> + * iavf_send_phc_read - Send request to read PHC time
> + * @adapter: private adapter structure
> + *
> + * Send a request to obtain the PTP hardware clock time. This allocates the
> + * VIRTCHNL_OP_1588_PTP_GET_TIME message and queues it up to send to
> + * indirectly read the PHC time.
> + *
> + * This function does not wait for the reply from the PF.
> + *
> + * Return: 0 if success, error code otherwise.
> + */
> +static int iavf_send_phc_read(struct iavf_adapter *adapter)
> +{
> +	struct iavf_ptp_aq_cmd *cmd;
> +
> +	if (!adapter->ptp.clock)
> +		return -EOPNOTSUPP;
> +
> +	cmd = iavf_allocate_ptp_cmd(VIRTCHNL_OP_1588_PTP_GET_TIME,
> +				    sizeof(struct virtchnl_phc_time));
> +	if (!cmd)
> +		return -ENOMEM;
> +
> +	iavf_queue_ptp_cmd(adapter, cmd);
> +
> +	return 0;
> +}
> +
> +/**
> + * iavf_read_phc_indirect - Indirectly read the PHC time via virtchnl
> + * @adapter: private adapter structure
> + * @ts: storage for the timestamp value
> + * @sts: system timestamp values before and after the read
> + *
> + * Used when the device does not have direct register access to the PHC time.
> + * Indirectly reads the time via the VIRTCHNL_OP_1588_PTP_GET_TIME, and waits
> + * for the reply from the PF.
> + *
> + * Based on some simple measurements using ftrace and phc2sys, this clock
> + * access method has about a ~110 usec latency even when the system is not
> + * under load. In order to achieve acceptable results when using phc2sys with
> + * the indirect clock access method, it is recommended to use more
> + * conservative proportional and integration constants with the P/I servo.
> + *
> + * Return: 0 if success, error code otherwise.
> + */
> +static int iavf_read_phc_indirect(struct iavf_adapter *adapter,
> +				  struct timespec64 *ts,
> +				  struct ptp_system_timestamp *sts)
> +{
> +	long ret;
> +	int err;
> +
> +	adapter->ptp.phc_time_ready = false;
> +	ptp_read_system_prets(sts);
> +
> +	err = iavf_send_phc_read(adapter);
> +	if (err)
> +		return err;
> +
> +	ret = wait_event_interruptible_timeout(adapter->ptp.phc_time_waitqueue,
> +					       adapter->ptp.phc_time_ready,
> +					       HZ);
> +	if (ret < 0)
> +		return ret;
> +	else if (!ret)
> +		return -EBUSY;
> +
> +	*ts = ns_to_timespec64(adapter->ptp.cached_phc_time);
> +
> +	ptp_read_system_postts(sts);

Usually prets()/postts() pair covers actual transaction time. That means
the last error check and ns_to_timespec64() are usually no covered.

Not sure though how precise it can be because of several queues used in 
the process..

> +
> +	return 0;
> +}
> +

Powered by blists - more mailing lists