Re: [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, 28 Aug 2024 13:15:09 +0200

> 
> 
> On 21.08.2024 16:31, Alexander Lobakin wrote:
>> 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().
> 
> I think that Jake's intention here was to execute the work that is already queued,
> not to queue new work

mod_delayed_work(0) works exactly as queue_work(), which is:

* if the work is already queued and the timeout is non-zero, modify the
timeout
* if the work is already queued and the timeout is zero, do nothing
* if the work is not queued, queue it

So my comment it still valid. You don't need delayed_work, but work_struct.

> 
>>
>>> +}
>>> +
>>> +/**
>>> + * 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

But why -ENODEV? Can you show me some other drivers and/or core PTP code
which use it?

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

Thanks,
Olek