Received-SPF: pass (google.com: domain of linux-kernel+bounces-177809-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 13 May 2024 17:14:47 +0100
From: Simon Horman <horms@kernel.org>
To: Bharat Bhushan <bbhushan2@marvell.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	sgoutham@marvell.com, gakula@marvell.com, sbhatta@marvell.com,
	hkelam@marvell.com, davem@davemloft.net, edumazet@google.com,
	kuba@kernel.org, pabeni@redhat.com, jerinj@marvell.com,
	lcherian@marvell.com, richardcochran@gmail.com
Subject: Re: [net-next,v2 3/8] octeontx2-af: Disable backpressure between CPT
 and NIX
Message-ID: <20240513161447.GR2787@kernel.org>
References: <20240513105446.297451-1-bbhushan2@marvell.com>
 <20240513105446.297451-4-bbhushan2@marvell.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240513105446.297451-4-bbhushan2@marvell.com>

On Mon, May 13, 2024 at 04:24:41PM +0530, Bharat Bhushan wrote:
> NIX can assert backpressure to CPT on the NIX<=>CPT link.
> Keep the backpressure disabled for now. NIX block anyways
> handles backpressure asserted by MAC due to PFC or flow
> control pkts.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>

..

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c

..

> @@ -592,8 +596,16 @@ int rvu_mbox_handler_nix_bp_disable(struct rvu *rvu,
>  	bp = &nix_hw->bp;
>  	chan_base = pfvf->rx_chan_base + req->chan_base;
>  	for (chan = chan_base; chan < (chan_base + req->chan_cnt); chan++) {
> -		cfg = rvu_read64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan));
> -		rvu_write64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan),
> +		/* CPT channel for a given link channel is always
> +		 * assumed to be BIT(11) set in link channel.
> +		 */
> +		if (cpt_link)
> +			chan_v = chan | BIT(11);
> +		else
> +			chan_v = chan;

Hi Bharat,

The chan_v logic above seems to appear twice in this patch.
I'd suggest adding a helper.

> +
> +		cfg = rvu_read64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v));
> +		rvu_write64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v),
>  			    cfg & ~BIT_ULL(16));
>  
>  		if (type == NIX_INTF_TYPE_LBK) {

..

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 7ec99c8d610c..e9d2e039a322 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -1705,6 +1705,31 @@ int otx2_nix_config_bp(struct otx2_nic *pfvf, bool enable)
>  }
>  EXPORT_SYMBOL(otx2_nix_config_bp);
>  
> +int otx2_nix_cpt_config_bp(struct otx2_nic *pfvf, bool enable)
> +{
> +	struct nix_bp_cfg_req *req;
> +
> +	if (enable)
> +		req = otx2_mbox_alloc_msg_nix_cpt_bp_enable(&pfvf->mbox);
> +	else
> +		req = otx2_mbox_alloc_msg_nix_cpt_bp_disable(&pfvf->mbox);
> +
> +	if (!req)
> +		return -ENOMEM;
> +
> +	req->chan_base = 0;
> +#ifdef CONFIG_DCB
> +	req->chan_cnt = pfvf->pfc_en ? IEEE_8021QAZ_MAX_TCS : 1;
> +	req->bpid_per_chan = pfvf->pfc_en ? 1 : 0;
> +#else
> +	req->chan_cnt =  1;
> +	req->bpid_per_chan = 0;
> +#endif

IMHO, inline #ifdefs reduce readability and reduce maintainability.

Would it be possible to either:

1. Include the pfc_en field in struct otx2_nic and make
   sure it is set to 0 if CONFIG_DCB is unset; or
2. Provide a wrapper that returns 0 if CONFIG_DCB is unset,
   otherwise pfvf->pfc_en.

I suspect 1 will have little downside and be easiest to implement.

> +
> +	return otx2_sync_mbox_msg(&pfvf->mbox);
> +}
> +EXPORT_SYMBOL(otx2_nix_cpt_config_bp);
> +
>  /* Mbox message handlers */
>  void mbox_handler_cgx_stats(struct otx2_nic *pfvf,
>  			    struct cgx_stats_rsp *rsp)

..