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

[Simon Horman <horms@kernel.org>]

On Tue, Oct 13, 2026 at 09:57:11AM +0000, netdev-bot+sashiko@kernel.org 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.

-- 
pw-bot: changes-requested