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 andSTATE_DEACTIVATED correspond to inactive targets, the latter isdue to automatic interface state changes and will tryrecover automatically, if the interface comes backonline.
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!