Received-SPF: pass (google.com: domain of linux-kernel+bounces-123123-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 28 Mar 2024 14:58:19 +0000
From: Simon Horman <horms@kernel.org>
To: Hariprasad Kelam <hkelam@marvell.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kuba@kernel.org,
	davem@davemloft.net, sgoutham@marvell.com, gakula@marvell.com,
	jerinj@marvell.com, lcherian@marvell.com, sbhatta@marvell.com,
	naveenm@marvell.com, edumazet@google.com, pabeni@redhat.com
Subject: Re: [net-next Patch] octeontx2-af: map management port always to
 first PF
Message-ID: <20240328145819.GN403975@kernel.org>
References: <20240327160348.3023-1-hkelam@marvell.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240327160348.3023-1-hkelam@marvell.com>

On Wed, Mar 27, 2024 at 09:33:48PM +0530, Hariprasad Kelam wrote:
> The user can enable or disable any MAC block or a few ports of the
> block. The management port's interface name varies depending on the
> setup of the user if its not mapped to the first pf.
> 
> The management port mapping is now configured to always connect to the
> first PF. This patch implements this change.
> 
> Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>

Hi Hariprasad and Sunil,

some feedback from my side.

> ---
>  .../net/ethernet/marvell/octeontx2/af/mbox.h  |  5 +-
>  .../ethernet/marvell/octeontx2/af/rvu_cgx.c   | 60 +++++++++++++++----
>  2 files changed, 53 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> index eb2a20b5a0d0..105d2e8f25df 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> @@ -638,7 +638,10 @@ struct cgx_lmac_fwdata_s {
>  	/* Only applicable if SFP/QSFP slot is present */
>  	struct sfp_eeprom_s sfp_eeprom;
>  	struct phy_s phy;
> -#define LMAC_FWDATA_RESERVED_MEM 1021
> +	u32 lmac_type;
> +	u32 portm_idx;
> +	u64 mgmt_port:1;
> +#define LMAC_FWDATA_RESERVED_MEM 1019
>  	u64 reserved[LMAC_FWDATA_RESERVED_MEM];
>  };
>  
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> index 72e060cf6b61..446344801576 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> @@ -118,15 +118,40 @@ static void rvu_map_cgx_nix_block(struct rvu *rvu, int pf,
>  		pfvf->nix_blkaddr = BLKADDR_NIX1;
>  }
>  
> -static int rvu_map_cgx_lmac_pf(struct rvu *rvu)
> +static bool rvu_cgx_is_mgmt_port(struct rvu *rvu, int cgx_id, int lmac_id)
> +{
> +	struct cgx_lmac_fwdata_s *fwdata;
> +
> +	fwdata =  &rvu->fwdata->cgx_fw_data_usx[cgx_id][lmac_id];
> +	if (fwdata->mgmt_port)
> +		return true;
> +
> +	return false;

nit: I think this could be more succinctly expressed as:

	return !!fwdata->mgmt_port;

> +}
> +
> +static void __rvu_map_cgx_lmac_pf(struct rvu *rvu, int pf, int cgx, int lmac)
>  {
>  	struct npc_pkind *pkind = &rvu->hw->pkind;
> +	int numvfs, hwvfs;
> +	int free_pkind;
> +
> +	rvu->pf2cgxlmac_map[pf] = cgxlmac_id_to_bmap(cgx, lmac);
> +	rvu->cgxlmac2pf_map[CGX_OFFSET(cgx) + lmac] = 1 << pf;

This isn't strictly related to this patch, but here
it seems implied that pf is not negative and <= 63, as
rvu->cgxlmac2pf_map[CGX_OFFSET(cgx) + lmac] is only 64 bits wide.

So firstly I wonder if pf should be unsigned

> +	free_pkind = rvu_alloc_rsrc(&pkind->rsrc);
> +	pkind->pfchan_map[free_pkind] = ((pf) & 0x3F) << 16;

Here pf is masked off so it is not more than 63.
But that seems to conflict with the assumption above that it is <= 63.

If there is a concern about it being larger, it should be
capped in the for loop that calls __rvu_map_cgx_lmac_pf() ?

> +	rvu_map_cgx_nix_block(rvu, pf, cgx, lmac);
> +	rvu->cgx_mapped_pfs++;
> +	rvu_get_pf_numvfs(rvu, pf, &numvfs, &hwvfs);
> +	rvu->cgx_mapped_vfs += numvfs;
> +}
> +
> +static int rvu_map_cgx_lmac_pf(struct rvu *rvu)
> +{
>  	int cgx_cnt_max = rvu->cgx_cnt_max;
>  	int pf = PF_CGXMAP_BASE;
>  	unsigned long lmac_bmap;
> -	int size, free_pkind;
>  	int cgx, lmac, iter;
> -	int numvfs, hwvfs;
> +	int size;
>  
>  	if (!cgx_cnt_max)
>  		return 0;
> @@ -155,6 +180,24 @@ static int rvu_map_cgx_lmac_pf(struct rvu *rvu)
>  		return -ENOMEM;
>  
>  	rvu->cgx_mapped_pfs = 0;
> +
> +	/* Map mgmt port always to first PF */
> +	for (cgx = 0; cgx < cgx_cnt_max; cgx++) {
> +		if (!rvu_cgx_pdata(cgx, rvu))
> +			continue;
> +		lmac_bmap = cgx_get_lmac_bmap(rvu_cgx_pdata(cgx, rvu));
> +		for_each_set_bit(iter, &lmac_bmap, rvu->hw->lmac_per_cgx) {
> +			lmac = cgx_get_lmacid(rvu_cgx_pdata(cgx, rvu), iter);
> +			if (rvu_cgx_is_mgmt_port(rvu, cgx, lmac)) {
> +				__rvu_map_cgx_lmac_pf(rvu, pf, cgx, lmac);
> +				pf++;
> +				goto non_mgmtport_mapping;
> +			}
> +		}
> +	}
> +
> +non_mgmtport_mapping:
> +
>  	for (cgx = 0; cgx < cgx_cnt_max; cgx++) {
>  		if (!rvu_cgx_pdata(cgx, rvu))
>  			continue;
> @@ -162,14 +205,9 @@ static int rvu_map_cgx_lmac_pf(struct rvu *rvu)
>  		for_each_set_bit(iter, &lmac_bmap, rvu->hw->lmac_per_cgx) {
>  			lmac = cgx_get_lmacid(rvu_cgx_pdata(cgx, rvu),
>  					      iter);
> -			rvu->pf2cgxlmac_map[pf] = cgxlmac_id_to_bmap(cgx, lmac);
> -			rvu->cgxlmac2pf_map[CGX_OFFSET(cgx) + lmac] = 1 << pf;
> -			free_pkind = rvu_alloc_rsrc(&pkind->rsrc);
> -			pkind->pfchan_map[free_pkind] = ((pf) & 0x3F) << 16;
> -			rvu_map_cgx_nix_block(rvu, pf, cgx, lmac);
> -			rvu->cgx_mapped_pfs++;
> -			rvu_get_pf_numvfs(rvu, pf, &numvfs, &hwvfs);
> -			rvu->cgx_mapped_vfs += numvfs;
> +			if (rvu_cgx_is_mgmt_port(rvu, cgx, lmac))
> +				continue;
> +			__rvu_map_cgx_lmac_pf(rvu, pf, cgx, lmac);
>  			pf++;
>  		}
>  	}

There seems to be a fair amount of code duplication above.
If we can assume that there is always a management port,
then perhaps the following is simpler (compile tested only!).

And if not, I'd suggest moving the outermost for loop and everything
within it into a helper with a parameter such that it can handle
the (first?) management port on one invocation, and non management
ports on the next invocation.

 static int rvu_map_cgx_lmac_pf(struct rvu *rvu)
 {
 	struct npc_pkind *pkind = &rvu->hw->pkind;
 	int cgx_cnt_max = rvu->cgx_cnt_max;
-	int pf = PF_CGXMAP_BASE;
+	int next_pf = PF_CGXMAP_BASE + 1;
 	unsigned long lmac_bmap;
 	int size, free_pkind;
 	int cgx, lmac, iter;
@@ -158,10 +167,20 @@ static int rvu_map_cgx_lmac_pf(struct rvu *rvu)
 	for (cgx = 0; cgx < cgx_cnt_max; cgx++) {
 		if (!rvu_cgx_pdata(cgx, rvu))
 			continue;
+
 		lmac_bmap = cgx_get_lmac_bmap(rvu_cgx_pdata(cgx, rvu));
 		for_each_set_bit(iter, &lmac_bmap, rvu->hw->lmac_per_cgx) {
+			int pf;
+
 			lmac = cgx_get_lmacid(rvu_cgx_pdata(cgx, rvu),
 					      iter);
+
+			/* Always use first PF for management port */
+			if (rvu_cgx_is_mgmt_port(rvu, cgx, lmac))
+				pf = PF_CGXMAP_BASE;
+			else
+				pf = next_pf++;
+
 			rvu->pf2cgxlmac_map[pf] = cgxlmac_id_to_bmap(cgx, lmac);
 			rvu->cgxlmac2pf_map[CGX_OFFSET(cgx) + lmac] = 1 << pf;
 			free_pkind = rvu_alloc_rsrc(&pkind->rsrc);