Re: [Intel-wired-lan] [PATCH iwl-next v10 05/14] iavf: negotiate PTP capabilities

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

On 21.08.2024 16:06, Alexander Lobakin wrote:
> From: Wojciech Drewek <wojciech.drewek@intel.com>
> Date: Wed, 21 Aug 2024 14:15:30 +0200
> 
>> From: Jacob Keller <jacob.e.keller@intel.com>
>>
>> Add a new extended capabilities negotiation to exchange information from
>> the PF about what PTP capabilities are supported by this VF. This
>> requires sending a VIRTCHNL_OP_1588_PTP_GET_CAPS message, and waiting
>> for the response from the PF. Handle this early on during the VF
>> initialization.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> Reviewed-by: Simon Horman <horms@kernel.org>
>> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> ---
>>  drivers/net/ethernet/intel/iavf/iavf.h        | 17 ++++-
>>  drivers/net/ethernet/intel/iavf/iavf_main.c   | 60 ++++++++++++++++
>>  drivers/net/ethernet/intel/iavf/iavf_ptp.h    |  9 +++
>>  drivers/net/ethernet/intel/iavf/iavf_types.h  | 36 ++++++++++
>>  .../net/ethernet/intel/iavf/iavf_virtchnl.c   | 72 +++++++++++++++++++
>>  5 files changed, 192 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/net/ethernet/intel/iavf/iavf_ptp.h
>>  create mode 100644 drivers/net/ethernet/intel/iavf/iavf_types.h
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
>> index f1506b3d01ce..871431bed64a 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf.h
>> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
>> @@ -40,6 +40,7 @@
>>  #include "iavf_txrx.h"
>>  #include "iavf_fdir.h"
>>  #include "iavf_adv_rss.h"
>> +#include "iavf_types.h"
>>  #include <linux/bitmap.h>
>>  
>>  #define DEFAULT_DEBUG_LEVEL_SHIFT 3
>> @@ -338,13 +339,16 @@ struct iavf_adapter {
>>  #define IAVF_FLAG_AQ_ENABLE_STAG_VLAN_INSERTION		BIT_ULL(37)
>>  #define IAVF_FLAG_AQ_DISABLE_STAG_VLAN_INSERTION	BIT_ULL(38)
>>  #define IAVF_FLAG_AQ_GET_SUPPORTED_RXDIDS		BIT_ULL(39)
>> +#define IAVF_FLAG_AQ_GET_PTP_CAPS			BIT_ULL(40)
>> +#define IAVF_FLAG_AQ_SEND_PTP_CMD			BIT_ULL(41)
>>  
>>  	/* AQ messages that must be sent after IAVF_FLAG_AQ_GET_CONFIG, in
>>  	 * order to negotiated extended capabilities.
>>  	 */
>>  #define IAVF_FLAG_AQ_EXTENDED_CAPS			\
>>  	(IAVF_FLAG_AQ_GET_OFFLOAD_VLAN_V2_CAPS |	\
>> -	 IAVF_FLAG_AQ_GET_SUPPORTED_RXDIDS)
>> +	 IAVF_FLAG_AQ_GET_SUPPORTED_RXDIDS |		\
>> +	 IAVF_FLAG_AQ_GET_PTP_CAPS)
>>  
>>  	/* flags for processing extended capability messages during
>>  	 * __IAVF_INIT_EXTENDED_CAPS. Each capability exchange requires
>> @@ -358,12 +362,16 @@ struct iavf_adapter {
>>  #define IAVF_EXTENDED_CAP_RECV_VLAN_V2			BIT_ULL(1)
>>  #define IAVF_EXTENDED_CAP_SEND_RXDID			BIT_ULL(2)
>>  #define IAVF_EXTENDED_CAP_RECV_RXDID			BIT_ULL(3)
>> +#define IAVF_EXTENDED_CAP_SEND_PTP			BIT_ULL(4)
>> +#define IAVF_EXTENDED_CAP_RECV_PTP			BIT_ULL(5)
>>  
>>  #define IAVF_EXTENDED_CAPS				\
>>  	(IAVF_EXTENDED_CAP_SEND_VLAN_V2 |		\
>>  	 IAVF_EXTENDED_CAP_RECV_VLAN_V2 |		\
>>  	 IAVF_EXTENDED_CAP_SEND_RXDID |			\
>> -	 IAVF_EXTENDED_CAP_RECV_RXDID)
>> +	 IAVF_EXTENDED_CAP_RECV_RXDID |			\
>> +	 IAVF_EXTENDED_CAP_SEND_PTP |			\
>> +	 IAVF_EXTENDED_CAP_RECV_PTP)
>>  
>>  	/* Lock to prevent possible clobbering of
>>  	 * current_netdev_promisc_flags
>> @@ -423,6 +431,8 @@ struct iavf_adapter {
>>  			     VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF)
>>  #define IAVF_RXDID_ALLOWED(a) ((a)->vf_res->vf_cap_flags & \
>>  			       VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC)
>> +#define IAVF_PTP_ALLOWED(a) ((a)->vf_res->vf_cap_flags & \
>> +			      VIRTCHNL_VF_CAP_PTP)
> 
> Bah, should've mentioned that where you introduce IAVF_RXDID_ALLOWED().
> I realize that the macros added previously are indented with spaces, but
> it's not sorta correct style for the kernel. Maybe you'd indent both new
> macros (RXDID and PTP) with tabs? You can also break the line different
> way if you want, like
> 
> #define IAVF_PTP_ALLOWED(a)					\
> 	((a)->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP)
> 
> Looks more clear than breaking it after the '&'.

sure

> 
>>  	struct virtchnl_vf_resource *vf_res; /* incl. all VSIs */
>>  	struct virtchnl_vsi_resource *vsi_res; /* our LAN VSI */
>>  	struct virtchnl_version_info pf_version;
>> @@ -430,6 +440,7 @@ struct iavf_adapter {
>>  		       ((_a)->pf_version.minor == 1))
>>  	struct virtchnl_vlan_caps vlan_v2_caps;
>>  	u64 supp_rxdids;
>> +	struct iavf_ptp ptp;
>>  	u16 msg_enable;
>>  	struct iavf_eth_stats current_stats;
>>  	struct iavf_vsi vsi;
> 
> [...]
> 
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
>> new file mode 100644
>> index 000000000000..65678e76c34f
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
>> @@ -0,0 +1,9 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright(c) 2024 Intel Corporation. */
>> +
>> +#ifndef _IAVF_PTP_H_
>> +#define _IAVF_PTP_H_
>> +
>> +#include "iavf_types.h"
>> +
>> +#endif /* _IAVF_PTP_H_ */
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_types.h b/drivers/net/ethernet/intel/iavf/iavf_types.h
>> new file mode 100644
>> index 000000000000..6b7029a1a5a7
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_types.h
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright(c) 2024 Intel Corporation. */
>> +
>> +#ifndef _IAVF_TYPES_H_
>> +#define _IAVF_TYPES_H_
>> +
>> +#include "iavf_types.h"
>> +
>> +#include <linux/avf/virtchnl.h>
>> +#include <linux/ptp_clock_kernel.h>
> 
> Oh well. I initially asked to introduce iavf_types.h to not bloat
> iavf.h, but now types.h includes big ptp_clock_kernel.h :z
> When I was reviewing PTP for idpf, I proposed to make this iavf_ptp in
> iavf_adapter a pointer and allocate it dynamically, so that iavf.h
> wouldn't need to include anything PTP-related at all. This way you
> wouldn't need iavf_types.h.
> What do you think?

I can try make it happen

> 
>> +
>> +/* structure used to queue PTP commands for processing */
>> +struct iavf_ptp_aq_cmd {
>> +	struct list_head list;
>> +	enum virtchnl_ops v_opcode:16;
>> +	u16 msglen;
>> +	u8 msg[] __counted_by(msglen);
>> +};
>> +
>> +/* fields used for PTP support */
> 
> Redundant comment I'd say.

Agree

> 
>> +struct iavf_ptp {
>> +	wait_queue_head_t phc_time_waitqueue;
>> +	struct virtchnl_ptp_caps hw_caps;
>> +	struct ptp_clock_info info;
>> +	struct ptp_clock *clock;
>> +	struct list_head aq_cmds;
>> +	u64 cached_phc_time;
>> +	unsigned long cached_phc_updated;
>> +	/* Lock protecting access to the AQ command list */
>> +	struct mutex aq_cmd_lock;
>> +	struct kernel_hwtstamp_config hwtstamp_config;
>> +	bool initialized:1;
>> +	bool phc_time_ready:1;
>> +};
>> +
>> +#endif /* _IAVF_TYPES_H_ */
> 
> [...]
> 
>> @@ -307,6 +343,38 @@ int iavf_get_vf_supported_rxdids(struct iavf_adapter *adapter)
>>  	return 0;
>>  }
>>  
>> +int iavf_get_vf_ptp_caps(struct iavf_adapter *adapter)
>> +{
>> +	struct virtchnl_ptp_caps caps = {};
>> +	struct iavf_hw *hw = &adapter->hw;
>> +	struct iavf_arq_event_info event;
>> +	enum virtchnl_ops op;
>> +	enum iavf_status err;
>> +
>> +	event.msg_buf = (u8 *)&caps;
>> +	event.buf_len = sizeof(caps);
>> +
>> +	while (1) {
>> +		/* When the AQ is empty, iavf_clean_arq_element will return
>> +		 * nonzero and this loop will terminate.
>> +		 */
>> +		err = iavf_clean_arq_element(hw, &event, NULL);
>> +		if (err != IAVF_SUCCESS)
>> +			return err;
>> +		op = (enum virtchnl_ops)le32_to_cpu(event.desc.cookie_high);
> 
> This cast is not needed.
> 
>> +		if (op == VIRTCHNL_OP_1588_PTP_GET_CAPS)
>> +			break;
> 
> Same comments as to one of the previous patches -- you can declare @op
> inside the loop and also take into consideration that cpu_to_le32(const)
> is faster than le32_to_cpu(var) on BE.

As in previous patch I'll use iavf_poll_virtchnl_msg. After that
your comments won't apply

> 
>> +	}
>> +
>> +	err = le32_to_cpu(event.desc.cookie_low);
>> +	if (err)
>> +		return err;
>> +
>> +	memcpy(&adapter->ptp.hw_caps, &caps, sizeof(caps));
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * iavf_configure_queues
>>   * @adapter: adapter structure
>> @@ -2423,6 +2491,10 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
>>  		memcpy(&adapter->supp_rxdids, msg,
>>  		       min_t(u16, msglen, sizeof(adapter->supp_rxdids)));
>>  		break;
>> +	case VIRTCHNL_OP_1588_PTP_GET_CAPS:
>> +		memcpy(&adapter->ptp.hw_caps, msg,
>> +		       min_t(u16, msglen, sizeof(adapter->ptp.hw_caps)));
> 
> Same as to one of the previous patches -- I'd avoid partial copying and
> check the msglen first to be the same as this sizeof().

Sure

> 
>> +		break;
>>  	case VIRTCHNL_OP_ENABLE_QUEUES:
>>  		/* enable transmits */
>>  		iavf_irq_enable(adapter, true);
> 
> Thanks,
> Olek