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

[netdev-bot+sashiko]

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 revocation after a VF FLR is fully
  asynchronous: enetc_msg_psi_msix() (enetc_msg.c:36-45) masks PSIIER…

--

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

On ENETC v4 a VF FLR resets PSIPMMR[SIn_MAC_UP] and PSIPMMR[SIn_MAC_MP]
to their default, which enables unicast and multicast promiscuous mode
for that SI.  The patch tracks the PF-managed promiscuous state in
enetc_vf_flags and adds a vf_flr_handler callback that re-applies it from
enetc_msg_task() after each FLR.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> index a4ffe1100bd7..c421c0e7355b 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);
> +
> +	mutex_lock(&pf->msg_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);
> +	mutex_unlock(&pf->msg_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 4aabeb23a386..55c23d4a73a8 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);
>  }

[ ... ]

> @@ -594,6 +611,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);
> @@ -602,6 +642,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 state before this
restore runs?

The hardware puts the SI into UC/MC promiscuous mode at the instant of the
FLR, but the policy is only re-applied from the work item:

enetc_msg_psi_msix() {
	enetc_disable_psiier_interrupts(pf);
	schedule_work(&si->msg_task);

	return IRQ_HANDLED;
}

enetc_msg_task() -> enetc_vf_flr_handler() -> pf->ops->vf_flr_handler()
	-> enetc4_pf_vf_flr_handler()

Since the guest owning the VF triggers the FLR itself (vfio-pci reset, or a
driver bind path reaching pcie_flr()), it knows exactly when that window
opens and only needs to re-arm an Rx BD ring to receive frames destined for
other SIs until enetc4_pf_vf_flr_handler() clears the bits in PSIPMMR.

The scan also happens once, at the top of enetc_msg_task():

	enetc_vf_flr_handler(pf);

	mr_mask = ENETC_PSIMR_MASK(pf->num_vfs);

and enetc_vf_flr_handler() takes a single PSIIDR snapshot with an early
return:

	flr_status = enetc_rd(hw, ENETC_PSIIDR) & flr_mask;
	if (!flr_status)
		return;

If an FLR lands while msg_task is already part-way through the VF message
loop, is the restore then delayed until the whole in-flight batch finishes
and the re-queued work runs?  The FLR bits are not re-checked before
enetc_enable_psiier_interrupts(pf) at the end of the work item.

Would it be feasible to clear PSIPMMR[SIn_MAC_UP]/[SIn_MAC_MP] directly in
enetc_msg_psi_msix() for the VFs whose FLR bits are set, and leave the full
policy re-apply in the work item?  As written the handler sleeps on
vf_state->lock and pf->msg_lock, so it cannot run from the hardirq.

[ ... ]

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