From: Wojciech Drewek <wojciech.dre...@intel.com>
Date: Wed, 28 Aug 2024 13:15:09 +0200
>
>
> On 21.08.2024 16:31, Alexander Lobakin wrote:
>> 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().
>
> 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
