Attempt to resume a previously deactivated target when the associated
interface comes back (NETDEV_UP event is received) by calling
__netpoll_setup_hold on the device.
For targets that were initally setup by mac address, their address is
also compared with the interface address (while still verifying that the
interface name matches).
__netpoll_setup_hold assumes RTNL is held (which is guaranteed to be the
case when handling the event) and holds a reference to the device in
case of success. This reference will be removed upon target (or
netconsole) removal by netpoll_cleanup.
Target transitions to STATE_DISABLED in case of failures resuming it to
avoid retrying the same target indefinitely.
Signed-off-by: Andre Carvalho <asantostc@gmail.com>
---
drivers/net/netconsole.c | 38 ++++++++++++++++++++++++++++++++++----
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 59d770bb4baa5f9616b10c0dfb39ed45a4eb7710..96485e979e61e0ed6c850ae3b29f46d529923f2d 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -135,10 +135,12 @@ enum target_state {
* @stats: Packet send stats for the target. Used for debugging.
* @state: State of the target.
* Visible from userspace (read-write).
- * We maintain a strict 1:1 correspondence between this and
- * whether the corresponding netpoll is active or inactive.
+ * From a userspace perspective, the target is either enabled or
+ * disabled. Internally, although both STATE_DISABLED and
+ * STATE_DEACTIVATED correspond to inactive netpoll the latter is
+ * due to interface state changes and may recover automatically.
* Also, other parameters of a target may be modified at
- * runtime only when it is disabled (state == STATE_DISABLED).
+ * runtime only when it is disabled (state != STATE_ENABLED).
* @extended: Denotes whether console is extended or not.
* @release: Denotes whether kernel release version should be prepended
* to the message. Depends on extended console.
@@ -1430,6 +1432,31 @@ static int prepare_extradata(struct netconsole_target *nt)
}
#endif /* CONFIG_NETCONSOLE_DYNAMIC */
+/* Attempts to resume logging to a deactivated target. */
+static void maybe_resume_target(struct netconsole_target *nt,
+ struct net_device *ndev)
+{
+ int ret;
+
+ if (strncmp(nt->np.dev_name, ndev->name, IFNAMSIZ))
+ return;
+
+ /* for targets specified by mac, also verify it matches the addr */
+ if (!is_broadcast_ether_addr(nt->np.dev_mac) &&
+ memcmp(nt->np.dev_mac, ndev->dev_addr, ETH_ALEN))
+ return;
+
+ ret = __netpoll_setup_hold(&nt->np, ndev);
+ if (ret) {
+ /* netpoll fails setup once, do not try again. */
+ nt->state = STATE_DISABLED;
+ } else {
+ nt->state = STATE_ENABLED;
+ pr_info("network logging resumed on interface %s\n",
+ nt->np.dev_name);
+ }
+}
+
/* Handle network interface device notifications */
static int netconsole_netdev_event(struct notifier_block *this,
unsigned long event, void *ptr)
@@ -1440,7 +1467,8 @@ static int netconsole_netdev_event(struct notifier_block *this,
bool stopped = false;
if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
- event == NETDEV_RELEASE || event == NETDEV_JOIN))
+ event == NETDEV_RELEASE || event == NETDEV_JOIN ||
+ event == NETDEV_UP))
goto done;
mutex_lock(&target_cleanup_list_lock);
@@ -1460,6 +1488,8 @@ static int netconsole_netdev_event(struct notifier_block *this,
stopped = true;
}
}
+ if (nt->state == STATE_DEACTIVATED && event == NETDEV_UP)
+ maybe_resume_target(nt, dev);
netconsole_target_put(nt);
}
spin_unlock_irqrestore(&target_list_lock, flags);
--
2.51.0
Hello Andre, On Sun, Sep 21, 2025 at 10:55:45PM +0100, Andre Carvalho wrote: > Attempt to resume a previously deactivated target when the associated > interface comes back (NETDEV_UP event is received) by calling > __netpoll_setup_hold on the device. > > For targets that were initally setup by mac address, their address is > also compared with the interface address (while still verifying that the > interface name matches). For targets that are set by the mac address, they don't necessarily get np.dev_name populated, do they? I am double checking netpoll_setup(), and if is_valid_ether_addr(np->dev_mac), I don't see np.dev_name being populated. > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > index 59d770bb4baa5f9616b10c0dfb39ed45a4eb7710..96485e979e61e0ed6c850ae3b29f46d529923f2d 100644 > --- a/drivers/net/netconsole.c > +++ b/drivers/net/netconsole.c > +/* Attempts to resume logging to a deactivated target. */ > +static void maybe_resume_target(struct netconsole_target *nt, > + struct net_device *ndev) > +{ > + int ret; > + > + if (strncmp(nt->np.dev_name, ndev->name, IFNAMSIZ)) > + return; But here, you expect that np.dev_name is populate, which I suppose it will fail if the target is binding by np->dev_mac. Should we also compare that the mac doesn't match before returning? --breno
Hi Breno, On Tue, Sep 23, 2025 at 05:22:25AM -0700, Breno Leitao wrote: > For targets that are set by the mac address, they don't necessarily get > np.dev_name populated, do they? > > I am double checking netpoll_setup(), and if > is_valid_ether_addr(np->dev_mac), I don't see np.dev_name being > populated. I was not expecting it to be the case either, bu my understanding is that np.dev_name does get populated by __netpoll_setup, which is called unconditionally at the end of netpoll_setup. __netpoll_setup eventually does: np->dev = ndev; strscpy(np->dev_name, ndev->name, IFNAMSIZ); I've confirmed that for targets bound by mac, np->dev_name is empty before these lines but then it is correctly populated here. For targets create by name, np->dev_name is already correctly set prior to this. Please, let me know if I'm missing something. > Should we also compare that the mac doesn't match before returning? Even though the above seem to work on my tests, I was not 100% sure we wanted to also check the dev_name when we initially bound by mac. I've also considered the approach below, which I think achieves what you are suggesting: if (!is_broadcast_ether_addr(nt->np.dev_mac)) { if(memcmp(nt->np.dev_mac, ndev->dev_addr, ETH_ALEN)) return; } else if (strncmp(nt->np.dev_name, ndev->name, IFNAMSIZ)) { return; } Let me know if you prefer this approach, it would allow resuming targets in case even if their dev_name changes. > --breno
Hello Andre, On Tue, Sep 23, 2025 at 08:30:39PM +0100, Andre Carvalho wrote: > On Tue, Sep 23, 2025 at 05:22:25AM -0700, Breno Leitao wrote: > > For targets that are set by the mac address, they don't necessarily get > > np.dev_name populated, do they? > > > > I am double checking netpoll_setup(), and if > > is_valid_ether_addr(np->dev_mac), I don't see np.dev_name being > > populated. > > I was not expecting it to be the case either, bu my understanding is that > np.dev_name does get populated by __netpoll_setup, which is called unconditionally > at the end of netpoll_setup. __netpoll_setup eventually does: > > np->dev = ndev; > strscpy(np->dev_name, ndev->name, IFNAMSIZ); > > I've confirmed that for targets bound by mac, np->dev_name is empty before these > lines but then it is correctly populated here. For targets create by name, > np->dev_name is already correctly set prior to this. > Please, let me know if I'm missing something. Thanks for confirming it. I think this might cause some semantics confusion for the user, given it is asking it to bind to mac, and later, netconsole is binding by dev_name. Let's say the following case: 1) netconsole is configured to bind to mac X which happens to be on eth0. 2) there is a PCI downstream failure which causes a re-enumeration 3) netconsole will get DEACTIVATED during phase 2 4) After the re-enumeration, eth0 becomes some other and interface (not the one with mac X). 5) Now you are going to bind do eth0 which is not the one with mac X. > > Should we also compare that the mac doesn't match before returning? > > Even though the above seem to work on my tests, I was not 100% sure we wanted > to also check the dev_name when we initially bound by mac. > I've also considered the approach below, which I think achieves what you are > suggesting: > > if (!is_broadcast_ether_addr(nt->np.dev_mac)) { > if(memcmp(nt->np.dev_mac, ndev->dev_addr, ETH_ALEN)) > return; > } else if (strncmp(nt->np.dev_name, ndev->name, IFNAMSIZ)) { > return; > } > > Let me know if you prefer this approach, it would allow resuming targets in case > even if their dev_name changes. I would prefer this approach than the current one, this would avoid the problem above. The other option is to always populate the mac during netpoll setup and then always resume based on mac. This seems a more precise resume. In this case, if the device goes to DEACTIVATED, then np.dev_mac will be populated, and you only compare it to check if you want to resume it.
On Sun, 21 Sep 2025 22:55:45 +0100 Andre Carvalho wrote: > + if (nt->state == STATE_DEACTIVATED && event == NETDEV_UP) > + maybe_resume_target(nt, dev); This uses GFP_KERNEL > netconsole_target_put(nt); > } > spin_unlock_irqrestore(&target_list_lock, flags); And we're under a spin lock. This gets hit in the selftest you're adding so please triple check the kernel config that you're testing with.
Hi Jakub, On Mon, Sep 22, 2025 at 04:50:57PM -0700, Jakub Kicinski wrote: > This gets hit in the selftest you're adding so please triple check > the kernel config that you're testing with. Sorry for the silly mistake. I'll make sure to adjust my local testing to include running all selftests with kernel/configs/debug.config to catch these before sending the patches.
© 2016 - 2025 Red Hat, Inc.