| 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:
<PH0PR18MB4474061FA888A03A74CE2D5DDE3F2@PH0PR18MB4474.namprd18.prod.outlook.com>
Date: Mon, 1 Apr 2024 08:38:46 +0000
From: Hariprasad Kelam <hkelam@...vell.com>
To: Simon Horman <horms@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kuba@...nel.org" <kuba@...nel.org>,
"davem@...emloft.net"
<davem@...emloft.net>,
Sunil Kovvuri Goutham <sgoutham@...vell.com>,
Geethasowjanya Akula <gakula@...vell.com>,
Jerin Jacob <jerinj@...vell.com>, Linu Cherian <lcherian@...vell.com>,
Subbaraya Sundeep Bhatta
<sbhatta@...vell.com>,
Naveen Mamindlapalli <naveenm@...vell.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"pabeni@...hat.com"
<pabeni@...hat.com>
Subject: ] Re: [net-next Patch] octeontx2-af: map management port always to
first PF
Hi Simon,
Thanks for the review, see inline
> 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;
> ACK, will fix in V2.
> > +}
> > +
> > +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() ?
>
Max PF value is from 0 to 63 will address proposed change in V2.
> > + 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.
>
Management ports might not be available on all devices, we will go with option2.
Thanks,
Hariprasad k
> 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