RE: [EXTERNAL] Re: [PATCH net] octeontx2-af: Fix representor flow rule PF_FUNC mask and channel In representor mode, an AF-installed TX rule must still match on PF_FUNC, so do not clear the mask when rep_mode is set. Also, do not overwrite a channel alrea

[Kalluru Mahammad Rafi [C]]

> -----Original Message-----
> From: netdev-bot+sashiko@xxxxxxxxxx <netdev-bot+sashiko@xxxxxxxxxx>
> Sent: Sunday, October 25, 2026 4:49 PM
> To: Kalluru Mahammad Rafi [C] <kmahammadraf@xxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Sunil Kovvuri
> Goutham <sgoutham@xxxxxxxxxxx>; Ratheesh Kannoth
> <rkannoth@xxxxxxxxxxx>; Geethasowjanya Akula <gakula@xxxxxxxxxxx>;
> Subbaraya Sundeep Bhatta <sbhatta@xxxxxxxxxxx>;
> andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; Aditya Thapliyal
> <athapliyal@xxxxxxxxxxx>
> Subject: [EXTERNAL] Re: [PATCH net] octeontx2-af: Fix representor flow rule
> PF_FUNC mask and channel In representor mode, an AF-installed TX rule
> must still match on PF_FUNC, so do not clear the mask when rep_mode is set.
> Also, do not overwrite a channel alread...
> 
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s)
> to consider. Critical: 0 · High: 0 (1) · Medium: 1 · Low: 3 If you believe none of
> the issues are legitimate, please reply to each issue of Medium severity or
> higher ZjQcmQRYFpfptBannerStart Prioritize security for external emails:
> Confirm sender and content safety before clicking links or opening
> attachments <https://us-phishalarm-
> ewt.proofpoint.com/EWT/v1/CRVmXkqW!tk3ZtR5041WaFC89mT3ZrgUl2rQGI
> DryWiu2Bp5I8NTNvs9TcdmjNQCL9v3AQwAXV5wg-
> NPZJxzkRqNEMIeendWqXX6socTLZgf_oprx$>
> Report Suspicious
> 
> ZjQcmQRYFpfptBannerEnd
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 1 · Low: 3
> 
> If you believe none of the issues are legitimate, please reply to each issue of
> Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The channel hunk in rvu_mbox_handler_npc_install_flow() (`if
>   (from_rep_dev && !req->channel) req->channel = pfvf->rx_chan_base;`)…
> - [Low] The commit bundles two unrelated fixes and gives them a single
>   Fixes: tag that only matches one of them.
> - [Low] The commit message has no blank line between subject and body.
> - [Low] The comment above the changed condition in npc_update_tx_entry()
>   still says 'If AF is installing then do not care about PF_FUNC in Send…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but it stays in
>   the representor TX path this patch is fixing.
> 

ACK all issues
pw-bot: cr
> --
> 
> octeontx2-af: Fix representor flow rule PF_FUNC mask and channel
> 
> When rvu->rep_mode is set, this patch keeps the PF_FUNC match on AF-
> installed NPC TX rules instead of wildcarding it. It also changes
> rvu_mbox_handler_npc_install_flow() for representor requests.
> req->channel is now replaced with the target's rx_chan_base only when
> req->the
> request leaves it at zero.
> 
> > octeontx2-af: Fix representor flow rule PF_FUNC mask and channel In
> representor mode, an AF-installed TX rule must still match on PF_FUNC, so do
> not clear the mask when rep_mode is set. Also, do not overwrite a channel
> already provided in the request when the rule comes from a representor
> device; only default to the PF/VF rx_chan_base when none is given.
> 
> [Severity: Low]
> This isn't a bug, but there's no blank line between the subject and the body.
> As a result, the whole paragraph becomes the subject line.
> 
> The first Signed-off-by is also:
> 
>   Signed-off-by: kmahammadraf <kmahammadraf@xxxxxxxxxxx>
> 
> That doesn't look like a real name, and it isn't the author. If kmahammadraf
> co-developed the patch, should there be a Co-developed-by:
> tag next to it? If kmahammadraf is the original author, should the From:
> line say so?
> 
> > Fixes: 6c40ca957fe5 ("octeontx2-pf: Adds TC offload support")
> 
> [Severity: Low]
> The channel override does come from 6c40ca957fe5. Is that also the right
> target for the PF_FUNC mask change?
> 
> The AF TX rules that need the PF_FUNC match were added in
> rvu_rep_install_tx_rule() by commit 683645a2317e ("octeontx2-af: Add
> packet path between representor and VF"). Those rules are installed with
> req.hdr.pcifunc = 0, and only PF_FUNC tells them apart. The wildcard itself
> goes back to commit fa2bf6baf2b1 ("octeontx2-af: Enable transmit side LBK
> link").
> 
> Would it be cleaner to split this into two patches, each with its own
> Fixes: tag? 683645a2317e and 6c40ca957fe5 went in on the same day, so
> stable selection is probably not affected.
> 
> The commit message also doesn't describe the symptom. Before this patch,
> the rte=true TX rule from rvu_rep_install_tx_rule() had no features and a
> wildcarded PF_FUNC. It therefore matched every TX packet on that NIX TX
> interface, and the first such entry (PF1) sent all PF/VF egress to the LBK
> channel with PF1's VLAN tag.
> 
> Could the commit message mention that?
> 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> > index d422bdd5e8f80..a569a63a21d9d 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> > @@ -1397,7 +1397,7 @@ static int npc_update_tx_entry(struct rvu *rvu,
> struct rvu_pfvf *pfvf,
> >  	/* If AF is installing then do not care about
> >  	 * PF_FUNC in Send Descriptor
> >  	 */
> > -	if (is_pffunc_af(req->hdr.pcifunc))
> > +	if (is_pffunc_af(req->hdr.pcifunc) && !rvu->rep_mode)
> >  		mask = 0;
> 
> [Severity: Low]
> This isn't a bug, but the comment above still says AF-installed rules don't care
> about PF_FUNC. When rep_mode is set, AF-installed TX rules now match on
> PF_FUNC (mask = ~0ULL, value htons(target)), and
> rvu_rep_install_tx_rule() relies on that.
> 
> Could the comment mention the rep_mode exception?
> 
> >
> >  	npc_update_entry(rvu, NPC_PF_FUNC, mdata, (__force
> > u16)htons(target),
> 
> [Severity: High]
> This is a pre-existing issue, but it remains in the representor TX path this patch
> is fixing. PF_FUNC is now the only matched TX key field. Do all the rte=false
> rules from rvu_rep_install_tx_rule() then end up with the same MCAM key?
> 
> drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c:rvu_rep_install_tx_rule(
> ) {
>     ...
> 	req.hdr.pcifunc = 0; /* AF is requester */
> 	if (rte) {
> 		req.vf = pcifunc;
> 	} else {
> 		req.vf = rvu->rep_pcifunc;
> 		req.packet.sq_id = vlan_tci;
> 		req.mask.sq_id = 0xffff;
> 	}
>     ...
> }
> 
> No req.features are set, so npc_update_flow() returns before it programs
> anything from req->packet:
> 
> 	if (!features)
> 		return;
> 
> Nothing in af/ appears to put sq_id into the key either; there is no NPC_SQ_ID
> key field. The only match left is PF_FUNC = htons(rvu->rep_pcifunc), and that
> is the same for every representee.
> 
> rvu_rep_install_mcam_rules() installs one rte=false TX entry per representee,
> in index order. rvu_mbox_handler_npc_install_flow() always enables TX
> rules.
> 
> Assuming the lowest-index entry wins when several match, wouldn't egress
> from every representor netdev hit the first enabled rte=false entry? It would
> then get that representee's vtag0_def VLAN, go out on
> RVU_SWITCH_LBK_CHAN, and be delivered to that single representee.
> 
> For example, frames that a bridge forwards to the Rpf1vf2 representor would
> seem to reach a different function.
> 
> [ ... ]
> 
> > @@ -1846,7 +1846,7 @@ int rvu_mbox_handler_npc_install_flow(struct
> rvu
> > *rvu,
> >
> >  	pfvf = rvu_get_pfvf(rvu, target);
> >
> > -	if (from_rep_dev)
> > +	if (from_rep_dev && !req->channel)
> >  		req->channel = pfvf->rx_chan_base;
> 
> [Severity: Medium]
> Which representor requester sends a non-zero channel here? The only in-tree
> sender seems to be otx2_tc_add_flow(). It is reached through
> rvu_rep_setup_tc_cb(), which reuses the representor PF's otx2_nic:
> 
> drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c:otx2_tc_add_flow() {
>     ...
> 	req->channel = nic->hw.rx_chan_base;
>     ...
> }
> 
> rx_chan_base never seems to be set for the representor PF.
> rvu_mbox_handler_nix_lf_alloc() jumps past nix_interface_init(), which is the
> only place pfvf->rx_chan_base is assigned:
> 
> 	if (is_rep_dev(rvu, pcifunc)) {
> 		pfvf->tx_chan_base = RVU_SWITCH_LBK_CHAN;
> 		pfvf->tx_chan_cnt = 1;
> 		goto exit;
> 	}
> 
> So the rx_chan_base returned to the PF, which is copied into
> hw.rx_chan_base, is 0. Every representor TC request still arrives with channel
> == 0. It is still replaced with the representee's rx_chan_base, as before this
> patch.
> 
> Does this hunk change anything for in-tree callers?
> 
> Can 0 safely mean "not provided"? NIX_CHAN_LBK_CHX(0, 0) evaluates to 0,
> and CN10K uses it as hw->lbk_chan_base, so 0 is a valid LBK channel.
> struct npc_install_flow_req also has no flag a caller could use to ask for
> channel 0 explicitly.
> 
> If the representor PF ever reports a non-zero rx_chan_base, wouldn't
> representee RX rules then match the representor PF's own channel rather
> than the representee's? That would weaken the override that 6c40ca957fe5
> added.
> 
> --
> Sashiko AI review · https://urldefense.com/v3/__https://netdev-
> ai.bots.linux.dev/sashiko/*/patchset/20261021091736.2406505-1-
> kmahammadraf*40marvell.com__;IyU!!CRVmXkqW!XrRTpXNxMhjfvSuo84Mg
> -ZYmvlutjg0qve9WUjlkni7Fk7U_SH2Ix2PrSYJHlM-
> P49glJtzMBeecTyWd6Itt_GsQAaq64ww$