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

Wojciech Drewek wojciech.drewek at intel.com
Wed Aug 28 11:15:09 UTC 2024



On 21.08.2024 16:31, Alexander Lobakin wrote:
> From: Wojciech Drewek <wojciech.drewek at intel.com>
> Date: Wed, 21 Aug 2024 14:15:32 +0200
> 
>> From: Jacob Keller <jacob.e.keller at 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().

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


More information about the Intel-wired-lan mailing list