Re: [PATCH v3 net-next 14/15] net: enetc: add PSI-to-VSI link status notification support for VF

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The PSI-to-VSI link-status notification protocol has no
  coalescing, retry or re-query path, so the newest link state can be…
- [Medium] The VF treats a CMD_SUCCESS reply to
  ENETC_MSG_REGISTER_LINK_CHANGE_NOTIFIER as a guarantee that the PF…
- [Medium] The PF-side link-status subscription (bit in
  pf->link_status_ms_mask) is acquired with no guaranteed release, and…
- [Medium] Teardown ordering bug: enetc_vf_remove()
  (drivers/net/ethernet/freescale/enetc/enetc_vf.c:773-774) calls…
- [Medium] Head-of-line blocking / lock inversion between rtnl_lock and
  the VSI-PSI mailbox.
- [Medium] enetc_vf_probe() enables the message-received interrupt and
  then calls netif_carrier_off(ndev) *after* register_netdev() with no…
- [Low] Claim: the commit message omits that
  enetc_vf_msg_handle_link_status() also reprograms hardware…

--

net: enetc: add PSI-to-VSI link status notification support for VF

This adds two enetc_si_ops hooks so that a phy-less ENETC v4 VF registers a
link status notifier with the PF over the VSI-to-PSI mailbox on open and
unregisters it on close. Incoming PSI-to-VSI notifications are taken by a
dedicated MSI-X vector and processed in a work item that updates the VF
carrier.

I had a few questions about the new paths below.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 803c5c541a5c..fb5df740650e 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -2935,11 +2935,31 @@ static void enetc_clear_interrupts(struct enetc_ndev_priv *priv)
>  static int enetc_phylink_connect(struct net_device *ndev)
>  {
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> +	struct enetc_si *si = priv->si;
>  	struct ethtool_keee edata;
>  	int err;
>  
>  	if (!priv->phylink) {
>  		/* phy-less mode */
> +		if (!si->ops->vf_reg_link_status_notifier)
> +			goto carrier_on;
> +
> +		/* For phy-less VFs on ENETC v4, attempt to register a link
> +		 * status notifier with the PF via the VSI-to-PSI messaging
> +		 * channel. If registration succeeds, the PF will immediately
> +		 * send the current link status and broadcast future link
> +		 * transitions; carrier state is then managed in
> +		 * enetc_vf_msg_handle_link_status(). If registration fails,
> +		 * fall back to the LS1028A behaviour and assert carrier
> +		 * unconditionally via netif_carrier_on().
> +		 */
> +		if (!si->ops->vf_reg_link_status_notifier(si))
> +			return 0;

[Severity: Medium]

Here a CMD_SUCCESS reply to ENETC_MSG_REGISTER_LINK_CHANGE_NOTIFIER is
taken as a promise that the PF registered the VF and will push the current
link status, so this path returns without calling netif_carrier_on() and
without any timeout fallback.

Does the PF side keep that promise? enetc_msg_register_link_status_notifier()
writes the SUCCESS reply first and only then checks sriov_enabled:

	enetc_pf_reply_msg(&pf->si->hw, vf_id, pf_msg);

	/* SR-IOV is being disabled if pf->sriov_enabled is false, so no
	 * need to set link_status_ms_mask and notify the link status.
	 */
	if (!pf->sriov_enabled)
		goto msg_unlock;

so both pf->link_status_ms_mask |= PSIMSGSR_MS(vf_id) and the immediate
enetc_msg_notify_vf_link_status() are skipped after the VF has already been
told SUCCESS.

That window is entered by enetc_sriov_configure(pdev, 0) (echo 0 >
sriov_numvfs) and by the pci_enable_sriov() error path, both before the VFs
are torn down.

Even with sriov_enabled true, the immediate notification can fail inside
enetc_pf_send_msg() (-EIO when the VF MS bit is still set, or the 200 ms
timeout) and is only reported with dev_err_ratelimited("PF notifies link
status failed").

In these cases, is the VF left administratively up with carrier off, with no
traffic, until a manual down/up?

> +
> +		dev_warn(&ndev->dev,
> +			 "Link status notifier registration failed\n");
> +
> +carrier_on:
>  		netif_carrier_on(ndev);
>  		return 0;
>  	}

[ ... ]

> @@ -3051,8 +3072,15 @@ int enetc_open(struct net_device *ndev)
>  err_alloc_rx:
>  	enetc_free_tx_resources(tx_res, priv->num_tx_rings);
>  err_alloc_tx:
> -	if (priv->phylink)
> +	if (priv->phylink) {
>  		phylink_disconnect_phy(priv->phylink);
> +	} else if (si->ops->vf_unreg_link_status_notifier &&
> +		   test_bit(ENETC_LINK_STATUS_NOTIFIER_REGISTERED,
> +			    &priv->flags)) {
> +		if (si->ops->vf_unreg_link_status_notifier(si))
> +			dev_warn(&ndev->dev,
> +				 "Link status notifier unregistration failed\n");
> +	}
>  err_phy_connect:
>  	enetc_free_irqs(priv);
>  err_setup_irqs:

[ ... ]

> @@ -3100,6 +3129,17 @@ int enetc_close(struct net_device *ndev)
>  		phylink_stop(priv->phylink);
>  		phylink_disconnect_phy(priv->phylink);
>  	} else {
> +		if (!si->ops->vf_unreg_link_status_notifier ||
> +		    !test_bit(ENETC_LINK_STATUS_NOTIFIER_REGISTERED,
> +			      &priv->flags))
> +			goto carrier_off;
> +
> +		if (!si->ops->vf_unreg_link_status_notifier(si))
> +			goto carrier_off;
> +
> +		dev_warn(&ndev->dev,
> +			 "Link status notifier unregistration failed\n");
> +carrier_off:
>  		netif_carrier_off(ndev);
>  	}

[Severity: Medium]

Both new unregister call sites, here and in the enetc_open() unwind, are
gated on ENETC_LINK_STATUS_NOTIFIER_REGISTERED, but that flag is only set
when the whole mailbox round trip returned 0:

enetc_vf_reg_link_status_notifier()
	err = enetc_msg_link_status_notifier(si, true);
	if (!err)
		set_bit(ENETC_LINK_STATUS_NOTIFIER_REGISTERED, &priv->flags);

On the PF side, enetc_msg_register_link_status_notifier() replies first and
sets pf->link_status_ms_mask |= PSIMSGSR_MS(vf_id) afterwards.

So if enetc_msg_vsi_send() hits its 200 ms read_poll_timeout (-ETIME) or
-EIO, the PF is registered while the VF flag stays clear, and this code
takes the goto carrier_off path so UNREGISTER is never sent. Can the
PF-side subscription be leaked this way?

A failed unregister is likewise only warned about and leaves the flag set,
and enetc_vf_remove() relies solely on unregister_netdev() -> enetc_close(),
so after close or unbind the PF keeps notifying a VF that can no longer
acknowledge, paying the full 200 ms poll under pf->msg_lock which also
serialises mailbox service for the other VFs.

enetc4_pf_vf_flr_handler() restores promisc state but does not clear the
reset VF's bit in link_status_ms_mask either. Would a forced unregister in
enetc_vf_remove(), plus clearing link_status_ms_mask on FLR, be appropriate?

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> index 4e717afba7f7..a4d0089ef1a9 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c

[ ... ]

> @@ -435,6 +481,128 @@ static void enetc_vf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
>  	enetc_load_primary_mac_addr(&si->hw, ndev);
>  }
>  
> +static void enetc_vf_enable_mr_int(struct enetc_si *si)
> +{
> +	if (is_enetc_rev1(si))
> +		return;
> +
> +	enetc_wr(&si->hw, ENETC_VSIIER, VSIIER_MRIE);
> +}
> +
> +static void enetc_vf_disable_mr_int(struct enetc_si *si)
> +{
> +	if (is_enetc_rev1(si))
> +		return;
> +
> +	enetc_wr(&si->hw, ENETC_VSIIER, 0);
> +}
> +
> +static void enetc_vf_msg_handle_link_status(struct enetc_si *si, u8 status)
> +{
> +	bool tx_pause = !!(status & ENETC_CLASS_CODE_TX_PAUSE_EN);
> +	bool link_down = !!(status & ENETC_CLASS_CODE_LINK_DOWN);
> +	struct enetc_ndev_priv *priv = netdev_priv(si->ndev);
> +	struct net_device *ndev = si->ndev;
> +
> +	rtnl_lock();

[Severity: Medium]

Is rtnl_lock() needed here? netif_carrier_on()/netif_carrier_off() do not
require it, and enetc_set_congestion_mode() already serialises on
si->gen_lock.

Taking rtnl in this work item ties PF message acknowledgement to rtnl
availability. Every VSI-to-PSI sender runs with rtnl held and busy-polls the
PF for up to 200 ms inside enetc_msg_vsi_send(): the two new notifier ops
called from enetc_phylink_connect() and enetc_close(), plus
enetc_vf_set_mac_addr() and the rx-mode async worker.

There is a single si->msg_task on an ordered workqueue and MRIE stays masked
from enetc_vf_msg_msix_handler() until it completes, so while one invocation
is parked on rtnl_lock() no further PF message can be acked:

VF work item                        VF rtnl holder (ip link set <vf> down)
enetc_vf_msg_task()
  enetc_vf_msg_handle_link_status()
    rtnl_lock()  <- blocks          enetc_close()
                                      vf_unreg_link_status_notifier()
                                        enetc_msg_vsi_send() 200 ms poll

The PF in turn holds pf->msg_lock while polling for that same VF's ack.

Does this resolve only when the 200 ms timeouts fire, leaving "VSI mailbox
timeout" and "Link status notifier unregistration failed", a leaked PF
registration and a lost link transition?

> +	if (!netif_running(ndev))
> +		goto unlock_rtnl;
> +
> +	if (link_down) {
> +		if (netif_carrier_ok(ndev)) {
> +			netif_carrier_off(ndev);
> +			netdev_info(ndev, "Link is Down\n");
> +		}
> +
> +		goto unlock_rtnl;
> +	}
> +
> +	/* Link is up */
> +	enetc_set_congestion_mode(priv, tx_pause);

[Severity: Low]

This isn't a bug, but the commit message says only that the work item
"parses the notification and updates the carrier state via
netif_carrier_on() or netif_carrier_off() accordingly". It does not mention
that the same notification reprograms hardware, updating the congestion
mode bit in ENETC_RBMR for every VF Rx BD ring from the PF TX pause bit.
Could that be spelled out in the changelog?

> +
> +	if (!netif_carrier_ok(ndev)) {
> +		netif_carrier_on(ndev);
> +		netdev_info(ndev, "Link is Up, tx pause %s\n",
> +			    tx_pause ? "on" : "off");
> +	}
> +
> +unlock_rtnl:
> +	rtnl_unlock();
> +}
> +
> +static void enetc_vf_msg_task(struct work_struct *work)
> +{
> +	struct enetc_si *si = container_of(work, struct enetc_si, msg_task);
> +	struct enetc_hw *hw = &si->hw;
> +	u8 class_id, class_code;
> +	u16 pf_msg;
> +
> +	/* W1C to clear the message received interrupt event */
> +	enetc_wr(hw, ENETC_VSIIDR, VSIIDR_MR);
> +
> +	/* Reading VSIMSGRR retrieves the message data and acknowledges to
> +	 * the PF that the message was received and another message can be
> +	 * sent.
> +	 */
> +	pf_msg = FIELD_GET(VSIMSGRR_MC, enetc_rd(hw, ENETC_VSIMSGRR));

[Severity: Medium]

Only the single 16-bit value VSIMSGRR currently holds is ever consumed, and
the VF never sends ENETC_MSG_GET_CURRENT_LINK_STATUS even though the
protocol defines it and the PF already handles it.

On the PF side, enetc_pf_send_msg() drops every VF whose MS bit is still
set:

	ms_status = enetc_rd(hw, ENETC_PSIMSGSR) & 0xffff;
	ms_mask &= ~ms_status;
	if (!ms_mask)
		return -EIO;

and enetc_msg_notify_vf_link_status() only logs "PF notifies link status
failed" without storing the pending state or retrying once the MS bit
clears.

If the VF has not read VSIMSGRR within the PF's 200 ms poll, the ack being
deferred to this work item with MRIE masked meanwhile, and the link then
changes again, the second transition is skipped for that VF and never
resent.

Does the VF carrier then stay at the older state, including carrier off
which blocks all VF transmit, until some later PF transition happens to
succeed?

> +	class_id = FIELD_GET(ENETC_PF_MSG_CLASS_ID, pf_msg);
> +
> +	switch (class_id) {
> +	case ENETC_MSG_CLASS_ID_LINK_STATUS:
> +		class_code = FIELD_GET(ENETC_PF_MSG_CLASS_CODE_U8, pf_msg);
> +		enetc_vf_msg_handle_link_status(si, class_code);
> +		break;
> +	default:
> +		dev_err(&si->pdev->dev,
> +			"Unsupported Message Class ID (0x%02x) from PF\n",
> +			class_id);
> +	}
> +
> +	enetc_vf_enable_mr_int(si);
> +}
> +
> +static irqreturn_t enetc_vf_msg_msix_handler(int irq, void *data)
> +{
> +	struct enetc_si *si = (struct enetc_si *)data;
> +
> +	enetc_vf_disable_mr_int(si);
> +	queue_work(si->workqueue, &si->msg_task);
> +
> +	return IRQ_HANDLED;
> +}
> +

[ ... ]

> @@ -447,8 +615,38 @@ static const struct enetc_si_ops enetc4_vsi_ops = {

[ ... ]

> +static void enetc_vf_wq_task_destroy(struct enetc_si *si)
> +{
> +	if (!si->workqueue)
> +		return;
> +
> +	disable_work_sync(&si->msg_task);
> +
> +	/* The MR interrupt may be re-enabled by si->msg_task */
> +	enetc_vf_disable_mr_int(si);
> +
> +	destroy_workqueue(si->workqueue);
> +}
> +
>  static int enetc_vf_probe(struct pci_dev *pdev,
>  			  const struct pci_device_id *ent)
>  {
> @@ -520,15 +718,33 @@ static int enetc_vf_probe(struct pci_dev *pdev,
>  		goto err_alloc_msix;
>  	}
>  
> +	err = enetc_vf_wq_task_init(si);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to init workqueue\n");
> +		goto err_wq_init;
> +	}
> +
> +	err = enetc_vf_register_msg_msix(si);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to register msg irq\n");
> +		goto err_register_msg_msix;
> +	}
> +
>  	err = register_netdev(ndev);
>  	if (err)
>  		goto err_reg_netdev;
>  
> +	/* Enable message received interrupt */
> +	enetc_vf_enable_mr_int(si);
>  	netif_carrier_off(ndev);

[Severity: Medium]

This netif_carrier_off() runs after register_netdev() has published the
interface and dropped rtnl, with no lock held, while the new work item
writes the same carrier bit under rtnl. If the probe thread is preempted
after register_netdev(), can this happen?

  probe                            udev / ifup
  register_netdev()
                                   enetc_open()
                                     enetc_phylink_connect()
                                       vf_reg_link_status_notifier()
                                   PF sends current link status
                                   enetc_vf_msg_task()
                                     netif_carrier_on()  (rtnl held)
  netif_carrier_off()   <- reverts it

Since the carrier is now driven only by PF transition notifications, with no
periodic refresh and no VF-side status query, the interface would stay up
with carrier off until the next physical PF link change.

Would moving netif_carrier_off() and enetc_vf_enable_mr_int() before
register_netdev() avoid this? The netif_running() check in
enetc_vf_msg_handle_link_status() should make the earlier interrupt enable
harmless.

>  
>  	return 0;
>  
>  err_reg_netdev:
> +	enetc_vf_free_msg_msix(si);
> +err_register_msg_msix:
> +	enetc_vf_wq_task_destroy(si);
> +err_wq_init:
>  	enetc_free_msix(priv);
>  err_config_si:
>  err_alloc_msix:
> @@ -554,8 +770,11 @@ static void enetc_vf_remove(struct pci_dev *pdev)
>  	struct enetc_msg_swbd msg;
>  
>  	priv = netdev_priv(si->ndev);
> +	enetc_vf_disable_mr_int(si);
>  	unregister_netdev(si->ndev);

[Severity: Medium]

Is this ordering intentional? enetc_vf_disable_mr_int() masks VSIIER while
the VF is still present in pf->link_status_ms_mask, because the
UNREGISTER_LINK_CHANGE_NOTIFIER message is only sent later from
enetc_close(), reached via unregister_netdev() -> dev_close().

A PF link transition landing in that window writes PSIMSGSR and polls up to
200 ms under pf->msg_lock, but nothing can read ENETC_VSIMSGRR any more:
the MR interrupt is masked, and enetc_vf_wq_task_destroy() then calls
disable_work_sync() and masks MRIE again, so enetc_vf_msg_task() never runs.
Per enetc_pf_send_msg()'s own comment, reading VSIMSGRR is the only way to
clear the MS bit, "the MS bit can only be written as 1, writing a 0 has no
effect".

Does the MS bit then stay asserted across an unbind, so that on the next
probe and open enetc_pf_send_msg()

	ms_mask &= ~ms_status;
	if (!ms_mask)
		return -EIO;

drops the initial link status notification for the freshly registered VF,
whose carrier stays off because enetc_phylink_connect() skips
netif_carrier_on() on registration success?

The early disable also looks redundant, since enetc_vf_wq_task_destroy()
already performs the final MRIE disable after disable_work_sync().

>  
> +	enetc_vf_free_msg_msix(si);
> +	enetc_vf_wq_task_destroy(si);
>  	enetc_free_msix(priv);
>  
>  	enetc_free_si_resources(priv);

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