Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Bandwidth check and PHY clock calculations do not account for DSC compression, incorrectly rejecting high-resolution modes and over-clocking the PHY. Pre-existing issues: - [High] Error path in `mtk_dsi_poweron` fails to decrement `refcount`, permanently leaving the DSI driver in a corrupted state. - [High] Signal interruptions during IRQ waits are incorrectly treated as successful operations. -- --- Patch [7]: [PATCH v5 07/11] drm/mediatek: mtk_dsi: Add support for MT8189 --- commit b1ac2564850b4d8fbc42fa9bf9284c320bec103e Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dsi: Add support for MT8189 This commit adds support for the DSI IP in the MT8189 SoC, which supports a higher link rate of 2.5Gbps. It introduces a max_link_rate_mbps member to handle SoC-specific link rates dynamically instead of using a hardcoded value, and casts calculations to u64 to prevent overflows. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index c5db63b93ff91..3f58683be70fa 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c [ ... ] > @@ -926,7 +928,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) > } > bit_per_pixel = ret; > [Severity: High] This is a pre-existing issue, but does this error path permanently leak the reference count? In mtk_dsi_poweron(), dsi->refcount is incremented at the start of the function. If mipi_dsi_pixel_format_to_bpp() fails, the function returns directly instead of jumping to the err_refcount label: static int mtk_dsi_poweron(struct mtk_dsi *dsi) { ... if (++dsi->refcount != 1) return 0; ret = mipi_dsi_pixel_format_to_bpp(dsi->format); if (ret < 0) { dev_err(dev, "Unknown MIPI DSI format %d\n", dsi->format); return ret; } Would this leave the DSI driver in a state where subsequent calls incorrectly assume the hardware is already powered on? > - dsi->data_rate = DIV_ROUND_UP_ULL(dsi->vm.pixelclock * bit_per_pixel, > + dsi->data_rate = DIV_ROUND_UP_ULL((u64)dsi->vm.pixelclock * > bit_per_pixel, > dsi->lanes); [Severity: High] Does this PHY clock calculation need to account for DSC compression? By using the uncompressed bit_per_pixel here, could this clock the PHY at the uncompressed data rate instead of the compressed DSC rate, potentially over-clocking the PHY and breaking the DSC feature? > > ret = clk_set_rate(dsi->hs_clk, dsi->data_rate); [ ... ] > @@ -1101,13 +1103,21 @@ mtk_dsi_bridge_mode_valid(struct drm_bridge *bridge, > const struct drm_display_mode *mode) > { > struct mtk_dsi *dsi = bridge_to_dsi(bridge); > + const struct mtk_dsi_driver_data *data = dsi->driver_data; > + u64 wanted_link_rate, max_link_rate; > int bpp; > > bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > if (bpp < 0) > return MODE_ERROR; > > - if (mode->clock * bpp / dsi->lanes > 1500000) > + wanted_link_rate = mode->clock; > + wanted_link_rate *= bpp; [Severity: High] Similarly, does the wanted_link_rate calculation fail to account for the DSC bit rate? If bpp remains the uncompressed value (e.g., 24 for RGB888) instead of the compressed rate from dsi->dsc->bits_per_pixel, will this incorrectly reject valid high-resolution DSC modes by returning MODE_CLOCK_HIGH? > + max_link_rate = data->max_link_rate_mbps; > + max_link_rate *= dsi->lanes; > + max_link_rate *= KILO; > + > + if (wanted_link_rate > max_link_rate) > return MODE_CLOCK_HIGH; [ ... ] [Severity: High] This isn't a bug introduced by this patch, but are signal interruptions incorrectly treated as successes during IRQ waits? In mtk_dsi_wait_for_irq_done(), wait_event_interruptible_timeout() can return -ERESTARTSYS if interrupted by a signal. However, both mtk_dsi_host_send_cmd() and mtk_dsi_switch_to_cmd_mode() evaluate this negative error code with a logical NOT: drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_host_send_cmd() { ... if (!mtk_dsi_wait_for_irq_done(dsi, flag, 2000)) return -ETIME; else return 0; } Since -ERESTARTSYS is non-zero, the condition evaluates to false, taking the else branch and returning 0 (success). Will this cause the driver to silently ignore interrupted hardware operations and incorrectly assume state changes have completed? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
