netdev - RE: [PATCH v2 iwl-next 07/10] idpf: add Tx timestamp capabilities negotiation

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <PH7PR11MB588553078B0F4CCA7C3E8D868E052@PH7PR11MB5885.namprd11.prod.outlook.com>
Date: Wed, 18 Dec 2024 15:02:44 +0000
From: "Olech, Milena" <milena.olech@...el.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "Lobakin, Aleksander"
	<aleksander.lobakin@...el.com>, "Tantilov, Emil S"
	<emil.s.tantilov@...el.com>, "Linga, Pavan Kumar"
	<pavan.kumar.linga@...el.com>
Subject: RE: [PATCH v2 iwl-next 07/10] idpf: add Tx timestamp capabilities
 negotiation

On 11/29/2024 4:38 PM Willem de Bruijn wrote:

>> Tx timestamp capabilities are negotiated for the uplink Vport.
>> Driver receives information about the number of available Tx timestamp
>> latches, the size of Tx timestamp value and the set of indexes used
>> for Tx timestamping.
>> 
>> Add function to get the Tx timestamp capabilities and parse the uplink
>> vport flag.
>> 
>> Reviewed-by: Alexander Lobakin <aleksander.lobakin@...el.com>
>> Co-developed-by: Emil Tantilov <emil.s.tantilov@...el.com>
>> Signed-off-by: Emil Tantilov <emil.s.tantilov@...el.com>
>> Co-developed-by: Pavan Kumar Linga <pavan.kumar.linga@...el.com>
>> Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@...el.com>
>> Signed-off-by: Milena Olech <milena.olech@...el.com>
>> ---
>> v1 -> v2: change the idpf_for_each_vport macro
>> 
>>  drivers/net/ethernet/intel/idpf/idpf.h        |   6 +
>>  drivers/net/ethernet/intel/idpf/idpf_ptp.c    |  69 ++++++++++
>>  drivers/net/ethernet/intel/idpf/idpf_ptp.h    |  96 ++++++++++++-
>>  .../net/ethernet/intel/idpf/idpf_virtchnl.c   |  11 ++
>>  .../ethernet/intel/idpf/idpf_virtchnl_ptp.c   | 128 +++++++++++++++++-
>>  drivers/net/ethernet/intel/idpf/virtchnl2.h   |  10 +-
>>  6 files changed, 317 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
>> index 1607e9641b23..14b82e93dab5 100644
>> --- a/drivers/net/ethernet/intel/idpf/idpf.h
>> +++ b/drivers/net/ethernet/intel/idpf/idpf.h
>> @@ -292,6 +292,7 @@ struct idpf_port_stats {
>>   * @port_stats: per port csum, header split, and other offload stats
>>   * @link_up: True if link is up
>>   * @sw_marker_wq: workqueue for marker packets
>> + * @tx_tstamp_caps: The capabilities negotiated for Tx timestamping
>>   */
>>  struct idpf_vport {
>>  	u16 num_txq;
>> @@ -336,6 +337,8 @@ struct idpf_vport {
>>  	bool link_up;
>>  
>>  	wait_queue_head_t sw_marker_wq;
>> +
>> +	struct idpf_ptp_vport_tx_tstamp_caps *tx_tstamp_caps;
>>  };
>>  
>>  /**
>> @@ -480,6 +483,9 @@ struct idpf_vport_config {
>>  
>>  struct idpf_vc_xn_manager;
>>  
>> +#define idpf_for_each_vport(adapter, i) \
>> +	for ((i) = 0; (i) < (adapter)->num_alloc_vports; (i)++)
>> +
>

Sorry for delayed response - I was OOO.

>I did not mean to make the code less readable. My suggestion was
>
>    #define idpf_for_each_vport(adapter, vport) \
>        for (int i = 0; vport = &(adapter)->vports[i], i < (adapter)->num_alloc_vports; i++)
>
>I see that this now requires defining a variable i outside of the
>loop. I suppose because of a possible namespace collision otherwise?
>
>That can be addressed with the same stringification as the original
>code. But then may as well revert to that. The following is no
>more readable
>
>    #define idpf_for_each_vport(adapter, vport) \
>        for (int __idx_##vport = 0; \
>             vport = &((adapter)->vports[__idx_##vport]), __idx_##vport < (adapter)->num_alloc_vports; \
>             __idx_##vport++)
>
>Please choose whichever path you prefer, including the original.
>

Ok, so if you don't mind, I'd prefer to keep the original version.
Will fix in v3.

>Not for this series, but a wrapper might also be helpful for
>idpf_vport_for_each_rxq. To isolate the singleq vs splitq in one
>location. I see that come up a come up times in this series too.

Sure, sounds like a good idea for the future.

Thanks,
Milena