Re: [PATCH net-next v4 2/6] net: ti: icssg-stats: Move long delayed work on system_dfl_long_wq

[Marco Crivellari]

On Sun, Oct 18, 2026 at 4:34 PM Simon Horman <horms@xxxxxxxxxx> wrote:
>
> On Tue, Oct 13, 2026 at 09:57:11AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue(s) to consider.
> >
> > Critical: 0 · High: 0 · Medium: 1 · Low: 0
> >
> > If you believe none of the issues are legitimate, please reply to each
> > issue of Medium severity or higher and explain why the AI is wrong.
> > If any of them are legitimate and you plan to respin, please let patchwork
> > know by including "pw-bot: cr" as a separate line at the end of your reply
> > (one such reply per series is enough).
> >
> > - [Medium] The conversion is incomplete, so one delayed_work is queued on
> >   two different workqueues.
> >
> > --
> >
> > net: ti: icssg-stats: Move long delayed work on system_dfl_long_wq
> >
> > This commit changes the self-requeue in icssg_stats_work_handler() from
> > the per-cpu system_long_wq to the unbound system_dfl_long_wq. The goal is
> > to let the periodic stats work benefit from scheduler task placement.
> >
> > > diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> > > index 7159baa0155cf..7d6d6692d8192 100644
> > > --- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
> > > +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> > > @@ -69,7 +69,7 @@ void icssg_stats_work_handler(struct work_struct *work)
> > >                                             stats_work.work);
> > >     emac_update_hardware_stats(emac);
> > >
> > > -   queue_delayed_work(system_long_wq, &emac->stats_work,
> > > +   queue_delayed_work(system_dfl_long_wq, &emac->stats_work,
> > >                        msecs_to_jiffies((STATS_TIME_LIMIT_1G_MS * 1000) / emac->speed));
> > >  }
> >
> > [Severity: Medium]
> > Is this conversion complete?
> >
> > Both ICSSG drivers use icssg_stats_work_handler() for the same
> > emac->stats_work. Both prueth_netdev_init() functions, in icssg_prueth.c
> > and icssg_prueth_sr1.c, do this:
> >
> >     INIT_DELAYED_WORK(&emac->stats_work, icssg_stats_work_handler);
> >
> > However, the first run is still queued on the per-cpu workqueue from
> > the ndo_open path:
> >
> > drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c:emac_ndo_open() {
> >     ...
> >       queue_work(system_long_wq, &emac->stats_work.work);
> >     ...
> > }
> >
> > The follow-up patch "net: ti: icssg-prueth: Move long delayed work on
> > system_dfl_long_wq" converts the matching call in icssg_prueth.c. No
> > patch in the series seems to change icssg_prueth_sr1.c.
> >
> > On SR1.0 hardware, the first stats run after each open would still go to
> > a per-cpu worker. Every later run would go to system_dfl_long_wq.
> >
> > Could mixing the two workqueues also bypass the non-reentrancy check in
> > __queue_work()?
> >
> > kernel/workqueue.c:__queue_work() {
> >     ...
> >               if (worker && worker->current_pwq->wq == wq) {
> >     ...
> > }
> >
> > The instance running on system_long_wq re-arms onto system_dfl_long_wq,
> > so this check fails. If the timer expires before the first worker
> > returns, a second instance could start at the same time on another
> > worker.
> >
> > Today this looks harmless. The handler does nothing after
> > queue_delayed_work(), and emac_update_hardware_stats() takes
> > prueth->stats_lock.
> >
> > Should emac_ndo_open() in icssg_prueth_sr1.c also switch to
> > system_dfl_long_wq, so stats_work is only ever queued on one workqueue?
>
> It seem to me that it would be worth getting this right upfront.

Thanks for your comment on this Simon.
It likely makes sense to change it there as well. I will check for the v5.

Thanks!

-- 

Marco Crivellari

SUSE Labs