Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] The newly added early return path in `esparser_isr()` incorrectly returns `IRQ_HANDLED` without clearing the hardware interrupt status, leading to an interrupt storm and violating shared IRQ semantics. - [Medium] The commit message falsely claims to fix a race condition where the ISR 'attempts to handle stream data and allocate internal tracking state structures', which does not exist in the code. Pre-existing issues: - [High] The synchronization and early-exit logic introduced by the patch is ineffective and leaves the system vulnerable to synchronous bus faults during teardown. - [High] Unconditional execution of `vdec_poweroff()` in `vdec_close()` causes clock underflows. -- --- Patch [5]: [PATCH v7 05/19] media: meson: vdec: Fix race condition and synchronize esparser IRQ --- commit 2f7e559e32b43d068f1e5390f023fbf7f75f1262 Author: Anand Moon <[email protected]> media: meson: vdec: Fix race condition and synchronize esparser IRQ This commit attempts to fix a race condition during session teardown sequences by synchronizing the esparser IRQ and adding a NULL check for the session pointer in the ISR. It aims to prevent the interrupt handler from running while context structures are dismantled. Closes: https://lore.kernel.org/all/[email protected]/ > During session teardown sequences in vdec_stop_streaming() and > vdec_close(), the 'esparser' hardware interrupt handler can still be > actively triggered or executing on another CPU core. This creates a > transient race condition where the ISR attempts to handle stream data and > allocate internal tracking state structures after session contexts have > been modified or freed. [Severity: Medium] Does the ISR actually allocate tracking state or handle stream data? Looking at esparser_isr(), the sess pointer is retrieved and checked for NULL, but the rest of the ISR does not appear to dereference this pointer or allocate any internal tracking structures. > diff --git a/drivers/staging/media/meson/vdec/esparser.c > b/drivers/staging/media/meson/vdec/esparser.c > index 4632346f04a9e..37749ede308c6 100644 > --- a/drivers/staging/media/meson/vdec/esparser.c > +++ b/drivers/staging/media/meson/vdec/esparser.c > @@ -60,6 +60,12 @@ static irqreturn_t esparser_isr(int irq, void *dev) > { > int int_status; > struct amvdec_core *core = dev; > + struct amvdec_session *sess; > + > + /* Secure an atomic snapshot to protect against concurrent teardown */ > + sess = smp_load_acquire(&core->cur_sess); > + if (!sess) > + return IRQ_HANDLED; [Severity: High] Is it safe to return IRQ_HANDLED here without interacting with the hardware to clear the interrupt? Because esparser_irq is registered with IRQF_SHARED, returning IRQ_HANDLED when cur_sess is NULL falsely tells the IRQ subsystem that this driver handled the interrupt. Will this cause an interrupt storm for level-triggered interrupts, or break spurious interrupt detection for other devices sharing this IRQ line? > > int_status = amvdec_read_parser(core, PARSER_INT_STATUS); > amvdec_write_parser(core, PARSER_INT_STATUS, int_status); [ ... ] > diff --git a/drivers/staging/media/meson/vdec/vdec.c > b/drivers/staging/media/meson/vdec/vdec.c > index 7ae3d5a9dd6ab..7689ffdb2e500 100644 > --- a/drivers/staging/media/meson/vdec/vdec.c > +++ b/drivers/staging/media/meson/vdec/vdec.c > @@ -486,6 +486,8 @@ static void vdec_stop_streaming(struct vb2_queue *q) > > /* Synchronize and flush pending hardware interrupt service > routines */ > synchronize_irq(core->vdec_irq); > + /* Ensure esparser ISR finishes executing */ > + synchronize_irq(core->esparser_irq); > > vdec_poweroff(sess); > vdec_free_canvas(sess); [Severity: High] This is a pre-existing issue, but does this synchronization logic fully close the race condition? The synchronize_irq() call waits for in-flight ISRs, but the interrupt line remains enabled. Later in this teardown path: vdec_stop_streaming() vdec_poweroff(sess); ... smp_store_release(&core->cur_sess, NULL); If the interrupt fires after vdec_poweroff() completes, but before cur_sess is set to NULL, the ISR will pass the !sess check and attempt to read PARSER_INT_STATUS from unclocked hardware. Could this lead to a synchronous bus fault and kernel panic? [ ... ] > @@ -994,6 +996,8 @@ static int vdec_close(struct file *file) > > /* Synchronize and flush pending hardware interrupt service routines */ > synchronize_irq(core->vdec_irq); > + /* Ensure esparser ISR finishes executing */ > + synchronize_irq(core->esparser_irq); > > if (!IS_ERR_OR_NULL(sess->recycle_thread)) { > kthread_stop(sess->recycle_thread); [Severity: High] This is a pre-existing issue, but does vdec_close() unconditionally call vdec_poweroff() even if the device was just opened without starting streaming? If vdec_open() is called and immediately followed by vdec_close(), calling vdec_poweroff() unconditionally would execute clk_disable_unprepare() on dos_clk and dos_parser_clk which were never enabled. Can this cause clock framework underflow warnings and potentially disable clocks for other hardware sharing the same clock tree? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
