Re: [PATCH v3 net-next 5/6] net: usb: pegasus: Move long delayed work on system_dfl_long_wq

[Alan Stern]

On Sat, Sep 05, 2026 at 05:04:50PM +0200, Sebastian Andrzej Siewior wrote:
> On 2026-08-28 10:10:01 [-0400], Alan Stern wrote:
> > > While this does make sense I don't see how this is related to this
> > > patch. The pegasus driver uses `system_long_wq'. This is a system wide
> > > workqueue_struct and is not limited to USB or this driver.
> > > This workqueue is per-CPU meaning if you enqueue the work item on CPU3
> > > it will be executed on CPU3. However pegasus uses a delayed work item
> > > and the timer can fire on any CPU so even if it is enqueued on CPU3 it
> > > could be executed on CPU1.
> > 
> > It doesn't matter what CPU the work item runs on.  Here's the deadlock 
> > sequence, in brief:
> > 
> > 	USB device reset cannot proceed until network interface's
> > 	->pre_reset() method returns.
> > 
> > 	The ->pre_reset() method cannot return until its call to
> > 	flush_workqueue() returns.
> > 
> > 	flush_workqueue() cannot return until the already executing
> > 	work item finishes.
> 
> Not sure where you pointing at but you have an unordered workqueue (such
> as system_long_wq/ system_dfl_long_wq then you can have more than one
> work item executed in parallel. One item does not stall the other so you
> can flush your work item (waiting for it's completion) without having all
> other work item completed.
> There is no need to flush the workqueue, that would force _all_ work
> item to complete. Flushing a workqueue would make sense if you have your
> own and you want to ensure that _all_ work items, that has been
> enqueued, did complete and you don't want to check them one by one.
> 
> To illustrate your point, the  example would translate to something like
> the following:
> 
> | static struct work_struct test_worker_busy;
> | static struct work_struct test_worker_reg;
> |  
> | static void test_worker_complete_fn(struct work_struct *work)
> | {
> |         int count = 0;
> |         while (1) {
> |                 ssleep(1);
> |                 count++;
> |                 if (count > 10)
> |                         break;
> |         }
> |         pr_err("%s()\n leaving", __func__);
> | }
> |  
> | static void test_worker_busy_fn(struct work_struct *work)
> | {
> |         while (1) {
> |                 ssleep(1);
> |                 pr_err("%s()\n", __func__);
> |         }
> | }
> |  
> | static void the_workers(void)
> | {
> |         INIT_WORK(&test_worker_busy, test_worker_busy_fn);
> |         INIT_WORK(&test_worker_reg, test_worker_complete_fn);
> |  
> |         queue_work(system_long_wq, &test_worker_busy);
> |         ssleep(1);
> |         queue_work(system_long_wq, &test_worker_reg);
> |         pr_err("%s() starting...\n", __func__);
> |         ssleep(1);
> |         pr_err("%s() cancel\n", __func__);
> |         flush_work(&test_worker_reg);
> |         pr_err("%s() moving on\n", __func__);
> | }
> 
> which leads to:
> 
> | [    4.136593] the_workers() starting...
> | [    4.137007] test_worker_busy_fn()
> | [    5.161198] the_workers() cancel
> | [    5.164832] test_worker_busy_fn()
> | [    6.184710] test_worker_busy_fn()
> | [    7.208525] test_worker_busy_fn()
> | [    8.232754] test_worker_busy_fn()
> | [    9.256522] test_worker_busy_fn()
> | [   10.280716] test_worker_busy_fn()
> | [   11.304523] test_worker_busy_fn()
> | [   12.328671] test_worker_busy_fn()
> | [   13.352517] test_worker_busy_fn()
> | [   14.376842] test_worker_busy_fn()
> | [   15.400539] test_worker_busy_fn()
> | [   15.402676] test_worker_complete_fn()
> | [   15.403290] the_workers() moving on
> | [   16.424483] test_worker_busy_fn()
> | [   17.448524] test_worker_busy_fn()
> 
> which means the test_worker_reg work item completed despite the fact
> that test_worker_busy remained busy.
> 
> Of course flushing the workqueue system_long_wq (via flush_workqueue())
> would stall but also raise a warning…
> 
> > 	The work item cannot finish until its kmalloc() call returns.
> > 
> > 	kmalloc() won't return until the kernel can free up memory by 
> > 	writing some pages to the swap partition.
> > 
> > 	The write to the swap partition cannot take place until the
> > 	usb_storage/uas driver carries it out.
> > 
> > 	usb_storage/uas cannot do anything until the USB device reset
> > 	is finished.
> 
> as shown, this is not a concern.
> 
> > > Therefore the suggested change system_long_wq -> system_dfl_long_wq
> > > should not make a difference here: it is a different workqueue and it is
> > > unbound (instead of per-CPU) but given the usage it is unchanged but
> > > more obvious. Also its usage recommendations (use this for long running
> > > items) is the same.
> > > 
> > > The plan is remove system_long_wq from the tree.
> > 
> > The point Oliver was making is that the driver shouldn't be using a 
> > general-purpose workqueue at all.  Switching from one general-purpose 
> > workqueue to another ignores this point; it's not the right thing to do.
> 
> Still the wrong thing to do? The driver should do either flush_work() or
> cancel_work_sync() (not flush_workqueue()).

I think we're in agreement.  If the driver relies on calling 
flush_workqueue(), it should not use a general-purpose workqueue.  
However, flush_work() or cancel_work_sync() is okay on an unordered 
general-purpose workqueue.

If Oliver still has any objections, he can raise them.

Alan Stern