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

[netdev-bot+sashiko@kernel.org]

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