On Sun, 28 Jun 2026 14:26:53 +0000
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
>
> New issues:
> - [High] Missing wait/pacing for short packets leads to TX FIFO overflows and
> transmission failures.
> - [High] The wait for a long packet's transmission can be prematurely
> satisfied by the PACKET_SENT interrupt of a preceding short packet.
>
> Pre-existing issues:
> - [High] A race condition during timeout handling causes a use-after-free on
> a stack-allocated completion object due to lockless IRQ dispatch.
> --
>
> commit 13757a7bc4e8503b706178de5f3a86e797b3589f
> Author: Andreas Kemnade <[email protected]>
>
> drm/omap: dsi: avoid sending bta sync all the time in writes
>
> This patch modifies the DSI write logic to avoid sending a BTA sync on
> every write unless acknowledgments are explicitly requested. Instead,
> it waits for the packet transmission to complete to prevent FIFO overflows.
> This change allows configuration of chips like the TC358762 that require
> specific commands before they can send data.
>
> > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c
> > b/drivers/gpu/drm/omapdrm/dss/dsi.c
> > index 27fe7bca9e2cf..f7079a49e824e 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> > @@ -2194,28 +2194,39 @@ static int dsi_vc_send_null(struct dsi_data *dsi,
> > int vc, int channel)
> > static int dsi_vc_write_common(struct omap_dss_device *dssdev, int vc,
> > const struct mipi_dsi_msg *msg)
> > {
> > + DECLARE_COMPLETION_ONSTACK(completion);
> > struct dsi_data *dsi = to_dsi_data(dssdev);
> > int r;
> >
> > if (mipi_dsi_packet_format_is_short(msg->type))
> > r = dsi_vc_send_short(dsi, vc, msg);
> > - else
> > + else {
>
> [Severity: High]
> The commit message states "wait for the packet to be sent to avoid FIFO
> overflows", but does this code omit that wait for short packets?
>
I would switch back to also wait there and have that confirmed by Ivo.
My testing was also done with short packets, so apparently the interrupt
comes there, too, so I can probably go back to the v1 way of doing things.
> If a panel driver sends a rapid sequence of short DCS configuration
> commands, it appears dsi_vc_send_short() will push to the FIFO and
> return immediately. Since the unconditional dsi_vc_send_bta_sync() was
> removed, could this lack of pacing cause the TX FIFO to overflow during
> consecutive short writes?
>
as said, I can turn back.
> > + /* wait for IRQ for long packet transmission confirmation */
> > + r = dsi_register_isr_vc(dsi, vc, dsi_completion_handler,
> > + &completion, DSI_VC_IRQ_PACKET_SENT);
>
> [Severity: High]
> Because short packets return immediately without waiting, what happens if
> a short packet is followed immediately by a long packet?
>
which would also avoid this problem.
> If the short packet finishes transmitting after the DSI_VC_IRQ_PACKET_SENT
> status is cleared by dsi_register_isr_vc() for the long packet, will the
> short packet's completion interrupt fire and prematurely satisfy the
> wait_for_completion_timeout() for the long packet?
>
> > + if (r)
> > + return r;
> > +
> > r = dsi_vc_send_long(dsi, vc, msg);
> >
> > - if (r < 0)
> > + if ((!r) && wait_for_completion_timeout(&completion,
> > + msecs_to_jiffies(500)) == 0)
> > + r = -EIO;
> > +
> > + dsi_unregister_isr_vc(dsi, vc, dsi_completion_handler,
> > + &completion, DSI_VC_IRQ_PACKET_SENT);
>
> [Severity: High]
> This is a pre-existing issue, but can this sequence lead to a
> use-after-free on the stack-allocated completion object?
>
> If the 500ms timeout expires, the thread wakes up, unregisters the ISR,
> returns -EIO, and destroys the stack frame. However, omap_dsi_irq_handler()
> makes a copy of the ISR table and executes handlers locklessly:
>
> omap_dsi_irq_handler() {
> memcpy(&dsi->isr_tables_copy, &dsi->isr_tables, sizeof(dsi->isr_tables));
> spin_unlock(&dsi->irq_lock);
> dsi_handle_isrs(&dsi->isr_tables_copy, ...);
> }
>
> If a delayed hardware interrupt fires concurrently, could the IRQ handler
> invoke dsi_completion_handler() from its local copy and call complete() on
> the now-destroyed stack pointer, since it doesn't synchronize with
> dsi_unregister_isr_vc()?
>
I looked around, the idea beding this copy seems to be invalid:
/* make a copy and unlock, so that isrs can unregister
* themselves */
nobody seems to do that. I will send a separate patch to remove that copy.
Regards,
Andreas
