[Intel-wired-lan] [PATCH iwl-next v8 02/14] ice: support Rx timestamp on flex descriptor
Mateusz Polchlopek
mateusz.polchlopek at intel.com
Thu Aug 8 11:04:14 UTC 2024
On 7/30/2024 2:54 PM, Alexander Lobakin wrote:
> From: Mateusz Polchlopek <mateusz.polchlopek at intel.com>
> Date: Tue, 30 Jul 2024 05:14:57 -0400
>
>> From: Simei Su <simei.su at 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
More information about the Intel-wired-lan
mailing list