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

[Wei Fang]

> > @@ -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.

The VF is being disabled, no need to notify the link status.

> 
> 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?

For the -EIO and timeout cases, both can only mean the VF is not consuming
its mailbox messages.

-EIO is returned only when the VF's MS bit is still set, i.e. it has not read the
previous PSI-to-VSI message, and the 200 ms timeout means it did not read
the new one within that window. Note that at registration time the VF's MS
bit should be clear: the VF's bit is not in link_status_ms_mask before it
registers, so the PF has never sent it an asynchronous notification. A healthy
VF therefore cannot hit -EIO on this path - a set MS bit at that point implies
either a stale unread message from a previous driver instance or a VF that is
stuck.

So in these cases the VF is already malfunctioning. If it does not process
mailbox messages at all, it cannot work correctly regardless of the carrier
state, and I don't think the PF should try to compensate for that. No change
is needed here.

> 
> > +
> > +		dev_warn(&ndev->dev,
> > +			 "Link status notifier registration failed\n");
> > +
> > +carrier_on:
> >  		netif_carrier_on(ndev);
> >  		return 0;
> >  	}
> 
> [ ... ]
> > @@ -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?

The ordering mismatch is real: if enetc_msg_vsi_send() hits -ETIME or -EIO, the
PF may have registered the VF while the VF-side
ENETC_LINK_STATUS_NOTIFIER_REGISTERED flag stays clear, so no UNREGISTER
is sent. But the consequences are much smaller than described.

After enetc_close() the VF still consumes and acknowledges PF messages. The
MR interrupt is enabled in enetc_vf_probe() and only disabled in enetc_vf_remove(),
not on close, and enetc_vf_msg_task() does not check the REGISTERED flag
- it always reads VSIMSGRR, which acknowledges the message and clears the
MS bit. So the PF's enetc_pf_send_msg() gets its acknowledgement promptly
and does not pay the 200 ms poll, and mailbox service for the other VFs is not
affected. enetc_vf_msg_handle_link_status() also bails out early when the netdev
is not running, so a stale subscription after close is harmless.

A leftover subscription only matters after enetc_vf_remove(), once the MR
interrupt is disabled and the VF no longer reads its mailbox. That is the same
situation as the earlier "VF goes away without unregistering" case, and it will
be mitigated by the follow-up change that removes the 200 ms synchronous
wait in enetc_pf_send_msg(): with a fire-and-forget notification, a VF that
never reads its message can no longer block the PF, since the pre-send MS
check simply skips it.

As for clearing link_status_ms_mask on FLR, I don't think that is right, for the
same reason as before: it is PF-side software state tracking which VFs
subscribed, and a VF can legitimately go through an FLR while remaining a
registered subscriber. Clearing it there would stop delivering link updates to
a VF that is still alive and still subscribed.

> > +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.

rtnl_lock() is not redundant here - it protects the netif_running() check.

> > +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?

This is the same underlying situation as the earlier link-status notification
question, and I don't think it needs a fix.

To lose a transition, the VF must fail to read VSIMSGRR within the PF's 200
ms poll window. On receiving the MR interrupt the VF only masks MRIE and
queues a work item, so under normal scheduling the acknowledgement
happens far sooner than 200 ms. A VF that cannot complete that within
200 ms is already malfunctioning - for example a stalled or suspended
guest - and I don't think the PF should be expected to compensate for that.

The link change cadence also makes this hard to hit: the PHY state machine
polls link status once per second, so consecutive transitions are normally at
least ~1 s apart, which is ample time for a healthy VF to consume the previous
message and clear its MS bit. In real usage link transitions are infrequent to
begin with.

So no change is needed here.

> >  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.

I think this is a theoretical concern rather than a practical one.

enetc_vf_enable_mr_int() and netif_carrier_off() are adjacent
statements in the same execution path, so the interval between
them is just a register write returning. The interrupt handler, on
the other hand, only masks MRIE and calls queue_work() - the
carrier is not touched there. Before netif_carrier_on() can run,
the work item has to be woken and scheduled, read VSIMSGRR,
acquire rtnl_lock and pass the netif_running() check. That chain
is considerably longer than the gap between those two adjacent
statements.