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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8453bc43-f1b2-44d0-8d85-d7d2c147d4a2@intel.com>
Date: Tue, 1 Oct 2024 15:49:03 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
CC: Wojciech Drewek <wojciech.drewek@...el.com>, <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: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Date: Tue, 1 Oct 2024 09:20:14 +0200

> 
> 
> On 8/21/2024 4:31 PM, 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().
>>
> 
> Sorry for late response but I was on quite long vacation.
> 
> Regarding your question - I think it is needed to have
> mod_delayed_work() here, at least as for now. I agree with your
> suggestion to use queue_work() but this function takes as parameter
> work_struct and in our case the adapter->watchdog_task field is of
> struct delayed_work type. It uses the function iavf_watchdog_task()
> which does plenty of things (including sending ptp cmd). Changing
> adapter->watchdog_task to be just struct work_struct is not applicable
> here as in iavf_main.c file in few places we add it to queue with
> different time.

Aaaah I'm sorry I didn't notice that watchdog_task is used in other
placed, not only here.
Ack, leave it as it is.

> 
> Make long story short - I agree with your argument but as far as this
> 0 delay is not causing performance regression then I will stick with
> this solution implemented by Jake.

Thanks,
Olek