Re: [Intel-wired-lan] [PATCH iwl-next v10 07/14] iavf: add support for indirect access to PHC time

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Wojciech Drewek <wojciech.drewek@intel.com>
Date: Wed, 21 Aug 2024 14:15:32 +0200

> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> Implement support for reading the PHC time indirectly via the
> VIRTCHNL_OP_1588_PTP_GET_TIME operation.

[...]

> +/**
> + * 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);

Are you sure you need delayed_work here? delayed_work is used only when
you need to run it after a delay. If the delay is always 0, then you
only need work_struct and queue_work().

> +}
> +
> +/**
> + * iavf_send_phc_read - Send request to read PHC time

[...]

> +static int iavf_ptp_gettimex64(struct ptp_clock_info *info,
> +			       struct timespec64 *ts,
> +			       struct ptp_system_timestamp *sts)
> +{
> +	struct iavf_adapter *adapter = iavf_clock_to_adapter(info);
> +
> +	if (!adapter->ptp.initialized)
> +		return -ENODEV;

Why is it -ENODEV here, but -EOPNOTSUPP several functions above, are you
sure these codes are the ones expected by the upper layers?

> +
> +	return iavf_read_phc_indirect(adapter, ts, sts);
> +}

[...]

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
> index c2ed24cef926..0bb4bddc1495 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
> @@ -6,9 +6,13 @@
>  
>  #include "iavf_types.h"
>  
> +#define iavf_clock_to_adapter(info)				\
> +	container_of_const(info, struct iavf_adapter, ptp.info)

It's only used in one file, are you sure you need it here in the header?
Or it will be used in later patches?

[...]

> +void iavf_virtchnl_send_ptp_cmd(struct iavf_adapter *adapter)
> +{
> +	struct device *dev = &adapter->pdev->dev;
> +	struct iavf_ptp_aq_cmd *cmd;
> +	int err;
> +
> +	if (!adapter->ptp.initialized) {

BTW does it make sense to introduce ptp.initialized since you can always
check ptp.clock for being %NULL and it will be the same?

> +		/* This shouldn't be possible to hit, since no messages should
> +		 * be queued if PTP is not initialized.
> +		 */
> +		pci_err(adapter->pdev, "PTP is not initialized\n");
> +		adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD;
> +		return;
> +	}
> +
> +	mutex_lock(&adapter->ptp.aq_cmd_lock);
> +	cmd = list_first_entry_or_null(&adapter->ptp.aq_cmds,
> +				       struct iavf_ptp_aq_cmd, list);
> +	if (!cmd) {
> +		/* no further PTP messages to send */
> +		adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD;
> +		goto out_unlock;
> +	}
> +
> +	if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) {
> +		/* bail because we already have a command pending */
> +		dev_err(dev, "Cannot send PTP command %d, command %d pending\n",

pci_err()

> +			cmd->v_opcode, adapter->current_op);
> +		goto out_unlock;
> +	}
> +
> +	err = iavf_send_pf_msg(adapter, cmd->v_opcode, cmd->msg, cmd->msglen);
> +	if (!err) {
> +		/* Command was sent without errors, so we can remove it from
> +		 * the list and discard it.
> +		 */
> +		list_del(&cmd->list);
> +		kfree(cmd);
> +	} else {
> +		/* We failed to send the command, try again next cycle */
> +		dev_warn(dev, "Failed to send PTP command %d\n", cmd->v_opcode);

pci_err() I'd say.

> +	}
> +
> +	if (list_empty(&adapter->ptp.aq_cmds))
> +		/* no further PTP messages to send */
> +		adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD;
> +
> +out_unlock:
> +	mutex_unlock(&adapter->ptp.aq_cmd_lock);
> +}
> +
>  /**
>   * iavf_print_link_message - print link up or down
>   * @adapter: adapter structure
> @@ -2093,6 +2151,39 @@ static void iavf_activate_fdir_filters(struct iavf_adapter *adapter)
>  		adapter->aq_required |= IAVF_FLAG_AQ_ADD_FDIR_FILTER;
>  }
>  
> +/**
> + * iavf_virtchnl_ptp_get_time - Respond to VIRTCHNL_OP_1588_PTP_GET_TIME
> + * @adapter: private adapter structure
> + * @data: the message from the PF
> + * @len: length of the message from the PF
> + *
> + * Handle the VIRTCHNL_OP_1588_PTP_GET_TIME message from the PF. This message
> + * is sent by the PF in response to the same op as a request from the VF.
> + * Extract the 64bit nanoseconds time from the message and store it in
> + * cached_phc_time. Then, notify any thread that is waiting for the update via
> + * the wait queue.
> + */
> +static void iavf_virtchnl_ptp_get_time(struct iavf_adapter *adapter,
> +				       void *data, u16 len)
> +{
> +	struct virtchnl_phc_time *msg;
> +
> +	if (len == sizeof(*msg)) {
> +		msg = (struct virtchnl_phc_time *)data;

Redundant cast.

> +	} else {
> +		dev_err_once(&adapter->pdev->dev,
> +			     "Invalid VIRTCHNL_OP_1588_PTP_GET_TIME from PF. Got size %u, expected %zu\n",
> +			     len, sizeof(*msg));
> +		return;
> +	}

	struct virtchnl_phc_time *msg = data;

	if (len != sizeof(*msg))
		// error path

	adapter->ptp.cached ...

IOW there's no point in this complex if-else.

> +
> +	adapter->ptp.cached_phc_time = msg->time;
> +	adapter->ptp.cached_phc_updated = jiffies;
> +	adapter->ptp.phc_time_ready = true;
> +
> +	wake_up(&adapter->ptp.phc_time_waitqueue);
> +}

Thanks,
Olek