diff mbox series

[Intel-wired-lan,iwl-next,v8,02/14] ice: support Rx timestamp on flex descriptor

Message ID 20240730091509.18846-3-mateusz.polchlopek@intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series Add support for Rx timestamping for both ice and iavf drivers. | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Mateusz Polchlopek July 30, 2024, 9:14 a.m. UTC 
From: Simei Su <simei.su@intel.com>

To support Rx timestamp offload, VIRTCHNL_OP_1588_PTP_CAPS is sent by
the VF to request PTP capability and responded by the PF what capability
is enabled for that VF.

Hardware captures timestamps which contain only 32 bits of nominal
nanoseconds, as opposed to the 64bit timestamps that the stack expects.
To convert 32b to 64b, we need a current PHC time.
VIRTCHNL_OP_1588_PTP_GET_TIME is sent by the VF and responded by the
PF with the current PHC time.

Signed-off-by: Simei Su <simei.su@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>
---
 drivers/net/ethernet/intel/ice/ice_base.c     |  3 -
 drivers/net/ethernet/intel/ice/ice_ptp.c      |  4 +-
 drivers/net/ethernet/intel/ice/ice_ptp.h      |  8 ++
 drivers/net/ethernet/intel/ice/ice_vf_lib.h   |  2 +
 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 86 ++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_virtchnl.h |  2 +
 .../intel/ice/ice_virtchnl_allowlist.c        |  6 ++
 include/linux/avf/virtchnl.h                  | 15 +++-
 8 files changed, 117 insertions(+), 9 deletions(-)

Comments

Alexander Lobakin July 30, 2024, 12:54 p.m. UTC | #1 
From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
Date: Tue, 30 Jul 2024 05:14:57 -0400

> From: Simei Su <simei.su@intel.com>
> 
> To support Rx timestamp offload, VIRTCHNL_OP_1588_PTP_CAPS is sent by
> the VF to request PTP capability and responded by the PF what capability
> is enabled for that VF.

[...]

> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> index be4266899690..fdc63fae1803 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> @@ -136,6 +136,8 @@ struct ice_vf {
>  	const struct ice_virtchnl_ops *virtchnl_ops;
>  	const struct ice_vf_ops *vf_ops;
>  
> +	struct virtchnl_ptp_caps ptp_caps;

This struct is of 48 bytes length, but only first 4 of them are used
(the `caps` field). Couldn't we just add `u32 ptp_caps` here?

> +
>  	/* devlink port data */
>  	struct devlink_port devlink_port;
>  
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> index 82ad4c6ff4d7..4f5e36c063e2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> @@ -495,6 +495,9 @@ static int ice_vc_get_vf_res_msg(struct ice_vf *vf, u8 *msg)
>  	if (vf->driver_caps & VIRTCHNL_VF_OFFLOAD_USO)
>  		vfres->vf_cap_flags |= VIRTCHNL_VF_OFFLOAD_USO;
>  
> +	if (vf->driver_caps & VIRTCHNL_VF_CAP_PTP)
> +		vfres->vf_cap_flags |= VIRTCHNL_VF_CAP_PTP;
> +
>  	vfres->num_vsis = 1;
>  	/* Tx and Rx queue are equal for VF */
>  	vfres->num_queue_pairs = vsi->num_txq;
> @@ -1783,9 +1786,17 @@ static int ice_vc_cfg_qs_msg(struct ice_vf *vf, u8 *msg)
>  				rxdid = ICE_RXDID_LEGACY_1;
>  			}
>  
> -			ice_write_qrxflxp_cntxt(&vsi->back->hw,
> -						vsi->rxq_map[q_idx],
> -						rxdid, 0x03, false);
> +			if (vf->driver_caps &
> +			    VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC &&
> +			    vf->driver_caps & VIRTCHNL_VF_CAP_PTP &&
> +			    qpi->rxq.flags & VIRTCHNL_PTP_RX_TSTAMP)

A separate set of parenthesis () around each bitop (& | etc).
E.g.

	if ((vf->driver_caps & FLEX_DESC) &&
	    (vf->driver_caps & ...))

> +				ice_write_qrxflxp_cntxt(&vsi->back->hw,
> +							vsi->rxq_map[q_idx],
> +							rxdid, 0x03, true);
> +			else
> +				ice_write_qrxflxp_cntxt(&vsi->back->hw,
> +							vsi->rxq_map[q_idx],
> +							rxdid, 0x03, false);

Looks a bit suboptimal to double the same call with the only difference
in one argument.

			bool ptp = (vf->driver_caps & FLEX_DESC) ...

			ice_write_qrxflxp_cntxt(hw, map, rxdid, 0x03,
						ptp);

Also, this 0x03... I'd #define it somewhere 'cause for example right now
I have no idea what this is about.


>  		}
>  	}
>  
> @@ -3788,6 +3799,65 @@ static int ice_vc_dis_vlan_insertion_v2_msg(struct ice_vf *vf, u8 *msg)
>  				     v_ret, NULL, 0);
>  }
>  
> +static int ice_vc_get_ptp_cap(struct ice_vf *vf, u8 *msg)

@msg can be const.
Also, I'd make it `const void *` or maybe even `const struct
virtchnl_ptp_caps *` right away.

> +{
> +	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
> +	u32 msg_caps;
> +	int ret;
> +
> +	/* VF is not in active state */
> +	if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
> +		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
> +		goto err;
> +	}

Alternatively, you can do it that way:

	enum ... v_ret = ERR_PARAM;

	if (!test_bit(ACTIVE, vf_states))
		goto err;

	v_ret = SUCCESS;

> +
> +	msg_caps = ((struct virtchnl_ptp_caps *)msg)->caps;
> +
> +	/* Any VF asking for RX timestamping and reading PHC will get that */
> +	if (msg_caps & (VIRTCHNL_1588_PTP_CAP_RX_TSTAMP |
> +	    VIRTCHNL_1588_PTP_CAP_READ_PHC))

Bad identation, READ_PHC must match the parenthesis it's enclosed to,
i.e. the second pair, not the first/outer one.

> +		vf->ptp_caps.caps = VIRTCHNL_1588_PTP_CAP_RX_TSTAMP |
> +				    VIRTCHNL_1588_PTP_CAP_READ_PHC;

Also, hmm, can't that be

	u32 caps = VIRTCHNL_1588_PTP_CAP_RX_TSTAMP |
		   VIRTCHNL_1588_PTP_CAP_READ_PHC;

	if (msg_caps & caps)
		vf->ptp_caps = caps;

?

> +
> +err:
> +	/* send the response back to the VF */
> +	ret = ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_1588_PTP_GET_CAPS, v_ret,
> +				    (u8 *)&vf->ptp_caps,
> +				    sizeof(struct virtchnl_ptp_caps));
> +	return ret;

1. return ice_vc_send ... directly.
2. Try to declare abstract message buffers as `void *`, not `u8 *`. u8
   is rather for cases when you need to actually read some bytes from
   the buffer.

> +}
> +
> +static int ice_vc_get_phc_time(struct ice_vf *vf)
> +{
> +	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
> +	struct virtchnl_phc_time *phc_time = NULL;
> +	struct ice_pf *pf = vf->pf;
> +	int len = 0;
> +	int ret;
> +
> +	if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
> +		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
> +		goto err;
> +	}

Same here regarding @v_ret.

> +
> +	len = sizeof(struct virtchnl_phc_time);
> +	phc_time = kzalloc(len, GFP_KERNEL);

1. __free(kfree) for @phc_time, so that the function will auto-free it
   and you won't need to call kfree() later.
2. Since @len is trivial, just open-code it here:

	phc_time = kzalloc(sizeof(*phc_time), GFP_KERNEL);

then later

> +	if (!phc_time) {
> +		v_ret = VIRTCHNL_STATUS_ERR_NO_MEMORY;
> +		len = 0;
> +		goto err;
> +	}

	u32 len = 0;

	if (!phc_time) {
		v_ret = NO_MEMORY;
		goto err;
	}

	len = sizeof(*phc);

> +
> +	phc_time->time = ice_ptp_read_src_clk_reg(pf, NULL);
> +
> +err:
> +	/* send the response back to the VF */
> +	ret = ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_1588_PTP_GET_TIME,
> +				    v_ret, (u8 *)phc_time, len);

(same regarding u8 vs void)

> +	kfree(phc_time);
> +	return ret;

Since kfree() won't be needed, just plain return ice_vc_...

[...]

> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c
> index d796dbd2a440..7a442a53f4cc 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c
> @@ -84,6 +84,11 @@ static const u32 fdir_pf_allowlist_opcodes[] = {
>  	VIRTCHNL_OP_ADD_FDIR_FILTER, VIRTCHNL_OP_DEL_FDIR_FILTER,
>  };
>  
> +/* VIRTCHNL_VF_CAP_PTP */
> +static const u32 ptp_allowlist_opcodes[] = {
> +	VIRTCHNL_OP_1588_PTP_GET_CAPS, VIRTCHNL_OP_1588_PTP_GET_TIME,
> +};

I'd make it 1 def per line, not two in the same line.

[...]

> diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
> index 5edfb01fa064..c4e7ac19be7c 100644
> --- a/include/linux/avf/virtchnl.h
> +++ b/include/linux/avf/virtchnl.h
> @@ -304,6 +304,18 @@ struct virtchnl_txq_info {
>  
>  VIRTCHNL_CHECK_STRUCT_LEN(24, virtchnl_txq_info);
>  
> +/* virtchnl_rxq_info_flags
> + *
> + * Definition of bits in the flags field of the virtchnl_rxq_info structure.
> + */
> +enum virtchnl_rxq_info_flags {
> +	/* If the VIRTCHNL_PTP_RX_TSTAMP bit of the flag field is set, this is
> +	 * a request to enable Rx timestamp. Other flag bits are currently
> +	 * reserved and they may be extended in the future.
> +	 */

Make a proper kdoc from the first sentence and leave the second to the
end of the kdoc.

/* virtchnl_rxq_info_flags - definition of bits ...
 *
 * @VIRTCHNL_PTP_RX_TSTAMP: request to enable Rx timestamping
 *
 * Other flag bits are currently reserved and ...
 */
enum ...

> +	VIRTCHNL_PTP_RX_TSTAMP = BIT(0),
> +};
> +
>  /* VIRTCHNL_OP_CONFIG_RX_QUEUE
>   * VF sends this message to set up parameters for one RX queue.
>   * External data buffer contains one instance of virtchnl_rxq_info.
> @@ -327,7 +339,8 @@ struct virtchnl_rxq_info {
>  	u32 max_pkt_size;
>  	u8 crc_disable;
>  	u8 rxdid;
> -	u8 pad1[2];
> +	u8 flags; /* see virtchnl_rxq_info_flags */

Or
	enum virtchnl_rxq_info_flags flags:8;

-- will do the same thing, but with strict type.

> +	u8 pad1;
>  	u64 dma_ring_addr;
>  
>  	/* see enum virtchnl_rx_hsplit; deprecated with AVF 1.0 */

Thanks,
Olek
Mateusz Polchlopek Aug. 8, 2024, 11:04 a.m. UTC | #2 
On 7/30/2024 2:54 PM, Alexander Lobakin wrote:
> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
> Date: Tue, 30 Jul 2024 05:14:57 -0400
> 
>> From: Simei Su <simei.su@intel.com>
>>
>> To support Rx timestamp offload, VIRTCHNL_OP_1588_PTP_CAPS is sent by
>> the VF to request PTP capability and responded by the PF what capability
>> is enabled for that VF.
> 
> [...]
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
>> index be4266899690..fdc63fae1803 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
>> @@ -136,6 +136,8 @@ struct ice_vf {
>>   	const struct ice_virtchnl_ops *virtchnl_ops;
>>   	const struct ice_vf_ops *vf_ops;
>>   
>> +	struct virtchnl_ptp_caps ptp_caps;
> 
> This struct is of 48 bytes length, but only first 4 of them are used
> (the `caps` field). Couldn't we just add `u32 ptp_caps` here?
> 
>> +
>>   	/* devlink port data */
>>   	struct devlink_port devlink_port;
>>   
>> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
>> index 82ad4c6ff4d7..4f5e36c063e2 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
>> @@ -495,6 +495,9 @@ static int ice_vc_get_vf_res_msg(struct ice_vf *vf, u8 *msg)
>>   	if (vf->driver_caps & VIRTCHNL_VF_OFFLOAD_USO)
>>   		vfres->vf_cap_flags |= VIRTCHNL_VF_OFFLOAD_USO;
>>   
>> +	if (vf->driver_caps & VIRTCHNL_VF_CAP_PTP)
>> +		vfres->vf_cap_flags |= VIRTCHNL_VF_CAP_PTP;
>> +
>>   	vfres->num_vsis = 1;
>>   	/* Tx and Rx queue are equal for VF */
>>   	vfres->num_queue_pairs = vsi->num_txq;
>> @@ -1783,9 +1786,17 @@ static int ice_vc_cfg_qs_msg(struct ice_vf *vf, u8 *msg)
>>   				rxdid = ICE_RXDID_LEGACY_1;
>>   			}
>>   
>> -			ice_write_qrxflxp_cntxt(&vsi->back->hw,
>> -						vsi->rxq_map[q_idx],
>> -						rxdid, 0x03, false);
>> +			if (vf->driver_caps &
>> +			    VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC &&
>> +			    vf->driver_caps & VIRTCHNL_VF_CAP_PTP &&
>> +			    qpi->rxq.flags & VIRTCHNL_PTP_RX_TSTAMP)
> 
> A separate set of parenthesis () around each bitop (& | etc).
> E.g.
> 
> 	if ((vf->driver_caps & FLEX_DESC) &&
> 	    (vf->driver_caps & ...))
> 
>> +				ice_write_qrxflxp_cntxt(&vsi->back->hw,
>> +							vsi->rxq_map[q_idx],
>> +							rxdid, 0x03, true);
>> +			else
>> +				ice_write_qrxflxp_cntxt(&vsi->back->hw,
>> +							vsi->rxq_map[q_idx],
>> +							rxdid, 0x03, false);
> 
> Looks a bit suboptimal to double the same call with the only difference
> in one argument.
> 
> 			bool ptp = (vf->driver_caps & FLEX_DESC) ...
> 
> 			ice_write_qrxflxp_cntxt(hw, map, rxdid, 0x03,
> 						ptp);
> 
> Also, this 0x03... I'd #define it somewhere 'cause for example right now
> I have no idea what this is about.
> 
> 
>>   		}
>>   	}
>>   
>> @@ -3788,6 +3799,65 @@ static int ice_vc_dis_vlan_insertion_v2_msg(struct ice_vf *vf, u8 *msg)
>>   				     v_ret, NULL, 0);
>>   }
>>   
>> +static int ice_vc_get_ptp_cap(struct ice_vf *vf, u8 *msg)
> 
> @msg can be const.
> Also, I'd make it `const void *` or maybe even `const struct
> virtchnl_ptp_caps *` right away.
> 

I am afraid that I can not do that in the scope of this series. This
touches virtchnl messages and seems to be bigger code refactor. I think
in the future we can think about redefining it but not in this series

>> +{
>> +	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
>> +	u32 msg_caps;
>> +	int ret;
>> +
>> +	/* VF is not in active state */
>> +	if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
>> +		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
>> +		goto err;
>> +	}
> 
> Alternatively, you can do it that way:
> 
> 	enum ... v_ret = ERR_PARAM;
> 
> 	if (!test_bit(ACTIVE, vf_states))
> 		goto err;
> 
> 	v_ret = SUCCESS;
> 
>> +
>> +	msg_caps = ((struct virtchnl_ptp_caps *)msg)->caps;
>> +
>> +	/* Any VF asking for RX timestamping and reading PHC will get that */
>> +	if (msg_caps & (VIRTCHNL_1588_PTP_CAP_RX_TSTAMP |
>> +	    VIRTCHNL_1588_PTP_CAP_READ_PHC))
> 
> Bad identation, READ_PHC must match the parenthesis it's enclosed to,
> i.e. the second pair, not the first/outer one.
> 
>> +		vf->ptp_caps.caps = VIRTCHNL_1588_PTP_CAP_RX_TSTAMP |
>> +				    VIRTCHNL_1588_PTP_CAP_READ_PHC;
> 
> Also, hmm, can't that be
> 
> 	u32 caps = VIRTCHNL_1588_PTP_CAP_RX_TSTAMP |
> 		   VIRTCHNL_1588_PTP_CAP_READ_PHC;
> 
> 	if (msg_caps & caps)
> 		vf->ptp_caps = caps;
> 
> ?
> 
>> +
>> +err:
>> +	/* send the response back to the VF */
>> +	ret = ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_1588_PTP_GET_CAPS, v_ret,
>> +				    (u8 *)&vf->ptp_caps,
>> +				    sizeof(struct virtchnl_ptp_caps));
>> +	return ret;
> 
> 1. return ice_vc_send ... directly.
> 2. Try to declare abstract message buffers as `void *`, not `u8 *`. u8
>     is rather for cases when you need to actually read some bytes from
>     the buffer.
> 
>> +}
>> +
>> +static int ice_vc_get_phc_time(struct ice_vf *vf)
>> +{
>> +	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
>> +	struct virtchnl_phc_time *phc_time = NULL;
>> +	struct ice_pf *pf = vf->pf;
>> +	int len = 0;
>> +	int ret;
>> +
>> +	if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
>> +		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
>> +		goto err;
>> +	}
> 
> Same here regarding @v_ret.
> 
>> +
>> +	len = sizeof(struct virtchnl_phc_time);
>> +	phc_time = kzalloc(len, GFP_KERNEL);
> 
> 1. __free(kfree) for @phc_time, so that the function will auto-free it
>     and you won't need to call kfree() later.
> 2. Since @len is trivial, just open-code it here:
> 
> 	phc_time = kzalloc(sizeof(*phc_time), GFP_KERNEL);
> 
> then later
> 
>> +	if (!phc_time) {
>> +		v_ret = VIRTCHNL_STATUS_ERR_NO_MEMORY;
>> +		len = 0;
>> +		goto err;
>> +	}
> 
> 	u32 len = 0;
> 
> 	if (!phc_time) {
> 		v_ret = NO_MEMORY;
> 		goto err;
> 	}
> 
> 	len = sizeof(*phc);
> 
>> +
>> +	phc_time->time = ice_ptp_read_src_clk_reg(pf, NULL);
>> +
>> +err:
>> +	/* send the response back to the VF */
>> +	ret = ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_1588_PTP_GET_TIME,
>> +				    v_ret, (u8 *)phc_time, len);
> 
> (same regarding u8 vs void)
> 
>> +	kfree(phc_time);
>> +	return ret;
> 
> Since kfree() won't be needed, just plain return ice_vc_...
> 
> [...]
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c
>> index d796dbd2a440..7a442a53f4cc 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c
>> @@ -84,6 +84,11 @@ static const u32 fdir_pf_allowlist_opcodes[] = {
>>   	VIRTCHNL_OP_ADD_FDIR_FILTER, VIRTCHNL_OP_DEL_FDIR_FILTER,
>>   };
>>   
>> +/* VIRTCHNL_VF_CAP_PTP */
>> +static const u32 ptp_allowlist_opcodes[] = {
>> +	VIRTCHNL_OP_1588_PTP_GET_CAPS, VIRTCHNL_OP_1588_PTP_GET_TIME,
>> +};
> 
> I'd make it 1 def per line, not two in the same line.
> 
> [...]
> 
>> diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
>> index 5edfb01fa064..c4e7ac19be7c 100644
>> --- a/include/linux/avf/virtchnl.h
>> +++ b/include/linux/avf/virtchnl.h
>> @@ -304,6 +304,18 @@ struct virtchnl_txq_info {
>>   
>>   VIRTCHNL_CHECK_STRUCT_LEN(24, virtchnl_txq_info);
>>   
>> +/* virtchnl_rxq_info_flags
>> + *
>> + * Definition of bits in the flags field of the virtchnl_rxq_info structure.
>> + */
>> +enum virtchnl_rxq_info_flags {
>> +	/* If the VIRTCHNL_PTP_RX_TSTAMP bit of the flag field is set, this is
>> +	 * a request to enable Rx timestamp. Other flag bits are currently
>> +	 * reserved and they may be extended in the future.
>> +	 */
> 
> Make a proper kdoc from the first sentence and leave the second to the
> end of the kdoc.
> 
> /* virtchnl_rxq_info_flags - definition of bits ...
>   *
>   * @VIRTCHNL_PTP_RX_TSTAMP: request to enable Rx timestamping
>   *
>   * Other flag bits are currently reserved and ...
>   */
> enum ...
> 
>> +	VIRTCHNL_PTP_RX_TSTAMP = BIT(0),
>> +};
>> +
>>   /* VIRTCHNL_OP_CONFIG_RX_QUEUE
>>    * VF sends this message to set up parameters for one RX queue.
>>    * External data buffer contains one instance of virtchnl_rxq_info.
>> @@ -327,7 +339,8 @@ struct virtchnl_rxq_info {
>>   	u32 max_pkt_size;
>>   	u8 crc_disable;
>>   	u8 rxdid;
>> -	u8 pad1[2];
>> +	u8 flags; /* see virtchnl_rxq_info_flags */
> 
> Or
> 	enum virtchnl_rxq_info_flags flags:8;
> 
> -- will do the same thing, but with strict type.
> 
>> +	u8 pad1;
>>   	u64 dma_ring_addr;
>>   
>>   	/* see enum virtchnl_rx_hsplit; deprecated with AVF 1.0 */
> 
> Thanks,
> Olek
Alexander Lobakin Aug. 8, 2024, 12:07 p.m. UTC | #3 
From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
Date: Thu, 8 Aug 2024 13:04:14 +0200

> 
> 
> On 7/30/2024 2:54 PM, Alexander Lobakin wrote:
>> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> Date: Tue, 30 Jul 2024 05:14:57 -0400
[...]

>>>   @@ -3788,6 +3799,65 @@ static int
>>> ice_vc_dis_vlan_insertion_v2_msg(struct ice_vf *vf, u8 *msg)
>>>                        v_ret, NULL, 0);
>>>   }
>>>   +static int ice_vc_get_ptp_cap(struct ice_vf *vf, u8 *msg)
>>
>> @msg can be const.
>> Also, I'd make it `const void *` or maybe even `const struct
>> virtchnl_ptp_caps *` right away.
>>
> 
> I am afraid that I can not do that in the scope of this series. This
> touches virtchnl messages and seems to be bigger code refactor. I think
> in the future we can think about redefining it but not in this series

I mean, just make this function take `const struct virtchnl_ptp_caps *`
instead of `u8 *`, you don't need to refactor VC for this one. If you
then get some "incompatible pointer types" warning, just cast the
argument to `const void *` where you call this function.

> 
>>> +{
>>> +    enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
>>> +    u32 msg_caps;
>>> +    int ret;
>>> +
>>> +    /* VF is not in active state */
>>> +    if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
>>> +        v_ret = VIRTCHNL_STATUS_ERR_PARAM;
>>> +        goto err;
>>> +    }

[...]

Thanks,
Olek
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
index 34beee29d205..04f0bfba4fb5 100644
--- a/drivers/net/ethernet/intel/ice/ice_base.c
+++ b/drivers/net/ethernet/intel/ice/ice_base.c
@@ -472,9 +472,6 @@  static int ice_setup_rx_ctx(struct ice_rx_ring *ring)
 	 */
 	if (vsi->type != ICE_VSI_VF)
 		ice_write_qrxflxp_cntxt(hw, pf_q, rxdid, 0x3, true);
-	else
-		ice_write_qrxflxp_cntxt(hw, pf_q, ICE_RXDID_LEGACY_1, 0x3,
-					false);
 
 	/* Absolute queue number out of 2K needs to be passed */
 	err = ice_write_rxq_ctx(hw, &rlan_ctx, pf_q);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 8ed6280af320..6144523179c1 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -286,8 +286,8 @@  void ice_ptp_restore_timestamp_mode(struct ice_pf *pf)
  * @sts: Optional parameter for holding a pair of system timestamps from
  *       the system clock. Will be ignored if NULL is given.
  */
-static u64
-ice_ptp_read_src_clk_reg(struct ice_pf *pf, struct ptp_system_timestamp *sts)
+u64 ice_ptp_read_src_clk_reg(struct ice_pf *pf,
+			     struct ptp_system_timestamp *sts)
 {
 	struct ice_hw *hw = &pf->hw;
 	u32 hi, lo, lo2;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index b8ab162a5538..8fc1d9d823f3 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -323,6 +323,8 @@  void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx, u8 idx);
 void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx);
 enum ice_tx_tstamp_work ice_ptp_process_ts(struct ice_pf *pf);
 
+u64 ice_ptp_read_src_clk_reg(struct ice_pf *pf,
+			     struct ptp_system_timestamp *sts);
 u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
 			const struct ice_pkt_ctx *pkt_ctx);
 void ice_ptp_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type);
@@ -360,6 +362,12 @@  static inline bool ice_ptp_process_ts(struct ice_pf *pf)
 	return true;
 }
 
+static inline u64 ice_ptp_read_src_clk_reg(struct ice_pf *pf,
+					   struct ptp_system_timestamp *sts)
+{
+	return 0;
+}
+
 static inline u64
 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
 		    const struct ice_pkt_ctx *pkt_ctx)
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
index be4266899690..fdc63fae1803 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
@@ -136,6 +136,8 @@  struct ice_vf {
 	const struct ice_virtchnl_ops *virtchnl_ops;
 	const struct ice_vf_ops *vf_ops;
 
+	struct virtchnl_ptp_caps ptp_caps;
+
 	/* devlink port data */
 	struct devlink_port devlink_port;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index 82ad4c6ff4d7..4f5e36c063e2 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -495,6 +495,9 @@  static int ice_vc_get_vf_res_msg(struct ice_vf *vf, u8 *msg)
 	if (vf->driver_caps & VIRTCHNL_VF_OFFLOAD_USO)
 		vfres->vf_cap_flags |= VIRTCHNL_VF_OFFLOAD_USO;
 
+	if (vf->driver_caps & VIRTCHNL_VF_CAP_PTP)
+		vfres->vf_cap_flags |= VIRTCHNL_VF_CAP_PTP;
+
 	vfres->num_vsis = 1;
 	/* Tx and Rx queue are equal for VF */
 	vfres->num_queue_pairs = vsi->num_txq;
@@ -1783,9 +1786,17 @@  static int ice_vc_cfg_qs_msg(struct ice_vf *vf, u8 *msg)
 				rxdid = ICE_RXDID_LEGACY_1;
 			}
 
-			ice_write_qrxflxp_cntxt(&vsi->back->hw,
-						vsi->rxq_map[q_idx],
-						rxdid, 0x03, false);
+			if (vf->driver_caps &
+			    VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC &&
+			    vf->driver_caps & VIRTCHNL_VF_CAP_PTP &&
+			    qpi->rxq.flags & VIRTCHNL_PTP_RX_TSTAMP)
+				ice_write_qrxflxp_cntxt(&vsi->back->hw,
+							vsi->rxq_map[q_idx],
+							rxdid, 0x03, true);
+			else
+				ice_write_qrxflxp_cntxt(&vsi->back->hw,
+							vsi->rxq_map[q_idx],
+							rxdid, 0x03, false);
 		}
 	}
 
@@ -3788,6 +3799,65 @@  static int ice_vc_dis_vlan_insertion_v2_msg(struct ice_vf *vf, u8 *msg)
 				     v_ret, NULL, 0);
 }
 
+static int ice_vc_get_ptp_cap(struct ice_vf *vf, u8 *msg)
+{
+	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
+	u32 msg_caps;
+	int ret;
+
+	/* VF is not in active state */
+	if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
+		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+		goto err;
+	}
+
+	msg_caps = ((struct virtchnl_ptp_caps *)msg)->caps;
+
+	/* Any VF asking for RX timestamping and reading PHC will get that */
+	if (msg_caps & (VIRTCHNL_1588_PTP_CAP_RX_TSTAMP |
+	    VIRTCHNL_1588_PTP_CAP_READ_PHC))
+		vf->ptp_caps.caps = VIRTCHNL_1588_PTP_CAP_RX_TSTAMP |
+				    VIRTCHNL_1588_PTP_CAP_READ_PHC;
+
+err:
+	/* send the response back to the VF */
+	ret = ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_1588_PTP_GET_CAPS, v_ret,
+				    (u8 *)&vf->ptp_caps,
+				    sizeof(struct virtchnl_ptp_caps));
+	return ret;
+}
+
+static int ice_vc_get_phc_time(struct ice_vf *vf)
+{
+	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
+	struct virtchnl_phc_time *phc_time = NULL;
+	struct ice_pf *pf = vf->pf;
+	int len = 0;
+	int ret;
+
+	if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
+		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+		goto err;
+	}
+
+	len = sizeof(struct virtchnl_phc_time);
+	phc_time = kzalloc(len, GFP_KERNEL);
+	if (!phc_time) {
+		v_ret = VIRTCHNL_STATUS_ERR_NO_MEMORY;
+		len = 0;
+		goto err;
+	}
+
+	phc_time->time = ice_ptp_read_src_clk_reg(pf, NULL);
+
+err:
+	/* send the response back to the VF */
+	ret = ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_1588_PTP_GET_TIME,
+				    v_ret, (u8 *)phc_time, len);
+	kfree(phc_time);
+	return ret;
+}
+
 static const struct ice_virtchnl_ops ice_virtchnl_dflt_ops = {
 	.get_ver_msg = ice_vc_get_ver_msg,
 	.get_vf_res_msg = ice_vc_get_vf_res_msg,
@@ -3821,6 +3891,8 @@  static const struct ice_virtchnl_ops ice_virtchnl_dflt_ops = {
 	.dis_vlan_stripping_v2_msg = ice_vc_dis_vlan_stripping_v2_msg,
 	.ena_vlan_insertion_v2_msg = ice_vc_ena_vlan_insertion_v2_msg,
 	.dis_vlan_insertion_v2_msg = ice_vc_dis_vlan_insertion_v2_msg,
+	.get_ptp_cap = ice_vc_get_ptp_cap,
+	.get_phc_time = ice_vc_get_phc_time,
 };
 
 /**
@@ -3951,6 +4023,8 @@  static const struct ice_virtchnl_ops ice_virtchnl_repr_ops = {
 	.dis_vlan_stripping_v2_msg = ice_vc_dis_vlan_stripping_v2_msg,
 	.ena_vlan_insertion_v2_msg = ice_vc_ena_vlan_insertion_v2_msg,
 	.dis_vlan_insertion_v2_msg = ice_vc_dis_vlan_insertion_v2_msg,
+	.get_ptp_cap = ice_vc_get_ptp_cap,
+	.get_phc_time = ice_vc_get_phc_time,
 };
 
 /**
@@ -4177,6 +4251,12 @@  void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event,
 	case VIRTCHNL_OP_DISABLE_VLAN_INSERTION_V2:
 		err = ops->dis_vlan_insertion_v2_msg(vf, msg);
 		break;
+	case VIRTCHNL_OP_1588_PTP_GET_CAPS:
+		err = ops->get_ptp_cap(vf, msg);
+		break;
+	case VIRTCHNL_OP_1588_PTP_GET_TIME:
+		err = ops->get_phc_time(vf);
+		break;
 	case VIRTCHNL_OP_UNKNOWN:
 	default:
 		dev_err(dev, "Unsupported opcode %d from VF %d\n", v_opcode,
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.h b/drivers/net/ethernet/intel/ice/ice_virtchnl.h
index 3a4115869153..e1c32f0f2e7a 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.h
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.h
@@ -61,6 +61,8 @@  struct ice_virtchnl_ops {
 	int (*dis_vlan_stripping_v2_msg)(struct ice_vf *vf, u8 *msg);
 	int (*ena_vlan_insertion_v2_msg)(struct ice_vf *vf, u8 *msg);
 	int (*dis_vlan_insertion_v2_msg)(struct ice_vf *vf, u8 *msg);
+	int (*get_ptp_cap)(struct ice_vf *vf, u8 *msg);
+	int (*get_phc_time)(struct ice_vf *vf);
 };
 
 #ifdef CONFIG_PCI_IOV
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c
index d796dbd2a440..7a442a53f4cc 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c
@@ -84,6 +84,11 @@  static const u32 fdir_pf_allowlist_opcodes[] = {
 	VIRTCHNL_OP_ADD_FDIR_FILTER, VIRTCHNL_OP_DEL_FDIR_FILTER,
 };
 
+/* VIRTCHNL_VF_CAP_PTP */
+static const u32 ptp_allowlist_opcodes[] = {
+	VIRTCHNL_OP_1588_PTP_GET_CAPS, VIRTCHNL_OP_1588_PTP_GET_TIME,
+};
+
 struct allowlist_opcode_info {
 	const u32 *opcodes;
 	size_t size;
@@ -104,6 +109,7 @@  static const struct allowlist_opcode_info allowlist_opcodes[] = {
 	ALLOW_ITEM(VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF, adv_rss_pf_allowlist_opcodes),
 	ALLOW_ITEM(VIRTCHNL_VF_OFFLOAD_FDIR_PF, fdir_pf_allowlist_opcodes),
 	ALLOW_ITEM(VIRTCHNL_VF_OFFLOAD_VLAN_V2, vlan_v2_allowlist_opcodes),
+	ALLOW_ITEM(VIRTCHNL_VF_CAP_PTP, ptp_allowlist_opcodes),
 };
 
 /**
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 5edfb01fa064..c4e7ac19be7c 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -304,6 +304,18 @@  struct virtchnl_txq_info {
 
 VIRTCHNL_CHECK_STRUCT_LEN(24, virtchnl_txq_info);
 
+/* virtchnl_rxq_info_flags
+ *
+ * Definition of bits in the flags field of the virtchnl_rxq_info structure.
+ */
+enum virtchnl_rxq_info_flags {
+	/* If the VIRTCHNL_PTP_RX_TSTAMP bit of the flag field is set, this is
+	 * a request to enable Rx timestamp. Other flag bits are currently
+	 * reserved and they may be extended in the future.
+	 */
+	VIRTCHNL_PTP_RX_TSTAMP = BIT(0),
+};
+
 /* VIRTCHNL_OP_CONFIG_RX_QUEUE
  * VF sends this message to set up parameters for one RX queue.
  * External data buffer contains one instance of virtchnl_rxq_info.
@@ -327,7 +339,8 @@  struct virtchnl_rxq_info {
 	u32 max_pkt_size;
 	u8 crc_disable;
 	u8 rxdid;
-	u8 pad1[2];
+	u8 flags; /* see virtchnl_rxq_info_flags */
+	u8 pad1;
 	u64 dma_ring_addr;
 
 	/* see enum virtchnl_rx_hsplit; deprecated with AVF 1.0 */