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/[email protected]/
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
