Hi Heikki,
On Thu, Jan 05, 2023 at 01:27:07PM +0200, Heikki Krogerus wrote:
> Hi,
>
> On Thu, Jan 05, 2023 at 01:42:40PM +0800, Linyu Yuan wrote:
> > When a PPM client unregisters with UCSI framework, connector specific work
> > queue is destroyed. However, a pending delayed work queued before may
> > still exist. Once the delay timer expires and the work is scheduled,
> > this can cause a system crash as the workqueue is destroyed already.
>
> When the workqueue is destroyed it's also flushed, no? So how could
> there be still something pending, delayed or not? Did you actually see
> this happening?
Yes, we encountered this during a scenario in which our PPM firmware
is undergoing a reset which is handled by calling ucsi_unregister().
The connectors' workqueues are destroyed but apparently the
destroy_workqueue() does *not* seem to take care pending delayed items.
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
...
Call trace:
__queue_work+0x1f4/0x550
delayed_work_timer_fn+0x28/0x38
call_timer_fn+0x3c/0x238
expire_timers+0xcc/0x168
__run_timers+0x194/0x1f8
run_timer_softirq+0x2c/0x54
_stext+0xec/0x3a8
__irq_exit_rcu+0xa0/0xfc
irq_exit_rcu+0x18/0x28
el0_interrupt+0x5c/0x138
__el0_irq_handler_common+0x20/0x30
el0t_64_irq_handler+0x18/0x28
el0t_64_irq+0x1a0/0x1a4
Code: eb16013f 54000300 aa1a03e0 9441be2a (f9400280)
In particular this is happening for the ucsi_check_connection() which is
the currently the only work item using a non-zero delay. If we look
closely at queue_delayed_work() we can see in that case it defers by
using a separate timer:
static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
struct delayed_work *dwork, unsigned long delay)
{
struct timer_list *timer = &dwork->timer;
struct work_struct *work = &dwork->work;
<...snip...>
/*
* If @delay is 0, queue @dwork->work immediately. This is for
* both optimization and correctness. The earliest @timer can
* expire is on the closest next tick and delayed_work users depend
* on that there's no such delay when @delay is 0.
*/
if (!delay) {
__queue_work(cpu, wq, &dwork->work);
return;
}
dwork->wq = wq;
^^^^^^^^ wq gets saved here, but might be
destroyed before timer expires
dwork->cpu = cpu;
timer->expires = jiffies + delay;
if (unlikely(cpu != WORK_CPU_UNBOUND))
add_timer_on(timer, cpu);
else
add_timer(timer);
}
In ucsi_unregister() we destroy the connector's wq, but there is a
pending timer still for the ucsi_check_connection() item and upon
expiry it tries to do the real __queue_work() call on a dangling 'wq'.
> > Fix this by moving all partner related delayed work to connector instance
> > and cancel all of them when ucsi_unregister() is called by PPM client.
>
> I would love to be able to cancel these works, though not because of
> driver removal - I'm more concerned about disconnections. The reason
> why each task is a unique work is because it allows the driver to add
> the same task to the queue as many times as needed, and that was
> needed inorder to recover from some firmware issues. If there's only a
> dedicated work per task like in your proposal, we can only schedule
> the task once at a time, and that may lead into other issues.
I see, queuing a task multiple times is a good reason to allocate the
workers dynamically. Then what we really need is a way to reliably
cancel & reclaim any pending items that are sitting on their own timers,
since they are otherwise unreachable via the 'wq'.
cancel_delayed_work(), cancel_delayed_work_sync(), flush_delayed_work()
all require the handle to the delayed_work itself which we don't keep a
reference to.
Do you have any other suggestions for this?
Thanks,
Jack
