On Sun, Nov 09, 2025 at 11:05:55AM +0000, 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.
Depending on how the target was setup (by mac or interface name), the corresponding field is compared with the device being brought up.
Targets that are candidates for resuming are removed from the target list and added to a temporarily list, as __netpoll_setup_hold might allocate. __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 | 62 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 6 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 5a374e6d178d..50d6df101c20 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.
* disabled. Internally, although both STATE_DISABLED and * STATE_DEACTIVATED correspond to inactive targets, the latter is * due to automatic interface state changes and will try * recover automatically, if the interface comes back * online.
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.@@ -1445,17 +1447,50 @@ 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;
- 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);- }
+}
I am not sure that helper is useful, I would simplify the last patch with this one and write something like:
/* Attempts to resume logging to a deactivated target. */ static void maybe_resume_target(struct netconsole_target *nt, struct net_device *ndev) { int ret;
ret = __netpoll_setup_hold(&nt->np, ndev); if (ret) { /* netpoll fails setup once, do not try again. */ nt->state = STATE_DISABLED; return; }
netdev_hold(ndev, &np->dev_tracker, GFP_KERNEL); nt->state = STATE_ENABLED; pr_info("network logging resumed on interface %s\n", nt->np.dev_name); }
+/* Check if the target was bound by mac address. */ +static bool bound_by_mac(struct netconsole_target *nt) +{
- return is_valid_ether_addr(nt->np.dev_mac);
+}
Awesome. I liked this helper. It might be useful it some other places, and eventually transformed into a specific type in the target (in case we need to in the future)
Can we use it egress_dev also? If so, please separate this in a separate patch.
+/* Checks if a target matches a device. */ +static bool target_match(struct netconsole_target *nt, struct net_device *ndev) +{
- if (bound_by_mac(nt))
return !memcmp(nt->np.dev_mac, ndev->dev_addr, ETH_ALEN);- return !strncmp(nt->np.dev_name, ndev->name, IFNAMSIZ);
+}
/* Handle network interface device notifications */ static int netconsole_netdev_event(struct notifier_block *this, unsigned long event, void *ptr) {
- unsigned long flags;
- struct netconsole_target *nt, *tmp; struct net_device *dev = netdev_notifier_info_to_dev(ptr);
- struct netconsole_target *nt, *tmp;
- LIST_HEAD(resume_list); bool stopped = false;
- unsigned long flags;
if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
event == NETDEV_RELEASE || event == NETDEV_JOIN))
event == NETDEV_RELEASE || event == NETDEV_JOIN ||event == NETDEV_UP))mutex_lock(&target_cleanup_list_lock); @@ -1475,11 +1510,26 @@ static int netconsole_netdev_event(struct notifier_block *this, stopped = true; } }
if (nt->state == STATE_DEACTIVATED && event == NETDEV_UP &&target_match(nt, dev))list_move(&nt->list, &resume_list);
I think it would be better to move the nt->state == STATE_DEACTIVATED to target_match and use the case above. As the following:
if (nt->np.dev == dev) { switch (event) { case NETDEV_CHANGENAME: .... case NETDEV_UP: if (target_match(nt, dev)) list_move(&nt->list, &resume_list);
netconsole_target_put(nt);} spin_unlock_irqrestore(&target_list_lock, flags); mutex_unlock(&target_cleanup_list_lock);
Write a comment saying that maybe_resume_target() might be called with IRQ enabled.
- list_for_each_entry_safe(nt, tmp, &resume_list, list) {
maybe_resume_target(nt, dev);/* At this point the target is either enabled or disabled and* was cleaned up before getting deactivated. Either way, add it* back to target list.*/spin_lock_irqsave(&target_list_lock, flags);list_move(&nt->list, &target_list);spin_unlock_irqrestore(&target_list_lock, flags);- }
- if (stopped) { const char *msg = "had an event";
Also, extract the code below in a static function. Similar to netconsole_process_cleanups_core(), but passing resume_list argument.
Let's try to keep netconsole_netdev_event() simple to read and reason about.