On 21.08.2024 16:31, Alexander Lobakin wrote:
> From: Wojciech Drewek <[email protected]>
> Date: Wed, 21 Aug 2024 14:15:32 +0200
>
>> From: Jacob Keller <[email protected]>
>>
>> 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().
I think that Jake's intention here was to execute the work that is already
queued,
not to queue new 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?
I'll use ENODEV in both cases
>
>> +
>> + 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?
I can define it in iavf_ptp.c if you want
>
> [...]
>
>> +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?
I'll think about it
>
>> + /* 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()
sure
>
>> + 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.
sure
>
>> + }
>> +
>> + 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.
yup
>
>> + } 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.
Agree
>
>> +
>> + 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
