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

[Wojciech Drewek <wojciech.drewek@intel.com>]

On 28.08.2024 14:05, Alexander Lobakin wrote:
> 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.

Okay, thx for explanation

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

I couldn't find many examples, since @initialized is set to false
mainly when PTP is not supported than I think EOPNOTSUPP would be a better pick.

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