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

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

[netdev-bot+sashiko]

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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012095452.135828-1-marco.crivellari%40suse.com

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

[Simon Horman]

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

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@kernel.org> wrote:
>
> 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.

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