Re: [PATCH net v3] octeontx2-af: Fix rep link state sync and workqueue races

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 10 potential
issue(s) to consider:

Critical: 0 · High: 2 (1) · Medium: 7 (1) · Low: 1

- [High] The new authorization gates added by this patch
  (`req->hdr.pcifunc != rvu->rep_pcifunc -> -EPERM` in…
- [High] rvu_mbox_handler_get_rep_cnt() now truncates rvu->rep_cnt /
  rep2pfvf_map to RVU_MAX_REP (64) entries, but every consumer in the AF…
- [Medium] rvu_rep_open() and rvu_rep_stop() in nic/rep.c now return
  early for every VF representor (`if (rep->pcifunc &…
- [Medium] The new teardown barrier in rvu_remove() only flushes
  rvu->afpf_wq_info.mbox_wq before destroy_workqueue(rep_wq), and its…
- [Medium] The representor PF's own TL2->LBK link enable performed by
  rvu_rep_pf_init() (rvu_rep.c:445-451, rvu_switch_enable_lbk_link(rvu,…
- [Medium] rvu_remove() decides whether to destroy rvu->rep_evt_wq, and
  clears the pointer, without holding rvu->rsrc_lock, whereas…
- [Medium] The patch introduces a lockless publish/consume protocol for
  rvu->rep_evt_wq (rvu_remove() uses WRITE_ONCE(),…
- [Medium] rvu_rep_install_mcam_rules() tests test_bit(NIXLF_INITIALIZED,
  &pfvf->flags) outside any lock and then calls…
- [Medium] Draining the representor event workqueue after unregistering
  interrupts causes mailbox communication timeouts.
- [Low] In the first-time GET_REP_CNT path, `rvu->rep_pcifunc =
  req->hdr.pcifunc;` (rvu_rep.c:504) executes before both -ENOMEM exits…

Pre-existing issues:
- [High] When rvu_rep_install_mcam_rules() fails (the new err_disable_lbk
  path still returns err), rvu_switch_enable()'s uninstall_rules label…
- [Medium] rvu_rep_install_tx_rule() first calls rvu_rep_tx_vlan_cfg(),
  which allocates a NIX TX VTAG definition entry and programs…

--

octeontx2-af: Fix rep link state sync and workqueue races

This moves the representor event workqueue creation from
rvu_rep_install_mcam_rules() into rvu_mbox_handler_get_rep_cnt(), adds
sender and bounds validation to the REP mailbox handlers, and makes LBK
link configuration take rsrc_lock.  A few questions below about the new
validation and teardown paths.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> index 937b085582b5..dc4e1fa900b7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> @@ -3721,6 +3721,20 @@ static void rvu_remove(struct pci_dev *pdev)
>  	rvu_unregister_dl(rvu);
>  	rvu_unregister_interrupts(rvu);
>  	rvu_flr_wq_destroy(rvu);
> +	if (rvu->rep_evt_wq) {
> +		struct workqueue_struct *rep_wq = rvu->rep_evt_wq;
> +
> +		/* NULL the pointer before flushing mbox_wq.  Any mbox handler
> +		 * still in flight will snapshot NULL via READ_ONCE() and return
> +		 * -EINVAL without calling queue_work(), so no new items can be
> +		 * added to rep_wq after flush_workqueue(mbox_wq) returns.
> +		 */
> +		WRITE_ONCE(rvu->rep_evt_wq, NULL);
> +		flush_workqueue(rvu->afpf_wq_info.mbox_wq);
> +		destroy_workqueue(rep_wq);
> +	} else {
> +		flush_workqueue(rvu->afpf_wq_info.mbox_wq);
> +	}

[Severity: Medium]
This teardown sits after rvu_unregister_interrupts(), which has
already disabled the AF-PF mailbox interrupt (RVU_AF_PFAF_MBOX_INT_ENA_W1C,
or cn20k_rvu_unregister_interrupts()).  destroy_workqueue() calls
drain_workqueue(), so any rep_evt_work still queued now runs to
completion here, and rvu_rep_wq_handler() -> rvu_rep_up_notify() does
otx2_mbox_msg_send_up() followed by otx2_mbox_wait_for_zero() and
otx2_mbox_wait_for_rsp() on rvu->afpf_wq_info.mbox_up.

With the mailbox interrupt already gone, the PF's reply is never
processed by the AF, so each of those waits has to time out rather
than complete.  With a non-empty rep_evtq_head that is paid per
queued event, all while holding rvu->mbox_lock, which stretches
rvu_remove() out by seconds at a time on a path userspace is
blocked on.

Would it be better to move this block ahead of
rvu_unregister_interrupts() so the pending up-notifies can still be
acknowledged, or alternatively to stop rvu_rep_wq_handler() from
sending anything once removal has started (a teardown flag checked
before otx2_mbox_msg_send_up(), or draining rep_evtq_head and
freeing the qentries without notifying)?  Note the same question
applies to the qentry list itself: nothing appears to free the
entries that are still on rvu->rep_evtq_head if the handler is not
allowed to consume them.

[Severity: Medium]
Is flushing only rvu->afpf_wq_info.mbox_wq enough to close the
queue_work() window the comment describes?

The same rvu_mbox_handler_* dispatch also runs on
rvu->afvf_wq_info.mbox_wq via rvu_afvf_mbox_handler(), and that
workqueue is destroyed much later, inside rvu_disable_sriov() ->
rvu_mbox_destroy().  For AF-VF messages the dispatcher clears only the
func bits and keeps the sender-supplied PF field, so
rvu_mbox_handler_nix_lf_start_rx()/nix_lf_stop_rx()/rvu_nix_lf_teardown()
can reach rvu_rep_notify_pfvf_state(), which sets
req.hdr.pcifunc = rvu->rep_pcifunc and therefore passes the new -EPERM
gate:

CPU0 (afvf mbox work)               CPU1 (rvu_remove)
  wq = READ_ONCE(rvu->rep_evt_wq);  /* non-NULL */
                                    WRITE_ONCE(rvu->rep_evt_wq, NULL);
                                    flush_workqueue(afpf mbox_wq);
                                    destroy_workqueue(rep_wq);
  queue_work(wq, &rvu->rep_evt_work);  /* freed wq */

Should the afvf mailbox workqueue be flushed here too (or the VF mailbox
torn down before this point)?  The qentry that handler already kmalloc'd
and put on rep_evtq_head is leaked in that case as well.

[Severity: Medium]
Can rvu->rep_evt_wq become non-NULL after this if () test?  The
producer, rvu_mbox_handler_get_rep_cnt(), now publishes the pointer
under rvu->rsrc_lock, while this teardown reads and clears it with no
lock and never re-checks:

CPU0 (get_rep_cnt work on mbox_wq)   CPU1 (rvu_remove)
                                     if (rvu->rep_evt_wq)  /* NULL */
                                     else
                                       flush_workqueue(mbox_wq);
  mutex_lock(&rvu->rsrc_lock);
  rvu->rep_evt_wq = alloc_workqueue(...);

The freshly created workqueue and its worker pool are then never
destroyed, and a REP_EVENT_NOTIFY message processed later in the same
drained batch can queue_work(&rvu->rep_evt_work) on a workqueue nothing
waits for, so rvu_rep_wq_handler() may touch rvu after
devm_kfree(&pdev->dev, rvu).  Would taking rsrc_lock here (or a
"removing" flag checked by the handler) close that?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> index a2781e0f504e..69b260fdd52d 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c

[ ... ]

> @@ -95,8 +101,17 @@ static void rvu_rep_wq_handler(struct work_struct *work)
>  int rvu_mbox_handler_rep_event_notify(struct rvu *rvu, struct rep_event *req,
>  				      struct msg_rsp *rsp)
>  {
> +	struct workqueue_struct *wq;
>  	struct rep_evtq_ent *qentry;
>  
> +	wq = READ_ONCE(rvu->rep_evt_wq);
> +	if (!wq)
> +		return -EINVAL;
> +
> +	/* Only the registered representor PF may send REP_EVENT_NOTIFY. */
> +	if (req->hdr.pcifunc != rvu->rep_pcifunc)
> +		return -EPERM;

[Severity: High]
How much does this gate actually restrict, given that rvu->rep_pcifunc
is whatever function sent MBOX_MSG_GET_REP_CNT first?

rvu_mbox_handler_get_rep_cnt() still does an unconditional
rvu->rep_pcifunc = req->hdr.pcifunc with no check that the sender is the
representor function (PCI_DEVID_RVU_REP), no capability check and no
rep_mode gating.  The PF driver forwards VF mailbox messages to the AF
verbatim - otx2_pfvf_mbox_handler() only rewrites the func bits and does
no message-ID filtering:

	msg->pcifunc &= ~RVU_PFVF_FUNC_MASK;
	msg->pcifunc |= (vf_idx + 1) & RVU_PFVF_FUNC_MASK;

So can a guest VF send GET_REP_CNT before the representor driver probes,
become rvu->rep_pcifunc, and then satisfy both new -EPERM checks?  From
there REP_EVENT_NOTIFY with req->pcifunc pointing at any other
CGX-mapped function passes is_pf_func_valid()/is_pf_cgxmapped() (those
only confirm the victim exists), and rvu_rep_up_notify() then does:

	if (event->event & RVU_EVENT_MAC_ADDR_CHANGE)
		ether_addr_copy(pfvf->mac_addr, event->evt_data.mac);

overwriting the victim's AF-side MAC and sending a forged up-notify to
the victim's PF.  is_rep_dev() is just a pcifunc comparison against
rvu->rep_pcifunc, so the same caller also gets representor treatment in
rvu_nix.c / rvu_npc_fs.c, and the real representor driver is locked out
afterwards because its GET_REP_CNT now returns -EPERM.  Should the
registration itself authenticate the representor rather than trusting
the first mailbox caller?

[ ... ]

>  int rvu_rep_notify_pfvf_state(struct rvu *rvu, u16 pcifunc, bool enable)
>  {
> -	struct rep_event *req;
> -	int pf;
> +	struct rep_event req = { 0 };
> +	struct msg_rsp rsp;
>  
>  	if (!is_pf_cgxmapped(rvu, rvu_get_pf(rvu->pdev, pcifunc)))
>  		return 0;
>  
> -	pf = rvu_get_pf(rvu->pdev, rvu->rep_pcifunc);
> -
> -	mutex_lock(&rvu->mbox_lock);
> -	req = otx2_mbox_alloc_msg_rep_event_up_notify(rvu, pf);

[ ... ]

> +	req.hdr.pcifunc = rvu->rep_pcifunc;
> +	req.event = RVU_EVENT_PFVF_STATE;
> +	req.pcifunc = pcifunc;
> +	req.evt_data.vf_state = enable;
> +	return rvu_mbox_handler_rep_event_notify(rvu, &req, &rsp);
>  }

[ ... ]

> @@ -334,19 +344,22 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
>  			continue;
>  
>  		pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
> +		pfvf = rvu_get_pfvf(rvu, pcifunc);
>  		rvu_get_nix_blkaddr(rvu, pcifunc);
> +		if (test_bit(NIXLF_INITIALIZED, &pfvf->flags))
> +			rvu_switch_enable_lbk_link(rvu, pcifunc, true);

[Severity: Medium]
Does moving rsrc_lock inside rvu_switch_enable_lbk_link() serialize
anything useful here?  The NIXLF_INITIALIZED test is outside the lock,
and the flag is set in rvu_mbox_handler_nix_lf_start_rx() and cleared in
rvu_mbox_handler_nix_lf_stop_rx()/rvu_nix_lf_teardown() with no lock at
all.  rvu_rep_install_mcam_rules() runs from
rvu_devlink_eswitch_mode_set() -> rvu_switch_enable(), which holds no
RVU lock:

CPU0 (devlink switchdev)              CPU1 (mbox NIX_LF_STOP_RX)
  test_bit(NIXLF_INITIALIZED) /* 1 */
                                      clear_bit(NIXLF_INITIALIZED);
                                      rvu_switch_update_rules(false)
                                        rvu_rep_update_rules()
                                          lbk_link(..., false); /* lock */
  rvu_switch_enable_lbk_link(true);
    /* lock, writes enable */

Doesn't that leave the TL2 LBK link on channel 63 enabled for a function
whose NIXLF was stopped or freed?  The new err_disable_lbk rollback and
the new rep_mode loop in rvu_switch_disable() can lose the same race
against a concurrent LF start.

[ ... ]

> @@ -366,30 +382,37 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
>  		}
>  	}
> +	return 0;
>  
> -	/* Initialize the wq for handling REP events */
> -	spin_lock_init(&rvu->rep_evtq_lock);
> -	INIT_LIST_HEAD(&rvu->rep_evtq_head);
> -	INIT_WORK(&rvu->rep_evt_work, rvu_rep_wq_handler);
> -	rvu->rep_evt_wq = alloc_workqueue("rep_evt_wq", WQ_PERCPU, 0);
> -	if (!rvu->rep_evt_wq) {
> -		dev_err(rvu->dev, "REP workqueue allocation failed\n");
> -		return -ENOMEM;
> +err_disable_lbk:
> +	/* Undo any LBK links enabled above before the MCAM rule failure.
> +	 * Disabling a link that was never enabled is a safe no-op.
> +	 */
> +	for (pf = 1; pf < hw->total_pfs; pf++) {
> +		if (!is_pf_cgxmapped(rvu, pf))
> +			continue;
> +		pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
> +		rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> +		rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
> +		for (vf = 0; vf < numvfs; vf++) {
> +			pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
> +			rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> +		}
>  	}
> -	return 0;
> +	return err;
>  }

[Severity: Medium]
Does this rollback miss the representor function itself?
rvu_rep_pf_init() enables the LBK link for rvu->rep_pcifunc:

	set_bit(NIXLF_INITIALIZED, &pfvf->flags);
	rvu_switch_enable_lbk_link(rvu, pcifunc, true);

but this loop, and the new rep_mode loop in rvu_switch_disable(), both
skip anything that is not CGX-mapped, and the representor is a separate
non-CGX-mapped function (PCI_DEVID_RVU_REP).  rvu_nix_tx_tl2_cfg()
explicitly allows the write for it:

	if (!is_pf_cgxmapped(rvu, pf) && !is_rep_dev(rvu, pcifunc))
		return;

So after a failed install, or after "devlink dev eswitch set mode
legacy", does the representor PF keep its TL2 nodes pointed at LBK
channel 63 with all matching NPC entries deleted?  Nothing re-evaluates
it either, since rvu_rep_pf_init() leaves NIXLF_INITIALIZED set on the
rep pfvf.

[Severity: High]
This isn't a bug introduced by this patch, but every rule-installation
failure now routes through this return err, so it may be worth fixing
alongside.  rvu_switch_enable()'s uninstall_rules label does
kfree(rswitch->entry2pcifunc) without clearing the pointer or resetting
rswitch->used_entries, and rvu_devlink_eswitch_mode_set() has already
committed rswitch->mode.  A following "devlink dev eswitch set mode
legacy" reaches rvu_switch_disable(), whose only guard is:

	if (!rswitch->used_entries)
		return;
	...
free_ents:
	...
	kfree(rswitch->entry2pcifunc);

which frees the same pointer a second time.  rvu_rep_update_rules() and
rvu_switch_update_rules(), reachable from NIX_LF_START_RX/STOP_RX
mailbox messages, are gated only on used_entries as well and then read
rswitch->entry2pcifunc[entry].  Should the failure path also clear
rswitch->entry2pcifunc and rswitch->used_entries?

[Severity: Medium]
This is a pre-existing issue, but does the new cleanup also need to
release TX VTAG definitions?  rvu_rep_install_tx_rule() calls
rvu_rep_tx_vlan_cfg() first, which allocates a NIX TX VTAG definition
entry and programs NIX_AF_TX_VTAG_DEFX_DATA/CTL, and only then calls
rvu_mbox_handler_npc_install_flow().  When the MCAM install fails the
returned vtag0_idx is dropped: err_disable_lbk only touches LBK links,
and rvu_switch_enable()'s uninstall_rules/free_entries labels only
delete NPC flows and free MCAM entries, while VTAG entries live in
nix_hw->txvlan and need nix_tx_vtag_free().  Do repeated failed
switchdev enables then consume the hardware VTAG definition table?

[ ... ]

> @@ -443,35 +466,92 @@ int rvu_mbox_handler_esw_cfg(struct rvu *rvu, struct esw_cfg_req *req,

[ ... ]

>  int rvu_mbox_handler_get_rep_cnt(struct rvu *rvu, struct msg_req *req,
>  				 struct get_rep_cnt_rsp *rsp)
>  {
> -	int pf, vf, numvfs, hwvf, rep = 0;
> +	int pf, vf, numvfs, hwvf, rep = 0, cnt;
> +	int ret = 0;
>  	u16 pcifunc;
> +	u16 *map;
> +
> +	/* Serialize first-time initialization. mbox_wq is WQ_PERCPU so
> +	 * handlers for different PFs can run concurrently; without this
> +	 * lock two callers could both observe rep2pfvf_map == NULL and
> +	 * double-allocate the workqueue, leaking one permanently.
> +	 */
> +	mutex_lock(&rvu->rsrc_lock);
> +
> +	if (rvu->rep2pfvf_map) {
> +		ret = rvu_rep_get_rep_map(rvu, req, rsp);
> +		goto unlock;
> +	}
>  
>  	rvu->rep_pcifunc = req->hdr.pcifunc;

[Severity: Low]
Is it intentional that rvu->rep_pcifunc is published before both -ENOMEM
exits below?  On either failure rvu->rep2pfvf_map stays NULL, so the
fast-path guard above does not fire for the next caller, which re-enters
this branch and overwrites rvu->rep_pcifunc.  Meanwhile the failed
caller already has is_rep_dev() true, and its pcifunc is consumed by
rvu_rep_install_rx_rule()/rvu_rep_install_tx_rule() (req.vf =
rvu->rep_pcifunc) and by rvu_rep_pf_init(), which sets
NIXLF_INITIALIZED and enables the LBK link for it.  Would assigning
rep_pcifunc only on the success path avoid that stale state?

> -	rsp->rep_cnt = rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs;
> -	rvu->rep_cnt = rsp->rep_cnt;
> +	cnt = min_t(int, rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs,
> +		    RVU_MAX_REP);

[Severity: High]
What happens on a part where rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs
exceeds RVU_MAX_REP?  The map is capped here, but the consumers are not:
rvu_switch_enable() still sizes the MCAM allocation from the uncapped
sum (times 4 in rep mode), and rvu_rep_install_mcam_rules() still loops
pf = 1..hw->total_pfs and vf = 0..numvfs with no bound tied to
rvu->rep_cnt.

Both rule installers take their VLAN/index from rvu_rep_get_vlan_id(),
which falls off its loop and returns the identifier of representor 0
when the pcifunc is not in the map:

	for (id = 0; id < rvu->rep_cnt; id++)
		if (rvu->rep2pfvf_map[id] == pcifunc)
			return id;
	return 0;

So for every function past the cap, does rvu_rep_install_rx_rule()
program a rule identical to representor 0's -

	vlan_tci = rep_id | BIT_ULL(8);
	req.vf = rvu->rep_pcifunc;
	req.index = rep_id;

- and steer that traffic to the wrong representor?  Their state events
are dropped on the peer side as well, since rvu_rep_get_repid() returns
-EINVAL and only the new warning fires.  Since numvfs is an 8-bit
per-PF field summed over all CGX PFs, 8 PFs with 8 VFs each already
passes 64.  Would rejecting such a configuration with a diagnostic here,
or applying the same cap in the rule installer and event producers, be
preferable to silent truncation?

[ ... ]

> +	/* Initialize the wq for handling REP events */
> +	spin_lock_init(&rvu->rep_evtq_lock);
> +	INIT_LIST_HEAD(&rvu->rep_evtq_head);
> +	INIT_WORK(&rvu->rep_evt_work, rvu_rep_wq_handler);
> +	rvu->rep_evt_wq = alloc_workqueue("rep_evt_wq", WQ_UNBOUND, 0);

[Severity: Medium]
The consumer side uses READ_ONCE(rvu->rep_evt_wq) as the gate for
touching rep_evtq_lock, rep_evtq_head and rep_evt_work, but the
publication here is a plain store with no smp_store_release() and the
reader has no acquire.  On a weakly ordered machine, can a consumer that
is not serialized with this function by rvu->mbox_lock - for example
rvu_mbox_handler_rep_event_notify() reached from an AF-VF mailbox work
item, since rvu_afvf_mbox_handler() does not take mbox_lock - see
rep_evt_wq non-NULL before the spin_lock_init()/INIT_LIST_HEAD()/
INIT_WORK() stores are visible, and then run spin_lock() and
list_add_tail() on uninitialized state?  The same ordering question
applies to rvu->rep_pcifunc, which now gates the -EPERM check.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
> index 49ce38685a7e..8c2c98403507 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
> @@ -12,11 +12,18 @@ void rvu_switch_enable_lbk_link(struct rvu *rvu, u16 pcifunc, bool enable)
>  {
>  	struct rvu_pfvf *pfvf = rvu_get_pfvf(rvu, pcifunc);
>  	struct nix_hw *nix_hw;
> +	int blkaddr;
>  
> -	nix_hw = get_nix_hw(rvu->hw, pfvf->nix_blkaddr);
> +	mutex_lock(&rvu->rsrc_lock);
> +	blkaddr = pfvf->nix_blkaddr;
> +	nix_hw = get_nix_hw(rvu->hw, blkaddr);
>  	/* Enable LBK links with channel 63 for TX MCAM rule */
> -	rvu_nix_tx_tl2_cfg(rvu, pfvf->nix_blkaddr, pcifunc,
> +	if (!nix_hw)
> +		goto unlock;
> +	rvu_nix_tx_tl2_cfg(rvu, blkaddr, pcifunc,
>  			   &nix_hw->txsch[NIX_TXSCH_LVL_TL2], enable);
> +unlock:
> +	mutex_unlock(&rvu->rsrc_lock);
>  }

[ ... ]

> @@ -229,8 +236,20 @@ void rvu_switch_disable(struct rvu *rvu)
>  	if (!rswitch->used_entries)
>  		return;
>  
> -	if (rvu->rep_mode)
> +	if (rvu->rep_mode) {
> +		for (pf = 1; pf < hw->total_pfs; pf++) {
> +			if (!is_pf_cgxmapped(rvu, pf))
> +				continue;
> +			pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
> +			rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> +			rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
> +			for (vf = 0; vf < numvfs; vf++) {
> +				pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
> +				rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> +			}
> +		}
>  		goto free_ents;
> +	}

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> index 0f5d5642d3f7..ef47e7e21901 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c

[ ... ]

> @@ -459,6 +465,9 @@ static int rvu_rep_open(struct net_device *dev)
>  	netif_carrier_on(dev);
>  	netif_tx_start_all_queues(dev);
>  
> +	if (rep->pcifunc & RVU_PFVF_FUNC_MASK)
> +		return 0;
> +
>  	evt.event = RVU_EVENT_PORT_STATE;
>  	evt.evt_data.port_state = 1;
>  	evt.pcifunc = rep->pcifunc;

[Severity: Medium]
These two early returns in rvu_rep_open() and rvu_rep_stop() skip the
RVU_EVENT_PORT_STATE notification for every VF representor, which is the
representor-to-representee link state propagation added by the commit in
the Fixes: tag (b8fea84a0468).  After this, does "ip link set <vf-rep>
up/down" still affect the representee?

The reason the notification does not work today looks like the missing
handler on the VF side - otx2vf_process_mbox_msg_up() only handles
MBOX_MSG_CGX_LINK_EVENT and answers everything else with:

	default:
		otx2_reply_invalid_msg(&vf->mbox.mbox_up, 0, 0, req->id);
		return -ENODEV;

while the PF side does implement
otx2_mbox_up_handler_rep_event_up_notify().  Would adding
MBOX_MSG_REP_EVENT_UP_NOTIFY to otx2vf_process_mbox_msg_up() be the fix
instead?  Neither the commit message nor the changelog mentions these
two hunks.

> @@ -478,6 +487,9 @@ static int rvu_rep_stop(struct net_device *dev)
>  	netif_carrier_off(dev);
>  	netif_tx_disable(dev);
>  
> +	if (rep->pcifunc & RVU_PFVF_FUNC_MASK)
> +		return 0;
> +
>  	evt.event = RVU_EVENT_PORT_STATE;
>  	evt.pcifunc = rep->pcifunc;
>  	rvu_rep_notify_pfvf(priv, RVU_EVENT_PORT_STATE, &evt);

Thanks for looking at these.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903074840.2373506-1-nshettyj%40marvell.com