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