> 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)); > } > -#endif Is 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.