RE: [PATCH] qede: fix CONFIG_INFINIBAND_QEDR=m build error
From: Mintz, Yuval <hidden>
Date: 2016-10-13 12:25:47
Also in:
lkml
config INFINIBAND_QEDR - tristate "QLogic qede RoCE sources [debug]" + bool "QLogic qede RoCE sources [debug]"
Given that the qedr submission is going to turn this back into a tristate, are you certain this is a good thing [from compilation coverage perspective]?
- if (cond) + if (IS_ENABLED(CONFIG_INFINIBAND_QEDR) && cond) qed_rdma_dpm_bar(p_hwfn, p_ptt);
Why not simply fix the qed_roce.h empty implementation?
-#if IS_ENABLED(CONFIG_INFINIBAND_QEDR)
/* Roce CNQ each requires: 1 status block + 1 CNQ. We divide the
* status blocks equally between L2 / RoCE but with consideration as
* to how many l2 queues / cnqs we have
*/
- if (p_hwfn->hw_info.personality == QED_PCI_ETH_ROCE) {
+ if (IS_ENABLED(CONFIG_INFINIBAND_QEDR) &&
+ p_hwfn->hw_info.personality == QED_PCI_ETH_ROCE) {
num_features++;
feat_num[QED_RDMA_CNQ] =
min_t(u32, RESC_NUM(p_hwfn, QED_SB) /
num_features,
RESC_NUM(p_hwfn, QED_RDMA_CNQ_RAM));
}
-#endifIs there any non-cosmetic gain here? I would gain that having the comment under the #ifdef is more meaningful than having the check in the actual condition.
-#if IS_ENABLED(CONFIG_INFINIBAND_QEDR) + if (!IS_ENABLED(CONFIG_INFINIBAND_QEDR)) + return 0; + num_l2_queues = 0; for_each_hwfn(cdev, i) num_l2_queues += FEAT_NUM(&cdev->hwfns[i], QED_PF_L2_QUE); @@ -738,7 +736,6 @@ static int qed_slowpath_setup_int(struct qed_dev *cdev, DP_VERBOSE(cdev, QED_MSG_RDMA, "roce_msix_cnt=%d roce_msix_base=%d\n", cdev->int_params.rdma_msix_cnt, cdev->int_params.rdma_msix_base); -#endif
While I don't mind, you could have argued is that we're not removing enough, not too much. I.e., perhaps the rdma_msix_* fields should also have been ifdef-ed instead. [in which case this solution would not have worked]
-#if IS_ENABLED(CONFIG_INFINIBAND_QEDR)
- params->rdma_pf_params.num_qps = QED_ROCE_QPS;
- params->rdma_pf_params.min_dpis = QED_ROCE_DPIS;
- /* divide by 3 the MRs to avoid MF ILT overflow */
- params->rdma_pf_params.num_mrs = RDMA_MAX_TIDS;
- params->rdma_pf_params.gl_pi = QED_ROCE_PROTOCOL_INDEX;
-#endif
+ if (IS_ENABLED(CONFIG_INFINIBAND_QEDR)) {
+ params->rdma_pf_params.num_qps = QED_ROCE_QPS;
+ params->rdma_pf_params.min_dpis = QED_ROCE_DPIS;
+ /* divide by 3 the MRs to avoid MF ILT overflow */
+ params->rdma_pf_params.num_mrs = RDMA_MAX_TIDS;
+ params->rdma_pf_params.gl_pi =
QED_ROCE_PROTOCOL_INDEX;
+ }Likewise
-#if IS_ENABLED(CONFIG_INFINIBAND_QEDR)
- case PROTOCOLID_ROCE:
- qed_async_roce_event(p_hwfn, p_eqe);
- return 0;
-#endif
case PROTOCOLID_COMMON:
return qed_sriov_eqe_event(p_hwfn,
p_eqe->opcode,
p_eqe->echo, &p_eqe->data);
+ case PROTOCOLID_ROCE:
+ if (IS_ENABLED(CONFIG_INFINIBAND_QEDR)) {
+ qed_async_roce_event(p_hwfn, p_eqe);
+ return 0;
+ }
+ /* fallthrough */Not sure whether it helps readability; It might give the false Impression that it's possible to receive an async roce event if roce is compiled-out, which isn't the case.