From: Mateusz Polchlopek <mateusz.polchlo...@intel.com>
Date: Tue, 30 Jul 2024 05:14:57 -0400
> From: Simei Su <simei...@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
