From: Wojciech Drewek <wojciech.dre...@intel.com>
Date: Wed, 21 Aug 2024 14:15:32 +0200
> From: Jacob Keller <jacob.e.kel...@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
