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

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

quoted

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@intel.com>
Co-developed-by: Emil Tantilov <redacted>
Signed-off-by: Emil Tantilov <redacted>
Co-developed-by: Pavan Kumar Linga <redacted>
Signed-off-by: Pavan Kumar Linga <redacted>
Signed-off-by: Milena Olech <redacted>
---
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