* [PATCH net-next 1/6] net: add netif_rx_mode_schedule_fresh()
2026-09-21 19:59 [PATCH net-next 0/6] enic: configure V2 VF addresses and receive mode Satish Kharat
@ 2026-09-21 19:59 ` Satish Kharat
2026-09-24 23:01 ` netdev-bot+sashiko
2026-09-21 19:59 ` [PATCH net-next 2/6] enic: serialize V2 VF mailbox requests Satish Kharat
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Satish Kharat @ 2026-09-21 19:59 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman
Cc: netdev, linux-kernel, Satish Kharat, sebaddel
Add a receive-mode scheduling helper for callers that need to replay
the complete configuration after a state transition.
Cancel a pending retry before queueing the immediate update
so stale retry timers cannot consume the new operation's retry
budget. Resetting the backoff also gives the fresh replay its full
bounded retry sequence.
Assisted-by: LLM
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
include/linux/netdevice.h | 1 +
net/core/dev_addr_lists.c | 21 +++++++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5d16737167ee..2246be72d811 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -5197,6 +5197,7 @@ static inline void __dev_mc_unsync(struct net_device *dev,
/* Functions used for secondary unicast and multicast support */
void dev_set_rx_mode(struct net_device *dev);
+void netif_rx_mode_schedule_fresh(struct net_device *dev);
void netif_rx_mode_schedule_retry(struct net_device *dev);
int netif_set_promiscuity(struct net_device *dev, int inc);
int dev_set_promiscuity(struct net_device *dev, int inc);
diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 08528ca0a8b3..4ae0510ce91f 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -1337,6 +1337,27 @@ static void netif_rx_mode_queue(struct net_device *dev)
__netdev_work_core_sched(dev, NETDEV_WORK_RX_MODE);
}
+/**
+ * netif_rx_mode_schedule_fresh() - schedule a fresh receive-mode update
+ * @dev: network device
+ *
+ * Cancel any pending retry and reset its backoff budget before scheduling an
+ * immediate receive-mode update. Callers use this after a state transition
+ * which requires the complete receive configuration to be replayed, rather
+ * than after a failed update (which uses netif_rx_mode_schedule_retry()).
+ *
+ * Context: sleepable. The caller must hold the device operations lock, or
+ * RTNL for a device which still uses RTNL-compatible operations.
+ */
+void netif_rx_mode_schedule_fresh(struct net_device *dev)
+{
+ might_sleep();
+ netdev_assert_locked_ops_compat(dev);
+ netif_rx_mode_cancel_retry(dev);
+ netif_rx_mode_queue(dev);
+}
+EXPORT_SYMBOL_GPL(netif_rx_mode_schedule_fresh);
+
static void netif_rx_mode_retry(struct timer_list *t)
{
struct net_device *dev =
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 1/6] net: add netif_rx_mode_schedule_fresh()
2026-09-21 19:59 ` [PATCH net-next 1/6] net: add netif_rx_mode_schedule_fresh() Satish Kharat
@ 2026-09-24 23:01 ` netdev-bot+sashiko
0 siblings, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 23:01 UTC (permalink / raw)
To: satishkh
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, netdev,
linux-kernel, sebaddel
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] Claimed dead code: netif_rx_mode_schedule_fresh() is added and
EXPORT_SYMBOL_GPL()'d with no in-tree caller in this patch.
- [Low] Claim that netif_rx_mode_schedule_fresh() cannot deliver the
documented 'complete configuration replay' for ndo_set_rx_mode_async…
Pre-existing issues:
- [High] dev->rx_mode_retry_timer is initialized for the whole net_device
lifetime in netif_rx_mode_init() (net/core/dev_addr_lists.c:1369) but…
--
net: add netif_rx_mode_schedule_fresh()
This adds a new exported helper that cancels any pending receive-mode retry
timer, resets the retry backoff counter, and then queues an immediate
receive-mode update via the netdev work core. It is meant for callers that
need to replay the full receive configuration after a state transition,
as opposed to netif_rx_mode_schedule_retry() which is used after a failed
update.
No Link: tags present in the commit header.
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5d16737167eed..2246be72d8114 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -5197,6 +5197,7 @@ static inline void __dev_mc_unsync(struct net_device *dev,
>
> /* Functions used for secondary unicast and multicast support */
> void dev_set_rx_mode(struct net_device *dev);
> +void netif_rx_mode_schedule_fresh(struct net_device *dev);
> void netif_rx_mode_schedule_retry(struct net_device *dev);
> int netif_set_promiscuity(struct net_device *dev, int inc);
> int dev_set_promiscuity(struct net_device *dev, int inc);
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 08528ca0a8b31..4ae0510ce91fa 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -1337,6 +1337,27 @@ static void netif_rx_mode_queue(struct net_device *dev)
> __netdev_work_core_sched(dev, NETDEV_WORK_RX_MODE);
> }
>
> +/**
> + * netif_rx_mode_schedule_fresh() - schedule a fresh receive-mode update
> + * @dev: network device
> + *
> + * Cancel any pending retry and reset its backoff budget before scheduling an
> + * immediate receive-mode update. Callers use this after a state transition
> + * which requires the complete receive configuration to be replayed, rather
> + * than after a failed update (which uses netif_rx_mode_schedule_retry()).
> + *
> + * Context: sleepable. The caller must hold the device operations lock, or
> + * RTNL for a device which still uses RTNL-compatible operations.
> + */
[Severity: Low]
The kernel-doc says the helper is for callers which need "the complete
receive configuration to be replayed". Does the helper on its own produce
that replay for an ndo_set_rx_mode_async driver?
The queued run ends up in netif_rx_mode_run(), which snapshots the address
lists and then reconciles them. __hw_addr_list_snapshot() copies sync_cnt
and refcount, __hw_addr_sync_dev() skips entries whose sync_cnt is already
nonzero, and the reconcile step then computes a zero delta, so addresses
that were already marked synced are not pushed to the driver again.
Checking the caller added later in the series, enic does invalidate that
state first:
drivers/net/ethernet/cisco/enic/enic_main.c:enic_reset_addr_lists() {
netif_addr_lock_bh(netdev);
__dev_uc_unsync(netdev, NULL);
__dev_mc_unsync(netdev, NULL);
netif_addr_unlock_bh(netdev);
...
}
and __hw_addr_unsync_dev() with a NULL unsync callback is documented and
implemented to just reset sync_cnt for each entry, so the following
netif_rx_mode_schedule_fresh() does re-push everything. That makes the
behaviour correct as used, but the requirement is only visible in the
driver.
Would it be worth stating in the kernel-doc that the caller is responsible
for clearing the per-address sync state (for example via
__dev_uc_unsync(dev, NULL) / __dev_mc_unsync(dev, NULL)) before calling
this helper?
> +void netif_rx_mode_schedule_fresh(struct net_device *dev)
> +{
> + might_sleep();
> + netdev_assert_locked_ops_compat(dev);
> + netif_rx_mode_cancel_retry(dev);
> + netif_rx_mode_queue(dev);
> +}
> +EXPORT_SYMBOL_GPL(netif_rx_mode_schedule_fresh);
> +
[Severity: Low]
This isn't a bug, but at this commit netif_rx_mode_schedule_fresh() is
exported with no in-tree caller.
Grepping the tree at the end of the series shows eight references in
drivers/net/ethernet/cisco/enic/enic_main.c (enic_open(), enic_reset() and
enic_tx_hang_reset() among them), so the symbol does get its first user
inside the same series, which is the usual "add the API in one patch, add
the users in the following patches" layout. Mentioning the intended first
consumer in the changelog would make that obvious to anyone bisecting or
reviewing this patch alone.
> static void netif_rx_mode_retry(struct timer_list *t)
> {
> struct net_device *dev =
[Severity: High]
This isn't a bug introduced by this patch, but since this patch adds a
second entry point that cancels dev->rx_mode_retry_timer, is the lifetime of
that timer fully covered anywhere?
netif_rx_mode_init() arms the timer for the whole net_device lifetime:
net/core/dev_addr_lists.c:netif_rx_mode_init() {
__hw_addr_init(&dev->rx_mode_addr_cache);
timer_setup(&dev->rx_mode_retry_timer, netif_rx_mode_retry, 0);
}
The only cancellation site in the tree is in __dev_close_many():
net/core/dev.c:__dev_close_many() {
...
netif_rx_mode_cancel_retry(dev);
netif_set_up(dev, false);
...
}
but netif_close_many() drops devices that are not IFF_UP before that:
net/core/dev.c:netif_close_many() {
/* Remove the devices that don't need to be closed */
list_for_each_entry_safe(dev, tmp, head, close_list)
if (!(dev->flags & IFF_UP))
list_del_init(&dev->close_list);
...
}
netif_rx_mode_schedule_retry() has no IFF_UP, netif_device_present() or
dev_isalive() guard, and bnxt arms the timer from inside ndo_open, which
runs before __dev_open() sets IFF_UP:
drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_init_chip() {
rc = bnxt_cfg_rx_mode(bp, &bp->dev->uc, true);
if (rc == -EAGAIN) {
netif_rx_mode_schedule_retry(bp->dev);
rc = 0;
}
...
}
(bnge_netdev.c does the same.) If that open subsequently fails, IFF_UP is
never set, so the close path skips the device and nothing cancels the timer.
On the teardown side, unregister_netdevice_many_notify() only calls
netdev_work_cancel_all(dev), and the free path only does
__hw_addr_flush(&dev->rx_mode_addr_cache); there is no
timer_shutdown_sync() or timer_delete_sync() for rx_mode_retry_timer.
Can free_netdev() then release the device while the timer is still queued,
so that netif_rx_mode_retry() dereferences freed memory through
timer_container_of() and netif_rx_mode_queue() -> netdev_work_enqueue() ->
dev_isalive(dev), and the timer wheel keeps a pointer into freed memory?
Would a timer_shutdown_sync(&dev->rx_mode_retry_timer) on the
unregister/free path, or an IFF_UP/present check in
netif_rx_mode_schedule_retry(), be the right fix here?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-b4-enic-sriov-v2-vf-receive-control-v1-0-67a0a6e08d43%40cisco.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 2/6] enic: serialize V2 VF mailbox requests
2026-09-21 19:59 [PATCH net-next 0/6] enic: configure V2 VF addresses and receive mode Satish Kharat
2026-09-21 19:59 ` [PATCH net-next 1/6] net: add netif_rx_mode_schedule_fresh() Satish Kharat
@ 2026-09-21 19:59 ` Satish Kharat
2026-09-24 23:01 ` netdev-bot+sashiko
2026-09-21 19:59 ` [PATCH net-next 3/6] enic: recover V2 VF mailbox when PF state is unknown Satish Kharat
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Satish Kharat @ 2026-09-21 19:59 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman
Cc: netdev, linux-kernel, Satish Kharat, sebaddel
The VF stores one expected reply and uses one completion for mailbox
requests. If two VF control paths issue requests at the same time, the
second request can replace the reply state for the first.
Add a request mutex used only by the VF. Hold it from before a request is
armed until its reply or timeout has been consumed. Recheck VF registration
after taking the mutex and clear pending state on send failure so a later
request cannot inherit it.
PF-side request processing is unchanged. Unsolicited PF notifications and
their acknowledgments remain asynchronous.
Assisted-by: LLM
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/net/ethernet/cisco/enic/enic.h | 1 +
drivers/net/ethernet/cisco/enic/enic_mbox.c | 49 +++++++++++++++++++++++++++--
2 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 7a509a056990..3945fe28f199 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -335,6 +335,7 @@ struct enic {
* the requester.
*/
struct completion mbox_comp;
+ struct mutex vf_mbox_request_lock; /* serializes VF request lifetimes */
spinlock_t mbox_state_lock; /* protects expected reply state */
u64 mbox_expected_msg_num;
u8 mbox_expected_reply;
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
index 5c93ca49552a..b8a18d9682b2 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
@@ -218,6 +218,32 @@ static int enic_mbox_wait_reply(struct enic *enic, unsigned long timeout_ms)
return err;
}
+static void enic_mbox_vf_request_start(struct enic *enic)
+{
+ mutex_lock(&enic->vf_mbox_request_lock);
+ reinit_completion(&enic->mbox_comp);
+ spin_lock_bh(&enic->mbox_state_lock);
+ enic->mbox_expected_msg_num = 0;
+ enic->mbox_expected_reply = 0;
+ spin_unlock_bh(&enic->mbox_state_lock);
+}
+
+static void enic_mbox_vf_request_abort(struct enic *enic)
+{
+ lockdep_assert_held(&enic->vf_mbox_request_lock);
+ spin_lock_bh(&enic->mbox_state_lock);
+ enic->mbox_expected_reply = 0;
+ enic->mbox_expected_msg_num = 0;
+ spin_unlock_bh(&enic->mbox_state_lock);
+ mutex_unlock(&enic->vf_mbox_request_lock);
+}
+
+static void enic_mbox_vf_request_finish(struct enic *enic)
+{
+ lockdep_assert_held(&enic->vf_mbox_request_lock);
+ mutex_unlock(&enic->vf_mbox_request_lock);
+}
+
int enic_mbox_send_link_state(struct enic *enic, u16 vf_id, u32 link_state)
{
struct enic_mbox_pf_link_state_notif_msg notif = {};
@@ -623,6 +649,7 @@ int enic_mbox_vf_capability_check(struct enic *enic)
u32 version;
int err;
+ enic_mbox_vf_request_start(enic);
WRITE_ONCE(enic->pf_cap_version, 0);
req.version = cpu_to_le32(ENIC_MBOX_CAP_VERSION_1);
@@ -630,11 +657,14 @@ int enic_mbox_vf_capability_check(struct enic *enic)
ENIC_MBOX_VF_CAPABILITY_REQUEST,
ENIC_MBOX_VF_CAPABILITY_REPLY,
&req, sizeof(req));
- if (err)
+ if (err) {
+ enic_mbox_vf_request_abort(enic);
return err;
+ }
err = enic_mbox_wait_reply(enic, 3000);
version = READ_ONCE(enic->pf_cap_version);
+ enic_mbox_vf_request_finish(enic);
if (err) {
netdev_warn(enic->netdev,
"MBOX: no capability reply from PF\n");
@@ -656,15 +686,19 @@ int enic_mbox_vf_register(struct enic *enic)
bool registered;
int err;
+ enic_mbox_vf_request_start(enic);
WRITE_ONCE(enic->vf_registered, false);
err = enic_mbox_vf_send_request(enic, ENIC_MBOX_VF_REGISTER_REQUEST,
ENIC_MBOX_VF_REGISTER_REPLY, NULL, 0);
- if (err)
+ if (err) {
+ enic_mbox_vf_request_abort(enic);
return err;
+ }
err = enic_mbox_wait_reply(enic, 3000);
registered = READ_ONCE(enic->vf_registered);
+ enic_mbox_vf_request_finish(enic);
if (err) {
netdev_warn(enic->netdev,
"MBOX: VF registration with PF timed out\n");
@@ -684,16 +718,24 @@ int enic_mbox_vf_unregister(struct enic *enic)
if (!READ_ONCE(enic->vf_registered))
return 0;
+ enic_mbox_vf_request_start(enic);
+ if (!READ_ONCE(enic->vf_registered)) {
+ enic_mbox_vf_request_finish(enic);
+ return 0;
+ }
err = enic_mbox_vf_send_request(enic,
ENIC_MBOX_VF_UNREGISTER_REQUEST,
ENIC_MBOX_VF_UNREGISTER_REPLY,
NULL, 0);
- if (err)
+ if (err) {
+ enic_mbox_vf_request_abort(enic);
return err;
+ }
err = enic_mbox_wait_reply(enic, 3000);
registered = READ_ONCE(enic->vf_registered);
+ enic_mbox_vf_request_finish(enic);
if (err)
return err;
if (registered)
@@ -711,6 +753,7 @@ void enic_mbox_init(struct enic *enic)
*/
if (!reinit) {
mutex_init(&enic->mbox_lock);
+ mutex_init(&enic->vf_mbox_request_lock);
init_completion(&enic->mbox_comp);
spin_lock_init(&enic->mbox_state_lock);
enic->mbox_msg_num = 0;
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 2/6] enic: serialize V2 VF mailbox requests
2026-09-21 19:59 ` [PATCH net-next 2/6] enic: serialize V2 VF mailbox requests Satish Kharat
@ 2026-09-24 23:01 ` netdev-bot+sashiko
0 siblings, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 23:01 UTC (permalink / raw)
To: satishkh
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, netdev,
linux-kernel, sebaddel
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The patch repairs merged code — the VF single-outstanding-reply
mailbox state (`enic->mbox_expected_reply`,…
--
enic: serialize V2 VF mailbox requests
This adds a VF-only mutex (vf_mbox_request_lock) that is held across the
lifetime of a V2 VF mailbox request, from arming the expected reply until
the reply or the timeout has been consumed. The three VF request helpers
enic_mbox_vf_capability_check(), enic_mbox_vf_register() and
enic_mbox_vf_unregister() are converted to the new start/abort/finish
helpers.
One question below about the changelog.
> enic: serialize V2 VF mailbox requests
>
> The VF stores one expected reply and uses one completion for mailbox
> requests. If two VF control paths issue requests at the same time, the
> second request can replace the reply state for the first.
[Severity: Low]
Should this carry a Fixes: tag, and is the described trigger reachable at
this point in the series?
The state being protected (enic->mbox_expected_reply,
enic->mbox_expected_msg_num, enic->mbox_comp) and all three request
helpers already exist in the baseline tree; they came in with commit
72b65c94058e ("enic: add MBOX VF handlers for capability, register and
link state"), whose changelog relies on a single-outstanding-reply
invariant that nothing in the code enforces. Since this changelog
describes a live defect ("the second request can replace the reply state
for the first"), a Fixes: tag pointing at that commit would let
backporters and stable tooling find the affected releases.
On reachability, the VF request issuers present at this commit look
mutually exclusive:
- enic_probe() runs single threaded, before register_netdev().
- enic_admin_chan_reopen() is called from enic_reset() and
enic_tx_hang_reset(), both of which run under rtnl_lock and return
early on !netif_running().
- enic_remove() disables and syncs the reset work items before it issues
the unregister:
disable_work_sync(&enic->reset);
disable_work_sync(&enic->tx_hang_reset);
disable_work_sync(&enic->change_mtu_work);
...
if (enic_is_sriov_vf_v2(enic)) {
if (READ_ONCE(enic->vf_registered)) {
int unreg_err = enic_mbox_vf_unregister(enic);
The additional request sites that could actually overlap (the delayed VF
admin MAC work and the mailbox receive-mode path) appear only in later
patches of this series.
Would it be more accurate to either name the commit being fixed with a
Fixes: tag, or state that this is preparatory serialization for the
request sites added later in the series?
> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index 5c93ca49552ac..b8a18d9682b2f 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> @@ -218,6 +218,32 @@ static int enic_mbox_wait_reply(struct enic *enic, unsigned long timeout_ms)
> return err;
> }
>
> +static void enic_mbox_vf_request_start(struct enic *enic)
> +{
> + mutex_lock(&enic->vf_mbox_request_lock);
> + reinit_completion(&enic->mbox_comp);
> + spin_lock_bh(&enic->mbox_state_lock);
> + enic->mbox_expected_msg_num = 0;
> + enic->mbox_expected_reply = 0;
> + spin_unlock_bh(&enic->mbox_state_lock);
> +}
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-b4-enic-sriov-v2-vf-receive-control-v1-0-67a0a6e08d43%40cisco.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 3/6] enic: recover V2 VF mailbox when PF state is unknown
2026-09-21 19:59 [PATCH net-next 0/6] enic: configure V2 VF addresses and receive mode Satish Kharat
2026-09-21 19:59 ` [PATCH net-next 1/6] net: add netif_rx_mode_schedule_fresh() Satish Kharat
2026-09-21 19:59 ` [PATCH net-next 2/6] enic: serialize V2 VF mailbox requests Satish Kharat
@ 2026-09-21 19:59 ` Satish Kharat
2026-09-24 23:01 ` netdev-bot+sashiko
2026-09-21 19:59 ` [PATCH net-next 4/6] enic: validate V2 VF configuration replies Satish Kharat
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Satish Kharat @ 2026-09-21 19:59 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman
Cc: netdev, linux-kernel, Satish Kharat, sebaddel
A mailbox send can complete and still lose its reply. In that case the VF
cannot know whether the PF changed its address or receive-filter state. A
send-completion timeout is different because the admin-WQ descriptor may
still belong to the device.
Reconnect the V2 VF mailbox after the first case. After a hardware send
timeout, stop using the channel and leave the timed-out DMA mapping for
admin-channel teardown to reclaim safely.
Stop V2 VF receive traffic when the VF can no longer trust that its state
matches the PF. Rebuild and register the admin channel again at a safe
open/reset boundary, then restore receive traffic after the station address
and filters have been replayed. Defer notification acknowledgments so
receive dispatch cannot block behind a VF request.
Reinitialize non-dynamic vNICs after a successful soft reset before
programming queue resources. This restores the initialization sequence used
during probe and ensures that reset recovery rebuilds the vNIC before
reopening the datapath.
Also use this recovery after malformed or dropped admin receive traffic,
and track whether the V2 datapath is open so a failed internal reset cannot
stop it twice.
Assisted-by: LLM
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/net/ethernet/cisco/enic/enic.h | 23 ++-
drivers/net/ethernet/cisco/enic/enic_admin.c | 27 +++-
drivers/net/ethernet/cisco/enic/enic_main.c | 208 ++++++++++++++++++++++-----
drivers/net/ethernet/cisco/enic/enic_mbox.c | 171 ++++++++++++++++++++--
drivers/net/ethernet/cisco/enic/enic_mbox.h | 2 +
drivers/net/ethernet/cisco/enic/enic_rq.c | 11 +-
6 files changed, 392 insertions(+), 50 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 3945fe28f199..782b8b1843ab 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -303,8 +303,21 @@ struct enic {
* left the resources freed.
*/
bool admin_chan_up;
- /* set on send timeout; cleared on channel re-open */
+ /* Blocks sends while the channel is closed or awaiting recovery. */
bool mbox_send_disabled;
+ /* A send timeout leaves a descriptor hardware-owned. Do not reopen the
+ * channel during this device lifetime until reset/DMA fencing is proven.
+ */
+ bool mbox_tx_poisoned;
+ /* After a lost or inconsistent reply, the VF cannot know whether the PF
+ * applied the request. Reconnect during the next open or reset.
+ */
+ bool vf_mbox_reconnect_required;
+ u32 vf_mbox_fault_generation;
+ /* One slow-path-owned predicate keeps the RX hot path fail-closed while
+ * VF registration is lost or receive state may not match the PF.
+ */
+ bool vf_rx_quarantined;
struct vnic_wq admin_wq;
struct vnic_rq admin_rq;
struct vnic_cq admin_cq[2];
@@ -325,6 +338,10 @@ struct enic {
spinlock_t vf_link_state_lock;
enum enic_vf_link_state vf_link_state;
bool vf_link_running;
+ /* Tracks a completely opened V2 VF datapath. An internal reset can stop
+ * it while netif_running() remains true, then fail before reopen.
+ */
+ bool vf_datapath_open;
/* MBOX protocol state — mbox_lock serializes admin WQ sends */
struct mutex mbox_lock;
@@ -337,6 +354,10 @@ struct enic {
struct completion mbox_comp;
struct mutex vf_mbox_request_lock; /* serializes VF request lifetimes */
spinlock_t mbox_state_lock; /* protects expected reply state */
+ spinlock_t vf_ack_lock; /* protects vf_ack_list */
+ struct list_head vf_ack_list;
+ struct work_struct vf_ack_work;
+ unsigned int vf_ack_count;
u64 mbox_expected_msg_num;
u8 mbox_expected_reply;
bool mbox_initialized;
diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
index 61c82b48044d..30c0a5c89a2d 100644
--- a/drivers/net/ethernet/cisco/enic/enic_admin.c
+++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
@@ -132,14 +132,22 @@ unsigned int enic_admin_wq_cq_service(struct enic *enic)
*/
#define ENIC_ADMIN_MSG_MAX 256
+static void enic_admin_rx_lost(struct enic *enic)
+{
+ if (enic_is_sriov_vf_v2(enic))
+ enic_mbox_vf_require_reconnect(enic);
+}
+
static void enic_admin_msg_enqueue(struct enic *enic, void *buf,
unsigned int len)
{
struct enic_admin_msg *msg;
msg = kmalloc_flex(*msg, data, len);
- if (!msg)
+ if (!msg) {
+ enic_admin_rx_lost(enic);
return;
+ }
msg->len = len;
memcpy(msg->data, buf, len);
@@ -152,6 +160,7 @@ static void enic_admin_msg_enqueue(struct enic *enic, void *buf,
netdev_warn(enic->netdev,
"admin msg backlog full (%u); dropping\n",
ENIC_ADMIN_MSG_MAX);
+ enic_admin_rx_lost(enic);
return;
}
list_add_tail(&msg->list, &enic->admin_msg_list);
@@ -194,8 +203,10 @@ unsigned int enic_admin_rq_cq_service(struct enic *enic)
rq_desc = desc;
bwf = le16_to_cpu(rq_desc->bytes_written_flags);
bytes_written = bwf & CQ_ENET_RQ_DESC_BYTES_WRITTEN_MASK;
- if (bytes_written > buf->len)
+ if (bytes_written > buf->len) {
+ enic_admin_rx_lost(enic);
goto next_desc;
+ }
dma_sync_single_for_cpu(&enic->pdev->dev,
buf->dma_addr, buf->len,
@@ -210,11 +221,13 @@ unsigned int enic_admin_rq_cq_service(struct enic *enic)
if (bwf & CQ_ENET_RQ_DESC_FLAGS_TRUNCATED) {
netdev_warn_once(enic->netdev,
"admin RQ: truncated message dropped\n");
+ enic_admin_rx_lost(enic);
goto next_desc;
}
if (!(rq_desc->flags & CQ_ENET_RQ_DESC_FLAGS_FCS_OK)) {
netdev_warn_once(enic->netdev,
"admin RQ: bad FCS, dropping message\n");
+ enic_admin_rx_lost(enic);
goto next_desc;
}
@@ -534,6 +547,11 @@ int enic_admin_channel_open(struct enic *enic)
if (!enic->has_admin_channel)
return -ENODEV;
+ if (READ_ONCE(enic->mbox_tx_poisoned)) {
+ netdev_err(enic->netdev,
+ "Refusing to reopen admin channel after send timeout\n");
+ return -EIO;
+ }
/* Keep MBOX sends disabled for the entire open sequence. It is
* cleared only after every resource is allocated and enabled below,
@@ -641,6 +659,11 @@ void enic_admin_channel_close(struct enic *enic)
enic_admin_teardown_intr(enic);
cancel_work_sync(&enic->link_notify_work);
cancel_work_sync(&enic->admin_msg_work);
+ /* admin_msg_work is the sole VF ACK producer. Drain it before the ACK
+ * worker so an enqueue cannot race the final cancel and queue purge.
+ */
+ if (enic_is_sriov_vf_v2(enic))
+ enic_mbox_vf_ack_cancel(enic);
enic_admin_msg_drain(enic);
enic_admin_qp_type_set(enic, QP_DISABLE);
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 9086e6dd558a..1c60e7a666ae 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -71,6 +71,8 @@
#define PCI_DEVICE_ID_CISCO_VIC_ENET_VF_V2 0x02b7 /* enet SRIOV V2 VF */
#define PCI_DEVICE_ID_CISCO_VIC_ENET_VF_USNIC 0x00cf /* enet USNIC VF */
+static int __enic_stop(struct net_device *netdev, bool remove_vf_station);
+
/* Supported devices */
static const struct pci_device_id enic_id_table[] = {
{ PCI_VDEVICE(CISCO, PCI_DEVICE_ID_CISCO_VIC_ENET) },
@@ -1718,6 +1720,8 @@ static void enic_notify_timer_start(struct enic *enic)
}
}
+static int enic_admin_chan_reopen(struct enic *enic);
+
/* rtnl lock is held, process context */
static int enic_open(struct net_device *netdev)
{
@@ -1736,6 +1740,30 @@ static int enic_open(struct net_device *netdev)
.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
};
+ /* A reply timeout invalidates the current request generation. Rebuild
+ * and re-register the channel before allocating datapath resources so a
+ * later userspace down/up can recover a failed open or reset handshake.
+ * A send timeout is intentionally not recoverable here because its WQ
+ * descriptor may still be hardware-owned.
+ */
+ if (enic_is_sriov_vf_v2(enic) &&
+ READ_ONCE(enic->mbox_tx_poisoned))
+ return -EIO;
+ if (enic_is_sriov_vf_v2(enic) &&
+ (!enic->admin_chan_up || !READ_ONCE(enic->vf_registered) ||
+ READ_ONCE(enic->vf_mbox_reconnect_required))) {
+ /* Re-registration makes the PF discard the old VF-requested
+ * filters. Clear the netdev-core synchronization state so the
+ * receive-mode callback replays the current address lists.
+ */
+ enic_reset_addr_lists(enic);
+ if (enic->admin_chan_up)
+ enic_admin_channel_close(enic);
+ err = enic_admin_chan_reopen(enic);
+ if (err)
+ return err;
+ }
+
err = enic_request_intr(enic);
if (err) {
netdev_err(netdev, "Unable to request irq.\n");
@@ -1794,17 +1822,41 @@ static int enic_open(struct net_device *netdev)
netdev_err(netdev, "Failed to enable device: %d\n", err);
goto err_out_dev_enable;
}
+ if (enic_is_sriov_vf_v2(enic)) {
+ /* Commit the replay only if no mailbox fault arrived while station
+ * and receive policy were being programmed. Keep the state lock
+ * through the carrier transition so a later fault necessarily wins
+ * and turns carrier back off.
+ */
+ spin_lock_bh(&enic->mbox_state_lock);
+ if (!READ_ONCE(enic->vf_registered) ||
+ READ_ONCE(enic->mbox_send_disabled) ||
+ READ_ONCE(enic->mbox_tx_poisoned) ||
+ READ_ONCE(enic->vf_mbox_reconnect_required)) {
+ err = -EIO;
+ } else {
+ WRITE_ONCE(enic->vf_rx_quarantined, false);
+ enic_mbox_vf_link_state_set_running(enic, true);
+ }
+ spin_unlock_bh(&enic->mbox_state_lock);
+ if (err) {
+ netdev_err(netdev,
+ "MBOX state changed during VF datapath open\n");
+ goto err_out_dev_disable;
+ }
+ }
for (i = 0; i < enic->intr_count; i++)
vnic_intr_unmask(&enic->intr[i]);
-
enic_notify_timer_start(enic);
enic_rfs_timer_start(enic);
if (enic_is_sriov_vf_v2(enic))
- enic_mbox_vf_link_state_set_running(enic, true);
+ enic->vf_datapath_open = true;
return 0;
+err_out_dev_disable:
+ enic_dev_disable(enic);
err_out_dev_enable:
for (i = 0; i < enic->rq_count; i++)
napi_disable(&enic->napi[i]);
@@ -1834,12 +1886,20 @@ static int enic_open(struct net_device *netdev)
}
/* rtnl lock is held, process context */
-static int enic_stop(struct net_device *netdev)
+static int __enic_stop(struct net_device *netdev, bool remove_vf_station)
{
struct enic *enic = netdev_priv(netdev);
unsigned int i;
int err;
+ /* Internal reset leaves netif_running() set while the datapath is down.
+ * If re-registration or reopen then fails, a later administrative close
+ * must not disable NAPI a second time.
+ */
+ if (enic_is_sriov_vf_v2(enic) && !enic->vf_datapath_open)
+ return 0;
+ (void)remove_vf_station;
+
for (i = 0; i < enic->intr_count; i++) {
vnic_intr_mask(&enic->intr[i]);
(void)vnic_intr_masked(&enic->intr[i]); /* flush write */
@@ -1893,10 +1953,17 @@ static int enic_stop(struct net_device *netdev)
vnic_cq_clean(&enic->cq[i]);
for (i = 0; i < enic->intr_count; i++)
vnic_intr_clean(&enic->intr[i]);
+ if (enic_is_sriov_vf_v2(enic))
+ enic->vf_datapath_open = false;
return 0;
}
+static int enic_stop(struct net_device *netdev)
+{
+ return __enic_stop(netdev, true);
+}
+
static int _enic_change_mtu(struct net_device *netdev, int new_mtu)
{
bool running = netif_running(netdev);
@@ -2196,14 +2263,15 @@ static bool enic_has_admin_chan(struct enic *enic)
(enic_sriov_enabled(enic) && enic->vf_type == ENIC_VF_TYPE_V2);
}
-/* Re-establish the admin/MBOX channel after a reset has re-created the data
- * path. Mirrors the relevant part of the probe / SR-IOV-enable sequence:
+/* Re-establish the admin/MBOX channel after a reset has re-created the vNIC
+ * resources. Mirrors the relevant part of the probe / SR-IOV-enable sequence:
* reinitialise MBOX and reopen the channel, then for a VF re-run the PF
* handshake (the reset wiped the VF's admin QP, so the VF must register
* again), or for a PF re-push the current link state to registered VFs.
*/
-static void enic_admin_chan_reopen(struct enic *enic)
+static int enic_admin_chan_reopen(struct enic *enic)
{
+ u32 recovery_generation = 0;
int err;
/* Install the MBOX receive handler and clear pending reply state before
@@ -2222,12 +2290,17 @@ static void enic_admin_chan_reopen(struct enic *enic)
*/
if (enic_is_sriov_vf_v2(enic))
WRITE_ONCE(enic->vf_registered, false);
+ if (enic_is_sriov_vf_v2(enic)) {
+ spin_lock_bh(&enic->mbox_state_lock);
+ recovery_generation = enic->vf_mbox_fault_generation;
+ spin_unlock_bh(&enic->mbox_state_lock);
+ }
err = enic_admin_channel_open(enic);
if (err) {
netdev_err(enic->netdev,
"admin channel reopen after reset failed: %d\n", err);
- return;
+ return err;
}
if (enic_is_sriov_vf_v2(enic)) {
@@ -2237,7 +2310,7 @@ static void enic_admin_chan_reopen(struct enic *enic)
"MBOX capability check after reset failed: %d\n",
err);
enic_admin_channel_close(enic);
- return;
+ return err;
}
err = enic_mbox_vf_register(enic);
if (err) {
@@ -2245,6 +2318,26 @@ static void enic_admin_chan_reopen(struct enic *enic)
"MBOX VF re-registration after reset failed: %d\n",
err);
enic_admin_channel_close(enic);
+ return err;
+ }
+ enic_reset_addr_lists(enic);
+ /* Capability negotiation and registration establish a new protocol
+ * generation. RX remains quarantined until enic_open() replays the
+ * station and receive policy.
+ */
+ spin_lock_bh(&enic->mbox_state_lock);
+ if (enic->vf_mbox_fault_generation != recovery_generation ||
+ READ_ONCE(enic->mbox_tx_poisoned)) {
+ err = -EAGAIN;
+ } else {
+ WRITE_ONCE(enic->vf_mbox_reconnect_required, false);
+ }
+ spin_unlock_bh(&enic->mbox_state_lock);
+ if (err) {
+ netdev_warn(enic->netdev,
+ "MBOX state changed during VF re-registration\n");
+ enic_admin_channel_close(enic);
+ return err;
}
} else {
/* The link came back up during enic_open() above while MBOX
@@ -2253,79 +2346,119 @@ static void enic_admin_chan_reopen(struct enic *enic)
*/
schedule_work(&enic->link_notify_work);
}
+
+ return 0;
}
static void enic_reset(struct work_struct *work)
{
struct enic *enic = container_of(work, struct enic, reset);
+ int err;
if (!netif_running(enic->netdev))
return;
rtnl_lock();
+ /* V2 protocol recovery can be queued immediately before ndo_stop()
+ * acquires RTNL. Recheck under RTNL so that new recovery path cannot
+ * reopen a device userspace just closed. Preserve the existing reset
+ * behavior for every other ENIC device.
+ */
+ if (enic_is_sriov_vf_v2(enic) && !netif_running(enic->netdev))
+ goto unlock;
/* Stop any activity from infiniband */
enic_set_api_busy(enic, true);
- /* Fully tear down the V2 admin/MBOX channel before the soft reset.
- * The reset wipes all hardware queues including the admin WQ/RQ;
- * closing first tells firmware to stop the admin QP (so it no longer
- * DMAs from the about-to-be-reset rings) and frees the admin resources
- * so they are cleanly re-allocated afterwards.
+ /* Stop the datapath and existing admin/MBOX channel before the soft
+ * reset. Do not send DEL_MAC from this path: a timeout would poison the
+ * channel while reset and fresh registration already discard the old
+ * VF-requested protocol state before the station address is replayed.
+ * Reopen allocates fresh admin resources after reset recreates the vNIC.
*/
+ __enic_stop(enic->netdev, false);
if (enic_has_admin_chan(enic))
enic_admin_channel_close(enic);
- enic_stop(enic->netdev);
if (enic_is_sriov_vf_v2(enic))
enic_mbox_vf_link_state_reset(enic);
+ err = enic_dev_soft_reset(enic);
+ if (err)
+ goto reset_out;
+
+ if (!enic_is_dynamic(enic)) {
+ err = vnic_dev_init(enic->vdev, 0);
+ if (err) {
+ netdev_err(enic->netdev,
+ "vNIC init after soft reset failed: %d\n",
+ err);
+ goto reset_out;
+ }
+ }
- enic_dev_soft_reset(enic);
enic_reset_addr_lists(enic);
enic_init_vnic_resources(enic);
enic_set_rss_nic_cfg(enic);
enic_dev_set_ig_vlan_rewrite_mode(enic);
enic_ext_cq(enic);
- enic_open(enic->netdev);
+ /* A V2 VF needs PF registration before enic_open() can install its
+ * station address. A V2 PF reopens afterwards and replays carrier.
+ */
+ if (enic_is_sriov_vf_v2(enic)) {
+ err = enic_admin_chan_reopen(enic);
+ if (err)
+ goto reset_out;
+ }
- /* Re-establish the admin/MBOX channel after the data path is back up.
- * It was fully torn down by enic_admin_channel_close() above;
- * enic_admin_chan_reopen() reopens it and, for a PF re-pushes link
- * state, or for a VF re-runs the probe-time PF handshake.
+ err = enic_open(enic->netdev);
+ if (err)
+ netdev_err(enic->netdev,
+ "Failed to reopen datapath after reset: %d\n", err);
+
+ /* A PF reopens its admin channel after the datapath and re-pushes link
+ * state. The VF handshake, which open depends on, completed above.
*/
- if (enic_has_admin_chan(enic))
+ if (enic_has_admin_chan(enic) && !enic_is_sriov_vf_v2(enic))
enic_admin_chan_reopen(enic);
+reset_out:
/* Allow infiniband to fiddle with the device again */
enic_set_api_busy(enic, false);
call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
+unlock:
rtnl_unlock();
}
static void enic_tx_hang_reset(struct work_struct *work)
{
struct enic *enic = container_of(work, struct enic, tx_hang_reset);
+ int err;
rtnl_lock();
+ /* The V2 changes below add admin-channel recovery to this worker. Do not
+ * let that new path reopen a VF after userspace completed ndo_stop();
+ * leave the existing behavior for other ENIC devices unchanged.
+ */
+ if (enic_is_sriov_vf_v2(enic) && !netif_running(enic->netdev))
+ goto unlock;
/* Stop any activity from infiniband */
enic_set_api_busy(enic, true);
- /* Fully tear down the V2 admin/MBOX channel before the hang reset, for
- * the same reason as the soft reset path: stop the admin QP and free
- * the admin resources before the hardware queues are wiped.
+ /* Preserve the firmware hang-notification contract by reporting the hung
+ * queue before stopping and cleaning it. As in the soft-reset path, skip
+ * DEL_MAC because reset and fresh registration are the cleanup boundary.
*/
+ enic_dev_hang_notify(enic);
+ __enic_stop(enic->netdev, false);
if (enic_has_admin_chan(enic))
enic_admin_channel_close(enic);
- enic_dev_hang_notify(enic);
- enic_stop(enic->netdev);
if (enic_is_sriov_vf_v2(enic))
enic_mbox_vf_link_state_reset(enic);
-
enic_dev_hang_reset(enic);
enic_reset_addr_lists(enic);
enic_init_vnic_resources(enic);
@@ -2333,21 +2466,30 @@ static void enic_tx_hang_reset(struct work_struct *work)
enic_dev_set_ig_vlan_rewrite_mode(enic);
enic_ext_cq(enic);
- enic_open(enic->netdev);
+ if (enic_is_sriov_vf_v2(enic)) {
+ err = enic_admin_chan_reopen(enic);
+ if (err)
+ goto hang_reset_out;
+ }
- /* Re-establish the admin/MBOX channel after the data path is back up.
- * It was fully torn down by enic_admin_channel_close() above;
- * enic_admin_chan_reopen() reopens it and, for a PF re-pushes link
- * state, or for a VF re-runs the probe-time PF handshake.
+ err = enic_open(enic->netdev);
+ if (err)
+ netdev_err(enic->netdev,
+ "Failed to reopen datapath after hang reset: %d\n", err);
+
+ /* A PF reopens its admin channel after the datapath and re-pushes link
+ * state. The VF handshake, which open depends on, completed above.
*/
- if (enic_has_admin_chan(enic))
+ if (enic_has_admin_chan(enic) && !enic_is_sriov_vf_v2(enic))
enic_admin_chan_reopen(enic);
+hang_reset_out:
/* Allow infiniband to fiddle with the device again */
enic_set_api_busy(enic, false);
call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
+unlock:
rtnl_unlock();
}
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
index b8a18d9682b2..2d77d41577a3 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
@@ -149,7 +149,20 @@ static int enic_mbox_send_msg_id(struct enic *enic, u8 msg_type,
* or free the buffer: the device may still DMA from dma_addr.
* Mark the channel unusable so no further sends are attempted.
*/
+ spin_lock_bh(&enic->mbox_state_lock);
WRITE_ONCE(enic->mbox_send_disabled, true);
+ WRITE_ONCE(enic->mbox_tx_poisoned, true);
+ if (enic_is_sriov_vf_v2(enic)) {
+ /* The request may have changed PF receive policy even though
+ * local descriptor ownership is still uncertain. Fail the VF
+ * closed and do not turn this into an ordinary protocol
+ * reconnect; the admin-channel lifecycle owns final reclamation.
+ */
+ WRITE_ONCE(enic->vf_rx_quarantined, true);
+ }
+ spin_unlock_bh(&enic->mbox_state_lock);
+ if (enic_is_sriov_vf_v2(enic))
+ enic_mbox_vf_link_state_set_running(enic, false);
}
netdev_dbg(enic->netdev,
@@ -184,6 +197,105 @@ static int enic_mbox_send_reply(struct enic *enic, u8 msg_type,
payload_len, msg_num, true, 0);
}
+struct enic_mbox_vf_ack {
+ struct list_head list;
+ u64 msg_num;
+ u16 ret_major;
+ u8 msg_type;
+};
+
+static void enic_mbox_vf_ack_work(struct work_struct *work)
+{
+ struct enic *enic = container_of(work, struct enic, vf_ack_work);
+ struct enic_mbox_vf_ack *pending;
+
+ for (;;) {
+ struct enic_mbox_generic_reply ack = {};
+ u8 msg_type;
+ int err;
+
+ spin_lock_bh(&enic->vf_ack_lock);
+ if (list_empty(&enic->vf_ack_list)) {
+ spin_unlock_bh(&enic->vf_ack_lock);
+ break;
+ }
+ pending = list_first_entry(&enic->vf_ack_list,
+ struct enic_mbox_vf_ack, list);
+ list_del(&pending->list);
+ enic->vf_ack_count--;
+ spin_unlock_bh(&enic->vf_ack_lock);
+
+ if (READ_ONCE(enic->mbox_send_disabled)) {
+ kfree(pending);
+ continue;
+ }
+
+ msg_type = pending->msg_type;
+ ack.ret_major = cpu_to_le16(pending->ret_major);
+ err = enic_mbox_send_reply(enic, msg_type, ENIC_MBOX_DST_PF,
+ &ack, sizeof(ack), pending->msg_num);
+ kfree(pending);
+ if (err && net_ratelimit())
+ netdev_warn(enic->netdev,
+ "MBOX: failed to send ACK type %u: %d\n",
+ msg_type, err);
+ }
+}
+
+static void enic_mbox_vf_queue_ack(struct enic *enic, u8 msg_type,
+ u64 msg_num, u16 ret_major)
+{
+ struct enic_mbox_vf_ack *pending;
+
+ if (READ_ONCE(enic->mbox_send_disabled))
+ return;
+ pending = kmalloc_obj(*pending, GFP_ATOMIC);
+ if (!pending) {
+ if (net_ratelimit())
+ netdev_warn(enic->netdev,
+ "MBOX: dropping ACK type %u: no memory\n",
+ msg_type);
+ return;
+ }
+ pending->msg_num = msg_num;
+ pending->ret_major = ret_major;
+ pending->msg_type = msg_type;
+
+ spin_lock_bh(&enic->vf_ack_lock);
+ if (READ_ONCE(enic->mbox_send_disabled) ||
+ enic->vf_ack_count >= ENIC_ADMIN_DESC_COUNT) {
+ spin_unlock_bh(&enic->vf_ack_lock);
+ kfree(pending);
+ if (net_ratelimit())
+ netdev_warn(enic->netdev,
+ "MBOX: dropping ACK type %u: queue unavailable\n",
+ msg_type);
+ return;
+ }
+ list_add_tail(&pending->list, &enic->vf_ack_list);
+ enic->vf_ack_count++;
+ spin_unlock_bh(&enic->vf_ack_lock);
+ schedule_work(&enic->vf_ack_work);
+}
+
+void enic_mbox_vf_ack_cancel(struct enic *enic)
+{
+ struct enic_mbox_vf_ack *pending, *tmp;
+ LIST_HEAD(discard);
+
+ if (!enic->mbox_initialized)
+ return;
+ cancel_work_sync(&enic->vf_ack_work);
+ spin_lock_bh(&enic->vf_ack_lock);
+ list_splice_init(&enic->vf_ack_list, &discard);
+ enic->vf_ack_count = 0;
+ spin_unlock_bh(&enic->vf_ack_lock);
+ list_for_each_entry_safe(pending, tmp, &discard, list) {
+ list_del(&pending->list);
+ kfree(pending);
+ }
+}
+
static int enic_mbox_vf_send_request(struct enic *enic, u8 request_type,
u8 expected_reply, void *payload,
u16 payload_len)
@@ -193,6 +305,26 @@ static int enic_mbox_vf_send_request(struct enic *enic, u8 request_type,
expected_reply);
}
+static void enic_mbox_vf_mark_reconnect_locked(struct enic *enic,
+ bool registration_lost)
+{
+ lockdep_assert_held(&enic->mbox_state_lock);
+
+ if (registration_lost)
+ WRITE_ONCE(enic->vf_registered, false);
+ enic->vf_mbox_fault_generation++;
+ WRITE_ONCE(enic->vf_mbox_reconnect_required, true);
+ WRITE_ONCE(enic->mbox_send_disabled, true);
+ WRITE_ONCE(enic->vf_rx_quarantined, true);
+}
+
+static void enic_mbox_vf_kick_recovery(struct enic *enic)
+{
+ enic_mbox_vf_link_state_set_running(enic, false);
+ if (netif_running(enic->netdev))
+ schedule_work(&enic->reset);
+}
+
static int enic_mbox_wait_reply(struct enic *enic, unsigned long timeout_ms)
{
unsigned long left;
@@ -203,9 +335,9 @@ static int enic_mbox_wait_reply(struct enic *enic, unsigned long timeout_ms)
if (left)
return 0;
- /* Invalidate a request that the handler has not already accepted. A
- * delayed reply cannot match a later request because message numbers are
- * monotonic across channel reopen.
+ /* Invalidate a request that the handler has not already accepted. Whether
+ * losing the reply invalidates the current protocol generation is an
+ * operation-specific decision made by the caller.
*/
spin_lock_bh(&enic->mbox_state_lock);
if (enic->mbox_expected_reply) {
@@ -468,9 +600,8 @@ static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload,
u64 msg_num)
{
struct enic_mbox_pf_link_state_notif_msg *notif = payload;
- struct enic_mbox_pf_link_state_ack_msg ack = {};
u32 link_state = le32_to_cpu(notif->link_state);
- int err;
+ u16 ret_major = 0;
spin_lock_bh(&enic->vf_link_state_lock);
switch (link_state) {
@@ -491,16 +622,16 @@ static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload,
default:
netdev_warn(enic->netdev, "MBOX: unknown link state %u\n",
link_state);
- ack.ack.ret_major = cpu_to_le16(ENIC_MBOX_ERR_GENERIC);
+ ret_major = ENIC_MBOX_ERR_GENERIC;
break;
}
spin_unlock_bh(&enic->vf_link_state_lock);
- err = enic_mbox_send_reply(enic, ENIC_MBOX_PF_LINK_STATE_ACK,
- ENIC_MBOX_DST_PF, &ack, sizeof(ack), msg_num);
- if (err && net_ratelimit())
- netdev_warn(enic->netdev,
- "MBOX: failed to send link state ACK: %d\n", err);
+ /* Notification dispatch must not wait behind a synchronous request send:
+ * its matching reply may be queued behind this notification.
+ */
+ enic_mbox_vf_queue_ack(enic, ENIC_MBOX_PF_LINK_STATE_ACK, msg_num,
+ ret_major);
}
void enic_mbox_vf_link_state_reset(struct enic *enic)
@@ -525,6 +656,17 @@ void enic_mbox_vf_link_state_set_running(struct enic *enic, bool running)
spin_unlock_bh(&enic->vf_link_state_lock);
}
+void enic_mbox_vf_require_reconnect(struct enic *enic)
+{
+ /* A fresh REGISTER transaction lets the PF discard any VF-requested
+ * configuration whose final state became uncertain.
+ */
+ spin_lock_bh(&enic->mbox_state_lock);
+ enic_mbox_vf_mark_reconnect_locked(enic, false);
+ spin_unlock_bh(&enic->mbox_state_lock);
+ enic_mbox_vf_kick_recovery(enic);
+}
+
static bool enic_mbox_vf_payload_ok(struct enic *enic, u8 msg_type,
u16 payload_len, size_t min_len)
{
@@ -600,6 +742,8 @@ static void enic_mbox_recv_handler(struct enic *enic, void *buf,
netdev_warn(enic->netdev,
"MBOX: truncated message (len %u < %zu)\n",
len, sizeof(*hdr));
+ if (!enic->vf_state)
+ enic_mbox_vf_require_reconnect(enic);
return;
}
@@ -756,7 +900,12 @@ void enic_mbox_init(struct enic *enic)
mutex_init(&enic->vf_mbox_request_lock);
init_completion(&enic->mbox_comp);
spin_lock_init(&enic->mbox_state_lock);
+ spin_lock_init(&enic->vf_ack_lock);
+ INIT_LIST_HEAD(&enic->vf_ack_list);
+ INIT_WORK(&enic->vf_ack_work, enic_mbox_vf_ack_work);
enic->mbox_msg_num = 0;
+ if (enic_is_sriov_vf_v2(enic))
+ WRITE_ONCE(enic->vf_rx_quarantined, true);
enic->mbox_initialized = true;
} else {
reinit_completion(&enic->mbox_comp);
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.h b/drivers/net/ethernet/cisco/enic/enic_mbox.h
index 60409bad2f28..37bc41a900f4 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.h
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.h
@@ -90,6 +90,8 @@ int enic_mbox_send_msg(struct enic *enic, u8 msg_type, u16 dst_vnic_id,
int enic_mbox_send_link_state(struct enic *enic, u16 vf_id, u32 link_state);
void enic_mbox_vf_link_state_reset(struct enic *enic);
void enic_mbox_vf_link_state_set_running(struct enic *enic, bool running);
+void enic_mbox_vf_ack_cancel(struct enic *enic);
+void enic_mbox_vf_require_reconnect(struct enic *enic);
int enic_mbox_vf_capability_check(struct enic *enic);
int enic_mbox_vf_register(struct enic *enic);
int enic_mbox_vf_unregister(struct enic *enic);
diff --git a/drivers/net/ethernet/cisco/enic/enic_rq.c b/drivers/net/ethernet/cisco/enic/enic_rq.c
index ccbf5c9a21d0..80fe7e819c37 100644
--- a/drivers/net/ethernet/cisco/enic/enic_rq.c
+++ b/drivers/net/ethernet/cisco/enic/enic_rq.c
@@ -330,8 +330,6 @@ static void enic_rq_indicate_buf(struct enic *enic, struct vnic_rq *rq,
u16 bytes_written, vlan_tci, checksum;
u32 rss_hash;
- rqstats->packets++;
-
cq_enet_rq_desc_dec((struct cq_enet_rq_desc *)cq_desc, &ingress_port,
&fcoe, &eop, &sop, &rss_type, &csum_not_calc,
&rss_hash, &bytes_written, &packet_error,
@@ -340,8 +338,15 @@ static void enic_rq_indicate_buf(struct enic *enic, struct vnic_rq *rq,
&tcp_udp_csum_ok, &udp, &tcp, &ipv4_csum_ok, &ipv6,
&ipv4, &ipv4_fragment, &fcs_ok);
- if (enic_rq_pkt_error(rq, packet_error, fcs_ok, bytes_written))
+ if (enic_rq_pkt_error(rq, packet_error, fcs_ok, bytes_written)) {
+ rqstats->packets++;
+ return;
+ }
+ if (unlikely(READ_ONCE(enic->vf_rx_quarantined))) {
+ dev_core_stats_rx_dropped_inc(enic->netdev);
return;
+ }
+ rqstats->packets++;
if (eop && bytes_written > 0) {
/* Good receive
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 3/6] enic: recover V2 VF mailbox when PF state is unknown
2026-09-21 19:59 ` [PATCH net-next 3/6] enic: recover V2 VF mailbox when PF state is unknown Satish Kharat
@ 2026-09-24 23:01 ` netdev-bot+sashiko
0 siblings, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 23:01 UTC (permalink / raw)
To: satishkh
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, netdev,
linux-kernel, sebaddel
Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.
Critical: 0 · High: 4 · Medium: 2 · Low: 2
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] enic_reset() gains two abort paths (enic_dev_soft_reset()
failure and the new vnic_dev_init() failure) that `goto reset_out`…
- [High] __enic_stop() still returns early mid-teardown when
vnic_wq_disable()/vnic_rq_disable() times out, and both reset workers…
- [High] enic_mbox_vf_kick_recovery() unconditionally does
schedule_work(&enic->reset) with no attempt counter, backoff, or…
- [High] enic: VF permanently locked out after send timeout due to
uncleared mbox_tx_poisoned
- [Medium] The new admin-channel recovery inside enic_open() can race
enic_remove(): enic_remove() unregisters the VF and calls…
- [Medium] Malformed admin messages can quarantine and reset a V2 VF
before the hardware-verified sender is checked: for messages shorter…
- [Low] Commit message and new in-tree comments (enic_open(),
enic_admin_chan_reopen(), struct enic.vf_mbox_reconnect_required)…
- [Low] The patch fixes several distinct user-visible defects (lost-reply
state divergence, RX served while VF state is unknown, double NAPI…
--
enic: recover V2 VF mailbox when PF state is unknown
This adds a reconnect/quarantine state machine for the V2 VF mailbox: a lost
or malformed reply marks the protocol generation stale, receive traffic is
dropped while the VF cannot trust its state, link-state acknowledgments are
deferred to a work item, and the reset workers now rebuild the admin channel
and re-run vnic_dev_init() before reopening the datapath.
A few questions below.
> Assisted-by: LLM
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
[Severity: Low]
Should this carry Fixes: tags and be split up? The change fixes several
distinct user-visible defects (state divergence after a lost reply, receive
traffic served while the VF state is unknown, a second NAPI disable after a
failed internal reset) but has no Fixes: tag.
It also bundles at least six independent mechanisms in one patch: the
mailbox reconnect state machine, the RX quarantine in the datapath, the
deferred notification-ACK worker, the new post-soft-reset vnic_dev_init()
that also affects PFs, the datapath-open tracking, and the new
reset-abort behaviour. Splitting these would make the double-stop fix or
the deferred ACK backportable without the permanent mbox_tx_poisoned
policy.
> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index 61c82b48044d0..30c0a5c89a2d2 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
> @@ -132,14 +132,22 @@ unsigned int enic_admin_wq_cq_service(struct enic *enic)
> */
> #define ENIC_ADMIN_MSG_MAX 256
>
> +static void enic_admin_rx_lost(struct enic *enic)
> +{
> + if (enic_is_sriov_vf_v2(enic))
> + enic_mbox_vf_require_reconnect(enic);
> +}
> +
[ ... ]
> @@ -194,8 +203,10 @@ unsigned int enic_admin_rq_cq_service(struct enic *enic)
> rq_desc = desc;
> bwf = le16_to_cpu(rq_desc->bytes_written_flags);
> bytes_written = bwf & CQ_ENET_RQ_DESC_BYTES_WRITTEN_MASK;
> - if (bytes_written > buf->len)
> + if (bytes_written > buf->len) {
> + enic_admin_rx_lost(enic);
> goto next_desc;
> + }
[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 9086e6dd558aa..1c60e7a666aeb 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
[ ... ]
> @@ -1736,6 +1740,30 @@ static int enic_open(struct net_device *netdev)
> .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> };
>
> + /* A reply timeout invalidates the current request generation. Rebuild
> + * and re-register the channel before allocating datapath resources so a
> + * later userspace down/up can recover a failed open or reset handshake.
> + * A send timeout is intentionally not recoverable here because its WQ
> + * descriptor may still be hardware-owned.
> + */
> + if (enic_is_sriov_vf_v2(enic) &&
> + READ_ONCE(enic->mbox_tx_poisoned))
> + return -EIO;
> + if (enic_is_sriov_vf_v2(enic) &&
> + (!enic->admin_chan_up || !READ_ONCE(enic->vf_registered) ||
> + READ_ONCE(enic->vf_mbox_reconnect_required))) {
> + /* Re-registration makes the PF discard the old VF-requested
> + * filters. Clear the netdev-core synchronization state so the
> + * receive-mode callback replays the current address lists.
> + */
> + enic_reset_addr_lists(enic);
> + if (enic->admin_chan_up)
> + enic_admin_channel_close(enic);
> + err = enic_admin_chan_reopen(enic);
> + if (err)
> + return err;
> + }
[Severity: Low]
Do these comments describe code that only exists in later patches of the
series? The comment in enic_open() talks about the PF discarding "old
VF-requested filters" and about the receive-mode callback replaying address
lists, and enic_admin_chan_reopen() says "RX remains quarantined until
enic_open() replays the station and receive policy".
At this commit the only VF to PF requests are CAPABILITY, REGISTER and
UNREGISTER, enic_set_rx_mode() is devcmd based, and enic_open() does not
install a station address for SR-IOV VFs, so there is nothing to replay
yet. The commit message has the same forward reference:
"then restore receive traffic after the station address
and filters have been replayed"
Would it read better to move these comments to the patches that add the
ADD_DEL_MAC / SET_PKT_FILTER transactions?
[Severity: Medium]
Can this new admin-channel rebuild inside ndo_open race enic_remove()?
enic_remove() unregisters the VF and calls enic_admin_channel_close()
before unregister_netdev(), and without rtnl_lock():
enic_remove()
if (enic_is_sriov_vf_v2(enic)) {
if (READ_ONCE(enic->vf_registered))
enic_mbox_vf_unregister(enic);
...
enic_admin_channel_close(enic);
}
unregister_netdev(netdev);
Meanwhile "ip link set dev X up" reaches enic_open(), sees !admin_chan_up
or !vf_registered, and runs enic_admin_channel_close() followed by
enic_admin_chan_reopen(). The gate in enic_admin_channel_close() is only
if (!enic->admin_chan_up)
return;
which is a plain bool read outside any lock, so it does not serialise the
two teardowns. Can this either double-free the admin resources and IRQ,
or leave a freshly reopened admin channel (requested MSI-X vector, DMA
rings, live work items) behind after free_netdev()? Disabling the reset
work items in enic_remove() does not cover ndo_open.
> @@ -1834,12 +1886,20 @@ static int enic_open(struct net_device *netdev)
> }
>
> /* rtnl lock is held, process context */
> -static int enic_stop(struct net_device *netdev)
> +static int __enic_stop(struct net_device *netdev, bool remove_vf_station)
> {
> struct enic *enic = netdev_priv(netdev);
> unsigned int i;
> int err;
>
> + /* Internal reset leaves netif_running() set while the datapath is down.
> + * If re-registration or reopen then fails, a later administrative close
> + * must not disable NAPI a second time.
> + */
> + if (enic_is_sriov_vf_v2(enic) && !enic->vf_datapath_open)
> + return 0;
> + (void)remove_vf_station;
> +
[ ... ]
> @@ -1893,10 +1953,17 @@ static int enic_stop(struct net_device *netdev)
> vnic_cq_clean(&enic->cq[i]);
> for (i = 0; i < enic->intr_count; i++)
> vnic_intr_clean(&enic->intr[i]);
> + if (enic_is_sriov_vf_v2(enic))
> + enic->vf_datapath_open = false;
>
> return 0;
> }
[Severity: High]
Can vf_datapath_open stay true after a partial teardown? __enic_stop()
still returns early when a queue refuses to stop:
for (i = 0; i < enic->wq_count; i++) {
err = vnic_wq_disable(&enic->wq[i].vwq);
if (err)
return err;
}
That return happens after napi_disable() but before enic_free_intr(),
vnic_wq_clean(), vnic_rq_clean(), page_pool_destroy() and before the new
"enic->vf_datapath_open = false" at the end of the function.
Both reset workers now invoke it as a bare statement and ignore the
error:
enic_dev_hang_notify(enic);
__enic_stop(enic->netdev, false);
enic_tx_hang_reset() runs precisely because a queue is hung, and
vnic_wq_disable()/vnic_rq_disable() poll the hardware running bit and
return -ETIMEDOUT when it does not clear. The worker then continues into
enic_dev_hang_reset() and enic_init_vnic_resources(), reprogramming the
ring control registers while to_use/to_clean/desc_avail still describe
the pre-reset ring, with buffers still DMA-mapped and data-path IRQs
still requested.
If enic_admin_chan_reopen() then fails and the worker exits via
hang_reset_out without napi_enable(), vf_datapath_open is still true, so
the guard at the top of __enic_stop() does not fire on the next
dev_close() and napi_disable() is called again on an already disabled
NAPI. napi_disable_locked() loops:
while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
usleep_range(20, 200);
val = READ_ONCE(n->state);
}
and only napi_enable() clears those bits, so does this spin forever with
RTNL held? Would clearing vf_datapath_open before the fallible teardown
steps, and propagating the __enic_stop() error, avoid this?
> @@ -2253,79 +2346,119 @@ static void enic_admin_chan_reopen(struct enic *enic)
> */
> schedule_work(&enic->link_notify_work);
> }
> +
> + return 0;
> }
>
> static void enic_reset(struct work_struct *work)
> {
> struct enic *enic = container_of(work, struct enic, reset);
> + int err;
>
> if (!netif_running(enic->netdev))
> return;
[ ... ]
> + __enic_stop(enic->netdev, false);
> if (enic_has_admin_chan(enic))
> enic_admin_channel_close(enic);
>
> - enic_stop(enic->netdev);
> if (enic_is_sriov_vf_v2(enic))
> enic_mbox_vf_link_state_reset(enic);
> + err = enic_dev_soft_reset(enic);
> + if (err)
> + goto reset_out;
> +
> + if (!enic_is_dynamic(enic)) {
> + err = vnic_dev_init(enic->vdev, 0);
> + if (err) {
> + netdev_err(enic->netdev,
> + "vNIC init after soft reset failed: %d\n",
> + err);
> + goto reset_out;
> + }
> + }
[Severity: High]
Is mbox_tx_poisoned ever cleared again? I cannot find a single
WRITE_ONCE(enic->mbox_tx_poisoned, false) anywhere in the series, so
after one admin-WQ send timeout in enic_mbox_send_msg_id() the flag
stays set for the rest of the driver's lifetime on that device, and
every recovery route is then closed:
- enic_admin_channel_open() returns -EIO up front,
- enic_open() returns -EIO before it touches any datapath resource,
- enic_reset() and enic_tx_hang_reset() abort at
enic_admin_chan_reopen(), which also re-checks the flag under
mbox_state_lock and turns a successful re-registration into
-EAGAIN.
So a single timeout leaves the VF with carrier down, vf_rx_quarantined
set, and "ip link set dev X down; ip link set dev X up" failing with
-EIO until the driver is unloaded or the device re-probed. Given that
ENIC_MBOX_POLL_TIMEOUT_US is a bounded poll against a PF that may just
be slow or busy, that seems like a heavy penalty for one lost send
completion.
The field comment says the channel must not be reopened "until
reset/DMA fencing is proven", but the reset path in this same patch
looks like it provides exactly that: enic_admin_channel_close() stops
the admin QP and runs vnic_wq_clean(&enic->admin_wq,
enic_admin_wq_buf_clean), which is what the commit message calls the
teardown that reclaims the timed-out mapping, and enic_reset() then
runs enic_dev_soft_reset() plus the new vnic_dev_init() before
anything is reopened. If that sequence is sufficient to retire the
hardware-owned descriptor, should the poison be cleared there rather
than being permanent? If it is not sufficient, could you spell out
what is still unfenced after the soft reset, and how an operator is
expected to get the VF back without a module reload?
Related: on the timeout path the kmalloc'd buffer and its
dma_map_single() mapping are deliberately leaked. If the channel can
never be reopened, is enic_admin_channel_close() still guaranteed to
run (and thus unmap it), or does this also leak the mapping until
device removal?
[Severity: High]
What happens to a plain PF (or dynamic/legacy VF) when either of these two
new aborts is taken? Before this change enic_reset() always ran
enic_init_vnic_resources() and enic_open() after the soft reset; now both
"goto reset_out" paths skip them, and reset_out only clears api_busy,
fires NETDEV_REBOOT and unlocks RTNL. The worker is not requeued.
enic_dev_soft_reset() is enic_dev_wait(vnic_dev_soft_reset,
vnic_dev_soft_reset_done, 0), so it returns -ETIMEDOUT after a couple of
seconds when firmware never completes, which is the unresponsive-firmware
case that triggers the reset in the first place.
After such an abort, netif_running() is still true while the vNIC is
disabled and unprogrammed, the IRQs are freed and every NAPI is left
disabled. The new double-stop guard is gated on enic_is_sriov_vf_v2(),
and vf_datapath_open is not even maintained for other device types, so
the next ndo_stop() or _enic_change_mtu():
if (running) {
err = enic_stop(netdev);
re-runs the full teardown and calls napi_disable() a second time, which
spins in napi_disable_locked() with RTNL held, and enic_free_intr()
re-issues free_irq() on already freed IRQs. Should these abort paths
either complete the reopen or at least mark the datapath as already
stopped for all device types?
> @@ -2333,21 +2466,30 @@ static void enic_tx_hang_reset(struct work_struct *work)
> enic_dev_set_ig_vlan_rewrite_mode(enic);
> enic_ext_cq(enic);
>
> - enic_open(enic->netdev);
> + if (enic_is_sriov_vf_v2(enic)) {
> + err = enic_admin_chan_reopen(enic);
> + if (err)
> + goto hang_reset_out;
> + }
[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index b8a18d9682b2f..2d77d41577a3a 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
[ ... ]
> @@ -193,6 +305,26 @@ static int enic_mbox_vf_send_request(struct enic *enic, u8 request_type,
> expected_reply);
> }
>
> +static void enic_mbox_vf_mark_reconnect_locked(struct enic *enic,
> + bool registration_lost)
> +{
> + lockdep_assert_held(&enic->mbox_state_lock);
> +
> + if (registration_lost)
> + WRITE_ONCE(enic->vf_registered, false);
> + enic->vf_mbox_fault_generation++;
> + WRITE_ONCE(enic->vf_mbox_reconnect_required, true);
> + WRITE_ONCE(enic->mbox_send_disabled, true);
> + WRITE_ONCE(enic->vf_rx_quarantined, true);
> +}
> +
> +static void enic_mbox_vf_kick_recovery(struct enic *enic)
> +{
> + enic_mbox_vf_link_state_set_running(enic, false);
> + if (netif_running(enic->netdev))
> + schedule_work(&enic->reset);
> +}
[Severity: High]
Is there anything that bounds how often this re-arms enic->reset? There
is no attempt counter, no delay and no "recovery already in progress"
check, and the only gate is netif_running(), which the new
vf_datapath_open comment documents as staying true across an internal
reset.
The reply handlers that call it run while the reset worker is itself
executing:
enic_reset()
enic_admin_chan_reopen()
enic_mbox_vf_capability_check()
enic_mbox_vf_register()
enic_mbox_vf_handle_reply()
if (recovery != ENIC_MBOX_VF_REPLY_OK)
enic_mbox_vf_kick_recovery(enic);
Because the PENDING bit was cleared when the worker started, that
schedule_work() queues another full reset. If the PF keeps rejecting the
handshake, for example a REGISTER reply carrying
ENIC_MBOX_ERR_VF_NOT_REGISTERED, or a malformed reply, does this become a
self-sustaining reset loop where every iteration takes rtnl_lock() across
admin-channel teardown, rebuild, a soft reset and synchronous mailbox
transactions?
enic_admin_rx_lost() in enic_admin_rq_cq_service() adds a per-descriptor
trigger for the same path, so a burst of bad descriptors can re-arm it
repeatedly within one poll. Would a retry budget plus a delayed requeue
be appropriate here?
> @@ -600,6 +742,8 @@ static void enic_mbox_recv_handler(struct enic *enic, void *buf,
> netdev_warn(enic->netdev,
> "MBOX: truncated message (len %u < %zu)\n",
> len, sizeof(*hdr));
> + if (!enic->vf_state)
> + enic_mbox_vf_require_reconnect(enic);
> return;
> }
[Severity: Medium]
Does this act on a message before the sender has been validated? For
messages shorter than struct enic_mbox_hdr, enic_admin_rq_cq_service()
skips the hardware-attributed rewrite:
sender_vlan = le16_to_cpu(rq_desc->vlan);
if (bytes_written >= sizeof(struct enic_mbox_hdr)) {
struct enic_mbox_hdr *hdr = buf->os_buf;
hdr->src_vnic_id = (sender_vlan == 0) ?
cpu_to_le16(ENIC_MBOX_DST_PF) :
cpu_to_le16(sender_vlan - 1);
}
so src_vnic_id keeps whatever the sender put there, and the message is
still enqueued. enic_mbox_recv_handler() then calls
enic_mbox_vf_require_reconnect() ahead of the
le16_to_cpu(hdr->src_vnic_id) == ENIC_MBOX_DST_PF
check used for all normal dispatch. Via
enic_mbox_vf_mark_reconnect_locked() that disables mailbox sends,
quarantines RX and, through enic_mbox_vf_kick_recovery(), drops carrier
and schedules a device reset. The new enic_admin_rx_lost() calls for
descriptor errors and backlog drops are sender-agnostic in the same way.
Should the sender check happen before any state-destroying recovery
action, so that admin traffic not attributable to the PF cannot force a
VF into quarantine and reset?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-b4-enic-sriov-v2-vf-receive-control-v1-0-67a0a6e08d43%40cisco.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 4/6] enic: validate V2 VF configuration replies
2026-09-21 19:59 [PATCH net-next 0/6] enic: configure V2 VF addresses and receive mode Satish Kharat
` (2 preceding siblings ...)
2026-09-21 19:59 ` [PATCH net-next 3/6] enic: recover V2 VF mailbox when PF state is unknown Satish Kharat
@ 2026-09-21 19:59 ` Satish Kharat
2026-09-24 23:01 ` netdev-bot+sashiko
2026-09-21 19:59 ` [PATCH net-next 5/6] enic: manage V2 VF station and administrative MAC Satish Kharat
2026-09-21 19:59 ` [PATCH net-next 6/6] enic: configure V2 VF receive mode over mailbox Satish Kharat
5 siblings, 1 reply; 13+ messages in thread
From: Satish Kharat @ 2026-09-21 19:59 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman
Cc: netdev, linux-kernel, Satish Kharat, sebaddel
Add the established V2 mailbox operations for MAC filters,
administrative-MAC notifications, and packet-filter settings. Keep request
buffers alive until the VF request completes so detailed replies can be
checked entry by entry.
Validate message framing, echoed operations, result counts,
operation-specific idempotent results, and applied packet-filter flags
before publishing a reply. If a reply is malformed or contradictory, or
the PF reports that the VF is no longer registered, the VF can no longer
trust that its state matches the PF. Require a new VF registration before
accepting traffic again.
The protocol's ret_minor field counts non-SKIPPED per-entry result codes,
including the idempotent DUPLICATE and NOT_FOUND outcomes. SKIPPED remains
an operation-specific policy result but is not part of that aggregate
count.
Return stable policy errors to callers while retaining retry semantics for
operations that made no state change.
This patch adds only the VF side of these operations. The in-tree V2 PF
enable path remains dormant because enic_driver does not yet register
.sriov_configure. A future PF activation series must implement PF-side
handling for the MAC-address and packet-filter mailbox requests, including
the VF policy checks, before wiring that callback.
Assisted-by: LLM
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/net/ethernet/cisco/enic/enic.h | 9 +
drivers/net/ethernet/cisco/enic/enic_mbox.c | 492 +++++++++++++++++++++++++++-
drivers/net/ethernet/cisco/enic/enic_mbox.h | 82 +++++
3 files changed, 565 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 782b8b1843ab..45992b355501 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -239,6 +239,8 @@ enum enic_vf_type {
};
/* Per-instance private data structure */
+struct enic_mac_addr;
+
struct enic {
struct net_device *netdev;
struct pci_dev *pdev;
@@ -360,6 +362,13 @@ struct enic {
unsigned int vf_ack_count;
u64 mbox_expected_msg_num;
u8 mbox_expected_reply;
+ int mbox_reply_status;
+ u16 mbox_reply_filter_flags;
+ /* The request mutex keeps this caller-owned reply array alive until the
+ * matching reply handler has copied all per-address result flags.
+ */
+ struct enic_mac_addr *mbox_reply_mac_addrs;
+ u16 mbox_reply_mac_count;
bool mbox_initialized;
/* PF: per-VF MBOX state, allocated when SRIOV V2 is enabled */
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
index 2d77d41577a3..3f73072170b9 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
@@ -6,6 +6,7 @@
#include <linux/dma-mapping.h>
#include <linux/delay.h>
#include <linux/completion.h>
+#include <linux/etherdevice.h>
#include "vnic_dev.h"
#include "vnic_wq.h"
@@ -376,6 +377,82 @@ static void enic_mbox_vf_request_finish(struct enic *enic)
mutex_unlock(&enic->vf_mbox_request_lock);
}
+/* Return with mbox_state_lock held when this handler owns the reply. */
+static bool enic_mbox_vf_reply_claim(struct enic *enic, u8 reply_type,
+ u64 msg_num, u8 *expected)
+{
+ spin_lock_bh(&enic->mbox_state_lock);
+ *expected = enic->mbox_expected_reply;
+ if (*expected == reply_type &&
+ enic->mbox_expected_msg_num == msg_num)
+ return true;
+ spin_unlock_bh(&enic->mbox_state_lock);
+
+ return false;
+}
+
+enum enic_mbox_vf_reply_recovery {
+ ENIC_MBOX_VF_REPLY_OK,
+ ENIC_MBOX_VF_REPLY_RECONNECT,
+ ENIC_MBOX_VF_REPLY_REGISTRATION_LOST,
+};
+
+static int
+enic_mbox_vf_classify_reply(bool malformed, u16 ret_major,
+ enum enic_mbox_vf_reply_recovery *recovery)
+{
+ *recovery = ENIC_MBOX_VF_REPLY_OK;
+ if (malformed) {
+ *recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
+ return -EIO;
+ }
+ /* Some deployed peers return a negative errno in this 16-bit field.
+ * Interpret protocol bits only when no unknown bits are present; otherwise
+ * an errno such as -EINVAL could accidentally look like registration loss.
+ */
+ if (!(ret_major & ~ENIC_MBOX_ERR_MASK) &&
+ (ret_major & ENIC_MBOX_ERR_VF_NOT_REGISTERED)) {
+ *recovery = ENIC_MBOX_VF_REPLY_REGISTRATION_LOST;
+ return -ENODEV;
+ }
+ if (!(ret_major & ~ENIC_MBOX_ERR_MASK) &&
+ (ret_major & ENIC_MBOX_ERR_MSG_NOT_SUPPORTED))
+ return -EOPNOTSUPP;
+ if (ret_major)
+ return -EIO;
+
+ return 0;
+}
+
+static void
+enic_mbox_vf_recover_reply_locked(struct enic *enic,
+ enum enic_mbox_vf_reply_recovery recovery)
+{
+ bool registration_lost;
+
+ lockdep_assert_held(&enic->mbox_state_lock);
+
+ if (recovery != ENIC_MBOX_VF_REPLY_OK) {
+ registration_lost =
+ recovery == ENIC_MBOX_VF_REPLY_REGISTRATION_LOST;
+ enic_mbox_vf_mark_reconnect_locked(enic, registration_lost);
+ }
+}
+
+static void enic_mbox_vf_reply_complete(struct enic *enic)
+{
+ lockdep_assert_held(&enic->mbox_state_lock);
+ enic->mbox_expected_reply = 0;
+ enic->mbox_expected_msg_num = 0;
+ /* Publish completion before releasing the state lock. A waiter that
+ * hit the timeout boundary may otherwise see the claimed state, finish the
+ * request, and let a new request reinitialize this completion before the
+ * old handler signals it.
+ */
+ complete(&enic->mbox_comp);
+ spin_unlock_bh(&enic->mbox_state_lock);
+}
+
int enic_mbox_send_link_state(struct enic *enic, u16 vf_id, u32 link_state)
{
struct enic_mbox_pf_link_state_notif_msg notif = {};
@@ -551,23 +628,21 @@ static void enic_mbox_vf_handle_reply(struct enic *enic, u8 reply_type,
void *payload, u64 msg_num)
{
struct enic_mbox_generic_reply *reply = payload;
+ enum enic_mbox_vf_reply_recovery recovery;
u16 ret_major = le16_to_cpu(reply->ret_major);
- u64 expected_msg_num;
- u8 expected_type;
+ u8 expected;
+ int status;
- spin_lock_bh(&enic->mbox_state_lock);
- expected_type = enic->mbox_expected_reply;
- expected_msg_num = enic->mbox_expected_msg_num;
- if (expected_type != reply_type || expected_msg_num != msg_num) {
- spin_unlock_bh(&enic->mbox_state_lock);
+ status = enic_mbox_vf_classify_reply(false, ret_major, &recovery);
+ if (!enic_mbox_vf_reply_claim(enic, reply_type, msg_num, &expected)) {
netdev_warn(enic->netdev,
- "MBOX: stale reply %u/%llu (expected %u/%llu), drop\n",
+ "MBOX: stale reply %u/%llu (expected %u), drop\n",
reply_type, (unsigned long long)msg_num,
- expected_type, (unsigned long long)expected_msg_num);
+ expected);
return;
}
- if (!ret_major) {
+ if (!status) {
switch (reply_type) {
case ENIC_MBOX_VF_CAPABILITY_REPLY: {
struct enic_mbox_vf_capability_reply_msg *cap = payload;
@@ -584,16 +659,168 @@ static void enic_mbox_vf_handle_reply(struct enic *enic, u8 reply_type,
break;
}
}
- enic->mbox_expected_reply = 0;
- enic->mbox_expected_msg_num = 0;
- complete(&enic->mbox_comp);
- spin_unlock_bh(&enic->mbox_state_lock);
+ enic_mbox_vf_recover_reply_locked(enic, recovery);
+ WRITE_ONCE(enic->mbox_reply_status, status);
+ enic_mbox_vf_reply_complete(enic);
if (ret_major)
netdev_warn(enic->netdev,
"MBOX: PF rejected reply type %u: %u/%u\n",
reply_type, ret_major,
le16_to_cpu(reply->ret_minor));
+ if (recovery != ENIC_MBOX_VF_REPLY_OK)
+ enic_mbox_vf_kick_recovery(enic);
+}
+
+static bool enic_mbox_vf_mac_reply_matches(const struct enic_mac_addr *request,
+ const struct enic_mac_addr *reply)
+{
+ u16 request_flags = le16_to_cpu(request->flags);
+ u16 reply_flags = le16_to_cpu(reply->flags);
+ u16 idempotent_result = reply_flags &
+ (ENIC_MAC_ADDR_FLAG_DUPLICATE |
+ ENIC_MAC_ADDR_FLAG_NOT_FOUND);
+ u16 result = reply_flags & ENIC_MAC_ADDR_FLAG_REPLY_MASK;
+ u16 expected_result;
+
+ if (!ether_addr_equal(request->addr, reply->addr) ||
+ (request_flags & ENIC_MAC_ADDR_FLAG_REQUEST_MASK) !=
+ (reply_flags & ENIC_MAC_ADDR_FLAG_REQUEST_MASK))
+ return false;
+ if (hweight16(result) > 1)
+ return false;
+
+ /* DUPLICATE is a successful ADD result and NOT_FOUND is a successful
+ * DELETE result. Neither is valid for the opposite operation, and a
+ * reply cannot report both outcomes for one entry.
+ */
+ expected_result = request_flags & ENIC_MAC_ADDR_FLAG_ADD ?
+ ENIC_MAC_ADDR_FLAG_DUPLICATE :
+ ENIC_MAC_ADDR_FLAG_NOT_FOUND;
+
+ return !idempotent_result || idempotent_result == expected_result;
+}
+
+static void enic_mbox_vf_handle_add_del_mac_reply(struct enic *enic,
+ void *payload, u16 msg_len,
+ u64 msg_num)
+{
+ struct enic_mbox_vf_add_del_mac_reply_msg *reply = payload;
+ enum enic_mbox_vf_reply_recovery recovery;
+ u16 reported_errors = 0;
+ u16 num_addrs = 0;
+ u16 ret_minor = 0;
+ u16 ret_major = 0;
+ u8 expected;
+ unsigned int i;
+ int status;
+
+ if (msg_len < sizeof(*reply)) {
+ status = enic_mbox_vf_classify_reply(true, 0, &recovery);
+ } else {
+ ret_major = le16_to_cpu(reply->reply.ret_major);
+ ret_minor = le16_to_cpu(reply->reply.ret_minor);
+ status = enic_mbox_vf_classify_reply(false, ret_major,
+ &recovery);
+ if (status == -EIO)
+ recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
+ }
+ if (status)
+ goto claim;
+
+ num_addrs = le16_to_cpu(reply->num_addrs);
+ if (!num_addrs || num_addrs > ENIC_MBOX_MAX_MAC_OPS ||
+ struct_size(reply, mac_addr, num_addrs) > msg_len) {
+ status = enic_mbox_vf_classify_reply(true, 0, &recovery);
+ goto claim;
+ }
+
+claim:
+ if (!enic_mbox_vf_reply_claim(enic, ENIC_MBOX_VF_ADD_DEL_MAC_REPLY,
+ msg_num, &expected))
+ return;
+ if (!status &&
+ (num_addrs != enic->mbox_reply_mac_count ||
+ !enic->mbox_reply_mac_addrs)) {
+ status = enic_mbox_vf_classify_reply(true, 0, &recovery);
+ } else if (!status) {
+ /* A detailed reply corresponds entry-for-entry with the request.
+ * Validate the echoed request fields and operation-specific results
+ * before exposing result flags to the waiting caller.
+ */
+ for (i = 0; i < num_addrs; i++) {
+ struct enic_mac_addr *request =
+ &enic->mbox_reply_mac_addrs[i];
+ u16 flags = le16_to_cpu(reply->mac_addr[i].flags);
+
+ if (!enic_mbox_vf_mac_reply_matches(request,
+ &reply->mac_addr[i])) {
+ status = enic_mbox_vf_classify_reply(true, 0,
+ &recovery);
+ break;
+ }
+ if (flags & ENIC_MAC_ADDR_FLAG_INDETERMINATE_MASK) {
+ status = -EIO;
+ recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
+ break;
+ }
+ if ((flags & ENIC_MAC_ADDR_FLAG_REPLY_MASK) &&
+ !(flags & ENIC_MAC_ADDR_FLAG_SKIPPED))
+ reported_errors++;
+ }
+ if (!status && reported_errors != ret_minor)
+ status = enic_mbox_vf_classify_reply(true, 0,
+ &recovery);
+
+ if (!status)
+ for (i = 0; i < num_addrs; i++)
+ enic->mbox_reply_mac_addrs[i].flags =
+ reply->mac_addr[i].flags;
+ }
+ /* After a malformed reply, the VF cannot trust that its state matches the
+ * PF. VF_NOT_REGISTERED means the PF removed all VF state. Both require a
+ * new registration; an ordinary policy rejection does not.
+ */
+ enic_mbox_vf_recover_reply_locked(enic, recovery);
+ WRITE_ONCE(enic->mbox_reply_status, status);
+ enic_mbox_vf_reply_complete(enic);
+ if (recovery != ENIC_MBOX_VF_REPLY_OK)
+ enic_mbox_vf_kick_recovery(enic);
+}
+
+static void enic_mbox_vf_handle_set_pkt_filter_reply(struct enic *enic,
+ void *payload, u16 msg_len,
+ u64 msg_num)
+{
+ struct enic_mbox_vf_set_pkt_filter_reply_msg *reply = payload;
+ enum enic_mbox_vf_reply_recovery recovery;
+ u16 applied = 0;
+ u16 ret_major = 0;
+ u8 expected;
+ int status;
+
+ if (msg_len < sizeof(*reply)) {
+ status = enic_mbox_vf_classify_reply(true, 0, &recovery);
+ } else {
+ ret_major = le16_to_cpu(reply->reply.ret_major);
+ status = enic_mbox_vf_classify_reply(false, ret_major,
+ &recovery);
+ if (status && (ret_major & ~ENIC_MBOX_ERR_MASK))
+ recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
+ }
+ if (!status)
+ applied = le16_to_cpu(reply->reply.ret_minor);
+
+ if (!enic_mbox_vf_reply_claim(enic,
+ ENIC_MBOX_VF_SET_PKT_FILTER_REPLY,
+ msg_num, &expected))
+ return;
+ enic_mbox_vf_recover_reply_locked(enic, recovery);
+ WRITE_ONCE(enic->mbox_reply_status, status);
+ WRITE_ONCE(enic->mbox_reply_filter_flags, applied);
+ enic_mbox_vf_reply_complete(enic);
+ if (recovery != ENIC_MBOX_VF_REPLY_OK)
+ enic_mbox_vf_kick_recovery(enic);
}
static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload,
@@ -679,6 +906,34 @@ static bool enic_mbox_vf_payload_ok(struct enic *enic, u8 msg_type,
return true;
}
+static void enic_mbox_vf_malformed_msg(struct enic *enic, u8 msg_type,
+ u64 msg_num)
+{
+ u8 expected;
+
+ switch (msg_type) {
+ case ENIC_MBOX_PF_LINK_STATE_NOTIF:
+ case ENIC_MBOX_PF_SET_ADMIN_MAC_NOTIF:
+ enic_mbox_vf_require_reconnect(enic);
+ return;
+ case ENIC_MBOX_VF_CAPABILITY_REPLY:
+ case ENIC_MBOX_VF_REGISTER_REPLY:
+ case ENIC_MBOX_VF_UNREGISTER_REPLY:
+ case ENIC_MBOX_VF_ADD_DEL_MAC_REPLY:
+ case ENIC_MBOX_VF_SET_PKT_FILTER_REPLY:
+ break;
+ default:
+ return;
+ }
+
+ if (!enic_mbox_vf_reply_claim(enic, msg_type, msg_num, &expected))
+ return;
+ enic_mbox_vf_mark_reconnect_locked(enic, false);
+ WRITE_ONCE(enic->mbox_reply_status, -EIO);
+ enic_mbox_vf_reply_complete(enic);
+ enic_mbox_vf_kick_recovery(enic);
+}
+
static void enic_mbox_vf_process_msg(struct enic *enic,
struct enic_mbox_hdr *hdr, void *payload,
u16 payload_len)
@@ -690,8 +945,10 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
size_t exp = sizeof(struct enic_mbox_vf_capability_reply_msg);
if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
- payload_len, exp))
+ payload_len, exp)) {
+ enic_mbox_vf_malformed_msg(enic, hdr->msg_type, msg_num);
return;
+ }
enic_mbox_vf_handle_reply(enic, hdr->msg_type, payload, msg_num);
break;
}
@@ -699,8 +956,10 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
size_t exp = sizeof(struct enic_mbox_vf_register_reply_msg);
if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
- payload_len, exp))
+ payload_len, exp)) {
+ enic_mbox_vf_malformed_msg(enic, hdr->msg_type, msg_num);
return;
+ }
enic_mbox_vf_handle_reply(enic, hdr->msg_type, payload, msg_num);
break;
}
@@ -708,8 +967,10 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
size_t exp = sizeof(struct enic_mbox_vf_register_reply_msg);
if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
- payload_len, exp))
+ payload_len, exp)) {
+ enic_mbox_vf_malformed_msg(enic, hdr->msg_type, msg_num);
return;
+ }
enic_mbox_vf_handle_reply(enic, hdr->msg_type, payload, msg_num);
break;
}
@@ -717,11 +978,21 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
size_t exp = sizeof(struct enic_mbox_pf_link_state_notif_msg);
if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
- payload_len, exp))
+ payload_len, exp)) {
+ enic_mbox_vf_malformed_msg(enic, hdr->msg_type, msg_num);
return;
+ }
enic_mbox_vf_handle_link_state(enic, payload, msg_num);
break;
}
+ case ENIC_MBOX_VF_ADD_DEL_MAC_REPLY:
+ enic_mbox_vf_handle_add_del_mac_reply(enic, payload,
+ payload_len, msg_num);
+ break;
+ case ENIC_MBOX_VF_SET_PKT_FILTER_REPLY:
+ enic_mbox_vf_handle_set_pkt_filter_reply(enic, payload,
+ payload_len, msg_num);
+ break;
default:
netdev_dbg(enic->netdev,
"MBOX: VF unhandled msg type %u\n",
@@ -761,6 +1032,10 @@ static void enic_mbox_recv_handler(struct enic *enic, void *buf,
netdev_warn(enic->netdev,
"MBOX: invalid msg_len %u (buf len %u)\n",
msg_len, len);
+ if (!enic->vf_state &&
+ le16_to_cpu(hdr->src_vnic_id) == ENIC_MBOX_DST_PF)
+ enic_mbox_vf_malformed_msg(enic, hdr->msg_type,
+ le64_to_cpu(hdr->msg_num));
return;
}
@@ -791,10 +1066,12 @@ int enic_mbox_vf_capability_check(struct enic *enic)
{
struct enic_mbox_vf_capability_msg req = {};
u32 version;
+ int status;
int err;
enic_mbox_vf_request_start(enic);
WRITE_ONCE(enic->pf_cap_version, 0);
+ WRITE_ONCE(enic->mbox_reply_status, 0);
req.version = cpu_to_le32(ENIC_MBOX_CAP_VERSION_1);
err = enic_mbox_vf_send_request(enic,
@@ -808,12 +1085,15 @@ int enic_mbox_vf_capability_check(struct enic *enic)
err = enic_mbox_wait_reply(enic, 3000);
version = READ_ONCE(enic->pf_cap_version);
+ status = READ_ONCE(enic->mbox_reply_status);
enic_mbox_vf_request_finish(enic);
if (err) {
netdev_warn(enic->netdev,
"MBOX: no capability reply from PF\n");
return err;
}
+ if (status)
+ return status;
if (version < ENIC_MBOX_CAP_VERSION_1) {
netdev_warn(enic->netdev,
@@ -828,10 +1108,12 @@ int enic_mbox_vf_capability_check(struct enic *enic)
int enic_mbox_vf_register(struct enic *enic)
{
bool registered;
+ int status;
int err;
enic_mbox_vf_request_start(enic);
WRITE_ONCE(enic->vf_registered, false);
+ WRITE_ONCE(enic->mbox_reply_status, 0);
err = enic_mbox_vf_send_request(enic, ENIC_MBOX_VF_REGISTER_REQUEST,
ENIC_MBOX_VF_REGISTER_REPLY, NULL, 0);
@@ -842,12 +1124,15 @@ int enic_mbox_vf_register(struct enic *enic)
err = enic_mbox_wait_reply(enic, 3000);
registered = READ_ONCE(enic->vf_registered);
+ status = READ_ONCE(enic->mbox_reply_status);
enic_mbox_vf_request_finish(enic);
if (err) {
netdev_warn(enic->netdev,
"MBOX: VF registration with PF timed out\n");
return err;
}
+ if (status)
+ return status;
if (!registered)
return -ENODEV;
@@ -858,15 +1143,18 @@ int enic_mbox_vf_register(struct enic *enic)
int enic_mbox_vf_unregister(struct enic *enic)
{
bool registered;
+ int status;
int err;
if (!READ_ONCE(enic->vf_registered))
return 0;
+
enic_mbox_vf_request_start(enic);
if (!READ_ONCE(enic->vf_registered)) {
enic_mbox_vf_request_finish(enic);
return 0;
}
+ WRITE_ONCE(enic->mbox_reply_status, 0);
err = enic_mbox_vf_send_request(enic,
ENIC_MBOX_VF_UNREGISTER_REQUEST,
@@ -879,14 +1167,182 @@ int enic_mbox_vf_unregister(struct enic *enic)
err = enic_mbox_wait_reply(enic, 3000);
registered = READ_ONCE(enic->vf_registered);
+ status = READ_ONCE(enic->mbox_reply_status);
enic_mbox_vf_request_finish(enic);
if (err)
return err;
+ if (status)
+ return status;
if (registered)
return -EACCES;
return 0;
}
+int enic_mbox_vf_add_del_macs(struct enic *enic,
+ struct enic_mac_addr *macs, u16 num_macs)
+{
+ struct enic_mbox_vf_add_del_mac_msg *req;
+ unsigned int i;
+ int status;
+ int err;
+
+ if (!READ_ONCE(enic->vf_registered) || !enic->has_admin_channel)
+ return -ENODEV;
+ if (!num_macs || num_macs > ENIC_MBOX_MAX_MAC_OPS)
+ return -EINVAL;
+
+ req = kzalloc_flex(*req, mac_addr, num_macs);
+ if (!req)
+ return -ENOMEM;
+
+ req->num_addrs = cpu_to_le16(num_macs);
+ for (i = 0; i < num_macs; i++)
+ req->mac_addr[i] = macs[i];
+
+ enic_mbox_vf_request_start(enic);
+ if (!READ_ONCE(enic->vf_registered) || !enic->has_admin_channel) {
+ err = -ENODEV;
+ } else {
+ spin_lock_bh(&enic->mbox_state_lock);
+ enic->mbox_reply_mac_addrs = macs;
+ enic->mbox_reply_mac_count = num_macs;
+ spin_unlock_bh(&enic->mbox_state_lock);
+ WRITE_ONCE(enic->mbox_reply_status, 0);
+ err = enic_mbox_vf_send_request(enic,
+ ENIC_MBOX_VF_ADD_DEL_MAC_REQUEST,
+ ENIC_MBOX_VF_ADD_DEL_MAC_REPLY,
+ req,
+ struct_size(req, mac_addr,
+ num_macs));
+ }
+ kfree(req);
+ if (err) {
+ spin_lock_bh(&enic->mbox_state_lock);
+ enic->mbox_reply_mac_addrs = NULL;
+ enic->mbox_reply_mac_count = 0;
+ spin_unlock_bh(&enic->mbox_state_lock);
+ enic_mbox_vf_request_abort(enic);
+ return err;
+ }
+
+ err = enic_mbox_wait_reply(enic, 3000);
+ status = READ_ONCE(enic->mbox_reply_status);
+ spin_lock_bh(&enic->mbox_state_lock);
+ enic->mbox_reply_mac_addrs = NULL;
+ enic->mbox_reply_mac_count = 0;
+ spin_unlock_bh(&enic->mbox_state_lock);
+ if (err) {
+ /* The PF may have updated its software ledger before a hardware
+ * failure whose reply was lost. A repeated idempotent operation could
+ * then appear converged while hardware state is stale, so replay from
+ * a fresh registration generation.
+ */
+ enic_mbox_vf_require_reconnect(enic);
+ enic_mbox_vf_request_finish(enic);
+ return err;
+ }
+ enic_mbox_vf_request_finish(enic);
+
+ return status;
+}
+
+int enic_mbox_vf_add_del_mac(struct enic *enic, const u8 *addr, bool add,
+ bool station)
+{
+ struct enic_mac_addr mac = {};
+ u16 flags = 0;
+ int err;
+
+ ether_addr_copy(mac.addr, addr);
+ if (add)
+ flags |= ENIC_MAC_ADDR_FLAG_ADD;
+ if (station)
+ flags |= ENIC_MAC_ADDR_FLAG_STATION;
+ mac.flags = cpu_to_le16(flags);
+
+ err = enic_mbox_vf_add_del_macs(enic, &mac, 1);
+ if (err)
+ return err;
+ if (le16_to_cpu(mac.flags) & ENIC_MAC_ADDR_FLAG_ERROR_MASK)
+ return -EACCES;
+
+ return 0;
+}
+
+int enic_mbox_vf_set_pkt_filter(struct enic *enic, int directed,
+ int multicast, int broadcast,
+ int promisc, int allmulti, u16 *applied_flags)
+{
+ struct enic_mbox_vf_set_pkt_filter_msg req = {};
+ u16 applied;
+ u16 flags = 0;
+ u16 required;
+ int status;
+ int err;
+
+ if (!READ_ONCE(enic->vf_registered) || !enic->has_admin_channel)
+ return -ENODEV;
+
+ if (directed)
+ flags |= CMD_PFILTER_DIRECTED;
+ if (multicast)
+ flags |= CMD_PFILTER_MULTICAST;
+ if (broadcast)
+ flags |= CMD_PFILTER_BROADCAST;
+ if (promisc)
+ flags |= CMD_PFILTER_PROMISCUOUS;
+ if (allmulti)
+ flags |= CMD_PFILTER_ALL_MULTICAST;
+ req.flags = cpu_to_le16(flags);
+
+ enic_mbox_vf_request_start(enic);
+ if (!READ_ONCE(enic->vf_registered) || !enic->has_admin_channel) {
+ enic_mbox_vf_request_abort(enic);
+ return -ENODEV;
+ }
+ WRITE_ONCE(enic->mbox_reply_status, 0);
+ WRITE_ONCE(enic->mbox_reply_filter_flags, 0);
+
+ err = enic_mbox_vf_send_request(enic,
+ ENIC_MBOX_VF_SET_PKT_FILTER_REQUEST,
+ ENIC_MBOX_VF_SET_PKT_FILTER_REPLY,
+ &req, sizeof(req));
+ if (err) {
+ enic_mbox_vf_request_abort(enic);
+ return err;
+ }
+
+ err = enic_mbox_wait_reply(enic, 3000);
+ status = READ_ONCE(enic->mbox_reply_status);
+ if (!err && !status) {
+ applied = READ_ONCE(enic->mbox_reply_filter_flags);
+ /* Directed, multicast, and broadcast are not policy-gated. The PF
+ * may only withhold the two broad receive modes, and may add directed
+ * reception because it is mandatory for a usable VF.
+ */
+ required = (flags | CMD_PFILTER_DIRECTED) &
+ ~(CMD_PFILTER_PROMISCUOUS |
+ CMD_PFILTER_ALL_MULTICAST);
+ if ((applied & ~(flags | CMD_PFILTER_DIRECTED)) ||
+ (applied & required) != required) {
+ netdev_warn(enic->netdev,
+ "MBOX: invalid packet filter reply %#x for request %#x\n",
+ applied, flags);
+ enic_mbox_vf_require_reconnect(enic);
+ status = -EIO;
+ } else if (applied_flags) {
+ *applied_flags = applied;
+ }
+ }
+ if (err)
+ enic_mbox_vf_require_reconnect(enic);
+ enic_mbox_vf_request_finish(enic);
+ if (err)
+ return err;
+
+ return status;
+}
+
void enic_mbox_init(struct enic *enic)
{
bool reinit = enic->mbox_initialized;
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.h b/drivers/net/ethernet/cisco/enic/enic_mbox.h
index 37bc41a900f4..5eca3a25671e 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.h
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.h
@@ -5,6 +5,7 @@
#define _ENIC_MBOX_H_
#include <linux/bits.h>
+#include <linux/if_ether.h>
#include <linux/types.h>
/*
@@ -22,6 +23,12 @@ enum enic_mbox_msg_type {
ENIC_MBOX_VF_UNREGISTER_REPLY = 5,
ENIC_MBOX_PF_LINK_STATE_NOTIF = 6,
ENIC_MBOX_PF_LINK_STATE_ACK = 7,
+ ENIC_MBOX_VF_ADD_DEL_MAC_REQUEST = 10,
+ ENIC_MBOX_VF_ADD_DEL_MAC_REPLY = 11,
+ ENIC_MBOX_PF_SET_ADMIN_MAC_NOTIF = 12,
+ ENIC_MBOX_PF_SET_ADMIN_MAC_ACK = 13,
+ ENIC_MBOX_VF_SET_PKT_FILTER_REQUEST = 14,
+ ENIC_MBOX_VF_SET_PKT_FILTER_REPLY = 15,
ENIC_MBOX_MAX
};
@@ -42,6 +49,9 @@ struct enic_mbox_generic_reply {
#define ENIC_MBOX_ERR_GENERIC BIT(0)
#define ENIC_MBOX_ERR_VF_NOT_REGISTERED BIT(1)
#define ENIC_MBOX_ERR_MSG_NOT_SUPPORTED BIT(2)
+#define ENIC_MBOX_ERR_MASK (ENIC_MBOX_ERR_GENERIC | \
+ ENIC_MBOX_ERR_VF_NOT_REGISTERED | \
+ ENIC_MBOX_ERR_MSG_NOT_SUPPORTED)
/* ENIC_MBOX_VF_CAPABILITY_REQUEST / _REPLY */
#define ENIC_MBOX_CAP_VERSION_0 0
@@ -80,6 +90,71 @@ struct enic_mbox_pf_link_state_ack_msg {
struct enic_mbox_generic_reply ack;
};
+/* ENIC_MBOX_PF_SET_ADMIN_MAC_NOTIF / _ACK */
+struct enic_mbox_pf_set_admin_mac_notif_msg {
+ u8 mac_addr[ETH_ALEN];
+ __le16 pad;
+};
+
+/* ENIC_MBOX_VF_ADD_DEL_MAC_REQUEST / _REPLY */
+#define ENIC_MAC_ADDR_FLAG_ADD BIT(0)
+#define ENIC_MAC_ADDR_FLAG_STATION BIT(1)
+#define ENIC_MAC_ADDR_FLAG_OVERFLOW BIT(8)
+#define ENIC_MAC_ADDR_FLAG_DUPLICATE BIT(9)
+#define ENIC_MAC_ADDR_FLAG_FAILED BIT(10)
+#define ENIC_MAC_ADDR_FLAG_NOT_FOUND BIT(11)
+#define ENIC_MAC_ADDR_FLAG_ERROR BIT(12)
+#define ENIC_MAC_ADDR_FLAG_NOT_PERMITTED BIT(13)
+#define ENIC_MAC_ADDR_FLAG_INVALID BIT(14)
+#define ENIC_MAC_ADDR_FLAG_SKIPPED BIT(15)
+
+#define ENIC_MAC_ADDR_FLAG_REQUEST_MASK GENMASK(7, 0)
+#define ENIC_MAC_ADDR_FLAG_REPLY_MASK GENMASK(15, 8)
+#define ENIC_MAC_ADDR_FLAG_INDETERMINATE_MASK \
+ (ENIC_MAC_ADDR_FLAG_FAILED | ENIC_MAC_ADDR_FLAG_ERROR)
+#define ENIC_MAC_ADDR_FLAG_PERMANENT_MASK \
+ (ENIC_MAC_ADDR_FLAG_OVERFLOW | ENIC_MAC_ADDR_FLAG_NOT_PERMITTED | \
+ ENIC_MAC_ADDR_FLAG_INVALID)
+#define ENIC_MAC_ADDR_FLAG_ERROR_MASK (ENIC_MAC_ADDR_FLAG_OVERFLOW | \
+ ENIC_MAC_ADDR_FLAG_FAILED | \
+ ENIC_MAC_ADDR_FLAG_ERROR | \
+ ENIC_MAC_ADDR_FLAG_NOT_PERMITTED | \
+ ENIC_MAC_ADDR_FLAG_INVALID | \
+ ENIC_MAC_ADDR_FLAG_SKIPPED)
+
+/* The protocol permits replacing all perfect filters and the station address
+ * in one request: one delete and one add operation for each address.
+ */
+#define ENIC_MBOX_MAX_MAC_OPS 130
+
+struct enic_mac_addr {
+ u8 addr[ETH_ALEN];
+ __le16 flags;
+};
+
+struct enic_mbox_vf_add_del_mac_msg {
+ __le16 num_addrs;
+ __le16 pad;
+ struct enic_mac_addr mac_addr[];
+};
+
+struct enic_mbox_vf_add_del_mac_reply_msg {
+ struct enic_mbox_generic_reply reply;
+ __le16 num_addrs;
+ __le16 pad;
+ struct enic_mac_addr mac_addr[];
+};
+
+/* ENIC_MBOX_VF_SET_PKT_FILTER_REQUEST / _REPLY */
+struct enic_mbox_vf_set_pkt_filter_msg {
+ __le16 flags;
+ __le16 pad;
+};
+
+struct enic_mbox_vf_set_pkt_filter_reply_msg {
+ struct enic_mbox_generic_reply reply;
+};
+
#define ENIC_MBOX_DST_PF 0xFFFF
struct enic;
@@ -95,5 +170,12 @@ void enic_mbox_vf_require_reconnect(struct enic *enic);
int enic_mbox_vf_capability_check(struct enic *enic);
int enic_mbox_vf_register(struct enic *enic);
int enic_mbox_vf_unregister(struct enic *enic);
+int enic_mbox_vf_add_del_macs(struct enic *enic,
+ struct enic_mac_addr *macs, u16 num_macs);
+int enic_mbox_vf_add_del_mac(struct enic *enic, const u8 *addr, bool add,
+ bool station);
+int enic_mbox_vf_set_pkt_filter(struct enic *enic, int directed, int multicast,
+ int broadcast, int promisc, int allmulti,
+ u16 *applied_flags);
#endif /* _ENIC_MBOX_H_ */
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 4/6] enic: validate V2 VF configuration replies
2026-09-21 19:59 ` [PATCH net-next 4/6] enic: validate V2 VF configuration replies Satish Kharat
@ 2026-09-24 23:01 ` netdev-bot+sashiko
0 siblings, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 23:01 UTC (permalink / raw)
To: satishkh
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, netdev,
linux-kernel, sebaddel
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 4 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] enic_mbox_vf_handle_add_del_mac_reply()
(drivers/net/ethernet/cisco/enic/enic_mbox.c:725-726) unconditionally…
- [Medium] enic_mbox_vf_handle_reply() now ends with `if (recovery !=
ENIC_MBOX_VF_REPLY_OK) enic_mbox_vf_kick_recovery(enic);`, which fires…
- [Low] ENIC_MBOX_PF_SET_ADMIN_MAC_NOTIF (12) / _ACK (13) are added to
the message enum and the notification type is listed in…
- [Low] The three new externally visible VF entry points
(enic_mbox_vf_add_del_macs(), enic_mbox_vf_add_del_mac(),…
- [Low] enic_mbox_vf_malformed_msg() turns any single mis-framed admin-
channel message attributed to the PF into a full protocol fault…
- [Low] enic_mbox_vf_reply_claim()
(drivers/net/ethernet/cisco/enic/enic_mbox.c:381) returns with…
Pre-existing issues:
- [Low] The VF now emits ENIC_MBOX_VF_ADD_DEL_MAC_REQUEST (10) and
ENIC_MBOX_VF_SET_PKT_FILTER_REQUEST (14), but…
--
enic: validate V2 VF configuration replies
This adds the V2 mailbox operations for MAC filters, administrative-MAC
notifications and packet-filter settings on the VF side, and keeps the
request buffer alive so per-entry reply results can be validated. On a
malformed or contradictory reply, or when the PF reports the VF is no longer
registered, the VF forces a new registration before accepting traffic again.
A few questions about the new recovery escalation and the lock handoff
between the claim and completion helpers.
> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index 2d77d41577a3a..3f73072170b91 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
[ ... ]
> @@ -376,6 +377,82 @@ static void enic_mbox_vf_request_finish(struct enic *enic)
> mutex_unlock(&enic->vf_mbox_request_lock);
> }
>
> +/* Return with mbox_state_lock held when this handler owns the reply. */
> +static bool enic_mbox_vf_reply_claim(struct enic *enic, u8 reply_type,
> + u64 msg_num, u8 *expected)
> +{
> + spin_lock_bh(&enic->mbox_state_lock);
> + *expected = enic->mbox_expected_reply;
> + if (*expected == reply_type &&
> + enic->mbox_expected_msg_num == msg_num)
> + return true;
> + spin_unlock_bh(&enic->mbox_state_lock);
> +
> + return false;
> +}
> +
> +enum enic_mbox_vf_reply_recovery {
> + ENIC_MBOX_VF_REPLY_OK,
> + ENIC_MBOX_VF_REPLY_RECONNECT,
> + ENIC_MBOX_VF_REPLY_REGISTRATION_LOST,
> +};
> +
> +static int
> +enic_mbox_vf_classify_reply(bool malformed, u16 ret_major,
> + enum enic_mbox_vf_reply_recovery *recovery)
> +{
> + *recovery = ENIC_MBOX_VF_REPLY_OK;
> + if (malformed) {
> + *recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
> + return -EIO;
> + }
> + /* Some deployed peers return a negative errno in this 16-bit field.
> + * Interpret protocol bits only when no unknown bits are present; otherwise
> + * an errno such as -EINVAL could accidentally look like registration loss.
> + */
> + if (!(ret_major & ~ENIC_MBOX_ERR_MASK) &&
> + (ret_major & ENIC_MBOX_ERR_VF_NOT_REGISTERED)) {
> + *recovery = ENIC_MBOX_VF_REPLY_REGISTRATION_LOST;
> + return -ENODEV;
> + }
> + if (!(ret_major & ~ENIC_MBOX_ERR_MASK) &&
> + (ret_major & ENIC_MBOX_ERR_MSG_NOT_SUPPORTED))
> + return -EOPNOTSUPP;
> + if (ret_major)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static void
> +enic_mbox_vf_recover_reply_locked(struct enic *enic,
> + enum enic_mbox_vf_reply_recovery recovery)
> +{
> + bool registration_lost;
> +
> + lockdep_assert_held(&enic->mbox_state_lock);
> +
> + if (recovery != ENIC_MBOX_VF_REPLY_OK) {
> + registration_lost =
> + recovery == ENIC_MBOX_VF_REPLY_REGISTRATION_LOST;
> + enic_mbox_vf_mark_reconnect_locked(enic, registration_lost);
> + }
> +}
> +
> +static void enic_mbox_vf_reply_complete(struct enic *enic)
> +{
> + lockdep_assert_held(&enic->mbox_state_lock);
> + enic->mbox_expected_reply = 0;
> + enic->mbox_expected_msg_num = 0;
> + /* Publish completion before releasing the state lock. A waiter that
> + * hit the timeout boundary may otherwise see the claimed state, finish the
> + * request, and let a new request reinitialize this completion before the
> + * old handler signals it.
> + */
> + complete(&enic->mbox_comp);
> + spin_unlock_bh(&enic->mbox_state_lock);
> +}
[Severity: Low]
Should this conditional acquire be annotated? enic_mbox_vf_reply_claim()
returns with enic->mbox_state_lock held on the true path and unlocked on
the false path, and the matching release happens in a different function,
enic_mbox_vf_reply_complete().
Neither helper carries __cond_acquires()/__acquires()/__releases(), and
spin_lock_bh() is annotated __acquires(lock) in this tree, so sparse and
clang context analysis cannot pair the acquire with the release for the
claim helper or for its callers (enic_mbox_vf_handle_reply,
enic_mbox_vf_handle_add_del_mac_reply,
enic_mbox_vf_handle_set_pkt_filter_reply, enic_mbox_vf_malformed_msg).
I traced all four callers and every true-branch path does reach
enic_mbox_vf_reply_complete(), so this is balanced at runtime today. Would
__cond_acquires(true, &enic->mbox_state_lock) on the claim helper and
__releases(&enic->mbox_state_lock) on the completion helper be preferable,
or could the lock be taken and dropped inside a single function? The
longest region between claim and complete walks up to
ENIC_MBOX_MAX_MAC_OPS (130) entries under spin_lock_bh, so any future early
return added there would leak the lock with BHs disabled.
> @@ -551,23 +628,21 @@ static void enic_mbox_vf_handle_reply(struct enic *enic, u8 reply_type,
> void *payload, u64 msg_num)
> {
> struct enic_mbox_generic_reply *reply = payload;
> + enum enic_mbox_vf_reply_recovery recovery;
> u16 ret_major = le16_to_cpu(reply->ret_major);
> - u64 expected_msg_num;
> - u8 expected_type;
> + u8 expected;
> + int status;
>
> + status = enic_mbox_vf_classify_reply(false, ret_major, &recovery);
> + if (!enic_mbox_vf_reply_claim(enic, reply_type, msg_num, &expected)) {
[ ... ]
> @@ -584,16 +659,168 @@ static void enic_mbox_vf_handle_reply(struct enic *enic, u8 reply_type,
> break;
> }
> }
> - enic->mbox_expected_reply = 0;
> - enic->mbox_expected_msg_num = 0;
> - complete(&enic->mbox_comp);
> - spin_unlock_bh(&enic->mbox_state_lock);
> + enic_mbox_vf_recover_reply_locked(enic, recovery);
> + WRITE_ONCE(enic->mbox_reply_status, status);
> + enic_mbox_vf_reply_complete(enic);
>
> if (ret_major)
> netdev_warn(enic->netdev,
> "MBOX: PF rejected reply type %u: %u/%u\n",
> reply_type, ret_major,
> le16_to_cpu(reply->ret_minor));
> + if (recovery != ENIC_MBOX_VF_REPLY_OK)
> + enic_mbox_vf_kick_recovery(enic);
> +}
[Severity: Medium]
Can this new kick re-queue the reset work from inside the reset work?
The CAPABILITY and REGISTER requests are issued from
enic_admin_chan_reopen(), which runs in the enic_reset() work item, and
enic_reset() only returns early when !netif_running(), which stays true
across an internal reset. If the reply carries
ENIC_MBOX_ERR_VF_NOT_REGISTERED, enic_mbox_vf_classify_reply() reports
ENIC_MBOX_VF_REPLY_REGISTRATION_LOST, so:
enic_reset() /* holds rtnl_lock */
enic_admin_chan_reopen()
enic_mbox_vf_capability_check()
enic_mbox_vf_handle_reply()
enic_mbox_vf_kick_recovery()
schedule_work(&enic->reset); /* re-queues the running work */
The work bit is cleared before the callback runs, so this re-queues rather
than being a no-op. With a PF that keeps answering the handshake with
VF_NOT_REGISTERED, does the device then cycle reset -> soft reset -> reopen
-> handshake failure -> reset with no fault budget or back-off, where the
baseline simply left the device down? Each iteration also holds rtnl_lock
across up to two 3000 ms mailbox waits.
This one depends on peer behaviour I could not confirm against real PF
firmware, so it may not be reachable in practice, but there is no
structural termination for the loop in the code.
> +
> +static bool enic_mbox_vf_mac_reply_matches(const struct enic_mac_addr *request,
> + const struct enic_mac_addr *reply)
> +{
[ ... ]
> +static void enic_mbox_vf_handle_add_del_mac_reply(struct enic *enic,
> + void *payload, u16 msg_len,
> + u64 msg_num)
> +{
> + struct enic_mbox_vf_add_del_mac_reply_msg *reply = payload;
> + enum enic_mbox_vf_reply_recovery recovery;
[ ... ]
> + if (msg_len < sizeof(*reply)) {
> + status = enic_mbox_vf_classify_reply(true, 0, &recovery);
> + } else {
> + ret_major = le16_to_cpu(reply->reply.ret_major);
> + ret_minor = le16_to_cpu(reply->reply.ret_minor);
> + status = enic_mbox_vf_classify_reply(false, ret_major,
> + &recovery);
> + if (status == -EIO)
> + recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
> + }
[Severity: Medium]
Does this override also escalate an ordinary policy rejection?
For a well-formed reply whose ret_major carries only
ENIC_MBOX_ERR_GENERIC, enic_mbox_vf_classify_reply() takes its last arm:
if (ret_major)
return -EIO;
leaving *recovery == ENIC_MBOX_VF_REPLY_OK. The unconditional
"if (status == -EIO)" above then rewrites that to
ENIC_MBOX_VF_REPLY_RECONNECT, so enic_mbox_vf_recover_reply_locked() ->
enic_mbox_vf_mark_reconnect_locked() sets mbox_send_disabled and
vf_rx_quarantined, bumps vf_mbox_fault_generation, and
enic_mbox_vf_kick_recovery() schedules enic->reset.
That appears to contradict the comment further down in this same function:
/* After a malformed reply, the VF cannot trust that its state matches the
* PF. VF_NOT_REGISTERED means the PF removed all VF state. Both require a
* new registration; an ordinary policy rejection does not.
*/
and the commit message, which says "Return stable policy errors to callers
while retaining retry semantics for operations that made no state change."
The sibling handler added by this same patch escalates only for ret_major
bits outside the protocol mask:
if (status && (ret_major & ~ENIC_MBOX_ERR_MASK))
recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
Should the MAC handler use the same narrower rule? Later in the series the
station-address install (enic_vf_station_addr_replace()) re-issues this
request on every open/reopen, so a PF that stably answers ERR_GENERIC would
keep re-arming the soft reset under rtnl_lock with RX quarantined.
[ ... ]
> +static void enic_mbox_vf_handle_set_pkt_filter_reply(struct enic *enic,
> + void *payload, u16 msg_len,
> + u64 msg_num)
> +{
[ ... ]
> + if (msg_len < sizeof(*reply)) {
> + status = enic_mbox_vf_classify_reply(true, 0, &recovery);
> + } else {
> + ret_major = le16_to_cpu(reply->reply.ret_major);
> + status = enic_mbox_vf_classify_reply(false, ret_major,
> + &recovery);
> + if (status && (ret_major & ~ENIC_MBOX_ERR_MASK))
> + recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
> + }
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-b4-enic-sriov-v2-vf-receive-control-v1-0-67a0a6e08d43%40cisco.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 5/6] enic: manage V2 VF station and administrative MAC
2026-09-21 19:59 [PATCH net-next 0/6] enic: configure V2 VF addresses and receive mode Satish Kharat
` (3 preceding siblings ...)
2026-09-21 19:59 ` [PATCH net-next 4/6] enic: validate V2 VF configuration replies Satish Kharat
@ 2026-09-21 19:59 ` Satish Kharat
2026-09-24 23:01 ` netdev-bot+sashiko
2026-09-21 19:59 ` [PATCH net-next 6/6] enic: configure V2 VF receive mode over mailbox Satish Kharat
5 siblings, 1 reply; 13+ messages in thread
From: Satish Kharat @ 2026-09-21 19:59 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman
Cc: netdev, linux-kernel, Satish Kharat, sebaddel
Use the V2 mailbox MAC operation for the VF station address and keep that
address separate from netdev secondary-unicast synchronization. Replace a
station address with one mailbox request containing a delete followed by an
add. If the PF reports that only part of the replacement was applied,
require the VF to register again before accepting traffic.
Treat the PF administrative MAC as authoritative. Process notifications
outside receive dispatch, preserve a delegated VF-selected address, refresh
the policy after every registration, and protect VF worker shutdown and
restart against teardown and recovery.
Install the station address during open and remove it during an ordinary
stop. At reset and removal boundaries, rely on VF_UNREGISTER or the next VF
registration to clear the old state.
Assisted-by: LLM
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/net/ethernet/cisco/enic/enic.h | 17 +
drivers/net/ethernet/cisco/enic/enic_admin.c | 2 +
drivers/net/ethernet/cisco/enic/enic_dev.c | 11 +
drivers/net/ethernet/cisco/enic/enic_dev.h | 1 +
drivers/net/ethernet/cisco/enic/enic_main.c | 566 ++++++++++++++++++++++++++-
drivers/net/ethernet/cisco/enic/enic_mbox.c | 30 ++
6 files changed, 623 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 45992b355501..0f0ef68130f2 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -340,6 +340,9 @@ struct enic {
spinlock_t vf_link_state_lock;
enum enic_vf_link_state vf_link_state;
bool vf_link_running;
+ /* Last station address which may still be installed at the PF. */
+ u8 vf_station_addr[ETH_ALEN] __aligned(2);
+ bool vf_station_addr_valid;
/* Tracks a completely opened V2 VF datapath. An internal reset can stop
* it while netif_running() remains true, then fail before reopen.
*/
@@ -360,6 +363,16 @@ struct enic {
struct list_head vf_ack_list;
struct work_struct vf_ack_work;
unsigned int vf_ack_count;
+ struct delayed_work vf_admin_mac_work;
+ spinlock_t vf_admin_mac_lock; /* protects pending admin MAC */
+ u8 vf_admin_mac[ETH_ALEN];
+ u8 vf_admin_mac_random_addr[ETH_ALEN];
+ bool vf_admin_mac_pending;
+ bool vf_admin_mac_random_valid;
+ bool vf_admin_mac_work_enabled;
+ bool vf_admin_mac_recovery_attempted;
+ u32 vf_admin_mac_generation;
+ u8 vf_admin_mac_retries;
u64 mbox_expected_msg_num;
u8 mbox_expected_reply;
int mbox_reply_status;
@@ -493,6 +506,10 @@ static inline int enic_dma_map_check(struct enic *enic, dma_addr_t dma_addr)
}
void enic_reset_addr_lists(struct enic *enic);
+void enic_vf_admin_mac_notify(struct enic *enic, const u8 *addr);
+void enic_vf_admin_mac_quiesce(struct enic *enic);
+void enic_vf_admin_mac_rearm(struct enic *enic);
+void enic_vf_admin_mac_purge(struct enic *enic);
int enic_sriov_enabled(struct enic *enic);
int enic_is_valid_vf(struct enic *enic, int vf);
int enic_is_dynamic(struct enic *enic);
diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
index 30c0a5c89a2d..5e52d4da8d3c 100644
--- a/drivers/net/ethernet/cisco/enic/enic_admin.c
+++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
@@ -641,6 +641,8 @@ void enic_admin_channel_close(struct enic *enic)
{
int err;
+ if (enic_is_sriov_vf_v2(enic))
+ enic_vf_admin_mac_quiesce(enic);
if (!enic->has_admin_channel)
return;
diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.c b/drivers/net/ethernet/cisco/enic/enic_dev.c
index 659787f73cf1..48c1ca9b40fb 100644
--- a/drivers/net/ethernet/cisco/enic/enic_dev.c
+++ b/drivers/net/ethernet/cisco/enic/enic_dev.c
@@ -32,6 +32,17 @@ int enic_dev_stats_dump(struct enic *enic, struct vnic_stats **vstats)
return err;
}
+int enic_dev_get_mac_addr(struct enic *enic, u8 *mac_addr)
+{
+ int err;
+
+ spin_lock_bh(&enic->devcmd_lock);
+ err = vnic_dev_get_mac_addr(enic->vdev, mac_addr);
+ spin_unlock_bh(&enic->devcmd_lock);
+
+ return err;
+}
+
int enic_dev_add_station_addr(struct enic *enic)
{
int err;
diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.h b/drivers/net/ethernet/cisco/enic/enic_dev.h
index 698d0cb02064..66d0e51681f5 100644
--- a/drivers/net/ethernet/cisco/enic/enic_dev.h
+++ b/drivers/net/ethernet/cisco/enic/enic_dev.h
@@ -26,6 +26,7 @@
int enic_dev_fw_info(struct enic *enic, struct vnic_devcmd_fw_info **fw_info);
int enic_dev_stats_dump(struct enic *enic, struct vnic_stats **vstats);
+int enic_dev_get_mac_addr(struct enic *enic, u8 *mac_addr);
int enic_dev_add_station_addr(struct enic *enic);
int enic_dev_del_station_addr(struct enic *enic);
int enic_dev_packet_filter(struct enic *enic, int directed, int multicast,
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 1c60e7a666ae..16412c29e932 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -89,6 +89,9 @@ MODULE_DEVICE_TABLE(pci, enic_id_table);
#define ENIC_LARGE_PKT_THRESHOLD 1000
#define ENIC_MAX_COALESCE_TIMERS 10
+#define ENIC_VF_ADMIN_MAC_MAX_RETRIES 4
+#define ENIC_VF_ADMIN_MAC_RETRY_MS 100
+#define ENIC_VF_ADMIN_MAC_REFRESH_RETRIES 3
/* Interrupt moderation table, which will be used to decide the
* coalescing timer values
* {rx_rate in Mbps, mapping percentage of the range}
@@ -1040,8 +1043,10 @@ void enic_reset_addr_lists(struct enic *enic)
{
struct net_device *netdev = enic->netdev;
+ netif_addr_lock_bh(netdev);
__dev_uc_unsync(netdev, NULL);
__dev_mc_unsync(netdev, NULL);
+ netif_addr_unlock_bh(netdev);
enic->mc_count = 0;
enic->uc_count = 0;
@@ -1065,6 +1070,456 @@ static int enic_set_mac_addr(struct net_device *netdev, char *addr)
return 0;
}
+static void enic_vf_station_recovery_required(struct enic *enic)
+{
+ enic_mbox_vf_require_reconnect(enic);
+}
+
+static void enic_vf_station_addr_set(struct enic *enic, const u8 *addr)
+{
+ ether_addr_copy(enic->vf_station_addr, addr);
+ enic->vf_station_addr_valid = true;
+}
+
+static int enic_vf_keep_nonstation_sync(struct net_device *netdev,
+ const u8 *addr)
+{
+ struct enic *enic = netdev_priv(netdev);
+
+ if (!ether_addr_equal(addr, enic->vf_station_addr))
+ return -ENOENT;
+ if (WARN_ON_ONCE(!enic->uc_count))
+ return 0;
+ enic->uc_count--;
+
+ return 0;
+}
+
+static void enic_vf_station_sync_reset(struct enic *enic)
+{
+ if (!enic->vf_station_addr_valid)
+ return;
+
+ /* The PF keys its MAC ledger by address. A station entry is also the
+ * receive filter for that address and must not retain a second core
+ * synchronization reference which could later delete the station.
+ */
+ netif_addr_lock_bh(enic->netdev);
+ __dev_uc_unsync(enic->netdev, enic_vf_keep_nonstation_sync);
+ netif_addr_unlock_bh(enic->netdev);
+}
+
+static int enic_vf_station_addr_del(struct enic *enic)
+{
+ int err;
+
+ if (!enic->vf_station_addr_valid)
+ return 0;
+
+ err = enic_mbox_vf_add_del_mac(enic, enic->vf_station_addr,
+ false, true);
+ if (!err) {
+ enic->vf_station_addr_valid = false;
+ } else if (READ_ONCE(enic->mbox_tx_poisoned)) {
+ enic_mbox_vf_link_state_set_running(enic, false);
+ } else if (err == -EACCES) {
+ /* A definitive rejected DELETE leaves an unrequested station
+ * filter installed. Fresh registration is the cleanup boundary.
+ */
+ enic_vf_station_recovery_required(enic);
+ }
+
+ return err;
+}
+
+/* End every replacement with an equal-address DEL+ADD pair. A bare ADD could
+ * report DUPLICATE for a secondary exact filter without applying the current
+ * station-address policy. If another station is tracked, delete it first in
+ * the same compound request.
+ */
+static int enic_vf_station_addr_replace(struct enic *enic, const u8 *addr)
+{
+ struct enic_mac_addr macs[3] = {};
+ bool all_deletes_skipped = true;
+ bool deletes_converged = true;
+ u16 num_macs = 0;
+ unsigned int i;
+ u16 result;
+ int err;
+
+ if (enic->vf_station_addr_valid &&
+ !ether_addr_equal(enic->vf_station_addr, addr)) {
+ ether_addr_copy(macs[num_macs].addr, enic->vf_station_addr);
+ macs[num_macs++].flags =
+ cpu_to_le16(ENIC_MAC_ADDR_FLAG_STATION);
+ }
+ ether_addr_copy(macs[num_macs].addr, addr);
+ macs[num_macs++].flags = cpu_to_le16(ENIC_MAC_ADDR_FLAG_STATION);
+ ether_addr_copy(macs[num_macs].addr, addr);
+ macs[num_macs++].flags = cpu_to_le16(ENIC_MAC_ADDR_FLAG_ADD |
+ ENIC_MAC_ADDR_FLAG_STATION);
+
+ err = enic_mbox_vf_add_del_macs(enic, macs, num_macs);
+ if (err) {
+ if (READ_ONCE(enic->mbox_tx_poisoned))
+ enic_mbox_vf_link_state_set_running(enic, false);
+ return err;
+ }
+
+ for (i = 0; i < num_macs - 1; i++) {
+ result = le16_to_cpu(macs[i].flags) &
+ ENIC_MAC_ADDR_FLAG_REPLY_MASK;
+ if (result != ENIC_MAC_ADDR_FLAG_SKIPPED)
+ all_deletes_skipped = false;
+ if (result && result != ENIC_MAC_ADDR_FLAG_NOT_FOUND)
+ deletes_converged = false;
+ }
+ result = le16_to_cpu(macs[num_macs - 1].flags) &
+ ENIC_MAC_ADDR_FLAG_REPLY_MASK;
+ if (deletes_converged && !result)
+ return 0;
+
+ /* A policy-rejected ADD can be reported with all preceding DELETEs
+ * skipped. No operation changed state in that coherent result tuple.
+ */
+ if (all_deletes_skipped && result) {
+ if (result & (ENIC_MAC_ADDR_FLAG_DUPLICATE |
+ ENIC_MAC_ADDR_FLAG_PERMANENT_MASK))
+ return -EACCES;
+ return -EIO;
+ }
+
+ /* After any other partial or contradictory result, the VF does not know
+ * which station address the PF kept. Re-register before accepting traffic.
+ */
+ enic_vf_station_recovery_required(enic);
+ for (i = 0; i < num_macs; i++)
+ if (le16_to_cpu(macs[i].flags) &
+ ENIC_MAC_ADDR_FLAG_PERMANENT_MASK)
+ return -EACCES;
+
+ return -EIO;
+}
+
+static void enic_vf_admin_mac_cache_selected(struct enic *enic,
+ const u8 *addr)
+{
+ /* A zero administrative policy delegates the operational address to
+ * the VF. Preserve a successful user selection across reconnects.
+ */
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ if (is_zero_ether_addr(enic->vf_admin_mac)) {
+ ether_addr_copy(enic->vf_admin_mac_random_addr, addr);
+ enic->vf_admin_mac_random_valid = true;
+ }
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+}
+
+static void enic_vf_admin_mac_work(struct work_struct *work)
+{
+ struct enic *enic = container_of(to_delayed_work(work), struct enic,
+ vf_admin_mac_work);
+ u8 policy[ETH_ALEN];
+ u8 selected[ETH_ALEN];
+ unsigned long delay = 0;
+ u32 generation = 0;
+ bool changed;
+ bool mutated = false;
+ bool reschedule = false;
+ bool station_installed = false;
+ bool stale = false;
+ bool zero_policy;
+ bool pending;
+ int err = 0;
+
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ pending = enic->vf_admin_mac_pending &&
+ enic->vf_admin_mac_work_enabled;
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+ if (!pending)
+ return;
+
+ /* Teardown owns RTNL while synchronously cancelling this work. Do not
+ * block that owner.
+ */
+ if (!rtnl_trylock()) {
+ mod_delayed_work(system_wq, &enic->vf_admin_mac_work,
+ msecs_to_jiffies(10));
+ return;
+ }
+
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ pending = enic->vf_admin_mac_pending &&
+ enic->vf_admin_mac_work_enabled;
+ if (pending) {
+ ether_addr_copy(policy, enic->vf_admin_mac);
+ generation = enic->vf_admin_mac_generation;
+ zero_policy = is_zero_ether_addr(policy);
+ if (zero_policy && enic->vf_admin_mac_random_valid)
+ ether_addr_copy(selected,
+ enic->vf_admin_mac_random_addr);
+ }
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+ if (!pending)
+ goto unlock;
+
+ if (zero_policy && !READ_ONCE(enic->vf_admin_mac_random_valid)) {
+ eth_random_addr(selected);
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ if (!enic->vf_admin_mac_pending ||
+ !enic->vf_admin_mac_work_enabled ||
+ enic->vf_admin_mac_generation != generation ||
+ !is_zero_ether_addr(enic->vf_admin_mac)) {
+ stale = true;
+ } else if (enic->vf_admin_mac_random_valid) {
+ ether_addr_copy(selected,
+ enic->vf_admin_mac_random_addr);
+ } else {
+ ether_addr_copy(enic->vf_admin_mac_random_addr, selected);
+ enic->vf_admin_mac_random_valid = true;
+ }
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+ if (stale)
+ goto unlock;
+ } else if (!zero_policy) {
+ ether_addr_copy(selected, policy);
+ }
+
+ if (!netif_device_present(enic->netdev) ||
+ !READ_ONCE(enic->vf_registered)) {
+ err = -EAGAIN;
+ goto unlock;
+ }
+
+ /* A nonzero notification describes a station address the PF has already
+ * installed. Zero delegates selection to the VF, which registers the
+ * selected random address through the policy-safe replacement request.
+ */
+ if (!READ_ONCE(enic->vf_datapath_open)) {
+ station_installed = !zero_policy &&
+ (!enic->vf_station_addr_valid ||
+ ether_addr_equal(enic->vf_station_addr, selected));
+ } else if (zero_policy) {
+ mutated = true;
+ err = enic_vf_station_addr_replace(enic, selected);
+ station_installed = !err;
+ } else if (enic->vf_station_addr_valid &&
+ !ether_addr_equal(enic->vf_station_addr, selected)) {
+ mutated = true;
+ err = enic_vf_station_addr_del(enic);
+ station_installed = !err;
+ } else {
+ station_installed = true;
+ }
+ if (err)
+ goto unlock;
+
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ if (!enic->vf_admin_mac_pending ||
+ !enic->vf_admin_mac_work_enabled ||
+ enic->vf_admin_mac_generation != generation) {
+ stale = true;
+ /* If policy changed while the request was running, the completed
+ * station change may no longer match it. Record recovery while holding
+ * vf_admin_mac_lock, which also protects VF worker shutdown. Teardown
+ * therefore either suppresses the update or sees it first, and already
+ * uses unregister/register to clear old state.
+ */
+ if (mutated && enic->vf_admin_mac_work_enabled)
+ enic_vf_station_recovery_required(enic);
+ } else {
+ enic->vf_admin_mac_pending = false;
+ enic->vf_admin_mac_retries = 0;
+ enic->vf_admin_mac_recovery_attempted = false;
+ if (!zero_policy)
+ enic->vf_admin_mac_random_valid = false;
+ }
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+ if (stale)
+ goto unlock;
+
+ changed = !ether_addr_equal(enic->netdev->dev_addr, selected);
+ enic_vf_station_sync_reset(enic);
+ eth_hw_addr_set(enic->netdev, selected);
+ if (!zero_policy || changed)
+ enic->netdev->addr_assign_type = zero_policy ?
+ NET_ADDR_RANDOM : NET_ADDR_SET;
+ ether_addr_copy(enic->mac_addr, selected);
+ if (station_installed)
+ enic_vf_station_addr_set(enic, selected);
+ enic_vf_station_sync_reset(enic);
+ if (enic->netdev->reg_state == NETREG_REGISTERED && changed)
+ call_netdevice_notifiers(NETDEV_CHANGEADDR, enic->netdev);
+ netdev_info(enic->netdev, "MBOX: admin MAC set to %pM\n", selected);
+
+unlock:
+ rtnl_unlock();
+
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ if (err && enic->vf_admin_mac_pending &&
+ enic->vf_admin_mac_work_enabled &&
+ generation == enic->vf_admin_mac_generation &&
+ !READ_ONCE(enic->mbox_send_disabled)) {
+ if (enic->vf_admin_mac_retries <
+ ENIC_VF_ADMIN_MAC_MAX_RETRIES) {
+ enic->vf_admin_mac_retries++;
+ reschedule = true;
+ delay = msecs_to_jiffies(ENIC_VF_ADMIN_MAC_RETRY_MS);
+ } else if (!enic->vf_admin_mac_recovery_attempted) {
+ enic->vf_admin_mac_recovery_attempted = true;
+ /* vf_admin_mac_lock also protects VF worker shutdown. */
+ enic_vf_station_recovery_required(enic);
+ }
+ }
+ if (enic->vf_admin_mac_pending &&
+ enic->vf_admin_mac_work_enabled &&
+ generation != enic->vf_admin_mac_generation) {
+ reschedule = true;
+ delay = 0;
+ }
+ pending = enic->vf_admin_mac_pending;
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+ if (pending && reschedule)
+ mod_delayed_work(system_wq, &enic->vf_admin_mac_work, delay);
+}
+
+void enic_vf_admin_mac_notify(struct enic *enic, const u8 *addr)
+{
+ bool new_policy;
+
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ new_policy = !enic->vf_admin_mac_pending ||
+ !ether_addr_equal(enic->vf_admin_mac, addr);
+ if (new_policy) {
+ ether_addr_copy(enic->vf_admin_mac, addr);
+ if (!is_zero_ether_addr(addr))
+ enic->vf_admin_mac_random_valid = false;
+ enic->vf_admin_mac_pending = true;
+ enic->vf_admin_mac_generation++;
+ enic->vf_admin_mac_retries = 0;
+ enic->vf_admin_mac_recovery_attempted = false;
+ }
+ if (new_policy && enic->vf_admin_mac_work_enabled)
+ mod_delayed_work(system_wq, &enic->vf_admin_mac_work, 0);
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+}
+
+void enic_vf_admin_mac_quiesce(struct enic *enic)
+{
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ enic->vf_admin_mac_work_enabled = false;
+ enic->vf_admin_mac_generation++;
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+ cancel_delayed_work_sync(&enic->vf_admin_mac_work);
+}
+
+void enic_vf_admin_mac_rearm(struct enic *enic)
+{
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ enic->vf_admin_mac_work_enabled = true;
+ if (enic->vf_admin_mac_pending)
+ mod_delayed_work(system_wq, &enic->vf_admin_mac_work, 0);
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+}
+
+void enic_vf_admin_mac_purge(struct enic *enic)
+{
+ enic_vf_admin_mac_quiesce(enic);
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ enic->vf_admin_mac_pending = false;
+ enic->vf_admin_mac_random_valid = false;
+ enic->vf_admin_mac_recovery_attempted = false;
+ enic->vf_admin_mac_retries = 0;
+ enic->vf_admin_mac_generation++;
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+}
+
+/* CMD_GET_MAC_ADDR is the PF-owned policy store. Re-read it after REGISTER so
+ * a lost notification cannot leave the VF using stale administrative policy.
+ */
+static int enic_vf_admin_mac_refresh(struct enic *enic)
+{
+ u8 previous_policy[ETH_ALEN];
+ u8 random_addr[ETH_ALEN];
+ u8 selected[ETH_ALEN];
+ u8 policy[ETH_ALEN];
+ u32 generation;
+ bool apply_policy;
+ bool changed;
+ bool pending;
+ bool policy_changed;
+ bool zero_policy;
+ unsigned int attempt;
+ int err;
+
+ for (attempt = 0; attempt < ENIC_VF_ADMIN_MAC_REFRESH_RETRIES;
+ attempt++) {
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ generation = enic->vf_admin_mac_generation;
+ pending = enic->vf_admin_mac_pending;
+ ether_addr_copy(previous_policy, enic->vf_admin_mac);
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+
+ err = enic_dev_get_mac_addr(enic, policy);
+ if (err)
+ return err;
+ if (is_multicast_ether_addr(policy))
+ return -EADDRNOTAVAIL;
+
+ zero_policy = is_zero_ether_addr(policy);
+ if (zero_policy)
+ eth_random_addr(random_addr);
+
+ spin_lock_bh(&enic->vf_admin_mac_lock);
+ if (enic->vf_admin_mac_generation != generation) {
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+ continue;
+ }
+
+ policy_changed = !ether_addr_equal(previous_policy, policy);
+ apply_policy = pending || policy_changed ||
+ !is_valid_ether_addr(enic->netdev->dev_addr);
+ if (!apply_policy) {
+ ether_addr_copy(selected, enic->netdev->dev_addr);
+ if (zero_policy) {
+ ether_addr_copy(enic->vf_admin_mac_random_addr,
+ selected);
+ enic->vf_admin_mac_random_valid = true;
+ }
+ } else if (zero_policy && enic->vf_admin_mac_random_valid) {
+ ether_addr_copy(selected,
+ enic->vf_admin_mac_random_addr);
+ } else if (zero_policy) {
+ ether_addr_copy(selected, random_addr);
+ ether_addr_copy(enic->vf_admin_mac_random_addr, selected);
+ enic->vf_admin_mac_random_valid = true;
+ } else {
+ ether_addr_copy(selected, policy);
+ enic->vf_admin_mac_random_valid = false;
+ }
+ ether_addr_copy(enic->vf_admin_mac, policy);
+ enic->vf_admin_mac_pending = false;
+ enic->vf_admin_mac_recovery_attempted = false;
+ enic->vf_admin_mac_retries = 0;
+ enic->vf_admin_mac_generation++;
+ spin_unlock_bh(&enic->vf_admin_mac_lock);
+
+ changed = !ether_addr_equal(enic->netdev->dev_addr, selected);
+ eth_hw_addr_set(enic->netdev, selected);
+ if (!zero_policy || changed)
+ enic->netdev->addr_assign_type = zero_policy ?
+ NET_ADDR_RANDOM : NET_ADDR_SET;
+ ether_addr_copy(enic->mac_addr, selected);
+ if (changed && enic->netdev->reg_state == NETREG_REGISTERED)
+ call_netdevice_notifiers(NETDEV_CHANGEADDR,
+ enic->netdev);
+
+ return 0;
+ }
+
+ return -EAGAIN;
+}
+
static int enic_set_mac_address_dynamic(struct net_device *netdev, void *p)
{
struct enic *enic = netdev_priv(netdev);
@@ -1072,6 +1527,45 @@ static int enic_set_mac_address_dynamic(struct net_device *netdev, void *p)
char *addr = saddr->sa_data;
int err;
+ if (enic_is_sriov_vf_v2(enic)) {
+ if (!is_valid_ether_addr(addr))
+ return -EADDRNOTAVAIL;
+ if (ether_addr_equal(addr, netdev->dev_addr))
+ return 0;
+
+ if (enic->vf_datapath_open &&
+ !READ_ONCE(enic->vf_registered))
+ return -ENODEV;
+
+ /* An internal reset keeps IFF_UP set while its failed reopen leaves
+ * the datapath closed. Cache the requested address in that state; the
+ * next successful open installs it together with the datapath.
+ */
+ if (!enic->vf_datapath_open) {
+ err = enic_set_mac_addr(netdev, addr);
+ if (!err)
+ enic_vf_admin_mac_cache_selected(enic, addr);
+ return err;
+ }
+
+ /* Keep the old software address visible until the complete station
+ * replacement proves convergence.
+ */
+ err = enic_vf_station_addr_replace(enic, addr);
+ if (err)
+ return err;
+
+ enic_vf_station_sync_reset(enic);
+ err = enic_set_mac_addr(netdev, addr);
+ if (!err) {
+ enic_vf_station_addr_set(enic, addr);
+ enic_vf_station_sync_reset(enic);
+ enic_vf_admin_mac_cache_selected(enic, addr);
+ }
+
+ return err;
+ }
+
if (netif_running(enic->netdev)) {
err = enic_dev_del_station_addr(enic);
if (err)
@@ -1726,6 +2220,7 @@ static int enic_admin_chan_reopen(struct enic *enic);
static int enic_open(struct net_device *netdev)
{
struct enic *enic = netdev_priv(netdev);
+ bool vf_mac_added = false;
unsigned int i;
int err, ret;
unsigned int max_pkt_len = netdev->mtu + VLAN_ETH_HLEN;
@@ -1807,6 +2302,25 @@ static int enic_open(struct net_device *netdev)
if (!enic_is_dynamic(enic) && !enic_is_sriov_vf(enic))
enic_dev_add_station_addr(enic);
+ if (enic_is_sriov_vf_v2(enic)) {
+ if (!READ_ONCE(enic->vf_registered)) {
+ netdev_err(netdev, "VF is not registered with its PF\n");
+ err = -ENODEV;
+ goto err_out_disable_wq;
+ }
+
+ err = enic_vf_station_addr_replace(enic, netdev->dev_addr);
+ if (err) {
+ netdev_err(netdev,
+ "Failed to register VF station address: %d\n",
+ err);
+ goto err_out_disable_wq;
+ }
+ enic_vf_station_addr_set(enic, netdev->dev_addr);
+ enic_vf_station_sync_reset(enic);
+ vf_mac_added = true;
+ }
+
enic_set_rx_mode(netdev);
netif_tx_wake_all_queues(netdev);
@@ -1864,6 +2378,15 @@ static int enic_open(struct net_device *netdev)
for (i = 0; i < enic->wq_count; i++)
napi_disable(&enic->napi[enic_cq_wq(enic, i)]);
netif_tx_disable(netdev);
+err_out_disable_wq:
+ if (vf_mac_added) {
+ ret = enic_vf_station_addr_del(enic);
+ if (ret) {
+ netdev_warn(netdev,
+ "Failed to remove VF station address during open rollback: %d\n",
+ ret);
+ }
+ }
if (!enic_is_dynamic(enic) && !enic_is_sriov_vf(enic))
enic_dev_del_station_addr(enic);
for (i = 0; i < enic->wq_count; i++)
@@ -1898,7 +2421,6 @@ static int __enic_stop(struct net_device *netdev, bool remove_vf_station)
*/
if (enic_is_sriov_vf_v2(enic) && !enic->vf_datapath_open)
return 0;
- (void)remove_vf_station;
for (i = 0; i < enic->intr_count; i++) {
vnic_intr_mask(&enic->intr[i]);
@@ -1923,6 +2445,15 @@ static int __enic_stop(struct net_device *netdev, bool remove_vf_station)
for (i = 0; i < enic->wq_count; i++)
napi_disable(&enic->napi[enic_cq_wq(enic, i)]);
netif_tx_disable(netdev);
+ if (remove_vf_station && enic_is_sriov_vf_v2(enic) &&
+ READ_ONCE(enic->vf_registered)) {
+ err = enic_vf_station_addr_del(enic);
+ if (err) {
+ netdev_warn(netdev,
+ "Failed to remove VF station address: %d\n",
+ err);
+ }
+ }
if (!enic_is_dynamic(enic) && !enic_is_sriov_vf(enic))
enic_dev_del_station_addr(enic);
@@ -2321,9 +2852,18 @@ static int enic_admin_chan_reopen(struct enic *enic)
return err;
}
enic_reset_addr_lists(enic);
+ enic->vf_station_addr_valid = false;
+ err = enic_vf_admin_mac_refresh(enic);
+ if (err) {
+ netdev_err(enic->netdev,
+ "Failed to refresh VF admin MAC after reset: %d\n",
+ err);
+ enic_admin_channel_close(enic);
+ return err;
+ }
/* Capability negotiation and registration establish a new protocol
- * generation. RX remains quarantined until enic_open() replays the
- * station and receive policy.
+ * generation after authoritative MAC policy is refreshed. RX remains
+ * quarantined until enic_open() replays station and receive policy.
*/
spin_lock_bh(&enic->mbox_state_lock);
if (enic->vf_mbox_fault_generation != recovery_generation ||
@@ -2339,6 +2879,7 @@ static int enic_admin_chan_reopen(struct enic *enic)
enic_admin_channel_close(enic);
return err;
}
+ enic_vf_admin_mac_rearm(enic);
} else {
/* The link came back up during enic_open() above while MBOX
* sends were still disabled (channel not yet reopened), so that
@@ -3322,6 +3863,9 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
* cancel_work_sync()) would otherwise act on an uninitialised work.
*/
INIT_WORK(&enic->link_notify_work, enic_link_notify_work_handler);
+ spin_lock_init(&enic->vf_admin_mac_lock);
+ INIT_DELAYED_WORK(&enic->vf_admin_mac_work,
+ enic_vf_admin_mac_work);
/* V2 VF: open admin channel and register with PF.
* Must happen before register_netdev so the VF is fully
@@ -3351,6 +3895,12 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
"MBOX VF registration failed: %d\n", err);
goto err_out_admin_close;
}
+ err = enic_vf_admin_mac_refresh(enic);
+ if (err) {
+ dev_err(dev,
+ "MBOX VF admin MAC refresh failed: %d\n", err);
+ goto err_out_admin_close;
+ }
}
netif_set_real_num_tx_queues(netdev, enic->wq_count);
@@ -3476,6 +4026,8 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
dev_err(dev, "Cannot register net device, aborting\n");
goto err_out_admin_close;
}
+ if (enic_is_sriov_vf_v2(enic))
+ enic_vf_admin_mac_rearm(enic);
return 0;
@@ -3531,9 +4083,13 @@ static void enic_remove(struct pci_dev *pdev)
/* Close the admin channel and unregister from the PF before
* unregister_netdev() to prevent a late PF notification from
- * touching a netdev that is being torn down.
+ * touching a netdev that is being torn down. VF_UNREGISTER is the
+ * protocol teardown operation: the PF removes all VF-requested
+ * configuration, including the station address, before replying.
*/
if (enic_is_sriov_vf_v2(enic)) {
+ enic_vf_admin_mac_quiesce(enic);
+
if (READ_ONCE(enic->vf_registered)) {
int unreg_err = enic_mbox_vf_unregister(enic);
@@ -3552,6 +4108,8 @@ static void enic_remove(struct pci_dev *pdev)
* enic_link_check() scheduled it just as SR-IOV was disabled.
*/
cancel_work_sync(&enic->link_notify_work);
+ if (enic_is_sriov_vf_v2(enic))
+ enic_vf_admin_mac_purge(enic);
#ifdef CONFIG_PCI_IOV
if (enic_sriov_enabled(enic)) {
if (enic->vf_type == ENIC_VF_TYPE_V2)
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
index 3f73072170b9..1c1d087fa966 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
@@ -861,6 +861,25 @@ static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload,
ret_major);
}
+static void enic_mbox_vf_handle_admin_mac(struct enic *enic, void *payload,
+ u64 msg_num)
+{
+ struct enic_mbox_pf_set_admin_mac_notif_msg *notif = payload;
+ u16 ret_major = 0;
+
+ if (is_multicast_ether_addr(notif->mac_addr)) {
+ netdev_warn(enic->netdev,
+ "MBOX: rejecting multicast admin MAC %pM\n",
+ notif->mac_addr);
+ ret_major = ENIC_MBOX_ERR_GENERIC;
+ } else {
+ enic_vf_admin_mac_notify(enic, notif->mac_addr);
+ }
+
+ enic_mbox_vf_queue_ack(enic, ENIC_MBOX_PF_SET_ADMIN_MAC_ACK,
+ msg_num, ret_major);
+}
+
void enic_mbox_vf_link_state_reset(struct enic *enic)
{
spin_lock_bh(&enic->vf_link_state_lock);
@@ -985,6 +1004,17 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
enic_mbox_vf_handle_link_state(enic, payload, msg_num);
break;
}
+ case ENIC_MBOX_PF_SET_ADMIN_MAC_NOTIF: {
+ size_t exp = sizeof(struct enic_mbox_pf_set_admin_mac_notif_msg);
+
+ if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
+ payload_len, exp)) {
+ enic_mbox_vf_malformed_msg(enic, hdr->msg_type, msg_num);
+ return;
+ }
+ enic_mbox_vf_handle_admin_mac(enic, payload, msg_num);
+ break;
+ }
case ENIC_MBOX_VF_ADD_DEL_MAC_REPLY:
enic_mbox_vf_handle_add_del_mac_reply(enic, payload,
payload_len, msg_num);
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 5/6] enic: manage V2 VF station and administrative MAC
2026-09-21 19:59 ` [PATCH net-next 5/6] enic: manage V2 VF station and administrative MAC Satish Kharat
@ 2026-09-24 23:01 ` netdev-bot+sashiko
0 siblings, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 23:01 UTC (permalink / raw)
To: satishkh
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, netdev,
linux-kernel, sebaddel
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 1 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The commit's central claim "Treat the PF administrative MAC as
authoritative" is not enforced.
- [Medium] Unaligned memory accesses in ether_addr_equal and
ether_addr_copy
- [Low] enic_vf_station_sync_reset() only clears ha->sync_cnt for the
station address; at the target commit enic_open() calls it two…
Pre-existing issues:
- [Low] enic_reset_addr_lists() takes netif_addr_lock_bh() only around
__dev_uc_unsync()/__dev_mc_unsync() and resets the driver-side mirrors…
--
enic: manage V2 VF station and administrative MAC
This adds V2 VF station-address management over the mailbox (delete plus add
in one compound request), keeps that address out of netdev secondary-unicast
synchronization, and adds a delayed work item plus a post-registration
refresh so the PF administrative MAC drives the VF operational address.
A few questions on the new code below.
> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 1c60e7a666aeb..16412c29e9321 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
[ ... ]
> @@ -1040,8 +1043,10 @@ void enic_reset_addr_lists(struct enic *enic)
> {
> struct net_device *netdev = enic->netdev;
>
> + netif_addr_lock_bh(netdev);
> __dev_uc_unsync(netdev, NULL);
> __dev_mc_unsync(netdev, NULL);
> + netif_addr_unlock_bh(netdev);
>
> enic->mc_count = 0;
> enic->uc_count = 0;
[Severity: Low]
This isn't a bug introduced by this patch, but should the new critical
section in enic_reset_addr_lists() also cover the counter reset?
The address-list lock is dropped before enic->mc_count, enic->uc_count and
enic->flags are cleared, and those same fields are updated from the
ndo_set_rx_mode sync callbacks under netif_addr_lock. The baseline took no
address-list lock here at all, so this is pre-existing, and for V2 VFs at
the end of the series every writer of these counters runs under RTNL
(netif_rx_mode_run() asserts the ops-compat lock before invoking
enic_set_vf_rx_mode), so the reset does not appear reachable concurrently
today. Would extending the lock to the counter reset make the intended
invariant explicit?
> @@ -1065,6 +1070,456 @@ static int enic_set_mac_addr(struct net_device *netdev, char *addr)
> return 0;
> }
>
[ ... ]
> +static int enic_vf_keep_nonstation_sync(struct net_device *netdev,
> + const u8 *addr)
> +{
> + struct enic *enic = netdev_priv(netdev);
> +
> + if (!ether_addr_equal(addr, enic->vf_station_addr))
> + return -ENOENT;
> + if (WARN_ON_ONCE(!enic->uc_count))
> + return 0;
> + enic->uc_count--;
> +
> + return 0;
> +}
> +
> +static void enic_vf_station_sync_reset(struct enic *enic)
> +{
> + if (!enic->vf_station_addr_valid)
> + return;
> +
> + /* The PF keys its MAC ledger by address. A station entry is also the
> + * receive filter for that address and must not retain a second core
> + * synchronization reference which could later delete the station.
> + */
> + netif_addr_lock_bh(enic->netdev);
> + __dev_uc_unsync(enic->netdev, enic_vf_keep_nonstation_sync);
> + netif_addr_unlock_bh(enic->netdev);
> +}
[Severity: Low]
Does enic_vf_station_sync_reset() actually keep the station address out of
core unicast synchronization at this point in the series?
It only clears ha->sync_cnt for the matching entry; the entry itself stays
on netdev->uc. __hw_addr_sync_dev() re-syncs every entry whose sync_cnt is
zero, so the next __dev_uc_sync() recreates the reference, after which a
core unsync can call enic_uc_unsync() -> enic_dev_del_addr() on the station
address while enic->vf_station_addr_valid is still true.
In enic_open() the reset is two statements before enic_set_rx_mode():
enic_vf_station_addr_set(enic, netdev->dev_addr);
enic_vf_station_sync_reset(enic);
vf_mac_added = true;
}
enic_set_rx_mode(netdev);
Worth noting this looks harmless by the end of the series: a later patch
("enic: configure V2 VF receive mode over mailbox") gives V2 VFs their own
ops table with only .ndo_set_rx_mode_async, guards the legacy
enic_set_rx_mode() call with if (!enic_is_sriov_vf_v2(enic)), and the async
collector skips any address equal to enic->vf_station_addr in both its add
and delete loops. Is the intent that this helper is only a stopgap until
that patch lands, and if so could it be squashed or the ordering noted?
[ ... ]
> +/* CMD_GET_MAC_ADDR is the PF-owned policy store. Re-read it after REGISTER so
> + * a lost notification cannot leave the VF using stale administrative policy.
> + */
> +static int enic_vf_admin_mac_refresh(struct enic *enic)
> +{
[ ... ]
> + policy_changed = !ether_addr_equal(previous_policy, policy);
> + apply_policy = pending || policy_changed ||
> + !is_valid_ether_addr(enic->netdev->dev_addr);
> + if (!apply_policy) {
> + ether_addr_copy(selected, enic->netdev->dev_addr);
[Severity: Medium]
Why does vf_station_addr get __aligned(2) in this same patch while
the new MAC buffers next to it do not?
vf_admin_mac and vf_admin_mac_random_addr are plain u8[ETH_ALEN] and
rely purely on the packing of the preceding bool fields
(vf_station_addr_valid, vf_link_running, ...) to land on an even
offset. Both are passed to ether_addr_copy() and ether_addr_equal()
in enic_vf_admin_mac_notify(), enic_vf_admin_mac_cache_selected(),
enic_vf_admin_mac_work() and enic_vf_admin_mac_refresh(), and on
builds without CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS those helpers
cast to u16 * and do three 16-bit accesses. On architectures that
trap unaligned accesses that is a fault, elsewhere it is a silent
fixup cost. The same applies to the on-stack buffers policy,
selected, previous_policy and random_addr, which sit among bool and
u8 locals in enic_vf_admin_mac_work() and enic_vf_admin_mac_refresh()
and are handed to the same helpers.
The explicit annotation on vf_station_addr suggests the author was
already aware of the requirement, so the omission on the neighbouring
fields looks accidental. Should __aligned(2) be applied consistently
to the new struct fields and the local address buffers, or is the
intent that only vf_station_addr needed it for some reason that is
not obvious from the patch?
[Severity: Medium]
Is the commit message claim "Treat the PF administrative MAC as
authoritative" enforced on this path?
enic_vf_admin_mac_refresh() compares the freshly read policy against the
cached previous_policy, not against the operational netdev->dev_addr. When
the PF policy value has not changed and dev_addr is valid, apply_policy is
false and selected is taken from dev_addr, so an address that disagrees
with the policy is preserved rather than corrected.
Combined with the V2 branch of enic_set_mac_address_dynamic() below, can a
locally installed address survive every later registration and reconnect?
[ ... ]
> @@ -1072,6 +1527,45 @@ static int enic_set_mac_address_dynamic(struct net_device *netdev, void *p)
> char *addr = saddr->sa_data;
> int err;
>
> + if (enic_is_sriov_vf_v2(enic)) {
> + if (!is_valid_ether_addr(addr))
> + return -EADDRNOTAVAIL;
> + if (ether_addr_equal(addr, netdev->dev_addr))
> + return 0;
> +
> + if (enic->vf_datapath_open &&
> + !READ_ONCE(enic->vf_registered))
> + return -ENODEV;
> +
> + /* An internal reset keeps IFF_UP set while its failed reopen leaves
> + * the datapath closed. Cache the requested address in that state; the
> + * next successful open installs it together with the datapath.
> + */
> + if (!enic->vf_datapath_open) {
> + err = enic_set_mac_addr(netdev, addr);
> + if (!err)
> + enic_vf_admin_mac_cache_selected(enic, addr);
> + return err;
> + }
[Severity: Medium]
Should this branch also check the requested address against
enic->vf_admin_mac?
With the datapath closed, the requested address is installed purely locally
via enic_set_mac_addr() with no mailbox request, so no PF policy check
happens at all. A nonzero administrative policy is then silently overridden:
ip link set <vf> down (__enic_stop() clears vf_datapath_open)
ip link set <vf> address <C>
On the next internal reset or reconnect, enic_vf_admin_mac_refresh() sees
previous_policy equal to the fetched policy and a valid dev_addr, so
apply_policy is false and <C> is kept. The following enic_open() then calls
enic_vf_station_addr_replace(netdev->dev_addr) and fails with -EACCES
("Failed to register VF station address"), leaving the interface unable to
come up until the policy address is restored by hand.
Note that a repeated PF notification does still correct this, since
enic_vf_admin_mac_notify() re-arms on !enic->vf_admin_mac_pending, but that
requires the PF to notify again. Would gating this branch on the cached
policy, or making apply_policy compare policy against dev_addr, match the
stated intent better?
[ ... ]
> @@ -1807,6 +2302,25 @@ static int enic_open(struct net_device *netdev)
> if (!enic_is_dynamic(enic) && !enic_is_sriov_vf(enic))
> enic_dev_add_station_addr(enic);
>
> + if (enic_is_sriov_vf_v2(enic)) {
> + if (!READ_ONCE(enic->vf_registered)) {
> + netdev_err(netdev, "VF is not registered with its PF\n");
> + err = -ENODEV;
> + goto err_out_disable_wq;
> + }
> +
> + err = enic_vf_station_addr_replace(enic, netdev->dev_addr);
> + if (err) {
> + netdev_err(netdev,
> + "Failed to register VF station address: %d\n",
> + err);
> + goto err_out_disable_wq;
> + }
> + enic_vf_station_addr_set(enic, netdev->dev_addr);
> + enic_vf_station_sync_reset(enic);
> + vf_mac_added = true;
> + }
> +
> enic_set_rx_mode(netdev);
>
> netif_tx_wake_all_queues(netdev);
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-b4-enic-sriov-v2-vf-receive-control-v1-0-67a0a6e08d43%40cisco.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 6/6] enic: configure V2 VF receive mode over mailbox
2026-09-21 19:59 [PATCH net-next 0/6] enic: configure V2 VF addresses and receive mode Satish Kharat
` (4 preceding siblings ...)
2026-09-21 19:59 ` [PATCH net-next 5/6] enic: manage V2 VF station and administrative MAC Satish Kharat
@ 2026-09-21 19:59 ` Satish Kharat
2026-09-24 23:01 ` netdev-bot+sashiko
5 siblings, 1 reply; 13+ messages in thread
From: Satish Kharat @ 2026-09-21 19:59 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman
Cc: netdev, linux-kernel, Satish Kharat, sebaddel
V2 VFs cannot program receive filters directly. Add an asynchronous
receive-mode callback that sends individual unicast and multicast address
changes and packet-filter settings through the VF's one-request-at-a-time
mailbox path.
Keep each callback to at most one VF mailbox request. Enable newly required
broad receive coverage before changing individual-address filters, remove
stale broad coverage before replaying a finite address list, and schedule a
fresh callback for the second half of a successful two-step transition.
Use the netdev address-list synchronization state to batch operations:
sync_cnt == 0 identifies a new entry, refcount > sync_cnt identifies an
address that is still requested, and refcount == sync_cnt identifies an
installed entry which has become stale. The asynchronous netdev core
reconciles the resulting sync_cnt changes from the snapshot back to the
live list.
Track detailed per-address results so idempotent outcomes converge, stable
policy denials do not consume the retry budget, and failed deletes or
contradictory replies require a new VF registration. Keep the station
address out of the secondary-unicast list.
Select the asynchronous netdev operations only for V2 VFs and detach the
device before mailbox teardown so no receive-mode callback can race
removal.
Assisted-by: LLM
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/net/ethernet/cisco/enic/enic.h | 4 +
drivers/net/ethernet/cisco/enic/enic_main.c | 499 +++++++++++++++++++++++++++-
2 files changed, 490 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 0f0ef68130f2..a20ca8fa7927 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -382,6 +382,10 @@ struct enic {
*/
struct enic_mac_addr *mbox_reply_mac_addrs;
u16 mbox_reply_mac_count;
+ u16 vf_pkt_filter_requested;
+ u16 vf_pkt_filter_applied;
+ bool vf_pkt_filter_valid;
+ bool vf_pkt_filter_update_pending;
bool mbox_initialized;
/* PF: per-VF MBOX state, allocated when SRIOV V2 is enabled */
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 16412c29e932..1cc1935ee5c2 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -1051,6 +1051,8 @@ void enic_reset_addr_lists(struct enic *enic)
enic->mc_count = 0;
enic->uc_count = 0;
enic->flags = 0;
+ enic->vf_pkt_filter_valid = false;
+ enic->vf_pkt_filter_update_pending = false;
}
static int enic_set_mac_addr(struct net_device *netdev, char *addr)
@@ -1348,8 +1350,12 @@ static void enic_vf_admin_mac_work(struct work_struct *work)
if (station_installed)
enic_vf_station_addr_set(enic, selected);
enic_vf_station_sync_reset(enic);
- if (enic->netdev->reg_state == NETREG_REGISTERED && changed)
- call_netdevice_notifiers(NETDEV_CHANGEADDR, enic->netdev);
+ if (enic->netdev->reg_state == NETREG_REGISTERED) {
+ if (changed)
+ call_netdevice_notifiers(NETDEV_CHANGEADDR,
+ enic->netdev);
+ netif_rx_mode_schedule_fresh(enic->netdev);
+ }
netdev_info(enic->netdev, "MBOX: admin MAC set to %pM\n", selected);
unlock:
@@ -1561,6 +1567,7 @@ static int enic_set_mac_address_dynamic(struct net_device *netdev, void *p)
enic_vf_station_addr_set(enic, addr);
enic_vf_station_sync_reset(enic);
enic_vf_admin_mac_cache_selected(enic, addr);
+ netif_rx_mode_schedule_fresh(netdev);
}
return err;
@@ -1603,17 +1610,412 @@ static int enic_set_mac_address(struct net_device *netdev, void *p)
return enic_dev_add_station_addr(enic);
}
+static u16 enic_rx_mode_to_pkt_filter(struct net_device *netdev,
+ unsigned int uc_count,
+ unsigned int mc_count)
+{
+ u16 flags = CMD_PFILTER_DIRECTED;
+
+ if (netdev->flags & IFF_MULTICAST)
+ flags |= CMD_PFILTER_MULTICAST;
+ if (netdev->flags & IFF_BROADCAST)
+ flags |= CMD_PFILTER_BROADCAST;
+ if ((netdev->flags & IFF_PROMISC) ||
+ uc_count > ENIC_UNICAST_PERFECT_FILTERS)
+ flags |= CMD_PFILTER_PROMISCUOUS;
+ if ((netdev->flags & IFF_ALLMULTI) ||
+ mc_count > ENIC_MULTICAST_PERFECT_FILTERS)
+ flags |= CMD_PFILTER_ALL_MULTICAST;
+
+ return flags;
+}
+
+static int enic_vf_set_pkt_filter(struct enic *enic, u16 flags,
+ u16 *applied_flags)
+{
+ u16 applied;
+ int err;
+
+ /* The PF can change trust policy independently and thereby withdraw
+ * broad modes. Always refresh those requests so the returned applied
+ * flags remain authoritative. The other modes are not trust-gated and
+ * can be reused for ordinary address-list churn.
+ */
+ if (!(flags & (CMD_PFILTER_PROMISCUOUS |
+ CMD_PFILTER_ALL_MULTICAST)) &&
+ enic->vf_pkt_filter_valid &&
+ enic->vf_pkt_filter_requested == flags) {
+ if (applied_flags)
+ *applied_flags = enic->vf_pkt_filter_applied;
+ return 0;
+ }
+
+ err = enic_mbox_vf_set_pkt_filter(enic,
+ !!(flags & CMD_PFILTER_DIRECTED),
+ !!(flags & CMD_PFILTER_MULTICAST),
+ !!(flags & CMD_PFILTER_BROADCAST),
+ !!(flags & CMD_PFILTER_PROMISCUOUS),
+ !!(flags & CMD_PFILTER_ALL_MULTICAST),
+ &applied);
+ if (err)
+ return err;
+
+ enic->vf_pkt_filter_requested = flags;
+ enic->vf_pkt_filter_applied = applied;
+ enic->vf_pkt_filter_valid = true;
+ if (applied_flags)
+ *applied_flags = applied;
+
+ return 0;
+}
+
+static void enic_vf_report_pkt_filter_denial(struct net_device *netdev,
+ u16 requested, u16 applied)
+{
+ if ((requested & CMD_PFILTER_PROMISCUOUS) &&
+ !(applied & CMD_PFILTER_PROMISCUOUS))
+ netdev_dbg(netdev, "PF policy denied promiscuous receive mode\n");
+ if ((requested & CMD_PFILTER_ALL_MULTICAST) &&
+ !(applied & CMD_PFILTER_ALL_MULTICAST))
+ netdev_dbg(netdev,
+ "PF policy denied all-multicast receive mode\n");
+}
+
+struct enic_vf_mac_op {
+ struct netdev_hw_addr *ha;
+ unsigned int *filter_count;
+};
+
+static unsigned int
+enic_vf_addr_list_count(const struct enic *enic,
+ const struct netdev_hw_addr_list *list,
+ bool unicast)
+{
+ const struct netdev_hw_addr *ha;
+ unsigned int count = 0;
+
+ netdev_hw_addr_list_for_each(ha, list)
+ if (ha->refcount > ha->sync_cnt &&
+ !(unicast && enic->vf_station_addr_valid &&
+ ether_addr_equal(ha->addr, enic->vf_station_addr)))
+ count++;
+
+ return count;
+}
+
+static int enic_vf_collect_mac_ops(struct enic *enic,
+ struct netdev_hw_addr_list *list,
+ bool unicast,
+ bool install_new,
+ unsigned int *filter_count,
+ struct enic_mac_addr *macs,
+ struct enic_vf_mac_op *ops,
+ u16 *num_ops)
+{
+ struct netdev_hw_addr *ha;
+ u16 pos = *num_ops;
+
+ /* Deletes precede adds so a full perfect-filter table has room for a
+ * replacement address in the same transaction.
+ */
+ netdev_hw_addr_list_for_each(ha, list) {
+ if (unicast && enic->vf_station_addr_valid &&
+ ether_addr_equal(ha->addr, enic->vf_station_addr))
+ continue;
+ if (!ha->sync_cnt ||
+ ha->refcount != ha->sync_cnt)
+ continue;
+ if (pos == ENIC_MBOX_MAX_MAC_OPS)
+ return -E2BIG;
+
+ ether_addr_copy(macs[pos].addr, ha->addr);
+ macs[pos].flags = 0;
+ ops[pos].ha = ha;
+ ops[pos].filter_count = filter_count;
+ pos++;
+ }
+
+ if (!install_new)
+ goto done;
+
+ netdev_hw_addr_list_for_each(ha, list) {
+ if (unicast && enic->vf_station_addr_valid &&
+ ether_addr_equal(ha->addr, enic->vf_station_addr))
+ continue;
+ if (ha->sync_cnt)
+ continue;
+ if (pos == ENIC_MBOX_MAX_MAC_OPS)
+ return -E2BIG;
+
+ ether_addr_copy(macs[pos].addr, ha->addr);
+ macs[pos].flags = cpu_to_le16(ENIC_MAC_ADDR_FLAG_ADD);
+ ops[pos].ha = ha;
+ ops[pos].filter_count = filter_count;
+ pos++;
+ }
+
+done:
+ *num_ops = pos;
+ return 0;
+}
+
+static int enic_vf_sync_mac_filters(struct enic *enic,
+ struct netdev_hw_addr_list *uc,
+ struct netdev_hw_addr_list *mc,
+ bool install_uc, bool install_mc,
+ bool *sent)
+{
+ struct enic_vf_mac_op *ops;
+ struct enic_mac_addr *macs;
+ bool permanent_add = false;
+ bool reconnect = false;
+ bool retryable_add = false;
+ u16 num_ops = 0;
+ unsigned int i;
+ int err = 0;
+
+ /* A class can contribute at most its installed perfect filters as
+ * deletes and its finite perfect-filter limit as adds. Keep the wire
+ * batch large enough for both classes so an update always fits in one
+ * transaction and -E2BIG remains only a defensive state-corruption
+ * check.
+ */
+ BUILD_BUG_ON(ENIC_MBOX_MAX_MAC_OPS <
+ 2 * (ENIC_UNICAST_PERFECT_FILTERS +
+ ENIC_MULTICAST_PERFECT_FILTERS));
+
+ *sent = false;
+ macs = kcalloc(ENIC_MBOX_MAX_MAC_OPS, sizeof(*macs), GFP_KERNEL);
+ if (!macs)
+ return -ENOMEM;
+ ops = kcalloc(ENIC_MBOX_MAX_MAC_OPS, sizeof(*ops), GFP_KERNEL);
+ if (!ops) {
+ err = -ENOMEM;
+ goto free_macs;
+ }
+
+ err = enic_vf_collect_mac_ops(enic, uc, true, install_uc,
+ &enic->uc_count,
+ macs, ops, &num_ops);
+ if (err)
+ goto free_ops;
+ err = enic_vf_collect_mac_ops(enic, mc, false, install_mc,
+ &enic->mc_count,
+ macs, ops, &num_ops);
+ if (err || !num_ops)
+ goto free_ops;
+
+ /* The protocol carries all changed unicast and multicast addresses in
+ * one request. Besides matching the native ABI, this bounds time spent
+ * in the RTNL-held asynchronous receive-mode callback to one MAC reply.
+ */
+ err = enic_mbox_vf_add_del_macs(enic, macs, num_ops);
+ if (err)
+ goto free_ops;
+ *sent = true;
+
+ for (i = 0; i < num_ops; i++) {
+ u16 flags = le16_to_cpu(macs[i].flags);
+ bool add = flags & ENIC_MAC_ADDR_FLAG_ADD;
+
+ if (flags & ENIC_MAC_ADDR_FLAG_SKIPPED) {
+ if (add)
+ retryable_add = true;
+ else
+ reconnect = true;
+ continue;
+ }
+ if (flags & ENIC_MAC_ADDR_FLAG_PERMANENT_MASK) {
+ if (add)
+ permanent_add = true;
+ else
+ reconnect = true;
+ continue;
+ }
+
+ if (add) {
+ ops[i].ha->sync_cnt++;
+ ops[i].ha->refcount++;
+ (*ops[i].filter_count)++;
+ } else {
+ ops[i].ha->sync_cnt--;
+ ops[i].ha->refcount--;
+ if (WARN_ON_ONCE(!*ops[i].filter_count))
+ err = -EIO;
+ else
+ (*ops[i].filter_count)--;
+ }
+ }
+
+ /* A failed DELETE can leave hardware accepting an address no longer in
+ * the requested list. Registration is the fail-closed cleanup boundary.
+ * A skipped ADD changes no acceptance state and can use the bounded core
+ * retry path. Permanent ADD denials remain stable policy results.
+ */
+ if (reconnect) {
+ enic_mbox_vf_require_reconnect(enic);
+ err = -EIO;
+ } else if (retryable_add) {
+ err = -EAGAIN;
+ } else if (permanent_add) {
+ err = -EACCES;
+ }
+
+free_ops:
+ kfree(ops);
+free_macs:
+ kfree(macs);
+ return err;
+}
+
+static int enic_set_vf_rx_mode(struct net_device *netdev,
+ struct netdev_hw_addr_list *uc,
+ struct netdev_hw_addr_list *mc)
+{
+ struct enic *enic = netdev_priv(netdev);
+ unsigned int uc_count = enic_vf_addr_list_count(enic, uc, true);
+ unsigned int mc_count = enic_vf_addr_list_count(enic, mc, false);
+ u16 flags = enic_rx_mode_to_pkt_filter(netdev, uc_count, mc_count);
+ bool uc_overflow = uc_count > ENIC_UNICAST_PERFECT_FILTERS;
+ bool mc_overflow = mc_count > ENIC_MULTICAST_PERFECT_FILTERS;
+ u16 broad_modes = CMD_PFILTER_PROMISCUOUS |
+ CMD_PFILTER_ALL_MULTICAST;
+ bool broad_enable_needed;
+ bool broad_withdrawal;
+ bool filter_needed;
+ bool sent;
+ u16 prefilter_flags;
+ u16 applied_flags;
+ int err;
+
+ if (!READ_ONCE(enic->vf_registered))
+ return -ENODEV;
+
+ filter_needed = !enic->vf_pkt_filter_valid ||
+ enic->vf_pkt_filter_requested != flags ||
+ (flags & broad_modes);
+ broad_enable_needed =
+ (!enic->vf_pkt_filter_valid && (flags & broad_modes)) ||
+ (flags & broad_modes & ~enic->vf_pkt_filter_requested);
+ broad_withdrawal = enic->vf_pkt_filter_valid &&
+ (enic->vf_pkt_filter_applied & broad_modes & ~flags);
+ if (broad_enable_needed) {
+ /* Establish newly required broad coverage before an independent
+ * exact-address rejection can block it. Retain any broad mode that
+ * is currently applied until its finite replacement is installed.
+ */
+ prefilter_flags = flags;
+ if (enic->vf_pkt_filter_valid)
+ prefilter_flags |= enic->vf_pkt_filter_applied & broad_modes;
+ err = enic_vf_set_pkt_filter(enic, prefilter_flags,
+ &applied_flags);
+ if (err)
+ return err;
+ enic->vf_pkt_filter_update_pending = false;
+ enic_vf_report_pkt_filter_denial(netdev, flags, applied_flags);
+ netif_rx_mode_schedule_fresh(netdev);
+ return 0;
+ }
+
+ /* A permanent exact-address rejection can leave an independent packet
+ * filter update pending. Give that update the next mailbox transaction,
+ * unless it would withdraw broad coverage before finite replacements are
+ * installed. A later receive-mode change can retry the rejected address.
+ */
+ if (enic->vf_pkt_filter_update_pending) {
+ if (!filter_needed || broad_withdrawal) {
+ enic->vf_pkt_filter_update_pending = false;
+ } else {
+ err = enic_vf_set_pkt_filter(enic, flags, &applied_flags);
+ if (err)
+ return err;
+ enic->vf_pkt_filter_update_pending = false;
+ enic_vf_report_pkt_filter_denial(netdev, flags, applied_flags);
+ return 0;
+ }
+ }
+
+ if (broad_withdrawal) {
+ /* Withdraw stale broad acceptance before installing its finite
+ * replacement. Keep this callback to one mailbox transaction and
+ * queue exact-filter reconciliation as fresh work. If that later
+ * reconciliation exhausts its retry budget, traffic may be dropped
+ * but PROMISC/ALLMULTI cannot remain enabled beyond the requested
+ * policy.
+ */
+ err = enic_vf_set_pkt_filter(enic, flags, &applied_flags);
+ if (err) {
+ /* The previously applied broad mode may still be active. Do
+ * not rely on the core's bounded retry budget to narrow it.
+ * Fresh registration is the fail-closed policy boundary. A
+ * local send timeout is already terminal and cannot use ordinary
+ * reconnect recovery.
+ */
+ if (!READ_ONCE(enic->mbox_tx_poisoned) &&
+ !READ_ONCE(enic->vf_mbox_reconnect_required))
+ enic_mbox_vf_require_reconnect(enic);
+ return err;
+ }
+ enic_vf_report_pkt_filter_denial(netdev, flags, applied_flags);
+ netif_rx_mode_schedule_fresh(netdev);
+ return 0;
+ }
+
+ /* Keep the finite subset of exact filters already installed for an
+ * overflowing class. Broad mode covers the remaining addresses when PF
+ * policy permits it, while the independent finite class can still make
+ * progress. When a class becomes finite again, withdraw stale broad
+ * acceptance first, then schedule its exact-filter reconciliation.
+ */
+ err = enic_vf_sync_mac_filters(enic, uc, mc, !uc_overflow,
+ !mc_overflow, &sent);
+ if (err == -EACCES) {
+ /* Do not spend the core retry budget repeating an exact operation
+ * that the PF rejected permanently. If the independent packet
+ * policy still needs an update, queue one fresh callback for it.
+ */
+ if (filter_needed) {
+ enic->vf_pkt_filter_update_pending = true;
+ netif_rx_mode_schedule_fresh(netdev);
+ }
+ return 0;
+ }
+ if (err)
+ return err;
+
+ if (sent && filter_needed) {
+ /* The address transaction succeeded. Queue the policy half as a
+ * fresh update rather than consuming the failure-retry budget.
+ */
+ netif_rx_mode_schedule_fresh(netdev);
+ return 0;
+ }
+
+ if (filter_needed) {
+ err = enic_vf_set_pkt_filter(enic, flags, &applied_flags);
+ if (err)
+ return err;
+ } else {
+ applied_flags = enic->vf_pkt_filter_applied;
+ }
+
+ enic_vf_report_pkt_filter_denial(netdev, flags, applied_flags);
+
+ return 0;
+}
+
/* netif_tx_lock held, BHs disabled */
static void enic_set_rx_mode(struct net_device *netdev)
{
struct enic *enic = netdev_priv(netdev);
- int directed = 1;
- int multicast = (netdev->flags & IFF_MULTICAST) ? 1 : 0;
- int broadcast = (netdev->flags & IFF_BROADCAST) ? 1 : 0;
- int promisc = (netdev->flags & IFF_PROMISC) ||
- netdev_uc_count(netdev) > ENIC_UNICAST_PERFECT_FILTERS;
- int allmulti = (netdev->flags & IFF_ALLMULTI) ||
- netdev_mc_count(netdev) > ENIC_MULTICAST_PERFECT_FILTERS;
+ u16 filter_flags = enic_rx_mode_to_pkt_filter(netdev,
+ netdev_uc_count(netdev),
+ netdev_mc_count(netdev));
+ int directed = !!(filter_flags & CMD_PFILTER_DIRECTED);
+ int multicast = !!(filter_flags & CMD_PFILTER_MULTICAST);
+ int broadcast = !!(filter_flags & CMD_PFILTER_BROADCAST);
+ int promisc = !!(filter_flags & CMD_PFILTER_PROMISCUOUS);
+ int allmulti = !!(filter_flags & CMD_PFILTER_ALL_MULTICAST);
unsigned int flags = netdev->flags |
(allmulti ? IFF_ALLMULTI : 0) |
(promisc ? IFF_PROMISC : 0);
@@ -2221,6 +2623,10 @@ static int enic_open(struct net_device *netdev)
{
struct enic *enic = netdev_priv(netdev);
bool vf_mac_added = false;
+ u16 vf_filter_applied;
+ u16 vf_filter_flags;
+ unsigned int vf_mc_count;
+ unsigned int vf_uc_count;
unsigned int i;
int err, ret;
unsigned int max_pkt_len = netdev->mtu + VLAN_ETH_HLEN;
@@ -2301,7 +2707,6 @@ static int enic_open(struct net_device *netdev)
if (!enic_is_dynamic(enic) && !enic_is_sriov_vf(enic))
enic_dev_add_station_addr(enic);
-
if (enic_is_sriov_vf_v2(enic)) {
if (!READ_ONCE(enic->vf_registered)) {
netdev_err(netdev, "VF is not registered with its PF\n");
@@ -2319,9 +2724,36 @@ static int enic_open(struct net_device *netdev)
enic_vf_station_addr_set(enic, netdev->dev_addr);
enic_vf_station_sync_reset(enic);
vf_mac_added = true;
+
+ netif_addr_lock_bh(netdev);
+ vf_uc_count = enic_vf_addr_list_count(enic, &netdev->uc, true);
+ vf_mc_count = enic_vf_addr_list_count(enic, &netdev->mc, false);
+ netif_addr_unlock_bh(netdev);
+
+ vf_filter_flags = enic_rx_mode_to_pkt_filter(netdev,
+ vf_uc_count,
+ vf_mc_count);
+ err = enic_vf_set_pkt_filter(enic, vf_filter_flags,
+ &vf_filter_applied);
+ if (err) {
+ netdev_err(netdev,
+ "Failed to configure VF packet filter: %d\n",
+ err);
+ goto err_out_disable_wq;
+ }
+ if ((vf_uc_count > ENIC_UNICAST_PERFECT_FILTERS &&
+ !(vf_filter_applied & CMD_PFILTER_PROMISCUOUS)) ||
+ (vf_mc_count > ENIC_MULTICAST_PERFECT_FILTERS &&
+ !(vf_filter_applied & CMD_PFILTER_ALL_MULTICAST))) {
+ netdev_err(netdev,
+ "PF denied receive mode required by VF address lists\n");
+ err = -EACCES;
+ goto err_out_disable_wq;
+ }
}
- enic_set_rx_mode(netdev);
+ if (!enic_is_sriov_vf_v2(enic))
+ enic_set_rx_mode(netdev);
netif_tx_wake_all_queues(netdev);
@@ -2956,6 +3388,13 @@ static void enic_reset(struct work_struct *work)
if (err)
netdev_err(enic->netdev,
"Failed to reopen datapath after reset: %d\n", err);
+ else if (enic_is_sriov_vf_v2(enic)) {
+ /* Internal reset bypasses __dev_open(), which normally schedules the
+ * asynchronous receive-mode upload after ndo_open. Schedule the same
+ * core replay for lists cleared by enic_reset_addr_lists().
+ */
+ netif_rx_mode_schedule_fresh(enic->netdev);
+ }
/* A PF reopens its admin channel after the datapath and re-pushes link
* state. The VF handshake, which open depends on, completed above.
@@ -3017,6 +3456,8 @@ static void enic_tx_hang_reset(struct work_struct *work)
if (err)
netdev_err(enic->netdev,
"Failed to reopen datapath after hang reset: %d\n", err);
+ else if (enic_is_sriov_vf_v2(enic))
+ netif_rx_mode_schedule_fresh(enic->netdev);
/* A PF reopens its admin channel after the datapath and re-pushes link
* state. The VF handshake, which open depends on, completed above.
@@ -3269,6 +3710,30 @@ static const struct net_device_ops enic_netdev_dynamic_ops = {
.ndo_features_check = enic_features_check,
};
+static const struct net_device_ops enic_netdev_vf_v2_ops = {
+ .ndo_open = enic_open,
+ .ndo_stop = enic_stop,
+ .ndo_start_xmit = enic_hard_start_xmit,
+ .ndo_get_stats64 = enic_get_stats,
+ .ndo_validate_addr = eth_validate_addr,
+ .ndo_set_rx_mode_async = enic_set_vf_rx_mode,
+ .ndo_set_mac_address = enic_set_mac_address_dynamic,
+ .ndo_change_mtu = enic_change_mtu,
+ .ndo_vlan_rx_add_vid = enic_vlan_rx_add_vid,
+ .ndo_vlan_rx_kill_vid = enic_vlan_rx_kill_vid,
+ .ndo_tx_timeout = enic_tx_timeout,
+ .ndo_set_vf_port = enic_set_vf_port,
+ .ndo_get_vf_port = enic_get_vf_port,
+ .ndo_set_vf_mac = enic_set_vf_mac,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ .ndo_poll_controller = enic_poll_controller,
+#endif
+#ifdef CONFIG_RFS_ACCEL
+ .ndo_rx_flow_steer = enic_rx_flow_steer,
+#endif
+ .ndo_features_check = enic_features_check,
+};
+
static const struct net_device_ops enic_netdev_ops = {
.ndo_open = enic_open,
.ndo_stop = enic_stop,
@@ -3936,7 +4401,9 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
*/
enic->rx_coalesce_usecs = enic->tx_coalesce_usecs;
- if (enic_is_dynamic(enic) || enic_is_sriov_vf(enic))
+ if (enic_is_sriov_vf_v2(enic))
+ netdev->netdev_ops = &enic_netdev_vf_v2_ops;
+ else if (enic_is_dynamic(enic) || enic_is_sriov_vf(enic))
netdev->netdev_ops = &enic_netdev_dynamic_ops;
else
netdev->netdev_ops = &enic_netdev_ops;
@@ -4083,12 +4550,18 @@ static void enic_remove(struct pci_dev *pdev)
/* Close the admin channel and unregister from the PF before
* unregister_netdev() to prevent a late PF notification from
- * touching a netdev that is being torn down. VF_UNREGISTER is the
+ * touching a netdev that is being torn down. VF_UNREGISTER is the
* protocol teardown operation: the PF removes all VF-requested
* configuration, including the station address, before replying.
+ * Detach first while holding RTNL so any running asynchronous
+ * receive-mode update has completed and queued updates cannot enter
+ * the mailbox callback.
*/
if (enic_is_sriov_vf_v2(enic)) {
enic_vf_admin_mac_quiesce(enic);
+ rtnl_lock();
+ netif_device_detach(netdev);
+ rtnl_unlock();
if (READ_ONCE(enic->vf_registered)) {
int unreg_err = enic_mbox_vf_unregister(enic);
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 6/6] enic: configure V2 VF receive mode over mailbox
2026-09-21 19:59 ` [PATCH net-next 6/6] enic: configure V2 VF receive mode over mailbox Satish Kharat
@ 2026-09-24 23:01 ` netdev-bot+sashiko
0 siblings, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 23:01 UTC (permalink / raw)
To: satishkh
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, netdev,
linux-kernel, sebaddel
Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 5 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] enic_open() now aborts with -EACCES for a V2 VF whenever the
netdev address lists exceed the perfect-filter limits (32 per class,…
- [High] The new asynchronous VF receive-mode path excludes the VF
station address only from the unicast class (`unicast &&…
- [Medium] An ADD reply carrying any ENIC_MAC_ADDR_FLAG_PERMANENT_MASK
bit (OVERFLOW, NOT_PERMITTED, INVALID) makes…
- [Medium] The patch teaches two of the three internal enic_open()
callers to schedule the asynchronous receive-mode replay, but leaves…
- [Medium] The split receive-mode update in enic_set_vf_rx_mode() does
not satisfy netif_rx_mode_sync()'s documented guarantee that the…
- [Medium] In enic_set_vf_rx_mode(), the vf_pkt_filter_update_pending
branch performs only the packet-filter mailbox transaction and then…
- [Medium] A failed mixed broad-mode transition can leave promiscuous
reception enabled indefinitely after the requested configuration…
- [Low] The broad-enable comment in enic_set_vf_rx_mode() claims 'Retain
any broad mode that is currently applied until its finite replacement…
--
enic: configure V2 VF receive mode over mailbox
This adds an asynchronous ndo_set_rx_mode_async callback for V2 VFs which
pushes individual unicast/multicast address changes and packet-filter
settings through the VF mailbox, keeping each callback to one mailbox
request. A separate netdev_ops table is selected for V2 VFs and the device
is detached before mailbox teardown in enic_remove().
A few questions about the new receive-mode paths below.
> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 16412c29e9321..1cc1935ee5c2c 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
[ ... ]
> @@ -1603,17 +1610,412 @@ static int enic_set_mac_address(struct net_device *netdev, void *p)
[ ... ]
> +static unsigned int
> +enic_vf_addr_list_count(const struct enic *enic,
> + const struct netdev_hw_addr_list *list,
> + bool unicast)
> +{
> + const struct netdev_hw_addr *ha;
> + unsigned int count = 0;
> +
> + netdev_hw_addr_list_for_each(ha, list)
> + if (ha->refcount > ha->sync_cnt &&
> + !(unicast && enic->vf_station_addr_valid &&
> + ether_addr_equal(ha->addr, enic->vf_station_addr)))
> + count++;
> +
> + return count;
> +}
> +
> +static int enic_vf_collect_mac_ops(struct enic *enic,
> + struct netdev_hw_addr_list *list,
> + bool unicast,
> + bool install_new,
> + unsigned int *filter_count,
> + struct enic_mac_addr *macs,
> + struct enic_vf_mac_op *ops,
> + u16 *num_ops)
> +{
> + struct netdev_hw_addr *ha;
> + u16 pos = *num_ops;
> +
> + /* Deletes precede adds so a full perfect-filter table has room for a
> + * replacement address in the same transaction.
> + */
> + netdev_hw_addr_list_for_each(ha, list) {
> + if (unicast && enic->vf_station_addr_valid &&
> + ether_addr_equal(ha->addr, enic->vf_station_addr))
> + continue;
[Severity: High]
Can the station address be deleted from the PF through the multicast
class? The station-address skip in both loops of
enic_vf_collect_mac_ops(), and in enic_vf_addr_list_count(), is gated on
the unicast argument, and enic_vf_sync_mac_filters() passes false for the
mc list:
err = enic_vf_collect_mac_ops(enic, mc, false, install_mc,
&enic->mc_count,
macs, ops, &num_ops);
__dev_mc_add() / __hw_addr_add_ex() do not validate the multicast bit, so
"ip maddr add <own MAC> dev ethX" from inside the guest puts the station
address into netdev->mc. That entry is then emitted as an ordinary ADD
(a DUPLICATE reply is accepted as a successful add by
enic_mbox_vf_mac_reply_matches(), and is in neither
ENIC_MAC_ADDR_FLAG_SKIPPED nor ENIC_MAC_ADDR_FLAG_PERMANENT_MASK), and a
later "ip maddr del" makes it stale and emits a DELETE for the same
address.
Since the PF keys its MAC ledger by address, as noted by the comment in
enic_vf_station_sync_reset():
/* The PF keys its MAC ledger by address. A station entry is also the
* receive filter for that address and must not retain a second core
* synchronization reference which could later delete the station.
*/
does that DELETE remove the station filter while the driver still
believes it is installed? vf_station_addr_valid stays true, the unicast
walk keeps skipping the address, and the successful reply means reconnect
is never requested. enic_vf_station_sync_reset() only calls
__dev_uc_unsync(), so it cannot drop or repair a multicast-side
reference.
The commit message says "Keep the station address out of the
secondary-unicast list" - should the same exclusion apply to the
multicast class, or is the multicast path expected to be unreachable for
a station-equal address?
Note the PF/firmware side is not in this tree. If the PF instead answers
NOT_PERMITTED for that ADD, the request lands in the silent -EACCES path
discussed below.
[ ... ]
> +static int enic_vf_sync_mac_filters(struct enic *enic,
> + struct netdev_hw_addr_list *uc,
> + struct netdev_hw_addr_list *mc,
> + bool install_uc, bool install_mc,
> + bool *sent)
> +{
[ ... ]
> + for (i = 0; i < num_ops; i++) {
> + u16 flags = le16_to_cpu(macs[i].flags);
> + bool add = flags & ENIC_MAC_ADDR_FLAG_ADD;
> +
> + if (flags & ENIC_MAC_ADDR_FLAG_SKIPPED) {
> + if (add)
> + retryable_add = true;
> + else
> + reconnect = true;
> + continue;
> + }
> + if (flags & ENIC_MAC_ADDR_FLAG_PERMANENT_MASK) {
> + if (add)
> + permanent_add = true;
> + else
> + reconnect = true;
> + continue;
> + }
[Severity: Medium]
Is ENIC_MAC_ADDR_FLAG_OVERFLOW really a stable policy result? In
enic_mbox.h it is grouped with the genuine refusals:
#define ENIC_MAC_ADDR_FLAG_PERMANENT_MASK \
(ENIC_MAC_ADDR_FLAG_OVERFLOW | ENIC_MAC_ADDR_FLAG_NOT_PERMITTED | \
ENIC_MAC_ADDR_FLAG_INVALID)
OVERFLOW looks like a capacity result (PF filter table full) rather than a
trust decision. Escalation to a broad mode is driven only by the
compile-time ENIC_UNICAST_PERFECT_FILTERS /
ENIC_MULTICAST_PERFECT_FILTERS constants applied to the netdev list
counts, never by the PF-reported result. When the PF's real per-VF quota
is smaller than 32, does the driver end up neither installing the exact
filter nor requesting broad coverage?
There is also no log here identifying the rejected address, and
enic_set_vf_rx_mode() turns this -EACCES into a 0 return to the core, so
netif_rx_mode_run() resets dev->rx_mode_retry_count and never revisits
the address. The ha keeps sync_cnt == 0 in the reconciled live list while
enic->uc_count / mc_count still claim table room. Would a netdev_warn
naming the address be appropriate?
[ ... ]
> +static int enic_set_vf_rx_mode(struct net_device *netdev,
> + struct netdev_hw_addr_list *uc,
> + struct netdev_hw_addr_list *mc)
> +{
[ ... ]
> + if (broad_enable_needed) {
> + /* Establish newly required broad coverage before an independent
> + * exact-address rejection can block it. Retain any broad mode that
> + * is currently applied until its finite replacement is installed.
> + */
[Severity: Low]
This isn't a bug, but does the comment match what the code does? It says
the currently applied broad mode is retained "until its finite
replacement is installed", while the broad_withdrawal branch below states
and implements the opposite ordering:
/* Withdraw stale broad acceptance before installing its finite
* replacement. Keep this callback to one mailbox transaction and
* queue exact-filter reconciliation as fresh work. ...
For a transition that both enables one broad mode and withdraws another
(applied = PROMISCUOUS, new flags = ALL_MULTICAST), the first callback
requests the union and the next callback withdraws PROMISCUOUS before any
finite replacement is installed. Could the comment be reworded to match?
> + prefilter_flags = flags;
> + if (enic->vf_pkt_filter_valid)
> + prefilter_flags |= enic->vf_pkt_filter_applied & broad_modes;
> + err = enic_vf_set_pkt_filter(enic, prefilter_flags,
> + &applied_flags);
> + if (err)
> + return err;
[Severity: Medium]
Can promiscuous reception stay enabled indefinitely after userspace
disabled it, when this branch fails?
With applied = CMD_PFILTER_PROMISCUOUS and new flags =
CMD_PFILTER_ALL_MULTICAST without PROMISCUOUS, both predicates are true:
broad_enable_needed = (flags & broad_modes & ~enic->vf_pkt_filter_requested);
broad_withdrawal = (enic->vf_pkt_filter_applied & broad_modes & ~flags);
broad_enable_needed wins, and this error return omits the fail-closed
handling the withdrawal branch performs:
if (!READ_ONCE(enic->mbox_tx_poisoned) &&
!READ_ONCE(enic->vf_mbox_reconnect_required))
enic_mbox_vf_require_reconnect(enic);
enic_mbox_vf_set_pkt_filter() only calls
enic_mbox_vf_require_reconnect() for reply-wait failures; a send-side
failure goes through enic_mbox_vf_request_abort() and returns err, for
example from enic_mbox_send_msg_id():
if (vnic_wq_desc_avail(wq) == 0) {
err = -ENOSPC;
goto unlock;
}
buf = kmalloc(total_len, GFP_KERNEL);
if (!buf) {
err = -ENOMEM;
goto unlock;
}
Since vf_pkt_filter_requested is unchanged on failure, every core retry
re-enters this same branch and never reaches the protected withdrawal
path, and netif_rx_mode_schedule_retry() gives up after
NETIF_RX_MODE_RETRY_MAX attempts with "rx_mode retry limit reached,
giving up". Should this error path also request fresh registration?
> + enic->vf_pkt_filter_update_pending = false;
> + enic_vf_report_pkt_filter_denial(netdev, flags, applied_flags);
> + netif_rx_mode_schedule_fresh(netdev);
> + return 0;
> + }
> +
> + /* A permanent exact-address rejection can leave an independent packet
> + * filter update pending. Give that update the next mailbox transaction,
> + * unless it would withdraw broad coverage before finite replacements are
> + * installed. A later receive-mode change can retry the rejected address.
> + */
> + if (enic->vf_pkt_filter_update_pending) {
> + if (!filter_needed || broad_withdrawal) {
> + enic->vf_pkt_filter_update_pending = false;
> + } else {
> + err = enic_vf_set_pkt_filter(enic, flags, &applied_flags);
> + if (err)
> + return err;
> + enic->vf_pkt_filter_update_pending = false;
> + enic_vf_report_pkt_filter_denial(netdev, flags, applied_flags);
> + return 0;
> + }
> + }
[Severity: Medium]
Does this else-branch discard the address work in the snapshot it was
given? It performs only the packet-filter transaction and returns 0
without touching uc/mc and without calling
netif_rx_mode_schedule_fresh(). Every other early "return 0" in this
function that leaves address work undone re-arms the callback.
The core treats 0 as fully installed, in netif_rx_mode_run():
if (err)
netif_rx_mode_schedule_retry(dev);
else
dev->rx_mode_retry_count = 0;
So an add (sync_cnt == 0) or a stale delete (refcount == sync_cnt) in
that snapshot is never programmed until some unrelated later rx-mode
change happens to re-queue the work. One sequence:
run N: permanent per-address denial -> -EACCES
-> vf_pkt_filter_update_pending = true, schedule fresh
userspace: dev_uc_add() -> __dev_set_rx_mode() -> netif_rx_mode_queue()
(ORs into the already pending NETDEV_WORK_RX_MODE bit)
run N+1: snapshots the new address, takes this pending branch,
sends SET_PKT_FILTER, returns 0
A second variant: netif_rx_mode_run() returns early while
!(dev->flags & IFF_UP), and __enic_stop() does not clear
vf_pkt_filter_update_pending, so the first run after a down/up cycle
spends its one transaction on the packet filter and drops the
accumulated add/delete work. Should this branch also schedule fresh
work?
> +
> + if (broad_withdrawal) {
> + /* Withdraw stale broad acceptance before installing its finite
> + * replacement. Keep this callback to one mailbox transaction and
> + * queue exact-filter reconciliation as fresh work. If that later
> + * reconciliation exhausts its retry budget, traffic may be dropped
> + * but PROMISC/ALLMULTI cannot remain enabled beyond the requested
> + * policy.
> + */
> + err = enic_vf_set_pkt_filter(enic, flags, &applied_flags);
[Severity: Medium]
Do the split updates still satisfy the synchronous guarantee that
netif_rx_mode_sync() provides to its callers? It performs exactly one
receive-mode invocation:
net/core/dev_addr_lists.c:netif_rx_mode_sync() {
if (__netdev_work_core_cancel(dev, NETDEV_WORK_RX_MODE))
netif_rx_mode_run(dev);
}
and callers such as the SIOCADDMULTI/SIOCDELMULTI handling in
net/core/dev_ioctl.c rely on that:
err = dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
netif_rx_mode_sync(dev);
For an up, registered V2 VF whose multicast list shrinks from more than
32 to 32 or fewer while ALL_MULTICAST is applied, this branch disables
ALL_MULTICAST, queues the individual addresses via
netif_rx_mode_schedule_fresh(), and returns 0 - so the ioctl returns with
ALLMULTI off and the individual groups not yet installed. The
broad_enable_needed branch and the "sent && filter_needed" path behave
the same way. The requeued run has to retake the ops-compat lock and can
block in enic_mbox_wait_reply(enic, 3000), so the window is not tiny. Is
a half-applied configuration at syscall return intended here?
[ ... ]
> + err = enic_vf_sync_mac_filters(enic, uc, mc, !uc_overflow,
> + !mc_overflow, &sent);
> + if (err == -EACCES) {
> + /* Do not spend the core retry budget repeating an exact operation
> + * that the PF rejected permanently. If the independent packet
> + * policy still needs an update, queue one fresh callback for it.
> + */
> + if (filter_needed) {
> + enic->vf_pkt_filter_update_pending = true;
> + netif_rx_mode_schedule_fresh(netdev);
> + }
> + return 0;
> + }
[ ... ]
> @@ -2319,9 +2724,36 @@ static int enic_open(struct net_device *netdev)
> enic_vf_station_addr_set(enic, netdev->dev_addr);
> enic_vf_station_sync_reset(enic);
> vf_mac_added = true;
> +
> + netif_addr_lock_bh(netdev);
> + vf_uc_count = enic_vf_addr_list_count(enic, &netdev->uc, true);
> + vf_mc_count = enic_vf_addr_list_count(enic, &netdev->mc, false);
> + netif_addr_unlock_bh(netdev);
> +
> + vf_filter_flags = enic_rx_mode_to_pkt_filter(netdev,
> + vf_uc_count,
> + vf_mc_count);
> + err = enic_vf_set_pkt_filter(enic, vf_filter_flags,
> + &vf_filter_applied);
> + if (err) {
> + netdev_err(netdev,
> + "Failed to configure VF packet filter: %d\n",
> + err);
> + goto err_out_disable_wq;
> + }
> + if ((vf_uc_count > ENIC_UNICAST_PERFECT_FILTERS &&
> + !(vf_filter_applied & CMD_PFILTER_PROMISCUOUS)) ||
> + (vf_mc_count > ENIC_MULTICAST_PERFECT_FILTERS &&
> + !(vf_filter_applied & CMD_PFILTER_ALL_MULTICAST))) {
> + netdev_err(netdev,
> + "PF denied receive mode required by VF address lists\n");
> + err = -EACCES;
> + goto err_out_disable_wq;
> + }
> }
[Severity: High]
Should ndo_open now fail when the PF declines a broad receive mode?
enic_set_vf_rx_mode() tolerates exactly this state at runtime - it calls
enic_vf_report_pkt_filter_denial(), which only emits netdev_dbg (compiled
out in default builds), and returns 0. Here the same state aborts the
open with -EACCES. Are the two policies meant to differ?
A reachable sequence with more than 32 multicast groups configured in the
guest (routine with several IPv6 addresses, mDNS/IGMP users, or a bridge
or macvlan on top):
enic_reset_addr_lists()
__dev_mc_unsync(netdev, NULL) /* drops only the driver's sync ref */
every still-requested address keeps refcount >= 1 with sync_cnt == 0, so
enic_vf_addr_list_count() counts it, enic_rx_mode_to_pkt_filter() sets
CMD_PFILTER_ALL_MULTICAST, and a non-trusted VF is denied that mode - the
protocol explicitly allows withholding the two broad modes, per the
required computation in enic_mbox_vf_set_pkt_filter(). The check above
then returns -EACCES and rolls the open back.
The commit message does not mention that ndo_open can now be rejected by
PF receive-mode policy. Could that be spelled out?
> - enic_set_rx_mode(netdev);
> + if (!enic_is_sriov_vf_v2(enic))
> + enic_set_rx_mode(netdev);
>
> netif_tx_wake_all_queues(netdev);
>
> @@ -2956,6 +3388,13 @@ static void enic_reset(struct work_struct *work)
> if (err)
> netdev_err(enic->netdev,
> "Failed to reopen datapath after reset: %d\n", err);
[Severity: High]
Continuing the above: in enic_reset() and enic_tx_hang_reset() the
enic_open() failure is only logged and control falls through to
reset_out, so the new -EACCES leaves vf_datapath_open false while IFF_UP
is still set and no further retry is queued. Does the interface stay
down permanently in that case, given that a manual down/up also fails
while the address lists and PF policy are unchanged?
> + else if (enic_is_sriov_vf_v2(enic)) {
> + /* Internal reset bypasses __dev_open(), which normally schedules the
> + * asynchronous receive-mode upload after ndo_open. Schedule the same
> + * core replay for lists cleared by enic_reset_addr_lists().
> + */
> + netif_rx_mode_schedule_fresh(enic->netdev);
> + }
[Severity: Medium]
Is the third internal enic_open() caller missing this same replay?
_enic_change_mtu(), reached from enic_change_mtu_work, reopens the
datapath outside __dev_open():
if (running) {
err = enic_open(netdev);
if (err)
return err;
}
with no netif_rx_mode_schedule_fresh(), and it bypasses the
dev_set_rx_mode() call that __dev_open() performs. enic_mtu_check()
schedules that work for SR-IOV VFs, and enic_is_sriov_vf() matches
PCI_DEVICE_ID_CISCO_VIC_ENET_VF_V2:
if (enic_is_dynamic(enic) || enic_is_sriov_vf(enic)) {
...
if (mtu != netdev->mtu)
schedule_work(&enic->change_mtu_work);
When that reopen takes enic_open()'s re-registration branch
(!admin_chan_up || !vf_registered || vf_mbox_reconnect_required), the
comment there states the contract this path would break:
/* Re-registration makes the PF discard the old VF-requested
* filters. Clear the netdev-core synchronization state so the
* receive-mode callback replays the current address lists.
*/
enic_reset_addr_lists(enic);
One concrete case with no other replay source: a previous enic_reset()
whose reopen failed leaves vf_datapath_open false and reconnect state set
with no further reset queued, then a firmware MTU notification drives
_enic_change_mtu(), whose enic_open() re-registers, calls
enic_reset_addr_lists(), and returns 0 with nothing scheduling the
replay. Do secondary unicast and multicast filters then stay uninstalled
while the reopen reports success?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-b4-enic-sriov-v2-vf-receive-control-v1-0-67a0a6e08d43%40cisco.com
^ permalink raw reply [flat|nested] 13+ messages in thread