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
