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.
  *		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);
+	}
+}
+
+/* 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);
+}
+
+/* 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);
 		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
 	mutex_unlock(&target_cleanup_list_lock);
 
+	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";
 

-- 
2.51.2

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

On Tue, Nov 11, 2025 at 02:12:26AM -0800, Breno Leitao wrote:
> > + *		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.
> 

This is much clearer, thanks for the suggestion. 

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

The main reason why I opted for a helper in netpoll was to keep reference
tracking for these devices strictly inside netpoll and have simmetry between
setup and cleanup. Having said that, this might be an overkill and I'm fine with 
dropping the helper and taking your suggestion.

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

In order to do that, we'd need to move bound_by_mac to netpolland make it available
to be called by netconsole. Let me know if you'd like me to do this in this series,
otherwise I'm also happy to refactor this separately from this series.

> > +		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);
> 

We are not able to handle this inside this switch because when target got deactivated, 
do_netpoll_cleanup sets nt->np.dev = NULL. Having said that, I can still move nt->state == STATE_DEACTIVATED
to inside target_match (maybe calling it deactivated_target_match) to make this slightly more readable. 

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

Ack.

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

Ack.

Thanks for the review!

-- 
Andre Carvalho

On Tue, Nov 11, 2025 at 07:18:46PM +0000, Andre Carvalho wrote:
> On Tue, Nov 11, 2025 at 02:12:26AM -0800, Breno Leitao wrote:
> > > + *		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.
> > 
> 
> This is much clearer, thanks for the suggestion. 
> 
> > > +	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:
> > 
> 
> The main reason why I opted for a helper in netpoll was to keep reference
> tracking for these devices strictly inside netpoll and have simmetry between
> setup and cleanup. Having said that, this might be an overkill and I'm fine with 
> dropping the helper and taking your suggestion.

Right, that makes sense. Would we have other owners for that function?

> 
> > > +
> > > +/* 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.
> 
> In order to do that, we'd need to move bound_by_mac to netpolland make it available
> to be called by netconsole. Let me know if you'd like me to do this in this series,
> otherwise I'm also happy to refactor this separately from this series.

Oh, I see the problem. That egress_dev() should belong to netconsole not
netpoll.

I've sent a patchset to start untangling netconsole and netpoll, and the
patchset was conflicting with the fix in 'net' 

https://lore.kernel.org/all/20250902-netpoll_untangle_v3-v1-0-51a03d6411be@debian.org/

Let's keep egress_dev() as it is for now, until we got them untangled.

> 
> > > +		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);
> > 
> 
> We are not able to handle this inside this switch because when target got deactivated, 

You are right, that is why we are doing the magic here. Please add
a comment in saying that maybe_resume_target() is IRQ usafe, thus,
cannot be called with IRQ disabled.

> do_netpoll_cleanup sets nt->np.dev = NULL. Having said that, I can still move nt->state == STATE_DEACTIVATED
> to inside target_match (maybe calling it deactivated_target_match) to make this slightly more readable. 

Awesome. Thanks for the patch!
--breno

On Wed, Nov 12, 2025 at 09:52:10AM -0800, Breno Leitao wrote:
> > The main reason why I opted for a helper in netpoll was to keep reference
> > tracking for these devices strictly inside netpoll and have simmetry between
> > setup and cleanup. Having said that, this might be an overkill and I'm fine with 
> > dropping the helper and taking your suggestion.
> 
> Right, that makes sense. Would we have other owners for that function?

I've looked at other drivers using netpoll and from what I could find all of them
are using __netpoll_setup paired with __netpoll_free. They don't seem to 
rely on dev_tracker to track references, I'd need to look a bit more to be certain,
but I think other callers are own the devices and track their lifecycle separately.
So I don't think this would be useful for them.

Since we are moving netpoll_cleanup to netconsole in your patch below, I think I should
drop the netpoll helper and keep it in netconsole. I wonder if we should consider
moving do_netpoll_cleanup to netconsole as well, since it seems to be the only caller
and then we would have the same symmetry I mentioned above.

So, to summarize, given your refactor patch I think it makes sense drop the previous
patch and do the netdev_hold in netconsole as you suggested. Does that sound good?

-- 
Andre Carvalho