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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The stats_work conversion is only half done because the SR1
  driver variant was missed.
- [Low] The commit message is boilerplate copied from the delayed-work
  conversions and does not describe this diff.

--

net: ti: icssg-prueth: Move long delayed work on system_dfl_long_wq

This patch changes the first queueing of emac->stats_work in
emac_ndo_open() from the per-CPU system_long_wq to the unbound
system_dfl_long_wq. The goal is to let the scheduler place the work.

> Currently the code enqueue work items using {queue|mod}_delayed_work(),
> using system_long_wq. This workqueue should be used when long works are
> expected and it is a per-cpu workqueue.
>
> The function(s) end up calling __queue_delayed_work(), which set a global
> timer that could fire anywhere, enqueuing the work where the timer fired.

[Severity: Low]
Does this description match the diff?

The only line changed is in emac_ndo_open() in icssg_prueth.c, and it
uses queue_work(), not {queue|mod}_delayed_work():

    queue_work(system_dfl_long_wq, &emac->stats_work.work);

queue_work() is queue_work_on(WORK_CPU_UNBOUND, wq, work). It queues the
embedded work_struct right away. It arms no timer and does not go through
__queue_delayed_work().

The delayed re-arm that the message describes lives in
icssg_stats_work_handler() in icssg_stats.c. The companion patch
"net: ti: icssg-stats: Move long delayed work on system_dfl_long_wq"
converts that one.

Could the message describe the immediate first kick of stats_work instead,
and mention the companion patch?

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 591be5c8056b4..0fba22c1046e9 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -1100,7 +1100,7 @@ static int emac_ndo_open(struct net_device *ndev)
>
>  	prueth->emacs_initialized++;
>
> -	queue_work(system_long_wq, &emac->stats_work.work);
> +	queue_work(system_dfl_long_wq, &emac->stats_work.work);

[Severity: Low]
Should the SR1 variant of this driver get the same change?

emac_ndo_open() in drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c still
does this:

    queue_work(system_long_wq, &emac->stats_work.work);

Both drivers set up stats_work with the same shared handler:

    INIT_DELAYED_WORK(&emac->stats_work, icssg_stats_work_handler);

After the companion icssg-stats patch, icssg_stats_work_handler() re-arms
itself on the unbound workqueue:

    queue_delayed_work(system_dfl_long_wq, &emac->stats_work,
                       msecs_to_jiffies((STATS_TIME_LIMIT_1G_MS * 1000) / emac->speed));

So on SR1 hardware, the first run after ndo_open goes to the per-CPU
system_long_wq. Every later run goes to system_dfl_long_wq.

The commit message reasoning ("the workqueue work doesn't rely on per-cpu
variables") applies to the SR1 call site too. Nothing else in the series
seems to convert it.

>
>  	return 0;
>

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