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? 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? > + /* 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? 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()? > + } > + if (r) > return r; > [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
