[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1a4232c2-34c9-4b14-a581-4bc2cdc11afd@intel.com>
Date: Wed, 28 Aug 2024 14:05:03 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
<horms@...nel.org>, <anthony.l.nguyen@...el.com>, <kuba@...nel.org>,
<alexandr.lobakin@...el.com>
Subject: Re: [PATCH iwl-next v10 07/14] iavf: add support for indirect access
to PHC time
From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Wed, 28 Aug 2024 13:15:09 +0200
>
>
> On 21.08.2024 16:31, Alexander Lobakin wrote:
>> From: Wojciech Drewek <wojciech.drewek@...el.com>
>> Date: Wed, 21 Aug 2024 14:15:32 +0200
>>
>>> From: Jacob Keller <jacob.e.keller@...el.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
Powered by blists - more mailing lists