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 <[email protected]>
> ---
> 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))
> goto done;
>
> 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.
