On Sun, Nov 01, 2026 at 09:22 PM, Jiaxing Hu <[email protected]> wrote: > drm_sched_stop() does not wait for a threaded handler that is already > running. Call synchronize_irq() after it, outside job_lock, which the > handler takes. > > Before the sync, mask the block's interrupt and clear its raw status, so > that an active core cannot signal a completion after it. Do that under > job_lock, since rocket_job_hw_submit() arms the same mask under that lock, > and only when pm_runtime_get_if_active() returns a positive count: the > reset holds no runtime PM reference, and with the domain down a register > access takes an async SError.
The above I understand. > Igor Paunovic's induced-reset runs on RK3588, including a two-task job that > puts hw_submit() on the IRQ thread, found no fault; as he put it, "this > does not show the race is closed". This is completely out of context for a commit message. How is somebody who is reading git log know who Igor is, and what his "induced-reset" is? > Link: https://lore.kernel.org/all/[email protected]/ > Link: > https://lore.kernel.org/all/caewpsh5mxtbuknouxm6yecmzyvdowquhvyvhaxq8homthd5...@mail.gmail.com/ > Link: https://lore.kernel.org/all/[email protected]/ > Link: > https://lore.kernel.org/all/[email protected]/ > Link: > https://lore.kernel.org/all/[email protected]/ > Suggested-by: Igor Paunovic <[email protected]> > Signed-off-by: Jiaxing Hu <[email protected]> > Tested-by: Igor Paunovic <[email protected]> # RK3588, three cores, > induced reset, JOB_TIMEOUT_MS=2 > --- > drivers/accel/rocket/rocket_job.c | 71 +++++++++++++++++++++++++++++-- > 1 file changed, 68 insertions(+), 3 deletions(-) > > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index 03e5f094f60b..5b2abf40b618 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -401,9 +401,74 @@ rocket_reset(struct rocket_core *core, struct > drm_sched_job *bad) > drm_sched_stop(&core->sched, bad); > > /* > - * Remaining interrupts have been handled, but we might still have > - * stuck jobs. Let's make sure the PM counters stay balanced by > - * manually calling pm_runtime_put_noidle(). > + * Mask the block before waiting. hw_submit() arms INTERRUPT_MASK on > + * every submit and only the hardirq clears it, so on an ordinary > + * timeout it is still live and a completion can arrive after the sync > + * returns. The next submit re-arms it, so nothing is lost here. > + * > + * Only when the device is already awake, though. This function holds no > + * runtime PM reference of its own: the only one in the window belongs > to > + * in_flight_job, and the completion path may have put it and cleared > the > + * pointer before the timeout worker got here. drm_sched_stop() above > can > + * block for a long time, and it drops every pending job's credits, so > + * rocket_job_is_idle() is true and nothing keeps the core resumed. On > + * this hardware a register access with the domain down takes an async > + * SError, so a reset must not be the thing that causes one. > + * > + * Only a positive answer will do. pm_runtime_get_if_active() tests > + * power.disable_depth before power.runtime_status, so -EINVAL MASKS a > + * suspended device rather than excluding one: > pm_runtime_force_suspend(), > + * which is this driver's own system suspend callback, disables runtime > PM > + * first and turns the clocks off second, and rocket_core_fini() > suspends > + * the core and disables before it cancels the timeout worker. Both > leave > + * the domain down with -EINVAL on offer. > + * > + * The cost is the other half of that ambiguity. A core that is still up > + * with runtime PM disabled (pm_runtime_force_suspend() before its > + * callback has run, or CONFIG_PM=n under COMPILE_TEST) is left > unmasked, > + * because writing to it would mean writing to the half that is down as > + * well. > + * > + * Clear the raw status along with the mask, the way the completion path > + * does. Masking alone leaves the DPU bit latched until > + * rocket_core_reset(), and the hardirq decides on raw status alone, so > a > + * fault from the IOMMU that shares this line would wake the thread > again > + * and what the comment below asserts would stop being true. > + * > + * UNDER job_lock, because rocket_job_hw_submit() arms this same > + * register and always runs under that lock. reset.pending is set here > + * without the lock and read there with it, so a submit that has already > + * passed its check can re-arm the mask after this clears it, and then > + * the synchronize_irq() below fences a handler that is no longer the > + * one that matters: the block is left running a task with its > + * interrupt live. rocket_job_handle_irq() avoids the same race on > + * OPERATION_ENABLE by making its completion writes under this lock. > + * > + * pm_runtime_get_if_active() does not invoke a callback -- it only > + * takes a reference on an already-active device -- and > + * pm_runtime_put_autosuspend() is asynchronous, so neither can re-enter > + * this driver's runtime PM callbacks while the lock is held. > + */ This is just absurd. The driver will quickly become unmaintainable if we merge this kind of comments. Please remove all LLM-generated text (cover letter, commit messages, and code comments) from the accel/rocket patches in this series, and replace it with your own. I don't mind if you write it first in your native language and use a LLM to translate it to english, but I need for you to understand the code you submit and write yourself any communication with humans. If you don't understand anything in the code well enough to write the commit messages, cover letter and comments, then I suggest you use the LLM to assist you in acquiring the knowledge that you are missing. Also, please read: Documentation/process/coding-assistants.rst Documentation/process/generated-content.rst Thanks, Tomeu
