Re: [PATCH v4 net-next 11/15] net: enetc: restore VF MAC promiscuous mode after FLR for ENETC v4

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The promiscuous-mode restoration is deferred to a workqueue:
  enetc_msg_psi_msix() only calls enetc_disable_psiier_interrupts() +…

--

net: enetc: restore VF MAC promiscuous mode after FLR for ENETC v4

A VF FLR on ENETC v4 resets PSIPMMR[SIn_MAC_UP] and PSIPMMR[SIn_MAC_MP]
to their promiscuous default, so the patch tracks the PF-managed UC/MC
promiscuous state per VF and re-applies it from a new vf_flr_handler
callback dispatched out of enetc_msg_task().

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> index abe1e8dafe24..6a9b1e079ae2 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> @@ -212,9 +212,29 @@ static void enetc4_pf_set_mac_filter(struct enetc_pf *pf, int type,
>  		enetc4_pf_set_mc_hash_filter(pf, mc);
>  }
>  
> +static void enetc4_pf_vf_flr_handler(struct enetc_pf *pf, int vf_id)
> +{
> +	struct enetc_vf_state *vf_state;
> +	bool uc_promisc, mc_promisc;
> +
> +	vf_state = &pf->vf_state[vf_id];
> +	mutex_lock(&vf_state->lock);
> +
> +	uc_promisc = !!(vf_state->flags & ENETC_VF_FLAG_UC_PROMISC);
> +	mc_promisc = !!(vf_state->flags & ENETC_VF_FLAG_MC_PROMISC);
> +
> +	spin_lock(&pf->si->gen_lock);
> +	enetc_set_si_uc_promisc(pf->si, vf_id + 1, uc_promisc);
> +	enetc_set_si_mc_promisc(pf->si, vf_id + 1, mc_promisc);
> +	spin_unlock(&pf->si->gen_lock);
> +
> +	mutex_unlock(&vf_state->lock);
> +}
> +
>  static const struct enetc_pf_ops enetc4_pf_ops = {
>  	.set_si_primary_mac = enetc4_pf_set_si_primary_mac,
>  	.get_si_primary_mac = enetc4_pf_get_si_primary_mac,
> +	.vf_flr_handler = enetc4_pf_vf_flr_handler,
>  };

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> index aa37a0123274..ff99937815c0 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> @@ -27,6 +27,9 @@ static void enetc_enable_psiier_interrupts(struct enetc_pf *pf)
>  	u32 psiier = ENETC_PSIMR_MASK(pf->num_vfs);
>  	struct enetc_hw *hw = &pf->si->hw;
>  
> +	if (pf->ops->vf_flr_handler)
> +		psiier |= ENETC_VFFLR_MASK(pf->num_vfs);
> +
>  	enetc_wr(hw, ENETC_PSIIER, psiier);
>  }

[ ... ]

> @@ -542,6 +559,29 @@ static void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int vf_id,
>  	kfree(msg);
>  }
>  
> +static void enetc_vf_flr_handler(struct enetc_pf *pf)
> +{
> +	u32 flr_mask = ENETC_VFFLR_MASK(pf->num_vfs);
> +	struct enetc_hw *hw = &pf->si->hw;
> +	u32 flr_status;
> +
> +	if (!pf->ops->vf_flr_handler)
> +		return;
> +
> +	flr_status = enetc_rd(hw, ENETC_PSIIDR) & flr_mask;
> +	if (!flr_status)
> +		return;
> +
> +	for (int i = 0; i < pf->num_vfs; i++) {
> +		if (!(ENETC_VFFLR_BIT(i) & flr_status))
> +			continue;
> +
> +		/* Clear FLR interrupt status, W1C */
> +		enetc_wr(hw, ENETC_PSIIDR, ENETC_VFFLR_BIT(i));
> +		pf->ops->vf_flr_handler(pf, i);
> +	}
> +}
> +
>  static void enetc_msg_task(struct work_struct *work)
>  {
>  	struct enetc_si *si = container_of(work, struct enetc_si, msg_task);
> @@ -550,6 +590,8 @@ static void enetc_msg_task(struct work_struct *work)
>  	u32 mr_status, mr_mask;
>  	int i;
>  
> +	enetc_vf_flr_handler(pf);
> +

[Severity: Medium]
How long can the SI stay in the hardware promiscuous default before this
restore runs?

The interrupt handler only masks PSIIER and queues the work:

drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_psi_msix() {
	enetc_disable_psiier_interrupts(pf);
	schedule_work(&si->msg_task);
}

so between the FLR taking effect (PSIPMMR[SIn_MAC_UP]/[SIn_MAC_MP] back at
their promiscuous reset values) and enetc_msg_task() ->
enetc_vf_flr_handler() -> enetc4_pf_vf_flr_handler() running, the SI keeps
receiving all port unicast and multicast traffic. Since the hardware default
ignores trust state, does this window also apply to an untrusted VF that has
never been granted promiscuous mode?

Is the window bounded here? PSIIER is only re-armed at the very end of
enetc_msg_task():

	for (i = 0; i < pf->num_vfs; i++) {
		...
		enetc_msg_handle_rxmsg(pf, i, &msg_code);
		...
	}

out:
	enetc_enable_psiier_interrupts(pf);

so an FLR latched while the message loop is in progress is not serviced until
that loop finishes. For a trusted VF the loop can reach
enetc_msg_get_link_speed(), which blocks on the global lock:

	rtnl_lock();
	phylink_ethtool_ksettings_get(priv->phylink, &link_info);
	rtnl_unlock();

Can a guest keep msg_task busy this way while another VF (or the same one)
sits in the FLR-induced promiscuous state?

Would it make sense to force UC/MC promiscuous off for the affected SI as
soon as the FLR event is observed, before the tracked policy is re-applied,
and to note the residual window in the commit message?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909100733.1139689-1-wei.fang%40oss.nxp.com