| 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
| ||
|
Message-ID: <20240328145819.GN403975@kernel.org>
Date: Thu, 28 Mar 2024 14:58:19 +0000
From: Simon Horman <horms@...nel.org>
To: Hariprasad Kelam <hkelam@...vell.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, kuba@...nel.org,
davem@...emloft.net, sgoutham@...vell.com, gakula@...vell.com,
jerinj@...vell.com, lcherian@...vell.com, sbhatta@...vell.com,
naveenm@...vell.com, edumazet@...gle.com, pabeni@...hat.com
Subject: Re: [net-next Patch] octeontx2-af: map management port always to
first PF
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@...vell.com>
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@...vell.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);
Powered by blists - more mailing lists