netdev - RE: [PATCH] qede: fix CONFIG_INFINIBAND

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Oct 2016 08:50:21 +0000
From:   "Mintz, Yuval" <Yuval.Mintz@...ium.com>
To:     Arnd Bergmann <arnd@...db.de>
CC:     Ariel Elior <Ariel.Elior@...gic.com>,
        "everest-linux-l2@...gic.com" <everest-linux-l2@...gic.com>,
        Alexander Duyck <aduyck@...antis.com>,
        "Amrani, Ram" <Ram.Amrani@...ium.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: RE: [PATCH] qede: fix CONFIG_INFINIBAND_QEDR=m build error

>  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.