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
